Merge pull request #1347 from rackerlabs/ironic-inspection-hook

feat(ironic): Add update_baremetal_port ironic inspection hook

Author: stevekeay

components/ironic/values.yaml

@@ -88,11 +88,12 @@ conf:
       loader_file_paths: "snponly.efi:/usr/lib/ipxe/snponly.efi"
     inspector:
       extra_kernel_params: ipa-collect-lldp=1
-      # Agent inspection hooks - ports hook removed to prevent port manipulation during agent inspection
-      # Default hooks include: ramdisk-error,validate-interfaces,ports,architecture
-      # We override to exclude 'ports' from the default hooks
-      default_hooks: "ramdisk-error,validate-interfaces,architecture"
-      hooks: "$default_hooks,pci-devices,parse-lldp,local-link-connection,resource-class"
+      # Agent inspection hooks run after inspecting in-band using the IPA image:
+      hooks: "ramdisk-error,validate-interfaces,architecture,pci-devices,validate-interfaces,parse-lldp,resource-class,update-baremetal-port"
+    redfish:
+      # Redfish inspection hooks run after inspecting out-of-band using the BMC:
+      inspection_hooks: "validate-interfaces,ports,port-bios-name,architecture,pci-devices"
+      add_ports: "all"
     # enable sensors and metrics for redfish metrics - https://docs.openstack.org/ironic/latest/admin/drivers/redfish/metrics.html
     sensor_data:
       send_sensor_data: true

python/ironic-understack/ironic_understack/conf.py

@@ -10,7 +10,22 @@ def setup_conf():
             "device_types_dir",
             help="directory storing Device Type description YAML files",
             default="/var/lib/understack/device-types",
-        )
+        ),
+        cfg.DictOpt(
+            "switch_name_vlan_group_mapping",
+            help="Dictionary of switch hostname suffix to vlan group name",
+            default={
+                "1": "network",
+                "2": "network",
+                "3": "network",
+                "4": "network",
+                "1f": "storage",
+                "2f": "storage",
+                "3f": "storage-appliance",
+                "4f": "storage-appliance",
+                "1d": "bmc",
+            },
+        ),
     ]
     cfg.CONF.register_group(grp)
     cfg.CONF.register_opts(opts, group=grp)

python/ironic-understack/ironic_understack/inspected_port.py

@@ -0,0 +1,56 @@
+from dataclasses import dataclass
+
+
+@dataclass
+class InspectedPort:
+    """Represents a parsed entry from Ironic inspection (inventory) data."""
+
+    mac_address: str
+    name: str
+    switch_system_name: str
+    switch_port_id: str
+    switch_chassis_id: str
+
+    @property
+    def local_link_connection(self) -> dict:
+        return {
+            "port_id": self.switch_port_id,
+            "switch_id": self.switch_chassis_id,
+            "switch_info": self.switch_system_name,
+        }
+
+    @property
+    def parsed_name(self) -> dict[str, str]:
+        parts = self.switch_system_name.split(".", maxsplit=1)
+        if len(parts) != 2:
+            raise ValueError(
+                "Failed to parse switch hostname - expecting name.dc in %s", self
+            )
+        switch_name, data_center_name = parts
+
+        parts = switch_name.rsplit("-", maxsplit=1)
+        if len(parts) != 2:
+            raise ValueError(
+                f"Unknown switch name format: {switch_name} - this hook requires "
+                f"that switch names follow the convention <cabinet-name>-<suffix>"
+            )
+
+        rack_name, switch_suffix = parts
+
+        return {
+            "rack_name": rack_name,
+            "switch_suffix": switch_suffix,
+            "data_center_name": data_center_name,
+        }
+
+    @property
+    def rack_name(self) -> str:
+        return self.parsed_name["rack_name"]
+
+    @property
+    def switch_suffix(self) -> str:
+        return self.parsed_name["switch_suffix"]
+
+    @property
+    def data_center_name(self) -> str:
+        return self.parsed_name["data_center_name"]

python/ironic-understack/ironic_understack/ironic_wrapper.py

@@ -0,0 +1,5 @@
+import ironic.objects
+
+
+def ironic_ports_for_node(context, node_id: str) -> list:
+    return ironic.objects.Port.list_by_node_id(context, node_id)

python/ironic-understack/ironic_understack/port_bios_name_hook.py

@@ -0,0 +1,62 @@
+from ironic.drivers.modules.inspector.hooks import base
+from oslo_log import log as logging
+
+from ironic_understack.ironic_wrapper import ironic_ports_for_node
+
+LOG = logging.getLogger(__name__)
+
+
+class PortBiosNameHook(base.InspectionHook):
+    """Set port.extra.bios_name and pxe_enabled fields from redfish data."""
+
+    # "ports" creates baremetal ports for each physical NIC, be sure to run this
+    # first because we will only be updating ports that already exist:
+    dependencies = ["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)
+            return
+
+        interface_names = {
+            i["mac_address"].upper(): i["name"] for i in inspected_interfaces
+        }
+
+        pxe_interface = _pxe_interface_name(inspected_interfaces)
+
+        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)
+            extra = baremetal_port.extra
+            current_bios_name = extra.get("bios_name")
+
+            if current_bios_name != required_bios_name:
+                LOG.info(
+                    "Port %(mac)s updating bios_name from %(old)s to %(new)s",
+                    {"mac": mac, "old": current_bios_name, "new": required_bios_name},
+                )
+
+                if required_bios_name:
+                    extra["bios_name"] = required_bios_name
+                else:
+                    extra.pop("bios_name", None)
+
+                baremetal_port.extra = extra
+                baremetal_port.save()
+
+            required_pxe = required_bios_name == pxe_interface
+            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()
+
+
+def _pxe_interface_name(inspected_interfaces: list[dict]) -> str:
+    """Use a heuristic to determine our default interface for PXE."""
+    names = sorted(i["name"] for i in inspected_interfaces)
+    for prefix in ["NIC.Integrated", "NIC.Slot"]:
+        for name in names:
+            if name.startswith(prefix):
+                return name

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

