Refuse breakouts that claim other ports

[ideaship]

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 free. That holds for every port of every bundled HWSKU except one.

On Accton-AS7726-32X the last 100G port has four lanes, but the next two
port names are already taken by independent 10G SFP+ ports:

Ethernet124     125,126,127,128       hundredGigE32     32    100000
Ethernet125     129                   tenGigE33         33     10000
Ethernet126     128                   tenGigE34         34     10000

A 4x breakout of Ethernet124 generates children Ethernet124/125/126/127, so
it claims Ethernet125 and Ethernet126 as sub-ports of a cage they are not
part of.

What that does to the generated config

port	before	after a 4x25G breakout of Ethernet124
Ethernet125	lanes=129 speed=10000 alias=Eth1/33	lanes=126 speed=25000 alias=Eth1/33/1
Ethernet126	lanes=128 speed=10000 alias=Eth1/34	lanes=127 speed=25000 alias=Eth1/34/1

All three fields are wrong, and both ports are re-presented as breakout
sub-ports. Two things make this reach a real switch rather than staying
theoretical:

• a breakout_ports entry is authoritative for a port's lanes and speed —
  the declared path stashes them directly, the inferred path recomputes them
  from the master's lane list via _calculate_breakout_port_lane();
• the PORT table is built from every entry in port_config, not only the
  interfaces present in NetBox, so no NetBox modelling of the SFP ports is
  needed for them to be emitted and corrupted. Every generated config for that
  HWSKU already contains all 34 ports.

2x50G collides the same way, on Ethernet126 alone.

The fix

_breakout_child_collisions() returns any child name that is a port of this
HWSKU in its own right. Both affected paths now check it before mutating
anything, so the group is dropped whole rather than half-applied and the
master keeps its own configuration:

• 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 needed no change. Its existing
topology gate already skips a group whose intermediate slots are ports in
port_config — the same condition — so only a comment was added there,
recording that the gate is load-bearing for correctness and not only for the
native-port misdetection it was written to prevent.

The collision test is simply 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 and no false positives to
suppress.

Why the existing tests could not have caught this

Every breakout test builds its port_config from a helper that returns a
single entry (occasionally two). A one-port port_config cannot express a
child slot occupied by another port, so this class of bug was structurally
unreachable by the suite — the fixtures encoded the same assumption as the code.

The new tests therefore load the .ini files shipped in this repo. The 400G case
keeps a synthetic port_config, because no bundled HWSKU has an 8-lane master
with an occupied child slot.

Note on the underlying port config

Worth flagging for whoever owns those files, though it is deliberately not
changed here. Upstream sonic-buildimage ships 32 ports for this HWSKU —
Ethernet125/Ethernet126 do not exist there — and its Broadcom config carries
them only as commented-out entries (#portmap_130=128:10:m,
#portmap_66=129:10:m). Lane 128 is muxed between the last 100G port's fourth
lane and a 10G port, so the vendor ships the 10G ports disabled to leave the
100G port all four lanes. Accton-AS7326-56X.ini has the same lane-128 overlap.

So these files declare port pairs the hardware cannot run simultaneously, which
is a data question separate from this change. Refusing to silently claim a port
that exists is correct either way, which is why the guard goes in regardless of
how that is settled.

Verification

• Full unit suite green; tests/unit/tasks/conductor/sonic at 701 passed.
• Each new guard verified to fail loudly: with the guards disabled, exactly the
  two corresponding tests fail, while the topology-gate test and the
  breakout-still-works control keep passing.
• flake8 and black clean.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The real_port_config fixture computes repo_root via Path(__file__).resolve().parents[5], which is fairly brittle to directory layout changes; consider deriving the path from a more stable anchor (e.g. using importlib.resources or a smaller fixed relative path from the test module) to make the tests more robust.
• The logic to build children port-name lists is duplicated across the NetBox-format, SONiC 400G, and standard SONiC paths; extracting a small helper (e.g. _breakout_children_from_group(...)) would reduce the chance of those paths diverging subtly in future changes.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `real_port_config` fixture computes `repo_root` via `Path(__file__).resolve().parents[5]`, which is fairly brittle to directory layout changes; consider deriving the path from a more stable anchor (e.g. using `importlib.resources` or a smaller fixed relative path from the test module) to make the tests more robust.
- The logic to build `children` port-name lists is duplicated across the NetBox-format, SONiC 400G, and standard SONiC paths; extracting a small helper (e.g. `_breakout_children_from_group(...)`) would reduce the chance of those paths diverging subtly in future changes.

## Individual Comments

### Comment 1
<location path="tests/unit/tasks/conductor/sonic/test_breakout_detection.py" line_range="579-588" />
<code_context>
+# ---------------------------------------------------------------------------
+
+
+@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):
+        repo_root = pathlib.Path(__file__).resolve().parents[5]
+        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)
+
</code_context>
<issue_to_address>
**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:

```python
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`.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

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.