DO NOT MERGE: CI-only check for the full SONiC E2E series - #2573
Closed
ideaship wants to merge 13 commits into
Closed
DO NOT MERGE: CI-only check for the full SONiC E2E series#2573ideaship wants to merge 13 commits into
ideaship wants to merge 13 commits into
Conversation
The directory holding the per-HWSKU port_config .ini files was hardcoded to /etc/sonic/port_config, which only exists inside the conductor image -- the Containerfile copies files/sonic/port_config there. A generator run from a checkout instead, as in local development, finds nothing at that path, and planting the files under /etc/sonic to work around it needs root. Introduce a SONIC_PORT_CONFIG_PATH setting in osism.settings, following the existing SONIC_* environment variable convention, and wire constants.PORT_CONFIG_PATH to it. The default is the previous hardcoded path, so container behaviour is identical; setting the variable points the generator at files/sonic/port_config/ in the checkout instead. Tests cover the default, the environment override, and the settings-to-constants wiring. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Add the Python side of the SONiC config-generation E2E golden test: tests/e2e/generate.py and tests/e2e/compare.py. generate.py drives sync_sonic() against a live NetBox and asserts success itself, because sync_sonic() returns only a device -> config dict and swallows per-device failures internally (it logs and moves on so one bad device does not abort the whole sync). To surface those failures here, generate.py installs a loguru sink that fails the run on any ERROR record, then exports the resulting config_db.json files for comparison. compare.py checks the exported files against tests/e2e/golden/: exact file-set equality (nothing missing, nothing extra) plus a structural diff of each file's JSON content, so a mismatch reports the offending keys/paths rather than an opaque "files differ". tests/unit/e2e/test_generate.py and test_compare.py cover both modules without needing a live NetBox. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Regeneration is the only path by which coverage can silently drop: in the normal comparison path an emptied table (or a disappearing device) changes the golden file and fails the comparison, but inside a several-hundred-line regenerated JSON diff either kind of loss is invisible. regenerate() now returns a list of entries describing coverage that was lost. There are two shapes: a "<file>: <TABLE>" entry for a table that was populated in the previous golden and became empty in the export, and a "<file>: file removed, had N populated tables" entry for a golden file whose device stopped being exported altogether and is being removed by the existing stale-file cleanup. The latter is the largest-granularity loss there is, and the most likely one in this project: a SONiC device is only generated when it is active, carries the managed-by-metalbox tag, and its role is in DEFAULT_SONIC_ROLES, so a single fixture typo silently drops a device's entire golden with no error. A stale golden that had no populated tables to begin with is not a loss and is still removed silently. main() prints this report and exits non-zero when the list is non-empty, unless the new --allow-coverage-loss flag is passed, making an intentional removal explicit in the command someone ran. Goldens are still written (or removed) either way; the guard only changes whether the run is reported as a failure. A count of lost tables/files was considered and rejected as the wrong shape: losing one table while gaining another would leave a count unchanged. main() now passes args.allow_coverage_loss through to regenerate() instead of dropping it on the floor, and the report header no longer claims every loss was "populated and are now empty" -- a removed golden file is also reported here and that wording did not fit it. Unit tests cover main()'s --regenerate branch itself (report text and exit code) both with and without --allow-coverage-loss; the existing tests all exercised regenerate() directly and never went through the CLI. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Add the infrastructure side of the SONiC config-generation E2E golden test: tests/e2e/compose.yaml, tests/e2e/deploy_netbox.sh and tests/e2e/sonic_golden_test.sh, plus the Makefile target and .gitignore entry that drive them. compose.yaml defines three services (NetBox, its Postgres database and Redis) -- no Kubernetes, so there is no kind/kubectl dependency and no cluster bring-up latency. NetBox is configured entirely through the environment variables its own baked-in configuration.py already reads; nothing is templated or mounted over it. API_TOKEN_PEPPERS is deliberately left unset so that a plain v1 API token can be minted for the test run instead of the peppered v2 format. NETBOX_PORT is the only variable interpolated into the compose file, because compose re-interpolates the whole file on every subcommand (up, ps, down, ...): keeping the rest of the configuration in the container environment avoids re-resolving values on each invocation. deploy_netbox.sh brings the stack up, waits for the healthchecks and mints/prints the API token. sonic_golden_test.sh is the harness entrypoint: it provisions NetBox via deploy_netbox.sh, then seeds it by installing netbox-manager into a dedicated venv (so its package pins never mutate this project's venv) and running it against the fixtures under tests/e2e/scenario/ -- netbox-manager's own bundled example/ data is not used. Concurrent seeding is disabled by default (SEED_PARALLEL=1) because seeding files in the same numeric group take KEY SHARE locks on shared parent dcim_device rows and can deadlock; the script documents the observed error and the condition that still causes it (200-fabric.yml cabling both leaves to a shared spine). Finally it runs sync_sonic() via generate.py and compares the result against tests/e2e/golden/. make sonic-e2e wires the above together; make sonic-e2e-regen adds --regenerate to rewrite the goldens. --regenerate refuses to run against a reused stack (CREATED_STACK==0) unless ALLOW_WARM_REGEN=1 is set: applying the fixtures as an UPDATE over whatever is already in a reused database can produce goldens a fresh stack -- the only kind CI ever uses -- would not reproduce, and nothing would otherwise say so. sonic_golden_test.sh also now accepts --allow-coverage-loss and forwards it to tests.e2e.compare, so an intentional fixture removal can get a green regen without editing the script by hand. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
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. 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>
The bundled netbox-manager example models no breakout ports and sets no explicit interface speeds, so the generator's breakout paths and its kbps->Mbps speed normalisation were never exercised by the SONiC E2E golden test. Add two standalone leaf devices on the shared E2E rack (positions 6 and 7, taking no cabling and needing none): - e2e-breakout-derived: Eth1/1/1..4 use the device type's 100gbase-x-qsfp28 interface type with no explicit speed, so the sub-port speed is derived from the interface type. Also carries a tagged VLAN on the plain Eth1/5 port, covering the VLAN / tagged-VLAN-to-port paths. - e2e-breakout-explicit: the same four sub-ports instead carry an explicit NetBox speed of 100000000 kbps, exercising the other unit the collection step must normalise. Both must yield sub-port speed 100000 in the generated config; confirmed via the regenerated goldens (BREAKOUT_CFG and BREAKOUT_PORTS populated on both, no bare speed "100" present). This brings config_db table coverage from 30 to 32 of 38. The two devices reuse the site, location, tenant, roles, tags and custom fields already seeded by 100-base.yml, and the edgecore-9726-32d-e2e device type is ported from ab8da03a. Related-Bug: #2478 Related-Bug: #2246 Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Give SONiC breakout detection an explicit, authoritative signal instead of inferring breakout structure from incidental NetBox artifacts. A device-level custom field sonic_parameters.breakout maps a master port (NetBox Eth1/N or canonical EthernetN, normalized via the hwsku port_config) to a mode string NxSpeedG. When a declaration is present for a resolvable master it is authoritative and fail-closed: the master is claimed into suppressed_masters before validation, so the inference path is suppressed for it even when the declared mode is invalid or two keys collide. The mode is validated structurally against the port_config lane count (L % N == 0); children and their exact per-child lane slices are computed from the mode (fixing 2x*/8x* which the count-based inference never handled), the physical port comes from the port_config index (correct on mixed-lane platforms), and config_generator uses the declared per-child speed and lanes ahead of any NetBox-derived value. Absent a breakout map, behaviour is unchanged by construction: the declared pass no-ops, suppressed_masters stays empty, and the inference branches (which only consult the set, never populate it) and their existing dedup are untouched. Structural validation only; the platform (platform.json) may still reject a structurally-valid mode. Adds unit coverage for the parsers, the resolver, mode emission across 4x/2x/8x/4x100G, key normalization, mixed-layout port index, collision and invalid/unresolvable/single-lane/malformed declarations, and the declared-child downstream precedence. Adds a third device, e2e-breakout-declared, to the SONiC E2E golden test: an Accton-AS9726-32D leaf carrying an authoritative sonic_parameters.breakout map with no sub-ports modelled in NetBox, so the committed golden proves the declared-mode path end to end rather than the inference fallback. It declares three splits on the 8-lane platform -- Ethernet0 4x100G, Ethernet8 2x50G, and physical key Eth1/9 (Ethernet64) 8x50G -- exercising key normalization, the 2x/8x cases count-based inference never handled, and the mixed-lane port_config index. This adds no new config_db table; coverage stays at 32 of 38 and the golden is the assertion that the new code path works. Regenerating on a fresh stack left the six existing goldens byte-unchanged. The declared path needs the same protection the detected paths now have. A declared breakout names its children after the master's lane offsets, so on Accton-AS7726-32X a 4x declaration on Ethernet124 would claim Ethernet125 and Ethernet126 -- two independent 10G SFP+ ports -- and silently rewrite their lanes, speed and alias, because a breakout_ports entry is authoritative for a port's lanes and speed. 2x50G collides the same way, on Ethernet126 alone. Reuse _breakout_child_collisions() and refuse before mutating anything, leaving BREAKOUT_CFG unset so the master stays an ordinary port and the declaration is dropped whole rather than half-applied. Tests cover both directions against the real shipped .ini via the existing real_port_config fixture. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Add a port-channel (LAG) device to the SONiC E2E synthetic fixtures. Until now the base/breakout fixtures modelled no LAGs, so PORTCHANNEL, PORTCHANNEL_INTERFACE and PORTCHANNEL_MEMBER were emitted by the generator but always empty in every golden -- that code path had no coverage. e2e-portchannel (rack E2E, position 9, edgecore-7726-32x-e2e / Accton-AS7726-32X, role leaf) reuses the site/location/tenant/tag objects created by 100-base.yml. It bonds Ethernet0 and Ethernet4 into PortChannel1: a NetBox LAG interface (type: lag) with the two data ports referencing it via their lag field, the same way real deployments model a port-channel. This brings cumulative non-empty config_db table coverage across the golden set from 32 to 35 of the 38 tables the generator emits (only the VXLAN EVPN/tunnel tables remain uncovered). Verified with a full down/regen/verify cycle against a freshly started NetBox stack, so the goldens reflect a from-scratch database rather than an UPDATE over a reused one. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
The base fixtures assign only one table_id-only VRF (vrf99 from
200-fabric.yml), so the EVPN/VXLAN subsystem was emitted-but-empty:
VXLAN_TUNNEL, VXLAN_TUNNEL_MAP and VXLAN_EVPN_NVO had no golden
coverage.
Add tests/e2e/scenario/resources/700-evpn.yml: a leaf device (e2e-evpn,
rack E2E position 10) with a Loopback0 whose primary IP becomes the
VXLAN tunnel source, and two VRFs assigned to data ports:
VrfStorage rd 2001 (a pure number) -> VNI 2001, populating
VXLAN_TUNNEL, VXLAN_TUNNEL_MAP, VXLAN_EVPN_NVO, the
L2VPN-EVPN BGP_GLOBALS_AF/BGP_GLOBALS_ROUTE_ADVERTISE,
and the synthesized Vlan2001 with its VLAN_INTERFACE.
vrf42 no rd -> the table_id-only branch (VRF["Vrf42"] =
{vrf_table_id: 42}), no EVPN/VXLAN.
Together with vrf99, VRF coverage goes from one to three VRFs across the
golden set. This does not introduce the VRF table itself (already
covered), only deepens it.
Ported from the retired testbed-derived scenario, dropping its
vars/rack/vlan stanzas (now provided by 100-base.yml) and its
managed-by-osism tag reference (that tag does not exist in the base
fixtures), and adding an explicit device_interface type on each new
port.
This is the last scenario in the series, so it is where the golden set
reaches every config_db table the generator can emit: `make
sonic-e2e-coverage` now reports 38 of 38 across 9 golden devices, up
from 30 of 38 on the base fixtures alone.
Verified from a fresh NetBox stack: regen is additive only (no existing
golden changed) and `make sonic-e2e` matches the goldens byte for byte.
Only the SNMP secrets decryption path stays unit-tested, because it
needs a vault and Redis, while the SNMP_SERVER_* tables themselves are
covered via the placeholder path.
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
tests/e2e/coverage.py reports which config_db tables the golden set reaches, but nothing consumes it: it is wired into neither sonic_golden_test.sh nor the Zuul job, so the number is only seen by someone who runs `make sonic-e2e-coverage` by hand. That is how the "38 of 38" claim came to exist as an unverifiable one-off in the first place, and leaving it human-run lets it decay the same way again. Add a unit test asserting the set of emitted-but-uncovered tables is empty. It closes the one coverage failure nothing else catches: a newly emitted table arriving with no golden. Coverage lost in the other direction -- a table that had a golden becoming empty -- already fails the golden comparison, because the golden file itself changes, and the regeneration path is covered separately by the coverage guard in compare.py. It belongs in the unit suite rather than the E2E job because it needs neither NetBox nor a generated config, only the generator source and the committed goldens. It therefore costs milliseconds and runs on every change, where the E2E job runs on a file matcher and a 2400s budget. This needs no .zuul.yaml change: coverage.py already exposes emitted_tables() and covered_tables(). The assertion message names the missing tables and points at `make sonic-e2e-regen`, because the cost of this gate is that adding a generator table now obliges the same change to add golden coverage, and that is a full regeneration cycle rather than a two-line edit. If a table genuinely cannot be reached by any fixture, the message says to exclude it here with a stated reason -- one visible exception, not a silently growing allowlist. This lands after the last scenario because it can only pass once the golden set is complete; the report itself lands with the first goldens, where it is still useful at 30 of 38. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Contributor
Author
|
Served its purpose — closing. The full series passed as one change: all ten jobs green, with For context on the timeout figure: the job this replaces consumed 80-92% of the same budget as a matter of course, which is why a node-local 301s apt stall was enough to fail it. At 800s that stall would still leave well over half the budget unused. The stack in #2562 now self-checks as it drains — each PR gets a real check once the one below it merges and its base becomes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do not review or merge. This PR exists only to get a CI signal, and will be
closed once it has one.
The series in #2562 is stacked, so eight of its nine PRs are based on
sonic-e2e-v2-*branches rather thanmain— and Zuul does not enqueue those.Only #2563, whose base is
main, has been checked. That leaves the part of theseries most in need of a check unchecked, including the Zuul job definition the
series itself adds.
This branch is the same commit as the head of #2571, retargeted at
mainso thewhole series gets checked as one change. Nothing here is unique to this PR;
review happens in the stack.
What this is actually testing
A local
make sonic-e2erun exercises the harness script directly, so itvalidates the fixtures, seeding, generation and golden comparison — but it says
nothing about the CI wiring, which is exactly the part that has never run. This
PR covers the rest:
python-osism-sonic-e2ejob definition and itsfilesmatcherplaybooks/pre-sonic-e2e.yml, including theaccept_ra=2sysctl theIPv6-only node needs to keep its default route once forwarding is enabled
playbooks/test-sonic-e2e.ymland the pipenv/[ansible]install it performsrequired-projectspull ofnetbox-manager, which supplies the seedingCLI
developer laptop
Expected result
Green, with
python-osism-sonic-e2epassing against the nine committed goldens.The coverage report is not part of the job, so its exit status does not affect
this run.