Add E2E golden tests for SONiC config generation#2483
Draft
ideaship wants to merge 9 commits into
Draft
Conversation
The directory holding the per-HWSKU port_config .ini files was hardcoded to /etc/sonic/port_config, which only exists inside the container image (the Dockerfile copies files/sonic/port_config there). Running the config generator outside a container -- for local development or an E2E test harness that points it at the in-repo files -- required root to plant files under /etc/sonic. 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 unchanged, and the value is read at import time like every other setting, so the container behavior is identical. Tests cover the default, the environment override, and the settings-to-constants wiring. Assisted-by: Claude:claude-fable-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Unit tests verify the SONiC config generator against fixture values that encode the author's assumptions, so a class of regressions -- such as mixed kbps/Mbps interface speeds -- can pass the unit suite while producing broken switch configurations. Nothing exercised the full pipeline: real NetBox data in, complete config_db.json out. Add a golden-file E2E harness that provisions NetBox on a local kind cluster (reusing the deploy script from a netbox-manager checkout), seeds it with the bundled example data, runs sync_sonic(), and compares every exported config_db file against goldens committed under tests/e2e/golden/. Any generator behavior change then surfaces as a reviewable golden diff instead of shipping silently. tests/e2e/generate.py drives the generation. sync_sonic() returns only a device-to-config dict and logs-and-swallows per-device failures, so the driver asserts success itself: the returned device set must match the expectation derived from the golden file names, empty configs fail, and a loguru sink turns any ERROR-level record into a failure. --no-expect skips the device-set check for the initial golden bootstrap. tests/e2e/compare.py enforces exact file-set equality (missing, extra, and mismatched exports are reported as distinct categories) and reports mismatches both as structural paths such as PORT|Ethernet4.speed and as a unified diff of the canonicalized JSON. Canonicalization sorts dictionary keys only; array order is part of the compared contract. --regenerate rewrites the goldens canonically for review. Both modules are pure logic and covered by the regular unit suite under tests/unit/e2e/. tests/e2e/run.sh and the Makefile targets sonic-e2e, sonic-e2e-regen, sonic-e2e-up and sonic-e2e-down tie the phases together. The exports use hostname identifiers (the seed devices carry no serial numbers) and the generator reads the in-repo port_config files via SONIC_PORT_CONFIG_PATH. netbox-manager is installed from the checkout rather than PyPI so a coordinated cross-repository change is tested against the changed code and data. Golden files are not yet committed; the initial set is bootstrapped with make sonic-e2e-regen and reviewed by hand. A Zuul job and seed data for known regression scenarios are follow-up work. Assisted-by: Claude:claude-fable-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
_add_snmp_configuration created a VaultLib instance for every device before looking at the secrets custom field. get_vault() reads the key file at /share/ansible_vault_password.key and fetches the encrypted vault password from Redis, so outside a worker container -- local development, or an E2E run against a plain NetBox -- it fails and logs three ERROR-level messages per synced device, even though there was nothing to decrypt in the first place. Defer vault construction until the device actually carries a non-empty secrets custom field. Devices with encrypted secrets are decrypted exactly as before; devices without secrets no longer touch the key file or Redis at all. Found by the E2E harness error guard, which treats any ERROR-level log record during config generation as a failure. Assisted-by: Claude:claude-fable-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
The harness previously installed netbox-manager into the project venv. netbox-manager pins its own versions of packages that python-osism pins exactly -- the install mutated five of them, including pynetbox -- so a local E2E run silently left the developer venv diverged from Pipfile.lock. Install netbox-manager into a dedicated venv (.venv-sonic-e2e, gitignored, reused across runs, overridable via SEED_VENV) instead. The venv's bin directory is prepended to PATH because netbox-manager drives Ansible through ansible-runner, which resolves ansible-playbook via PATH rather than relative to its own interpreter. Assisted-by: Claude:claude-fable-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Bootstrap the golden set from the netbox-manager example data via make sonic-e2e-regen: the four testbed switches plus the OOB switch, generated by sync_sonic() against a seeded NetBox 4.5.10. The files pin current generator behavior as a change detector; they were cross-checked against their independent sources rather than reviewed line by line: - hostname, hwsku, management IP, loopback addresses (v4/v6) and the eth0 MAC match the seed resources for each device - all 34 PORT entries of the AS7726-32X devices carry exactly the speeds of files/sonic/port_config/Accton-AS7726-32X.ini (the seed sets no NetBox speed overrides) - BGP ASNs follow the documented rule (4200 prefix + loopback-derived suffix, e.g. 192.168.16.27 -> 4200016027) and the interconnected spine pair shares the group minimum 4200016029 - a second seed-and-generate cycle against a fresh NetBox deployment reproduced all five files byte-for-byte (determinism) The VLAN tables are empty on every device: the example data assigns VLANs only to the switches' own management interfaces, never to data ports, so the generator's VLAN paths are not yet exercised. Coverage for those (and for the breakout regression scenarios) comes with the planned scenario seed data. Assisted-by: Claude:claude-fable-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Wire the E2E golden test into CI as python-osism-sonic-e2e: provision NetBox on a kind cluster, seed it with the netbox-manager example data, run sync_sonic() and compare the exported config_db files against tests/e2e/golden/ via tests/e2e/run.sh. The netbox-manager checkout comes from required-projects, so the job consumes its seed data and CLI at tip-of-main and a Depends-On change is tested against the changed code and data. The pre-run playbook is adapted from netbox-manager's pre-e2e.yml (same pinned kind, kubectl and Helm versions with SHA256-verified downloads, same IPv6 accept_ra workaround for SLAAC-only CI nodes) and must stay in sync with it until the setup is factored into a shared zuul-jobs role. On top of that it installs curl, openssl and python3-venv for the harness script and its seeding venv. In check the job carries a files matcher so it only runs for changes that can alter the generated output: conductor code, settings, the port_config files, the harness itself, the CI playbooks, and the dependency pins. periodic-daily runs it unconditionally, bounding silent drift of the netbox-manager seed data to a day. Assisted-by: Claude:claude-fable-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
The CI run of python-osism-sonic-e2e failed at the generation step: importing sync_sonic pulls in the whole conductor package, whose utils module does `from ansible import constants` at import time, and ansible-core was not present in the job's venv. The gap is easy to miss because every other environment masks it: the unit tests stub the ansible modules in tests/unit/conftest.py (so the unit job and a local pytest run stay green without ansible-core), and the container image installs requirements.ansible.txt explicitly. The E2E driver is the first consumer of the real import chain in a plain pipenv environment. Have run.sh install the project's own [ansible] extra (which resolves to requirements.ansible.txt, the same pin the container uses) into the project venv before generating. The step is idempotent and covers local runs from a fresh venv as well as CI. Assisted-by: Claude:claude-fable-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 breakout detection and the
kbps-to-Mbps speed normalisation are never exercised by the base
golden test -- exactly the generation logic behind the recent
customer-visible breakout speed regression.
Add a scenario overlay applied as a second seeding pass. It defines a
minimal synthetic device type (hwsku Accton-AS9726-32D) and two
devices that each break the 8-lane 400G master Eth1/1 into a 4x100G
group. The three non-master sub-ports are absent from the port_config
.ini and so are generated by _add_missing_breakout_ports. The two
devices differ only in where the sub-port speed comes from:
e2e-breakout-derived speed derived from the interface type; no
explicit NetBox speed. This is how real
deployments model breakouts.
e2e-breakout-explicit the same sub-ports with an explicit NetBox
speed in kbps -- the other unit the
collection step must normalise.
Both must yield sub-port speed 100000. A tagged-VLAN port is added on
the derived device for VLAN/VLAN_MEMBER coverage the base example
lacks.
The seed data is synthetic; real customer NetBox configs were used
only as a modelling reference for interface naming and speed
conventions, with no customer identifiers copied.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
Goldens for the two breakout scenario devices, generated against a
seeded NetBox. Both devices produce sub-port speed 100000 for the
4x100G group -- the same value whether the speed is derived from the
interface type or set explicitly in kbps.
Verified the guard by reverting the fix locally against these goldens:
- Restoring the mixed-unit collection (explicit speed left in kbps)
together with the downstream division turns the derived golden red
(Ethernet0/2/4/6 speed 100000 -> 100, the customer-visible
symptom) while the explicit golden stays green.
- Dropping the division instead turns the explicit golden red
(speed 100000 -> 100000000, raw kbps) while the derived golden
stays green.
The current code keeps both green, so either regression would now
fail the E2E test with a speed diff on the affected device.
Assisted-by: Claude:claude-fable-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
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.
Summary
Adds an end-to-end golden test for the SONiC config generator: provision NetBox on a local kind cluster, seed it with the netbox-manager example data, run
sync_sonic(), and compare every exportedconfig_db.jsonagainst golden files committed undertests/e2e/golden/. Any behavior change in the generator becomes a reviewable golden diff in the same PR.Motivation
Unit tests verify the generator against fixture values that encode the author's assumptions, so a class of regressions — such as mixed kbps/Mbps interface speeds — can pass the unit suite while producing broken switch configurations. Nothing exercised the full pipeline (real NetBox data in, complete config_db out) until now:
"speed": "100")Changes
SONIC_PORT_CONFIG_PATHsetting so the generator can read the in-repofiles/sonic/port_config/outside the container imagetests/e2e/generate.py— drivessync_sonic(); since it returns only a device→config dict and swallows per-device failures, the driver asserts success itself (expected device set derived from golden filenames, empty-config detection, a loguru sink that fails the run on any ERROR record)tests/e2e/compare.py— exact file-set equality (missing / extra / mismatched reported separately), structural diff paths likePORT|Ethernet4.speed, canonical unified diff,--regeneratefor intentional output changestests/e2e/run.sh+ Makefile targetssonic-e2e,sonic-e2e-regen,sonic-e2e-up,sonic-e2e-down; netbox-manager is installed from a checkout (sibling dir orNETBOX_MANAGER_DIR) into a dedicated venv.inispeeds, the ASN derivation rule) and reproduced byte-for-byte by a second seed-and-generate cyclepython-osism-sonic-e2ein check (restricted by a files matcher to changes that can alter generator output) and periodic-daily (unconditional, bounds seed-data drift); netbox-manager comes fromrequired-projects, soDepends-Onis honored for its code and seed data_add_snmp_configurationno longer builds the Redis-backed Ansible vault for devices without encrypted secrets — it logged three ERRORs per device in any environment without vault infrastructure (found by the new error guard)Verification
tests/unit/e2e/)Follow-up work
pre-e2e.ymlOpened as draft so the new Zuul job can validate itself on this PR (the files matcher includes
.zuul.yaml,tests/e2e/, and the playbooks).🤖 Generated with Claude Code