From 33c6016d9c845b124f243572e1539db0f4730223 Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Wed, 22 Jul 2026 18:42:37 +0530 Subject: [PATCH 1/4] make enroll-netdev re-runnable via find-or-create --- .../tests/test_enroll_netdev.py | 309 ++++++++++++++++-- .../main/enroll_netdev.py | 215 ++++++++++-- 2 files changed, 484 insertions(+), 40 deletions(-) diff --git a/python/understack-workflows/tests/test_enroll_netdev.py b/python/understack-workflows/tests/test_enroll_netdev.py index 171975793..546e93b11 100644 --- a/python/understack-workflows/tests/test_enroll_netdev.py +++ b/python/understack-workflows/tests/test_enroll_netdev.py @@ -3,14 +3,30 @@ from unittest.mock import MagicMock from unittest.mock import call +import pytest +from ironicclient.common.apiclient import exceptions as ironic_exceptions + from understack_workflows import ironic_node from understack_workflows.main import enroll_netdev +ENROLL_ARGS = dict( + name="leaf01", + physical_network="f20-1-network", + port1_mac="00:11:22:33:44:55", + port1_switch="spine01.example.net", + port1_intf="Ethernet1/1", + port2_mac="00:11:22:33:44:66", + port2_switch="spine02.example.net", + port2_intf="Ethernet1/2", +) + def make_ironic_client(): fake_client = MagicMock() - node = SimpleNamespace(uuid="node-123") + node = SimpleNamespace(uuid="node-123", driver="netdev", provision_state="enroll") + fake_client.node.get.side_effect = ironic_exceptions.NotFound() fake_client.node.create.return_value = node + fake_client.port.list.return_value = [] fake_client.port.create.side_effect = [ SimpleNamespace(uuid="port-1"), SimpleNamespace(uuid="port-2"), @@ -18,6 +34,33 @@ def make_ironic_client(): return fake_client, node +def existing_port(label, mac, switch, interface, name=None, category="network"): + return SimpleNamespace( + uuid=f"uuid-{label}", + address=mac, + name=name or f"leaf01:{label}", + physical_network="f20-1-network", + category=category, + local_link_connection={ + "switch_id": "00:00:00:00:00:00", + "switch_info": switch, + "port_id": interface, + }, + ) + + +def matching_ports(): + """Two existing ports that exactly match the ENROLL_ARGS request.""" + return [ + existing_port( + "port1", "00:11:22:33:44:55", "spine01.example.net", "Ethernet1/1" + ), + existing_port( + "port2", "00:11:22:33:44:66", "spine02.example.net", "Ethernet1/2" + ), + ] + + def test_enroll_creates_node_ports_logs_and_makes_available(mocker, caplog): caplog.set_level(logging.INFO) fake_ironic, node = make_ironic_client() @@ -26,17 +69,7 @@ def test_enroll_creates_node_ports_logs_and_makes_available(mocker, caplog): return_value=fake_ironic, ) - enroll_netdev.enroll( - name="leaf01", - physical_network="f20-1-network", - port1_mac="00:11:22:33:44:55", - port1_switch="spine01.example.net", - port1_intf="Ethernet1/1", - port2_mac="00:11:22:33:44:66", - port2_switch="spine02.example.net", - port2_intf="Ethernet1/2", - resource_class=None, - ) + enroll_netdev.enroll(**ENROLL_ARGS, resource_class=None) fake_ironic.node.create.assert_called_once_with( automated_clean=False, @@ -118,14 +151,7 @@ def test_enroll_records_external_cmdb_id_and_custom_resource_class(mocker): ) enroll_netdev.enroll( - name="leaf01", - physical_network="f20-1-network", - port1_mac="00:11:22:33:44:55", - port1_switch="spine01.example.net", - port1_intf="Ethernet1/1", - port2_mac="00:11:22:33:44:66", - port2_switch="spine02.example.net", - port2_intf="Ethernet1/2", + **ENROLL_ARGS, external_cmdb_id="cmdb-1", resource_class="switch", ) @@ -139,6 +165,249 @@ def test_enroll_records_external_cmdb_id_and_custom_resource_class(mocker): ) +def test_enroll_reuses_existing_node_and_matching_ports(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="manageable" + ) + fake_ironic.port.list.return_value = matching_ports() + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + fake_ironic.node.create.assert_not_called() + fake_ironic.port.create.assert_not_called() + fake_ironic.port.update.assert_not_called() + fake_ironic.node.update.assert_called_once_with( + "node-123", + [{"op": "add", "path": "/resource_class", "value": "generic"}], + ) + # Node was already manageable: no manage transition, only provide. + fake_ironic.node.set_provision_state.assert_called_once_with( + "node-123", + "provide", + cleansteps=None, + runbook=None, + disable_ramdisk=None, + ) + + +def test_enroll_updates_existing_port_and_creates_missing_one(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="manageable" + ) + stale_port1 = existing_port( + "port1", "00:11:22:33:44:55", "old-switch.example.net", "Ethernet9/9" + ) + fake_ironic.port.list.return_value = [stale_port1] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + fake_ironic.port.update.assert_called_once_with( + "uuid-port1", + [ + { + "op": "add", + "path": "/local_link_connection", + "value": { + "switch_id": "00:00:00:00:00:00", + "switch_info": "spine01.example.net", + "port_id": "Ethernet1/1", + }, + }, + ], + ) + fake_ironic.port.create.assert_called_once_with( + address="00:11:22:33:44:66", + category="network", + local_link_connection={ + "switch_id": "00:00:00:00:00:00", + "switch_info": "spine02.example.net", + "port_id": "Ethernet1/2", + }, + name="leaf01:port2", + node_uuid="node-123", + physical_network="f20-1-network", + ) + + +def test_enroll_skips_transitions_when_node_already_available(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="available" + ) + fake_ironic.port.list.return_value = matching_ports() + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + fake_ironic.node.set_provision_state.assert_not_called() + fake_ironic.node.wait_for_provision_state.assert_not_called() + + +def test_enroll_fails_on_existing_node_with_other_driver(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="idrac", provision_state="active" + ) + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + with pytest.raises(RuntimeError, match="refusing to enroll it as a netdev"): + enroll_netdev.enroll(**ENROLL_ARGS) + + fake_ironic.node.create.assert_not_called() + fake_ironic.port.create.assert_not_called() + + +def test_enroll_fails_on_node_in_unexpected_state(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="active" + ) + fake_ironic.port.list.return_value = matching_ports() + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + with pytest.raises(RuntimeError, match="Cannot enroll node in provision_state"): + enroll_netdev.enroll(**ENROLL_ARGS) + + # State is checked before any mutation: node is not patched, no transitions. + fake_ironic.node.update.assert_not_called() + fake_ironic.node.set_provision_state.assert_not_called() + + +def test_enroll_rejects_duplicate_port_macs(mocker): + fake_ironic, _ = make_ironic_client() + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + args = dict(ENROLL_ARGS) + args["port2_mac"] = args["port1_mac"] + + with pytest.raises(ValueError, match="must be different"): + enroll_netdev.enroll(**args) + + # Fail fast, before any Ironic calls. + fake_ironic.node.get.assert_not_called() + fake_ironic.node.create.assert_not_called() + + +def test_enroll_rejects_port_name_conflict(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="manageable" + ) + # A port named leaf01:port1 exists, but with a MAC other than requested. + fake_ironic.port.list.return_value = [ + existing_port( + "port1", "00:aa:bb:cc:dd:ee", "spine01.example.net", "Ethernet1/1" + ), + ] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + with pytest.raises(RuntimeError, match="already exists with MAC"): + enroll_netdev.enroll(**ENROLL_ARGS) + + fake_ironic.port.create.assert_not_called() + + +def test_enroll_updates_port_with_null_local_link_connection(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="manageable" + ) + port1 = existing_port( + "port1", "00:11:22:33:44:55", "spine01.example.net", "Ethernet1/1" + ) + port1.local_link_connection = None + fake_ironic.port.list.return_value = [ + port1, + existing_port( + "port2", "00:11:22:33:44:66", "spine02.example.net", "Ethernet1/2" + ), + ] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + # Whole local_link_connection object is replaced, not nested keys, so the + # patch is valid even though the existing value was null. + fake_ironic.port.update.assert_called_once_with( + "uuid-port1", + [ + { + "op": "add", + "path": "/local_link_connection", + "value": { + "switch_id": "00:00:00:00:00:00", + "switch_info": "spine01.example.net", + "port_id": "Ethernet1/1", + }, + }, + ], + ) + + +def test_enroll_converges_existing_port_missing_category(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="manageable" + ) + port1 = existing_port( + "port1", "00:11:22:33:44:55", "spine01.example.net", "Ethernet1/1" + ) + port1.category = None + fake_ironic.port.list.return_value = [ + port1, + existing_port( + "port2", "00:11:22:33:44:66", "spine02.example.net", "Ethernet1/2" + ), + ] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + fake_ironic.port.update.assert_called_once_with( + "uuid-port1", + [{"op": "add", "path": "/category", "value": "network"}], + ) + + def test_argument_parser_defaults_resource_class_to_generic(): args = enroll_netdev.argument_parser().parse_args( [ diff --git a/python/understack-workflows/understack_workflows/main/enroll_netdev.py b/python/understack-workflows/understack_workflows/main/enroll_netdev.py index 5240a3367..452888b99 100644 --- a/python/understack-workflows/understack_workflows/main/enroll_netdev.py +++ b/python/understack-workflows/understack_workflows/main/enroll_netdev.py @@ -3,6 +3,8 @@ import os from dataclasses import dataclass +from ironicclient.common.apiclient import exceptions as ironic_exceptions +from ironicclient.common.utils import args_array_to_patch from ironicclient.v1.node import Node from understack_workflows import helpers @@ -11,6 +13,14 @@ logger = logging.getLogger(__name__) +DEFAULT_RESOURCE_CLASS = "generic" +PLACEHOLDER_SWITCH_ID = "00:00:00:00:00:00" + +# Provision states from which enrollment can proceed to "available". +# Anything else (e.g. "active", "deploying") means the node is in use +# and we must not touch it. +REENROLLABLE_STATES = {"enroll", "manageable", "available"} + @dataclass(frozen=True) class NetdevPort: @@ -20,8 +30,22 @@ class NetdevPort: interface: str +def _has_cmdb_id(external_cmdb_id: int | str | None) -> bool: + """Return True unless the CMDB ID is unset (None or empty string). + + A truthiness check would wrongly drop a valid CMDB ID of 0. + """ + return external_cmdb_id not in (None, "") + + def main() -> None: - """Create a netdev baremetal node, its 2 ports, and make it available.""" + """Create a netdev baremetal node, its 2 ports, and make it available. + + Re-running with the same parameters is safe: an existing node with the + same name is reused (and updated), existing ports are matched by MAC + address and updated in place, and provision state transitions are only + performed when needed. + """ helpers.setup_logger() args = argument_parser().parse_args() @@ -50,9 +74,9 @@ def enroll( port2_switch: str, port2_intf: str, external_cmdb_id: int | str | None = None, - resource_class: str | None = "generic", + resource_class: str | None = DEFAULT_RESOURCE_CLASS, ) -> None: - effective_resource_class = resource_class or "generic" + effective_resource_class = resource_class or DEFAULT_RESOURCE_CLASS logger.info( "Starting enroll-netdev workflow name=%s physical_network=%s " "resource_class=%s", @@ -61,7 +85,7 @@ def enroll( effective_resource_class, ) - if external_cmdb_id: + if _has_cmdb_id(external_cmdb_id): logger.info( "Recording external_cmdb_id=%s on the Ironic node", external_cmdb_id, @@ -69,45 +93,87 @@ def enroll( else: logger.info("No external_cmdb_id provided") + if port1_mac.lower() == port2_mac.lower(): + raise ValueError( + f"port1_mac and port2_mac must be different, both are {port1_mac}" + ) + client = IronicClient() - node = create_netdev_node( + node = find_or_create_netdev_node( client=client, name=name, resource_class=effective_resource_class, external_cmdb_id=external_cmdb_id, ) + node_ports = list(client.list_ports(node.uuid)) + ports_by_mac = {(p.address or "").lower(): p for p in node_ports} + ports_by_name = {p.name: p for p in node_ports if p.name} + ports = [ NetdevPort("port1", port1_mac, port1_switch, port1_intf), NetdevPort("port2", port2_mac, port2_switch, port2_intf), ] for port in ports: - create_netdev_port( + find_or_create_netdev_port( client=client, node=node, node_name=name, physical_network=physical_network, port=port, + existing=ports_by_mac.get(port.mac.lower()), + ports_by_name=ports_by_name, ) + make_available(node) logger.info( - "[node:%s] Requesting manage transition, expecting manageable", + "Completed enroll-netdev workflow name=%s node_uuid=%s", + name, node.uuid, ) - ironic_node.transition(node, target_state="manage", expected_state="manageable") - logger.info("[node:%s] Node is manageable", node.uuid) + + +def find_or_create_netdev_node( + *, + client: IronicClient, + name: str, + resource_class: str, + external_cmdb_id: int | str | None = None, +) -> Node: + try: + node = client.get_node(name) + except ironic_exceptions.NotFound: + return create_netdev_node( + client=client, + name=name, + resource_class=resource_class, + external_cmdb_id=external_cmdb_id, + ) + + if node.driver != "netdev": + raise RuntimeError( + f"Node {name} ({node.uuid}) already exists with driver " + f"{node.driver!r}; refusing to enroll it as a netdev" + ) + + if node.provision_state not in REENROLLABLE_STATES: + raise RuntimeError( + f"[node:{node.uuid}] Cannot enroll node in provision_state " + f"{node.provision_state!r}; expected one of " + f"{sorted(REENROLLABLE_STATES)}" + ) logger.info( - "[node:%s] Requesting provide transition, expecting available", + "[node:%s] Reusing existing netdev node name=%s provision_state=%s", node.uuid, - ) - ironic_node.transition(node, target_state="provide", expected_state="available") - logger.info("[node:%s] Node is available", node.uuid) - logger.info( - "Completed enroll-netdev workflow name=%s node_uuid=%s", name, - node.uuid, + node.provision_state, ) + updates = [f"resource_class={resource_class}"] + if _has_cmdb_id(external_cmdb_id): + updates.append(f"extra/external_cmdb_id={external_cmdb_id}") + client.update_node(node.uuid, args_array_to_patch("add", updates)) + return node def create_netdev_node( @@ -123,7 +189,7 @@ def create_netdev_node( "name": name, "resource_class": resource_class, } - if external_cmdb_id: + if _has_cmdb_id(external_cmdb_id): node_data["extra"] = {"external_cmdb_id": external_cmdb_id} logger.info( @@ -139,6 +205,84 @@ def create_netdev_node( return node +def find_or_create_netdev_port( + *, + client: IronicClient, + node: Node, + node_name: str, + physical_network: str, + port: NetdevPort, + existing=None, + ports_by_name: dict | None = None, +) -> None: + port_name = f"{node_name}:{port.label}" + ports_by_name = ports_by_name or {} + + if existing is None: + name_clash = ports_by_name.get(port_name) + if name_clash is not None: + raise RuntimeError( + f"[node:{node.uuid}] Port {port_name} already exists with MAC " + f"{name_clash.address}, but requested MAC is {port.mac}" + ) + create_netdev_port( + client=client, + node=node, + node_name=node_name, + physical_network=physical_network, + port=port, + ) + return + + desired_llc = { + "switch_id": PLACEHOLDER_SWITCH_ID, + "switch_info": port.switch, + "port_id": port.interface, + } + current_llc = existing.local_link_connection or {} + + patch = [] + if existing.name != port_name: + name_clash = ports_by_name.get(port_name) + if name_clash is not None and name_clash.uuid != existing.uuid: + raise RuntimeError( + f"[node:{node.uuid}] Cannot rename port {existing.uuid} to " + f"{port_name}: that name is already used by port {name_clash.uuid}" + ) + patch.append({"op": "add", "path": "/name", "value": port_name}) + if existing.physical_network != physical_network: + patch.append( + {"op": "add", "path": "/physical_network", "value": physical_network} + ) + if getattr(existing, "category", None) != "network": + patch.append({"op": "add", "path": "/category", "value": "network"}) + 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 + # when the parent object is absent. + patch.append( + {"op": "add", "path": "/local_link_connection", "value": desired_llc} + ) + + if not patch: + logger.info( + "[node:%s] Port name=%s mac=%s already up to date, skipping", + node.uuid, + port_name, + port.mac, + ) + return + + logger.info( + "[node:%s] Updating existing port uuid=%s mac=%s patch=%s", + node.uuid, + existing.uuid, + port.mac, + patch, + ) + client.update_port(existing.uuid, patch) + + def create_netdev_port( *, client: IronicClient, @@ -152,7 +296,7 @@ def create_netdev_port( "address": port.mac, "category": "network", "local_link_connection": { - "switch_id": "00:00:00:00:00:00", + "switch_id": PLACEHOLDER_SWITCH_ID, "switch_info": port.switch, "port_id": port.interface, }, @@ -169,7 +313,7 @@ def create_netdev_port( port_name, port.mac, physical_network, - "00:00:00:00:00:00", + PLACEHOLDER_SWITCH_ID, port.switch, port.interface, ) @@ -182,6 +326,37 @@ def create_netdev_port( ) +def make_available(node: Node) -> None: + state = node.provision_state + + if state == "available": + logger.info("[node:%s] Node is already available", node.uuid) + return + + if state not in REENROLLABLE_STATES: + raise RuntimeError( + f"[node:{node.uuid}] Cannot make node available from " + f"provision_state {state!r}" + ) + + if state == "enroll": + logger.info( + "[node:%s] Requesting manage transition, expecting manageable", + node.uuid, + ) + ironic_node.transition( + node, target_state="manage", expected_state="manageable" + ) + logger.info("[node:%s] Node is manageable", node.uuid) + + logger.info( + "[node:%s] Requesting provide transition, expecting available", + node.uuid, + ) + ironic_node.transition(node, target_state="provide", expected_state="available") + logger.info("[node:%s] Node is available", node.uuid) + + def argument_parser(): parser = argparse.ArgumentParser( prog=os.path.basename(__file__), @@ -217,7 +392,7 @@ def argument_parser(): parser.add_argument( "--resource-class", required=False, - default="generic", + default=DEFAULT_RESOURCE_CLASS, help="Ironic resource class", ) return parser From 64cf0dd3c4cde471f71948da2f0892e8a3b918ae Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Thu, 23 Jul 2026 16:41:17 +0530 Subject: [PATCH 2/4] for testing --- .../tests/test_enroll_netdev.py | 20 +++++++++---------- .../main/enroll_netdev.py | 6 ++---- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/python/understack-workflows/tests/test_enroll_netdev.py b/python/understack-workflows/tests/test_enroll_netdev.py index 546e93b11..1cf3c66ff 100644 --- a/python/understack-workflows/tests/test_enroll_netdev.py +++ b/python/understack-workflows/tests/test_enroll_netdev.py @@ -9,16 +9,16 @@ from understack_workflows import ironic_node from understack_workflows.main import enroll_netdev -ENROLL_ARGS = dict( - name="leaf01", - physical_network="f20-1-network", - port1_mac="00:11:22:33:44:55", - port1_switch="spine01.example.net", - port1_intf="Ethernet1/1", - port2_mac="00:11:22:33:44:66", - port2_switch="spine02.example.net", - port2_intf="Ethernet1/2", -) +ENROLL_ARGS = { + "name": "leaf01", + "physical_network": "f20-1-network", + "port1_mac": "00:11:22:33:44:55", + "port1_switch": "spine01.example.net", + "port1_intf": "Ethernet1/1", + "port2_mac": "00:11:22:33:44:66", + "port2_switch": "spine02.example.net", + "port2_intf": "Ethernet1/2", +} def make_ironic_client(): diff --git a/python/understack-workflows/understack_workflows/main/enroll_netdev.py b/python/understack-workflows/understack_workflows/main/enroll_netdev.py index 452888b99..029b3de67 100644 --- a/python/understack-workflows/understack_workflows/main/enroll_netdev.py +++ b/python/understack-workflows/understack_workflows/main/enroll_netdev.py @@ -18,7 +18,7 @@ # Provision states from which enrollment can proceed to "available". # Anything else (e.g. "active", "deploying") means the node is in use -# and we must not touch it. +# and we are not touching it. REENROLLABLE_STATES = {"enroll", "manageable", "available"} @@ -344,9 +344,7 @@ def make_available(node: Node) -> None: "[node:%s] Requesting manage transition, expecting manageable", node.uuid, ) - ironic_node.transition( - node, target_state="manage", expected_state="manageable" - ) + ironic_node.transition(node, target_state="manage", expected_state="manageable") logger.info("[node:%s] Node is manageable", node.uuid) logger.info( From a6ba354a7b85e4084cfb13cf0f9e4ecc7f0de83e Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Thu, 23 Jul 2026 16:54:24 +0530 Subject: [PATCH 3/4] for testing --- workflows/argo-events/workflowtemplates/enroll-netdev.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workflows/argo-events/workflowtemplates/enroll-netdev.yaml b/workflows/argo-events/workflowtemplates/enroll-netdev.yaml index 549e4e798..17bad87b1 100644 --- a/workflows/argo-events/workflowtemplates/enroll-netdev.yaml +++ b/workflows/argo-events/workflowtemplates/enroll-netdev.yaml @@ -38,7 +38,7 @@ spec: template: enroll-netdev - name: enroll-netdev container: - image: ghcr.io/rackerlabs/understack/ironic-nautobot-client:latest + image: ghcr.io/rackerlabs/understack/ironic-nautobot-client:pr-2158 command: - enroll-netdev args: From c5f90c9e194d95391cefa6effdd4f669e8cbc67d Mon Sep 17 00:00:00 2001 From: Nidhi Rai Date: Fri, 24 Jul 2026 20:43:48 +0530 Subject: [PATCH 4/4] for testing --- .../tests/test_enroll_netdev.py | 380 +++++++++++++++++- .../main/enroll_netdev.py | 318 ++++++++++++--- .../workflowtemplates/enroll-netdev.yaml | 24 +- 3 files changed, 619 insertions(+), 103 deletions(-) diff --git a/python/understack-workflows/tests/test_enroll_netdev.py b/python/understack-workflows/tests/test_enroll_netdev.py index 1cf3c66ff..504cdd003 100644 --- a/python/understack-workflows/tests/test_enroll_netdev.py +++ b/python/understack-workflows/tests/test_enroll_netdev.py @@ -12,12 +12,18 @@ ENROLL_ARGS = { "name": "leaf01", "physical_network": "f20-1-network", - "port1_mac": "00:11:22:33:44:55", - "port1_switch": "spine01.example.net", - "port1_intf": "Ethernet1/1", - "port2_mac": "00:11:22:33:44:66", - "port2_switch": "spine02.example.net", - "port2_intf": "Ethernet1/2", + "ports": [ + { + "mac": "00:11:22:33:44:55", + "switch": "spine01.example.net", + "intf": "Ethernet1/1", + }, + { + "mac": "00:11:22:33:44:66", + "switch": "spine02.example.net", + "intf": "Ethernet1/2", + }, + ], } @@ -259,6 +265,106 @@ def test_enroll_skips_transitions_when_node_already_available(mocker): fake_ironic.node.wait_for_provision_state.assert_not_called() +def test_enroll_steps_available_node_down_to_update_ports(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="available" + ) + # port1 needs an update; port2 already matches. + stale_port1 = existing_port( + "port1", "00:11:22:33:44:55", "old-switch.example.net", "Ethernet9/9" + ) + fake_ironic.port.list.return_value = [ + stale_port1, + existing_port( + "port2", "00:11:22:33:44:66", "spine02.example.net", "Ethernet1/2" + ), + ] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + # Ironic forbids port connectivity changes while available, so the node is + # stepped available -> manageable, the port is updated, then provided back. + fake_ironic.node.set_provision_state.assert_has_calls( + [ + call( + "node-123", + "manage", + cleansteps=None, + runbook=None, + disable_ramdisk=None, + ), + call( + "node-123", + "provide", + cleansteps=None, + runbook=None, + disable_ramdisk=None, + ), + ] + ) + fake_ironic.port.update.assert_called_once() + + +def test_enroll_is_true_noop_when_available_and_everything_matches(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", + driver="netdev", + provision_state="available", + resource_class="generic", + extra={}, + ) + fake_ironic.port.list.return_value = matching_ports() + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + # Nothing drifted: no metadata patch, no port work, no state churn. + fake_ironic.node.update.assert_not_called() + fake_ironic.port.update.assert_not_called() + fake_ironic.port.create.assert_not_called() + fake_ironic.node.set_provision_state.assert_not_called() + + +def test_enroll_updates_metadata_only_on_available_node(mocker, caplog): + caplog.set_level(logging.INFO) + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", + driver="netdev", + provision_state="available", + resource_class="old-class", + extra={}, + ) + fake_ironic.port.list.return_value = matching_ports() + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS, resource_class="new-class") + + # resource_class changed: node is patched, but no port work and no + # transition, and the log does not claim "nothing to do". + fake_ironic.node.update.assert_called_once_with( + "node-123", + [{"op": "add", "path": "/resource_class", "value": "new-class"}], + ) + fake_ironic.node.set_provision_state.assert_not_called() + assert "nothing to do" not in caplog.text + + def test_enroll_fails_on_existing_node_with_other_driver(mocker): fake_ironic, _ = make_ironic_client() fake_ironic.node.get.side_effect = None @@ -304,10 +410,15 @@ def test_enroll_rejects_duplicate_port_macs(mocker): return_value=fake_ironic, ) - args = dict(ENROLL_ARGS) - args["port2_mac"] = args["port1_mac"] + args = { + **ENROLL_ARGS, + "ports": [ + {"mac": "00:11:22:33:44:55", "switch": "s1", "intf": "e1"}, + {"mac": "00:11:22:33:44:55", "switch": "s2", "intf": "e2"}, + ], + } - with pytest.raises(ValueError, match="must be different"): + with pytest.raises(ValueError, match="Duplicate MAC"): enroll_netdev.enroll(**args) # Fail fast, before any Ironic calls. @@ -315,6 +426,157 @@ def test_enroll_rejects_duplicate_port_macs(mocker): fake_ironic.node.create.assert_not_called() +def test_enroll_creates_n_ports(mocker): + fake_ironic, node = make_ironic_client() + fake_ironic.port.create.side_effect = [ + SimpleNamespace(uuid="port-1"), + SimpleNamespace(uuid="port-2"), + SimpleNamespace(uuid="port-3"), + ] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll( + name="leaf01", + physical_network="f20-1-network", + ports=[ + {"mac": "00:11:22:33:44:55", "switch": "s1", "intf": "Eth1/1"}, + {"mac": "00:11:22:33:44:66", "switch": "s2", "intf": "Eth1/2"}, + {"mac": "00:11:22:33:44:77", "switch": "s3", "intf": "Eth1/3"}, + ], + ) + + assert fake_ironic.port.create.call_count == 3 + created_names = [c.kwargs["name"] for c in fake_ironic.port.create.call_args_list] + assert created_names == ["leaf01:port1", "leaf01:port2", "leaf01:port3"] + + +def test_enroll_warns_and_keeps_orphan_ports(mocker, caplog): + caplog.set_level(logging.WARNING) + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="manageable" + ) + orphan = existing_port( + "port9", "00:99:99:99:99:99", "old.example.net", "Ethernet9/9" + ) + fake_ironic.port.list.return_value = [*matching_ports(), orphan] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + fake_ironic.port.delete.assert_not_called() + assert "00:99:99:99:99:99 name=leaf01:port9 is not in the request" in caplog.text + + +def test_enroll_shrinking_ports_leaves_extra_as_orphan(mocker, caplog): + caplog.set_level(logging.WARNING) + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="manageable" + ) + # Node currently has 3 ports; the request only lists the first two. + port3 = existing_port( + "port3", "00:11:22:33:44:77", "spine03.example.net", "Ethernet1/3" + ) + fake_ironic.port.list.return_value = [*matching_ports(), port3] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + # port3 is not in the request: warned about, left in place, not touched. + fake_ironic.port.delete.assert_not_called() + fake_ironic.port.update.assert_not_called() + assert "00:11:22:33:44:77 name=leaf01:port3 is not in the request" in caplog.text + + +def test_enroll_removing_middle_port_fails_safely(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="manageable" + ) + # Existing: port1=..:55, port2=..:66, port3=..:77 (named leaf01:port1/2/3). + fake_ironic.port.list.return_value = [ + *matching_ports(), + existing_port( + "port3", "00:11:22:33:44:77", "spine03.example.net", "Ethernet1/3" + ), + ] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + # Operator drops the middle port and submits [..:55, ..:77]. Positionally + # ..:77 becomes port2, but leaf01:port2 already exists with MAC ..:66. + args = { + **ENROLL_ARGS, + "ports": [ + { + "mac": "00:11:22:33:44:55", + "switch": "spine01.example.net", + "intf": "Ethernet1/1", + }, + { + "mac": "00:11:22:33:44:77", + "switch": "spine03.example.net", + "intf": "Ethernet1/3", + }, + ], + } + + with pytest.raises(RuntimeError, match="already used by port"): + enroll_netdev.enroll(**args) + + fake_ironic.port.update.assert_not_called() + fake_ironic.port.create.assert_not_called() + + +def test_build_netdev_ports_rejects_unknown_fields(): + with pytest.raises(ValueError, match="unknown field"): + enroll_netdev.build_netdev_ports( + [ + { + "label": "uplink-a", + "mac": "00:11:22:33:44:55", + "switch": "s1", + "intf": "e1", + } + ] + ) + + +def test_build_netdev_ports_requires_fields(): + with pytest.raises(ValueError, match="missing required field"): + enroll_netdev.build_netdev_ports([{"mac": "00:11:22:33:44:55"}]) + + +def test_build_netdev_ports_requires_non_empty(): + with pytest.raises(ValueError, match="At least one port"): + enroll_netdev.build_netdev_ports([]) + + +def test_parse_ports_arg_rejects_non_json(): + with pytest.raises(ValueError, match="must be valid JSON"): + enroll_netdev.parse_ports_arg("not-json") + + +def test_parse_ports_arg_rejects_non_array(): + with pytest.raises(ValueError, match="must be a JSON array"): + enroll_netdev.parse_ports_arg('{"mac": "x"}') + + def test_enroll_rejects_port_name_conflict(mocker): fake_ironic, _ = make_ironic_client() fake_ironic.node.get.side_effect = None @@ -336,6 +598,8 @@ def test_enroll_rejects_port_name_conflict(mocker): enroll_netdev.enroll(**ENROLL_ARGS) fake_ironic.port.create.assert_not_called() + # Plan-first: a port conflict must not leave the node metadata patched. + fake_ironic.node.update.assert_not_called() def test_enroll_updates_port_with_null_local_link_connection(mocker): @@ -408,6 +672,87 @@ def test_enroll_converges_existing_port_missing_category(mocker): ) +def test_enroll_preserves_real_switch_id_when_only_metadata_changes(mocker): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="manageable" + ) + # port1 already has a real switch_id and a stale switch_info/port_id. + port1 = existing_port( + "port1", "00:11:22:33:44:55", "old-switch.example.net", "Ethernet9/9" + ) + port1.local_link_connection = { + "switch_id": "aa:bb:cc:dd:ee:ff", + "switch_info": "old-switch.example.net", + "port_id": "Ethernet9/9", + } + fake_ironic.port.list.return_value = [ + port1, + existing_port( + "port2", "00:11:22:33:44:66", "spine02.example.net", "Ethernet1/2" + ), + ] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + # switch_info/port_id converge to the request, but the real switch_id is + # kept rather than being reset to the placeholder. + fake_ironic.port.update.assert_called_once_with( + "uuid-port1", + [ + { + "op": "add", + "path": "/local_link_connection", + "value": { + "switch_id": "aa:bb:cc:dd:ee:ff", + "switch_info": "spine01.example.net", + "port_id": "Ethernet1/1", + }, + }, + ], + ) + + +def test_enroll_does_not_touch_port_with_real_switch_id_and_matching_metadata( + mocker, +): + fake_ironic, _ = make_ironic_client() + fake_ironic.node.get.side_effect = None + fake_ironic.node.get.return_value = SimpleNamespace( + uuid="node-123", driver="netdev", provision_state="available" + ) + port1 = existing_port( + "port1", "00:11:22:33:44:55", "spine01.example.net", "Ethernet1/1" + ) + port1.local_link_connection = { + "switch_id": "aa:bb:cc:dd:ee:ff", + "switch_info": "spine01.example.net", + "port_id": "Ethernet1/1", + } + fake_ironic.port.list.return_value = [ + port1, + existing_port( + "port2", "00:11:22:33:44:66", "spine02.example.net", "Ethernet1/2" + ), + ] + mocker.patch( + "understack_workflows.ironic.client.get_ironic_client", + return_value=fake_ironic, + ) + + enroll_netdev.enroll(**ENROLL_ARGS) + + # A real switch_id with otherwise-matching metadata is a no-op: no port + # update and no state churn on the available node. + fake_ironic.port.update.assert_not_called() + fake_ironic.node.set_provision_state.assert_not_called() + + def test_argument_parser_defaults_resource_class_to_generic(): args = enroll_netdev.argument_parser().parse_args( [ @@ -415,20 +760,13 @@ def test_argument_parser_defaults_resource_class_to_generic(): "leaf01", "--physical-network", "f20-1-network", - "--port1-mac", - "00:11:22:33:44:55", - "--port1-switch", - "spine01.example.net", - "--port1-intf", - "Ethernet1/1", - "--port2-mac", - "00:11:22:33:44:66", - "--port2-switch", - "spine02.example.net", - "--port2-intf", - "Ethernet1/2", + "--ports", + '[{"mac": "00:11:22:33:44:55", "switch": "s1", "intf": "e1"}]', ] ) assert args.resource_class == "generic" assert args.external_cmdb_id == "" + assert enroll_netdev.parse_ports_arg(args.ports) == [ + {"mac": "00:11:22:33:44:55", "switch": "s1", "intf": "e1"} + ] diff --git a/python/understack-workflows/understack_workflows/main/enroll_netdev.py b/python/understack-workflows/understack_workflows/main/enroll_netdev.py index 029b3de67..65f4234df 100644 --- a/python/understack-workflows/understack_workflows/main/enroll_netdev.py +++ b/python/understack-workflows/understack_workflows/main/enroll_netdev.py @@ -1,4 +1,5 @@ import argparse +import json import logging import os from dataclasses import dataclass @@ -21,6 +22,12 @@ # and we are not touching it. REENROLLABLE_STATES = {"enroll", "manageable", "available"} +# Required keys for each entry in the --ports list. These are also the only +# accepted keys: labels are positional (portN), not user-controlled, so a +# stray "label" (or any other key) is rejected rather than silently ignored. +REQUIRED_PORT_FIELDS = ("mac", "switch", "intf") +ALLOWED_PORT_FIELDS = frozenset(REQUIRED_PORT_FIELDS) + @dataclass(frozen=True) class NetdevPort: @@ -39,12 +46,16 @@ def _has_cmdb_id(external_cmdb_id: int | str | None) -> bool: def main() -> None: - """Create a netdev baremetal node, its 2 ports, and make it available. + """Create a netdev baremetal node, its ports, and make it available. + + Re-running with the same parameters is safe: an existing node with the same + name is reused (and updated), existing ports are matched by MAC address and + updated in place, and provision state transitions are only performed when + needed. - Re-running with the same parameters is safe: an existing node with the - same name is reused (and updated), existing ports are matched by MAC - address and updated in place, and provision state transitions are only - performed when needed. + Any number of ports may be supplied via --ports. Ports are named + positionally (portN = Nth entry), so the list order must stay stable across + runs. """ helpers.setup_logger() args = argument_parser().parse_args() @@ -52,37 +63,80 @@ def main() -> None: enroll( name=args.name, physical_network=args.physical_network, - port1_mac=args.port1_mac, - port1_switch=args.port1_switch, - port1_intf=args.port1_intf, - port2_mac=args.port2_mac, - port2_switch=args.port2_switch, - port2_intf=args.port2_intf, + ports=parse_ports_arg(args.ports), external_cmdb_id=args.external_cmdb_id, resource_class=args.resource_class, ) +def parse_ports_arg(raw: str) -> list[dict]: + """Parse the --ports argument (a JSON array) into a list of dicts.""" + try: + data = json.loads(raw) + except json.JSONDecodeError as exc: + raise ValueError(f"--ports must be valid JSON: {exc}") from exc + if not isinstance(data, list): + raise ValueError("--ports must be a JSON array of port objects") + return data + + +def build_netdev_ports(ports: list[dict]) -> list[NetdevPort]: + """Validate the ports list and return NetdevPort objects. + + Labels are positional (port1, port2, ... portN): the Nth entry is always + named portN, so the caller must keep the list order stable across runs. + MAC addresses must be unique within the request. + """ + if not ports: + raise ValueError("At least one port must be provided") + + result: list[NetdevPort] = [] + seen_macs: set[str] = set() + for index, entry in enumerate(ports, start=1): + if not isinstance(entry, dict): + raise ValueError(f"Port {index} must be a JSON object") + missing = [key for key in REQUIRED_PORT_FIELDS if not entry.get(key)] + if missing: + raise ValueError( + f"Port {index} is missing required field(s): {', '.join(missing)}" + ) + unknown = set(entry) - ALLOWED_PORT_FIELDS + if unknown: + raise ValueError( + f"Port {index} has unknown field(s): {', '.join(sorted(unknown))}" + ) + mac_key = entry["mac"].lower() + if mac_key in seen_macs: + raise ValueError(f"Duplicate MAC {entry['mac']} in ports list") + seen_macs.add(mac_key) + result.append( + NetdevPort( + label=f"port{index}", + mac=entry["mac"], + switch=entry["switch"], + interface=entry["intf"], + ) + ) + return result + + def enroll( *, name: str, physical_network: str, - port1_mac: str, - port1_switch: str, - port1_intf: str, - port2_mac: str, - port2_switch: str, - port2_intf: str, + ports: list[dict], external_cmdb_id: int | str | None = None, resource_class: str | None = DEFAULT_RESOURCE_CLASS, ) -> None: effective_resource_class = resource_class or DEFAULT_RESOURCE_CLASS + netdev_ports = build_netdev_ports(ports) logger.info( "Starting enroll-netdev workflow name=%s physical_network=%s " - "resource_class=%s", + "resource_class=%s port_count=%s", name, physical_network, effective_resource_class, + len(netdev_ports), ) if _has_cmdb_id(external_cmdb_id): @@ -93,13 +147,8 @@ def enroll( else: logger.info("No external_cmdb_id provided") - if port1_mac.lower() == port2_mac.lower(): - raise ValueError( - f"port1_mac and port2_mac must be different, both are {port1_mac}" - ) - client = IronicClient() - node = find_or_create_netdev_node( + node, created = find_or_create_netdev_node( client=client, name=name, resource_class=effective_resource_class, @@ -110,13 +159,12 @@ def enroll( ports_by_mac = {(p.address or "").lower(): p for p in node_ports} ports_by_name = {p.name: p for p in node_ports if p.name} - ports = [ - NetdevPort("port1", port1_mac, port1_switch, port1_intf), - NetdevPort("port2", port2_mac, port2_switch, port2_intf), - ] - for port in ports: - find_or_create_netdev_port( - client=client, + # Plan first (no mutations): decide which ports need create/update. This + # lets us skip all work when a re-run has nothing to change, only step the + # node's provision state when we actually have port work to do, and fail on + # a port conflict before mutating anything. + plans = [ + plan_netdev_port( node=node, node_name=name, physical_network=physical_network, @@ -124,8 +172,61 @@ def enroll( existing=ports_by_mac.get(port.mac.lower()), ports_by_name=ports_by_name, ) + for port in netdev_ports + ] + pending = [plan for plan in plans if plan is not None] + + warn_orphan_ports(node, node_ports, netdev_ports) + + # Apply the reused node's metadata only after planning has validated, so a + # port conflict does not leave resource_class/external_cmdb_id half-updated. + # A freshly created node already has these set from create. + metadata_changed = False + if not created: + metadata_changed = update_node_metadata( + client=client, + node=node, + resource_class=effective_resource_class, + external_cmdb_id=external_cmdb_id, + ) + + state = node.provision_state + + if not pending and state == "available": + if metadata_changed: + logger.info( + "[node:%s] Node metadata updated; already available with no " + "port changes", + node.uuid, + ) + else: + logger.info( + "[node:%s] Node already available and up to date, " "nothing to do", + node.uuid, + ) + return + + # Ironic refuses port connectivity changes (local_link_connection, + # physical_network, ...) while the node is "available", so step it back to + # "manageable" before applying any port work, then provide it again below. + if pending and state == "available": + logger.info( + "[node:%s] Stepping available -> manageable to apply port changes", + node.uuid, + ) + ironic_node.transition(node, target_state="manage", expected_state="manageable") + state = "manageable" + + for plan in pending: + apply_port_plan( + client=client, + node=node, + node_name=name, + physical_network=physical_network, + plan=plan, + ) - make_available(node) + make_available(node, state) logger.info( "Completed enroll-netdev workflow name=%s node_uuid=%s", name, @@ -133,22 +234,50 @@ def enroll( ) +def warn_orphan_ports( + node: Node, node_ports: list, netdev_ports: list[NetdevPort] +) -> None: + """Warn about existing ports on the node that are not in the request. + + These are left in place rather than deleted: removing ports on a re-run + would let a shortened request silently destroy data. + """ + requested = {port.mac.lower() for port in netdev_ports} + for existing in node_ports: + if (existing.address or "").lower() not in requested: + logger.warning( + "[node:%s] Existing port uuid=%s mac=%s name=%s is not in the " + "request; leaving it in place", + node.uuid, + existing.uuid, + existing.address, + existing.name, + ) + + def find_or_create_netdev_node( *, client: IronicClient, name: str, resource_class: str, external_cmdb_id: int | str | None = None, -) -> Node: +) -> tuple[Node, bool]: + """Find an existing netdev node by name, or create one. + + Returns (node, created). A reused node is validated (driver and provision + state) but not mutated here; its metadata is applied later by the caller + once port planning has succeeded. + """ try: node = client.get_node(name) except ironic_exceptions.NotFound: - return create_netdev_node( + node = create_netdev_node( client=client, name=name, resource_class=resource_class, external_cmdb_id=external_cmdb_id, ) + return node, True if node.driver != "netdev": raise RuntimeError( @@ -169,11 +298,34 @@ def find_or_create_netdev_node( name, node.provision_state, ) - updates = [f"resource_class={resource_class}"] + return node, False + + +def update_node_metadata( + *, + client: IronicClient, + node: Node, + resource_class: str, + external_cmdb_id: int | str | None = None, +) -> bool: + """Patch resource_class/external_cmdb_id only if they differ from the node. + + Returns True if a patch was sent, so the caller can report accurately. + """ + updates = [] + if getattr(node, "resource_class", None) != resource_class: + updates.append(f"resource_class={resource_class}") if _has_cmdb_id(external_cmdb_id): - updates.append(f"extra/external_cmdb_id={external_cmdb_id}") + current = (getattr(node, "extra", None) or {}).get("external_cmdb_id") + if current != external_cmdb_id: + updates.append(f"extra/external_cmdb_id={external_cmdb_id}") + + if not updates: + return False + + logger.info("[node:%s] Updating node metadata %s", node.uuid, updates) client.update_node(node.uuid, args_array_to_patch("add", updates)) - return node + return True def create_netdev_node( @@ -205,16 +357,24 @@ def create_netdev_node( return node -def find_or_create_netdev_port( +def plan_netdev_port( *, - client: IronicClient, node: Node, node_name: str, physical_network: str, port: NetdevPort, existing=None, ports_by_name: dict | None = None, -) -> None: +) -> dict | None: + """Decide what a single port needs, without mutating anything. + + Returns a plan dict describing the action, or None if the port already + matches the request. Raises on a name/MAC conflict. + + Plan shapes: + {"kind": "create", "port": NetdevPort} + {"kind": "update", "existing": Port, "patch": [json-patch ops]} + """ port_name = f"{node_name}:{port.label}" ports_by_name = ports_by_name or {} @@ -225,21 +385,24 @@ def find_or_create_netdev_port( f"[node:{node.uuid}] Port {port_name} already exists with MAC " f"{name_clash.address}, but requested MAC is {port.mac}" ) - create_netdev_port( - client=client, - node=node, - node_name=node_name, - physical_network=physical_network, - port=port, - ) - return + return {"kind": "create", "port": port} + + current_llc = existing.local_link_connection or {} + + # Preserve a real switch_id once it has been recorded: only (re)write it + # while the stored value is still the placeholder (or unset). switch_info + # and port_id always converge to the request. + current_switch_id = current_llc.get("switch_id") + if current_switch_id in (None, "", PLACEHOLDER_SWITCH_ID): + switch_id = PLACEHOLDER_SWITCH_ID + else: + switch_id = current_switch_id desired_llc = { - "switch_id": PLACEHOLDER_SWITCH_ID, + "switch_id": switch_id, "switch_info": port.switch, "port_id": port.interface, } - current_llc = existing.local_link_connection or {} patch = [] if existing.name != port_name: @@ -271,13 +434,41 @@ def find_or_create_netdev_port( port_name, port.mac, ) + return None + + return {"kind": "update", "existing": existing, "patch": patch} + + +def apply_port_plan( + *, + client: IronicClient, + node: Node, + node_name: str, + physical_network: str, + plan: dict, +) -> None: + """Apply one plan produced by plan_netdev_port. + + The node must already be in a state that permits port connectivity changes + (enroll or manageable); the caller is responsible for that. + """ + if plan["kind"] == "create": + create_netdev_port( + client=client, + node=node, + node_name=node_name, + physical_network=physical_network, + port=plan["port"], + ) return + existing = plan["existing"] + patch = plan["patch"] logger.info( "[node:%s] Updating existing port uuid=%s mac=%s patch=%s", node.uuid, existing.uuid, - port.mac, + existing.address, patch, ) client.update_port(existing.uuid, patch) @@ -326,9 +517,13 @@ def create_netdev_port( ) -def make_available(node: Node) -> None: - state = node.provision_state +def make_available(node: Node, state: str) -> None: + """Drive the node to "available" given its current provision state. + ``state`` is passed in rather than read from the node because earlier steps + may have already transitioned it (e.g. available -> manageable) without + refreshing the local node object. + """ if state == "available": logger.info("[node:%s] Node is already available", node.uuid) return @@ -366,19 +561,14 @@ def argument_parser(): required=True, help="Port physical_network", ) - parser.add_argument("--port1-mac", required=True, help="MAC address for port1") - parser.add_argument("--port1-switch", required=True, help="Switch name for port1") - parser.add_argument( - "--port1-intf", - required=True, - help="Switch interface name for port1", - ) - parser.add_argument("--port2-mac", required=True, help="MAC address for port2") - parser.add_argument("--port2-switch", required=True, help="Switch name for port2") parser.add_argument( - "--port2-intf", + "--ports", required=True, - help="Switch interface name for port2", + help=( + "JSON array of ports in order (portN = Nth entry), e.g. " + '[{"mac": "..", "switch": "..", "intf": ".."}]. ' + "Keep the list order stable across runs." + ), ) parser.add_argument( "--external-cmdb-id", diff --git a/workflows/argo-events/workflowtemplates/enroll-netdev.yaml b/workflows/argo-events/workflowtemplates/enroll-netdev.yaml index 17bad87b1..a797512fe 100644 --- a/workflows/argo-events/workflowtemplates/enroll-netdev.yaml +++ b/workflows/argo-events/workflowtemplates/enroll-netdev.yaml @@ -21,12 +21,10 @@ spec: parameters: - name: name - name: physical_network - - name: port1_mac - - name: port1_switch - - name: port1_intf - - name: port2_mac - - name: port2_switch - - name: port2_intf + # JSON array of ports in order (portN = Nth entry). Keep the order + # stable across runs, e.g. + # [{"mac":"..","switch":"..","intf":".."},{"mac":"..","switch":"..","intf":".."}] + - name: ports - name: external_cmdb_id value: "" - name: resource_class @@ -46,18 +44,8 @@ spec: - "{{workflow.parameters.name}}" - --physical-network - "{{workflow.parameters.physical_network}}" - - --port1-mac - - "{{workflow.parameters.port1_mac}}" - - --port1-switch - - "{{workflow.parameters.port1_switch}}" - - --port1-intf - - "{{workflow.parameters.port1_intf}}" - - --port2-mac - - "{{workflow.parameters.port2_mac}}" - - --port2-switch - - "{{workflow.parameters.port2_switch}}" - - --port2-intf - - "{{workflow.parameters.port2_intf}}" + - --ports + - "{{workflow.parameters.ports}}" - --external-cmdb-id - "{{workflow.parameters.external_cmdb_id}}" - --resource-class