Skip to content

Add synthetic base fixtures and the SONiC E2E job - #2566

Open
ideaship wants to merge 4 commits into
mainfrom
sonic-e2e-v2-fixtures
Open

Add synthetic base fixtures and the SONiC E2E job#2566
ideaship wants to merge 4 commits into
mainfrom
sonic-e2e-v2-fixtures

Conversation

@ideaship

@ideaship ideaship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Part of the series tracked in #2562, which explains the ordering and what each PR covers. Based on the preceding PR in the stack, so review only the top commits here.

This is where the test first runs end to end, and the largest PR in the
series
— 3528 lines, of which roughly 2700 is generated golden JSON. Four
commits: five synthetic devices and their goldens, the coverage report, a README
section, and the Zuul job.

It is deliberately not split further. The harness is not independently testable
in smaller pieces, and the Zuul job has to land together with the goldens it
compares against, or the check is red on arrival.

Two things to know before approving:

Once this job exists it gates changes under osism/tasks/conductor/,
osism/settings.py, Makefile and .zuul.yaml. That makes this the point of
no return for the series.

The synthetic-fixture half has never run in Zuul. Only the compose change
has a green CI run, on a branch that no longer exists. The check on this PR is
the first real test of the fixtures, so it is the one to watch.

coverage.py reports 30 of 38 emitted tables here, and exits non-zero saying
so. That is the honest number for the base fixtures alone; the four scenario PRs
above take it to 38.

@ideaship ideaship changed the title sonic e2e v2 fixtures Add synthetic base fixtures and the SONiC E2E job Aug 5, 2026
@berendt
berendt force-pushed the sonic-e2e-v2-fixtures branch from 72f0dce to e260acd Compare August 5, 2026 15:10
@ideaship
ideaship force-pushed the sonic-e2e-v2-fixtures branch from e260acd to e03efd0 Compare August 5, 2026 19:51
@berendt
berendt force-pushed the sonic-e2e-v2-fixtures branch from e03efd0 to 8d3c443 Compare August 6, 2026 10:56
@ideaship
ideaship force-pushed the sonic-e2e-v2-fixtures branch from 8d3c443 to ba8c624 Compare August 6, 2026 12:10
@ideaship
ideaship force-pushed the sonic-e2e-v2-fixtures branch from ba8c624 to 9efcf83 Compare August 6, 2026 12:19
Base automatically changed from sonic-e2e-v2-compose to main August 7, 2026 05:35
@berendt
berendt force-pushed the sonic-e2e-v2-fixtures branch from 9efcf83 to 7dce270 Compare August 7, 2026 05:35
@ideaship
ideaship marked this pull request as ready for review August 7, 2026 05:58
@ideaship
ideaship requested a review from berendt August 7, 2026 05:58

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/e2e/coverage.py" line_range="61-58" />
<code_context>
+    return tables
+
+
+def covered_tables(golden_dir=GOLDEN_DIR):
+    """Tables that are non-empty in at least one golden file."""
+    import json
+
+    tables = set()
+    for path in sorted(Path(golden_dir).glob("*.json")):
+        config = json.loads(path.read_text())
+        for table, value in config.items():
+            if value:
+                tables.add(table)
+    return tables
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `covered_tables` to ensure the "non-empty" heuristic behaves as intended for different JSON value types.

Currently `covered_tables` treats any truthy JSON value as coverage and any falsy value (e.g. `[]`, `{}`, `0`, `False`, `""`) as uncovered. Please add focused tests with small JSON examples to:
- Confirm empty lists/dicts are ignored.
- Clarify how numeric/boolean values are handled (and whether they’re supported or rejected).
- Define behavior for non-dict top-level JSON (e.g. fail clearly or be explicitly unsupported).
These tests will pin down the intended semantics and guard against future changes in the golden format or producer.

Suggested implementation:

```python
def covered_tables(golden_dir=GOLDEN_DIR):
    """Tables that are non-empty in at least one golden file.

    A table is considered covered if its value in at least one golden JSON file
    is truthy. Falsy values (e.g. [], {}, 0, False, "") are treated as uncovered.
    The golden file must contain a JSON object mapping table names to coverage
    descriptors; non-object top-level JSON is rejected.
    """
    import json

    tables = set()
    for path in sorted(Path(golden_dir).glob("*.json")):
        config = json.loads(path.read_text())
        if not isinstance(config, dict):
            raise TypeError(
                f"{path} must contain a JSON object mapping table names to values, "
                f"got {type(config).__name__}"
            )
        for table, value in config.items():
            if value:
                tables.add(table)
    return tables

```

