[Nexthop][fboss2-dev] Unify agent/BGP config session + decouple ConfigSession header - #1446
Open
hillol-nexthop wants to merge 4 commits into
Open
[Nexthop][fboss2-dev] Unify agent/BGP config session + decouple ConfigSession header#1446hillol-nexthop wants to merge 4 commits into
hillol-nexthop wants to merge 4 commits into
Conversation
This was referenced Jul 31, 2026
hillol-nexthop
force-pushed
the
bgp-session-unify
branch
from
July 31, 2026 16:14
52a0a62 to
0a68317
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 D114376130. (Because this pull request was imported automatically, there will not be any future comments.) |
hillol-nexthop
force-pushed
the
bgp-session-unify
branch
from
August 3, 2026 03:14
0a68317 to
d9cff93
Compare
|
@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
bgp-session-unify
branch
from
August 10, 2026 04:44
d9cff93 to
34f671e
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
This was referenced Aug 10, 2026
A first `config protocol bgp ...` edit on a freshly imaged box discards the running BGP config, leaving bgpd to crash-loop on an unset router_id. The bgp++ RPM installs /etc/coop/bgpcpp.conf as a plain file (router_id set), and the unit starts bgpd with --config /etc/coop/bgpcpp.conf. ConfigSession, however, treated the *promoted* /etc/coop/bgpcpp/bgpcpp.conf as the live read for BGP — a file that does not exist until the first commit. So loadBgpConfig() fell through to schema defaults, and the commit (which also replaces /etc/coop/bgpcpp.conf with a symlink into bgpcpp/) promoted that near-empty config over the running one. This is what fails as ConfigBgpGlobalTest.SetCountConfedsInAsPathLenTrue on a pristine box: bgpd never binds its thrift port and the test throws Connection refused; the next BGP test passes because teardown restarts bgpd. The agent domain never had this bug: its systemPath is the symlink the agent actually reads, so seeding works whether that path is still a plain file or already the promoted symlink. BGP pointed it at the promoted file instead. That inconsistency is the bug. Change: - ConfigDomain.systemPath for BGP -> getBgpSystemConfigLinkPath(). Safe because nothing writes via systemPath: writes use promotedPath, symlink creation uses symlinkPath, and systemPath is read only by 'config session diff'. - loadBgpConfig() seeds from staged edits -> the daemon's config path -> the promoted path as a backstop for a missing symlink. - initializeGit() populates the promoted path from the daemon's config path before the baseline commit, so the first revision carries a BGP snapshot and a rollback to it cannot delete the running bgpcpp.conf. (The companion ConfigBgpTestBase::systemBgpConfigPath() change is omitted here: that fixture does not exist on this branch yet and the hunk rides the BGP test PRs stacked above.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@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
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
Follow-up to #1344 (BGP-aware fboss2 config session): unify the agent and BGP
domains inside
ConfigSessionand decouple theConfigSession.hheader fromthe heavy generated thrift types.
BGP_RESTARTaction level intoAGENT_WARMBOOT(bgpd has nohitless reload; its restart already runs the agent-warmboot code path).
ConfigDomaindescriptor +configDomains()and sharedper-domain helpers so
commit(),rollback()andconfig session diffhandle the agent and BGP domains uniformly (private
DiffDomainremoved).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.
config session clearonto a staticstagedSessionFilePaths()and reuse
ConfigSession::readStagedContent()in diff.ConfigSession::saveConfig(service, level)generic over the service andreduce
saveBgpConfig()to a thin wrapper.ConfigSession.h: use the*_types_fwd.hforward-declaration headers, holdagentConfig_/bgpConfig_by
std::unique_ptr, and drop theconfigLoaded_/bgpConfigLoaded_bools(null == not loaded).
Test Plan
fboss/cli/fboss2/test/config:cmd_config_testpasses (the config-session /commit / diff / BGP / clear unit tests cover the refactored paths).
//fboss/cli/...builds and tests pass on top ofupstream/main.