diff --git a/osism/tasks/conductor/sonic/config_generator.py b/osism/tasks/conductor/sonic/config_generator.py index 24f05ef9..507923da 100644 --- a/osism/tasks/conductor/sonic/config_generator.py +++ b/osism/tasks/conductor/sonic/config_generator.py @@ -263,9 +263,13 @@ def generate_sonic_config(device, hwsku, device_as_mapping=None, config_version= # Convert NetBox interface name to SONiC format for lookup interface_speed = getattr(interface, "speed", None) # Track if speed was explicitly set in NetBox (not derived from port type) - speed_explicit = interface_speed is not None - # If speed is not set, try to get it from port type - if not interface_speed and hasattr(interface, "type") and interface.type: + speed_explicit = bool(interface_speed) + if speed_explicit: + # NetBox stores explicit interface speeds in kbps, normalize + # to Mbps so "speed" always carries a single unit + interface_speed = int(interface_speed) // 1000 + elif hasattr(interface, "type") and interface.type: + # Speeds derived from the port type are already in Mbps interface_speed = get_speed_from_port_type(interface.type.value) sonic_name = convert_netbox_interface_to_sonic(interface, device) netbox_interfaces[sonic_name] = { @@ -521,25 +525,24 @@ def _add_port_configurations( # Override with NetBox data if available # - Always use explicitly set NetBox speed (overrides port config) # - Use derived speed (from port type) only if port config has no speed - # Note: NetBox stores speed in kbps, SONiC expects Mbps (divide by 1000) + # Note: netbox_interfaces speeds are already normalized to Mbps if port_name in netbox_interfaces: netbox_speed = netbox_interfaces[port_name]["speed"] speed_explicit = netbox_interfaces[port_name].get("speed_explicit", False) if netbox_speed: - # Convert NetBox speed (kbps) to SONiC speed (Mbps) - sonic_speed = str(int(netbox_speed) // 1000) + sonic_speed = str(int(netbox_speed)) if speed_explicit: # Explicitly set speed in NetBox always takes precedence if sonic_speed != str(port_speed): logger.info( - f"Using explicit NetBox speed {netbox_speed} kbps -> {sonic_speed} Mbps for port {port_name} " + f"Using explicit NetBox speed {sonic_speed} Mbps for port {port_name} " f"(overriding port config speed: {port_speed})" ) port_speed = sonic_speed elif not port_speed or port_speed == "0": # Derived speed (from port type) only used if port config has no speed logger.info( - f"Using derived NetBox speed {netbox_speed} kbps -> {sonic_speed} Mbps for port {port_name} " + f"Using derived NetBox speed {sonic_speed} Mbps for port {port_name} " f"(hardware config had: {port_speed})" ) port_speed = sonic_speed @@ -550,11 +553,9 @@ def _add_port_configurations( # Override with individual breakout port speed from NetBox if available if port_name in netbox_interfaces and netbox_interfaces[port_name]["speed"]: - # Convert NetBox speed (kbps) to SONiC speed (Mbps) - netbox_speed = netbox_interfaces[port_name]["speed"] - port_speed = str(int(netbox_speed) // 1000) + port_speed = str(int(netbox_interfaces[port_name]["speed"])) logger.debug( - f"Using NetBox speed {netbox_speed} kbps -> {port_speed} Mbps for breakout port {port_name}" + f"Using NetBox speed {port_speed} Mbps for breakout port {port_name}" ) elif master_port in breakout_info["breakout_cfgs"]: # Fallback to extracting speed from breakout mode @@ -758,12 +759,11 @@ def _add_missing_breakout_ports( master_port = breakout_info["breakout_ports"][port_name]["master"] # Override with individual breakout port speed from NetBox if available - # Note: NetBox stores speed in kbps, SONiC expects Mbps (divide by 1000) + # Note: netbox_interfaces speeds are already normalized to Mbps if port_name in netbox_interfaces and netbox_interfaces[port_name]["speed"]: - netbox_speed = netbox_interfaces[port_name]["speed"] - port_speed = str(int(netbox_speed) // 1000) + port_speed = str(int(netbox_interfaces[port_name]["speed"])) logger.debug( - f"Using NetBox speed {netbox_speed} kbps -> {port_speed} Mbps for missing breakout port {port_name}" + f"Using NetBox speed {port_speed} Mbps for missing breakout port {port_name}" ) elif master_port in breakout_info["breakout_cfgs"]: # Fallback to extracting speed from breakout mode diff --git a/osism/tasks/conductor/sonic/interface.py b/osism/tasks/conductor/sonic/interface.py index aab218f0..23aee339 100644 --- a/osism/tasks/conductor/sonic/interface.py +++ b/osism/tasks/conductor/sonic/interface.py @@ -40,6 +40,27 @@ def get_speed_from_port_type(port_type): return speed +def get_interface_speed(interface): + """Get the speed of a NetBox interface in Mbps. + + Explicitly set NetBox speeds are stored in kbps and are converted; + when unset, the speed is derived from the interface type (already Mbps). + + Args: + interface: NetBox interface object + + Returns: + int: Speed in Mbps, or None if neither an explicit speed nor a + recognized port type is available + """ + speed = getattr(interface, "speed", None) + if speed: + return int(speed) // 1000 + if hasattr(interface, "type") and interface.type: + return get_speed_from_port_type(interface.type.value) + return None + + def convert_netbox_interface_to_sonic(device_interface, device=None): """Convert NetBox interface name to SONiC interface name with device-specific mapping. @@ -724,17 +745,9 @@ def detect_breakout_ports(device): # Determine breakout mode based on number of subports and speed num_subports = len(breakout_group) - interface_speed = getattr( - breakout_group[0][1], "speed", None + interface_speed = get_interface_speed( + breakout_group[0][1] ) - if ( - not interface_speed - and hasattr(breakout_group[0][1], "type") - and breakout_group[0][1].type - ): - interface_speed = get_speed_from_port_type( - breakout_group[0][1].type.value - ) # Calculate breakout mode if interface_speed == 10000 and num_subports == 4: @@ -830,15 +843,7 @@ def detect_breakout_ports(device): for iface in interfaces: if iface.name == ethernet_name: # Check if this interface has 100G speed - iface_speed = getattr(iface, "speed", None) - if ( - not iface_speed - and hasattr(iface, "type") - and iface.type - ): - iface_speed = get_speed_from_port_type( - iface.type.value - ) + iface_speed = get_interface_speed(iface) # 400G breakout uses 100G per port if iface_speed == 100000: @@ -898,13 +903,7 @@ def detect_breakout_ports(device): for iface in interfaces: if iface.name == ethernet_name: # Check if this interface has a speed that suggests breakout - iface_speed = getattr(iface, "speed", None) - if ( - not iface_speed - and hasattr(iface, "type") - and iface.type - ): - iface_speed = get_speed_from_port_type(iface.type.value) + iface_speed = get_interface_speed(iface) # Only consider as breakout if speed is 50G or less AND we have 4 consecutive ports # This prevents regular 100G ports from being treated as breakout ports @@ -919,15 +918,7 @@ def detect_breakout_ports(device): master_port = f"Ethernet{base_port}" # Determine breakout mode based on speed - interface_speed = getattr(sonic_breakout_group[0][1], "speed", None) - if ( - not interface_speed - and hasattr(sonic_breakout_group[0][1], "type") - and sonic_breakout_group[0][1].type - ): - interface_speed = get_speed_from_port_type( - sonic_breakout_group[0][1].type.value - ) + interface_speed = get_interface_speed(sonic_breakout_group[0][1]) if interface_speed == 25000: brkout_mode = "4x25G" diff --git a/tests/unit/tasks/conductor/sonic/_detection_helpers.py b/tests/unit/tasks/conductor/sonic/_detection_helpers.py index b71964ae..e99d8c42 100644 --- a/tests/unit/tasks/conductor/sonic/_detection_helpers.py +++ b/tests/unit/tasks/conductor/sonic/_detection_helpers.py @@ -22,7 +22,8 @@ def _make_iface(name, *, speed=None, type_value=None, lag=None): """Build a NetBox-shaped interface stub. ``type_value`` becomes ``interface.type.value`` (set ``None`` for no type), - ``speed`` mirrors ``interface.speed``, and ``lag`` is the LAG-parent stub. + ``speed`` mirrors ``interface.speed`` (kbps, as stored by NetBox), and + ``lag`` is the LAG-parent stub. """ return SimpleNamespace( name=name, diff --git a/tests/unit/tasks/conductor/sonic/test_breakout_detection.py b/tests/unit/tasks/conductor/sonic/test_breakout_detection.py index 52295c5b..df593947 100644 --- a/tests/unit/tasks/conductor/sonic/test_breakout_detection.py +++ b/tests/unit/tasks/conductor/sonic/test_breakout_detection.py @@ -203,11 +203,11 @@ def test_detect_breakout_ports_cache_raises_returns_empty( @pytest.mark.parametrize( "speed, expected_mode", [ - (10000, "4x10G"), - (25000, "4x25G"), - (50000, "4x50G"), - (100000, "4x100G"), - (200000, "4x200G"), + (10_000_000, "4x10G"), + (25_000_000, "4x25G"), + (50_000_000, "4x50G"), + (100_000_000, "4x100G"), + (200_000_000, "4x200G"), ], ) def test_detect_breakout_ports_netbox_format_supported_speeds( @@ -241,7 +241,7 @@ def test_detect_breakout_ports_netbox_unsupported_speed_skipped( ): # 40G with 4 subports is not in the supported speed table → continue. device = _make_sonic_device() - interfaces = _netbox_breakout_interfaces(speed=40000) + interfaces = _netbox_breakout_interfaces(speed=40_000_000) port_config = _port_config_for_port() patch_breakout_helpers(interfaces=interfaces, port_config=port_config) @@ -274,7 +274,7 @@ def test_detect_breakout_ports_netbox_8_lane_master_uses_offset_2( # 400G master with 8 lanes — subport offsets multiply by 2, so the # breakout sub-ports map to Ethernet0,2,4,6 instead of 0,1,2,3. device = _make_sonic_device() - interfaces = _netbox_breakout_interfaces(speed=100000) + interfaces = _netbox_breakout_interfaces(speed=100_000_000) port_config = _port_config_for_port(lanes="1,2,3,4,5,6,7,8") patch_breakout_helpers(interfaces=interfaces, port_config=port_config) @@ -300,7 +300,7 @@ def test_detect_breakout_ports_netbox_single_subport_not_breakout( patch_breakout_helpers, ): device = _make_sonic_device() - interfaces = [_make_iface("Eth1/49/1", speed=100000)] + interfaces = [_make_iface("Eth1/49/1", speed=100_000_000)] port_config = _port_config_for_port() patch_breakout_helpers(interfaces=interfaces, port_config=port_config) @@ -321,7 +321,7 @@ def test_detect_breakout_ports_netbox_processed_groups_dedup( # twice on the first iter — once for the sonic name, once for the master # — and not at all on the others). device = _make_sonic_device() - interfaces = _netbox_breakout_interfaces(speed=100000) + interfaces = _netbox_breakout_interfaces(speed=100_000_000) port_config = _port_config_for_port() patch_breakout_helpers(interfaces=interfaces, port_config=port_config) @@ -339,8 +339,8 @@ def test_detect_breakout_ports_netbox_two_independent_groups( ): # Two distinct breakout groups (Eth1/49 and Eth1/50) yield two masters. device = _make_sonic_device() - interfaces = _netbox_breakout_interfaces(speed=100000) + [ - _make_iface(f"Eth1/50/{i}", speed=100000) for i in (1, 2, 3, 4) + interfaces = _netbox_breakout_interfaces(speed=100_000_000) + [ + _make_iface(f"Eth1/50/{i}", speed=100_000_000) for i in (1, 2, 3, 4) ] port_config = { **_port_config_for_port(), @@ -369,7 +369,7 @@ def test_detect_breakout_ports_netbox_two_independent_groups( def test_detect_breakout_ports_sonic_400g_first_group(patch_breakout_helpers): device = _make_sonic_device() - interfaces = [_make_iface(f"Ethernet{n}", speed=100000) for n in (0, 2, 4, 6)] + interfaces = [_make_iface(f"Ethernet{n}", speed=100_000_000) for n in (0, 2, 4, 6)] port_config = _port_config_for_port( alias="Eth1/1", lanes="1,2,3,4,5,6,7,8", speed="400000" ) @@ -393,7 +393,9 @@ def test_detect_breakout_ports_sonic_400g_first_group(patch_breakout_helpers): def test_detect_breakout_ports_sonic_400g_second_group(patch_breakout_helpers): # Master ``Ethernet8`` → physical port ``1/2`` (the 2nd 400G cage). device = _make_sonic_device() - interfaces = [_make_iface(f"Ethernet{n}", speed=100000) for n in (8, 10, 12, 14)] + interfaces = [ + _make_iface(f"Ethernet{n}", speed=100_000_000) for n in (8, 10, 12, 14) + ] port_config = _port_config_for_port( sonic_port="Ethernet8", alias="Eth1/2", @@ -416,7 +418,7 @@ def test_detect_breakout_ports_sonic_400g_speed_mismatch_skipped( # 8-lane master, but the four interfaces are at 50G — not 100G — so the # 400G branch's per-port speed check fails and the group is rejected. device = _make_sonic_device() - interfaces = [_make_iface(f"Ethernet{n}", speed=50000) for n in (0, 2, 4, 6)] + interfaces = [_make_iface(f"Ethernet{n}", speed=50_000_000) for n in (0, 2, 4, 6)] port_config = _port_config_for_port( alias="Eth1/1", lanes="1,2,3,4,5,6,7,8", speed="400000" ) @@ -435,7 +437,7 @@ def test_detect_breakout_ports_sonic_400g_4_lane_master_falls_through( # Master ``Ethernet0`` only has 4 lanes — not a 400G port. Four # consecutive interfaces at 25G then trigger the SONiC standard branch. device = _make_sonic_device() - interfaces = [_make_iface(f"Ethernet{n}", speed=25000) for n in range(4)] + interfaces = [_make_iface(f"Ethernet{n}", speed=25_000_000) for n in range(4)] port_config = _port_config_for_port(alias="Eth1/1") patch_breakout_helpers(interfaces=interfaces, port_config=port_config) @@ -462,8 +464,8 @@ def test_detect_breakout_ports_sonic_400g_4_lane_master_falls_through( @pytest.mark.parametrize( "speed, expected_mode", [ - (25000, "4x25G"), - (50000, "4x50G"), + (25_000_000, "4x25G"), + (50_000_000, "4x50G"), ], ) def test_detect_breakout_ports_sonic_standard_supported_speeds( @@ -473,7 +475,7 @@ def test_detect_breakout_ports_sonic_standard_supported_speeds( interfaces = [_make_iface(f"Ethernet{n}", speed=speed) for n in range(4)] # The 400G branch peeks at ``Ethernet0`` in the port_config; provide a # 4-lane entry so it skips the 400G code path cleanly. - port_config = _port_config_for_port(alias="Eth1/1", speed=str(speed)) + port_config = _port_config_for_port(alias="Eth1/1", speed=str(speed // 1000)) patch_breakout_helpers(interfaces=interfaces, port_config=port_config) result = detect_breakout_ports(device) @@ -496,7 +498,7 @@ def test_detect_breakout_ports_sonic_standard_at_100g_skipped( # Regular 100G ports (4 consecutive Ethernet0..3) are NOT a breakout — # the speed filter (``<= 50000``) excludes them. device = _make_sonic_device() - interfaces = [_make_iface(f"Ethernet{n}", speed=100000) for n in range(4)] + interfaces = [_make_iface(f"Ethernet{n}", speed=100_000_000) for n in range(4)] port_config = _port_config_for_port(alias="Eth1/1") patch_breakout_helpers(interfaces=interfaces, port_config=port_config) @@ -511,7 +513,7 @@ def test_detect_breakout_ports_sonic_standard_only_three_interfaces( ): # A 3-port group is incomplete — no breakout configured. device = _make_sonic_device() - interfaces = [_make_iface(f"Ethernet{n}", speed=25000) for n in range(3)] + interfaces = [_make_iface(f"Ethernet{n}", speed=25_000_000) for n in range(3)] port_config = _port_config_for_port(alias="Eth1/1") patch_breakout_helpers(interfaces=interfaces, port_config=port_config) diff --git a/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py b/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py index 62a58c30..97a6031b 100644 --- a/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py +++ b/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py @@ -670,6 +670,42 @@ def test_generate_sonic_config_handles_interfaces_without_optional_fields( patch_orchestrator_helpers._add_port_configurations.assert_called_once() +def test_generate_sonic_config_normalizes_interface_speeds_to_mbps( + mocker, patch_orchestrator_helpers, make_orchestrator_device +): + """Explicit NetBox speeds are stored in kbps and must be converted to + Mbps at collection time; speeds derived from the port type are already + Mbps and pass through unchanged — ``netbox_interfaces`` carries a + single unit for all downstream consumers.""" + patch_base_config(mocker) + iface_explicit = SimpleNamespace( + name="Ethernet0", + speed=100_000_000, # 100 G in kbps + type=SimpleNamespace(value="100gbase-x-qsfp28"), + tags=[], + ) + iface_derived = SimpleNamespace( + name="Ethernet1", + speed=None, + type=SimpleNamespace(value="100gbase-x-qsfp28"), + tags=[], + ) + patch_orchestrator_helpers.get_cached_device_interfaces.return_value = [ + iface_explicit, + iface_derived, + ] + patch_orchestrator_helpers.get_speed_from_port_type.return_value = 100_000 + + generate_sonic_config(make_orchestrator_device(), "HWSKU") + + args, _ = patch_orchestrator_helpers._add_port_configurations.call_args + netbox_interfaces = args[5] + assert netbox_interfaces["Ethernet0"]["speed"] == 100_000 + assert netbox_interfaces["Ethernet0"]["speed_explicit"] is True + assert netbox_interfaces["Ethernet1"]["speed"] == 100_000 + assert netbox_interfaces["Ethernet1"]["speed_explicit"] is False + + def test_generate_sonic_config_get_cached_interfaces_failure_logged( mocker, patch_orchestrator_helpers, make_orchestrator_device ): diff --git a/tests/unit/tasks/conductor/sonic/test_config_generator_ports.py b/tests/unit/tasks/conductor/sonic/test_config_generator_ports.py index 390d62fd..4fd03ee0 100644 --- a/tests/unit/tasks/conductor/sonic/test_config_generator_ports.py +++ b/tests/unit/tasks/conductor/sonic/test_config_generator_ports.py @@ -212,7 +212,7 @@ def test_explicit_netbox_speed_overrides_port_config( device=device, ) - assert config["PORT"]["Ethernet0"]["speed"] == "100" + assert config["PORT"]["Ethernet0"]["speed"] == "100000" def test_derived_netbox_speed_only_used_when_port_speed_empty( self, config, device, mocker, patch_post_loop_hooks @@ -244,9 +244,9 @@ def test_derived_netbox_speed_only_used_when_port_speed_empty( # Existing speed is kept when explicit=False assert config["PORT"]["Ethernet0"]["speed"] == "40000" - # speed "0" / "" → derived NetBox speed is applied (kbps → Mbps) - assert config["PORT"]["Ethernet4"]["speed"] == "25" - assert config["PORT"]["Ethernet8"]["speed"] == "25" + # speed "0" / "" → derived NetBox speed (already Mbps) is applied + assert config["PORT"]["Ethernet4"]["speed"] == "25000" + assert config["PORT"]["Ethernet8"]["speed"] == "25000" def test_breakout_port_speed_from_netbox( self, config, device, mocker, patch_post_loop_hooks @@ -277,8 +277,8 @@ def test_breakout_port_speed_from_netbox( device=device, ) - # 25000 kbps → 25 Mbps for breakout port - assert config["PORT"]["Ethernet1"]["speed"] == "25" + # NetBox speed (already Mbps) is used as-is for breakout port + assert config["PORT"]["Ethernet1"]["speed"] == "25000" @pytest.mark.parametrize( "brkout_mode, expected_speed", @@ -664,7 +664,7 @@ def test_existing_port_skipped(self, config, mocker): assert config["PORT"]["Ethernet1"] == {"sentinel": True} alias.assert_not_called() - def test_speed_from_netbox_converts_kbps_to_mbps(self, config, mocker): + def test_speed_from_netbox_used_as_mbps(self, config, mocker): mocker.patch.object( config_generator, "convert_sonic_interface_to_alias", return_value="a" ) @@ -672,8 +672,9 @@ def test_speed_from_netbox_converts_kbps_to_mbps(self, config, mocker): "breakout_cfgs": {"Ethernet0": {"brkout_mode": "4x100G"}}, "breakout_ports": {"Ethernet1": {"master": "Ethernet0"}}, } - # NetBox stores speed in kbps; 25 G = 25_000_000 kbps -> 25_000 Mbps - netbox_interfaces = {"Ethernet1": _nb_iface(speed=25_000_000)} + # netbox_interfaces speeds are normalized to Mbps at collection time; + # a 100 G breakout port derived from its port type carries 100_000 + netbox_interfaces = {"Ethernet1": _nb_iface(speed=100_000)} _add_missing_breakout_ports( config, @@ -684,7 +685,11 @@ def test_speed_from_netbox_converts_kbps_to_mbps(self, config, mocker): netbox_interfaces=netbox_interfaces, ) - assert config["PORT"]["Ethernet1"]["speed"] == "25000" + assert config["PORT"]["Ethernet1"]["speed"] == "100000" + assert ( + config["PORT"]["Ethernet1"]["valid_speeds"] + == "100000,50000,25000,10000,1000" + ) @pytest.mark.parametrize( "brkout_mode, expected_speed", diff --git a/tests/unit/tasks/conductor/sonic/test_interface_conversion.py b/tests/unit/tasks/conductor/sonic/test_interface_conversion.py index c5046911..5ba272ed 100644 --- a/tests/unit/tasks/conductor/sonic/test_interface_conversion.py +++ b/tests/unit/tasks/conductor/sonic/test_interface_conversion.py @@ -25,6 +25,7 @@ convert_netbox_interface_to_sonic, convert_sonic_interface_to_alias, get_connected_interfaces, + get_interface_speed, get_port_config, get_speed_from_port_type, ) @@ -97,6 +98,34 @@ def test_get_speed_from_port_type_numeric_input_coerced(): assert get_speed_from_port_type(1234) is None +# --------------------------------------------------------------------------- +# get_interface_speed +# --------------------------------------------------------------------------- + + +def test_get_interface_speed_explicit_kbps_converted_to_mbps(): + # An explicitly set NetBox speed is stored in kbps and takes precedence + # over the port type. + iface = SimpleNamespace( + speed=100_000_000, type=SimpleNamespace(value="10gbase-x-sfpp") + ) + assert get_interface_speed(iface) == 100_000 + + +def test_get_interface_speed_derived_from_port_type_already_mbps(): + iface = SimpleNamespace(speed=None, type=SimpleNamespace(value="100gbase-x-qsfp28")) + assert get_interface_speed(iface) == 100_000 + + +@pytest.mark.parametrize("speed", [None, 0]) +def test_get_interface_speed_without_speed_and_type_returns_none(speed): + assert get_interface_speed(SimpleNamespace(speed=speed, type=None)) is None + + +def test_get_interface_speed_without_speed_attribute_returns_none(): + assert get_interface_speed(SimpleNamespace()) is None + + # --------------------------------------------------------------------------- # convert_netbox_interface_to_sonic # ---------------------------------------------------------------------------