```python
It works in two independent steps:


def test_covered_tables_ignores_empty_containers(tmp_path):
    """Empty lists/dicts should be ignored, non-empty containers counted."""
    import json

    golden = {
        "EMPTY_LIST": [],
        "EMPTY_DICT": {},
        "NONEMPTY_LIST": ["x"],
        "NONEMPTY_DICT": {"k": "v"},
    }
    (tmp_path / "golden.json").write_text(json.dumps(golden))

    from tests.e2e.coverage import covered_tables

    result = covered_tables(golden_dir=tmp_path)
    assert result == {"NONEMPTY_LIST", "NONEMPTY_DICT"}


def test_covered_tables_numeric_and_boolean_values(tmp_path):
    """Numeric/boolean/string values are treated by truthiness."""
    import json

    golden = {
        "ZERO": 0,
        "ONE": 1,
        "FALSE_BOOL": False,
        "TRUE_BOOL": True,
        "EMPTY_STRING": "",
        "NONEMPTY_STRING": "value",
    }
    (tmp_path / "golden.json").write_text(json.dumps(golden))

    from tests.e2e.coverage import covered_tables

    result = covered_tables(golden_dir=tmp_path)
    assert result == {"ONE", "TRUE_BOOL", "NONEMPTY_STRING"}


def test_covered_tables_rejects_non_object_top_level_json(tmp_path):
    """Non-dict top-level JSON should fail clearly."""
    import json
    import pytest

    # List top-level JSON
    (tmp_path / "list.json").write_text(json.dumps(["not", "a", "dict"]))
    # Scalar top-level JSON
    (tmp_path / "scalar.json").write_text(json.dumps(42))

    from tests.e2e.coverage import covered_tables

    with pytest.raises(TypeError):
        covered_tables(golden_dir=tmp_path)

```

1. If `tests/e2e/coverage.py` is not intended to be imported as `tests.e2e.coverage` during test runs, adjust the imports in the new tests to import `covered_tables` via the actual module path used in your test runner.
2. If you already have a dedicated test module for `coverage.py` (e.g. `tests/e2e/test_coverage.py`), you may prefer to move these `test_covered_tables_*` functions into that file instead of keeping them inline; the bodies can be reused as-is.
3. Ensure `pytest` is available in your test environment, as the non-object JSON test relies on `pytest.raises`. If you use a different testing framework, replace the `pytest.raises` usage with the equivalent assertion mechanism.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread tests/e2e/coverage.py
@berendt

berendt commented Aug 7, 2026

Copy link
Copy Markdown
Member

@ideaship Please check the Sourcery review.

@ideaship
ideaship force-pushed the sonic-e2e-v2-fixtures branch from 7dce270 to 21d627d Compare August 7, 2026 08:07
@ideaship ideaship moved this from New to Ready for review in Human Board Aug 7, 2026
The SONiC E2E golden test previously depended on osism/testbed's
example seed data, seeded through netbox-manager. Replace it with
frozen, in-repo synthetic NetBox fixtures under
tests/e2e/scenario/resources/ (100-base.yml, 150-context.yml,
200-fabric.yml, 250-oob.yml, 260-metalbox.yml) plus the minimal
edgecore-7726-32x-e2e device type, and commit the first four golden
files this scenario produces (e2e-spine-1, e2e-leaf-1, e2e-leaf-2,
e2e-oob-1). The goldens are now reproducible without any reference to
osism/testbed.

Fixture topology: one spine (e2e-spine-1) cabled to two leaves
(e2e-leaf-1, e2e-leaf-2) on numbered /31 point-to-point links inside a
prefix with the Transfer IPAM role, a standalone OOB switch
(e2e-oob-1), and a metalbox (e2e-metalbox-1) that is not itself a
SONiC device. e2e-leaf-1 also carries an access port (untagged VLAN
100) and a trunk port (tagged VLAN 200) plus a VLAN200 SVI, and
e2e-leaf-2 carries a table_id-only VRF (vrf99) on a data port.

Device filter contract, reverse-engineered from
osism/tasks/conductor/sonic: a device is generated only when all
three hold -- status=active, tagged managed-by-metalbox, and
role.slug is one of DEFAULT_SONIC_ROLES (spine/leaf/switch here). Any
one missing silently skips the device: no golden, no error.
e2e-metalbox-1 deliberately fails this filter (role metalbox, no
managed-by-metalbox tag) so it is never generated.

Each switch's ASN is derived from its Loopback0 address (4200 +
zero-padded 3rd/4th octet), so the fixture's IP addresses are
load-bearing, not free choices:

  e2e-spine-1  172.16.10.1/20   192.168.20.1/32   ASN 4200020001
  e2e-leaf-1   172.16.10.11/20  192.168.20.11/32  ASN 4200020011
  e2e-leaf-2   172.16.10.12/20  192.168.20.12/32  ASN 4200020012
  e2e-oob-1    172.16.10.21/20  192.168.20.21/32  ASN 4200020021

The metalbox unlocks DNS_NAMESERVER/NTP_SERVER by holding
172.16.10.254/20 -- an address inside the switches' shared /20 OOB
subnet (172.16.0.0/20) -- on a non-mgmt_only interface.
_get_metalbox_ip_for_device() matches purely by that subnet
membership and explicitly skips mgmt_only interfaces; it never
follows a cable, so no cabling to the metalbox is required.

