diff --git a/osism/tasks/conductor/sonic/connections.py b/osism/tasks/conductor/sonic/connections.py index 5c74a187..2c6cdfd3 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 @@ -290,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/osism/tasks/conductor/sonic/interface.py b/osism/tasks/conductor/sonic/interface.py index e4f8b9c5..aab218f0 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}" @@ -445,7 +451,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)) @@ -993,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 ) 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/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") diff --git a/tests/unit/tasks/conductor/sonic/test_cache.py b/tests/unit/tasks/conductor/sonic/test_cache.py index 2e69bdd2..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) # --------------------------------------------------------------------------- @@ -294,6 +296,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) # --------------------------------------------------------------------------- diff --git a/tests/unit/tasks/conductor/sonic/test_connections.py b/tests/unit/tasks/conductor/sonic/test_connections.py index c3ad8719..e486bc1f 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( @@ -509,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]) == [] diff --git a/tests/unit/tasks/conductor/sonic/test_interface_conversion.py b/tests/unit/tasks/conductor/sonic/test_interface_conversion.py index 1c7f0974..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): @@ -516,6 +526,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. @@ -574,10 +600,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 # --------------------------------------------------------------------------- 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 # ---------------------------------------------------------------------------