[Nexthop][fboss2-dev] Unify agent/BGP config session + decouple ConfigSession header by hillol-nexthop · Pull Request #1446 · meta/fboss

hillol-nexthop · 2026-07-31T16:10:43Z

Base of the fboss2-dev config protocol bgp stack (follow-up to the merged #1344). Everything above it in the list below builds on this PR, so merge this one first.

Stack (bottom → top): #1446 (this PR) → #1345 → #1391 → #1395 → #1401 → #1485 → #1475 → #1486 → #1477 → #1487 → #1488 → #1489 → #1490 → #1491

Summary

Follow-up to #1344 (BGP-aware fboss2 config session): unify the agent and BGP
domains inside ConfigSession and decouple the ConfigSession.h header from
the heavy generated thrift types.

Make ConfigActionLevel generic across services (review feedback below):
HITLESS / SERVICE_RESTART / DISRUPTIVE_SERVICE_RESTART replace
HITLESS / AGENT_WARMBOOT / AGENT_COLDBOOT (+ the bgpd-only BGP_RESTART).
What a level means is decided per service from the (service, level) pair in
FbossServiceUtil::restartTypeName(): agent → warmboot / coldboot, bgpd (no
warmboot or hitless reload) → plain restart. config session commit labels
the restarted services accordingly (wedge_agent (warmboot), bgpd (restart)).
A session staged before this change (old level names in
~/.fboss2/cli_metadata.json) no longer parses and loses its recorded
restart level (WARNING logged) -- config session clear it first.
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).

Test Plan

fboss/cli/fboss2/test/config:cmd_config_test passes (the config-session /
commit / diff / BGP / clear unit tests cover the refactored paths); re-run in
full after the action-level rename (871 tests).
//fboss/cli/... builds and tests pass on top of upstream/main.
Verified the agent no-op commit behaviour live on test switches.

facebook-github-tools · 2026-07-31T16:14:04Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

meta-codesync · 2026-07-31T16:18:27Z

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.)

facebook-github-tools · 2026-08-03T03:14:58Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

facebook-github-tools · 2026-08-10T04:45:54Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

facebook-github-tools · 2026-08-11T05:27:57Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

induvsuresh

Overall lgtm but have some comments on whether BGP restart has been removed on config save

facebook-github-tools · 2026-08-19T06:08:18Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

facebook-github-tools · 2026-08-19T10:20:06Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

facebook-github-tools · 2026-08-19T10:55:18Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

facebook-github-tools · 2026-09-02T05:43:29Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

