Skip to content

[Cisco][fboss2] [FBOSSCSC-36] Support BGP version in OSS builds - #1665

Open
selekkala wants to merge 2 commits into
meta:mainfrom
selekkala:codex/fix-oss-bgp-version
Open

selekkala wants to merge 2 commits into
meta:mainfrom
selekkala:codex/fix-oss-bgp-version

Conversation

@selekkala

@selekkala selekkala commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Pre-submission checklist

  • I've run the linters locally and fixed lint errors related to the files modified in this PR.
  • pre-commit run --files for every modified file.

Summary

fboss2 show bgp version currently throws unconditionally in OSS builds because the Meta-internal getRegexExportedValues extension is unavailable.

This change adds an additive TBgpService.getBuildInfo RPC to the shared BGP Thrift definition and uses it only in the OSS CLI path. Internal builds retain the existing fb303 path. If the CLI reaches a pre-API daemon, it converts Thrift's UNKNOWN_METHOD response into an actionable upgrade-and-restart message.

The BGP daemon-side handler lives in the separate facebook/BGP repository and must be deployed together with this client change. The compatibility error makes mixed-version rollout explicit rather than failing with the previous unsupported-command exception.

Test Plan

  • pre-commit run --files cmake/CliFboss2Test.cmake fboss/cli/fboss2/commands/show/bgp/CmdShowVersionBgp.cpp fboss/cli/fboss2/test/CmdShowBgpVersionTest.cpp fboss/cli/fboss2/test/bgp/MockBgpClient.h neteng/fboss/bgp/if/bgp_thrift.thrift — passed.
  • Added CmdShowBgpVersionTestFixture.QueryClientReturnsRunningBuildInfo to verify the CLI returns daemon-provided metadata.
  • Added CmdShowBgpVersionTestFixture.QueryClientRejectsOldDaemon to verify the mixed-version compatibility error.
  • Runtime validation on two OSS switches with a daemon implementing the additive RPC.
Live OSS router validation logs (two switches)

Switch A:

$ systemctl is-active fboss_hw_agent@0 fboss_sw_agent openr bgpd
active
active
active
active

$ fboss2 show bgp version
Package Name: bgp
Package Info: bgp-cd8f26fd
Package Version: cd8f26fd
Build Details:
        Platform: Linux
        Revision: cd8f26fd

$ fboss2 show bgp summary
BGP is up for 9m 52s
Router ID - 9.9.9.9, Local ASN - 65001
Peers: UP - 2, TOTAL - 2
Paths: Received - 3, Accepted - 3, Sent - 5
Loc-RIB Prefix Count: 5

Switch B:

$ systemctl is-active fboss_hw_agent@0 fboss_sw_agent openr bgpd
active
active
active
active

$ fboss2 show bgp version
Package Name: bgp
Package Info: bgp-cd8f26fd
Package Version: cd8f26fd
Build Details:
        Platform: Linux
        Revision: cd8f26fd

$ fboss2 show bgp summary
BGP is up for 9m 48s
Router ID - 8.8.8.8, Local ASN - 65001
Peers: UP - 2, TOTAL - 2
Paths: Received - 3, Accepted - 3, Sent - 5
Loc-RIB Prefix Count: 5

Both switches had the BGP Thrift listener active and no BGP warnings or new coredumps during the validation window.

The focused C++ tests are included for CI execution; the production image used for runtime validation does not package test binaries.

@selekkala
selekkala requested review from a team as code owners September 21, 2026 21:21
@meta-cla meta-cla Bot added the CLA Signed label Sep 21, 2026
@github-actions github-actions Bot added the cisco label Sep 21, 2026
@selekkala selekkala changed the title [Cisco][fboss2] Support BGP version in OSS builds [Cisco][fboss2] [FBOSSCSC-36] Support BGP version in OSS builds Sep 21, 2026
@srikrishnagopu

Copy link
Copy Markdown
Contributor

@induvsuresh @AarjunC please review.

@meta-codesync

meta-codesync Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

@AarjunC has imported this pull request. If you are a Meta employee, you can view this in D122188731.

@AarjunC

AarjunC commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Hi @selekkala, can you please take a look at failing unit test?

[ RUN      ] CmdShowBgpVersionTestFixture.QueryClientRejectsOldDaemon
I0928 12:24:29.403010     1 ThriftServiceUtils.cpp:71] Set preferred event base backend to Epoll
I0928 12:24:29.403329  2119 ThriftServer.cpp:2053] Using randomly generated TLS ticket keys.
I0928 12:24:29.403465  2119 ThriftServer.cpp:973] Using resource pools on address/port [::1]:0: thrift flag: true, enable gflag: false, disable gflag: false, runtime actions: 
I0928 12:24:29.403572  2119 ThriftServer.cpp:1533] Resource pools configured: 2
I0928 12:24:29.404808  2119 ThriftServer.cpp:731] ThriftServer listening on address/port: [::1]:37831
/var/FBOSS/fboss/fboss/cli/fboss2/test/CmdShowBgpVersionTest.cpp:47: Failure
Expected: CmdShowVersionBgp().queryClient(localhost()) throws an exception of type std::runtime_error.
  Actual: it throws apache::thrift::TApplicationException with description "Unknown function getBuildInfo".

[  FAILED  ] CmdShowBgpVersionTestFixture.QueryClientRejectsOldDaemon (5 ms)

@facebook-github-tools

Copy link
Copy Markdown

@selekkala has updated the pull request. You must reimport the pull request before landing.

@selekkala

Copy link
Copy Markdown
Contributor Author

Hi @AarjunC, thanks for flagging this. The mock was not correctly reproducing the UNKNOWN_METHOD response from an older BGP daemon. I updated the test to simulate that behavior accurately, and both BGP version tests now pass. The fix is in commit 8661e8a615. Please take another look.

@AarjunC

AarjunC commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

@selekkala, I don't see getBuildInfo() implementation in https://github.com/facebook/BGP/blob/main/neteng/fboss/bgp/cpp/BgpServiceBase.h .

Is there different PR open for the same?

Cc: @induvsuresh

@selekkala

selekkala commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor Author

Hi @AarjunC, you’re right—the daemon-side implementation was missing from facebook/BGP.

I opened the companion PR facebook/BGP#14. It adds and implements TBgpService.getBuildInfo() in BgpServiceBase, exposes the required build metadata, and includes unit coverage.

The two PRs can be merged independently because the RPC is additive:

For the complete show version bgp functionality, both changes must be deployed together. The buildimage integration should then update the BGP pin and remove the temporary carried BGP patch.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants