diff --git a/python/ironic-understack/ironic_understack/port_bios_name_hook.py b/python/ironic-understack/ironic_understack/port_bios_name_hook.py index 631487884..d631befbe 100644 --- a/python/ironic-understack/ironic_understack/port_bios_name_hook.py +++ b/python/ironic-understack/ironic_understack/port_bios_name_hook.py @@ -1,6 +1,8 @@ from typing import ClassVar +from ironic.common import exception from ironic.drivers.modules.inspector.hooks import base +from ironic.objects.bios import BIOSSetting from oslo_log import log as logging from ironic_understack.ironic_wrapper import ironic_ports_for_node @@ -8,26 +10,24 @@ LOG = logging.getLogger(__name__) PXE_BIOS_NAME_PREFIXES = ["NIC.Integrated", "NIC.Slot"] +BIOS_SETTING_NAME = "HttpDev1Interface" class PortBiosNameHook(base.InspectionHook): - """Set name, extra.bios_name and pxe_enabled fields from redfish data. + """Set bios_name, pxe_enabled, local_link_connection and physical_network. - In addition, this hook ensures that the PXE port has sufficient data to - allow neutron to boot a new node for inspection. If the physical_network - and local_link_connections are not populated, we fill them with placeholder - data. + Populates extra.bios_name and port name from inspection inventory, then + determines the PXE port from the BIOS HttpDev1Interface setting (populated + during enrolment). Falls back to a naming-convention heuristic if the + BIOS setting is unavailable. - This is necessary because neutron throws errors if the port doesn't have - those fields filled in. + The PXE port gets pxe_enabled=True plus placeholder physical_network and + local_link_connection values that neutron requires. """ - # "ports" hook creates baremetal ports for each physical NIC, be sure to run - # this first because we will only be updating ports that already exist: dependencies: ClassVar[list[str]] = ["ports"] def __call__(self, task, inventory, plugin_data): - """Populate the baremetal_port.extra.bios_name attribute.""" inspected_interfaces = inventory.get("interfaces") if not inspected_interfaces: LOG.error("No interfaces in inventory for node %s", task.node.uuid) @@ -37,25 +37,80 @@ def __call__(self, task, inventory, plugin_data): i["mac_address"].upper(): i["name"] for i in inspected_interfaces } - pxe_interface = _pxe_interface_name(inspected_interfaces, task.node.uuid) + pxe_nic = _bios_pxe_nic(task) for baremetal_port in ironic_ports_for_node(task.context, task.node.id): mac = baremetal_port.address.upper() - required_bios_name = interface_names.get(mac) - required_pxe = required_bios_name == pxe_interface - - _set_port_extra(baremetal_port, mac, required_bios_name) - _set_port_pxe_enabled(baremetal_port, mac, required_pxe) - _set_port_name(baremetal_port, mac, required_bios_name, task.node.name) - _set_port_physical_network(baremetal_port, mac, required_pxe) - _set_port_local_link_connection(baremetal_port, mac, required_pxe) - + bios_name = interface_names.get(mac) + + _set_port_extra(baremetal_port, mac, bios_name) + _set_port_name(baremetal_port, mac, bios_name, task.node.name) + + if pxe_nic: + is_pxe = bios_name is not None and ( + pxe_nic.startswith(bios_name) or bios_name.startswith(pxe_nic) + ) + else: + # Fallback: heuristic based on naming convention + is_pxe = bios_name == _pxe_interface_name( + inspected_interfaces, task.node.uuid + ) + + if baremetal_port.pxe_enabled != is_pxe: + LOG.info( + "Port %s (%s) pxe_enabled %s -> %s", + mac, + bios_name, + baremetal_port.pxe_enabled, + is_pxe, + ) + baremetal_port.pxe_enabled = is_pxe + baremetal_port.save() + + if is_pxe: + _set_port_physical_network(baremetal_port, mac) + _set_port_local_link_connection(baremetal_port, mac) + + +def _bios_pxe_nic(task): + """Read the BIOS PXE NIC FQDD, or return None if unavailable.""" + try: + task.driver.bios.cache_bios_settings(task) + except Exception: + LOG.warning( + "Cannot cache BIOS settings for node %s, " + "falling back to naming heuristic for PXE port.", + task.node.uuid, + ) + return None + + try: + setting = BIOSSetting.get(task.context, task.node.id, BIOS_SETTING_NAME) + except exception.BIOSSettingNotFound: + LOG.warning( + "BIOS setting %s not found for node %s, " + "falling back to naming heuristic for PXE port.", + BIOS_SETTING_NAME, + task.node.uuid, + ) + return None + + if not setting.value: + LOG.warning( + "BIOS setting %s is empty for node %s, " + "falling back to naming heuristic for PXE port.", + BIOS_SETTING_NAME, + task.node.uuid, + ) + return None -def _set_port_pxe_enabled(baremetal_port, mac, required_pxe): - if baremetal_port.pxe_enabled != required_pxe: - LOG.info("Port %s changed pxe_enabled to %s", mac, required_pxe) - baremetal_port.pxe_enabled = required_pxe - baremetal_port.save() + LOG.info( + "Node %s BIOS %s = %s", + task.node.uuid, + BIOS_SETTING_NAME, + setting.value, + ) + return setting.value def _set_port_extra(baremetal_port, mac, required_bios_name): @@ -90,15 +145,15 @@ def _set_port_name(baremetal_port, mac, required_bios_name, node_name): baremetal_port.save() -def _set_port_physical_network(baremetal_port, mac, required_pxe): - if required_pxe and not baremetal_port.physical_network: +def _set_port_physical_network(baremetal_port, mac): + if not baremetal_port.physical_network: LOG.info("Port %s changing physical_network from None to 'enrol'", mac) baremetal_port.physical_network = "enrol" baremetal_port.save() -def _set_port_local_link_connection(baremetal_port, mac, required_pxe): - if required_pxe and not baremetal_port.local_link_connection: +def _set_port_local_link_connection(baremetal_port, mac): + if not baremetal_port.local_link_connection: baremetal_port.local_link_connection = { "port_id": "None", "switch_id": "00:00:00:00:00:00", diff --git a/python/ironic-understack/ironic_understack/tests/test_port_bios_name_hook.py b/python/ironic-understack/ironic_understack/tests/test_port_bios_name_hook.py index b86887f20..edd6b772f 100644 --- a/python/ironic-understack/ironic_understack/tests/test_port_bios_name_hook.py +++ b/python/ironic-understack/ironic_understack/tests/test_port_bios_name_hook.py @@ -1,5 +1,6 @@ import logging +from ironic.common import exception from oslo_utils import uuidutils from ironic_understack.port_bios_name_hook import PortBiosNameHook @@ -13,107 +14,179 @@ } -def test_adding_bios_name(mocker, caplog): +def _make_task(mocker, bios_value=None, bios_error=None, cache_error=None): + mock_node = mocker.Mock(id=1234, uuid=uuidutils.generate_uuid()) + mock_node.name = "Dell-CR1MB0" + task = mocker.Mock(node=mock_node, context=mocker.Mock()) + + if cache_error: + task.driver.bios.cache_bios_settings.side_effect = cache_error + elif bios_error: + mocker.patch( + "ironic_understack.port_bios_name_hook.BIOSSetting.get", + side_effect=bios_error, + ) + elif bios_value is not None: + setting = mocker.Mock(value=bios_value) + mocker.patch( + "ironic_understack.port_bios_name_hook.BIOSSetting.get", + return_value=setting, + ) + else: + setting = mocker.Mock(value=None) + mocker.patch( + "ironic_understack.port_bios_name_hook.BIOSSetting.get", + return_value=setting, + ) + + return task + + +def _make_port(mocker, mac, **kwargs): + defaults = { + "uuid": uuidutils.generate_uuid(), + "address": mac, + "extra": {}, + "pxe_enabled": False, + "physical_network": None, + "local_link_connection": {}, + } + defaults.update(kwargs) + port = mocker.Mock(**defaults) + port.name = kwargs.get("name", "some-arbitrary-name") + return port + + +def test_pxe_from_bios_setting(mocker, caplog): + """BIOS HttpDev1Interface determines pxe_enabled.""" caplog.set_level(logging.DEBUG) + task = _make_task(mocker, bios_value="NIC.Integrated.1-1-1") - node_uuid = uuidutils.generate_uuid() - mock_context = mocker.Mock() - mock_node = mocker.Mock(id=1234) - mock_node.name = "Dell-CR1MB0" - mock_task = mocker.Mock(node=mock_node, context=mock_context) - mock_port = mocker.Mock( - uuid=uuidutils.generate_uuid(), - node_id=node_uuid, - address="11:11:11:11:11:11", - extra={}, - local_link_connection={}, - physical_network=None, + port1 = _make_port(mocker, "11:11:11:11:11:11") + port2 = _make_port(mocker, "22:22:22:22:22:22") + + mocker.patch( + "ironic_understack.port_bios_name_hook.ironic_ports_for_node", + return_value=[port1, port2], ) - mock_port.name = "some-arbitrary-name" + + PortBiosNameHook().__call__(task, _INVENTORY, {}) + + assert port1.pxe_enabled is True + assert port2.pxe_enabled is False + assert port1.extra == {"bios_name": "NIC.Integrated.1-1"} + assert port1.name == "Dell-CR1MB0:NIC.Integrated.1-1" + assert port1.physical_network == "enrol" + + +def test_pxe_fallback_when_cache_fails(mocker, caplog): + """Falls back to heuristic when BIOS cache fails.""" + caplog.set_level(logging.DEBUG) + task = _make_task(mocker, cache_error=Exception("no BIOS")) + + port1 = _make_port(mocker, "11:11:11:11:11:11") + port2 = _make_port(mocker, "22:22:22:22:22:22") mocker.patch( "ironic_understack.port_bios_name_hook.ironic_ports_for_node", - return_value=[mock_port], + return_value=[port1, port2], ) - PortBiosNameHook().__call__(mock_task, _INVENTORY, {}) + PortBiosNameHook().__call__(task, _INVENTORY, {}) + + # Heuristic picks NIC.Integrated.1-1 (first sorted match) + assert port1.pxe_enabled is True + assert port2.pxe_enabled is False + assert "falling back to naming heuristic" in caplog.text - assert mock_port.extra == {"bios_name": "NIC.Integrated.1-1"} - assert mock_port.name == "Dell-CR1MB0:NIC.Integrated.1-1" - assert mock_port.pxe_enabled - assert mock_port.physical_network == "enrol" - assert mock_port.local_link_connection == { - "port_id": "None", - "switch_id": "00:00:00:00:00:00", - "switch_info": "None", - } - mock_port.save.assert_called() + +def test_pxe_fallback_when_setting_missing(mocker, caplog): + """Falls back to heuristic when BIOS setting not found.""" + caplog.set_level(logging.DEBUG) + task = _make_task( + mocker, + bios_error=exception.BIOSSettingNotFound(node="fake", name="HttpDev1Interface"), + ) + + port1 = _make_port(mocker, "11:11:11:11:11:11") + port2 = _make_port(mocker, "22:22:22:22:22:22") + + mocker.patch( + "ironic_understack.port_bios_name_hook.ironic_ports_for_node", + return_value=[port1, port2], + ) + + PortBiosNameHook().__call__(task, _INVENTORY, {}) + + assert port1.pxe_enabled is True + assert port2.pxe_enabled is False + assert "falling back to naming heuristic" in caplog.text def test_retaining_physical_network(mocker, caplog): + """Existing physical_network and local_link_connection are preserved.""" caplog.set_level(logging.DEBUG) + task = _make_task(mocker, bios_value="NIC.Integrated.1-1-1") - node_uuid = uuidutils.generate_uuid() - mock_context = mocker.Mock() - mock_node = mocker.Mock(id=1234) - mock_node.name = "Dell-CR1MB0" - mock_task = mocker.Mock(node=mock_node, context=mock_context) - mock_port = mocker.Mock( - uuid=uuidutils.generate_uuid(), - node_id=node_uuid, - address="11:11:11:11:11:11", - extra={}, + port = _make_port( + mocker, + "11:11:11:11:11:11", + physical_network="previous_value", local_link_connection={ "port_id": "Ethernet1/19", "switch_id": "00:00:00:00:00:00", "switch_info": "a1-2-3", }, - physical_network="previous_value", ) - mock_port.name = "some-arbitrary-name" mocker.patch( "ironic_understack.port_bios_name_hook.ironic_ports_for_node", - return_value=[mock_port], + return_value=[port], ) - PortBiosNameHook().__call__(mock_task, _INVENTORY, {}) + PortBiosNameHook().__call__(task, _INVENTORY, {}) + + assert port.physical_network == "previous_value" + assert port.local_link_connection["port_id"] == "Ethernet1/19" - assert mock_port.extra == {"bios_name": "NIC.Integrated.1-1"} - assert mock_port.name == "Dell-CR1MB0:NIC.Integrated.1-1" - assert mock_port.pxe_enabled - assert mock_port.physical_network == "previous_value" - assert mock_port.local_link_connection == { - "port_id": "Ethernet1/19", - "switch_id": "00:00:00:00:00:00", - "switch_info": "a1-2-3", - } - mock_port.save.assert_called() + +def test_clears_pxe_on_previously_enabled_port(mocker, caplog): + """Port that was pxe_enabled but no longer matches gets cleared.""" + caplog.set_level(logging.DEBUG) + task = _make_task(mocker, bios_value="NIC.Integrated.1-2-1") + + port1 = _make_port(mocker, "11:11:11:11:11:11", pxe_enabled=True) + port2 = _make_port(mocker, "22:22:22:22:22:22") + + mocker.patch( + "ironic_understack.port_bios_name_hook.ironic_ports_for_node", + return_value=[port1, port2], + ) + + PortBiosNameHook().__call__(task, _INVENTORY, {}) + + assert port1.pxe_enabled is False + assert port2.pxe_enabled is True def test_removing_bios_name(mocker, caplog): + """Port with unknown MAC gets bios_name removed.""" caplog.set_level(logging.DEBUG) + task = _make_task(mocker, bios_value="NIC.Integrated.1-1-1") - node_uuid = uuidutils.generate_uuid() - mock_context = mocker.Mock() - mock_node = mocker.Mock(id=1234) - mock_node.name = "Dell-CR1MB0" - mock_task = mocker.Mock(node=mock_node, context=mock_context) - mock_port = mocker.Mock( - uuid=uuidutils.generate_uuid(), - node_id=node_uuid, - address="33:33:33:33:33:33", + port = _make_port( + mocker, + "33:33:33:33:33:33", extra={"bios_name": "old_name_no_longer_valid"}, + name="original-name", ) - mock_port.name = "original-name" mocker.patch( "ironic_understack.port_bios_name_hook.ironic_ports_for_node", - return_value=[mock_port], + return_value=[port], ) - PortBiosNameHook().__call__(mock_task, _INVENTORY, {}) + PortBiosNameHook().__call__(task, _INVENTORY, {}) - assert mock_port.name == "original-name" - assert "bios_name" not in mock_port.extra - mock_port.save.assert_called() + assert port.name == "original-name" + assert "bios_name" not in port.extra