From ab7bf21d7c49fa9cfad730265f868206cf0db4e4 Mon Sep 17 00:00:00 2001 From: Steve Keay Date: Mon, 23 Mar 2026 14:40:05 +0000 Subject: [PATCH] Set pxe_enabled from BIOS setting instead of naming heuristic As part of the enrolment process we populate the BIOS setting for which NIC to use for PXE boot (HttpDev1Interface). This is likely to be the correct interface, so stop using the naming-convention heuristic in PortBiosNameHook to choose the PXE port and instead honour the setting that we pull from the BIOS. This now uses Ironic's built-in bios interface (task.driver.bios.cache_bios_settings) to populate the BIOS settings DB, then reads HttpDev1Interface via BIOSSetting.get() to determine which NIC to mark as the PXE port. The prefix-matching logic handles the case where the BIOS FQDD is more specific than the EthernetInterface identity (e.g. NIC.Integrated.1-1-1 vs NIC.Integrated.1-1). --- .../ironic_understack/port_bios_name_hook.py | 113 +++++++--- .../tests/test_port_bios_name_hook.py | 203 ++++++++++++------ 2 files changed, 222 insertions(+), 94 deletions(-) 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