Skip to content

Commit d2af7de

Browse files
Verify committed global config via bgpd getRunningConfig RPC
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).
1 parent d1a6519 commit d2af7de

3 files changed

Lines changed: 53 additions & 8 deletions

File tree

fboss/cli/fboss2/test/integration_test/BUCK

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ oncall("fboss_oss")
1313
#
1414
# Unlike the Python-based CLI tests, these C++ tests directly invoke the CLI
1515
# library code rather than spawning a subprocess.
16+
# bazelify: extra_dep = //neteng/fboss/bgp/if:bgp_thrift
1617
cpp_binary(
1718
name = "fboss2_integration_test",
1819
srcs = [

fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,12 @@
55
*
66
* Scope: the BGP *global* tunables only. Each positive test stages the change
77
* AND commits it, then asserts the value landed at the correct thrift field
8-
* path in the promoted system config (/etc/coop/bgpcpp/bgpcpp.conf) that the
9-
* bgpd daemon consumes. Session-lifecycle behavior (clear / diff / rollback /
10-
* commit-restart mechanics) lives in ConfigBgpSessionTest.
8+
* path in bgpd's own running config — fetched over its getRunningConfig RPC
9+
* (see ConfigBgpTestBase::setAndCommit) — which proves the daemon parsed and
10+
* adopted the promoted /etc/coop/bgpcpp/bgpcpp.conf after the commit-triggered
11+
* restart, not merely that the CLI wrote the file. Session-lifecycle behavior
12+
* (clear / diff / rollback / commit-restart mechanics) lives in
13+
* ConfigBgpSessionTest.
1114
*
1215
* - count-confeds-in-as-path-len <true|false>
1316
* -> BgpConfig.count_confeds_in_as_path_len

fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
#include <thread>
2929

3030
#include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h"
31+
#include "fboss/cli/fboss2/utils/CmdClientUtilsCommon.h"
32+
#include "neteng/fboss/bgp/if/gen-cpp2/TBgpService.h"
3133

3234
namespace facebook::fboss {
3335

@@ -78,6 +80,36 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest {
7880
return folly::parseJson(content);
7981
}
8082

83+
// The running config as reported by the bgpd daemon itself, over its
84+
// TBgpService::getRunningConfig thrift RPC. Unlike readSystemBgpConfig()
85+
// (the promoted file on disk, which only proves what the CLI wrote), this
86+
// proves the daemon parsed and adopted the config after the post-commit
87+
// restart. Mirrors Fboss2IntegrationTest::getRunningConfig() for the agent.
88+
//
89+
// Retries on connection errors: systemd reports bgpd "active" as soon as
90+
// the process starts, but the thrift server only binds its port a few
91+
// seconds later — an RPC issued right after a commit-triggered restart
92+
// would otherwise race that window and get ECONNREFUSED.
93+
folly::dynamic readRunningBgpConfigViaRpc(
94+
std::chrono::seconds timeout = std::chrono::seconds(30)) const {
95+
auto deadline = std::chrono::steady_clock::now() + timeout;
96+
while (true) {
97+
try {
98+
HostInfo hostInfo("localhost");
99+
auto client = utils::createClient<apache::thrift::Client<
100+
facebook::neteng::fboss::bgp::thrift::TBgpService>>(hostInfo);
101+
std::string configStr;
102+
client->sync_getRunningConfig(configStr);
103+
return folly::parseJson(configStr);
104+
} catch (const std::exception&) {
105+
if (std::chrono::steady_clock::now() >= deadline) {
106+
throw;
107+
}
108+
std::this_thread::sleep_for(std::chrono::seconds(1));
109+
}
110+
}
111+
}
112+
81113
// Root of the git repo that versions both the agent config (cli/agent.conf)
82114
// and the BGP config (bgpcpp/bgpcpp.conf).
83115
std::string coopRepoDir() const {
@@ -162,10 +194,19 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest {
162194
}
163195

164196
// Stage a global change, commit it, wait for bgpd to come back, and return
165-
// the promoted system config (the authoritative post-commit location).
166-
// A no-op commit (staged value already in the system config) yields no SHA
167-
// and is fine: the caller's assertions on the returned config still verify
168-
// the value; commit/restart mechanics are covered by ConfigBgpSessionTest.
197+
// bgpd's own running config as reported over its getRunningConfig RPC.
198+
//
199+
// The returned config is the daemon's adopted view, which is strictly
200+
// stronger than the promoted file on disk (readSystemBgpConfig): a value
201+
// present here proves the commit promoted /etc/coop/bgpcpp/bgpcpp.conf AND
202+
// that bgpd parsed and adopted it after the commit-triggered restart, not
203+
// merely that the CLI wrote the file. The running config shares the same
204+
// BgpConfig JSON schema as the promoted file, so callers assert the same
205+
// field paths against it.
206+
//
207+
// A no-op commit (staged value already current) yields no SHA and is fine:
208+
// the caller's assertions on the returned config still verify the value;
209+
// commit/restart mechanics are covered by ConfigBgpSessionTest.
169210
folly::dynamic setAndCommit(const std::string& attr, const std::string& value)
170211
const {
171212
discardSession();
@@ -174,7 +215,7 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest {
174215
EXPECT_TRUE(waitForBgpDaemonActive())
175216
<< "bgpd did not return active after commit; state="
176217
<< bgpDaemonActiveState();
177-
return readSystemBgpConfig();
218+
return readRunningBgpConfigViaRpc();
178219
}
179220

180221
// Trailing-whitespace-trimmed stdout of a `systemctl` query for bgpd.

0 commit comments

Comments
 (0)