fix(sonic): emit up/down for BGP_NEIGHBOR_AF - #2618
Open
ideaship wants to merge 1 commit into
Open
Conversation
The generator wrote {"admin_status": "true"} for every BGP_NEIGHBOR_AF
row. The ConfigDB YANG typedef admin_status is a strict enumeration of
up and down (sonic-types.yang.j2), so "true" is outside the type, and
upstream's own YANG test config for this table uses "up". Every other
table the generator writes -- PORT, PORTCHANNEL, LOOPBACK, VLAN,
VLAN_INTERFACE and the MGMT_ tables -- already emits up/down, which
left BGP_NEIGHBOR_AF as the lone outlier.
This is not cosmetic. config reload runs YANG validation and aborts on
failure. On sonic-utilities master the no-filename form validates
/etc/sonic/config_db.json, which is exactly how this project applies a
generated config, so "true" becomes a hard failure in a future
release; on 202405 and later an explicit-filename reload already
aborts.
Behaviour on the switch is unchanged, because frrcfgd converts up and
down to true and false internally. Switches already running a
generated config hold "true" and will show the new value as a diff the
next time their config is regenerated.
The bundled validator rejected every generated AF row before this
change and accepts them now, which is what allows osism sonic validate
to be used as a gate.
Twelve assertions in the BGP test module are updated to match.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. This changes the administrative state value emitted for every affected BGP neighbor address family, which could activate or fail to activate routing sessions and cause route withdrawal or a network outage. Reverting restores the previous generated configuration, but any traffic disruption or route changes that occurred before the revert are not undone.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
Problem
The SONiC config generator wrote
{"admin_status": "true"}for everyBGP_NEIGHBOR_AFrow. That field is typedstypes:admin_statusinthe AF grouping, and the typedef is a strict enumeration:
—
sonic-types.yang.j2#L88-L94So
"true"is outside the type, not a lenient spelling of it. Upstream's ownYANG test config for this table uses
"up", with a real boolean(
send_default_route) immediately beside it, so the model distinguishesdeliberately:
tests_config/bgp.json#L387-L395Every other table this generator writes —
PORT,PORTCHANNEL,LOOPBACK,VLAN,VLAN_INTERFACEand theMGMT_tables — already emitsup/down.BGP_NEIGHBOR_AFwas the lone outlier.Why it is not cosmetic
config reloadruns YANG validation and aborts on failure, viaconfig_file_yang_validation. Onsonic-utilitiesmaster theno-filename form validates
/etc/sonic/config_db.json:—
config/main.py#L2226-L2258That is exactly how this project applies a generated config (
config loadfollowed by
config reload -y), so"true"becomes a hard failure in afuture release.
On 202505 the validator exists but is reached only under
if filename is not None, the explicit-filename path:config/main.py#L1961-L1967. So an explicit-filename reloadalready aborts today, while the no-filename form does not. The block quoted
above is what master adds.
Every link is pinned to a commit SHA so the line ranges stay valid:
sonic-buildimage8aaea5e3649a,sonic-utilitiese3768cbdc0f4and7511bf2f8ea3(202505).Impact
Behaviour on the switch is unchanged:
frrcfgd'shdl_admin_statusconvertsup/downtotrue/falseinternally, so the consumer already expects thisspelling:
frrcfgd.py#L1456-L1466Switches running a previously generated config hold
"true"and will show thenew value as a diff the next time their config is regenerated. Harmless, for
the same reason.
The bundled ConfigDB validator rejected every generated AF row before this
change (
Input should be 'up' or 'down') and accepts them now. That removes thelargest source of noise from
osism sonic validatefor these tables, but thecommand is not yet usable as a gate:
PORT.adv_speedsis modelled as a listwhile ConfigDB carries a string, and
BGP_NEIGHBOR.local_addris flaggedagainst its union's leafref arms while a plain
inet:ip-addressarm makes thevalue legal. Both are separate.
Tests
Twelve assertions in the BGP test module are updated to match.
712 passedintests/unit/tasks/conductor/sonic/,3139 passedintests/unit(4pre-existing xfails), flake8 and black clean.
Note for reviewers
No committed golden pins the old value: the SONiC E2E goldens are not in
main, they exist only on the draft stack rooted at:The matching golden update belongs to that stack and lands with it, so this
change breaks nothing currently in
main.🤖 Generated with Claude Code