From c496f5e1d41ccb1487c3410564d69b5e77ebafce Mon Sep 17 00:00:00 2001 From: Steve Keay Date: Thu, 29 Oct 2026 18:05:06 +0000 Subject: [PATCH] Fix configuration handling for pci device types The original attempted to pass a dictionary through the oslo configuration, but in practice this gets mangled on its journey through helm -> ironic.conf -> olso so we re-implement as a list of strings. --- components/ironic/README.md | 18 +++ .../ironic_understack/conf/__init__.py | 8 +- .../inspect_hook_pci_device_capabilities.py | 52 +++++++-- ...st_inspect_hook_pci_device_capabilities.py | 109 +++++++++++------- 4 files changed, 133 insertions(+), 54 deletions(-) diff --git a/components/ironic/README.md b/components/ironic/README.md index d91262c42..0f4cca106 100644 --- a/components/ironic/README.md +++ b/components/ironic/README.md @@ -55,6 +55,24 @@ helm --namespace openstack \ At this point Ironic will go through some initialization and start up. +## PCI device capabilities + +The optional `pci-device-capabilities` inspection hook reads a list of bounded +dictionaries from the `[ironic_understack]` section of `ironic.conf`. In the +OpenStack-Helm values this is configured as: + +```yaml +conf: + ironic: + ironic_understack: + pci_device_capabilities: + - "{vendor_id:0x1002,product_id:0x75a8,name:gpu_amd_mi350p}" +``` + +OpenStack-Helm joins the list into one `pci_device_capabilities` value in +`ironic.conf`, and oslo.config parses each bounded dictionary as a separate +device. + ## Validating Ironic You can run an OpenStack client in the cluster to validate it is running correctly. diff --git a/python/ironic-understack/ironic_understack/conf/__init__.py b/python/ironic-understack/ironic_understack/conf/__init__.py index 7955f510f..62e5e3a65 100644 --- a/python/ironic-understack/ironic_understack/conf/__init__.py +++ b/python/ironic-understack/ironic_understack/conf/__init__.py @@ -1,4 +1,5 @@ from oslo_config import cfg +from oslo_config import types CONF = cfg.CONF @@ -31,10 +32,11 @@ def setup_conf(): "1d": "bmc", }, ), - cfg.DictOpt( + cfg.ListOpt( "pci_device_capabilities", - help="Dictionary of pci device IDs (xxxx:xxxx) to capability name", - default={}, + item_type=types.Dict(bounds=True), + help="List of PCI device dictionaries and capability names", + default=None, ), ] cfg.CONF.register_group(grp) diff --git a/python/ironic-understack/ironic_understack/hooks/inspect_hook_pci_device_capabilities.py b/python/ironic-understack/ironic_understack/hooks/inspect_hook_pci_device_capabilities.py index 64220b4d8..73b305059 100644 --- a/python/ironic-understack/ironic_understack/hooks/inspect_hook_pci_device_capabilities.py +++ b/python/ironic-understack/ironic_understack/hooks/inspect_hook_pci_device_capabilities.py @@ -13,12 +13,16 @@ class PciDeviceCapabilitiesHook(base.InspectionHook): """Control a node's PCI device capabilities based on PCI device IDs. - When this hook is configured with config in the config file that looks like: + The hook reads configuration from the ``[ironic_understack]`` section of the + config file: - ironic_understack: - pci_device_capabilities: - 1002:75a8: "amd_gpu_mi350p" - 8086:2002: "intel_duperkard_2" + [ironic_understack] + pci_device_capabilities = "{vendor_id:0x1002,product_id:0x75a8,name:amd_gpu_mi350p}" + + The Helm configuration for this looks like: + + pci_device_capabilities: + - "{vendor_id:0x1002,product_id:0x75a8,name:amd_gpu_mi350p}" Upon inspection, a `capability` will be added to the node for each matching PCI device that is present in the inventory. @@ -38,12 +42,8 @@ class PciDeviceCapabilitiesHook(base.InspectionHook): def __call__(self, task, inventory, plugin_data): """Set node capabilities for each applicable PCI device.""" - capability_names: dict[str, str] = ( - CONF.ironic_understack.pci_device_capabilities - ) - + capability_names = configured_capability_names() if capability_names is None: - LOG.debug("ironic_understack.pci_device_capabilities not configured.") return pci_devices = inventory.get("pci_devices") @@ -68,6 +68,34 @@ def __call__(self, task, inventory, plugin_data): task.node.save() +def configured_capability_names() -> dict[str, str] | None: + """Return configured PCI device capability names. + + An omitted option means that this hook is not configured. An explicitly + empty list intentionally removes any capabilities owned by this hook. + """ + devices = CONF.ironic_understack.pci_device_capabilities + if devices is None: + LOG.debug("PCI device capabilities are not configured") + return None + + capability_names = {} + required_keys = {"vendor_id", "product_id", "name"} + for index, device in enumerate(devices): + if not isinstance(device, dict) or set(device) != required_keys: + raise ValueError( + f"PCI device capability entry {index} must contain exactly " + "vendor_id, product_id, and name" + ) + if not all(isinstance(device[key], str) for key in required_keys): + raise ValueError( + f"PCI device capability entry {index} must contain string values" + ) + capability_names[pci_device_slug(device)] = device["name"] + + return capability_names + + def pci_device_capabilities( pci_devices: list[dict[str, str]], capability_names: dict[str, str], @@ -100,6 +128,6 @@ def pci_device_capabilities( def pci_device_slug(pci_device: dict[str, str]) -> str: """Return device ID in a format like "8086:4002".""" - vendor_id = pci_device["vendor_id"].removeprefix("0x") - product_id = pci_device["product_id"].removeprefix("0x") + vendor_id = pci_device["vendor_id"].lower().removeprefix("0x") + product_id = pci_device["product_id"].lower().removeprefix("0x") return f"{vendor_id}:{product_id}" diff --git a/python/ironic-understack/ironic_understack/tests/test_inspect_hook_pci_device_capabilities.py b/python/ironic-understack/ironic_understack/tests/test_inspect_hook_pci_device_capabilities.py index ef6c40fea..3388cd848 100644 --- a/python/ironic-understack/ironic_understack/tests/test_inspect_hook_pci_device_capabilities.py +++ b/python/ironic-understack/ironic_understack/tests/test_inspect_hook_pci_device_capabilities.py @@ -6,13 +6,23 @@ ) from ironic_understack.hooks.inspect_hook_pci_device_capabilities import pci_device_slug +AMD_GPU = { + "vendor_id": "0x1002", + "product_id": "0x75a8", + "name": "amd_gpu_mi350p", +} -def test_adds_pci_capability_to_node_without_capabilities(mocker): + +def configure_pci_devices(mocker, *devices): mocker.patch( "ironic_understack.hooks.inspect_hook_pci_device_capabilities." "CONF.ironic_understack.pci_device_capabilities", - {"1002:75a8": "amd_gpu_mi350p"}, + list(devices), ) + + +def test_adds_pci_capability_to_node_without_capabilities(mocker): + configure_pci_devices(mocker, AMD_GPU) inventory = { "pci_devices": [ {"vendor_id": "0x1002", "product_id": "0x75a8"}, @@ -30,12 +40,33 @@ def test_adds_pci_capability_to_node_without_capabilities(mocker): node.save.assert_called_once_with() -def test_preserves_non_pci_capabilities(mocker): - mocker.patch( - "ironic_understack.hooks.inspect_hook_pci_device_capabilities." - "CONF.ironic_understack.pci_device_capabilities", - {"1002:75a8": "amd_gpu_mi350p"}, +def test_counts_all_matching_devices_from_redfish_inventory(mocker): + configure_pci_devices(mocker, AMD_GPU) + inventory = { + "pci_devices": [ + {"vendor_id": "0x1002", "product_id": "0x75a8"}, + {"vendor_id": "0x1002", "product_id": "0x75a8"}, + {"vendor_id": "0x1002", "product_id": "0x75a8"}, + {"vendor_id": "0x1002", "product_id": "0x75a8"}, + {"vendor_id": "0x1002", "product_id": "0x75a8"}, + {"vendor_id": "0x1002", "product_id": "0x75a8"}, + {"vendor_id": "0x1002", "product_id": "0x75a8"}, + {"vendor_id": "0x1002", "product_id": "0x75a8"}, + ] + } + node = mocker.Mock(properties={}) + task = mocker.Mock(node=node) + + PciDeviceCapabilitiesHook()(task, inventory, {}) + + node.set_property.assert_called_once_with( + "capabilities", "pci_device_amd_gpu_mi350p:8" ) + node.save.assert_called_once_with() + + +def test_preserves_non_pci_capabilities(mocker): + configure_pci_devices(mocker, AMD_GPU) inventory = { "pci_devices": [ {"vendor_id": "0x1002", "product_id": "0x75a8"}, @@ -55,11 +86,7 @@ def test_preserves_non_pci_capabilities(mocker): def test_replaces_stale_pci_capability(mocker): - mocker.patch( - "ironic_understack.hooks.inspect_hook_pci_device_capabilities." - "CONF.ironic_understack.pci_device_capabilities", - {"1002:75a8": "amd_gpu_mi350p"}, - ) + configure_pci_devices(mocker, AMD_GPU) inventory = { "pci_devices": [ {"vendor_id": "0x1002", "product_id": "0x75a8"}, @@ -80,11 +107,7 @@ def test_replaces_stale_pci_capability(mocker): def test_does_not_duplicate_current_pci_capability(mocker): - mocker.patch( - "ironic_understack.hooks.inspect_hook_pci_device_capabilities." - "CONF.ironic_understack.pci_device_capabilities", - {"1002:75a8": "amd_gpu_mi350p"}, - ) + configure_pci_devices(mocker, AMD_GPU) inventory = { "pci_devices": [ {"vendor_id": "0x1002", "product_id": "0x75a8"}, @@ -103,11 +126,7 @@ def test_does_not_duplicate_current_pci_capability(mocker): def test_removes_pci_capability_when_device_is_no_longer_present(mocker): - mocker.patch( - "ironic_understack.hooks.inspect_hook_pci_device_capabilities." - "CONF.ironic_understack.pci_device_capabilities", - {"1002:75a8": "amd_gpu_mi350p"}, - ) + configure_pci_devices(mocker, AMD_GPU) inventory = { "pci_devices": [ {"vendor_id": "0x1022", "product_id": "0x7901"}, @@ -124,32 +143,24 @@ def test_removes_pci_capability_when_device_is_no_longer_present(mocker): node.save.assert_called_once_with() -def test_does_nothing_when_pci_capabilities_are_not_configured(mocker): - mocker.patch( - "ironic_understack.hooks.inspect_hook_pci_device_capabilities." - "CONF.ironic_understack.pci_device_capabilities", - None, - ) +def test_removes_pci_capability_when_device_is_no_longer_configured(mocker): + configure_pci_devices(mocker) inventory = { "pci_devices": [ {"vendor_id": "0x1002", "product_id": "0x75a8"}, ] } - node = mocker.Mock(properties={}) + node = mocker.Mock(properties={"capabilities": "pci_device_amd_gpu_mi350p:1"}) task = mocker.Mock(node=node) PciDeviceCapabilitiesHook()(task, inventory, {}) - node.set_property.assert_not_called() - node.save.assert_not_called() + node.set_property.assert_called_once_with("capabilities", "") + node.save.assert_called_once_with() def test_does_nothing_when_inventory_has_no_pci_devices(mocker): - mocker.patch( - "ironic_understack.hooks.inspect_hook_pci_device_capabilities." - "CONF.ironic_understack.pci_device_capabilities", - {"1002:75a8": "amd_gpu_mi350p"}, - ) + configure_pci_devices(mocker, AMD_GPU) inventory = {} node = mocker.Mock(properties={}) task = mocker.Mock(node=node) @@ -161,13 +172,25 @@ def test_does_nothing_when_inventory_has_no_pci_devices(mocker): def test_does_nothing_when_pci_device_list_is_empty(mocker): + configure_pci_devices(mocker, AMD_GPU) + inventory = {"pci_devices": []} + node = mocker.Mock(properties={}) + task = mocker.Mock(node=node) + + PciDeviceCapabilitiesHook()(task, inventory, {}) + + node.set_property.assert_not_called() + node.save.assert_not_called() + + +def test_does_nothing_when_configuration_is_omitted(mocker): mocker.patch( "ironic_understack.hooks.inspect_hook_pci_device_capabilities." "CONF.ironic_understack.pci_device_capabilities", - {"1002:75a8": "amd_gpu_mi350p"}, + None, ) - inventory = {"pci_devices": []} - node = mocker.Mock(properties={}) + inventory = {"pci_devices": [{"vendor_id": "0x1002", "product_id": "0x75a8"}]} + node = mocker.Mock(properties={"capabilities": "pci_device_old:1"}) task = mocker.Mock(node=node) PciDeviceCapabilitiesHook()(task, inventory, {}) @@ -218,3 +241,11 @@ def test_pci_device_slug_accepts_ids_without_hexadecimal_prefixes(): slug = pci_device_slug(pci_device) assert slug == "1002:75a8" + + +def test_pci_device_slug_normalizes_uppercase_ids(): + pci_device = {"vendor_id": "0X1002", "product_id": "0X75A8"} + + slug = pci_device_slug(pci_device) + + assert slug == "1002:75a8"