joseph5wu · 2026-09-02T22:25:25Z

 enum ConfigActionLevel {
  HITLESS = 0, // Can be applied with reloadConfig() - default
-  AGENT_WARMBOOT = 1, // Requires agent warmboot restart
+  // Requires an agent warmboot restart (forwarding state retained).
+  AGENT_WARMBOOT = 1,
  AGENT_COLDBOOT = 2, // Requires agent coldboot restart (clears ASIC state)
-  BGP_RESTART = 3, // Requires a restart of the bgpd (BGP++) service
+  // Plain service restart, for daemons with no warmboot or hitless-reload
+  // support (bgpd). Mechanically identical to a warmboot restart (plain
+  // systemctl restart-and-wait, no coldboot marker). Levels are only ever
+  // compared within one service, so 3 > 2 does NOT mean "more impactful
+  // than a coldboot".
+  SERVICE_RESTART = 3,


@hillol-nexthop This naming is a bit confusing and not very explicit especially AGENT_WARMBOOT and SERVICE_RESTART are basically the same thing?
Now with BGP config support, or potential future Qsfp config support, I think we need to revisit the meaning of these enums.
Correct me if I'm wrong, right now, you need ServiceType and ConfigActionLevel to decide what's the highest priority you need when handing session commit.
Can we consider to rename ConfigActionLevel to be:

enum ConfigActionLevel { HITLESS = 0, // Can be applied with reloadConfig() if the service supports GRACEFUL_SERVICE_RESTART = 1, // Restart service gracefully (i.e agent warmboot or regular bgp restart) DISRUPTIVE_SERVICE_RESTART = 2, // Restart service disruptively (i.e agent coldboot) }

My main goal is to make this action level generic without worrying if someone do `session.commit(AGENT, SERVICE_RESTART);

@joseph5wu My initial implementation didn't even consider SERVICE_RESTART and I took the AGENT_WARMBOOT path for BGP which technically works. But I agree with Indu's comment that BGP doesn't support warmboot. So the code gives a false indication. GRACEFUL_SERVICE_RESTART is nothing but warmboot only and I don't think BGP implementation qualifies for this. Coldboot or DISRUPTIVE_SERVICE_RESTART involves marker file which again BGP doesn't use. So, BGP doesn't truly fit into any of these buckets.
My mental construct is:
Agent- {Warmboot, Coldboot}
BGP - {Restart}
Also with the new implementation the warmboot would be doing systemcl stop and start while with BGP just systemctl restart is fine.
So one way is to keep ConfigSession as the base class and implement derived classes for agents, BGP, qsfp And these restart specifics can be kept in the derived class implementation.

After discussed offline, we decided to go with these three enums:

hitless

service_restart

disruptive_service_restart

The goal is to keep the action level enum to be generic without being too specific for certain service's behavior, while in the code, we can base on SERVICE + ACTION_LEVEL to decide what kinda service restart needed in certain config.

Updated the code

facebook-github-tools · 2026-09-04T05:49:34Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

induvsuresh · 2026-09-04T12:57:09Z

@hillol-nexthop Looks like number of builds are failing in the PR - Build & Test fboss2 CLI, QSFP etc
is this intended? when I import the PR also I have CI failures

facebook-github-tools · 2026-09-04T15:47:22Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

hillol-nexthop · 2026-09-04T15:50:05Z

@hillol-nexthop Looks like number of builds are failing in the PR - Build & Test fboss2 CLI, QSFP etc is this intended? when I import the PR also I have CI failures

The failures are in getdeps stage where ftpmirror.gnu.org returned HTTP 502. I have rebased the PR and the CI is rerunning. Hopefully all turns green.

Follow-up cleanups to the BGP-aware config session infra (meta#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.

…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).

bgpd has no warmboot, so recording AGENT_WARMBOOT for BGP config changes was misleading in the enum, the session metadata, and the commit output ("bgpd (warmboot)"). Add a SERVICE_RESTART level used for BGP; it short-circuits to the same plain restart-and-wait mechanism as a warmboot restart (only AGENT_COLDBOOT writes the coldboot marker). Commit output now prints bgpd without a boot-type suffix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review feedback: assert the no-op commit in scenario 2 returns an empty commitSha, and for symmetry assert the real commits return a non-empty one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review feedback: AGENT_WARMBOOT and the bgpd-only SERVICE_RESTART were the same thing, and the level names encoded one service's boot types. Rename the levels so they describe impact only: HITLESS (unchanged) AGENT_WARMBOOT -> SERVICE_RESTART AGENT_COLDBOOT -> DISRUPTIVE_SERVICE_RESTART and drop the interim SERVICE_RESTART = 3. What a level means for a given service is decided from the (service, level) pair in FbossServiceUtil::restartTypeName(): for the agent SERVICE_RESTART is a warmboot and DISRUPTIVE_SERVICE_RESTART a coldboot (marker files); bgpd has neither, so both are a plain restart. `config session commit` labels each restarted service accordingly ("wedge_agent (warmboot)", "bgpd (restart)").

facebook-github-tools · 2026-09-07T05:41:49Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

Empty commit; previous run failed on a GNU mirror outage downloading binutils. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

facebook-github-tools · 2026-09-09T17:37:41Z

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

meta-codesync · 2026-09-11T02:02:27Z

@joseph5wu merged this pull request in dde8684.

hillol-nexthop requested a review from a team as a code owner July 31, 2026 16:10

meta-cla Bot added the CLA Signed label Jul 31, 2026

github-actions Bot added the nexthop label Jul 31, 2026

hillol-nexthop force-pushed the bgp-session-unify branch from 52a0a62 to 0a68317 Compare July 31, 2026 16:14

hillol-nexthop force-pushed the bgp-session-unify branch from 0a68317 to d9cff93 Compare August 3, 2026 03:14

This was referenced Aug 6, 2026

[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy community-list commands #1475

Closed

[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy prefix-list commands #1477

Closed

hillol-nexthop force-pushed the bgp-session-unify branch from d9cff93 to 34f671e Compare August 10, 2026 04:44

induvsuresh reviewed Aug 17, 2026

View reviewed changes

Comment thread fboss/cli/fboss2/session/ConfigSession.cpp Resolved

induvsuresh requested changes Aug 17, 2026

View reviewed changes

Comment thread fboss/cli/fboss2/cli_metadata.thrift Resolved

Comment thread fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp Resolved

Comment thread fboss/cli/fboss2/session/ConfigSession.h Resolved

Comment thread fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp Resolved

hillol-nexthop force-pushed the bgp-session-unify branch from 541dca2 to 5ca1149 Compare August 19, 2026 10:55

hillol-nexthop requested a review from induvsuresh August 31, 2026 04:47

hillol-nexthop force-pushed the bgp-session-unify branch from 5ca1149 to 7b9c9de Compare September 2, 2026 05:43

induvsuresh approved these changes Sep 2, 2026

View reviewed changes

joseph5wu reviewed Sep 2, 2026

View reviewed changes

hillol-nexthop force-pushed the bgp-session-unify branch from cf84763 to f78e197 Compare September 4, 2026 15:47

hillol-nexthop and others added 5 commits September 7, 2026 04:58

Assert commitSha in threeWayMergeScenarios commits

238782b

Review feedback: assert the no-op commit in scenario 2 returns an empty commitSha, and for symmetry assert the real commits return a non-empty one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

hillol-nexthop force-pushed the bgp-session-unify branch from f78e197 to f1896f7 Compare September 7, 2026 05:41

Retrigger CI

b1f5c6b

Empty commit; previous run failed on a GNU mirror outage downloading binutils. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

meta-codesync Bot closed this in dde8684 Sep 11, 2026

meta-codesync Bot added the Merged label Sep 11, 2026

Conversation

hillol-nexthop commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

facebook-github-tools Bot commented Jul 31, 2026

Uh oh!

meta-codesync Bot commented Jul 31, 2026

Uh oh!

facebook-github-tools Bot commented Aug 3, 2026

Uh oh!

facebook-github-tools Bot commented Aug 10, 2026

Uh oh!

facebook-github-tools Bot commented Aug 11, 2026

Uh oh!

Uh oh!

induvsuresh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-tools Bot commented Aug 19, 2026

Uh oh!

facebook-github-tools Bot commented Aug 19, 2026

Uh oh!

facebook-github-tools Bot commented Aug 19, 2026

Uh oh!

facebook-github-tools Bot commented Sep 2, 2026

Uh oh!

joseph5wu Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

hillol-nexthop Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

joseph5wu Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

hillol-nexthop Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

facebook-github-tools Bot commented Sep 4, 2026

Uh oh!

induvsuresh commented Sep 4, 2026

Uh oh!

facebook-github-tools Bot commented Sep 4, 2026

Uh oh!

hillol-nexthop commented Sep 4, 2026

Uh oh!

facebook-github-tools Bot commented Sep 7, 2026

Uh oh!

facebook-github-tools Bot commented Sep 9, 2026

Uh oh!

meta-codesync Bot commented Sep 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hillol-nexthop commented Jul 31, 2026 •

edited

Loading