From bd8fb9f95e6b03b472294c5ed6df3b9143d64280 Mon Sep 17 00:00:00 2001 From: Doug Goldstein Date: Thu, 20 Aug 2026 16:32:33 -0500 Subject: [PATCH 1/3] docs: document Ironic node extra.external_cmdb_id field The Ironic node `extra` dict carries UnderStack-specific metadata that the Nautobot device sync consumes, but it was previously undocumented. Document the `extra` field and the `external_cmdb_id` key as they exist today. --- docs/operator-guide/openstack-ironic.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/operator-guide/openstack-ironic.md b/docs/operator-guide/openstack-ironic.md index e5bef2851..b64ec02eb 100644 --- a/docs/operator-guide/openstack-ironic.md +++ b/docs/operator-guide/openstack-ironic.md @@ -9,6 +9,23 @@ The primary Ironic objects you'll interact with are: - **Nodes**: Represent physical servers, containing hardware specifications, BMC credentials, and provisioning state - **Ports**: Represent physical network connections to switches, identified by MAC addresses +### Node `extra` metadata + +A node's `extra` field is a free-form dictionary on the Ironic node. UnderStack stores a small set +of well-known keys there, which the Nautobot device sync reads when reconciling the node into +Nautobot. Keys that UnderStack does not recognize are ignored by the sync. + +The following key is consumed today: + +- `external_cmdb_id` — an identifier for the node in an external CMDB. During sync it is copied to + the Nautobot device custom field of the same name (`external_cmdb_id`). + +Set it with: + +```bash +openstack baremetal node set ${NODE_UUID} --extra external_cmdb_id=CMDB-000000 +``` + ### Hardware Enrollment Hardware enrollment is an automated process in UnderStack. For details on how servers are discovered and enrolled, see [TODO: Hardware Enrollment Documentation]. From 47f6c43affb9819c1984b12116673366d08b0c79 Mon Sep 17 00:00:00 2001 From: Doug Goldstein Date: Thu, 20 Aug 2026 17:06:09 -0500 Subject: [PATCH 2/3] feat(understack-workflows): resolve Nautobot Location from region Resolve the OpenStack region this deployment manages to its Nautobot Location once, up front, and thread that Location object down to fetch_node_details so location-scoped Nautobot lookups can be scoped to it (e.g. resolving a rack). On the event path, handle_node_event now uses the OpenStack connection it already receives (conn.config.region_name) instead of discarding it. On the resync CLI path the region is derived from the same clouds.yaml/env the IronicClient uses. In both cases get_location_for_region looks up the Location and fails fast if the region does not resolve to exactly one Nautobot Location. No node-sync behavior change yet, but objects and lookups that should be location-scoped can and should use this. --- .../tests/test_nautobot_device_sync.py | 64 ++++++++++++++----- .../tests/test_resync_ironic_to_nautobot.py | 19 ++++-- .../main/resync_ironic_to_nautobot.py | 8 ++- .../oslo_event/nautobot_device_sync.py | 44 +++++++++++-- 4 files changed, 111 insertions(+), 24 deletions(-) diff --git a/python/understack-workflows/tests/test_nautobot_device_sync.py b/python/understack-workflows/tests/test_nautobot_device_sync.py index 976b7758f..2de9231bf 100644 --- a/python/understack-workflows/tests/test_nautobot_device_sync.py +++ b/python/understack-workflows/tests/test_nautobot_device_sync.py @@ -508,13 +508,22 @@ class TestSyncDeviceToNautobot: def mock_nautobot(self): return MagicMock() + @pytest.fixture + def location(self): + return MagicMock() + @patch("understack_workflows.oslo_event.nautobot_device_sync.IronicClient") @patch("understack_workflows.oslo_event.nautobot_device_sync.fetch_node_details") @patch( "understack_workflows.oslo_event.nautobot_device_sync.sync_interfaces_from_data" ) def test_sync_creates_new_device( - self, mock_sync_interfaces, mock_fetch, mock_ironic_class, mock_nautobot + self, + mock_sync_interfaces, + mock_fetch, + mock_ironic_class, + mock_nautobot, + location, ): node_uuid = str(uuid.uuid4()) device_info = DeviceInfo( @@ -530,7 +539,7 @@ def test_sync_creates_new_device( mock_nautobot.dcim.devices.create.return_value = MagicMock() mock_sync_interfaces.return_value = EXIT_STATUS_SUCCESS - result = sync_device_to_nautobot(node_uuid, mock_nautobot) + result = sync_device_to_nautobot(node_uuid, mock_nautobot, location) assert result == EXIT_STATUS_SUCCESS mock_nautobot.dcim.devices.create.assert_called_once() @@ -541,7 +550,12 @@ def test_sync_creates_new_device( "understack_workflows.oslo_event.nautobot_device_sync.sync_interfaces_from_data" ) def test_sync_updates_existing_device( - self, mock_sync_interfaces, mock_fetch, mock_ironic_class, mock_nautobot + self, + mock_sync_interfaces, + mock_fetch, + mock_ironic_class, + mock_nautobot, + location, ): node_uuid = str(uuid.uuid4()) device_info = DeviceInfo( @@ -562,20 +576,20 @@ def test_sync_updates_existing_device( mock_nautobot.dcim.devices.get.return_value = existing_device mock_sync_interfaces.return_value = EXIT_STATUS_SUCCESS - result = sync_device_to_nautobot(node_uuid, mock_nautobot) + result = sync_device_to_nautobot(node_uuid, mock_nautobot, location) assert result == EXIT_STATUS_SUCCESS mock_nautobot.dcim.devices.create.assert_not_called() - def test_sync_with_empty_uuid_returns_error(self, mock_nautobot): - result = sync_device_to_nautobot("", mock_nautobot) + def test_sync_with_empty_uuid_returns_error(self, mock_nautobot, location): + result = sync_device_to_nautobot("", mock_nautobot, location) assert result == EXIT_STATUS_FAILURE @patch("understack_workflows.oslo_event.nautobot_device_sync.IronicClient") @patch("understack_workflows.oslo_event.nautobot_device_sync.fetch_node_details") def test_sync_without_location_skips_for_uninspected_node( - self, mock_fetch, mock_ironic_class, mock_nautobot + self, mock_fetch, mock_ironic_class, mock_nautobot, location ): """Test that sync skips gracefully for uninspected nodes without location.""" node_uuid = str(uuid.uuid4()) @@ -583,7 +597,7 @@ def test_sync_without_location_skips_for_uninspected_node( mock_fetch.return_value = (device_info, {}, []) mock_nautobot.dcim.devices.get.return_value = None - result = sync_device_to_nautobot(node_uuid, mock_nautobot) + result = sync_device_to_nautobot(node_uuid, mock_nautobot, location) # Should fail since no location available assert result == EXIT_STATUS_FAILURE @@ -596,7 +610,12 @@ def test_sync_without_location_skips_for_uninspected_node( "understack_workflows.oslo_event.nautobot_device_sync.sync_interfaces_from_data" ) def test_sync_recreates_device_with_mismatched_uuid( - self, mock_sync_interfaces, mock_fetch, mock_ironic_class, mock_nautobot + self, + mock_sync_interfaces, + mock_fetch, + mock_ironic_class, + mock_nautobot, + location, ): """Test device with mismatched UUID is deleted and recreated.""" node_uuid = str(uuid.uuid4()) @@ -622,7 +641,7 @@ def test_sync_recreates_device_with_mismatched_uuid( mock_nautobot.dcim.devices.create.return_value = MagicMock() mock_sync_interfaces.return_value = EXIT_STATUS_SUCCESS - result = sync_device_to_nautobot(node_uuid, mock_nautobot) + result = sync_device_to_nautobot(node_uuid, mock_nautobot, location) assert result == EXIT_STATUS_SUCCESS # Should delete old device @@ -636,7 +655,12 @@ def test_sync_recreates_device_with_mismatched_uuid( "understack_workflows.oslo_event.nautobot_device_sync.sync_interfaces_from_data" ) def test_sync_device_not_found_by_name_creates_new( - self, mock_sync_interfaces, mock_fetch, mock_ironic_class, mock_nautobot + self, + mock_sync_interfaces, + mock_fetch, + mock_ironic_class, + mock_nautobot, + location, ): """Test that device not found by UUID or name is created.""" node_uuid = str(uuid.uuid4()) @@ -655,7 +679,7 @@ def test_sync_device_not_found_by_name_creates_new( mock_nautobot.dcim.devices.create.return_value = MagicMock() mock_sync_interfaces.return_value = EXIT_STATUS_SUCCESS - result = sync_device_to_nautobot(node_uuid, mock_nautobot) + result = sync_device_to_nautobot(node_uuid, mock_nautobot, location) assert result == EXIT_STATUS_SUCCESS mock_nautobot.dcim.devices.create.assert_called_once() @@ -666,7 +690,12 @@ def test_sync_device_not_found_by_name_creates_new( "understack_workflows.oslo_event.nautobot_device_sync.sync_interfaces_from_data" ) def test_sync_uuid_mismatch_uses_old_device_location( - self, mock_sync_interfaces, mock_fetch, mock_ironic_class, mock_nautobot + self, + mock_sync_interfaces, + mock_fetch, + mock_ironic_class, + mock_nautobot, + location, ): """Test that location is preserved from old device when new node has none. @@ -698,7 +727,7 @@ def test_sync_uuid_mismatch_uses_old_device_location( mock_nautobot.dcim.devices.create.return_value = MagicMock() mock_sync_interfaces.return_value = EXIT_STATUS_SUCCESS - result = sync_device_to_nautobot(node_uuid, mock_nautobot) + result = sync_device_to_nautobot(node_uuid, mock_nautobot, location) assert result == EXIT_STATUS_SUCCESS # Should delete old device after preserving location @@ -762,12 +791,17 @@ def test_handle_node_event_success(self, mock_sync, mock_conn, mock_nautobot): } }, } + mock_conn.config.region_name = "iad3" mock_sync.return_value = EXIT_STATUS_SUCCESS result = handle_node_event(mock_conn, mock_nautobot, event_data) assert result == EXIT_STATUS_SUCCESS - mock_sync.assert_called_once_with(node_uuid, mock_nautobot) + # The connection's region resolves a Nautobot Location that is threaded + # through to the sync. + mock_nautobot.dcim.locations.get.assert_called_once_with(name="iad3") + location = mock_nautobot.dcim.locations.get.return_value + mock_sync.assert_called_once_with(node_uuid, mock_nautobot, location) def test_handle_node_event_no_uuid(self, mock_conn, mock_nautobot): event_data = {"payload": {"ironic_object.data": {}}} diff --git a/python/understack-workflows/tests/test_resync_ironic_to_nautobot.py b/python/understack-workflows/tests/test_resync_ironic_to_nautobot.py index bfac33dfb..31023c916 100644 --- a/python/understack-workflows/tests/test_resync_ironic_to_nautobot.py +++ b/python/understack-workflows/tests/test_resync_ironic_to_nautobot.py @@ -21,13 +21,15 @@ def test_default_args(self): class TestSyncNodes: """Test cases for sync_nodes function.""" + @patch("understack_workflows.main.resync_ironic_to_nautobot.get_openstack_client") @patch("understack_workflows.main.resync_ironic_to_nautobot.IronicClient") @patch( "understack_workflows.main.resync_ironic_to_nautobot.sync_device_to_nautobot" ) - def test_sync_all_nodes_success(self, mock_sync, mock_ironic_class): + def test_sync_all_nodes_success(self, mock_sync, mock_ironic_class, mock_get_conn): mock_ironic = MagicMock() mock_ironic_class.return_value = mock_ironic + mock_get_conn.return_value.config.region_name = "iad3" mock_node1 = MagicMock(uuid="uuid-1", name="node-1") mock_node2 = MagicMock(uuid="uuid-2", name="node-2") mock_ironic.list_nodes.return_value = [mock_node1, mock_node2] @@ -39,14 +41,19 @@ def test_sync_all_nodes_success(self, mock_sync, mock_ironic_class): assert result.total == 2 assert result.failed == 0 assert mock_sync.call_count == 2 + # The region's Location is resolved once and passed to each sync call. + location = nautobot.dcim.locations.get.return_value + mock_sync.assert_called_with("uuid-2", nautobot, location) + @patch("understack_workflows.main.resync_ironic_to_nautobot.get_openstack_client") @patch("understack_workflows.main.resync_ironic_to_nautobot.IronicClient") @patch( "understack_workflows.main.resync_ironic_to_nautobot.sync_device_to_nautobot" ) - def test_sync_single_node(self, mock_sync, mock_ironic_class): + def test_sync_single_node(self, mock_sync, mock_ironic_class, mock_get_conn): mock_ironic = MagicMock() mock_ironic_class.return_value = mock_ironic + mock_get_conn.return_value.config.region_name = "iad3" mock_node = MagicMock(uuid="uuid-1", name="node-1") mock_ironic.list_nodes.return_value = [mock_node] mock_sync.return_value = 0 @@ -58,13 +65,15 @@ def test_sync_single_node(self, mock_sync, mock_ironic_class): assert result.failed == 0 mock_ironic.list_nodes.assert_called_once() + @patch("understack_workflows.main.resync_ironic_to_nautobot.get_openstack_client") @patch("understack_workflows.main.resync_ironic_to_nautobot.IronicClient") @patch( "understack_workflows.main.resync_ironic_to_nautobot.sync_device_to_nautobot" ) - def test_sync_with_failures(self, mock_sync, mock_ironic_class): + def test_sync_with_failures(self, mock_sync, mock_ironic_class, mock_get_conn): mock_ironic = MagicMock() mock_ironic_class.return_value = mock_ironic + mock_get_conn.return_value.config.region_name = "iad3" mock_node1 = MagicMock(uuid="uuid-1", name="node-1") mock_node2 = MagicMock(uuid="uuid-2", name="node-2") mock_ironic.list_nodes.return_value = [mock_node1, mock_node2] @@ -77,13 +86,15 @@ def test_sync_with_failures(self, mock_sync, mock_ironic_class): assert result.failed == 1 assert result.succeeded == 1 + @patch("understack_workflows.main.resync_ironic_to_nautobot.get_openstack_client") @patch("understack_workflows.main.resync_ironic_to_nautobot.IronicClient") @patch( "understack_workflows.main.resync_ironic_to_nautobot.sync_device_to_nautobot" ) - def test_sync_no_nodes(self, mock_sync, mock_ironic_class): + def test_sync_no_nodes(self, mock_sync, mock_ironic_class, mock_get_conn): mock_ironic = MagicMock() mock_ironic_class.return_value = mock_ironic + mock_get_conn.return_value.config.region_name = "iad3" mock_ironic.list_nodes.return_value = [] nautobot = MagicMock() diff --git a/python/understack-workflows/understack_workflows/main/resync_ironic_to_nautobot.py b/python/understack-workflows/understack_workflows/main/resync_ironic_to_nautobot.py index cf1c282f9..47de51e76 100644 --- a/python/understack-workflows/understack_workflows/main/resync_ironic_to_nautobot.py +++ b/python/understack-workflows/understack_workflows/main/resync_ironic_to_nautobot.py @@ -14,6 +14,8 @@ from understack_workflows.helpers import parser_nautobot_args from understack_workflows.helpers import setup_logger from understack_workflows.ironic.client import IronicClient +from understack_workflows.openstack.client import get_openstack_client +from understack_workflows.oslo_event.nautobot_device_sync import get_location_for_region from understack_workflows.oslo_event.nautobot_device_sync import sync_device_to_nautobot from understack_workflows.resync import SyncResult from understack_workflows.resync import get_nautobot_client @@ -30,13 +32,17 @@ def argument_parser() -> argparse.ArgumentParser: def sync_nodes(nautobot: pynautobot.api) -> SyncResult: """Sync Ironic nodes to Nautobot.""" ironic = IronicClient() + # Resolve the region's Nautobot Location once; it scopes location-scoped + # lookups (e.g. racks, whose names are only unique within a location). + region = get_openstack_client().config.region_name + location = get_location_for_region(nautobot, region) nodes = ironic.list_nodes() result = SyncResult() for node in nodes: result.total += 1 logger.info("Syncing node: %s (%s)", node.uuid, node.name) - if sync_device_to_nautobot(node.uuid, nautobot) != 0: + if sync_device_to_nautobot(node.uuid, nautobot, location) != 0: result.failed += 1 logger.error("Failed to sync node %s", node.uuid) diff --git a/python/understack-workflows/understack_workflows/oslo_event/nautobot_device_sync.py b/python/understack-workflows/understack_workflows/oslo_event/nautobot_device_sync.py index c0de60273..e5895c00a 100644 --- a/python/understack-workflows/understack_workflows/oslo_event/nautobot_device_sync.py +++ b/python/understack-workflows/understack_workflows/oslo_event/nautobot_device_sync.py @@ -21,6 +21,7 @@ from openstack.connection import Connection from pynautobot import RequestError from pynautobot.core.api import Api as Nautobot +from pynautobot.core.response import Record from understack_workflows.ironic.client import IronicClient from understack_workflows.ironic.provision_state_mapper import ProvisionStateMapper @@ -56,6 +57,32 @@ def _is_retryable_error(exc: BaseException) -> bool: return False +def get_location_for_region(nautobot_client: Nautobot, region: str) -> Record: + """Resolve an OpenStack region name to its Nautobot Location. + + A deployment manages a single OpenStack region, which maps to a single + Nautobot Location (the location name matches the region name). Resolving + it once, up front, lets location-scoped Nautobot lookups be scoped to + this Location. + + Args: + nautobot_client: Nautobot API client + region: OpenStack region name + + Returns: + The Nautobot Location record for the region. + + Raises: + ValueError: if the region does not resolve to exactly one Location. + """ + location = nautobot_client.dcim.locations.get(name=region) + if not location or isinstance(location, list): + raise ValueError( + f"OpenStack region {region!r} did not resolve to a single Nautobot location" + ) + return location + + @dataclass class DeviceInfo: """Complete device information synced to Nautobot. @@ -243,6 +270,7 @@ def fetch_node_details( node_uuid: str, ironic_client: IronicClient, nautobot_client: Nautobot, + location: Record, ) -> tuple[DeviceInfo, dict, list]: """Fetch complete device info from Ironic. @@ -250,6 +278,8 @@ def fetch_node_details( node_uuid: Ironic node UUID ironic_client: Ironic API client nautobot_client: Nautobot API client (for switch location lookup) + location: Nautobot Location for this deployment's region, used to scope + location-scoped Nautobot lookups Returns: Tuple of (DeviceInfo, inventory dict, ports list) @@ -538,6 +568,7 @@ def _find_or_create_nautobot_device( def sync_device_to_nautobot( node_uuid: str, nautobot_client: Nautobot, + location: Record, sync_interfaces: bool = True, ) -> int: """Sync an Ironic node to Nautobot. @@ -553,6 +584,8 @@ def sync_device_to_nautobot( Args: node_uuid: Ironic node UUID nautobot_client: Nautobot API client + location: Nautobot Location for this deployment's region, used to scope + location-scoped Nautobot lookups sync_interfaces: Whether to also sync interfaces (default: True) Returns: @@ -566,7 +599,7 @@ def sync_device_to_nautobot( ironic_client = IronicClient() ironic_node_info, inventory, ports = fetch_node_details( - node_uuid, ironic_client, nautobot_client + node_uuid, ironic_client, nautobot_client, location ) nautobot_device = _find_or_create_nautobot_device( @@ -627,7 +660,7 @@ def _extract_node_uuid_from_event(event_data: dict[str, Any]) -> str | None: def handle_node_event( - _conn: Connection, nautobot_client: Nautobot, event_data: dict[str, Any] + conn: Connection, nautobot_client: Nautobot, event_data: dict[str, Any] ) -> int: """Handle any Ironic node event and sync to Nautobot. @@ -639,7 +672,9 @@ def handle_node_event( - baremetal.node.maintenance_set.end Args: - _conn: OpenStack connection (unused, kept for handler signature) + conn: OpenStack connection; its region resolves the Nautobot Location + used to scope location-scoped lookups to the region this deployment + manages nautobot_client: Nautobot API client event_data: Raw event data dict @@ -654,7 +689,8 @@ def handle_node_event( event_type = event_data.get("event_type", "unknown") logger.info("Handling %s for node %s", event_type, node_uuid) - return sync_device_to_nautobot(node_uuid, nautobot_client) + location = get_location_for_region(nautobot_client, conn.config.region_name) + return sync_device_to_nautobot(node_uuid, nautobot_client, location) def delete_device_from_nautobot(node_uuid: str, nautobot_client: Nautobot) -> int: From c78a59b36f8ebdc6741e525bf259ac8ef8a4254f Mon Sep 17 00:00:00 2001 From: Doug Goldstein Date: Thu, 20 Aug 2026 17:08:41 -0500 Subject: [PATCH 3/3] feat(understack-workflows): Nautobot sync extra.rack and extra.position The Nautobot device sync previously derived a node's rack and location only from the switches its ports are cabled to, and never set the rack-unit position. Allow an explicit placement via the node's Ironic extra field: rack (a Nautobot rack name) and position (the rack unit). When both are present and the rack name resolves to a single Nautobot rack, the sync uses them for rack, location, and position, overriding the switch-based lookup. The lookup is scoped to the region's Nautobot location (a rack name is only unique within a location), an ambiguous match falls back cleanly, and the position is validated before any state is mutated. If either field is missing or the rack can't be resolved, the sync falls back to the existing switch-based behavior. Also stop preserving the old device's position on re-enrollment when extra supplies one, so extra stays authoritative. --- docs/operator-guide/openstack-ironic.md | 14 +- .../tests/test_nautobot_device_sync.py | 187 ++++++++++++++++++ .../oslo_event/nautobot_device_sync.py | 98 ++++++++- 3 files changed, 295 insertions(+), 4 deletions(-) diff --git a/docs/operator-guide/openstack-ironic.md b/docs/operator-guide/openstack-ironic.md index b64ec02eb..83c37a0fe 100644 --- a/docs/operator-guide/openstack-ironic.md +++ b/docs/operator-guide/openstack-ironic.md @@ -15,15 +15,25 @@ A node's `extra` field is a free-form dictionary on the Ironic node. UnderStack of well-known keys there, which the Nautobot device sync reads when reconciling the node into Nautobot. Keys that UnderStack does not recognize are ignored by the sync. -The following key is consumed today: +The following keys are consumed today: - `external_cmdb_id` — an identifier for the node in an external CMDB. During sync it is copied to the Nautobot device custom field of the same name (`external_cmdb_id`). +- `rack` — the **name** of the Nautobot rack the node lives in. When set together with `position`, + the sync places the device in that rack (and its location), overriding the default behavior of + inferring the rack from the switches the node is cabled to. +- `position` — the rack unit the node occupies. Maps to the Nautobot device `position`; the rack + face defaults to `front`. -Set it with: +`rack` and `position` are only applied when **both** are present and the rack name resolves to a +single Nautobot rack. If either is missing (or the rack can't be found), the sync falls back to +deriving the rack and location from the node's connected switches. + +Set them with: ```bash openstack baremetal node set ${NODE_UUID} --extra external_cmdb_id=CMDB-000000 +openstack baremetal node set ${NODE_UUID} --extra rack=RACK-NAME --extra position=12 ``` ### Hardware Enrollment diff --git a/python/understack-workflows/tests/test_nautobot_device_sync.py b/python/understack-workflows/tests/test_nautobot_device_sync.py index 2de9231bf..c1f000670 100644 --- a/python/understack-workflows/tests/test_nautobot_device_sync.py +++ b/python/understack-workflows/tests/test_nautobot_device_sync.py @@ -22,6 +22,9 @@ _populate_from_inventory, ) from understack_workflows.oslo_event.nautobot_device_sync import _populate_from_node +from understack_workflows.oslo_event.nautobot_device_sync import ( + _set_location_from_extra, +) from understack_workflows.oslo_event.nautobot_device_sync import ( _set_location_from_switches, ) @@ -29,6 +32,7 @@ from understack_workflows.oslo_event.nautobot_device_sync import ( delete_device_from_nautobot, ) +from understack_workflows.oslo_event.nautobot_device_sync import fetch_node_details from understack_workflows.oslo_event.nautobot_device_sync import ( handle_node_delete_event, ) @@ -301,6 +305,189 @@ def test_set_location_switch_not_found(self, device_info, mock_nautobot): assert device_info.location_id is None +class TestSetLocationFromExtra: + """Test cases for _set_location_from_extra function.""" + + @pytest.fixture + def device_info(self): + return DeviceInfo(uuid="test-uuid") + + @pytest.fixture + def mock_nautobot(self): + return MagicMock() + + @pytest.fixture + def mock_rack(self): + rack = MagicMock() + rack.id = "rack-uuid" + rack.location.id = "location-uuid" + return rack + + @pytest.fixture + def location(self): + loc = MagicMock() + loc.id = "region-loc-uuid" + loc.name = "iad3" + return loc + + def test_sets_rack_location_and_position( + self, device_info, mock_nautobot, mock_rack, location + ): + node = MagicMock() + node.extra = {"rack": "R1", "position": 12} + mock_nautobot.dcim.racks.get.return_value = mock_rack + + assert ( + _set_location_from_extra(device_info, node, mock_nautobot, location) is True + ) + + # Lookup is scoped to the region's location. + mock_nautobot.dcim.racks.get.assert_called_once_with( + name="R1", location_id="region-loc-uuid" + ) + assert device_info.rack_id == "rack-uuid" + assert device_info.location_id == "location-uuid" + assert device_info.position == 12 + + def test_position_string_is_coerced_to_int( + self, device_info, mock_nautobot, mock_rack, location + ): + node = MagicMock() + node.extra = {"rack": "R1", "position": "12"} + mock_nautobot.dcim.racks.get.return_value = mock_rack + + assert ( + _set_location_from_extra(device_info, node, mock_nautobot, location) is True + ) + assert device_info.position == 12 + + def test_invalid_position_falls_back_before_lookup( + self, device_info, mock_nautobot, location + ): + """A non-integer position falls back without touching device_info.""" + node = MagicMock() + node.extra = {"rack": "R1", "position": "not-a-number"} + + assert ( + _set_location_from_extra(device_info, node, mock_nautobot, location) + is False + ) + # Position is validated before the rack lookup, so no API call or + # partial mutation happens. + mock_nautobot.dcim.racks.get.assert_not_called() + assert device_info.rack_id is None + assert device_info.location_id is None + assert device_info.position is None + + def test_missing_position_falls_back(self, device_info, mock_nautobot, location): + node = MagicMock() + node.extra = {"rack": "R1"} + + assert ( + _set_location_from_extra(device_info, node, mock_nautobot, location) + is False + ) + mock_nautobot.dcim.racks.get.assert_not_called() + assert device_info.rack_id is None + assert device_info.location_id is None + assert device_info.position is None + + def test_missing_rack_falls_back(self, device_info, mock_nautobot, location): + node = MagicMock() + node.extra = {"position": 12} + + assert ( + _set_location_from_extra(device_info, node, mock_nautobot, location) + is False + ) + mock_nautobot.dcim.racks.get.assert_not_called() + assert device_info.rack_id is None + + def test_empty_extra_falls_back(self, device_info, mock_nautobot, location): + node = MagicMock() + node.extra = None + + assert ( + _set_location_from_extra(device_info, node, mock_nautobot, location) + is False + ) + mock_nautobot.dcim.racks.get.assert_not_called() + + def test_rack_not_found_falls_back(self, device_info, mock_nautobot, location): + node = MagicMock() + node.extra = {"rack": "unknown", "position": 12} + mock_nautobot.dcim.racks.get.return_value = None + + assert ( + _set_location_from_extra(device_info, node, mock_nautobot, location) + is False + ) + assert device_info.rack_id is None + assert device_info.location_id is None + assert device_info.position is None + + def test_ambiguous_rack_falls_back(self, device_info, mock_nautobot, location): + """Pynautobot .get() raises ValueError when >1 rack matches.""" + node = MagicMock() + node.extra = {"rack": "R1", "position": 12} + mock_nautobot.dcim.racks.get.side_effect = ValueError( + "get() returned more than one result" + ) + + assert ( + _set_location_from_extra(device_info, node, mock_nautobot, location) + is False + ) + assert device_info.rack_id is None + assert device_info.location_id is None + assert device_info.position is None + + def test_rack_list_result_falls_back(self, device_info, mock_nautobot, location): + """Pynautobot's typing allows .get() to return a list; treat as ambiguous.""" + node = MagicMock() + node.extra = {"rack": "R1", "position": 12} + mock_nautobot.dcim.racks.get.return_value = [MagicMock(), MagicMock()] + + assert ( + _set_location_from_extra(device_info, node, mock_nautobot, location) + is False + ) + assert device_info.rack_id is None + assert device_info.location_id is None + assert device_info.position is None + + def test_extra_overrides_switch_lookup(self, mock_nautobot, mock_rack, location): + """When extra provides rack+position, switches are not consulted.""" + node = MagicMock() + node.properties = {} + node.traits = None + node.provision_state = "active" + node.lessee = None + node.extra = {"rack": "R1", "position": 12} + mock_nautobot.dcim.racks.get.return_value = mock_rack + + ironic_client = MagicMock() + ironic_client.get_node.return_value = node + ironic_client.get_node_inventory.return_value = {} + # A cabled port that would otherwise drive switch-based location. + ironic_client.list_ports.return_value = [ + MagicMock(local_link_connection={"switch_info": "switch1.example.com"}) + ] + + device_info, _, _ = fetch_node_details( + "test-uuid", ironic_client, mock_nautobot, location + ) + + assert device_info.rack_id == "rack-uuid" + assert device_info.location_id == "location-uuid" + assert device_info.position == 12 + mock_nautobot.dcim.racks.get.assert_called_once_with( + name="R1", location_id="region-loc-uuid" + ) + # Switch lookup (dcim.devices.get) must not be used. + mock_nautobot.dcim.devices.get.assert_not_called() + + class TestGetRecordValue: """Test cases for _get_record_value function.""" diff --git a/python/understack-workflows/understack_workflows/oslo_event/nautobot_device_sync.py b/python/understack-workflows/understack_workflows/oslo_event/nautobot_device_sync.py index e5895c00a..ae37a6eae 100644 --- a/python/understack-workflows/understack_workflows/oslo_event/nautobot_device_sync.py +++ b/python/understack-workflows/understack_workflows/oslo_event/nautobot_device_sync.py @@ -207,6 +207,97 @@ def _generate_device_name(device_info: DeviceInfo) -> None: device_info.name = f"{device_info.manufacturer}-{device_info.serial_number}" +def _set_location_from_extra( + device_info: DeviceInfo, + node, + nautobot_client: Nautobot, + location: Record, +) -> bool: + """Determine device location from the node's ``extra`` field. + + Uses ``extra.rack`` (a Nautobot rack name) and ``extra.position`` (the + rack unit the node occupies). Both must be present for this to take + effect; when either is missing, or the rack cannot be resolved in + Nautobot, this returns ``False`` so the caller falls back to deriving + location from the connected switches. + + Rack names are only unique within a location, so the lookup is scoped to + ``location`` (this deployment's region). + + Args: + device_info: DeviceInfo to update with location, rack and position + node: Ironic node object + nautobot_client: Nautobot API client + location: Nautobot Location used to scope the rack lookup + + Returns: + True if rack, location and position were set from ``extra``. + """ + extra = node.extra or {} + rack_name = extra.get("rack") + position = extra.get("position") + + # Both are required; otherwise fall back to switch-based lookup. + if not rack_name or position is None: + return False + + # Validate position before touching device_info so a bad value can't leave + # a half-applied placement. + try: + rack_position = int(position) + except (TypeError, ValueError): + logger.warning( + "extra.position %r for node %s is not an integer, falling back " + "to switch-based location", + position, + device_info.uuid, + ) + return False + + try: + # Scope by location: rack names are only unique within a location. + try: + rack = nautobot_client.dcim.racks.get( + name=rack_name, location_id=location.id + ) + except ValueError: + # pynautobot's .get() raises when more than one rack matches. + logger.warning( + "extra.rack %s (location %s) for node %s matched multiple " + "Nautobot racks, falling back to switch-based location", + rack_name, + location.name, + device_info.uuid, + ) + return False + + # pynautobot's typing allows .get() to return a list; treat that (and + # an empty result) as "did not resolve to a single rack". + if not rack or isinstance(rack, list): + logger.warning( + "extra.rack %s (location %s) for node %s did not resolve to a " + "single Nautobot rack, falling back to switch-based location", + rack_name, + location.name, + device_info.uuid, + ) + return False + + device_info.rack_id = rack.id + device_info.location_id = _get_record_value(rack.location, "id") + device_info.position = rack_position + return True + + except Exception as e: + # Re-raise retryable errors (503, connection issues) to trigger retry + if _is_retryable_error(e): + raise + logger.error( + "Failed to set location from extra for node %s: %s", device_info.uuid, e + ) + return False + + def _set_location_from_switches( device_info: DeviceInfo, ports: list, @@ -301,7 +392,10 @@ def fetch_node_details( _populate_from_node(device_info, node) _populate_from_inventory(device_info, inventory) _generate_device_name(device_info) - _set_location_from_switches(device_info, ports, nautobot_client) + # Prefer an explicit rack/position from the node's extra field; fall back + # to deriving location from the connected switches when it isn't set. + if not _set_location_from_extra(device_info, node, nautobot_client, location): + _set_location_from_switches(device_info, ports, nautobot_client) return device_info, inventory, ports @@ -485,7 +579,7 @@ def _preserve_location_from_device(device_info: DeviceInfo, nautobot_device) -> device_info.rack_id = old_rack_id logger.info("Preserving rack %s from old device", old_rack_id) - if nautobot_device.position is not None: + if nautobot_device.position is not None and device_info.position is None: device_info.position = nautobot_device.position logger.info("Preserving position %s from old device", nautobot_device.position)