DO NOT MERGE: measure seeding with --parallel 4 - #2535
Closed
ideaship wants to merge 18 commits into
Closed
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>
The base example models no LAGs, so PORTCHANNEL, PORTCHANNEL_MEMBER and PORTCHANNEL_INTERFACE are emitted-but-empty. Add a device that bonds two data ports into PortChannel1, modeled the way NetBox and real deployments express a LAG: a type=lag interface plus the member ports referencing it via their lag field. Introduces a shared minimal device type (edgecore-7726-32x-e2e, hwsku Accton-AS7726-32X) with a management port and two data ports. The golden pins the resulting PORTCHANNEL / PORTCHANNEL_MEMBER / PORTCHANNEL_INTERFACE entries. Seed data is synthetic; the base example switches and the existing scenario goldens are unaffected. Assisted-by: Claude:claude-fable-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
The base example assigns no VRFs, so the whole EVPN/VXLAN/VRF
subsystem is emitted-but-empty: VRF, VXLAN_TUNNEL, VXLAN_TUNNEL_MAP,
VXLAN_EVPN_NVO, the L2VPN-EVPN BGP_GLOBALS_AF and
BGP_GLOBALS_ROUTE_ADVERTISE. Generation keys purely on NetBox VRF
objects assigned to interfaces (interface.vrf) -- no config context
or tags are involved.
Add a leaf device with a Loopback0 (its primary IP becomes the VXLAN
tunnel source) and two VRFs assigned to data ports:
VrfStorage rd 2001 (a pure number) -> VNI 2001, which populates all
of the tables above (route-target auto-derived as 2001:1)
plus the synthesized Vlan2001 and its VLAN_INTERFACE.
vrf42 no rd -> the table_id-only branch: VRF Vrf42 with
vrf_table_id 42 and no EVPN/VXLAN.
The golden pins the resulting tables. Seed data is synthetic and
reuses the shared device type from the port-channel scenario; the
base switches and existing goldens are unaffected.
Assisted-by: Claude:claude-fable-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. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Roger Luethi <luethi@osism.tech>
Add e2e-breakout-declared to the SONiC E2E golden test: an
Accton-AS9726-32D switch carrying an authoritative device-level
sonic_parameters.breakout map (no sub-ports modelled in NetBox), so a
golden diff proves the explicit declared-mode path rather than the
inference fallback.
The map covers three splits on the 8-lane platform: Ethernet0 4x100G
(2 lanes/child), Ethernet8 2x50G (4 lanes/child, the case count-based
inference never handled), and physical key Eth1/9 -> Ethernet64 8x50G
(1 lane/child, exercising key normalization). The committed golden
records the expected BREAKOUT_CFG (MANUAL owner, mode string, port_config
index), BREAKOUT_PORTS (child -> {master} only), and per-child PORT
lanes/speed.
Regenerating the goldens left all existing files byte-unchanged,
confirming the merged explicit-mode feature does not perturb the
inference path.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Roger Luethi <luethi@osism.tech>
The job runs kind plus a four-container NetBox stack plus the Ansible client on a 2 vCPU / 4 GB node. Measured node size alone accounts for 1.53× on the NetBox deploy and 2.19× on seeding. ubuntu-noble-large is SCS-8V-32-100 and already used by several OSISM jobs. Since max-servers is a count cap, job concurrency is unaffected. Assisted-by: Claude:claude-haiku-4-5-20251001 Signed-off-by: Roger Luethi <luethi@osism.tech>
The SONiC E2E golden test only needs a NetBox REST API, its
PostgreSQL and its Valkey -- it does not need Kubernetes. Replace
the kind cluster and Helm chart with a docker compose stack
(postgres, valkey, netbox) defined in tests/e2e/compose.yaml, plus
a deploy_netbox.sh that starts the stack and mints the API token.
NetBox is configured entirely through the environment variables its
own baked /etc/netbox/config/configuration.py reads, so no
configuration file needs to be mounted. API_TOKEN_PEPPERS is
deliberately left unset: the image's super_user.py only creates an
API token when a pepper is configured, and then only a "v2" one,
which pynetbox / netbox.netbox cannot use. deploy_netbox.sh instead
mints a deterministic v1 token for the superuser via `manage.py
shell`, which NetBox still accepts through v4.6. The NetBox version
is pinned to v4.5.10 because the golden files were generated against
it; postgres and valkey are pinned to a patch level.
compose.yaml's only interpolated value is NETBOX_PORT (default
8080), so `docker compose down`, `ps` and `logs` all work from an
environment where it is unset. The secret key, superuser password
and DB password are fixed literals rather than ${VAR:?} because this
stack is ephemeral, loopback-only, and thrown away after every run.
This commit is purely additive: tests/e2e/run.sh still calls
netbox-manager's kind-based deploy script, so the existing E2E
harness keeps working unchanged. Wiring run.sh to the new compose
stack is the next commit.
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
The SONiC E2E job spent roughly 1650s of its ~2100s pre+run time on the kind cluster bring-up, the Helm-installed NetBox chart, and seeding through a port-forward, while Kubernetes itself contributed nothing the test exercises -- it only added resource caps and worker backoff behind that slowness. tests/e2e/compose.yaml now runs NetBox (postgres, valkey, netbox) directly on the host via docker compose. run.sh is rewired accordingly: phase 1 now calls the local tests/e2e/deploy_netbox.sh, which starts the stack and mints the API token; the port-forward and its readiness poll are gone because `up --wait` already establishes readiness via healthchecks and the API is published on 127.0.0.1:NETBOX_PORT directly. Cluster-name and namespace variables are replaced by a `compose()` helper bound to compose.yaml. Diagnostics on failure now dump `compose ps` and the NetBox application log instead of kubectl node/pod/event output, and cleanup tears the stack down with `compose down --volumes --remove-orphans` instead of deleting a kind cluster. playbooks/pre-sonic-e2e.yml no longer installs kind, kubectl or Helm (the pinned versions/checksums and the six related tasks are removed); ensure-docker already installs docker-compose-plugin alongside docker-ce, so the node needs no new package. The accept_ra IPv6 workaround stays, since the CI node's SLAAC default route is still at risk once Docker enables forwarding. The Makefile targets are repointed the same way: sonic-e2e/-regen no longer pass CLUSTER_NAME, sonic-e2e-up runs the local deploy_netbox.sh, and sonic-e2e-down calls `docker compose ... down` instead of `kind delete cluster`. netbox-manager itself is untouched and still provisions its own E2E NetBox on kind; only this repo's fixture moved to compose. Verified with a full local `make sonic-e2e`: NetBox boots, is seeded with the netbox-manager example data plus the three scenario overlay files, 10 SONiC device configs are generated, and the exports match tests/e2e/golden/ unchanged. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Five fixes found in the final review of the docker compose NetBox fixture for the SONiC E2E golden test: - run.sh: cap dump_diagnostics' `compose logs` at the last 200 lines (--tail 200). Uncapped, it fires on the most common failure -- a golden mismatch in phase 4, where compare.py has already printed the useful diff -- and buries that diff under megabytes of NetBox first-boot migration and postgres per-connection logs in the Zuul console. 200 lines still covers a genuine boot failure. - pre-sonic-e2e.yml: rewrite the accept_ra comment, which still justified the guard by citing `kind create cluster` even though kind was removed from this playbook in an earlier commit. It now explains the guard in terms of the current setup (compose's IPv4-only default bridge probably won't trigger IPv6 forwarding) while noting the guard is kept deliberately as cheap insurance, and that removing it should be its own experiment. - pre-sonic-e2e.yml: correct the "Install required packages" comment, which claimed curl is used by run.sh directly; it is now only used by the NetBox container's healthcheck. - .zuul.yaml: update the stale job comment saying NetBox is provisioned "on kind" to describe the docker compose stack. - deploy_netbox.sh: validate NETBOX_TOKEN is exactly 40 hex characters right after it is defaulted. This guards against a caller-supplied token breaking out of the Python string literal it is interpolated into (a quote or newline), and against a malformed token being silently rejected by NetBox much later, far from the actual cause. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Throwaway measurement commit -- not for merge. Seeding is the largest remaining item: 555s of the 1335s pre+run on 8 vCPU, i.e. 42%. Locally, --parallel 4 cut it from 302s to 167s (1.81x) on an otherwise identical stack. Safe on ordering: netbox-manager sorts resource files by leading number, runs the groups sequentially, and parallelises only within a group, so 000 -> 100 -> 200 -> 300 still holds. Static analysis of the 16 files in the 300- group found 251 distinct objects touched with zero overlap between files. The golden comparison is itself the race detector -- interleaved seeding would change the generated configs. The overlay call gets the flag for consistency but should not benefit: its three files are in distinct numeric groups and serialise anyway. This also tests whether the ubuntu-noble-large upgrade earns its keep. Compose on 2 vCPU measured 1235s vs 1335s on 8 vCPU -- no gain, because the critical path is single-threaded. Parallel seeding is the first change that could actually use the extra cores. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Contributor
Author
|
Measurement complete — closing as planned. Result: large win. Base seeding 250.0s vs 554.8s baseline = 2.22x, better than the 1.81x measured locally. pre+run 1025s vs 1335s (-310s). Goldens matched, so no race manifested. The overlay pass was unchanged (49.8s vs 47.4s) exactly as predicted — its three files sit in distinct numeric groups and serialise regardless. Worth adopting in #2532. |
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.
Throwaway measurement PR — do not review, do not merge. Closed once it reports a number.
Identical to the draft in
plus
--parallel 4on the netbox-manager seeding calls.Question: how much does parallel seeding save in CI, and does
ubuntu-noble-largefinally earn its keep?Seeding is the largest remaining item — 555s of the 1335s pre+run, i.e. 42%. Locally
--parallel 4cut it 302s → 167s (1.81×).This doubles as a test of the node upgrade. Compose measured 1235s on 2 vCPU vs 1335s on 8 vCPU — no gain, because the critical path is single-threaded (sequential Django migrations, then one Ansible process issuing serial API calls). Parallel seeding is the first change that could actually use the extra cores, so if the nodeset upgrade is ever going to pay off, it is here.
Ordering is preserved: netbox-manager sorts resource files by leading number, runs groups sequentially, and parallelises only within a group. Static analysis of the 16 files in the
300-group found 251 distinct objects touched with zero overlap. The golden comparison is itself the race detector — interleaved seeding would alter the generated configs, so a green run is meaningful evidence.🤖 Generated with Claude Code