Skip to content

Refuse breakouts that claim other ports #2560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
berendt merged 1 commit into from
Aug 5, 2026
+203 −21
Merged

Refuse breakouts that claim other ports #2560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 63 additions & 20 deletions osism/tasks/conductor/sonic/interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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",
Expand All @@ -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
}
Expand Down Expand Up @@ -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",
Expand All @@ -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}

Expand Down
14 changes: 14 additions & 0 deletions tests/unit/tasks/conductor/sonic/_detection_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
127 changes: 126 additions & 1 deletion tests/unit/tasks/conductor/sonic/test_breakout_detection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
"""

Comment on lines +578 to +587

@sourcery-ai sourcery-ai Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The real_port_config fixture relies on a hard-coded parents[5] path, which may be brittle if the test file is moved or the repo layout changes.

Consider resolving the repo root in a layout-independent way, e.g., walking up parents until you hit a known marker (pyproject.toml/setup.cfg/tests) or the files/sonic/port_config directory itself. This avoids relying on a fixed parents[5] depth and keeps the fixture working if the file is moved or the tree structure changes.

Suggested implementation:

import pathlib
from types import SimpleNamespace

import pytest


@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 _find_repo_root(start: pathlib.Path) -> pathlib.Path:
        """Walk up parents until we find a known repo marker or port_config dir."""
        current = start
        while True:
            port_config_dir = current / "files" / "sonic" / "port_config"
            if port_config_dir.exists():
                return current

            # Common project-root markers
            if any(
                (current / marker).exists()
                for marker in ("pyproject.toml", "setup.cfg", "setup.py", "tests")
            ):
                return current

            if current.parent == current:
                raise RuntimeError("Could not locate repository root for tests")

            current = current.parent

    def _load(hwsku):
        repo_root = _find_repo_root(pathlib.Path(__file__).resolve())
        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)

    return _load
  1. Ensure interface_module is imported or otherwise available in this test module; if it is not yet imported, add an appropriate import (e.g., from <module> import interface_module) alongside the other imports.
  2. If your repo layout uses a different root marker than pyproject.toml, setup.cfg, setup.py, or tests, add that marker to the list in _find_repo_root.

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"] == {}