Two generator behaviours were not obvious from the code and cost
iterations to find, so are called out in comments in 200-fabric.yml:
a newly created interface not present in the device type requires an
explicit `type`, and a point-to-point link's address must fall inside
a prefix with the Transfer IPAM role or the link is treated as
IP-unnumbered and BGP_NEIGHBOR/BGP_NEIGHBOR_AF stay empty despite
being cabled and addressed.

This covers 30 of the 38 config_db tables; ACL_TABLE/ACL_RULE and the
rest come from the base scaffold regardless of fixtures.
PORTCHANNEL*, the breakout paths and the EVPN/VXLAN/multi-VRF tables
are out of scope here and land with their own scenario files and
goldens in later PRs, which must not modify these five base files
since everything device-wide is intentionally concentrated here.

Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
The "N of M config_db tables covered" claim made whenever the golden set
grows had only ever been established by an ad-hoc script run once during
development, so nobody could re-derive or check it afterwards.

Add tests/e2e/coverage.py, run by `make sonic-e2e-coverage`. It works in
two independent steps: derive the tables the generator can emit from
config["TABLE"]/cfg["TABLE"] assignments under
osism/tasks/conductor/sonic/ (excluding the generated schema package,
which is data rather than emission logic), then collect the tables that
are non-empty in at least one file under tests/e2e/golden/. Both sides
are derived on every run, so this needs no updating as scenarios are
added -- each one simply makes the reported number go up.

It is a reporting tool, not a gate: it is not wired into
sonic_golden_test.sh or the Zuul job, and the golden comparison stays
the only check that can fail a run. It does exit non-zero while any
emitted table has no golden, which against the base fixtures alone is
the honest answer -- 30 of 38, naming the eight tables the breakout,
port-channel and EVPN scenarios go on to cover.

Both halves are pure functions over a directory tree, so they are unit
tested in tests/unit/e2e/, next to the comparator and the generation
driver. The emission regex is where the risk sits: it produces the
reported denominator and decides the exit code, and because a table
that is non-empty in the goldens but missed by the regex does not fail
the run, a silent narrowing would surface only as a quietly shrinking
number.

It lands here, with the first goldens, rather than with the last
scenario, so the number is available and meaningful while the golden set
is still being built up.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
The README explains how to run the unit and integration suites, but the
E2E golden test added in this series had no entry point outside the
Makefile and the harness script's own header comment, so there was
nothing pointing a newcomer at "make sonic-e2e".

Add a third section in the same shape as the existing two: what the test
does, the prerequisites beyond the development dependencies (docker with
the compose plugin, openssl, and a netbox-manager checkout found as a
sibling directory or via NETBOX_MANAGER_DIR), and the Makefile targets
for running, iterating against a reused stack, and regenerating.

The coverage report gets its own mention because it is the one part of
this suite nothing else surfaces: it is not wired into the harness or
the Zuul job, so `make sonic-e2e-coverage` is the only way anyone sees
which config_db tables the golden set reaches.

Two behaviours are called out because neither is guessable from the
error it produces. Regeneration refuses a stack left over from an
earlier run, since applying the fixtures over a populated database can
yield goldens that a fresh stack -- which CI always uses -- would not
reproduce. And seeding applies every file under
tests/e2e/scenario/resources/ regardless of git status, so a stray file
there joins the fixture set; the harness header records that this has
broken a run twice.

The remaining environment overrides are left to
tests/e2e/sonic_golden_test.sh, which already documents them, rather
than duplicated here where they would drift.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
Add python-osism-sonic-e2e as a Zuul job, wired via
playbooks/pre-sonic-e2e.yml (pre-run) and
playbooks/test-sonic-e2e.yml (run).

The job uses nodeset: ubuntu-noble and timeout: 2400, since bringing
up the compose stack, installing netbox-manager into its own venv,
seeding NetBox and generating/comparing SONiC configs for every
supported HWSKU takes longer than the default job timeout. A files
matcher restricts when it runs in the check pipeline to changes that
can affect the generated output or the harness itself (settings,
conductor/sonic code, the E2E tests, the playbooks, Pipfile.lock,
files/sonic/, requirements.txt and requirements.ansible.txt (the
sonic_golden_test.sh harness installs the [ansible] extra that
setup.cfg maps to the latter), setup.cfg itself, and
.zuul.yaml/Makefile); it also runs unconditionally on periodic-daily.
netbox-manager is still pulled at tip-of-main via required-projects
because it remains the seeding tool, so a Depends-On is honored for
its code -- but its example/ seed data is no longer used, so this job
no longer detects drift in that data.

pre-sonic-e2e.yml retains the accept_ra=2 sysctl because the Zuul
node is IPv6-only and learns its default route via SLAAC; without
it, router advertisements are not accepted on interfaces where
forwarding is enabled and the node loses its route to the outside
network.

Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
@berendt
berendt force-pushed the sonic-e2e-v2-fixtures branch from 21d627d to 67c10a0 Compare August 7, 2026 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready for review

Development

Successfully merging this pull request may close these issues.

3 participants