sonic: refuse breakouts that claim other ports (#2560)

A detected breakout names its children after the master's lane offsets
-- Ethernet<base + n*lanes_per_child> -- which assumes every slot below
the next master is unused. That holds for every port of every bundled
HWSKU except one: on Accton-AS7726-32X the last 100G port, Ethernet124,
has four lanes, but Ethernet125 and Ethernet126 are independent 10G SFP+
ports occupying two of its four child slots.

Claiming them as children silently reconfigures two working ports. A
breakout_ports entry is authoritative for a port's lanes and speed, and
the PORT table is built from every entry in port_config rather than only
the interfaces present in NetBox, so both ports are rewritten on any
device with that HWSKU: lanes 129 and 128 become 126 and 127, speed
10000 becomes 25000, and the aliases Eth1/33 and Eth1/34 become
Eth1/33/1 and Eth1/34/1, presenting them as breakout sub-ports.

Add _breakout_child_collisions() and refuse before mutating anything, so
the group is dropped whole rather than half-applied and the master keeps
its own configuration. Two of the three detection paths needed it:

  - the Eth<module>/<port>/<subport> path, which computes child names
    from the master offset and had no check at all;
  - the SONiC-name 400G grouping path, likewise.

The SONiC-name standard grouping path already refuses these groups. Its
topology gate skips a group whose intermediate slots are ports in
port_config, which is the same condition this collision test applies. A
comment now records that the gate is load-bearing for correctness and
not only for the native-port misdetection it was written for.

The collision test is that a child name other than the master is itself
a key in port_config. Swept across every master of all nine bundled
HWSKUs at 1x/2x/4x/8x, it flags exactly the three real cases on that one
HWSKU and nothing else, so it needs no per-HWSKU exception list.

The new tests load the .ini files shipped in this repo instead of
building a port_config inline. That is the point: the existing
breakout tests use one- or two-entry port_configs, which cannot
express an occupied child slot, so this class of bug was
structurally unreachable by the suite. The 400G case keeps a
synthetic port_config because no bundled HWSKU has an 8-lane master
with an occupied child slot.

The tests locate the repo root by walking up for the setup.cfg
marker rather than a fixed parent depth, matching how
tests/integration/conftest.py finds it; a hard-coded depth breaks
silently when a test module moves.

Assisted-by: Claude:claude-opus-5

Signed-off-by: Roger Luethi <luethi@osism.tech>

Author: ideaship

osism/tasks/conductor/sonic/interface.py

@@ -641,6 +641,20 @@ def get_connected_interfaces(device, portchannel_info=None):
     return _get_connected_interfaces(device, portchannel_info)
 
 
+def _breakout_child_collisions(children, master, port_config):
+    """Children that are separate ports of this HWSKU rather than free slots.
+
+    Breakout children are named after the master's lane offsets, which assumes
+    every slot below the next master is unused. That holds for every port of
+    every bundled HWSKU but one: on Accton-AS7726-32X the last 100G port
+    (Ethernet124, four lanes) is followed by Ethernet125 and Ethernet126, two
+    independent 10G SFP+ ports occupying two of its four child slots. Claiming
+    those as children rewrites their lanes, speed and alias, so a breakout that
+    would do it has to be refused.
+    """
+    return [c for c in children if c != master and c in port_config]
+
+
 def detect_breakout_ports(device):
     """Detect breakout ports from NetBox device interfaces using the centralized breakout logic.
 
@@ -767,14 +781,6 @@ def detect_breakout_ports(device):
                                 # Calculate physical port number (1/1 -> port 1, 1/2 -> port 2, etc.)
                                 physical_port_num = f"{module}/{port}"
 
-                                # Add breakout config for master port
-                                breakout_cfgs[master_port] = {
-                                    "breakout_owner": "MANUAL",
-                                    "brkout_mode": brkout_mode,
-                                    "port": physical_port_num,
-                                }
-
-                                # Add all subports to breakout_ports
                                 min_subport = breakout_group[0][0]
 
                                 # Determine the offset multiplier based on master port lane count
@@ -797,12 +803,31 @@ def detect_breakout_ports(device):
                                                 f"8 lanes, using offset multiplier {offset_multiplier}"
                                             )
 
-                                for subport, iface in breakout_group:
-                                    current_offset = (
-                                        subport - min_subport
-                                    ) * offset_multiplier
-                                    sonic_port_num = base_port_num + current_offset
-                                    port_name = f"Ethernet{sonic_port_num}"
+                                children = [
+                                    "Ethernet"
+                                    f"{base_port_num + (subport - min_subport) * offset_multiplier}"
+                                    for subport, _iface in breakout_group
+                                ]
+                                collisions = _breakout_child_collisions(
+                                    children, master_port, port_config
+                                )
+                                if collisions:
+                                    logger.error(
+                                        f"Breakout of {master_port} would claim "
+                                        f"{', '.join(collisions)}, which are separate "
+                                        f"ports on this HWSKU; skipping the group"
+                                    )
+                                    continue
+
+                                # Add breakout config for master port
+                                breakout_cfgs[master_port] = {
+                                    "breakout_owner": "MANUAL",
+                                    "brkout_mode": brkout_mode,
+                                    "port": physical_port_num,
+                                }
+
+                                # Add all subports to breakout_ports
+                                for port_name in children:
                                     breakout_ports[port_name] = {"master": master_port}
 
                                 logger.debug(
@@ -861,6 +886,24 @@ def detect_breakout_ports(device):
                                     physical_port_index = (base_port_400g // 8) + 1
                                     physical_port_num = f"1/{physical_port_index}"
 
+                                    children = [
+                                        f"Ethernet{port_num_400g}"
+                                        for port_num_400g, _iface in (
+                                            sonic_400g_breakout_group
+                                        )
+                                    ]
+                                    collisions = _breakout_child_collisions(
+                                        children, master_port, port_config
+                                    )
+                                    if collisions:
+                                        logger.error(
+                                            f"400G breakout of {master_port} would "
+                                            f"claim {', '.join(collisions)}, which are "
+                                            f"separate ports on this HWSKU; skipping "
+                                            f"the group"
+                                        )
+                                        continue
+
                                     # Add breakout config for master port
                                     breakout_cfgs[master_port] = {
                                         "breakout_owner": "MANUAL",
@@ -869,11 +912,7 @@ def detect_breakout_ports(device):
                                     }
 
                                     # Add all ports to breakout_ports
-                                    for (
-                                        port_num_400g,
-                                        iface,
-                                    ) in sonic_400g_breakout_group:
-                                        port_name = f"Ethernet{port_num_400g}"
+                                    for port_name in children:
                                         breakout_ports[port_name] = {
                                             "master": master_port
                                         }
@@ -955,6 +994,10 @@ def detect_breakout_ports(device):
                     physical_port_index = (base_port // 4) + 1
                     physical_port_num = f"1/{physical_port_index}"
 
+                    # NOTE: the topology gate above already refuses a group whose
+                    # intermediate slots are ports in port_config, which is the
+                    # same condition _breakout_child_collisions() tests. No
+                    # separate collision check is needed on this path.
                     # Add breakout config for master port
                     breakout_cfgs[master_port] = {
                         "breakout_owner": "MANUAL",
@@ -963,7 +1006,7 @@ def detect_breakout_ports(device):
                     }
 
                     # Add all ports to breakout_ports
-                    for port_num, iface in sonic_breakout_group:
+                    for port_num, _iface in sonic_breakout_group:
                         port_name = f"Ethernet{port_num}"
                         breakout_ports[port_name] = {"master": master_port}
 

tests/unit/tasks/conductor/sonic/_detection_helpers.py

@@ -6,9 +6,23 @@
 the module is private (``_``-prefixed) so pytest does not collect it.
 """
 
+from pathlib import Path
 from types import SimpleNamespace
 
 
+def repo_root():
+    """Return the repository root, found by its ``setup.cfg`` marker.
+
+    Walking up beats hard-coding a parent depth, which silently breaks when a
+    test module moves. ``tests/integration/conftest.py`` locates the root the
+    same way, for the same reason.
+    """
+    for parent in Path(__file__).resolve().parents:
+        if (parent / "setup.cfg").exists():
+            return parent
+    raise RuntimeError("no repository root with setup.cfg above this file")
+
+
 def _make_sonic_device(device_id=1, name="sw1", hwsku="TEST-HWSKU"):
     """Build a NetBox device stub carrying ``custom_fields.sonic_parameters.hwsku``."""
     return SimpleNamespace(

tests/unit/tasks/conductor/sonic/test_breakout_detection.py

@@ -16,7 +16,7 @@
 from osism.tasks.conductor.sonic import interface as interface_module
 from osism.tasks.conductor.sonic.interface import detect_breakout_ports
 
-from ._detection_helpers import _make_iface, _make_sonic_device
+from ._detection_helpers import _make_iface, _make_sonic_device, repo_root
 
 # ---------------------------------------------------------------------------
 # Helpers
@@ -568,3 +568,128 @@ def test_detect_breakout_ports_sonic_standard_speed_resolved_from_port_type(
     result = detect_breakout_ports(device)
 
     assert result["breakout_cfgs"]["Ethernet0"]["brkout_mode"] == "4x25G"
+
+
+# ---------------------------------------------------------------------------
+# Child slots occupied by another port
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def real_port_config(monkeypatch):
+    """Load a port_config from the .ini files actually shipped in this repo.
+
+    The helpers above build port_configs with one or two entries, which cannot
+    express a child slot already occupied by another port -- the one shape that
+    makes a breakout unsafe, and the reason this class of bug went unnoticed.
+    These tests need the real file.
+    """
+
+    def _load(hwsku):
+        monkeypatch.setattr(
+            interface_module,
+            "PORT_CONFIG_PATH",
+            str(repo_root() / "files" / "sonic" / "port_config"),
+        )
+        interface_module.clear_port_config_cache()
+        return interface_module.get_port_config(hwsku)
+
+    yield _load
+    interface_module.clear_port_config_cache()
+
+
+def test_netbox_format_breakout_refused_when_child_slot_is_another_port(
+    patch_breakout_helpers, real_port_config
+):
+    """Eth1/32 on Accton-AS7726-32X is Ethernet124, a four-lane 100G port whose
+    third and fourth child slots are Ethernet125 and Ethernet126 -- independent
+    10G SFP+ ports. Breaking it out would rewrite their lanes, speed and alias,
+    so the group is dropped whole, master BREAKOUT_CFG included.
+    """
+    port_config = real_port_config("Accton-AS7726-32X")
+    assert {"Ethernet125", "Ethernet126"} <= set(port_config)
+
+    device = _make_sonic_device()
+    interfaces = _netbox_breakout_interfaces(speed=25_000_000, port=32)
+    patch_breakout_helpers(interfaces=interfaces, port_config=port_config)
+
+    result = detect_breakout_ports(device)
+
+    assert result["breakout_cfgs"] == {}
+    assert result["breakout_ports"] == {}
+
+
+def test_netbox_format_breakout_allowed_when_child_slots_are_free(
+    patch_breakout_helpers, real_port_config
+):
+    """The same HWSKU's first port must still break out: Ethernet0's children
+    are Ethernet1-3, none of which is a port in its own right.
+    """
+    port_config = real_port_config("Accton-AS7726-32X")
+
+    device = _make_sonic_device()
+    interfaces = _netbox_breakout_interfaces(speed=25_000_000, port=1)
+    patch_breakout_helpers(interfaces=interfaces, port_config=port_config)
+
+    result = detect_breakout_ports(device)
+
+    assert result["breakout_cfgs"]["Ethernet0"]["brkout_mode"] == "4x25G"
+    assert sorted(result["breakout_ports"]) == [
+        "Ethernet0",
+        "Ethernet1",
+        "Ethernet2",
+        "Ethernet3",
+    ]
+
+
+def test_sonic_format_breakout_already_refused_by_the_topology_gate(
+    patch_breakout_helpers, real_port_config
+):
+    """The same collision reached through SONiC-format names rather than
+    Eth1/<port>/<subport>. This path needs no collision check: the topology gate
+    already skips a group whose intermediate slots are ports in port_config,
+    which is the same condition. Pinned here on the real port_config because
+    nothing else covered it, and because that gate is now load-bearing for
+    correctness rather than only for native-port misdetection.
+    """
+    port_config = real_port_config("Accton-AS7726-32X")
+
+    device = _make_sonic_device()
+    interfaces = [
+        _make_iface(f"Ethernet{n}", speed=25_000_000) for n in (124, 125, 126, 127)
+    ]
+    patch_breakout_helpers(interfaces=interfaces, port_config=port_config)
+
+    result = detect_breakout_ports(device)
+
+    assert result["breakout_cfgs"] == {}
+    assert result["breakout_ports"] == {}
+
+
+def test_sonic_400g_breakout_refused_when_child_slot_is_another_port(
+    patch_breakout_helpers,
+):
+    """The 400G grouping path takes the same guard. No bundled HWSKU has an
+    8-lane master with an occupied child slot, so the port_config here is
+    built to that shape: an 8-lane master at Ethernet0 whose 4x100G children
+    would be Ethernet0/2/4/6, with Ethernet4 present as its own port.
+    """
+    port_config = {
+        **_port_config_for_port(lanes="1,2,3,4,5,6,7,8", speed="400000"),
+        **_port_config_for_port(
+            sonic_port="Ethernet4",
+            alias="hundredGigE99",
+            lanes="9",
+            index="99",
+            speed="10000",
+        ),
+    }
+
+    device = _make_sonic_device()
+    interfaces = [_make_iface(f"Ethernet{n}", speed=100_000_000) for n in (0, 2, 4, 6)]
+    patch_breakout_helpers(interfaces=interfaces, port_config=port_config)
+
+    result = detect_breakout_ports(device)
+
+    assert result["breakout_cfgs"] == {}
+    assert result["breakout_ports"] == {}