Normalize NetBox interface speeds to Mbps at collection time

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

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"

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,

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)
 

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
 ):