diff --git a/python/understack-workflows/tests/test_enroll_fw.py b/python/understack-workflows/tests/test_enroll_fw.py index 303d3fada..f169af384 100644 --- a/python/understack-workflows/tests/test_enroll_fw.py +++ b/python/understack-workflows/tests/test_enroll_fw.py @@ -64,6 +64,7 @@ def _actual_port( address=mac, physical_network=physnet, category="network", + extra={"bios_name": label}, local_link_connection={ "switch_id": switch_id, "switch_info": switch, @@ -109,6 +110,7 @@ def test_enroll_fw_hands_metadata_to_the_engine(mocker): "management_switch_port": "Ethernet1/24", }, extra={"mate_serial": "026701010045"}, + properties={}, ) diff --git a/python/understack-workflows/tests/test_netdev_reconciler.py b/python/understack-workflows/tests/test_netdev_reconciler.py index 9267c8d8a..244131088 100644 --- a/python/understack-workflows/tests/test_netdev_reconciler.py +++ b/python/understack-workflows/tests/test_netdev_reconciler.py @@ -49,6 +49,9 @@ def existing_port(label, mac, switch, interface, name=None, category="network"): name=name or f"leaf01:{label}", physical_network="f20-1-network", category=category, + # bios_name defaults to the label, matching what the engine writes, so + # an otherwise-matching port is a true no-op. + extra={"bios_name": label}, local_link_connection={ "switch_id": "00:00:00:00:00:00", "switch_info": switch, @@ -90,6 +93,7 @@ def test_enroll_creates_node_ports_logs_and_makes_available(mocker, caplog): call( address="00:11:22:33:44:55", category="network", + extra={"bios_name": "port1"}, local_link_connection={ "switch_id": "00:00:00:00:00:00", "switch_info": "spine01.example.net", @@ -102,6 +106,7 @@ def test_enroll_creates_node_ports_logs_and_makes_available(mocker, caplog): call( address="00:11:22:33:44:66", category="network", + extra={"bios_name": "port2"}, local_link_connection={ "switch_id": "00:00:00:00:00:00", "switch_info": "spine02.example.net", @@ -328,6 +333,7 @@ def test_enroll_updates_existing_port_and_creates_missing_one(mocker): fake_ironic.port.create.assert_called_once_with( address="00:11:22:33:44:66", category="network", + extra={"bios_name": "port2"}, local_link_connection={ "switch_id": "00:00:00:00:00:00", "switch_info": "spine02.example.net", @@ -856,6 +862,7 @@ def test_enroll_switch_id_override_on_create(mocker): fake_ironic.port.create.assert_called_once_with( address="00:11:22:33:44:55", category="network", + extra={"bios_name": "port1"}, local_link_connection={ "switch_id": "aa:bb:cc:dd:ee:ff", "switch_info": "spine01.example.net", diff --git a/python/understack-workflows/understack_workflows/firewall.py b/python/understack-workflows/understack_workflows/firewall.py index 5a3373190..492177f43 100644 --- a/python/understack-workflows/understack_workflows/firewall.py +++ b/python/understack-workflows/understack_workflows/firewall.py @@ -18,13 +18,18 @@ def firewall_metadata( management_switch: str = "", management_switch_port: str = "", mate_serial: str = "", -) -> tuple[dict, dict]: - """Build (driver_info, extra) from the firewall fields (non-empty only). + serial: str = "", + vendor: str = "", + model: str = "", +) -> tuple[dict, dict, dict]: + """Build (driver_info, extra, properties) from the firewall fields. - Management access goes in driver_info (mirroring how servers store - redfish_address); the HA mate serial goes in extra. external_cmdb_id is not - handled here -- the caller decides where it goes (the enroll engine folds it - into extra; the metadata patch adds it explicitly). + Only non-empty values are included. Management access goes in driver_info + (mirroring how servers store redfish_address); the device serial and HA mate + serial go in extra; vendor/model go in properties (which the Nautobot device + sync reads). external_cmdb_id is not handled here -- the caller decides where + it goes (the enroll engine folds it into extra; the metadata patch adds it + explicitly). """ driver_info = { key: value @@ -35,18 +40,28 @@ def firewall_metadata( }.items() if value } - extra = {"mate_serial": mate_serial} if mate_serial else {} - return driver_info, extra + extra = { + key: value + for key, value in {"serial": serial, "mate_serial": mate_serial}.items() + if value + } + properties = { + key: value for key, value in {"vendor": vendor, "model": model}.items() if value + } + return driver_info, extra, properties -def apply_node_metadata(client, node, driver_info: dict, extra: dict) -> None: - """Diff-patch driver_info/extra onto an existing node (any provision state). +def apply_node_metadata( + client, node, driver_info: dict, extra: dict, properties: dict | None = None +) -> None: + """Diff-patch driver_info/extra/properties onto a node (any provision state). Only the supplied keys are considered; keys the request does not mention are left untouched. """ node_driver_info = getattr(node, "driver_info", None) or {} node_extra = getattr(node, "extra", None) or {} + node_properties = getattr(node, "properties", None) or {} updates = [] for key, value in driver_info.items(): @@ -55,6 +70,9 @@ def apply_node_metadata(client, node, driver_info: dict, extra: dict) -> None: for key, value in extra.items(): if node_extra.get(key) != value: updates.append(f"extra/{key}={value}") + for key, value in (properties or {}).items(): + if node_properties.get(key) != value: + updates.append(f"properties/{key}={value}") if not updates: logger.info("[node:%s] Firewall metadata already up to date", node.uuid) diff --git a/python/understack-workflows/understack_workflows/main/enroll_fw.py b/python/understack-workflows/understack_workflows/main/enroll_fw.py index 9a968a9eb..78ef939ae 100644 --- a/python/understack-workflows/understack_workflows/main/enroll_fw.py +++ b/python/understack-workflows/understack_workflows/main/enroll_fw.py @@ -32,6 +32,9 @@ def main() -> None: management_switch=args.management_switch, management_switch_port=args.management_switch_port, mate_serial=args.mate_serial, + serial=args.serial, + vendor=args.vendor, + model=args.model, ) @@ -80,16 +83,22 @@ def enroll_fw( management_switch: str = "", management_switch_port: str = "", mate_serial: str = "", + serial: str = "", + vendor: str = "", + model: str = "", ) -> None: resource_class = _require_specific_resource_class(resource_class) management_switch, management_switch_port = _require_management_location( management_switch, management_switch_port ) - driver_info, extra = firewall.firewall_metadata( + driver_info, extra, properties = firewall.firewall_metadata( management_ip=management_ip, management_switch=management_switch, management_switch_port=management_switch_port, mate_serial=mate_serial, + serial=serial, + vendor=vendor, + model=model, ) # Look up the node first so we can tell an in-service (active) firewall from @@ -110,6 +119,7 @@ def enroll_fw( external_cmdb_id=external_cmdb_id, driver_info=driver_info, extra=extra, + properties=properties, ) return @@ -124,6 +134,7 @@ def enroll_fw( external_cmdb_id=external_cmdb_id, driver_info=driver_info, extra=extra, + properties=properties, ) @@ -146,6 +157,7 @@ def _update_active_firewall( external_cmdb_id: int | str | None, driver_info: dict, extra: dict, + properties: dict, ) -> None: """Update firewall metadata on an in-service (active) node, in place. @@ -187,7 +199,7 @@ def _update_active_firewall( "updating firewall metadata only", node.uuid, ) - firewall.apply_node_metadata(client, node, driver_info, active_extra) + firewall.apply_node_metadata(client, node, driver_info, active_extra, properties) def _reject_structural_drift( @@ -242,7 +254,10 @@ def argument_parser(): parser.add_argument( "--ports", required=True, - help="JSON array of ports (same format as enroll-netdev)", + help="JSON array of ports (same format as enroll-netdev). 'switch' must " + "be the switch FQDN (e.g. n11-22-1.dfw3.rackspace.net) so the Nautobot " + "sync can resolve the cable. Optional per-port 'bios_name' (the device " + "interface name); defaults to the label.", ) parser.add_argument( "--resource-class", @@ -279,6 +294,24 @@ def argument_parser(): default="", help="HA mate serial number -> extra.mate_serial", ) + parser.add_argument( + "--serial", + required=False, + default="", + help="Device's own serial number -> extra.serial (Nautobot sync)", + ) + parser.add_argument( + "--vendor", + required=False, + default="Palo Alto", + help="Device vendor -> properties.vendor (Nautobot sync)", + ) + parser.add_argument( + "--model", + required=False, + default="", + help="Device model, e.g. PA-1410 -> properties.model (Nautobot sync)", + ) return parser diff --git a/python/understack-workflows/understack_workflows/netdev_reconciler.py b/python/understack-workflows/understack_workflows/netdev_reconciler.py index 0efa6e9b8..9865baf59 100644 --- a/python/understack-workflows/understack_workflows/netdev_reconciler.py +++ b/python/understack-workflows/understack_workflows/netdev_reconciler.py @@ -24,7 +24,7 @@ # that converge on re-runs. switch_id is optional: when omitted the placeholder # is used (and an existing real value is preserved). REQUIRED_PORT_FIELDS = ("label", "mac", "switch", "intf") -OPTIONAL_PORT_FIELDS = ("switch_id",) +OPTIONAL_PORT_FIELDS = ("switch_id", "bios_name") ALLOWED_PORT_FIELDS = frozenset(REQUIRED_PORT_FIELDS + OPTIONAL_PORT_FIELDS) @@ -35,6 +35,10 @@ class NetdevPort: switch: str interface: str switch_id: str | None = None + # The device's own interface name (e.g. "ethernet1/19"), stored in the + # Ironic port's extra.bios_name. The Nautobot device sync uses it to name + # the interface. Defaults to the label when not supplied. + bios_name: str | None = None def _has_cmdb_id(external_cmdb_id: int | str | None) -> bool: @@ -91,6 +95,11 @@ def build_netdev_ports(ports: list[dict]) -> list[NetdevPort]: raise ValueError( f"Port {index} switch_id must be a string, got {switch_id!r}" ) + bios_name = entry.get("bios_name") + if bios_name is not None and not isinstance(bios_name, str): + raise ValueError( + f"Port {index} bios_name must be a string, got {bios_name!r}" + ) unknown = set(entry) - ALLOWED_PORT_FIELDS if unknown: raise ValueError( @@ -113,6 +122,9 @@ def build_netdev_ports(ports: list[dict]) -> list[NetdevPort]: switch=entry["switch"], interface=entry["intf"], switch_id=entry.get("switch_id"), + # bios_name is the device's interface name; for firewalls it + # equals the label, so default to it when not supplied. + bios_name=entry.get("bios_name") or entry["label"], ) ) return result @@ -127,18 +139,20 @@ def enroll( resource_class: str | None = DEFAULT_RESOURCE_CLASS, driver_info: dict | None = None, extra: dict | None = None, + properties: dict | None = None, ) -> None: effective_resource_class = resource_class or DEFAULT_RESOURCE_CLASS netdev_ports = build_netdev_ports(ports) - # driver_info/extra are generic Ironic node metadata a caller may want set - # (e.g. the firewall workflow records management access in driver_info). - # They are written inside the enrollment lifecycle -- at node create, or - # patched before the node is made available -- so the node is never - # allocatable in an under-described state. external_cmdb_id is folded into - # extra for convenience. + # driver_info/extra/properties are generic Ironic node metadata a caller may + # want set (e.g. the firewall workflow records management access in + # driver_info and vendor/model in properties). They are written inside the + # enrollment lifecycle -- at node create, or patched before the node is made + # available -- so the node is never allocatable in an under-described state. + # external_cmdb_id is folded into extra for convenience. node_driver_info = dict(driver_info or {}) node_extra = dict(extra or {}) + node_properties = dict(properties or {}) if _has_cmdb_id(external_cmdb_id): node_extra["external_cmdb_id"] = external_cmdb_id @@ -154,6 +168,8 @@ def enroll( logger.info("Recording driver_info=%s on the Ironic node", node_driver_info) if node_extra: logger.info("Recording extra=%s on the Ironic node", node_extra) + if node_properties: + logger.info("Recording properties=%s on the Ironic node", node_properties) client = IronicClient() node, created = find_or_create_netdev_node( @@ -162,6 +178,7 @@ def enroll( resource_class=effective_resource_class, driver_info=node_driver_info, extra=node_extra, + properties=node_properties, ) node_ports = list(client.list_ports(node.uuid)) @@ -216,6 +233,7 @@ def enroll( resource_class=effective_resource_class, driver_info=node_driver_info, extra=node_extra, + properties=node_properties, ) if not pending and state == "available": @@ -277,6 +295,7 @@ def find_or_create_netdev_node( resource_class: str, driver_info: dict, extra: dict, + properties: dict | None = None, ) -> tuple[Node, bool]: """Find an existing netdev node by name, or create one. @@ -293,6 +312,7 @@ def find_or_create_netdev_node( resource_class=resource_class, driver_info=driver_info, extra=extra, + properties=properties or {}, ) return node, True @@ -325,12 +345,13 @@ def update_node_metadata( resource_class: str, driver_info: dict, extra: dict, + properties: dict | None = None, ) -> bool: - """Patch resource_class/driver_info/extra keys that differ from the node. + """Patch resource_class/driver_info/extra/properties keys that differ. - Only the supplied driver_info/extra keys are considered; existing keys the - request does not mention are left untouched. Returns True if a patch was - sent, so the caller can report accurately. + Only the supplied driver_info/extra/properties keys are considered; existing + keys the request does not mention are left untouched. Returns True if a patch + was sent, so the caller can report accurately. """ updates = [] if getattr(node, "resource_class", None) != resource_class: @@ -346,6 +367,11 @@ def update_node_metadata( if node_extra.get(key) != value: updates.append(f"extra/{key}={value}") + node_properties = getattr(node, "properties", None) or {} + for key, value in (properties or {}).items(): + if node_properties.get(key) != value: + updates.append(f"properties/{key}={value}") + if not updates: return False @@ -361,6 +387,7 @@ def create_netdev_node( resource_class: str, driver_info: dict, extra: dict, + properties: dict | None = None, ) -> Node: node_data = { "automated_clean": False, @@ -372,6 +399,8 @@ def create_netdev_node( node_data["driver_info"] = driver_info if extra: node_data["extra"] = extra + if properties: + node_data["properties"] = properties logger.info( "Creating netdev Ironic node name=%s driver=%s " @@ -455,6 +484,13 @@ def plan_netdev_port( ) if getattr(existing, "category", None) != "network": patch.append({"op": "add", "path": "/category", "value": "network"}) + # Converge extra.bios_name (the device's interface name) so re-runs update + # it. Replace the whole extra object is unsafe (would drop other keys), so + # patch the single nested key. + bios_name = port.bios_name or port.label + current_extra = getattr(existing, "extra", None) or {} + if current_extra.get("bios_name") != bios_name: + patch.append({"op": "add", "path": "/extra/bios_name", "value": bios_name}) if any(current_llc.get(key) != value for key, value in desired_llc.items()): # Replace the whole object rather than nested keys: Ironic allows # local_link_connection to be null, and a nested JSON patch would fail @@ -520,9 +556,11 @@ def create_netdev_port( ) -> None: port_name = f"{node_name}:{port.label}" switch_id = port.switch_id or PLACEHOLDER_SWITCH_ID + bios_name = port.bios_name or port.label port_data = { "address": port.mac, "category": "network", + "extra": {"bios_name": bios_name}, "local_link_connection": { "switch_id": switch_id, "switch_info": port.switch, diff --git a/workflows/argo-events/workflowtemplates/enroll-fw.yaml b/workflows/argo-events/workflowtemplates/enroll-fw.yaml index 4fb9c2c36..8b66173c9 100644 --- a/workflows/argo-events/workflowtemplates/enroll-fw.yaml +++ b/workflows/argo-events/workflowtemplates/enroll-fw.yaml @@ -37,6 +37,14 @@ spec: - name: management_switch_port - name: mate_serial value: "" + # Device identity, read by the Nautobot device sync. serial -> extra; + # vendor/model -> properties. 'switch' in ports must be the switch FQDN. + - name: serial + value: "" + - name: vendor + value: "Palo Alto" + - name: model + value: "" templates: - name: main steps: @@ -66,6 +74,12 @@ spec: - "{{workflow.parameters.management_switch_port}}" - --mate-serial - "{{workflow.parameters.mate_serial}}" + - --serial + - "{{workflow.parameters.serial}}" + - --vendor + - "{{workflow.parameters.vendor}}" + - --model + - "{{workflow.parameters.model}}" volumeMounts: - mountPath: /etc/openstack name: baremetal-manage