From 14235801111b5cf6cd27608167a18add70741c13 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:37:13 +0200 Subject: [PATCH 01/10] tests/enums: pin openstack-core dependency chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing test for collection-openstack-core only verified that keystone, neutron, nova, glance, cinder, and placement were reachable somewhere in the tree. A silent rewiring — e.g. promoting octavia to a direct child of keystone, or decoupling neutron from wait-for-nova — would not have been detected. Add a link-by-link chain test that walks each step of the keystone → neutron → wait-for-nova → octavia path and asserts that each hop resolves to a Role instance with the exact expected name. This pins the structural wiring, not just reachability. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- tests/unit/data/test_enums.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/unit/data/test_enums.py b/tests/unit/data/test_enums.py index 61bb057d..da485f0f 100644 --- a/tests/unit/data/test_enums.py +++ b/tests/unit/data/test_enums.py @@ -254,6 +254,18 @@ def test_map_role2role_collection_openstack_core_includes_core_services(): assert {"keystone", "neutron", "nova", "glance", "cinder", "placement"} <= names +def test_map_role2role_collection_openstack_core_chain_keystone_to_octavia(): + roots = MAP_ROLE2ROLE["collection-openstack-core"] + keystone = find_role(roots, "keystone") + assert keystone is not None and isinstance(keystone, Role) + neutron = find_role(keystone.dependencies, "neutron") + assert neutron is not None and isinstance(neutron, Role) + wait_for_nova = find_role(neutron.dependencies, "wait-for-nova") + assert wait_for_nova is not None and isinstance(wait_for_nova, Role) + octavia = find_role(wait_for_nova.dependencies, "octavia") + assert octavia is not None and isinstance(octavia, Role) + + def test_map_role2role_collection_monitoring_grafana_depends_on_prometheus(): prometheus = find_role(MAP_ROLE2ROLE["collection-monitoring"], "prometheus") From 0026aa4daadfb03905662f0d35d2848f5fd6f02e Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:37:48 +0200 Subject: [PATCH 02/10] conductor/utils: fix KeyError in deep_compare for missing nested key deep_compare raised KeyError when a key in `a` held a dict value but that key was absent from `b`. The dict branch called `b[key]` unconditionally (line 29), while the scalar branch above it had a `key not in b` guard. In production deep_compare diffs a desired-state config against the current-state config. When the current state is missing an entire nested section the call would crash rather than record the addition. Fix by replacing `b[key]` with `b.get(key, {})` so a missing key is treated as an empty dict, causing the subtree from `a` to be recorded in full as a required addition. Add a regression test: `a = {"outer": {"inner": "val", "other": 1}}`, `b = {}` must produce `updates == {"outer": {"inner": "val", "other": 1}}` rather than raising KeyError. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- osism/tasks/conductor/utils.py | 2 +- tests/unit/tasks/conductor/test_utils.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/osism/tasks/conductor/utils.py b/osism/tasks/conductor/utils.py index 7234738a..815a5753 100644 --- a/osism/tasks/conductor/utils.py +++ b/osism/tasks/conductor/utils.py @@ -26,7 +26,7 @@ def deep_compare(a, b, updates): updates[key] = value else: updates[key] = {} - deep_compare(a[key], b[key], updates[key]) + deep_compare(a[key], b.get(key, {}), updates[key]) if not updates[key]: updates.pop(key) diff --git a/tests/unit/tasks/conductor/test_utils.py b/tests/unit/tasks/conductor/test_utils.py index 975a2f57..c3186ac5 100644 --- a/tests/unit/tasks/conductor/test_utils.py +++ b/tests/unit/tasks/conductor/test_utils.py @@ -99,6 +99,16 @@ def test_deep_compare_records_list_value_difference(): assert updates == {"items": [1, 2, 3]} +def test_deep_compare_nested_key_missing_from_b_records_whole_subtree(): + a = {"outer": {"inner": "val", "other": 1}} + b = {} + updates = {} + + deep_compare(a, b, updates) + + assert updates == {"outer": {"inner": "val", "other": 1}} + + # --------------------------------------------------------------------------- # deep_merge # --------------------------------------------------------------------------- From 82b5c9b50fa91e32dfe7fda1cf67e66e2becc3b0 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:38:06 +0200 Subject: [PATCH 03/10] tests/sonic/cache: verify lock acquisition per public method The existing concurrency smoke test checked that two threads do not crash and return consistent results, but it did not verify that each public method of InterfaceCache acquires the lock exactly once per call. A future refactor removing the lock from clear() or get_cache_stats() would pass the full suite undetected. Add three per-method spy tests that replace cache._lock with a MagicMock wrapping a real threading.Lock and assert __enter__.call_count == 1 after a single call to get_device_interfaces, clear, and get_cache_stats respectively. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- .../unit/tasks/conductor/sonic/test_cache.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/unit/tasks/conductor/sonic/test_cache.py b/tests/unit/tasks/conductor/sonic/test_cache.py index 2e69bdd2..be30638d 100644 --- a/tests/unit/tasks/conductor/sonic/test_cache.py +++ b/tests/unit/tasks/conductor/sonic/test_cache.py @@ -294,6 +294,34 @@ def worker(): assert mock_nb.dcim.interfaces.filter.call_count == 1 +def test_get_device_interfaces_acquires_lock_once(mock_nb): + mock_nb.dcim.interfaces.filter.return_value = [] + cache = InterfaceCache() + cache._lock = MagicMock(wraps=threading.Lock()) + + cache.get_device_interfaces(1) + + assert cache._lock.__enter__.call_count == 1 + + +def test_clear_acquires_lock_once(): + cache = InterfaceCache() + cache._lock = MagicMock(wraps=threading.Lock()) + + cache.clear() + + assert cache._lock.__enter__.call_count == 1 + + +def test_get_cache_stats_acquires_lock_once(): + cache = InterfaceCache() + cache._lock = MagicMock(wraps=threading.Lock()) + + cache.get_cache_stats() + + assert cache._lock.__enter__.call_count == 1 + + # --------------------------------------------------------------------------- # get_interface_cache (module-level, thread-local) # --------------------------------------------------------------------------- From 42d0425187cc3849f9783dd6c171368912645a17 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:38:21 +0200 Subject: [PATCH 04/10] tests/sonic/cache: fully reset thread-local in autouse fixture clear_interface_cache() empties the internal _cache dict but leaves _thread_local.interface_cache pointing to the now-empty InterfaceCache instance. After the fixture ran, get_interface_cache_stats() on the main thread returned {} instead of None because the attribute was still set on the thread-local. This could mask a test that asserts get_interface_cache_stats() is None on the main thread after a prior test has populated the cache: the fixture would leave the attribute set and the assertion would see {} and silently pass when it should not. Fix by deleting the thread-local attribute after clearing, using _thread_local.__dict__.pop so that subsequent tests start with no thread-local cache, matching the state of a fresh thread. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- tests/unit/tasks/conductor/sonic/test_cache.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/tasks/conductor/sonic/test_cache.py b/tests/unit/tasks/conductor/sonic/test_cache.py index be30638d..e69bd0a6 100644 --- a/tests/unit/tasks/conductor/sonic/test_cache.py +++ b/tests/unit/tasks/conductor/sonic/test_cache.py @@ -5,6 +5,7 @@ import pytest +import osism.tasks.conductor.sonic.cache as _cache_mod from osism.tasks.conductor.sonic.cache import ( InterfaceCache, clear_interface_cache, @@ -43,6 +44,7 @@ def _reset_thread_local(): """ yield clear_interface_cache() + _cache_mod._thread_local.__dict__.pop("interface_cache", None) # --------------------------------------------------------------------------- From 823ec599451ab36f9fa2fb35490be29819557446 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:39:17 +0200 Subject: [PATCH 05/10] sonic/connections: per-interface exception recovery in get_connected_interfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The outer try/except wrapped the entire interface loop. A single interface that raised from get_connected_device_via_interface would abort processing of all subsequent interfaces for that device. Issue #2205 required per-interface recovery (skip the offending interface, log a warning, continue), but no inner try/except existed. The existing test used only one interface, so it could not distinguish "loop aborted" from "interface skipped" — both produced an empty set and a single warning. Restructure the exception handling: - Outer try/except covers only get_cached_device_interfaces. A failure there returns the empty sets immediately as before. - Inner try/except wraps all per-interface processing — connection lookup, SONiC name conversion, set update, and port-channel mapping. Any failure skips that interface, logs a warning, and continues. Add a regression test with two interfaces: the first raises, the second connects normally. Assert that the second interface still appears in the returned set. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- osism/tasks/conductor/sonic/connections.py | 29 ++++++++++++------- .../tasks/conductor/sonic/test_connections.py | 27 +++++++++++++++++ 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/osism/tasks/conductor/sonic/connections.py b/osism/tasks/conductor/sonic/connections.py index 5c74a187..291bcb45 100644 --- a/osism/tasks/conductor/sonic/connections.py +++ b/osism/tasks/conductor/sonic/connections.py @@ -75,18 +75,24 @@ def get_connected_interfaces( Returns: tuple: (set of connected interfaces, set of connected port channels) """ - connected_interfaces = set() - connected_portchannels = set() + connected_interfaces: Set[str] = set() + connected_portchannels: Set[str] = set() try: # Get all interfaces for the device (using cache) interfaces = get_cached_device_interfaces(device.id) + except Exception as e: + logger.warning( + f"Could not get interface connections for device {device.name}: {e}" + ) + return connected_interfaces, connected_portchannels - for interface in interfaces: - # Skip management-only interfaces - if hasattr(interface, "mgmt_only") and interface.mgmt_only: - continue + for interface in interfaces: + # Skip management-only interfaces + if hasattr(interface, "mgmt_only") and interface.mgmt_only: + continue + try: # Check if interface is connected using connected_endpoints API connected_device = get_connected_device_via_interface(interface, device.id) @@ -104,11 +110,12 @@ def get_connected_interfaces( ): pc_name = portchannel_info["member_mapping"][sonic_interface_name] connected_portchannels.add(pc_name) - - except Exception as e: - logger.warning( - f"Could not get interface connections for device {device.name}: {e}" - ) + except Exception as e: + logger.warning( + f"Could not process interface {getattr(interface, 'name', interface)}" + f" on device {device.name}: {e}" + ) + continue return connected_interfaces, connected_portchannels diff --git a/tests/unit/tasks/conductor/sonic/test_connections.py b/tests/unit/tasks/conductor/sonic/test_connections.py index c3ad8719..1019db87 100644 --- a/tests/unit/tasks/conductor/sonic/test_connections.py +++ b/tests/unit/tasks/conductor/sonic/test_connections.py @@ -239,6 +239,33 @@ def test_get_connected_interfaces_helper_exception_logs_warning(mocker, make_int assert "sw1" in warning.call_args.args[0] +def test_get_connected_interfaces_continues_after_per_interface_exception( + mocker, make_interface +): + """An exception on one interface must not abort processing of subsequent ones.""" + device = SimpleNamespace(id=1, name="sw1") + bad_iface = make_interface(name="Ethernet0") + good_iface = make_interface(name="Ethernet4") + peer = SimpleNamespace(id=2, name="peer") + + mocker.patch( + "osism.tasks.conductor.sonic.connections.get_cached_device_interfaces", + return_value=[bad_iface, good_iface], + ) + mocker.patch( + "osism.tasks.conductor.sonic.connections.get_connected_device_via_interface", + side_effect=[RuntimeError("boom"), peer], + ) + mocker.patch( + "osism.tasks.conductor.sonic.connections.convert_netbox_interface_to_sonic", + return_value="Ethernet4", + ) + + connected, _ = connections.get_connected_interfaces(device) + + assert connected == {"Ethernet4"} + + def test_get_connected_interfaces_cache_lookup_failure_returns_empty(mocker): device = SimpleNamespace(id=1, name="sw1") mocker.patch( From fb3dfd98fba2b2247444264d184bfd97554a122f Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:39:39 +0200 Subject: [PATCH 06/10] sonic/connections: remove unreachable group-size guard find_interconnected_devices had an `if len(group) > 1:` guard around the final append. A device only enters role_graphs when it has at least one in-role peer, so every BFS starting from a node in the graph always collects >= 2 devices. The guard could never be False and the dead branch (silently dropping a group) was never taken. A test comment incorrectly attributed the empty result for a single-spine topology to this guard; the actual reason is that the spine never enters role_graphs at all because it has no in-role peers. Remove the guard and update the test comment to accurately describe the mechanism. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- osism/tasks/conductor/sonic/connections.py | 3 +-- tests/unit/tasks/conductor/sonic/test_connections.py | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/osism/tasks/conductor/sonic/connections.py b/osism/tasks/conductor/sonic/connections.py index 291bcb45..2c6cdfd3 100644 --- a/osism/tasks/conductor/sonic/connections.py +++ b/osism/tasks/conductor/sonic/connections.py @@ -297,8 +297,7 @@ def find_interconnected_devices( visited.add(neighbor_id) queue.append(neighbor_id) - if len(group) > 1: # Only include groups with multiple devices - all_groups.append(group) + all_groups.append(group) return all_groups diff --git a/tests/unit/tasks/conductor/sonic/test_connections.py b/tests/unit/tasks/conductor/sonic/test_connections.py index 1019db87..e486bc1f 100644 --- a/tests/unit/tasks/conductor/sonic/test_connections.py +++ b/tests/unit/tasks/conductor/sonic/test_connections.py @@ -536,8 +536,8 @@ def test_find_interconnected_devices_single_spine_no_peers(make_device, wire_top spine = make_device(1, role_slug="spine") wire_topology(device_interfaces={1: []}, connections_map={}) - # Single device with no peers — the role_graphs entry is empty and - # groups of size 1 are dropped. + # Single device with no in-role peers — the device never enters + # role_graphs, so the BFS has nothing to walk and returns no groups. assert connections.find_interconnected_devices([spine]) == [] From fde40d39219f5b40378b68ce1ea019f9d8941ffc Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:40:12 +0200 Subject: [PATCH 07/10] sonic/interface: fix _extract_port_number_from_alias for Eth(PortN) format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fallback regex used r"(\d+)$" which requires digits at the end of the string. "Eth(Port5)" ends with ")", not a digit, so the regex never matched and the function returned None instead of 5. The test for this case documented None as "actual behavior" with a comment acknowledging the spec violation rather than surfacing the bug. Replace the two-step strategy with three steps: 1. r"Eth(\d+)\(Port(\d+)\)" — existing primary, handles Eth54(Port54) 2. r"\(Port(\d+)\)" — new explicit handler for Eth(Port5) and similar aliases that lack a digit directly after "Eth" 3. r"(\d+)$" — trailing-number fallback for generic aliases such as "hundredGigE49" or "QSFP28-49" A digit-anywhere fallback (r"(\d+)") was considered but rejected: it would misparse aliases like "QSFP28-49" by returning 28 instead of 49. The explicit \(Port(\d+)\) pattern is narrow enough to avoid that while still covering the Eth(PortN) family. Update the Eth(Port5) test to assert 5 and add a QSFP28-49 regression test that pins the trailing-number fallback against the prefix-digit hazard. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- osism/tasks/conductor/sonic/interface.py | 11 ++++++++++- .../conductor/sonic/test_interface_conversion.py | 15 +++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/osism/tasks/conductor/sonic/interface.py b/osism/tasks/conductor/sonic/interface.py index e4f8b9c5..3d6efedb 100644 --- a/osism/tasks/conductor/sonic/interface.py +++ b/osism/tasks/conductor/sonic/interface.py @@ -445,7 +445,16 @@ def _extract_port_number_from_alias(alias): ) return port_number - # Fallback to number at end of alias + # Handle Eth(PortN) format where no digit follows "Eth" directly + paren_port_match = re.search(r"\(Port(\d+)\)", alias) + if paren_port_match: + port_number = int(paren_port_match.group(1)) + logger.debug( + f"Extracted port number {port_number} from Eth(Port) alias '{alias}'" + ) + return port_number + + # Fallback to trailing number (e.g. "hundredGigE49", "twentyFiveGigE1") match = re.search(r"(\d+)$", alias) if match: port_number = int(match.group(1)) diff --git a/tests/unit/tasks/conductor/sonic/test_interface_conversion.py b/tests/unit/tasks/conductor/sonic/test_interface_conversion.py index 1c7f0974..35c59bdd 100644 --- a/tests/unit/tasks/conductor/sonic/test_interface_conversion.py +++ b/tests/unit/tasks/conductor/sonic/test_interface_conversion.py @@ -574,10 +574,17 @@ def test_extract_port_number_no_digits_returns_none(): assert _extract_port_number_from_alias("foo") is None -def test_extract_port_number_paren_alias_without_leading_digit_returns_none(): - # "Eth(Port5)" matches neither regex (no \d+ before "(", trailing char - # is ")", so $-anchored digit search fails). Documents actual behavior. - assert _extract_port_number_from_alias("Eth(Port5)") is None +def test_extract_port_number_paren_alias_without_leading_digit(): + # "Eth(Port5)" has no digit immediately after "Eth", so the primary + # Eth\d+(Port\d+) regex does not match. The explicit \(Port(\d+)\) + # pattern handles it next and returns 5. + assert _extract_port_number_from_alias("Eth(Port5)") == 5 + + +def test_extract_port_number_alias_with_prefix_digits_uses_trailing(): + # "QSFP28-49": the \(Port(\d+)\) pattern does not match, and the + # trailing-number fallback correctly returns 49 rather than 28. + assert _extract_port_number_from_alias("QSFP28-49") == 49 # --------------------------------------------------------------------------- From aedfac5db6e4fdc620c2c8fecbfad933572e3d50 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:41:04 +0200 Subject: [PATCH 08/10] tests/sonic/interface: cover breakout alias-no-number fallback path _convert_using_port_config had two tested breakout paths: base port not found at all (empty port_config), and base port found with a valid alias. The path where the base port IS found but its alias contains no extractable number was not tested. Give this case its own early return and warning in the production code rather than sharing the "Could not find base port" message with the genuinely-not-found case. The new message is: "Could not extract port number from alias '...' for breakout interface ..." Add a test: port_config with Ethernet4 having alias "no-digits", ethernet_num=5 so _find_base_port_for_breakout walks back to Ethernet4, _extract_port_number_from_alias returns None, the fallback produces "Eth1/2/2", and the accurate warning is asserted via loguru_logs. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- osism/tasks/conductor/sonic/interface.py | 6 ++++++ .../conductor/sonic/test_interface_conversion.py | 16 ++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/osism/tasks/conductor/sonic/interface.py b/osism/tasks/conductor/sonic/interface.py index 3d6efedb..b25cc006 100644 --- a/osism/tasks/conductor/sonic/interface.py +++ b/osism/tasks/conductor/sonic/interface.py @@ -386,6 +386,12 @@ def _convert_using_port_config( ) return result + logger.warning( + f"Could not extract port number from alias '{base_alias}' " + f"for breakout interface {sonic_interface_name}" + ) + return _convert_using_speed_calculation(ethernet_num, None, is_breakout) + # Fallback if base port not found logger.warning( f"Could not find base port for breakout interface {sonic_interface_name}" diff --git a/tests/unit/tasks/conductor/sonic/test_interface_conversion.py b/tests/unit/tasks/conductor/sonic/test_interface_conversion.py index 35c59bdd..558cb318 100644 --- a/tests/unit/tasks/conductor/sonic/test_interface_conversion.py +++ b/tests/unit/tasks/conductor/sonic/test_interface_conversion.py @@ -516,6 +516,22 @@ def test_convert_using_port_config_breakout_no_base_port_falls_back(): assert _convert_using_port_config("Ethernet5", 5, True, {}) == "Eth1/2/2" +def test_convert_using_port_config_breakout_base_port_found_but_alias_has_no_number( + loguru_logs, +): + # is_breakout=True; base port Ethernet4 IS found in port_config, but its + # alias "no-digits" yields no extractable number. The code should log a + # warning and fall back to speed-based calculation. + # _convert_using_speed_calculation(5, None, True): + # base_port=4, subport=2, physical_port=2 → "Eth1/2/2" + port_config = {"Ethernet4": {"alias": "no-digits"}} + + assert _convert_using_port_config("Ethernet5", 5, True, port_config) == "Eth1/2/2" + assert _has_log( + loguru_logs, "WARNING", "Could not extract port number from alias 'no-digits'" + ) + + def test_convert_using_port_config_regular_missing_alias_falls_back(): # Regular path with port present but alias unparseable → legacy calc. # Ethernet4 with speed=None falls to multiplier 1 → Eth1/5. From ef468940cb773a10d8309a977fc3c02db03003d4 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:42:30 +0200 Subject: [PATCH 09/10] tests/sonic/interface: assert warning logs in fallback tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six convert_netbox_interface_to_sonic tests verified only the return value of fallback paths but never checked that the expected logger.warning call was actually made. A future refactor silently removing or changing those warnings would pass the full suite. Add a _has_log helper (same pattern as test_config.py) and extend each of the six tests with a WARNING-level assertion: - string + device=None → "Device object required" - object + device=None → "Device object required" - no HWSKU → "No HWSKU found" - cache failure → "Could not fetch device interfaces" - empty port config → "No port config found" - port config raises → "Could not load port config" AI-assisted: Claude Code Signed-off-by: Roger Luethi --- .../sonic/test_interface_conversion.py | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/unit/tasks/conductor/sonic/test_interface_conversion.py b/tests/unit/tasks/conductor/sonic/test_interface_conversion.py index 558cb318..c5046911 100644 --- a/tests/unit/tasks/conductor/sonic/test_interface_conversion.py +++ b/tests/unit/tasks/conductor/sonic/test_interface_conversion.py @@ -46,6 +46,10 @@ def _reset_port_config_cache(): clear_port_config_cache() +def _has_log(records, level, substring): + return any(r["level"] == level and substring in r["message"] for r in records) + + def _make_device(hwsku="Accton-AS7326-56X", device_id=1, name="sw-1"): """Build a minimal NetBox-shaped device with sonic_parameters.""" custom_fields = ( @@ -105,24 +109,27 @@ def test_convert_netbox_interface_already_sonic_returns_unchanged(): assert convert_netbox_interface_to_sonic(iface, device) == "Ethernet4" -def test_convert_netbox_interface_string_without_device_returns_input(): +def test_convert_netbox_interface_string_without_device_returns_input(loguru_logs): assert convert_netbox_interface_to_sonic("Eth1/1", device=None) == "Eth1/1" + assert _has_log(loguru_logs, "WARNING", "Device object required") -def test_convert_netbox_interface_object_without_device_returns_name(): +def test_convert_netbox_interface_object_without_device_returns_name(loguru_logs): iface = _make_iface("Eth1/1") assert convert_netbox_interface_to_sonic(iface, device=None) == "Eth1/1" + assert _has_log(loguru_logs, "WARNING", "Device object required") -def test_convert_netbox_interface_device_without_hwsku_returns_input(): +def test_convert_netbox_interface_device_without_hwsku_returns_input(loguru_logs): device = _make_device(hwsku=None) # sonic_parameters dict, no "hwsku" key iface = _make_iface("Eth1/1") assert convert_netbox_interface_to_sonic(iface, device) == "Eth1/1" + assert _has_log(loguru_logs, "WARNING", "No HWSKU found") -def test_convert_netbox_interface_cache_failure_returns_input(mocker): +def test_convert_netbox_interface_cache_failure_returns_input(mocker, loguru_logs): mocker.patch( "osism.tasks.conductor.sonic.interface.get_cached_device_interfaces", side_effect=RuntimeError("netbox down"), @@ -131,9 +138,10 @@ def test_convert_netbox_interface_cache_failure_returns_input(mocker): iface = _make_iface("Eth1/1") assert convert_netbox_interface_to_sonic(iface, device) == "Eth1/1" + assert _has_log(loguru_logs, "WARNING", "Could not fetch device interfaces") -def test_convert_netbox_interface_empty_port_config_returns_input(mocker): +def test_convert_netbox_interface_empty_port_config_returns_input(mocker, loguru_logs): mocker.patch( "osism.tasks.conductor.sonic.interface.get_cached_device_interfaces", return_value=[_make_iface("Eth1/1")], @@ -145,9 +153,10 @@ def test_convert_netbox_interface_empty_port_config_returns_input(mocker): device = _make_device() assert convert_netbox_interface_to_sonic(_make_iface("Eth1/1"), device) == "Eth1/1" + assert _has_log(loguru_logs, "WARNING", "No port config found") -def test_convert_netbox_interface_port_config_raises_returns_input(mocker): +def test_convert_netbox_interface_port_config_raises_returns_input(mocker, loguru_logs): mocker.patch( "osism.tasks.conductor.sonic.interface.get_cached_device_interfaces", return_value=[_make_iface("Eth1/1")], @@ -159,6 +168,7 @@ def test_convert_netbox_interface_port_config_raises_returns_input(mocker): device = _make_device() assert convert_netbox_interface_to_sonic(_make_iface("Eth1/1"), device) == "Eth1/1" + assert _has_log(loguru_logs, "WARNING", "Could not load port config") def test_convert_netbox_interface_standard_form_resolves_via_alias(mocker): From 2a95960c2fb3be5090b9fe28db97f49ce6a3431e Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 29 Apr 2026 10:42:42 +0200 Subject: [PATCH 10/10] sonic/interface: remove dead interface_speed in detect_port_channels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In detect_port_channels, interface_speed was computed for every LAG member (including a potential get_speed_from_port_type call and its logging side-effect) but the value was never used — it was not forwarded to convert_netbox_interface_to_sonic, which does not accept a speed parameter. Remove the dead three-line computation block. The conversion call is unchanged; only the unused variable assignment is removed. AI-assisted: Claude Code Signed-off-by: Roger Luethi --- osism/tasks/conductor/sonic/interface.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/osism/tasks/conductor/sonic/interface.py b/osism/tasks/conductor/sonic/interface.py index b25cc006..aab218f0 100644 --- a/osism/tasks/conductor/sonic/interface.py +++ b/osism/tasks/conductor/sonic/interface.py @@ -1008,15 +1008,6 @@ def detect_port_channels(device): if hasattr(interface, "lag") and interface.lag: lag_parent = interface.lag - # Convert NetBox interface name to SONiC format - interface_speed = getattr(interface, "speed", None) - if ( - not interface_speed - and hasattr(interface, "type") - and interface.type - ): - interface_speed = get_speed_from_port_type(interface.type.value) - sonic_interface_name = convert_netbox_interface_to_sonic( interface, device )