From 8018af52dedd22572fcb74438370cef2ea7fea52 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 5 Aug 2026 13:06:12 +0200 Subject: [PATCH] sonic: refuse breakouts that claim other ports A detected breakout names its children after the master's lane offsets -- Ethernet -- 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// 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 --- osism/tasks/conductor/sonic/interface.py | 83 +++++++++--- .../conductor/sonic/_detection_helpers.py | 14 ++ .../sonic/test_breakout_detection.py | 127 +++++++++++++++++- 3 files changed, 203 insertions(+), 21 deletions(-) diff --git a/osism/tasks/conductor/sonic/interface.py b/osism/tasks/conductor/sonic/interface.py index 2ee32feaa..71e2af41a 100644 --- a/osism/tasks/conductor/sonic/interface.py +++ b/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} diff --git a/tests/unit/tasks/conductor/sonic/_detection_helpers.py b/tests/unit/tasks/conductor/sonic/_detection_helpers.py index e99d8c429..e970a5151 100644 --- a/tests/unit/tasks/conductor/sonic/_detection_helpers.py +++ b/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( diff --git a/tests/unit/tasks/conductor/sonic/test_breakout_detection.py b/tests/unit/tasks/conductor/sonic/test_breakout_detection.py index 95198aac5..8c34d5355 100644 --- a/tests/unit/tasks/conductor/sonic/test_breakout_detection.py +++ b/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//. 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"] == {}