fix: configuration handling for pci device types

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.

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)

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}"

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"