@@ -0,0 +1,63 @@
+import logging
+
+from oslo_utils import uuidutils
+
+from ironic_understack.port_bios_name_hook import PortBiosNameHook
+
+_INVENTORY = {
+    "memory": {"physical_mb": 98304},
+    "interfaces": [
+        {"mac_address": "11:11:11:11:11:11", "name": "NIC.Integrated.1-1"},
+        {"mac_address": "22:22:22:22:22:22", "name": "NIC.Integrated.1-2"},
+    ],
+}
+
+
+def test_adding_bios_name(mocker, caplog):
+    caplog.set_level(logging.DEBUG)
+
+    node_uuid = uuidutils.generate_uuid()
+    mock_context = mocker.Mock()
+    mock_node = mocker.Mock(id=1234)
+    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={},
+    )
+
+    mocker.patch(
+        "ironic_understack.port_bios_name_hook.ironic_ports_for_node",
+        return_value=[mock_port],
+    )
+
+    PortBiosNameHook().__call__(mock_task, _INVENTORY, {})
+
+    assert mock_port.extra == {"bios_name": "NIC.Integrated.1-1"}
+    mock_port.save.assert_called()
+
+
+def test_removing_bios_name(mocker, caplog):
+    caplog.set_level(logging.DEBUG)
+
+    node_uuid = uuidutils.generate_uuid()
+    mock_context = mocker.Mock()
+    mock_node = mocker.Mock(id=1234)
+    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",
+        extra={"bios_name": "old_name_no_longer_valid"},
+    )
+
+    mocker.patch(
+        "ironic_understack.port_bios_name_hook.ironic_ports_for_node",
+        return_value=[mock_port],
+    )
+
+    PortBiosNameHook().__call__(mock_task, _INVENTORY, {})
+
+    assert "bios_name" not in mock_port.extra
+    mock_port.save.assert_called()

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

@@ -0,0 +1,108 @@
+import logging
+
+import ironic.objects
+from oslo_utils import uuidutils
+
+import ironic_understack
+from ironic_understack.update_baremetal_port import UpdateBaremetalPortsHook
+
+# load some metaprgramming normally taken care of during Ironic initialization:
+ironic.objects.register_all()
+
+_INVENTORY = {}
+_PLUGIN_DATA = {
+    "all_interfaces": {
+        "ex1": {
+            "name": "ex1",
+            "mac_address": "11:11:11:11:11:11",
+        },
+        "ex2": {
+            "name": "ex2",
+            "mac_address": "22:22:22:22:22:22",
+        },
+        "ex3": {
+            "name": "ex3",
+            "mac_address": "33:33:33:33:33:33",
+        },
+        "ex4": {
+            "name": "ex4",
+            "mac_address": "44:44:44:44:44:44",
+        },
+    },
+    "parsed_lldp": {
+        "ex1": {
+            "switch_chassis_id": "88:5a:92:ec:54:59",
+            "switch_port_id": "Ethernet1/18",
+            "switch_system_name": "f20-3-1.iad3",
+        },
+        "ex2": {
+            "switch_chassis_id": "88:5a:92:ec:54:59",
+            "switch_port_id": "Ethernet1/18",
+            "switch_system_name": "f20-3-2.iad3",
+        },
+        "ex3": {
+            "switch_chassis_id": "88:5a:92:ec:54:59",
+            "switch_port_id": "Ethernet1/18",
+            "switch_system_name": "f20-3-1f.iad3",
+        },
+        "ex4": {
+            "switch_chassis_id": "88:5a:92:ec:54:59",
+            "switch_port_id": "Ethernet1/18",
+            "switch_system_name": "f20-3-2f.iad3",
+        },
+    },
+}
+
+MAPPING = {
+    "1": "network",
+    "2": "network",
+    "1f": "storage",
+    "2f": "storage",
+    "-1d": "bmc",
+}
+
+
+def test_with_valid_data(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="11:11:11:11:11:11",
+        local_link_connection={},
+        physical_network="original_value",
+    )
+
+    mocker.patch(
+        "ironic_understack.update_baremetal_port.ironic_ports_for_node",
+        return_value=[mock_port],
+    )
+    mocker.patch(
+        "ironic_understack.update_baremetal_port.CONF.ironic_understack.switch_name_vlan_group_mapping",
+        MAPPING,
+    )
+    mocker.patch("ironic_understack.update_baremetal_port.objects.TraitList.create")
+
+    mock_traits.get_trait_names.return_value = ["CUSTOM_BMC_SWITCH", "bar"]
+
+    UpdateBaremetalPortsHook().__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-1.iad3",
+    }
+    assert mock_port.physical_network == "f20-3-network"
+    mock_port.save.assert_called()
+
+    mock_traits.get_trait_names.assert_called_once()
+    mock_traits.destroy.assert_called_once_with("CUSTOM_BMC_SWITCH")
+    ironic_understack.update_baremetal_port.objects.TraitList.create.assert_called_once_with(
+        mock_context, 1234, ["CUSTOM_NETWORK_SWITCH", "CUSTOM_STORAGE_SWITCH"]
+    )
+    mock_node.save.assert_called_once()