fix: Rework enrol workflow

python/ironic-understack/ironic_understack/inspect_hook_update_baremetal_ports.py

@@ -38,6 +38,7 @@ def __call__(self, task, inventory, plugin_data):
         - local_link_info.switch_id (e.g. "aa:bb:cc:dd:ee:ff")
         - local_link_info.switch_info (e.g. "a1-1-1.ord1.rackspace.net")
         - physical_network (e.g. "a1-1-network")
+        - pxe flag (set only for the connection to primary network switch)
 
         We also add or remove node "traits" based on the inventory data.  We
         control the trait "CUSTOM_STORAGE_SWITCH".
@@ -60,7 +61,13 @@ def __call__(self, task, inventory, plugin_data):
             {"node": node_uuid, "groups": vlan_groups},
         )
 
-        _update_port_attrs(task, ports_by_mac, vlan_groups, node_uuid)
+        pxe_switch_names = _pxe_switch_names(vlan_groups)
+        LOG.debug(
+            "Node=%(node)s pxe_switch_names=%(switches)s",
+            {"node": node_uuid, "switches": sorted(pxe_switch_names)},
+        )
+
+        _update_port_attrs(task, ports_by_mac, vlan_groups, pxe_switch_names, node_uuid)
         _set_node_traits(task, {x for x in vlan_groups.values() if x})
 
 
@@ -89,21 +96,30 @@ def _normalise_switch_name(name: str) -> str:
     return name
 
 
-def _update_port_attrs(task, ports_by_mac, vlan_groups, node_uuid):
+def _update_port_attrs(task, ports_by_mac, vlan_groups, pxe_switch_names, node_uuid):
     for baremetal_port in ironic_ports_for_node(task.context, task.node.id):
         inspected_port = ports_by_mac.get(baremetal_port.address)
         if inspected_port:
             vlan_group = vlan_groups.get(inspected_port.switch_system_name)
+            pxe_enabled = inspected_port.switch_system_name in pxe_switch_names
             LOG.info(
-                "Port=%(uuid)s Node=%(node)s is connected %(lldp)s, %(vlan_group)s",
+                "Port=%(uuid)s Node=%(node)s is connected %(lldp)s, "
+                "%(vlan_group)s pxe_enabled=%(pxe_enabled)s",
                 {
                     "uuid": baremetal_port.uuid,
                     "node": node_uuid,
                     "lldp": inspected_port,
                     "vlan_group": vlan_group,
+                    "pxe_enabled": pxe_enabled,
                 },
             )
