[Nexthop][fboss2-dev] Add fboss2-dev config protocol bgp global command#1345
Open
hillol-nexthop wants to merge 2 commits into
Open
[Nexthop][fboss2-dev] Add fboss2-dev config protocol bgp global command#1345hillol-nexthop wants to merge 2 commits into
hillol-nexthop wants to merge 2 commits into
Conversation
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
2 times, most recently
from
7ef6303 to
5512012
Compare
Make the fboss2 ConfigSession and `config session` lifecycle BGP-aware. This is
the base for the `config protocol bgp` commands, which stack on top.
- ConfigSession owns the BGP config as a typed bgp::thrift::BgpConfig, exactly
the way it owns cfg::AgentConfig for the agent. getBgpConfig()/saveBgpConfig()
expose the WHOLE typed config and stage it to ~/.fboss2/bgp_config.json.
ConfigSession is BGP-aware but agnostic about the config's internal structure:
it has no notion of "global" vs "peer" vs "peer-group" -- a command mutates
whichever typed fields it needs, mirroring how interface commands mutate
getAgentConfig().sw()->ports(). Because the whole config round-trips through
the typed struct, no surgical merge / preserve-list is needed.
- commit() promotes bgp_config.json to /etc/coop/bgpcpp/bgpcpp.conf, restarts
bgp_pp, and versions it in the same /etc/coop git repo as agent.conf.
- config session clear/diff/rebase/rollback are BGP-aware: clear removes a
staged BGP session, diff shows staged BGP changes, rebase 3-way-merges
bgpcpp.conf, rollback restores bgpcpp.conf + restarts bgp_pp (using metadata
history so BGP-only commits are reachable).
- FbossServiceUtil + cli_metadata gain the bgp_pp (BGP_PP) service/action; the
config lib gains the bgp_config cpp2 dependency.
- Unit tests in CmdConfigSession{,Diff}Test cover the typed round-trip, commit,
rebase, and rollback behavior.
Test Plan:
- bazel test //fboss/cli/fboss2/test/config:cmd_config_test //fboss/cli/fboss2/test:cmd_test
- fboss2_integration_test on a DUT (ConfigConcurrentSessionsTest.ConflictAndRebase,
ConfigSessionClearTest, ConfigInterfaceMtuTest, ConfigInterfaceDescriptionTest): 5/5 pass
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `fboss2-dev config protocol bgp global <attr> <value>` on top of the BGP-aware ConfigSession (base PR). Edits the typed bgp::thrift::BgpConfig via ConfigSession::getBgpConfig()/saveBgpConfig() -- the whole-config, scope-agnostic typed API (no global/peer/peer-group special-casing in ConfigSession). - Collapse the 10 per-attribute global command classes into one dispatcher; reject cluster-id (no BgpConfig field) instead of writing dead config; bound switch-limit / max_golden_vips so out-of-range values aren't truncated. - Integration tests: ConfigBgpGlobalTest (each attr set+commit, verified in the promoted /etc/coop/bgpcpp/bgpcpp.conf) and ConfigBgpSessionTest (clear/diff/commit/rollback + a no-op-restart regression), sharing ConfigBgpTestBase. Test Plan: - bazel test //fboss/cli/fboss2/test/config:cmd_config_test - bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test - fboss2_integration_test on a DUT with bgp_pp active: ConfigBgpSessionTest 6/6 pass (clear, diff, commit-restarts-bgp_pp, rollback-restores-config, and the unchanged-config does-not-restart case); agent-session regression (ConfigInterfaceMtuTest) passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
from
5512012 to
c1ab18b
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
1 participant
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
Add
fboss2-dev config protocol bgp global <attr> <value>on top of theBGP-aware
ConfigSession. It edits the typedbgp::thrift::BgpConfigviaConfigSession::getBgpConfig()/saveBgpConfig()— the whole-config,scope-agnostic typed API (no global/peer/peer-group special-casing in
ConfigSession).rejects
cluster-id(noBgpConfigfield) instead of writing dead config;bounds
switch-limit/max_golden_vipsso out-of-range values aren'ttruncated.
ConfigBgpGlobalTest(each attribute set+commit, verifiedin the promoted
/etc/coop/bgpcpp/bgpcpp.conf) andConfigBgpSessionTest(clear/diff/commit/rollback + an unchanged-config no-op-restart regression),
sharing
ConfigBgpTestBase.Test Plan
fboss/cli/fboss2/test/config:cmd_config_testfboss2_integration_teston a DUT withbgp_ppactive:ConfigBgpSessionTest6/6 pass (clear, diff, commit-restarts-bgp_pp,rollback-restores-config, unchanged-config-does-not-restart); agent-session
Integration test output (
bgp_ppactive)