[Nexthop][fboss2-dev] Add fboss2-dev config protocol bgp global command - #1345
Open
hillol-nexthop wants to merge 6 commits into
Open
[Nexthop][fboss2-dev] Add fboss2-dev config protocol bgp global command#1345hillol-nexthop wants to merge 6 commits into
hillol-nexthop wants to merge 6 commits into
Conversation
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
3 times, most recently
from
June 30, 2026 14:27
5512012 to
c1ab18b
Compare
hillol-nexthop
marked this pull request as ready for review
June 30, 2026 16:26
induvsuresh
approved these changes
Jul 13, 2026
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
5 times, most recently
from
July 16, 2026 09:14
ff6beed to
1b66091
Compare
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
2 times, most recently
from
July 17, 2026 14:07
eccdab8 to
d2af7de
Compare
This was referenced Jul 20, 2026
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
from
July 21, 2026 15:14
3e2f959 to
e9b8e2b
Compare
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
from
July 31, 2026 16:11
e9b8e2b to
b5ace7d
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
from
July 31, 2026 16:14
b5ace7d to
ff0770b
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D113469141. (Because this pull request was imported automatically, there will not be any future comments.) |
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
from
August 3, 2026 03:14
ff0770b to
b8343e5
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Follow-up cleanups to the BGP-aware config session infra (facebook#1344): - Collapse the BGP_RESTART action level into AGENT_WARMBOOT (bgpd has no hitless reload; its restart already runs the agent-warmboot code path). - Introduce a single ConfigDomain descriptor + configDomains() and shared per-domain helpers so commit(), rollback() and `config session diff` handle the agent and BGP domains uniformly (private DiffDomain removed). - Make the agent skip-when-unchanged like BGP: a commit whose staged config equals what is already promoted is a true no-op (no git revision, no symlink churn, no reloadConfig()/bgpd restart). Change detection is semantic (compare the deserialized thrift structs), so formatting-only diffs don't count. - Consolidate `config session clear` onto a static stagedSessionFilePaths() and reuse ConfigSession::readStagedContent() in diff. - Make ConfigSession::saveConfig(service, level) generic over the service and reduce saveBgpConfig() to a thin wrapper. - Keep the heavy generated thrift headers out of ConfigSession.h: use the *_types_fwd.h forward-declaration headers, hold agentConfig_/bgpConfig_ by std::unique_ptr, and drop the configLoaded_/bgpConfigLoaded_ bools (null == not loaded). - clang-tidy: use auto for the SimpleJSONSerializer template-cast results. Built fboss2-dev + the config unit tests; config-session/commit/diff/BGP/clear tests pass. Verified the agent no-op behaviour live on test switches.
Removed comments about the destructor definition in ConfigSession.h.
…thInterface call main renamed findFirstEthInterface() to getRandomInterfacePortName() (virtual-management-port fix); convert the branch-added no-op-commit test to the new helper. Drop the three includes ConfigSession.cpp no longer uses directly (misc-include-cleaner runs as errors in CI).
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
from
August 10, 2026 04:44
367d892 to
67cae8c
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
This was referenced Aug 10, 2026
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.
- Extract the value parsers (parseBool/parseInt/parseNonNegInt32, handler Result) into a shared BgpCliValueParsers.h so sibling BGP dispatchers can reuse them, and add a bounded parseAsn4Byte: local-asn/confed-asn accepted any uint64 and silently persisted out-of-range ASNs (>= 2^32 wrap the i64 field negative). - positionals_at_end() on the global command: CLI11's parent-chain subcommand fallthrough steals value tokens that match a sibling command name (e.g. a policy named "peer-group") and misparses the command. - Integration test base: probe the bgpd unit, and pass -c safe.directory=/etc/coop on the raw git invocations (gitHead / bgpTrackedAtRevision), mirroring the CLI's Git class -- /etc/coop is owned by another user (e.g. coop) on provisioned devices, which git otherwise rejects as dubious ownership and the helpers silently return empty results.
The commit-path global-attribute tests asserted on the promoted /etc/coop/bgpcpp/bgpcpp.conf, which only proves what the CLI wrote to disk. Route ConfigBgpTestBase::setAndCommit() through a new readRunningBgpConfigViaRpc() -- TBgpService::getRunningConfig against the local bgpd -- so every positive ConfigBgpGlobalTest asserts its attribute in the daemon's own view of its config, proving bgpd parsed and adopted the promoted file after the commit-triggered restart. The helper retries briefly on connection errors: systemd reports bgpd active as soon as the process starts, but its thrift server binds the port a few seconds later, so an RPC issued right after a restart races that window. Test Plan: - bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test - fboss2_integration_test on a DUT with bgpd active: ConfigBgpGlobalTest 7/7 pass (each attribute set+commit verified in bgpd's getRunningConfig view, plus the invalid-bool and negative-graceful-restart-time reject paths).
hillol-nexthop
force-pushed
the
bgpglobal-global
branch
from
August 10, 2026 10:12
67cae8c to
73261c3
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
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.
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)