sonic: document and test config generator ownership model#2297
Draft
ideaship wants to merge 2 commits into
Draft
sonic: document and test config generator ownership model#2297ideaship wants to merge 2 commits into
ideaship wants to merge 2 commits into
Conversation
The SONiC config generator starts from a deep copy of the device's /etc/sonic/config_db.json, but helpers subsequently replace whole entries in certain sections via unconditional assignment rather than merging. The implicit rule is that generated sections are fully owned by the generator and always overwritten on regen; only sections not touched by the generator pass through unchanged. This assumption was first surfaced during review of PR #2279, which expanded the set of overwritten keys in ROUTE_REDISTRIBUTE. It was noted again during review of PR #2290, whose new tests use empty scaffolds and therefore do not constrain the base-config overwrite behavior. In both cases the assumption was undocumented, leaving reviewers and contributors without a clear statement of intent. Document the ownership model in the generate_sonic_config() docstring so that the rule is visible to contributors maintaining the function, and so that future PRs can be evaluated against an explicit contract rather than an implicit one. AI-assisted: Claude Code Signed-off-by: Roger Luethi <luethi@osism.tech>
The previous commit documented the config generator ownership model in
the generate_sonic_config() docstring: generated sections are fully
owned by the generator and entries in those sections are unconditionally
overwritten on regen, not preserved from /etc/sonic/config_db.json.
Without tests the rule is only a claim. This commit adds two groups
of tests to make it concrete:
Illustrative tests (test_config_generator_ownership.py):
- _add_vrf_configuration replaces BGP_GLOBALS[vrf_name] wholesale;
any operator-added field (e.g. a custom timer) is silently dropped.
- _add_vrf_configuration replaces VLAN[Vlan{vni}] wholesale; any
operator-added field (e.g. a description) is silently dropped.
- _add_vrf_configuration resets the ROUTE_REDISTRIBUTE entry for the
generated key to {}; any pre-existing route policy is dropped.
- Sections not written by these helpers pass through unchanged.
- _add_vlan_configuration replaces VLAN[VlanX] wholesale; any
operator-added field is silently dropped.
Violation test (test_config_generator_orchestrator.py):
- Pre-existing fields in BGP_GLOBALS['default'] must not survive
regen per the ownership rule. This test currently fails because
the orchestrator writes BGP_GLOBALS['default'] via key-level
updates rather than replacing the entry. Reviewers must decide
whether to fix the orchestrator to comply, or refine the ownership
rule to treat the default VRF as a merge-only exception.
AI-assisted: Claude Code
Signed-off-by: Roger Luethi <luethi@osism.tech>
ideaship
force-pushed
the
doc-sonic-generator-ownership-model
branch
from
cae57c6 to
9952d93
Compare
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.
Motivation
PRs #2279 and #2290 both ran into the same unresolved question: when
generate_sonic_config()regenerates a SONiC config, which sections does it own outright, and which can an operator customize directly inconfig_db.json?In #2279, a reviewer noted the assumption implied by unconditional assignment to
ROUTE_REDISTRIBUTEbut left the question open. In #2290, new tests used empty scaffolds, so they neither confirmed nor denied base-config preservation — and the same question arises on closer inspection of that PR. This PR tries to break the pattern.What this PR does
Docstring (
generate_sonic_config): states the ownership model as the code largely implements it — generated sections (BGP_GLOBALS, VLAN, VLAN_INTERFACE, ROUTE_REDISTRIBUTE, VXLAN_TUNNEL, VXLAN_TUNNEL_MAP, etc.) are fully owned by the generator; entries in those sections are unconditionally overwritten on each regen, and pre-existing values fromconfig_db.jsonare not preserved.Illustrative tests (
test_config_generator_ownership.py): five passing tests showing what "owned" means in practice — pre-existing operator fields inBGP_GLOBALS[vrf_name],VLAN[Vlan{vni}],ROUTE_REDISTRIBUTE[key], andVLAN[VlanX]are silently dropped on regen.Violation test (
test_config_generator_orchestrator.py): one failing test that exposes an inconsistency in the current implementation —BGP_GLOBALS["default"]is handled differently from the rest. The orchestrator updates it with key-level writes rather than replacing the entry, so pre-existing fields survive regen. The test is intentionally left failing to flag this for discussion.What this PR is not
This is not a proposal to enforce a specific policy, and it does not fix the
BGP_GLOBALS["default"]inconsistency. The goal is to make the implicit rule visible and invite a decision:BGP_GLOBALS["default"]specifically: should the orchestrator be fixed to replace the entry on regen (which would make the failing test pass), or should the rule be refined to treat the default VRF as a merge-only exception?Settling this once would avoid relitigating it in every PR that touches the config generator.