Correct base config provenance and make its path configurable - #2561
Merged
Conversation
The config ownership model described the file the generator builds on as "the device's image-provided base config_db.json". That is wrong, and the error is not cosmetic: it is the premise the model reasons from. The base config is this repo's files/sonic/config_db.json, which the Containerfile installs into the conductor image at /etc/sonic/config_db.json. generate_sonic_config() reads that path from its own container filesystem. Nothing from a switch's own config_db.json ever reaches the generator, and no code writes that path at runtime. Everything device-specific comes from NetBox and the policy in this module, while the base config supplies the shared content of the inherited, read-only and pass-through tables. The file is therefore an input this repo controls, not foreign vendor or operator content. Two consequences follow, and the model captured neither. The "operator hand-edits are unsupported" rationale that justified drop-and-rebuild aimed at a surface that does not exist on this path: a file baked into a container image is not operator-editable to begin with. Meanwhile the role that file actually had -- carrying shipped defaults -- went unrecognised, so classifying those tables as owned silently deleted them. That is how the default-VRF regression came about: the BGP entries shipped in the base config, the owned-table drop removed them on every regen, nothing regenerated them, and the default VRF stopped advertising its routes into EVPN. #2515 Correct the provenance wherever it is stated -- the ownership model on generate_sonic_config, the INHERITED/READ_ONLY constant comments, the DEFAULT_VRF_* comment, _get_gnmi_port, and the test docstrings that restate the taxonomy. Add a "Where defaults belong" paragraph to the model, because the provenance alone does not tell a reader what to do: owned tables are dropped before any helper runs, so a populated owned table in the base config is dead content, and defaults the generator needs must live in this module as policy constants. The rule is about owned tables specifically: a read-only table such as TELEMETRY legitimately lives only in the base config, because the generator reads it rather than emitting it. Note separately that files/ is not Python package data, so a generator running outside the conductor image finds no base config at all. Rename IMAGE_CONSUMED_TABLE_KEYS to READ_ONLY_TABLE_KEYS. Fixing the prose while the constant still says "image" would leave the documentation contradicting the name, and "image" is precisely the word that invited the confusion: the base config does come from an image, but the conductor's, not the switch's. The replacement names the behaviour rather than the source, because the source no longer discriminates -- inherited and pass-through tables come from the base config too -- while "read but never written" is what sets this category apart, and is how the docstring already defines it. Documentation, comments and test docstrings only; no behaviour change; the SONiC unit tests pass unchanged. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Owned tables are dropped before any helper runs, so content the shipped base config_db.json puts in one is discarded on every regen: it reads as configuration but never reaches a switch. Nothing enforced that. The default-VRF regression is exactly this failure. The BGP entries shipped in files/sonic/config_db.json, the owned-table drop removed them, nothing regenerated them, and the default VRF stopped advertising its routes into EVPN: #2515 No existing test could catch it, and adding more of the same would not help. Every base-config fixture is built from TOP_LEVEL_SCAFFOLD_KEYS with all tables empty (make_base_config), so dropping populated content is indistinguishable from dropping nothing -- the orchestrator test asserting BGP_GLOBALS equals exactly router_id/local_asn passes either way. The exhaustive stale-entry sweep seeds a sentinel into every owned table and asserts it is gone, which verifies the drop works; the drop is the mechanism at fault, so a test that asserts deletion happens cannot detect over-deletion. Add a static guard that reads the real shipped file and fails when it populates a generator-owned table. It needs no fixture, so it cannot drift from production the way the scaffolds did, and it generalises: it fires for the next table that gets orphaned, not just for the five that were. Verified against the pre-fix base config, where it names BGP_GLOBALS, BGP_GLOBALS_AF, BGP_GLOBALS_ROUTE_ADVERTISE, ROUTE_REDISTRIBUTE and VRF. The failure message points at the fix -- move the values into policy constants and emit them from a helper -- and warns against the easy way out, adding the table to the allowlist. SNMP_SERVER is the one allowlisted table: _add_snmp_configuration emits SNMP_SERVER["SYSTEM"] on every run with hardcoded defaults regardless of NetBox data, so the shipped entry is redundant rather than load-bearing. It could be dropped from the base config, which would let the allowlist go away; that is left out here to keep this change test-only. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
The base config_db.json path was hardcoded to /etc/sonic/config_db.json. That is correct inside the conductor image, where the Containerfile installs files/sonic/config_db.json at exactly that path, and wrong everywhere else: a generator running outside the image finds nothing there and silently proceeds with an empty base config. Silently, because the miss was logged at debug level. Nothing in the output says so either -- the config still generates, it just omits the DEVICE_METADATA localhost fields the generator does not write itself, every pass-through table, and the TELEMETRY gNMI port the control-plane ACL rule reads, which falls back to a default. A golden test harness generating in a venv rather than the conductor image hits exactly this: its goldens carry the 38 tables the generator emits rather than the 58 a real conductor starts from. Add SONIC_BASE_CONFIG_PATH, defaulting to the previous hardcoded value so production behaviour is unchanged, and read it through settings. A harness can then point at the in-repo files/sonic/config_db.json and generate what the conductor generates. Raise the not-found log from debug to warning. Generating without a base config is not a routine condition to be discovered by turning debug logging on; it means the output is incomplete, and the operator or harness should see that. ERROR is too strong: the fallback is deliberate and the run still produces usable config for the owned tables, which is what a from-scratch device needs. Tests cover the setting's default and override, that the generator reads the configured path rather than a hardcoded one, and that the missing-base case warns. patch_base_config now returns the open mock so a test can assert on the path it was called with. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The SHIPPED_BASE_CONFIG_PATH calculation in the ownership test relies on
Path(__file__).parents[5] / 'files' / 'sonic' / 'config_db.json', which is quite brittle to repo layout changes; consider anchoring this via a more stable reference (e.g. a known top-level marker file/dir or a helper that centralizes the repo root resolution) to make the guard more resilient to future restructuring.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The SHIPPED_BASE_CONFIG_PATH calculation in the ownership test relies on `Path(__file__).parents[5] / 'files' / 'sonic' / 'config_db.json'`, which is quite brittle to repo layout changes; consider anchoring this via a more stable reference (e.g. a known top-level marker file/dir or a helper that centralizes the repo root resolution) to make the guard more resilient to future restructuring.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9 tasks
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
3 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.
Context for the wider effort this came out of: #2562. This change is
independent of that series and can land in either order — nothing in the series
uses the setting added here.
The SONiC config generator builds each device's config on top of a base
config_db.json. That base is this repo'sfiles/sonic/config_db.json,which the Containerfile installs into the conductor image at
/etc/sonic/config_db.json— nothing from a switch's own config ever reachesthe generator.
Until now that path was hardcoded, and the ownership model documented it as
"the device's image-provided base config_db.json", which is the switch's image
rather than the conductor's. Both problems have consequences.
What the hardcoded path costs
A generator running outside the conductor image — local development, or a test
harness in a venv — finds nothing at that path and proceeds with an empty
base. Silently, because the miss was logged at debug level, and because the
generator's own output is unaffected: every table it emits is still there.
What is missing is everything only the base supplies — the
DEVICE_METADATAlocalhost fields the generator does not write itself (
type,default_config_profile,frr_mgmt_framework_config,intf_naming_mode) andevery pass-through table:
FEATURE,CLASSIFIER_TABLE, thePOLICY_*set,the
ECMP_LOADSHARE_*pair,SWITCH,ZTP,KDUMPand the rest. Countedagainst the shipped base, that is 24 tables populated in production that a
venv-generated config omits entirely. The read-only
TELEMETRYgNMI port isabsent too, but degrades to
DEFAULT_GNMI_PORTrather than vanishing.What the wrong provenance cost
Classifying the base config as foreign, operator-editable content justified
dropping and rebuilding owned tables — but a file baked into a container image
is not an operator-editable surface to begin with. Meanwhile the role that file
actually had, carrying shipped defaults, went unrecognised, so classifying those
tables as owned silently deleted their content on every regen.
That is the default-VRF regression: BGP entries shipped in the base config, the
owned-table drop removed them, nothing regenerated them, and the default VRF
stopped advertising its routes into EVPN.
The three commits
sonic: document base config as repo-shipped— correct the provenancewherever it is stated, and add a "Where defaults belong" paragraph, because the
provenance alone does not tell a reader what to do: owned tables are dropped
before any helper runs, so a populated owned table in the base config is dead
content and defaults the generator needs must live in the module as policy
constants. Renames
IMAGE_CONSUMED_TABLE_KEYStoREAD_ONLY_TABLE_KEYS, since"image" is the word that invited the confusion. Documentation and comments only.
sonic: guard base config against owned content— the substantive part. Astatic test reads the real shipped file and fails when it populates a
generator-owned table. It needs no fixture, so it cannot drift from production,
and it generalises: it fires for the next table that gets orphaned, not just the
five that were. Verified against the pre-fix base config, where it names
BGP_GLOBALS,BGP_GLOBALS_AF,BGP_GLOBALS_ROUTE_ADVERTISE,ROUTE_REDISTRIBUTEandVRF.Worth being explicit about why more of the existing style of test would not
have helped: every base-config fixture is built from
TOP_LEVEL_SCAFFOLD_KEYSwith all tables empty, so dropping populated content is indistinguishable from
dropping nothing. The exhaustive stale-entry sweep seeds a sentinel into every
owned table and asserts it is gone, which verifies the drop works — but the drop
is the mechanism at fault, and a test asserting that deletion happens cannot
detect over-deletion.
SNMP_SERVERis the one allowlisted table:_add_snmp_configurationemitsSNMP_SERVER["SYSTEM"]on every run with hardcoded defaults regardless ofNetBox data, so the shipped entry is redundant rather than load-bearing. It
could be dropped from the base config, which would let the allowlist go away;
left out here to keep that commit test-only.
sonic: make the base config path configurable— addSONIC_BASE_CONFIG_PATH, and raise the not-found log from debug to warning.Generating without a base config is not a routine condition to discover by
turning debug logging on; it means the output is incomplete.
ERRORwould betoo strong, since the fallback is deliberate and the run still produces usable
config for the owned tables, which is what a from-scratch device needs.
Blast radius
Nothing changes in production or in CI.
SONIC_BASE_CONFIG_PATHdefaults to theprevious hardcoded value, so the conductor behaves exactly as before, and no
caller sets it yet — no generated config moves, and no golden file changes. What
the PR buys is that a harness or a developer can now point at the in-repo copy
and generate what the conductor generates.
Verification
Full unit suite passes (3083 passed, 4 pre-existing xfails).
flake8andblackclean.