-            _set_port_attributes(baremetal_port, node_uuid, inspected_port, vlan_group)
+            _set_port_attributes(
+                baremetal_port,
+                node_uuid,
+                inspected_port,
+                vlan_group,
+                pxe_enabled,
+            )
         else:
             LOG.info(
                 "Port=%(uuid)s Node=%(node)s has no LLDP connection",
@@ -117,6 +133,7 @@ def _set_port_attributes(
     node_uuid: str,
     inspected_port: InspectedPort,
     physical_network: str | None,
+    pxe_enabled: bool,
 ):
     category = None
     if physical_network:
@@ -149,6 +166,23 @@ def _set_port_attributes(
         if port.category != category:
             port.category = category
 
+        if port.pxe_enabled == pxe_enabled:
+            LOG.debug(
+                "Node %s port %s pxe_enabled already set to %s",
+                node_uuid,
+                port.id,
+                pxe_enabled,
+            )
+        else:
+            LOG.debug(
+                "Updating node %s port %s pxe_enabled from %s to %s",
+                node_uuid,
+                port.id,
+                port.pxe_enabled,
+                pxe_enabled,
+            )
+            port.pxe_enabled = pxe_enabled
+
         port.save()
     except exception.IronicException as e:
         LOG.warning(
@@ -162,6 +196,7 @@ def _clear_port_attributes(port: Any, node_uuid: str):
         port.local_link_connection = {}
         port.physical_network = None
         port.category = None
+        port.pxe_enabled = False
         port.save()
     except exception.IronicException as e:
         LOG.warning(
@@ -170,6 +205,28 @@ def _clear_port_attributes(port: Any, node_uuid: str):
         )
 
 
+def _pxe_switch_names(vlan_groups: dict[str, str | None]) -> set[str]:
+    network_groups: dict[str, list[str]] = {}
+
+    for switch_name, vlan_group in vlan_groups.items():
+        if vlan_group and vlan_group.endswith("-network"):
+            network_groups.setdefault(vlan_group, []).append(switch_name)
+
+    return {
+        _natural_sorted_switch_names(switch_names)[0]
+        for switch_names in network_groups.values()
+        if switch_names
+    }
+
+
+def _natural_sorted_switch_names(switch_names: list[str]) -> list[str]:
+    return sorted(switch_names, key=_natural_sort_key)
+
+
+def _natural_sort_key(value: str) -> list[int | str]:
+    return [int(part) if part.isdigit() else part for part in re.split(r"(\d+)", value)]
+
+
 def _set_node_traits(task, vlan_groups: set[str]):
     """Add or remove traits to the node.
 

python/ironic-understack/ironic_understack/port_bios_name_hook.py

@@ -13,13 +13,44 @@
 class PortBiosNameHook(base.InspectionHook):
     """Set bios_name, pxe_enabled, local_link_connection and physical_network.
 
-    Populates extra.bios_name and port name from inspection inventory, then
-    determines PXE-enabled ports from node.extra["enrolled_pxe_ports"]
-    (populated during enrolment). If that data is unavailable, all
-    NIC.Integrated.* and NIC.Slot.* ports are treated as PXE-enabled.
-
-    PXE ports get pxe_enabled=True plus placeholder physical_network and
-    local_link_connection values that neutron requires.
+    Runs after the "ports" hook has created a baremetal port for each NIC in the
+    box.
+
+    We set the `name` and `extra.bios_name` for each port using the BIOS names
+    in the inventory data that was collected by redfish inspection.
+
+    When the native Ironic "ports" hook runs, it creates any missing ports but
+    lamentably it sets the "pxe" flag on every port it creates.
+
+    We clear that flag, because it causes neutron to do extra work and assign
+    extra IP addresses.  We know that it is a newly-created port and not one
+    that we specifically set to PXE, because newly created ports don't have a
+    physical_network, whereas we always set that property at the same time we
+    set the PXE flag.
+
+    If this node has no PXE ports at all, then we assume that this box has just
+    been enrolled and has not yet undergone a successful agent inspection.
+    Agent inspection will be the next step, and therefore we need to set up the
+    bare minimum that is required by Ironic/Neutron to prepare to boot the IPA
+    image.
+
+    Even though PXE is not in use, the provisioning network is still required,
+    because that is how the agent communicates with Ironic.  Neutron wants to
+    make a port in the provisioning network, and it will error out unless it can
+    find a suitable baremetal port.
+
+    We choose one arbitrary baremetal port and we populate its attributes with
+    dummy data to enable Ironic/Neutron to do an IPA boot:
+
+      - pxe_enabled=True
+      - physical_network="enrol" (placeholder value)
+      - local_link_connection set to dummy data as placeholder
+
+    Note that this only works because neutron is not completely controlling the
+    DHCP server, so it doesn't matter if we choose the wrong port.  If this
+    situation changes then we would need to configure all possible NICs with
+    placeholder data, which would result in a configuration for every single
+    NIC.
     """
 
     dependencies: ClassVar[list[str]] = ["ports"]
@@ -34,54 +65,48 @@ def __call__(self, task, inventory, plugin_data):
             i["mac_address"].upper(): i["name"] for i in inspected_interfaces
         }
 
-        pxe_nics = _enrolled_pxe_nics(task)
+        ports = list(ironic_ports_for_node(task.context, task.node.id))
+        if not ports:
+            LOG.error("No baremetal ports in Ironic for node %s", task.node.uuid)
+            return
 
-        for baremetal_port in ironic_ports_for_node(task.context, task.node.id):
+        for baremetal_port in ports:
             mac = baremetal_port.address.upper()
             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)
+            _clear_unwanted_pxe(baremetal_port)
 
-            is_pxe = bios_name is not None and any(
-                pxe_nic.startswith(bios_name) or bios_name.startswith(pxe_nic)
-                for pxe_nic in pxe_nics
-            )
+        if not any(port.pxe_enabled for port in ports):
+            _set_port_pxe_placeholder(ports[0])
 
-            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 _enrolled_pxe_nics(task) -> list[str]:
-    """Read enrolled PXE NIC names from node.extra, or use broad prefixes."""
-    enrolled_pxe_nics = task.node.extra.get("enrolled_pxe_ports")
-    if enrolled_pxe_nics:
-        LOG.info(
-            "Set node %s pxe flag on interfaces from extra.enrolled_pxe_ports %s",
-            task.node.uuid,
-            enrolled_pxe_nics,
-        )
-        return enrolled_pxe_nics
-    else:
-        LOG.warning(
-            "Node %s extra.enrolled_pxe_ports is missing, "
-            "setting pxe flag on all interfaces starting %s.",
-            task.node.uuid,
-            PXE_BIOS_NAME_PREFIXES,
-        )
-        return PXE_BIOS_NAME_PREFIXES
+
+def _set_port_pxe_placeholder(baremetal_port):
+    LOG.info(
+        "Populating port %s with placeholder PXE data to support enrol.",
+        baremetal_port.address,
+    )
+    baremetal_port.pxe_enabled = True
+    baremetal_port.physical_network = "enrol"
+    baremetal_port.local_link_connection = {
+        "port_id": "None",
+        "switch_id": "00:00:00:00:00:00",
+        "switch_info": "None",
+    }
+    baremetal_port.save()
+
+
+def _clear_unwanted_pxe(baremetal_port):
+    if baremetal_port.physical_network:
+        return
+
+    if not baremetal_port.pxe_enabled:
+        return
+
+    LOG.info("Clearing port %s PXE flag.", baremetal_port.address)
+    baremetal_port.pxe_enabled = False
+    baremetal_port.save()
 
 
 def _set_port_extra(baremetal_port, mac, required_bios_name):
@@ -114,25 +139,3 @@ def _set_port_name(baremetal_port, mac, required_bios_name, node_name):
             )
             baremetal_port.name = required_port_name
             baremetal_port.save()
-
-
-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):
-    if not baremetal_port.local_link_connection:
-        baremetal_port.local_link_connection = {
-            "port_id": "None",
-            "switch_id": "00:00:00:00:00:00",
-            "switch_info": "None",
-        }
-        LOG.info(
-            "Port %s changing local_link_connection from None to %s",
-            mac,
-            baremetal_port.local_link_connection,
-        )
-        baremetal_port.save()

python/ironic-understack/ironic_understack/tests/test_inspect_hook_update_baremetal_ports.py

@@ -78,6 +78,7 @@ def test_with_valid_network_port(mocker, caplog):
         local_link_connection={},
         physical_network="original_value",
         category=None,
+        pxe_enabled=False,
     )
 
     mocker.patch(
@@ -102,6 +103,7 @@ def test_with_valid_network_port(mocker, caplog):
     }
     assert mock_port.physical_network == "f20-3-network"
     assert mock_port.category == "network"
+    assert mock_port.pxe_enabled is True
     mock_port.save.assert_called()
 
 
@@ -120,6 +122,7 @@ def test_with_valid_storage_port(mocker, caplog):
         local_link_connection={},
         physical_network="original_value",
         category=None,
+        pxe_enabled=True,
     )
 
     mocker.patch(
@@ -144,6 +147,51 @@ def test_with_valid_storage_port(mocker, caplog):
     }
     assert mock_port.physical_network is None
     assert mock_port.category == "storage"
+    assert mock_port.pxe_enabled is False
+    mock_port.save.assert_called()
+
+
+def test_secondary_network_port_has_pxe_disabled(mocker, caplog):
+    caplog.set_level(logging.DEBUG)
+
+    node_uuid = uuidutils.generate_uuid()
+    mock_traits = mocker.Mock()
+    mock_context = mocker.Mock()
+    mock_node = mocker.Mock(id=1234, traits=mock_traits)
+    mock_task = mocker.Mock(node=mock_node, context=mock_context)
+    mock_port = mocker.Mock(
+        uuid=uuidutils.generate_uuid(),
+        node_id=node_uuid,
+        address="22:22:22:22:22:22",
+        local_link_connection={},
+        physical_network="original_value",
+        category=None,
+        pxe_enabled=True,
+    )
+
+    mocker.patch(
+        "ironic_understack.inspect_hook_update_baremetal_ports.ironic_ports_for_node",
+        return_value=[mock_port],
+    )
+    mocker.patch(
+        "ironic_understack.inspect_hook_update_baremetal_ports.CONF.ironic_understack.switch_name_vlan_group_mapping",
+        MAPPING,
+    )
+    mocker.patch(
+        "ironic_understack.inspect_hook_update_baremetal_ports.objects.TraitList.create"
+    )
+    mock_traits.get_trait_names.return_value = ["CUSTOM_BMC_SWITCH", "bar"]
+
+    InspectHookUpdateBaremetalPorts().__call__(mock_task, _INVENTORY, _PLUGIN_DATA)
+
+    assert mock_port.local_link_connection == {
+        "port_id": "Ethernet1/18",
+        "switch_id": "88:5a:92:ec:54:59",
+        "switch_info": "f20-3-2.iad3.rackspace.net",
+    }
+    assert mock_port.physical_network == "f20-3-network"
+    assert mock_port.category == "network"
+    assert mock_port.pxe_enabled is False
     mock_port.save.assert_called()