diff --git a/docs/operator-guide/openstack-ironic.md b/docs/operator-guide/openstack-ironic.md index e5bef2851..83c37a0fe 100644 --- a/docs/operator-guide/openstack-ironic.md +++ b/docs/operator-guide/openstack-ironic.md @@ -9,6 +9,33 @@ 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 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`. + +`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 Hardware enrollment is an automated process in UnderStack. For details on how servers are discovered and enrolled, see [TODO: Hardware Enrollment Documentation]. diff --git a/python/understack-workflows/tests/test_nautobot_device_sync.py b/python/understack-workflows/tests/test_nautobot_device_sync.py index 976b7758f..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.""" @@ -508,13 +695,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 +726,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 +737,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 +763,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 +784,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 +797,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 +828,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 +842,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 +866,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 +877,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 +914,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 +978,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..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 @@ -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. @@ -180,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, @@ -243,6 +361,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 +369,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) @@ -271,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 @@ -455,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) @@ -538,6 +662,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 +678,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 +693,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 +754,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 +766,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 +783,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: