From f7d3c48281c366b5a0416b012b85889f02891f46 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 24 Jul 2026 16:02:54 +0000 Subject: [PATCH 1/4] Unify agent/BGP config session + decouple ConfigSession header Follow-up cleanups to the BGP-aware config session infra (#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. --- fboss/cli/fboss2/cli_metadata.thrift | 6 +- .../config/session/CmdConfigSessionClear.cpp | 62 +- .../config/session/CmdConfigSessionCommit.cpp | 5 - .../config/session/CmdConfigSessionDiff.cpp | 81 +- fboss/cli/fboss2/session/ConfigSession.cpp | 758 +++++++++--------- fboss/cli/fboss2/session/ConfigSession.h | 119 ++- fboss/cli/fboss2/session/FbossServiceUtil.cpp | 5 +- .../test/config/CmdConfigSessionTest.cpp | 118 ++- .../ConfigInterfaceDescriptionTest.cpp | 49 ++ 9 files changed, 676 insertions(+), 527 deletions(-) diff --git a/fboss/cli/fboss2/cli_metadata.thrift b/fboss/cli/fboss2/cli_metadata.thrift index f933911900a53..de1071db3a120 100644 --- a/fboss/cli/fboss2/cli_metadata.thrift +++ b/fboss/cli/fboss2/cli_metadata.thrift @@ -17,9 +17,11 @@ namespace cpp2 facebook.fboss.cli // changes. enum ConfigActionLevel { HITLESS = 0, // Can be applied with reloadConfig() - default - AGENT_WARMBOOT = 1, // Requires agent warmboot restart + // Requires a service restart that preserves state where possible. For the + // agent this is a warmboot (forwarding state retained); for bgpd (BGP++), + // which has no hitless reload, it is a plain service restart. + AGENT_WARMBOOT = 1, AGENT_COLDBOOT = 2, // Requires agent coldboot restart (clears ASIC state) - BGP_RESTART = 3, // Requires a restart of the bgpd (BGP++) service } // Identifier for different services that can be configured diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionClear.cpp b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionClear.cpp index 6f130f1e79bd9..bd78275f0d69a 100644 --- a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionClear.cpp +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionClear.cpp @@ -24,61 +24,27 @@ namespace facebook::fboss { CmdConfigSessionClearTraits::RetType CmdConfigSessionClear::queryClient( const HostInfo& /* hostInfo */) { - // Use static path getters to check for session files without calling - // getInstance(), which would create a session if one doesn't exist - std::string sessionConfigPath = ConfigSession::getSessionConfigPathStatic(); - std::string metadataPath = ConfigSession::getSessionMetadataPathStatic(); - std::string bgpConfigPath = ConfigSession::getBgpSessionConfigPathStatic(); - - std::error_code ec; - bool removedConfig = false; - bool removedMetadata = false; - bool removedBgpConfig = false; - - // Remove session config file (~/.fboss2/agent.conf) - if (fs::exists(sessionConfigPath)) { - fs::remove(sessionConfigPath, ec); - if (ec) { - throw std::runtime_error( - fmt::format( - "Failed to remove session config file {}: {}", - sessionConfigPath, - ec.message())); + // Remove each staged session file (agent + BGP configs and the metadata). + // stagedSessionFilePaths() is the single source of truth, so this handles + // every config domain uniformly -- including a BGP-only session -- without + // calling getInstance() (which would create a session we are trying to + // clear). Only individual files are removed; the ~/.fboss2 directory stays. + bool removedAny = false; + for (const auto& path : ConfigSession::stagedSessionFilePaths()) { + if (!fs::exists(path)) { + continue; } - removedConfig = true; - } - - // Remove metadata file (~/.fboss2/cli_metadata.json) - if (fs::exists(metadataPath)) { - ec.clear(); - fs::remove(metadataPath, ec); - if (ec) { - throw std::runtime_error( - fmt::format( - "Failed to remove metadata file {}: {}", - metadataPath, - ec.message())); - } - removedMetadata = true; - } - - // Remove staged BGP config file (~/.fboss2/bgp_config.json). BGP global edits - // are staged here (alongside any peer edits from BgpConfigSession), so a - // BGP-only session must be cleared too. - if (fs::exists(bgpConfigPath)) { - ec.clear(); - fs::remove(bgpConfigPath, ec); + std::error_code ec; + fs::remove(path, ec); if (ec) { throw std::runtime_error( fmt::format( - "Failed to remove BGP session config file {}: {}", - bgpConfigPath, - ec.message())); + "Failed to remove session file {}: {}", path, ec.message())); } - removedBgpConfig = true; + removedAny = true; } - if (removedConfig || removedMetadata || removedBgpConfig) { + if (removedAny) { return "Config session cleared successfully."; } return "No config session exists. Nothing to clear."; diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp index 178be1c072dc6..18ffbed977b6e 100644 --- a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp @@ -59,11 +59,6 @@ CmdConfigSessionCommitTraits::RetType CmdConfigSessionCommit::queryClient( fmt::format("{} (warmboot)", serviceName)); } break; - case cli::ConfigActionLevel::BGP_RESTART: - for (const auto& serviceName : serviceNamesList) { - restartedServices.push_back(fmt::format("{} (restart)", serviceName)); - } - break; case cli::ConfigActionLevel::HITLESS: for (const auto& serviceName : serviceNamesList) { reloadedServices.push_back(serviceName); diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp index 784af028a8ae0..7659277d3eb72 100644 --- a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp @@ -34,37 +34,7 @@ namespace facebook::fboss { namespace { -// Git-relative paths of the two config files tracked in the /etc/coop repo. -constexpr auto kAgentGitRelPath = "cli/agent.conf"; -constexpr auto kBgpGitRelPath = "bgpcpp/bgpcpp.conf"; - -// A diffable config domain. The agent config and the BGP config are tracked in -// the same /etc/coop git repo but live in different files; `config session -// diff` shows whichever domain(s) are staged/relevant. -struct DiffDomain { - std::string name; // "Agent" / "BGP" (section header when >1 domain shown) - std::string gitRelPath; // path in the git repo (e.g. cli/agent.conf) - std::string systemPath; // current live file - std::string sessionPath; // staged session file (~/.fboss2/...) - bool staged; // a session edit is staged for this domain -}; - -std::vector allDomains(ConfigSession& session) { - return { - DiffDomain{ - "Agent", - kAgentGitRelPath, - session.getSystemConfigPath(), - session.getSessionConfigPath(), - session.sessionExists()}, - DiffDomain{ - "BGP", - kBgpGitRelPath, - session.getBgpSystemConfigPath(), - session.getBgpSessionConfigPath(), - session.bgpSessionExists()}, - }; -} +using ConfigDomain = ConfigSession::ConfigDomain; // Read a file, returning empty content (not an error) when it doesn't exist. std::string readFileOrEmpty(const std::string& path) { @@ -76,20 +46,23 @@ std::string readFileOrEmpty(const std::string& path) { // Get config content from a revision specifier for a specific domain file. // "current" reads the live system file. A path absent at the given revision // (e.g. a commit predating BGP config) is treated as empty content. +// validationPath is a file present in every commit (the agent config), used to +// distinguish a genuinely invalid revision from a domain simply absent there. std::pair getRevisionContent( const std::string& revision, - const DiffDomain& domain, + const ConfigDomain& domain, + const std::string& validationPath, Git& git) { if (revision == "current") { return {readFileOrEmpty(domain.systemPath), "current live config"}; } std::string resolvedSha = git.resolveRef(revision); // Verify the revision is real before treating a missing domain path as empty. - // cli/agent.conf is present in every commit (including the initial one), so a - // genuinely invalid revision throws here and propagates; only a path absent + // The agent config is present in every commit (including the initial one), so + // a genuinely invalid revision throws here and propagates; only a path absent // at an otherwise-valid revision (e.g. bgpcpp.conf before BGP existed) is // treated as empty. - git.fileAtRevision(resolvedSha, kAgentGitRelPath); + git.fileAtRevision(resolvedSha, validationPath); std::string content; try { content = git.fileAtRevision(resolvedSha, domain.gitRelPath); @@ -184,35 +157,35 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient( const utils::RevisionList& revisions) { auto& session = ConfigSession::getInstance(); auto& git = session.getGit(); - auto domains = allDomains(session); + auto domains = session.configDomains(); + + // A git path present in every commit (the agent config), used to validate a + // revision in getRevisionContent(). configDomains() lists the agent first. + std::string validationPath = domains.front().gitRelPath; // Modes 1 and 2 both diff each staged domain's session file against some // "base" (current live config for mode 1; a revision for mode 2). The only // difference is how the base content+label is obtained, so share the loop. auto diffStagedDomains = [&](const std::function( - const DiffDomain&)>& getBase) { - int stagedCount = 0; + const ConfigDomain&)>& getBase) { + // Read each domain's staged content once via the shared primitive + // (nullopt == not staged), so we neither re-stat nor re-read files. + std::vector> staged; for (const auto& d : domains) { - stagedCount += d.staged ? 1 : 0; + if (auto content = session.readStagedContent(d)) { + staged.emplace_back(d, std::move(*content)); + } } std::string out; - for (const auto& d : domains) { - if (!d.staged) { - continue; - } + for (const auto& [d, sessionContent] : staged) { auto [baseContent, baseLabel] = getBase(d); - std::string sessionContent; - if (!folly::readFile(d.sessionPath.c_str(), sessionContent)) { - throw std::runtime_error( - "Failed to read session config from " + d.sessionPath); - } appendSection( out, d.name, executeDiff( baseContent, sessionContent, baseLabel, "session config"), - stagedCount > 1); + staged.size() > 1); } return out; }; @@ -222,7 +195,7 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient( if (!session.hasActiveSession()) { return "No config session exists. Make a config change first."; } - return diffStagedDomains([&](const DiffDomain& d) { + return diffStagedDomains([&](const ConfigDomain& d) { return std::make_pair( readFileOrEmpty(d.systemPath), std::string("current live config")); }); @@ -233,8 +206,8 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient( if (!session.hasActiveSession()) { return "No config session exists. Make a config change first."; } - return diffStagedDomains([&](const DiffDomain& d) { - return getRevisionContent(revisions[0], d, git); + return diffStagedDomains([&](const ConfigDomain& d) { + return getRevisionContent(revisions[0], d, validationPath, git); }); } @@ -245,8 +218,8 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient( // when more than one domain is shown. std::vector> sections; // {name, body} for (const auto& d : domains) { - auto [c1, l1] = getRevisionContent(revisions[0], d, git); - auto [c2, l2] = getRevisionContent(revisions[1], d, git); + auto [c1, l1] = getRevisionContent(revisions[0], d, validationPath, git); + auto [c2, l2] = getRevisionContent(revisions[1], d, validationPath, git); if (c1.empty() && c2.empty()) { continue; // domain absent at both revisions } diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index aea3bcfc941dc..219f7deeadb28 100644 --- a/fboss/cli/fboss2/session/ConfigSession.cpp +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -321,6 +322,10 @@ ConfigSession::ConfigSession( // and tests don't need git initialization or config file copying } +// Out-of-line so the unique_ptr members' (forward-declared) types are complete +// here where they are destroyed. +ConfigSession::~ConfigSession() = default; + namespace { std::unique_ptr& getInstancePtr() { static std::unique_ptr instance; @@ -361,6 +366,17 @@ std::string ConfigSession::getBgpSessionConfigPathStatic() { return getSessionDir() + "/bgp_config.json"; } +std::vector ConfigSession::stagedSessionFilePaths() { + // Per-domain staged config files plus the session metadata. Keep this in sync + // with configDomains() (the sessionPath of each domain); a new domain adds + // one line here. + return { + getSessionConfigPathStatic(), // agent: ~/.fboss2/agent.conf + getBgpSessionConfigPathStatic(), // bgp: ~/.fboss2/bgp_config.json + getSessionMetadataPathStatic(), // shared: ~/.fboss2/cli_metadata.json + }; +} + std::string ConfigSession::fileAtRevisionOrEmpty( const std::string& revision, const std::string& gitRelPath) const { @@ -389,6 +405,149 @@ std::string ConfigSession::getCliConfigPath() const { return systemConfigDir_ + "/cli/agent.conf"; } +std::vector ConfigSession::configDomains() const { + return { + ConfigDomain{ + cli::ServiceType::AGENT, + "Agent", + getSessionConfigPath(), // ~/.fboss2/agent.conf + kAgentGitRelPath, // cli/agent.conf + getCliConfigPath(), // /etc/coop/cli/agent.conf (promoted) + getSystemConfigPath(), // /etc/coop/agent.conf (symlink, live read) + getSystemConfigPath(), // symlink IS the system path for the agent + kAgentGitRelPath, // symlink -> cli/agent.conf + cli::ConfigActionLevel::HITLESS, // rollback reloads the agent + }, + ConfigDomain{ + cli::ServiceType::BGP, + "BGP", + getBgpSessionConfigPath(), // ~/.fboss2/bgp_config.json + kBgpGitRelPath, // bgpcpp/bgpcpp.conf + getBgpSystemConfigPath(), // /etc/coop/bgpcpp/bgpcpp.conf (promoted) + getBgpSystemConfigPath(), // the promoted file is also the live read + getBgpSystemConfigLinkPath(), // /etc/coop/bgpcpp.conf (symlink) + kBgpGitRelPath, // symlink -> bgpcpp/bgpcpp.conf + cli::ConfigActionLevel::AGENT_WARMBOOT, // rollback restarts bgpd + }, + }; +} + +std::optional ConfigSession::readStagedContent( + const ConfigDomain& domain) const { + if (!fs::exists(domain.sessionPath)) { + return std::nullopt; + } + std::string content; + if (!folly::readFile(domain.sessionPath.c_str(), content)) { + throw std::runtime_error( + fmt::format( + "Failed to read session config from {}", domain.sessionPath)); + } + return content; +} + +std::string ConfigSession::readPromotedContent( + const ConfigDomain& domain) const { + std::string content; + if (fs::exists(domain.promotedPath)) { + if (!folly::readFile(domain.promotedPath.c_str(), content)) { + throw std::runtime_error( + fmt::format( + "Failed to read current config from {}", domain.promotedPath)); + } + } + return content; +} + +void ConfigSession::promoteDomain( + const ConfigDomain& domain, + const std::string& content, + std::vector& commitFiles) const { + ensureDirectoryExists(fs::path(domain.promotedPath).parent_path().string()); + folly::writeFileAtomic( + domain.promotedPath, content, 0644, folly::SyncType::WITH_SYNC); + commitFiles.push_back(domain.promotedPath); + // Keep the daemon-facing path a symlink into the CLI-managed dir so the + // daemon needs no per-device --config override. The symlink is git-tracked + // alongside the config so a rollback restores it. + atomicSymlinkUpdate(domain.symlinkPath, domain.symlinkTarget); + commitFiles.push_back(domain.symlinkPath); +} + +void ConfigSession::restorePromotedDomain( + const ConfigDomain& domain, + const std::string& oldContent, + bool existed) const { + if (existed) { + folly::writeFileAtomic( + domain.promotedPath, oldContent, 0644, folly::SyncType::WITH_SYNC); + } else { + std::error_code rmEc; + fs::remove(domain.promotedPath, rmEc); + } +} + +void ConfigSession::clearStagedDomain(const ConfigDomain& domain) { + std::error_code ec; + fs::remove(domain.sessionPath, ec); + if (ec) { + LOG(WARNING) << fmt::format( + "Failed to remove session config {}: {}", + domain.sessionPath, + ec.message()); + } + // Drop the in-memory cache (null == not loaded) so the next access re-seeds + // from the promoted config. + switch (domain.service) { + case cli::ServiceType::AGENT: + agentConfig_.reset(); + break; + case cli::ServiceType::BGP: + bgpConfig_.reset(); + break; + } +} + +bool ConfigSession::domainContentEqual( + const ConfigDomain& domain, + const std::string& a, + const std::string& b) const { + // Empty (missing) content cannot be parsed as a struct; compare bytes. Both + // empty -> equal; empty vs non-empty -> changed. + if (a.empty() || b.empty()) { + return a == b; + } + try { + switch (domain.service) { + case cli::ServiceType::AGENT: { + cfg::AgentConfig sa, sb; + apache::thrift::SimpleJSONSerializer::deserialize( + a, sa); + apache::thrift::SimpleJSONSerializer::deserialize( + b, sb); + return sa == sb; + } + case cli::ServiceType::BGP: { + bgp::thrift::BgpConfig sa, sb; + apache::thrift::SimpleJSONSerializer::deserialize< + bgp::thrift::BgpConfig>(a, sa); + apache::thrift::SimpleJSONSerializer::deserialize< + bgp::thrift::BgpConfig>(b, sb); + return sa == sb; + } + } + } catch (const std::exception& ex) { + // Malformed JSON on either side: fall back to a byte comparison rather than + // crashing the commit/rollback. Differing bytes are then treated as a + // change (the safe, conservative outcome). + LOG(WARNING) << "Semantic config comparison for " << domain.name + << " failed to parse; falling back to byte comparison: " + << ex.what(); + return a == b; + } + return a == b; // unreachable: switch above is exhaustive +} + bool ConfigSession::sessionExists() const { return fs::exists(getSessionConfigPath()); } @@ -397,34 +556,35 @@ bool ConfigSession::hasActiveSession() const { // An agent config session (agent.conf) OR a protocol session staged outside // agent.conf. BGP is the latter: a staged ~/.fboss2/bgp_config.json (written // by either the typed global config here or BgpConfigSession's peer edits) - // with a recorded BGP_RESTART action, but never touching agent.conf. + // with a recorded restart (AGENT_WARMBOOT) action, but never touching + // agent.conf. return sessionExists() || bgpSessionExists(); } cfg::AgentConfig& ConfigSession::getAgentConfig() { - if (!configLoaded_) { + if (!agentConfig_) { loadConfig(); } - return agentConfig_; + return *agentConfig_; } const cfg::AgentConfig& ConfigSession::getAgentConfig() const { - if (!configLoaded_) { + if (!agentConfig_) { throw std::runtime_error( "Config not loaded yet. Call getAgentConfig() (non-const) first."); } - return agentConfig_; + return *agentConfig_; } utils::PortMap& ConfigSession::getPortMap() { - if (!configLoaded_) { + if (!agentConfig_) { loadConfig(); } return *portMap_; } const utils::PortMap& ConfigSession::getPortMap() const { - if (!configLoaded_) { + if (!agentConfig_) { throw std::runtime_error( "Config not loaded yet. Call getPortMap() (non-const) first."); } @@ -432,59 +592,57 @@ const utils::PortMap& ConfigSession::getPortMap() const { } void ConfigSession::rebuildPortMap() { - if (!configLoaded_) { + if (!agentConfig_) { loadConfig(); } - portMap_ = std::make_unique(agentConfig_); + portMap_ = std::make_unique(*agentConfig_); } void ConfigSession::saveConfig( cli::ServiceType service, cli::ConfigActionLevel actionLevel) { - if (!configLoaded_) { - throw std::runtime_error("No config loaded to save"); + // Serialize whichever typed config this service owns and stage it to that + // domain's session file. The round-trip through serialize -> parse -> + // toPrettyJson is needed because SimpleJSONSerializer emits Thrift maps with + // integer keys (e.g. clientIdToAdminDistance) as string keys; going through + // facebook::thrift::to_dynamic() directly would keep integer keys and make + // folly::toPrettyJson() fail (JSON object keys must be strings). + std::string prettyJson; + std::string sessionPath; + switch (service) { + case cli::ServiceType::AGENT: { + if (!agentConfig_) { + throw std::runtime_error("No config loaded to save"); + } + auto json = apache::thrift::SimpleJSONSerializer::serialize( + *agentConfig_); + prettyJson = folly::toPrettyJson(folly::parseJson(json)); + sessionPath = getSessionConfigPath(); + break; + } + case cli::ServiceType::BGP: { + if (!bgpConfig_) { + loadBgpConfig(); + } + auto json = apache::thrift::SimpleJSONSerializer::serialize( + *bgpConfig_); + prettyJson = folly::toPrettyJson(folly::parseJson(json)); + sessionPath = getBgpSessionConfigPath(); + break; + } } - // We need to do a round-trip through serialize -> parse -> toPrettyJson - // because SimpleJSONSerializer handles Thrift maps with integer keys - // (like clientIdToAdminDistance) by converting them to strings. - // If we use facebook::thrift::to_dynamic() directly, the integer keys - // are preserved as integers in the folly::dynamic object, which causes - // folly::toPrettyJson() to fail because JSON objects requires string keys. - std::string json = - apache::thrift::SimpleJSONSerializer::serialize( - agentConfig_); - std::string prettyJson = folly::toPrettyJson(folly::parseJson(json)); - // Use folly::writeFileAtomic with sync to avoid race conditions when multiple // threads/processes write to the same session file. WITH_SYNC ensures data // is flushed to disk before the atomic rename, preventing readers from // seeing partial/corrupted data. folly::writeFileAtomic( - getSessionConfigPath(), prettyJson, 0644, folly::SyncType::WITH_SYNC); - - // Automatically record the command from /proc/self/cmdline. - // This ensures all config commands are tracked without requiring manual - // instrumentation in each command implementation. - // Note: When running CLI commands directly (e.g., in tests), - // /proc/self/cmdline may not contain the CLI command, so we gracefully skip - // command tracking. - std::string rawCmd = readCommandLineFromProc(); - // Only record if this is a config command and not already the last one - // recorded as that'd be idempotent anyway. Strip any leading flags. - auto pos = rawCmd.find("config "); - if (pos != std::string::npos) { - std::string cmd = rawCmd.substr(pos); - if (commands_.empty() || commands_.back() != cmd) { - commands_.push_back(cmd); - } - } - - // Update the required action metadata for this service - updateRequiredAction(service, actionLevel); + sessionPath, prettyJson, 0644, folly::SyncType::WITH_SYNC); - // Save command history and action levels to metadata - saveMetadata(); + // Record the command from /proc/self/cmdline and bump this service's required + // action level + metadata. Shared with recordServiceAction() so command + // tracking and action bookkeeping are identical for every service. + recordServiceAction(service, actionLevel); } void ConfigSession::saveConfig() { @@ -533,7 +691,7 @@ bool ConfigSession::bgpSessionExists() const { } void ConfigSession::loadBgpConfig() { - if (bgpConfigLoaded_) { + if (bgpConfig_) { return; } @@ -556,52 +714,40 @@ void ConfigSession::loadBgpConfig() { } } - bgpConfig_ = bgp::thrift::BgpConfig(); + bgpConfig_ = std::make_unique(); if (!content.empty()) { try { apache::thrift::SimpleJSONSerializer::deserialize( - content, bgpConfig_); + content, *bgpConfig_); } catch (const std::exception& ex) { LOG(WARNING) << "Failed to parse BGP config, starting from defaults: " << ex.what(); - bgpConfig_ = bgp::thrift::BgpConfig(); + *bgpConfig_ = bgp::thrift::BgpConfig(); } } - bgpConfigLoaded_ = true; } bgp::thrift::BgpConfig& ConfigSession::getBgpConfig() { - if (!bgpConfigLoaded_) { + if (!bgpConfig_) { loadBgpConfig(); } - return bgpConfig_; + return *bgpConfig_; } const bgp::thrift::BgpConfig& ConfigSession::getBgpConfig() const { - if (!bgpConfigLoaded_) { + if (!bgpConfig_) { throw std::runtime_error( "BGP config not loaded yet. Call getBgpConfig() (non-const) first."); } - return bgpConfig_; + return *bgpConfig_; } void ConfigSession::saveBgpConfig() { - if (!bgpConfigLoaded_) { - loadBgpConfig(); - } - - // Serialize the entire typed config (round-tripped through parse so integer - // map keys become string keys, mirroring saveConfig() for the agent). - auto json = - apache::thrift::SimpleJSONSerializer::serialize(bgpConfig_); - std::string prettyJson = folly::toPrettyJson(folly::parseJson(json)); - folly::writeFileAtomic( - getBgpSessionConfigPath(), prettyJson, 0644, folly::SyncType::WITH_SYNC); - - // Record the command (mirrors saveConfig) and that bgpd must be restarted - // for this change to take effect on a subsequent `config session commit`. - recordServiceAction( - cli::ServiceType::BGP, cli::ConfigActionLevel::BGP_RESTART); + // Convenience wrapper over the generic saveConfig(), mirroring the no-arg + // saveConfig() for the agent. bgpd has no hitless reload, so a staged BGP + // change always requires a bgpd restart (AGENT_WARMBOOT) on the next + // `config session commit`. + saveConfig(cli::ServiceType::BGP, cli::ConfigActionLevel::AGENT_WARMBOOT); } Git& ConfigSession::getGit() { @@ -763,7 +909,6 @@ ConfigSession::applyServiceActions( switch (level) { case cli::ConfigActionLevel::AGENT_COLDBOOT: case cli::ConfigActionLevel::AGENT_WARMBOOT: - case cli::ConfigActionLevel::BGP_RESTART: serviceNames[service] = fbossServiceUtil_->restartService(service, level); break; @@ -790,16 +935,16 @@ void ConfigSession::loadConfig() { fmt::format("Failed to read config file: {}", sessionConfigPath)); } + agentConfig_ = std::make_unique(); apache::thrift::SimpleJSONSerializer::deserialize( - configJson, agentConfig_); + configJson, *agentConfig_); // Handle the legacy case where config might be a bare SwitchConfig - if (*agentConfig_.sw() == cfg::SwitchConfig()) { + if (*agentConfig_->sw() == cfg::SwitchConfig()) { apache::thrift::SimpleJSONSerializer::deserialize( - configJson, *agentConfig_.sw()); + configJson, *agentConfig_->sw()); } - portMap_ = std::make_unique(agentConfig_); - configLoaded_ = true; + portMap_ = std::make_unique(*agentConfig_); } void ConfigSession::initializeSession() { @@ -807,15 +952,15 @@ void ConfigSession::initializeSession() { // Resume an existing session if EITHER an agent (agent.conf) or a BGP // (bgp_config.json) session is staged. Keying only on the agent session file // would misdetect a BGP-only session as fresh and clear its recorded - // BGP_RESTART action on the next (separate-process) CLI invocation, - // silently dropping the staged change at commit time. + // restart (AGENT_WARMBOOT) action on the next (separate-process) CLI + // invocation, silently dropping the staged change at commit time. if (!hasActiveSession()) { // Starting a new session - reset all state to ensure we don't carry over // stale data from a previous session (e.g., if the singleton persisted // in memory but the session files were deleted). commands_.clear(); requiredActions_.clear(); - configLoaded_ = false; + agentConfig_.reset(); // Ensure the session config directory exists ensureDirectoryExists(sessionConfigDir_); @@ -924,9 +1069,6 @@ void ConfigSession::copySystemConfigToSession() const { } ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { - // A BGP-only session stages bgp_config.json but never agent.conf, so the - // agent-config file operations below are guarded on hasAgentSession. - const bool hasAgentSession = sessionExists(); if (!hasActiveSession()) { throw std::runtime_error( "No config session exists. Make a config change first."); @@ -951,205 +1093,106 @@ ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { Git::shortSha1(currentHead))); } - std::string cliConfigDir = getCliConfigDir(); - std::string cliConfigPath = getCliConfigPath(); - std::string sessionConfigPath = getSessionConfigPath(); - std::string systemConfigPath = getSystemConfigPath(); + ensureDirectoryExists(getCliConfigDir()); - ensureDirectoryExists(cliConfigDir); - - // Read the staged agent config (only present for an agent config session; a - // BGP-only session never writes agent.conf). oldConfigData is read for - // rollback if needed. - std::string sessionConfigData; - std::string oldConfigData; - if (hasAgentSession) { - if (!folly::readFile(sessionConfigPath.c_str(), sessionConfigData)) { - throw std::runtime_error( - fmt::format( - "Failed to read session config from {}", sessionConfigPath)); - } - if (fs::exists(cliConfigPath)) { - if (!folly::readFile(cliConfigPath.c_str(), oldConfigData)) { - throw std::runtime_error( - fmt::format("Failed to read CLI config from {}", cliConfigPath)); - } - } - } - - // Capture the running BGP system config up front: it tells us whether the - // staged BGP config actually changed (so we can skip a needless bgpd - // restart) and is the snapshot we restore if the commit fails partway. - const std::string bgpSystemPath = getBgpSystemConfigPath(); - const bool bgpSystemConfigExisted = fs::exists(bgpSystemPath); - std::string bgpOldData; - if (bgpSystemConfigExisted) { - // Check the read: a silently-failed read would leave bgpOldData empty and - // make the restore-on-failure path below write an empty bgpcpp.conf, - // corrupting the running config (mirrors the guard in rollback()). - if (!folly::readFile(bgpSystemPath.c_str(), bgpOldData)) { - throw std::runtime_error( - fmt::format( - "Failed to read current BGP config from {}", bgpSystemPath)); + // Per-domain staged/changed analysis, applied uniformly to agent and BGP. + // A domain is "staged" when a session edit exists; it is "pending" (needs + // promotion + a service action) only when the staged content differs from + // what is already promoted. This skip-when-unchanged rule is the same for + // both domains, so re-committing an unchanged config is a true no-op: no git + // revision, no symlink churn, and no reloadConfig()/bgpd restart. + struct Pending { + ConfigDomain domain; + std::string staged; + std::string oldPromoted; + bool promotedExisted; + }; + std::vector stagedDomains; + std::vector pending; + // actions ends up holding exactly the pending domains' required action levels + // (returned in CommitResult and passed to applyServiceActions). + auto actions = requiredActions_; + for (const auto& domain : configDomains()) { + auto staged = readStagedContent(domain); + if (!staged) { + actions.erase(domain.service); // nothing staged for this domain + continue; } - } - - // saveBgpConfig() records BGP_RESTART unconditionally, so re-committing an - // unchanged BGP config would otherwise still bounce bgpd (a disruptive, - // traffic-affecting restart for no effective change). Treat BGP as changed - // only when the staged config differs from what is running. - bool bgpConfigChanged = false; - if (requiredActions_.count(cli::ServiceType::BGP) > 0 && bgpSessionExists()) { - std::string stagedBgpData; - if (!folly::readFile(getBgpSessionConfigPath().c_str(), stagedBgpData)) { - throw std::runtime_error( - fmt::format( - "Failed to read staged BGP config from {}", - getBgpSessionConfigPath())); + stagedDomains.push_back(domain); + std::string oldPromoted = readPromotedContent(domain); + if (domainContentEqual(domain, *staged, oldPromoted)) { + actions.erase(domain.service); // unchanged -> no promote, no action + continue; } - bgpConfigChanged = (stagedBgpData != bgpOldData); + pending.push_back( + {domain, + std::move(*staged), + std::move(oldPromoted), + fs::exists(domain.promotedPath)}); } - // Copy requiredActions_ before we reset it (returned in CommitResult) and - // drop BGP when the BGP config is unchanged, so an unchanged BGP commit - // neither promotes the config nor restarts bgpd. - auto actions = requiredActions_; - if (!bgpConfigChanged) { - actions.erase(cli::ServiceType::BGP); - } - - // Early return if there are no changes to commit. - const bool agentConfigChanged = - hasAgentSession && sessionConfigData != oldConfigData; - if (!agentConfigChanged && actions.empty()) { + // Nothing that is staged actually changed -> no-op. + if (pending.empty()) { return CommitResult{"", {}, {}}; } - // Write the metadata file alongside the config revision. - // This is required for rollback functionality. - // Use folly::writeFileAtomic instead of fs::copy_file so that we only write - // file content without calling fchmod() on the destination — fchmod fails - // with EPERM when the target is owned by a different user (e.g. root) even - // if the caller has group-write permission on the file. + // Write the metadata file alongside the config revision (required for + // rollback). Use folly::writeFileAtomic rather than fs::copy_file so we only + // write content without fchmod()ing a differently-owned destination (EPERM). + // Nothing has been promoted yet, so a failure here simply aborts. std::string metadataPath = getMetadataPath(); - std::string targetMetadataPath = - fmt::format("{}/cli_metadata.json", cliConfigDir); + std::string targetMetadataPath = getSystemMetadataPath(); std::string metadataContent; if (!folly::readFile(metadataPath.c_str(), metadataContent)) { LOG(WARNING) << "Failed to read session metadata from " << metadataPath << "; committing empty metadata"; metadataContent = "{}"; } - try { - folly::writeFileAtomic( - targetMetadataPath, metadataContent, 0664, folly::SyncType::WITH_SYNC); - } catch (const std::exception& e) { - if (!oldConfigData.empty()) { - folly::writeFileAtomic( - cliConfigPath, oldConfigData, 0644, folly::SyncType::WITH_SYNC); - } - throw std::runtime_error( - fmt::format( - "Failed to copy metadata to {}: {}", targetMetadataPath, e.what())); - } + folly::writeFileAtomic( + targetMetadataPath, metadataContent, 0664, folly::SyncType::WITH_SYNC); - // Files to include in the git commit. The metadata file is always part of a - // commit; agent.conf and its symlink are added only when an agent config - // session was staged. BGP config (if staged this session) is added below; it - // lives under /etc/coop/bgpcpp/ so it's versioned by this same /etc/coop - // repo. std::vector commitFiles = {targetMetadataPath}; - - if (hasAgentSession) { - // Atomically write the session config to the CLI config path - folly::writeFileAtomic( - cliConfigPath, sessionConfigData, 0644, folly::SyncType::WITH_SYNC); - - // Ensure the system config symlink points to the CLI config - atomicSymlinkUpdate(systemConfigPath, "cli/agent.conf"); - - commitFiles.push_back(cliConfigPath); - commitFiles.push_back(systemConfigPath); - } - - // Apply the config based on the required action level std::string commitSha; std::map> serviceNames; - // stagingBgp reflects the trimmed action set: false when the BGP config was - // unchanged, so we neither promote bgpcpp.conf nor restart bgpd below. The - // prior BGP config (bgpOldData / bgpSystemConfigExisted, captured above) is - // the snapshot restored if the commit fails partway. - const bool stagingBgp = actions.count(cli::ServiceType::BGP) > 0; - std::string bgpConfPath; - try { - // If this session staged BGP config changes, promote the staged - // bgp_config.json to /etc/coop/bgpcpp/bgpcpp.conf BEFORE bgpd is - // restarted below, so the restart picks up the new config. The promoted - // file is committed as part of this commit's git operation. The staged - // session file is left in place until the whole commit succeeds, so a - // failure here can be rolled back and retried. - if (stagingBgp && bgpSessionExists()) { - std::string staged; - if (!folly::readFile(getBgpSessionConfigPath().c_str(), staged)) { - throw std::runtime_error( - fmt::format( - "Failed to read staged BGP config from {}", - getBgpSessionConfigPath())); + // Promote every changed domain (staged -> git-tracked file + daemon + // symlink) BEFORE applying its service action, so the reload/restart picks + // up the new config. Session files are left in place until the whole commit + // succeeds, so a failure here can be rolled back and retried. + for (const auto& p : pending) { + promoteDomain(p.domain, p.staged, commitFiles); + } + // Track every other domain's running config in this commit too, so a later + // rollback has a snapshot to restore instead of wiping it (e.g. a + // bgpcpp.conf present on disk but not yet committed). git dedups unchanged + // content, so re-adding an already-tracked file is a no-op. + std::set pendingServices; + for (const auto& p : pending) { + pendingServices.insert(p.domain.service); + } + for (const auto& domain : configDomains()) { + if (pendingServices.count(domain.service) == 0 && + fs::exists(domain.promotedPath)) { + commitFiles.push_back(domain.promotedPath); } - ensureDirectoryExists(getBgpSystemConfigDir()); - folly::writeFileAtomic( - getBgpSystemConfigPath(), staged, 0644, folly::SyncType::WITH_SYNC); - bgpConfPath = getBgpSystemConfigPath(); - commitFiles.push_back(bgpConfPath); - // Keep the daemon-facing path (/etc/coop/bgpcpp.conf) a symlink into the - // CLI-managed bgpcpp/ subdir, mirroring agent.conf -> cli/agent.conf. - // bgpd reads its provisioned --config /etc/coop/bgpcpp.conf and follows - // the symlink, so no per-device systemd --config override is needed. The - // symlink is git-tracked alongside the config so a rollback restores it. - atomicSymlinkUpdate(getBgpSystemConfigLinkPath(), kBgpGitRelPath); - commitFiles.push_back(getBgpSystemConfigLinkPath()); - } else if (bgpSystemConfigExisted) { - // Agent-only commit: still track the running bgpd config so a later - // rollback has a snapshot to restore instead of wiping it. No restart — - // the file is already the running config. - commitFiles.push_back(bgpSystemPath); } serviceNames = applyServiceActions(actions, hostInfo); - // Create a Git commit with all changed files: - // - cli/agent.conf (the config file) - // - cli/cli_metadata.json (the metadata file) - // - agent.conf (the symlink, in case it was updated) - // - bgpcpp/bgpcpp.conf (the bgp config, if staged this session) std::string commitMessage = fmt::format("Config commit by {}", username_); commitSha = git_->commit(commitFiles, commitMessage, username_, ""); LOG(INFO) << "Config committed as " << Git::shortSha1(commitSha); } catch (const std::exception& ex) { - // Rollback: restore the old config, then re-apply actions - // on the old config so services pick up the previous configuration + // Restore each promoted domain to its prior state, then re-apply actions on + // the old config so services pick up the previous configuration. Staged + // session files are left intact so the user can retry. try { - if (!oldConfigData.empty()) { - folly::writeFileAtomic( - cliConfigPath, oldConfigData, 0644, folly::SyncType::WITH_SYNC); - } - // Restore the BGP system config to its pre-commit state. The staged - // session file is left intact, so the user can retry after the failure - // is resolved. - if (stagingBgp && !bgpConfPath.empty()) { - if (bgpSystemConfigExisted) { - folly::writeFileAtomic( - bgpConfPath, bgpOldData, 0644, folly::SyncType::WITH_SYNC); - } else { - std::error_code rmEc; - fs::remove(bgpConfPath, rmEc); - } + for (const auto& p : pending) { + restorePromotedDomain(p.domain, p.oldPromoted, p.promotedExisted); } applyServiceActions(actions, hostInfo); } catch (const std::exception& rollbackEx) { - // If rollback also fails, include both errors in the message throw std::runtime_error( fmt::format( "Failed to apply config: {}. Additionally, failed to rollback the config: {}", @@ -1162,35 +1205,17 @@ ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { ex.what())); } - // Now that the commit has fully succeeded, clear the staged BGP session file - // (it was deliberately left in place for rollback). Force a re-seed from the - // newly promoted system config on next access. - if (stagingBgp) { - std::error_code rmEc; - fs::remove(getBgpSessionConfigPath(), rmEc); - bgpConfigLoaded_ = false; - } - - // Only remove the agent session config after everything succeeded. - if (hasAgentSession) { - std::error_code ec; - fs::remove(sessionConfigPath, ec); - if (ec) { - // Log warning but don't fail - the commit succeeded - LOG(WARNING) << fmt::format( - "Failed to remove session config {}: {}", - sessionConfigPath, - ec.message()); - } - } - - // Reset action level for all services after successful commit - for (const auto& [service, level] : actions) { - resetRequiredAction(service); + // The commit fully succeeded: the session is consumed, so clear every staged + // domain's session file and reset its recorded action level. + for (const auto& domain : stagedDomains) { + clearStagedDomain(domain); + resetRequiredAction(domain.service); } base_ = commitSha; - // Force config reload from system config on next access - configLoaded_ = false; + // Force a reload from the promoted config on next access (null == not + // loaded). + agentConfig_.reset(); + bgpConfig_.reset(); return CommitResult{commitSha, actions, serviceNames}; } @@ -1278,12 +1303,13 @@ void ConfigSession::rebase() { base_ = currentHead; saveMetadata(); - // Reload in-memory state for whichever domains were rebased. + // Reload in-memory state for whichever domains were rebased (null == reload + // on next access). if (agentMerged) { loadConfig(); } if (bgpMerged) { - bgpConfigLoaded_ = false; + bgpConfig_.reset(); } } @@ -1306,36 +1332,16 @@ std::string ConfigSession::rollback(const HostInfo& hostInfo) { std::string ConfigSession::rollback( const HostInfo& hostInfo, const std::string& commitSha) { - std::string cliConfigDir = getCliConfigDir(); - std::string cliConfigPath = getCliConfigPath(); - std::string systemConfigPath = getSystemConfigPath(); - - ensureDirectoryExists(cliConfigDir); + ensureDirectoryExists(getCliConfigDir()); // Resolve the commit SHA (in case it's a short SHA or ref) std::string resolvedSha = git_->resolveRef(commitSha); - // Get the config and metadata content from the target commit - // The paths in git are relative to the repo root - std::string targetConfigData = - git_->fileAtRevision(resolvedSha, "cli/agent.conf"); + // Read the target metadata; this is present in every commit, so it also + // validates the revision (a bad ref throws here and propagates). + std::string metadataPath = getSystemMetadataPath(); std::string targetMetadataData = git_->fileAtRevision(resolvedSha, "cli/cli_metadata.json"); - std::string metadataPath = fmt::format("{}/cli_metadata.json", cliConfigDir); - - // Target BGP config at that revision ("" if the commit predates BGP config). - std::string bgpSystemPath = getBgpSystemConfigPath(); - std::string targetBgpData = - fileAtRevisionOrEmpty(resolvedSha, kBgpGitRelPath); - - // Read the current config for rollback if needed - std::string oldConfigData; - if (fs::exists(cliConfigPath)) { - if (!folly::readFile(cliConfigPath.c_str(), oldConfigData)) { - throw std::runtime_error( - fmt::format("Failed to read current config from {}", cliConfigPath)); - } - } std::string oldMetadataData; if (fs::exists(metadataPath)) { if (!folly::readFile(metadataPath.c_str(), oldMetadataData)) { @@ -1343,51 +1349,51 @@ std::string ConfigSession::rollback( fmt::format("Failed to read current metadata from {}", metadataPath)); } } - std::string oldBgpData; - const bool bgpSystemExisted = fs::exists(bgpSystemPath); - if (bgpSystemExisted) { - if (!folly::readFile(bgpSystemPath.c_str(), oldBgpData)) { - // Don't proceed: a failed read here would make the restore-on-failure - // path below write an empty bgpcpp.conf, corrupting the running config. - throw std::runtime_error( - fmt::format( - "Failed to read current BGP config from {}", bgpSystemPath)); - } - } - - // Only act on a service whose config actually changes in this rollback. A - // BGP-only commit leaves cli/agent.conf identical, and vice versa. - const bool agentChanged = targetConfigData != oldConfigData; - const bool bgpChanged = targetBgpData != oldBgpData; - // Always restore the metadata (it records the new rollback base). Only - // rewrite the agent config + symlink when it actually changed, to avoid - // needless writes and symlink churn on a BGP-only rollback. + // Per-domain: target content at the revision vs the currently-promoted + // content. A rollback only acts on a domain whose config actually changes + // (a BGP-only commit leaves cli/agent.conf identical, and vice versa). + struct DomainRollback { + ConfigDomain domain; + std::string target; + std::string oldPromoted; + bool promotedExisted; + bool changed; + }; + std::vector doms; + for (const auto& domain : configDomains()) { + // fileAtRevisionOrEmpty: a path absent at the revision (e.g. bgpcpp.conf + // before BGP existed) is treated as empty content -> remove on rollback. + std::string target = fileAtRevisionOrEmpty(resolvedSha, domain.gitRelPath); + std::string oldPromoted = readPromotedContent(domain); + bool existed = fs::exists(domain.promotedPath); + bool changed = !domainContentEqual(domain, target, oldPromoted); + doms.push_back( + {domain, std::move(target), std::move(oldPromoted), existed, changed}); + } + + // Always restore the metadata (it records the new rollback base). Promote + // each changed domain to its target (or remove its file if the domain didn't + // exist at that revision), leaving unchanged domains untouched to avoid + // needless writes and symlink churn. folly::writeFileAtomic( metadataPath, targetMetadataData, 0644, folly::SyncType::WITH_SYNC); - if (agentChanged) { - folly::writeFileAtomic( - cliConfigPath, targetConfigData, 0644, folly::SyncType::WITH_SYNC); - atomicSymlinkUpdate(systemConfigPath, "cli/agent.conf"); - } - - // Promote the target BGP config (if it changed) so bgpd picks it up when - // restarted below. An empty target means BGP didn't exist at that revision, - // so remove the running config file. - if (bgpChanged) { - if (!targetBgpData.empty()) { - ensureDirectoryExists(getBgpSystemConfigDir()); - folly::writeFileAtomic( - bgpSystemPath, targetBgpData, 0644, folly::SyncType::WITH_SYNC); - } else if (bgpSystemExisted) { + std::vector rollbackFiles = {metadataPath}; + for (const auto& dr : doms) { + if (!dr.changed) { + continue; + } + if (!dr.target.empty()) { + promoteDomain(dr.domain, dr.target, rollbackFiles); + } else if (dr.promotedExisted) { std::error_code rmEc; - fs::remove(bgpSystemPath, rmEc); + fs::remove(dr.domain.promotedPath, rmEc); if (rmEc) { throw std::runtime_error( fmt::format( - "Failed to remove BGP config {} while rolling back to a " - "pre-BGP revision: {}", - bgpSystemPath, + "Failed to remove {} while rolling back to a revision that " + "predates it: {}", + dr.domain.promotedPath, rmEc.message())); } } @@ -1396,53 +1402,31 @@ std::string ConfigSession::rollback( // Apply the rolled-back config - if this fails, restore prior state. std::string newCommitSha; try { - // Reload the agent only if its config changed. - if (agentChanged) { - auto client = utils::createClient< - apache::thrift::Client>(hostInfo); - client->sync_reloadConfig(); - } - // Restart bgpd only if its config changed. - if (bgpChanged) { - ensureFbossServiceUtil(hostInfo); - fbossServiceUtil_->restartService( - cli::ServiceType::BGP, cli::ConfigActionLevel::BGP_RESTART); + // Reload/restart only the services whose config changed, each at its + // domain's rollback action level (agent -> HITLESS reload, bgpd -> + // restart). + std::map actions; + for (const auto& dr : doms) { + if (dr.changed) { + actions[dr.domain.service] = dr.domain.rollbackActionLevel; + } } + applyServiceActions(actions, hostInfo); - // Create a Git commit for the rollback. Metadata always changes; the agent - // config + symlink are included only when they were actually rewritten - // above (a BGP-only rollback leaves them untouched, so committing them - // could capture unrelated on-disk drift into the rollback commit). - std::vector rollbackFiles = {metadataPath}; - if (agentChanged) { - rollbackFiles.push_back(cliConfigPath); - rollbackFiles.push_back(systemConfigPath); - } - if (bgpChanged && !targetBgpData.empty()) { - rollbackFiles.push_back(bgpSystemPath); - } std::string commitMessage = fmt::format( "Rollback to {} by {}", Git::shortSha1(resolvedSha), username_); newCommitSha = git_->commit(rollbackFiles, commitMessage, username_, ""); LOG(INFO) << "Rollback committed as " << Git::shortSha1(newCommitSha); } catch (const std::exception& ex) { - // Rollback: restore the old config, metadata, and BGP config. + // Restore the old metadata and each changed domain's config. try { - if (!oldConfigData.empty()) { - folly::writeFileAtomic( - cliConfigPath, oldConfigData, 0644, folly::SyncType::WITH_SYNC); - } if (!oldMetadataData.empty()) { folly::writeFileAtomic( metadataPath, oldMetadataData, 0644, folly::SyncType::WITH_SYNC); } - if (bgpChanged) { - if (bgpSystemExisted) { - folly::writeFileAtomic( - bgpSystemPath, oldBgpData, 0644, folly::SyncType::WITH_SYNC); - } else { - std::error_code rmEc; - fs::remove(bgpSystemPath, rmEc); + for (const auto& dr : doms) { + if (dr.changed) { + restorePromotedDomain(dr.domain, dr.oldPromoted, dr.promotedExisted); } } } catch (const std::exception& rollbackEx) { @@ -1460,9 +1444,10 @@ std::string ConfigSession::rollback( } // The on-disk config changed underneath any cached in-memory state; force a - // reload on next access regardless of whether the session is clean. - configLoaded_ = false; - bgpConfigLoaded_ = false; + // reload on next access (null == not loaded) regardless of session + // cleanliness. + agentConfig_.reset(); + bgpConfig_.reset(); // Update the session state after rollback // Check if the current session is clean (no pending changes) @@ -1475,23 +1460,16 @@ std::string ConfigSession::rollback( // from its own rolled-back data. Unconditionally writing the agent session // file would materialize a phantom agent session after a BGP-only rollback // (and leave the BGP session file stale); keep the two domains symmetric. - if (sessionExists()) { - folly::writeFileAtomic( - getSessionConfigPath(), - targetConfigData, - 0644, - folly::SyncType::WITH_SYNC); - } - if (bgpSessionExists()) { - if (!targetBgpData.empty()) { + for (const auto& dr : doms) { + if (!fs::exists(dr.domain.sessionPath)) { + continue; + } + if (!dr.target.empty()) { folly::writeFileAtomic( - getBgpSessionConfigPath(), - targetBgpData, - 0644, - folly::SyncType::WITH_SYNC); + dr.domain.sessionPath, dr.target, 0644, folly::SyncType::WITH_SYNC); } else { std::error_code rmEc; - fs::remove(getBgpSessionConfigPath(), rmEc); + fs::remove(dr.domain.sessionPath, rmEc); } } diff --git a/fboss/cli/fboss2/session/ConfigSession.h b/fboss/cli/fboss2/session/ConfigSession.h index 8283f67ce824c..a21e20797893f 100644 --- a/fboss/cli/fboss2/session/ConfigSession.h +++ b/fboss/cli/fboss2/session/ConfigSession.h @@ -9,12 +9,16 @@ #pragma once +// Forward-declaration-only headers for the typed configs; the full generated +// types are heavy and are only needed in ConfigSession.cpp (agentConfig_ and +// bgpConfig_ are held by unique_ptr, so an incomplete type suffices here). +#include #include #include +#include #include #include -#include "configerator/structs/neteng/fboss/bgp/gen-cpp2/bgp_config_types.h" -#include "fboss/agent/gen-cpp2/agent_config_types.h" +#include "fboss/agent/gen-cpp2/agent_config_types_fwd.h" #include "fboss/cli/fboss2/gen-cpp2/cli_metadata_types.h" #include "fboss/cli/fboss2/session/FbossServiceUtil.h" #include "fboss/cli/fboss2/session/Git.h" @@ -86,7 +90,10 @@ namespace facebook::fboss { class ConfigSession { public: ConfigSession(); - virtual ~ConfigSession() = default; + // Defined out-of-line in the .cpp: the unique_ptr members hold + // forward-declared types, so the destructor must be emitted where those + // types are complete. + virtual ~ConfigSession(); // Get or create the current config session // If no session exists, copies /etc/coop/agent.conf to ~/.fboss2/agent.conf @@ -107,6 +114,13 @@ class ConfigSession { // clear` without instantiating a session). static std::string getBgpSessionConfigPathStatic(); + // All per-session staged files under ~/.fboss2 that `config session clear` + // should remove: every config domain's staged file plus the session + // metadata. Static so callers can clear a session without getInstance() + // (which would create one). A new config domain adds one entry here rather + // than a new block in the clear command. + static std::vector stagedSessionFilePaths(); + // Get the path to the session config file (~/.fboss2/agent.conf) std::string getSessionConfigPath() const; @@ -130,6 +144,38 @@ class ConfigSession { std::map> serviceNames; }; + // Describes one config "domain" managed by a session. The agent config and + // the BGP config are two such domains: both are staged in ~/.fboss2, promoted + // to a git-tracked file under /etc/coop, exposed to their daemon via a stable + // symlink, and applied via a service action. commit(), rollback() and `config + // session diff` iterate configDomains() so the two are handled uniformly; the + // per-domain differences (paths, service, how a rollback applies) live here + // rather than as branches in each routine. + struct ConfigDomain { + cli::ServiceType service; // AGENT / BGP -- feeds applyServiceActions() + std::string name; // "Agent" / "BGP" (diff section headers, logs) + std::string sessionPath; // staged edits (~/.fboss2/...) + std::string gitRelPath; // path within the /etc/coop git repo + std::string promotedPath; // absolute git-tracked file that is written + std::string systemPath; // live file to read for diff (agent: the symlink) + std::string symlinkPath; // daemon-facing stable path (a symlink) + std::string symlinkTarget; // relative target of symlinkPath + // Action used when a rollback changes this domain (no recorded action is + // available then): HITLESS reloads the agent; AGENT_WARMBOOT restarts bgpd. + cli::ConfigActionLevel rollbackActionLevel; + }; + + // The config domains this session manages (agent + BGP), in a stable order + // (agent first). Public so `config session diff` can share the same list. + std::vector configDomains() const; + + // Staged content for a domain, or nullopt if no session edit is staged. + // Throws if the session file exists but cannot be read. Public so `config + // session diff` shares the same "is it staged + its content" primitive that + // commit()/rollback() use. + std::optional readStagedContent( + const ConfigDomain& domain) const; + // Atomically commit the session to /etc/coop/cli/agent.conf and create a git // commit. For HITLESS changes, also calls reloadConfig() on the agent. // For AGENT_RESTART changes, restarts the agent via systemd. @@ -172,10 +218,10 @@ class ConfigSession { // subsequent getPortMap() lookups reflect the change. void rebuildPortMap(); - // Save the configuration back to the session file. - // Also updates the required action level for the specified service - // (if the new level is higher than the current one). - // This combines saving the config and updating its associated metadata. + // Serialize the given service's typed config (AGENT -> agentConfig_, + // BGP -> bgpConfig_) to that domain's staged session file, and record the + // command + bump the service's required action level (if the new level is + // higher than the current one). One generic entry point for every service. void saveConfig(cli::ServiceType service, cli::ConfigActionLevel actionLevel); // Save the configuration for AGENT service with HITLESS action level. void saveConfig(); @@ -205,9 +251,10 @@ class ConfigSession { bgp::thrift::BgpConfig& getBgpConfig(); const bgp::thrift::BgpConfig& getBgpConfig() const; - // Persist the typed BGP config back to ~/.fboss2/bgp_config.json and record - // that bgpd must be restarted for this change to take effect on a - // subsequent `config session commit`. Mirrors saveConfig() for the agent. + // Convenience wrapper over saveConfig(BGP, AGENT_WARMBOOT): persists the + // typed BGP config to ~/.fboss2/bgp_config.json and records that bgpd must be + // restarted on the next `config session commit`. Mirrors the no-arg + // saveConfig() for the agent. void saveBgpConfig(); // ~/.fboss2/bgp_config.json (staged BGP edits) @@ -282,15 +329,15 @@ class ConfigSession { // Git instance for version control operations std::unique_ptr git_; - // Lazy-initialized configuration and port map - cfg::AgentConfig agentConfig_; + // Lazy-initialized configuration and port map. agentConfig_ is null until + // loadConfig() populates it (null == "not loaded"), which is why it is a + // pointer -- that also keeps the heavy generated type out of this header. + std::unique_ptr agentConfig_; std::unique_ptr portMap_; - bool configLoaded_ = false; - // Typed view of the entire BGP config (lazily loaded), mirroring - // agentConfig_. - bgp::thrift::BgpConfig bgpConfig_; - bool bgpConfigLoaded_ = false; + // Typed view of the entire BGP config, mirroring agentConfig_: null until + // loadBgpConfig() populates it. + std::unique_ptr bgpConfig_; // /etc/coop/bgpcpp (directory holding the bgpd daemon's config) std::string getBgpSystemConfigDir() const; @@ -303,8 +350,46 @@ class ConfigSession { // defaults). Mirrors loadConfig() for the agent. void loadBgpConfig(); + // ==================== Per-domain primitives ==================== + // Shared building blocks used by commit()/rollback() so both the agent and + // BGP domains go through identical logic (see ConfigDomain / + // configDomains()). readStagedContent() is declared public above. + + // Currently-promoted (git-tracked) content, or "" if the file does not exist. + // Throws if the file exists but cannot be read (so a silent read failure + // never masquerades as "no config", which a later restore would write back + // empty). + std::string readPromotedContent(const ConfigDomain& domain) const; + // Promote staged content to the domain's git-tracked file and refresh its + // daemon-facing symlink, appending both to commitFiles for the git commit. + void promoteDomain( + const ConfigDomain& domain, + const std::string& content, + std::vector& commitFiles) const; + // Restore a domain's promoted file to prior content (or remove it if it did + // not previously exist). Used by the commit/rollback failure paths. + void restorePromotedDomain( + const ConfigDomain& domain, + const std::string& oldContent, + bool existed) const; + // Remove a domain's staged session file and drop its in-memory cache so the + // next access re-seeds from disk. Called after a successful commit. + void clearStagedDomain(const ConfigDomain& domain); + // Compare two serialized configs for a domain by deserializing each into its + // typed thrift struct (cfg::AgentConfig / bgp::thrift::BgpConfig) and using + // struct equality. This is a SEMANTIC comparison, so formatting-only + // differences (whitespace, key ordering, integer-vs-string map keys, a + // raw-seeded file vs a round-tripped one) do not count as a change. Falls + // back to a byte comparison when either side is empty or fails to parse. + bool domainContentEqual( + const ConfigDomain& domain, + const std::string& a, + const std::string& b) const; + // git relative path of the bgpd config tracked in the /etc/coop repo. static constexpr auto kBgpGitRelPath = "bgpcpp/bgpcpp.conf"; + // git relative path of the agent config tracked in the /etc/coop repo. + static constexpr auto kAgentGitRelPath = "cli/agent.conf"; // Like Git::fileAtRevision but returns "" instead of throwing when the path // does not exist at that revision (e.g. a pre-BGP commit). Used by // rebase/rollback/diff so a missing bgpcpp.conf is treated as empty. diff --git a/fboss/cli/fboss2/session/FbossServiceUtil.cpp b/fboss/cli/fboss2/session/FbossServiceUtil.cpp index 060d1a886bde4..c178555e0ece9 100644 --- a/fboss/cli/fboss2/session/FbossServiceUtil.cpp +++ b/fboss/cli/fboss2/session/FbossServiceUtil.cpp @@ -159,7 +159,7 @@ std::vector FbossServiceUtil::reloadConfig( } case cli::ServiceType::BGP: // bgpd has no hitless reloadConfig() RPC; config changes are applied by - // restarting the service (BGP_RESTART), so this path is never taken. + // restarting the service (AGENT_WARMBOOT), so this path is never taken. throw std::runtime_error( "bgpd does not support config reload; it must be restarted"); } @@ -177,9 +177,6 @@ std::vector FbossServiceUtil::restartService( case cli::ConfigActionLevel::AGENT_WARMBOOT: restartType = "warmboot"; break; - case cli::ConfigActionLevel::BGP_RESTART: - restartType = "restart"; - break; case cli::ConfigActionLevel::HITLESS: // Not expected: HITLESS is applied via reloadConfig(), not restart. restartType = "reload"; diff --git a/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp index f142911167558..7fcaf01ddee55 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp @@ -955,6 +955,62 @@ TEST_F(ConfigSessionTestFixture, concurrentSessionConflict) { EXPECT_THAT(content, ::testing::Not(::testing::HasSubstr("User2 change"))); } +// BGP analog of concurrentSessionConflict: two BGP sessions start from the same +// base; once user1 commits (advancing HEAD), user2's commit must be rejected +// because its base is now stale -- one session "steps onto" the other. Only +// user1's BGP change reaches the running bgpd config. +TEST_F(ConfigSessionTestFixture, concurrentBgpSessionConflict) { + fs::path sessionDir1 = getTestHomeDir() / ".fboss2_user1"; + fs::path sessionDir2 = getTestHomeDir() / ".fboss2_user2"; + fs::path bgpSys = getTestEtcDir() / "coop" / "bgpcpp" / "bgpcpp.conf"; + + auto makeSession = [&](const fs::path& dir) { + auto s = std::make_unique( + dir.string(), (getTestEtcDir() / "coop").string()); + // BGP commits restart bgpd via systemd; mock it out. Only user1 commits, so + // no agent reload is triggered (no mocked agent server needed). + s->setMockSystemdFactory([] { + return std::make_unique<::testing::NiceMock>(); + }); + return s; + }; + + // Both users start sessions at the same base (current HEAD). + auto session1 = makeSession(sessionDir1); + auto session2 = makeSession(sessionDir2); + + // User1 stages a BGP change and commits -> HEAD advances. + session1->getBgpConfig().router_id() = "1.1.1.1"; + session1->setCommandLine("config protocol bgp global router-id 1.1.1.1"); + session1->saveBgpConfig(); + EXPECT_FALSE(session1->commit(localhost()).commitSha.empty()); + + // User2 stages a different BGP change on top of the now-stale base. + session2->getBgpConfig().router_id() = "2.2.2.2"; + session2->setCommandLine("config protocol bgp global router-id 2.2.2.2"); + session2->saveBgpConfig(); + + // User2's commit must fail because user1 already advanced HEAD. + EXPECT_THROW( + { + try { + session2->commit(localhost()); + } catch (const std::runtime_error& e) { + EXPECT_THAT( + e.what(), + ::testing::HasSubstr("system configuration has changed")); + throw; + } + }, + std::runtime_error); + + // Only user1's BGP change reached the running bgpd config. + std::string content; + ASSERT_TRUE(folly::readFile(bgpSys.string().c_str(), content)); + EXPECT_THAT(content, ::testing::HasSubstr("1.1.1.1")); + EXPECT_THAT(content, ::testing::Not(::testing::HasSubstr("2.2.2.2"))); +} + TEST_F(ConfigSessionTestFixture, rebaseSuccessNoConflict) { // Test successful rebase when user2's changes don't conflict with user1's fs::path sessionDir1 = getTestHomeDir() / ".fboss2_user1"; @@ -1087,8 +1143,12 @@ TEST_F(ConfigSessionTestFixture, threeWayMergeScenarios) { fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; setupMockedAgentServer(); - // 5 commits: 2 in scenario 1, 2 in scenario 2, 1 in scenario 3 (rebase fails) - EXPECT_CALL(getMockAgent(), reloadConfig()).Times(5); + // Reloads happen only when a commit actually changes the promoted config + // (agent skip-when-unchanged): scenario 1 = 2 commits (both change config); + // scenario 2's session2 rebases to the SAME value session1 committed, so its + // commit is a no-op and does NOT reload -> only 1 reload there; scenario 3 = + // 1 commit (session2's rebase throws before committing). Total = 2 + 1 + 1. + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(4); // Scenario 1: Only session changed, head unchanged // User1 commits, User2 changes different field - should merge cleanly @@ -1236,6 +1296,48 @@ TEST_F(ConfigSessionTestFixture, emptyCommit) { EXPECT_TRUE(session.sessionExists()); } +// Re-committing an agent config that is byte-identical to what is already +// promoted must be a no-op: no git revision and (crucially) no reloadConfig(). +// A config command records an AGENT action even when it sets a field to its +// current value, so commit() must skip based on content equality (the same +// skip-when-unchanged rule BGP uses). +TEST_F(ConfigSessionTestFixture, commitUnchangedAgentConfigIsNoOp) { + fs::path sessionDir = getTestHomeDir() / ".fboss2"; + + setupMockedAgentServer(); + // The first (real) commit reloads once; the second (unchanged) commit must + // NOT reload. + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(1); + + // First commit: set a description and commit. This normalizes cli/agent.conf + // into the canonical serialized form. + { + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); + (*session.getAgentConfig().sw()->ports())[0].description() = "same_desc"; + session.setCommandLine("config interface eth1/1/1 description same_desc"); + session.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + ASSERT_FALSE(session.commit(localhost()).commitSha.empty()); + } + + // Second commit: set the SAME description again -> staged config is identical + // to what is running, so the commit is a no-op (empty commitSha, no reload). + { + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); + (*session.getAgentConfig().sw()->ports())[0].description() = "same_desc"; + session.setCommandLine("config interface eth1/1/1 description same_desc"); + session.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + auto result = session.commit(localhost()); + EXPECT_TRUE(result.commitSha.empty()) + << "re-committing an unchanged agent config should be a no-op (no reload)"; + EXPECT_EQ(result.actions.count(cli::ServiceType::AGENT), 0u) + << "unchanged agent config must not apply a reload"; + } +} + // Test that committing twice in a row - second commit should be empty TEST_F(ConfigSessionTestFixture, commitTwiceSecondIsEmpty) { fs::path sessionDir = getTestHomeDir() / ".fboss2"; @@ -1326,9 +1428,11 @@ TEST_F(ConfigSessionTestFixture, bgpConfigEditPreservesOtherSections) { EXPECT_EQ(saved["peer_groups"][0]["name"].asString(), "RACK"); // A BGP restart must be recorded so `config session commit` applies it. + // bgpd has no hitless reload, so the recorded level is AGENT_WARMBOOT (a + // plain service restart for BGP). EXPECT_EQ( session.getRequiredAction(cli::ServiceType::BGP), - cli::ConfigActionLevel::BGP_RESTART); + cli::ConfigActionLevel::AGENT_WARMBOOT); } // A staged BGP edit persists to disk and is seeded back by a fresh session via @@ -1450,7 +1554,7 @@ TEST_F(ConfigSessionTestFixture, rollbackBgpConfig) { // Re-committing a BGP config that is byte-identical to the running // /etc/coop/bgpcpp/bgpcpp.conf must be a no-op: no git commit and (crucially) -// no disruptive bgpd restart. saveBgpConfig() records BGP_RESTART +// no disruptive bgpd restart. saveBgpConfig() records a restart // unconditionally, so commit() compares staged vs running content. TEST_F(ConfigSessionTestFixture, commitUnchangedBgpConfigIsNoOp) { fs::path sessionDir = getTestHomeDir() / ".fboss2"; @@ -1481,7 +1585,7 @@ TEST_F(ConfigSessionTestFixture, commitUnchangedBgpConfigIsNoOp) { EXPECT_TRUE(result.commitSha.empty()) << "committing an unchanged BGP config should be a no-op (no restart)"; EXPECT_EQ(result.actions.count(cli::ServiceType::BGP), 0u) - << "unchanged BGP config must not apply BGP_RESTART"; + << "unchanged BGP config must not apply a bgpd restart"; } } @@ -1527,8 +1631,8 @@ TEST_F(ConfigSessionTestFixture, commitThrowsWhenRunningBgpConfigUnreadable) { // A BGP-only session (bgp_config.json + metadata present, agent.conf session // file absent) must RESUME on the next CLI invocation, not be misdetected as -// fresh -- otherwise requiredActions_ (BGP_RESTART) is cleared and the -// staged change is silently dropped at commit time. +// fresh -- otherwise requiredActions_ (the recorded bgpd restart) is cleared +// and the staged change is silently dropped at commit time. TEST_F(ConfigSessionTestFixture, bgpOnlySessionResumesAcrossInvocations) { fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path agentSess = sessionDir / "agent.conf"; diff --git a/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp index c0cb7b2a1543d..1347c6e2809a6 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp @@ -89,3 +89,52 @@ TEST_F(ConfigInterfaceDescriptionTest, SetAndVerifyDescription) { XLOG(INFO) << "TEST PASSED"; } + +// Setting an interface description to the value it already has must produce a +// no-op commit: the staged agent.conf is byte-identical to what is running, so +// `config session commit` reports "Nothing to commit" and does NOT reload the +// agent (skip-when-unchanged). Regression test for the agent-side unification. +TEST_F(ConfigInterfaceDescriptionTest, SetSameDescriptionIsNoOpCommit) { + XLOG(INFO) << "[Step 1] Finding an interface to test..."; + Interface interface = findFirstEthInterface(); + const std::string originalDescription = interface.description; + XLOG(INFO) << " Using interface: " << interface.name << " (description: '" + << originalDescription << "')"; + + // Set a deterministic description and commit so cli/agent.conf is in the + // canonical serialized form. + std::string testDescription = "CLI_E2E_NOOP_DESCRIPTION"; + XLOG(INFO) << "[Step 2] Setting description to '" << testDescription + << "' and committing..."; + setInterfaceDescription(interface.name, testDescription); + waitForInterfaceInfo(interface.name, [&](const auto& info) { + return info.description == testDescription; + }); + + // Stage the SAME description again, then commit directly so we can inspect + // the output. The staged config equals what is running -> the commit is a + // no-op. + XLOG(INFO) << "[Step 3] Re-setting the SAME description and committing..."; + auto setResult = runCli( + {"config", "interface", interface.name, "description", testDescription}); + ASSERT_EQ(setResult.exitCode, 0) << setResult.stderr; + auto commitResult = runCli({"config", "session", "commit"}); + ASSERT_EQ(commitResult.exitCode, 0) << commitResult.stderr; + XLOG(INFO) << " Commit output: " << commitResult.stdout; + EXPECT_NE(commitResult.stdout.find("Nothing to commit"), std::string::npos) + << "Re-committing an unchanged description must be a no-op, got: " + << commitResult.stdout; + EXPECT_EQ(commitResult.stdout.find("reloaded"), std::string::npos) + << "A no-op commit must not reload the agent, got: " + << commitResult.stdout; + + // Restore the original description. + XLOG(INFO) << "[Step 4] Restoring original description ('" + << originalDescription << "')..."; + setInterfaceDescription(interface.name, originalDescription); + waitForInterfaceInfo(interface.name, [&](const auto& info) { + return info.description == originalDescription; + }); + + XLOG(INFO) << "TEST PASSED"; +} From 8d7400328f57e2f28f86e938171fab5a6b85b922 Mon Sep 17 00:00:00 2001 From: hillol-nexthop Date: Fri, 31 Jul 2026 12:31:39 +0530 Subject: [PATCH 2/4] Remove comments regarding destructor in ConfigSession Removed comments about the destructor definition in ConfigSession.h. --- fboss/cli/fboss2/session/ConfigSession.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fboss/cli/fboss2/session/ConfigSession.h b/fboss/cli/fboss2/session/ConfigSession.h index a21e20797893f..0d36a5b34ec8e 100644 --- a/fboss/cli/fboss2/session/ConfigSession.h +++ b/fboss/cli/fboss2/session/ConfigSession.h @@ -90,9 +90,7 @@ namespace facebook::fboss { class ConfigSession { public: ConfigSession(); - // Defined out-of-line in the .cpp: the unique_ptr members hold - // forward-declared types, so the destructor must be emitted where those - // types are complete. + virtual ~ConfigSession(); // Get or create the current config session From 34f671eaee24f0aae5eb2b7e09d1f78259027f0c Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 31 Jul 2026 08:18:16 +0000 Subject: [PATCH 3/4] Fix rebase fallout: orphaned ConfigSession includes, stale findFirstEthInterface 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). --- fboss/cli/fboss2/session/ConfigSession.cpp | 3 --- .../test/integration_test/ConfigInterfaceDescriptionTest.cpp | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index 219f7deeadb28..7d837eac51a99 100644 --- a/fboss/cli/fboss2/session/ConfigSession.cpp +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -41,13 +41,10 @@ #include "fboss/agent/AgentDirectoryUtil.h" #include "fboss/agent/gen-cpp2/agent_config_types.h" #include "fboss/agent/gen-cpp2/switch_config_types.h" -#include "fboss/agent/if/gen-cpp2/FbossCtrl.h" -#include "fboss/agent/if/gen-cpp2/FbossCtrlAsyncClient.h" #include "fboss/cli/fboss2/gen-cpp2/cli_metadata_types.h" #include "fboss/cli/fboss2/session/FbossServiceUtil.h" #include "fboss/cli/fboss2/session/Git.h" #include "fboss/cli/fboss2/utils/CmdClientUtils.h" -#include "fboss/cli/fboss2/utils/CmdClientUtilsCommon.h" #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fboss/cli/fboss2/utils/PortMap.h" diff --git a/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp index 1347c6e2809a6..952c566675b38 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp @@ -96,7 +96,7 @@ TEST_F(ConfigInterfaceDescriptionTest, SetAndVerifyDescription) { // agent (skip-when-unchanged). Regression test for the agent-side unification. TEST_F(ConfigInterfaceDescriptionTest, SetSameDescriptionIsNoOpCommit) { XLOG(INFO) << "[Step 1] Finding an interface to test..."; - Interface interface = findFirstEthInterface(); + Interface interface = getInterfaceInfo(getRandomInterfacePortName()); const std::string originalDescription = interface.description; XLOG(INFO) << " Using interface: " << interface.name << " (description: '" << originalDescription << "')"; From 03823a21206de80628e9c94a19bb86f5fc47a529 Mon Sep 17 00:00:00 2001 From: hillol-nexthop Date: Sat, 8 Aug 2026 02:54:51 +0530 Subject: [PATCH 4/4] fboss2 config session seeds BGP config from the daemon's config path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- fboss/cli/fboss2/session/ConfigSession.cpp | 37 ++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index 7d837eac51a99..8803b9e981647 100644 --- a/fboss/cli/fboss2/session/ConfigSession.cpp +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -421,7 +421,10 @@ std::vector ConfigSession::configDomains() const { getBgpSessionConfigPath(), // ~/.fboss2/bgp_config.json kBgpGitRelPath, // bgpcpp/bgpcpp.conf getBgpSystemConfigPath(), // /etc/coop/bgpcpp/bgpcpp.conf (promoted) - getBgpSystemConfigPath(), // the promoted file is also the live read + getBgpSystemConfigLinkPath(), // /etc/coop/bgpcpp.conf (the symlink, + // as for the agent: it is what bgpd + // reads, and before the first commit + // it is the image-installed file) getBgpSystemConfigLinkPath(), // /etc/coop/bgpcpp.conf (symlink) kBgpGitRelPath, // symlink -> bgpcpp/bgpcpp.conf cli::ConfigActionLevel::AGENT_WARMBOOT, // rollback restarts bgpd @@ -696,17 +699,30 @@ void ConfigSession::loadBgpConfig() { // schema defaults. A read failure on a file that exists is logged (not // silently treated as "no config") so a permission/IO error doesn't // masquerade as a fresh session. + // The running config is read through the daemon's own --config path, which + // is a symlink to the promoted file once a commit has happened and the + // plain file the bgp++ RPM installs before that. Seeding from it (rather + // than from the promoted path directly) is what keeps a first BGP edit on a + // freshly imaged box from starting at schema defaults and having the commit + // discard the running config, leaving bgpd to crash-loop on an unset + // router_id. The promoted path is a backstop for a missing symlink. std::string content; std::string sessionPath = getBgpSessionConfigPath(); + std::string linkPath = getBgpSystemConfigLinkPath(); std::string systemPath = getBgpSystemConfigPath(); if (fs::exists(sessionPath)) { if (!folly::readFile(sessionPath.c_str(), content)) { LOG(WARNING) << "Failed to read staged BGP config " << sessionPath << "; starting from defaults"; } + } else if (fs::exists(linkPath)) { + if (!folly::readFile(linkPath.c_str(), content)) { + LOG(WARNING) << "Failed to read system BGP config " << linkPath + << "; starting from defaults"; + } } else if (fs::exists(systemPath)) { if (!folly::readFile(systemPath.c_str(), content)) { - LOG(WARNING) << "Failed to read system BGP config " << systemPath + LOG(WARNING) << "Failed to read promoted BGP config " << systemPath << "; starting from defaults"; } } @@ -1030,9 +1046,26 @@ void ConfigSession::initializeGit() { // the first revision has no BGP snapshot, so a rollback to it would read // the empty target as "BGP never existed" and DELETE the running // bgpcpp.conf. + // + // Mirroring the agent above: if the promoted path doesn't exist yet but + // the daemon's config path resolves to a readable file (the bgp++ RPM + // ships it as a plain file), populate the promoted path from it. Copy + // rather than rename — bgpd reads /etc/coop/bgpcpp.conf right now, and + // commit() is what later replaces it with a symlink to the promoted copy. std::vector initialFiles = { cliConfigPath, initialMetadataPath}; std::string bgpSystemPath = getBgpSystemConfigPath(); + std::string bgpLinkPath = getBgpSystemConfigLinkPath(); + if (!fs::exists(bgpSystemPath) && fs::exists(bgpLinkPath)) { + // fs::exists follows symlinks, so a dangling one is correctly skipped. + std::string bgpSeedContent; + if (folly::readFile(bgpLinkPath.c_str(), bgpSeedContent) && + !bgpSeedContent.empty()) { + ensureDirectoryExists(getBgpSystemConfigDir()); + folly::writeFileAtomic( + bgpSystemPath, bgpSeedContent, 0644, folly::SyncType::WITH_SYNC); + } + } if (fs::exists(bgpSystemPath)) { initialFiles.push_back(bgpSystemPath); }