diff --git a/fboss/cli/fboss2/cli_metadata.thrift b/fboss/cli/fboss2/cli_metadata.thrift index f933911900a53..853af61d1a56b 100644 --- a/fboss/cli/fboss2/cli_metadata.thrift +++ b/fboss/cli/fboss2/cli_metadata.thrift @@ -14,12 +14,17 @@ namespace cpp2 facebook.fboss.cli // Action level required for config changes to take effect. // Used to track the highest impact action needed when committing config -// changes. +// changes. The levels are generic across services; how a level is carried out +// for a given service (agent warmboot vs plain bgpd restart, ...) is decided by +// FbossServiceUtil::restartService() from the (service, level) pair. enum ConfigActionLevel { - HITLESS = 0, // Can be applied with reloadConfig() - default - AGENT_WARMBOOT = 1, // Requires agent warmboot restart - AGENT_COLDBOOT = 2, // Requires agent coldboot restart (clears ASIC state) - BGP_RESTART = 3, // Requires a restart of the bgpd (BGP++) service + // Can be applied with reloadConfig() if the service supports it - default + HITLESS = 0, + // Restart the service gracefully, retaining state where the service can: + // an agent warmboot, or a plain restart for daemons with no warmboot (bgpd). + SERVICE_RESTART = 1, + // Restart the service disruptively: an agent coldboot (clears ASIC state). + DISRUPTIVE_SERVICE_RESTART = 2, } // Identifier for different services that can be configured diff --git a/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.cpp b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.cpp index 912d094956065..5892b99bde9bb 100644 --- a/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.cpp +++ b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.cpp @@ -215,8 +215,8 @@ std::string applyProfileImpl( // Removing a port must not be applied on a live agent (crashes the SwAgent // -- see T281221621); escalate the commit to a coldboot. if (actionLevel != nullptr) { - *actionLevel = - std::max(*actionLevel, cli::ConfigActionLevel::AGENT_COLDBOOT); + *actionLevel = std::max( + *actionLevel, cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART); } } @@ -631,9 +631,10 @@ CmdConfigInterfaceTraits::RetType CmdConfigInterface::queryClient( utils::InterfaceList resolved(std::vector{}); // The commit action level required by this command, escalated by attribute // handlers as needed. Starts HITLESS (reloadConfig); a profile change that - // removes a port escalates it to AGENT_COLDBOOT, because applying a port - // removal on a live agent can crash the SwAgent (LookupClassRouteUpdater - // dereferences the removed port's now-absent interface). See T281221621. + // removes a port escalates it to DISRUPTIVE_SERVICE_RESTART, because applying + // a port removal on a live agent can crash the SwAgent + // (LookupClassRouteUpdater dereferences the removed port's now-absent + // interface). See T281221621. cli::ConfigActionLevel actionLevel = cli::ConfigActionLevel::HITLESS; if (profileValue.has_value()) { results.push_back( diff --git a/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h index ae6de78d3e60f..ace58305d7a8e 100644 --- a/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h +++ b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h @@ -76,10 +76,11 @@ class CmdConfigInterface // unchanged). Declared here for unit testing. // // If `actionLevel` is non-null, it is escalated (raised, never lowered) to the -// commit action level this profile change requires -- e.g. AGENT_COLDBOOT when -// the change subsumes/removes a port (see T281221621). Callers thread one -// ConfigActionLevel through every attribute handler and commit at the resulting -// level, so any future attribute needing warmboot/coldboot can escalate it too. +// commit action level this profile change requires -- e.g. +// DISRUPTIVE_SERVICE_RESTART when the change subsumes/removes a port (see +// T281221621). Callers thread one ConfigActionLevel through every attribute +// handler and commit at the resulting level, so any future attribute needing +// warmboot/coldboot can escalate it too. std::string applyProfileImpl( ProfileValidator& validator, cfg::SwitchConfig& swConfig, diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp index 51ebe482cc20e..4ac9c9a257c77 100644 --- a/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp +++ b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp @@ -62,7 +62,7 @@ CmdConfigInterfaceSwitchportAccessVlan::queryClient( // Save the updated config // VLAN changes require agent warmboot as reloadConfig() doesn't apply them ConfigSession::getInstance().saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); std::string interfaceList = folly::join(", ", interfaces.getNames()); std::string message = "Successfully set access VLAN for interface(s) " + diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/trunk/allowed/vlan/CmdConfigInterfaceSwitchportTrunkAllowedVlan.cpp b/fboss/cli/fboss2/commands/config/interface/switchport/trunk/allowed/vlan/CmdConfigInterfaceSwitchportTrunkAllowedVlan.cpp index 7c58d05ec4bc2..62095c63bb279 100644 --- a/fboss/cli/fboss2/commands/config/interface/switchport/trunk/allowed/vlan/CmdConfigInterfaceSwitchportTrunkAllowedVlan.cpp +++ b/fboss/cli/fboss2/commands/config/interface/switchport/trunk/allowed/vlan/CmdConfigInterfaceSwitchportTrunkAllowedVlan.cpp @@ -161,7 +161,7 @@ CmdConfigInterfaceSwitchportTrunkAllowedVlan::queryClient( // VLAN changes require agent warmboot as reloadConfig() doesn't apply them session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); return fmt::format( "Successfully {} {} VLAN-port association(s) for VLAN(s) {} on interface(s) {}", diff --git a/fboss/cli/fboss2/commands/config/l2/learning_mode/CmdConfigL2LearningMode.cpp b/fboss/cli/fboss2/commands/config/l2/learning_mode/CmdConfigL2LearningMode.cpp index c459fa6b27fbd..02c11d7d1cb8d 100644 --- a/fboss/cli/fboss2/commands/config/l2/learning_mode/CmdConfigL2LearningMode.cpp +++ b/fboss/cli/fboss2/commands/config/l2/learning_mode/CmdConfigL2LearningMode.cpp @@ -85,7 +85,8 @@ CmdConfigL2LearningModeTraits::RetType CmdConfigL2LearningMode::queryClient( // Save the updated config - L2 learning mode changes require agent cold boot // to clear ASIC state session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_COLDBOOT); + cli::ServiceType::AGENT, + cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART); return fmt::format( "Successfully set L2 learning mode to '{}'", diff --git a/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp index eac393edd6fae..5eb722548479c 100644 --- a/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp @@ -146,7 +146,7 @@ CmdConfigQosBufferPoolTraits::RetType CmdConfigQosBufferPool::queryClient( // Save the updated config - buffer pool changes require agent restart session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); return fmt::format("Successfully configured buffer-pool '{}'", poolName); } diff --git a/fboss/cli/fboss2/commands/config/qos/priority_group_policy/CmdConfigQosPriorityGroupPolicyGroupId.cpp b/fboss/cli/fboss2/commands/config/qos/priority_group_policy/CmdConfigQosPriorityGroupPolicyGroupId.cpp index e7a2772d9df30..8159a6a739d4c 100644 --- a/fboss/cli/fboss2/commands/config/qos/priority_group_policy/CmdConfigQosPriorityGroupPolicyGroupId.cpp +++ b/fboss/cli/fboss2/commands/config/qos/priority_group_policy/CmdConfigQosPriorityGroupPolicyGroupId.cpp @@ -182,7 +182,7 @@ CmdConfigQosPriorityGroupPolicyGroupId::queryClient( // Save the updated config session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); return fmt::format( "Successfully configured priority-group-policy '{}' group-id {}", diff --git a/fboss/cli/fboss2/commands/config/qos/queue_config/CmdConfigQosQueueConfigQueueId.cpp b/fboss/cli/fboss2/commands/config/qos/queue_config/CmdConfigQosQueueConfigQueueId.cpp index d124e8a0b4555..9952c6e640828 100644 --- a/fboss/cli/fboss2/commands/config/qos/queue_config/CmdConfigQosQueueConfigQueueId.cpp +++ b/fboss/cli/fboss2/commands/config/qos/queue_config/CmdConfigQosQueueConfigQueueId.cpp @@ -65,7 +65,7 @@ CmdConfigQosQueueConfigQueueId::queryClient( } session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); return fmt::format( "Successfully configured queue-config '{}' queue-id {}", 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..bbee7bb80602d 100644 --- a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp @@ -16,6 +16,7 @@ #include #include #include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/session/FbossServiceUtil.h" namespace facebook::fboss { @@ -47,21 +48,16 @@ CmdConfigSessionCommitTraits::RetType CmdConfigSessionCommit::queryClient( const auto& serviceNamesList = it->second; switch (level) { - case cli::ConfigActionLevel::AGENT_COLDBOOT: + case cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART: + case cli::ConfigActionLevel::SERVICE_RESTART: + // Label with what the level meant for this service (agent: warmboot + // or coldboot; bgpd: a plain restart). for (const auto& serviceName : serviceNamesList) { restartedServices.push_back( - fmt::format("{} (coldboot)", serviceName)); - } - break; - case cli::ConfigActionLevel::AGENT_WARMBOOT: - for (const auto& serviceName : serviceNamesList) { - restartedServices.push_back( - fmt::format("{} (warmboot)", serviceName)); - } - break; - case cli::ConfigActionLevel::BGP_RESTART: - for (const auto& serviceName : serviceNamesList) { - restartedServices.push_back(fmt::format("{} (restart)", serviceName)); + fmt::format( + "{} ({})", + serviceName, + FbossServiceUtil::restartTypeName(service, level))); } break; case cli::ConfigActionLevel::HITLESS: diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp index 3d4e1aefa0be9..24b5732ff310b 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); @@ -186,35 +159,35 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient( auto& session = ConfigSession::getInstance(ConfigSession::SessionInit::ReadOnly); 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; }; @@ -224,7 +197,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")); }); @@ -235,8 +208,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); }); } @@ -247,8 +220,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/commands/config/switch/admin_distance/CmdConfigAdminDistance.cpp b/fboss/cli/fboss2/commands/config/switch/admin_distance/CmdConfigAdminDistance.cpp index be33e89c25f8d..46eebc7a52bbc 100644 --- a/fboss/cli/fboss2/commands/config/switch/admin_distance/CmdConfigAdminDistance.cpp +++ b/fboss/cli/fboss2/commands/config/switch/admin_distance/CmdConfigAdminDistance.cpp @@ -119,7 +119,8 @@ CmdConfigAdminDistanceTraits::RetType CmdConfigAdminDistance::queryClient( // routes are not re-stamped when the map changes. A coldboot is required to // flush and re-program all routes with the updated admin distances. session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_COLDBOOT); + cli::ServiceType::AGENT, + cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART); return fmt::format( "Successfully set admin distance for client-id {} to {}. " diff --git a/fboss/cli/fboss2/commands/delete/qos/default_policy/CmdDeleteQosDefaultPolicy.cpp b/fboss/cli/fboss2/commands/delete/qos/default_policy/CmdDeleteQosDefaultPolicy.cpp index 49d9cdc3b3759..fa66169c1bcb1 100644 --- a/fboss/cli/fboss2/commands/delete/qos/default_policy/CmdDeleteQosDefaultPolicy.cpp +++ b/fboss/cli/fboss2/commands/delete/qos/default_policy/CmdDeleteQosDefaultPolicy.cpp @@ -39,7 +39,8 @@ CmdDeleteQosDefaultPolicyTraits::RetType CmdDeleteQosDefaultPolicy::queryClient( // handle bookkeeping in the hw agent and the next qos policy change aborts // it. Relax to HITLESS once the agent handle-ownership bug is fixed. session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_COLDBOOT); + cli::ServiceType::AGENT, + cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART); return fmt::format( "Successfully removed default QoS policy '{}'", removedName); diff --git a/fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfig.cpp b/fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfig.cpp index e4e6588e9528e..a9a63d50f669b 100644 --- a/fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfig.cpp +++ b/fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfig.cpp @@ -45,7 +45,7 @@ CmdDeleteQosQueueConfigTraits::RetType CmdDeleteQosQueueConfig::queryClient( defaultPortQueues.clear(); session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); return "Successfully deleted the default queue config"; } @@ -77,7 +77,7 @@ CmdDeleteQosQueueConfigTraits::RetType CmdDeleteQosQueueConfig::queryClient( portQueueConfigs.erase(it); session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); return fmt::format("Successfully deleted queue config '{}'", name.getName()); } diff --git a/fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfigQueueId.cpp b/fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfigQueueId.cpp index bd1ad6f2f93d8..bf3a69791a338 100644 --- a/fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfigQueueId.cpp +++ b/fboss/cli/fboss2/commands/delete/qos/queue_config/CmdDeleteQosQueueConfigQueueId.cpp @@ -62,7 +62,7 @@ CmdDeleteQosQueueConfigQueueId::queryClient( configList->erase(it); session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); return fmt::format( "Successfully deleted queue config '{}' queue-id {}", diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index fe7127050499f..45ed8f7a35d1d 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 @@ -40,12 +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/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/ConfigFileUtils.h" #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fboss/cli/fboss2/utils/PortMap.h" @@ -55,17 +54,6 @@ namespace facebook::fboss { namespace { // anonymous namespace -cli::ConfigSessionMetadata parseMetadata(const std::string& content) { - const auto json = folly::parseJson(content); - cli::ConfigSessionMetadata metadata; - facebook::thrift::from_dynamic( - metadata, - json, - facebook::thrift::dynamic_format::PORTABLE, - facebook::thrift::format_adherence::LENIENT); - return metadata; -} - /* * Atomically update a symlink to point to a new target. * This creates a temporary symlink and then atomically renames it over the @@ -332,6 +320,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; @@ -372,6 +364,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 { @@ -400,6 +403,154 @@ 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 + // Rollback floor: reload the agent, unless a commit being undone + // recorded a higher level (see rolledBackActionLevels()). + cli::ConfigActionLevel::HITLESS, + }, + ConfigDomain{ + cli::ServiceType::BGP, + "BGP", + getBgpSessionConfigPath(), // ~/.fboss2/bgp_config.json + kBgpGitRelPath, // bgpcpp/bgpcpp.conf + getBgpSystemConfigPath(), // /etc/coop/bgpcpp/bgpcpp.conf (promoted) + 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::SERVICE_RESTART, // 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()); } @@ -408,34 +559,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 (SERVICE_RESTART) 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."); } @@ -443,21 +595,46 @@ 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; + } } - auto prettyJson = utils::serializeToPrettyJson(agentConfig_); - // May not exist yet if this session was constructed ReadOnly. ensureDirectoryExists(sessionConfigDir_); @@ -466,30 +643,12 @@ void ConfigSession::saveConfig( // 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); - } - } + sessionPath, prettyJson, 0644, folly::SyncType::WITH_SYNC); - // Update the required action metadata for this service - updateRequiredAction(service, actionLevel); - - // 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() { @@ -538,7 +697,7 @@ bool ConfigSession::bgpSessionExists() const { } void ConfigSession::loadBgpConfig() { - if (bgpConfigLoaded_) { + if (bgpConfig_) { return; } @@ -546,63 +705,68 @@ 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"; } } - 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(); - } - - auto prettyJson = utils::serializeToPrettyJson(bgpConfig_); - 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 (SERVICE_RESTART) on the next + // `config session commit`. + saveConfig(cli::ServiceType::BGP, cli::ConfigActionLevel::SERVICE_RESTART); } Git& ConfigSession::getGit() { @@ -642,7 +806,13 @@ void ConfigSession::loadMetadata() { // Parse JSON with symbolic enum names using fbthrift's folly_dynamic API // LENIENT adherence allows parsing both string names and integer values try { - const auto metadata = parseMetadata(content); + folly::dynamic json = folly::parseJson(content); + cli::ConfigSessionMetadata metadata; + facebook::thrift::from_dynamic( + metadata, + json, + facebook::thrift::dynamic_format::PORTABLE, + facebook::thrift::format_adherence::LENIENT); requiredActions_ = *metadata.action(); commands_ = *metadata.commands(); base_ = *metadata.base(); @@ -758,9 +928,8 @@ ConfigSession::applyServiceActions( std::map> serviceNames; for (const auto& [service, level] : actions) { switch (level) { - case cli::ConfigActionLevel::AGENT_COLDBOOT: - case cli::ConfigActionLevel::AGENT_WARMBOOT: - case cli::ConfigActionLevel::BGP_RESTART: + case cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART: + case cli::ConfigActionLevel::SERVICE_RESTART: serviceNames[service] = fbossServiceUtil_->restartService(service, level); break; @@ -788,16 +957,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(SessionInit init) { @@ -806,15 +975,15 @@ void ConfigSession::initializeSession(SessionInit init) { // 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 (SERVICE_RESTART) 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(); if (init == SessionInit::ReadOnly) { return; // leave ~/.fboss2 alone @@ -891,9 +1060,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); } @@ -927,9 +1113,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."); @@ -954,205 +1137,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(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)); - } - } - } + ensureDirectoryExists(getCliConfigDir()); - // 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: {}", @@ -1165,35 +1249,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}; } @@ -1281,12 +1347,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(); } } @@ -1298,8 +1365,14 @@ ConfigSession::rolledBackActionLevels(const std::string& resolvedSha) const { return levels; } try { - const auto metadata = - parseMetadata(git_->fileAtRevision(commit.sha1, kMetadataGitRelPath)); + folly::dynamic json = folly::parseJson( + git_->fileAtRevision(commit.sha1, kMetadataGitRelPath)); + cli::ConfigSessionMetadata metadata; + facebook::thrift::from_dynamic( + metadata, + json, + facebook::thrift::dynamic_format::PORTABLE, + facebook::thrift::format_adherence::LENIENT); for (const auto& [service, level] : *metadata.action()) { auto it = levels.find(service); if (it == levels.end() || @@ -1340,36 +1413,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, kMetadataGitRelPath); - 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)) { @@ -1377,54 +1430,61 @@ 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; + // 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}); + } // Reload/restart only the services whose config changed. Each starts at its - // default rollback action level and is promoted to the highest level - // recorded by any commit being undone: undoing a change needs at least the - // action applying it did (e.g. a VLAN membership change cannot be applied - // with a hitless reload in either direction). Computed before any file is - // touched so a git failure here aborts cleanly. + // domain's default rollback action level and is promoted to the highest + // level recorded by any commit being undone: undoing a change needs at least + // the action applying it did (e.g. a VLAN membership change cannot be + // applied with a hitless reload in either direction). Computed before any + // file is touched so a git failure here aborts cleanly. auto recordedLevels = rolledBackActionLevels(resolvedSha); std::map actions; - auto actionFor = [&recordedLevels]( - cli::ServiceType service, cli::ConfigActionLevel floor) { - auto it = recordedLevels.find(service); + for (const auto& dr : doms) { + if (!dr.changed) { + continue; + } + auto level = dr.domain.rollbackActionLevel; + auto it = recordedLevels.find(dr.domain.service); if (it != recordedLevels.end() && - static_cast(it->second) > static_cast(floor)) { - return it->second; + static_cast(it->second) > static_cast(level)) { + level = it->second; } - return floor; - }; - if (agentChanged) { - actions[cli::ServiceType::AGENT] = - actionFor(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - } - if (bgpChanged) { - actions[cli::ServiceType::BGP] = - actionFor(cli::ServiceType::BGP, cli::ConfigActionLevel::BGP_RESTART); + actions[dr.domain.service] = level; } // The rollback commit's metadata must record the actions IT applied, not the // target commit's: a later rollback undoing this one crosses the same // changes and reads this action map to pick its own level. try { - auto metadata = parseMetadata(targetMetadataData); + folly::dynamic json = folly::parseJson(targetMetadataData); + cli::ConfigSessionMetadata metadata; + facebook::thrift::from_dynamic( + metadata, + json, + facebook::thrift::dynamic_format::PORTABLE, + facebook::thrift::format_adherence::LENIENT); metadata.action() = actions; targetMetadataData = folly::toPrettyJson( facebook::thrift::to_dynamic( @@ -1438,34 +1498,28 @@ std::string ConfigSession::rollback( ex.what())); } - // 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. + // 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())); } } @@ -1476,40 +1530,20 @@ std::string ConfigSession::rollback( try { 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) { @@ -1527,9 +1561,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) @@ -1542,23 +1577,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 458658b6fd8f9..a0b436ace2126 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" @@ -91,7 +95,7 @@ class ConfigSession { explicit ConfigSession(SessionInit init = SessionInit::CreateIfAbsent); - virtual ~ConfigSession() = default; + virtual ~ConfigSession(); // Get or create the current config session. // If no session exists, copies /etc/coop/agent.conf to ~/.fboss2/agent.conf, @@ -114,6 +118,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; @@ -137,6 +148,40 @@ 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 + // Minimum action used when a rollback changes this domain: HITLESS reloads + // the agent; SERVICE_RESTART restarts bgpd. rollback() promotes this to the + // highest level recorded by the commits being undone (see + // rolledBackActionLevels()). + 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. @@ -179,10 +224,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(); @@ -212,9 +257,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, SERVICE_RESTART): 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) @@ -230,9 +276,9 @@ class ConfigSession { const Git& getGit() const; // Update the required action level for the current session. - // Tracks the highest action level across all config commands. - // Higher action levels take precedence (AGENT_COLDBOOT > AGENT_WARMBOOT > - // HITLESS). + // Tracks the highest action level across all config commands, per service. + // Higher action levels take precedence (DISRUPTIVE_SERVICE_RESTART > + // SERVICE_RESTART > HITLESS). void updateRequiredAction( cli::ServiceType service, cli::ConfigActionLevel actionLevel); @@ -273,7 +319,7 @@ class ConfigSession { virtual std::string readCommandLineFromProc() const; // Apply actions (restart or reload) to all services based on their action - // levels. For WARMBOOT/COLDBOOT, restarts the service. For HITLESS, reloads + // levels. For the restart levels, restarts the service. For HITLESS, reloads // the config. // Returns a map of service type to list of actual systemd service names. std::map> applyServiceActions( @@ -292,15 +338,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; @@ -313,8 +359,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"; // git relative path of the CLI metadata tracked in the /etc/coop repo. static constexpr auto kMetadataGitRelPath = "cli/cli_metadata.json"; // Like Git::fileAtRevision but returns "" instead of throwing when the path @@ -328,7 +412,7 @@ class ConfigSession { // a rollback to resolvedSha would undo (i.e. commits in (resolvedSha, HEAD]). // Undoing a change needs at least the action level applying it did (e.g. a // VLAN membership change requires an agent warmboot in both directions), so - // rollback() promotes each service's default action to this level. + // rollback() promotes each domain's default action to this level. // If resolvedSha is not found in the metadata history, the max over the // whole history is returned (conservative). std::map rolledBackActionLevels( diff --git a/fboss/cli/fboss2/session/FbossServiceUtil.cpp b/fboss/cli/fboss2/session/FbossServiceUtil.cpp index 060d1a886bde4..7e69adf52d9d3 100644 --- a/fboss/cli/fboss2/session/FbossServiceUtil.cpp +++ b/fboss/cli/fboss2/session/FbossServiceUtil.cpp @@ -159,41 +159,45 @@ 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 (SERVICE_RESTART), so this path is never taken. throw std::runtime_error( "bgpd does not support config reload; it must be restarted"); } return reloadedServices; } -std::vector FbossServiceUtil::restartService( +std::string FbossServiceUtil::restartTypeName( cli::ServiceType service, cli::ConfigActionLevel level) { - std::string restartType; + // The action level is generic; what it means is decided per service. Only + // the agent distinguishes a warmboot from a coldboot -- bgpd has neither, so + // every restart level is a plain restart for it. switch (level) { - case cli::ConfigActionLevel::AGENT_COLDBOOT: - restartType = "coldboot"; - break; - case cli::ConfigActionLevel::AGENT_WARMBOOT: - restartType = "warmboot"; - break; - case cli::ConfigActionLevel::BGP_RESTART: - restartType = "restart"; - break; + case cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART: + return service == cli::ServiceType::AGENT ? "coldboot" : "restart"; + case cli::ConfigActionLevel::SERVICE_RESTART: + return service == cli::ServiceType::AGENT ? "warmboot" : "restart"; case cli::ConfigActionLevel::HITLESS: // Not expected: HITLESS is applied via reloadConfig(), not restart. - restartType = "reload"; - break; + return "reload"; } + return "restart"; +} + +std::vector FbossServiceUtil::restartService( + cli::ServiceType service, + cli::ConfigActionLevel level) { + const std::string restartType = restartTypeName(service, level); auto services = getServicesToRestart(service); LOG(INFO) << "Restarting " << getServiceName(service) << " (" << restartType << ")..."; - // Only AGENT_COLDBOOT needs the coldboot marker file; every other level is a - // plain restart-and-wait. - if (level == cli::ConfigActionLevel::AGENT_COLDBOOT) { + // Only an agent coldboot needs the coldboot marker files; every other + // (service, level) pair is the same plain restart-and-wait sequence. + if (service == cli::ServiceType::AGENT && + level == cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART) { performColdboot(services); } else { performWarmboot(services); diff --git a/fboss/cli/fboss2/session/FbossServiceUtil.h b/fboss/cli/fboss2/session/FbossServiceUtil.h index f2db1bacb9b0b..02d893bafa62a 100644 --- a/fboss/cli/fboss2/session/FbossServiceUtil.h +++ b/fboss/cli/fboss2/session/FbossServiceUtil.h @@ -62,6 +62,12 @@ class FbossServiceUtil { // Returns the systemd service name for a given service type. static std::string getServiceName(cli::ServiceType service); + // Human-readable restart kind for a (service, level) pair, e.g. the agent + // restarts by "warmboot" or "coldboot", bgpd (no warmboot) by "restart". + static std::string restartTypeName( + cli::ServiceType service, + cli::ConfigActionLevel level); + private: std::unique_ptr systemd_; std::vector switchIndexes_; diff --git a/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportTrunkAllowedVlanTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportTrunkAllowedVlanTest.cpp index a1bb68c97cf1a..a536e0c5f6ab4 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportTrunkAllowedVlanTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportTrunkAllowedVlanTest.cpp @@ -284,7 +284,7 @@ TEST_F( // VLAN changes require an agent warmboot, so the save must record it EXPECT_EQ( session.getRequiredAction(cli::ServiceType::AGENT), - cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ConfigActionLevel::SERVICE_RESTART); } TEST_F( @@ -434,7 +434,7 @@ TEST_F( EXPECT_THAT(result, HasSubstr("No changes made")); // A no-op must skip the save, so the required action stays at the - // default (HITLESS) instead of being escalated to AGENT_WARMBOOT. + // default (HITLESS) instead of being escalated to SERVICE_RESTART. auto& session = ConfigSession::getInstance(); EXPECT_EQ( session.getRequiredAction(cli::ServiceType::AGENT), diff --git a/fboss/cli/fboss2/test/config/CmdConfigInterfaceTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigInterfaceTest.cpp index 6f859fa4081d3..3bf5eaec93bab 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigInterfaceTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigInterfaceTest.cpp @@ -1911,9 +1911,9 @@ TEST_F( } } -// A subsuming profile change escalates the action level to AGENT_COLDBOOT so -// the CLI commits it as an agent coldboot instead of a hitless reloadConfig -// (T281221621). +// A subsuming profile change escalates the action level to +// DISRUPTIVE_SERVICE_RESTART so the CLI commits it as an agent coldboot instead +// of a hitless reloadConfig (T281221621). TEST_F(ApplyProfileSubsumeTestFixture, subsumeEscalatesToColdboot) { setupTestableConfigSession( "config interface", "eth1/1/1 profile PROFILE_400G_8_PAM4_RS544X2N"); @@ -1929,7 +1929,7 @@ TEST_F(ApplyProfileSubsumeTestFixture, subsumeEscalatesToColdboot) { "PROFILE_400G_8_PAM4_RS544X2N", &actionLevel); - EXPECT_EQ(actionLevel, cli::ConfigActionLevel::AGENT_COLDBOOT); + EXPECT_EQ(actionLevel, cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART); } // ============================================================================ diff --git a/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp index 49c981eaeaad6..ca33ed5694a11 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp @@ -629,14 +629,14 @@ TEST_F(ConfigSessionTestFixture, actionLevelUpdateAndGet) { TestableConfigSession session( sessionDir.string(), (getTestEtcDir() / "coop").string()); - // Update to AGENT_WARMBOOT + // Update to SERVICE_RESTART session.updateRequiredAction( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); // Verify the action level was updated EXPECT_EQ( session.getRequiredAction(cli::ServiceType::AGENT), - cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ConfigActionLevel::SERVICE_RESTART); } TEST_F(ConfigSessionTestFixture, actionLevelHigherTakesPrecedence) { @@ -646,18 +646,18 @@ TEST_F(ConfigSessionTestFixture, actionLevelHigherTakesPrecedence) { TestableConfigSession session( sessionDir.string(), (getTestEtcDir() / "coop").string()); - // Update to AGENT_WARMBOOT first + // Update to SERVICE_RESTART first session.updateRequiredAction( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); // Try to "downgrade" to HITLESS - should be ignored session.updateRequiredAction( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - // Verify action level remains at AGENT_WARMBOOT + // Verify action level remains at SERVICE_RESTART EXPECT_EQ( session.getRequiredAction(cli::ServiceType::AGENT), - cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ConfigActionLevel::SERVICE_RESTART); } TEST_F(ConfigSessionTestFixture, actionLevelReset) { @@ -667,9 +667,9 @@ TEST_F(ConfigSessionTestFixture, actionLevelReset) { TestableConfigSession session( sessionDir.string(), (getTestEtcDir() / "coop").string()); - // Set to AGENT_WARMBOOT + // Set to SERVICE_RESTART session.updateRequiredAction( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); // Reset the action level session.resetRequiredAction(cli::ServiceType::AGENT); @@ -692,7 +692,7 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsToMetadataFile) { // Load the config (required before saveConfig) session.getAgentConfig(); session.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); } // Verify metadata file exists and has correct JSON format @@ -705,7 +705,7 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsToMetadataFile) { EXPECT_TRUE(json.count("action")); EXPECT_TRUE(json["action"].isObject()); EXPECT_TRUE(json["action"].count("AGENT")); - EXPECT_EQ(json["action"]["AGENT"].asString(), "AGENT_WARMBOOT"); + EXPECT_EQ(json["action"]["AGENT"].asString(), "SERVICE_RESTART"); } TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { @@ -718,7 +718,7 @@ TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { fs::create_directories(sessionDir); std::ofstream metaFile(metadataFile); // Use symbolic enum names for human readability - metaFile << R"({"action":{"AGENT":"AGENT_WARMBOOT"}})"; + metaFile << R"({"action":{"AGENT":"SERVICE_RESTART"}})"; metaFile.close(); // Also create the session config file (otherwise session will overwrite from @@ -732,7 +732,7 @@ TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { // Verify action level was loaded EXPECT_EQ( session.getRequiredAction(cli::ServiceType::AGENT), - cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ConfigActionLevel::SERVICE_RESTART); } TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { @@ -746,7 +746,7 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { // Load the config (required before saveConfig) session1.getAgentConfig(); session1.saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); } // Second session: verify action level was persisted @@ -756,7 +756,7 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { EXPECT_EQ( session2.getRequiredAction(cli::ServiceType::AGENT), - cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ConfigActionLevel::SERVICE_RESTART); } } @@ -982,6 +982,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"; @@ -1114,8 +1170,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 @@ -1129,14 +1189,16 @@ TEST_F(ConfigSessionTestFixture, threeWayMergeScenarios) { session1.setCommandLine("config interface eth1/1/1 name port0_renamed"); session1.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - session1.commit(localhost()); + auto result1 = session1.commit(localhost()); + EXPECT_FALSE(result1.commitSha.empty()); (*session2.getAgentConfig().sw()->ports())[1].description() = "port1_desc"; session2.setCommandLine("config interface eth1/1/2 description port1_desc"); session2.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); EXPECT_NO_THROW(session2.rebase()); - session2.commit(localhost()); + auto result2 = session2.commit(localhost()); + EXPECT_FALSE(result2.commitSha.empty()); std::string content; EXPECT_TRUE(folly::readFile(cliConfigPath.c_str(), content)); @@ -1155,14 +1217,18 @@ TEST_F(ConfigSessionTestFixture, threeWayMergeScenarios) { session1.setCommandLine("config interface eth1/1/1 description same_value"); session1.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - session1.commit(localhost()); + auto result1 = session1.commit(localhost()); + EXPECT_FALSE(result1.commitSha.empty()); (*session2.getAgentConfig().sw()->ports())[0].description() = "same_value"; session2.setCommandLine("config interface eth1/1/1 description same_value"); session2.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); EXPECT_NO_THROW(session2.rebase()); - session2.commit(localhost()); + // Rebased session is identical to what session1 committed -> no-op commit + // (empty sha, no reload). + auto result2 = session2.commit(localhost()); + EXPECT_TRUE(result2.commitSha.empty()); std::string content; EXPECT_TRUE(folly::readFile(cliConfigPath.c_str(), content)); @@ -1181,7 +1247,8 @@ TEST_F(ConfigSessionTestFixture, threeWayMergeScenarios) { "config interface eth1/1/1 description user1_value"); session1.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - session1.commit(localhost()); + auto result1 = session1.commit(localhost()); + EXPECT_FALSE(result1.commitSha.empty()); (*session2.getAgentConfig().sw()->ports())[0].description() = "user2_value"; session2.setCommandLine( @@ -1263,6 +1330,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"; @@ -1353,9 +1462,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 (and no warmboot), so the recorded level is + // SERVICE_RESTART (a plain service restart). EXPECT_EQ( session.getRequiredAction(cli::ServiceType::BGP), - cli::ConfigActionLevel::BGP_RESTART); + cli::ConfigActionLevel::SERVICE_RESTART); } // A staged BGP edit persists to disk and is seeded back by a fresh session via @@ -1477,7 +1588,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"; @@ -1508,7 +1619,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"; } } @@ -1554,8 +1665,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"; @@ -1662,14 +1773,14 @@ TEST_F(ConfigSessionTestFixture, rollbackUsesRecordedActionLevel) { EXPECT_CALL( *mock, restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT)) + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART)) .Times(1); session->setCommandLine( "config interface eth1/1/1 switchport access vlan 3000"); (*session->getAgentConfig().sw()->ports())[0].description() = "Second version"; session->saveConfig( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); ASSERT_FALSE(session->commit(localhost()).commitSha.empty()); } @@ -1682,7 +1793,7 @@ TEST_F(ConfigSessionTestFixture, rollbackUsesRecordedActionLevel) { EXPECT_CALL( *mock, restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT)) + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART)) .Times(1); std::string rollbackSha = session->rollback(localhost(), firstCommitSha); EXPECT_FALSE(rollbackSha.empty()); @@ -1700,7 +1811,7 @@ TEST_F(ConfigSessionTestFixture, rollbackUsesRecordedActionLevel) { EXPECT_CALL( *mock, restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT)) + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART)) .Times(1); // No-arg rollback: back to the "Second version" commit. std::string rollbackSha = session->rollback(localhost()); diff --git a/fboss/cli/fboss2/test/config/ConfigSessionSystemdTest.cpp b/fboss/cli/fboss2/test/config/ConfigSessionSystemdTest.cpp index e52990bfd8ba2..ed2910871e34c 100644 --- a/fboss/cli/fboss2/test/config/ConfigSessionSystemdTest.cpp +++ b/fboss/cli/fboss2/test/config/ConfigSessionSystemdTest.cpp @@ -57,7 +57,7 @@ TEST(FbossServiceUtilTest, RestartService_MonolithicMode_Warmboot) { std::vector{}, /*multiSwitch=*/false, std::move(mockSystemd)); auto services = util.restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); EXPECT_EQ(services.size(), 1); EXPECT_EQ(services[0], "wedge_agent"); @@ -76,7 +76,8 @@ TEST(FbossServiceUtilTest, RestartService_MonolithicMode_Coldboot) { std::vector{}, /*multiSwitch=*/false, std::move(mockSystemd)); auto services = util.restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_COLDBOOT); + cli::ServiceType::AGENT, + cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART); EXPECT_EQ(services.size(), 1); EXPECT_EQ(services[0], "wedge_agent"); @@ -100,7 +101,7 @@ TEST(FbossServiceUtilTest, RestartService_SplitMode_Warmboot_SingleHwAgent) { std::vector{0}, /*multiSwitch=*/true, std::move(mockSystemd)); auto services = util.restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); EXPECT_EQ(services.size(), 2); EXPECT_EQ(services[0], "fboss_hw_agent@0"); @@ -126,7 +127,8 @@ TEST(FbossServiceUtilTest, RestartService_SplitMode_Coldboot_SingleHwAgent) { std::vector{0}, /*multiSwitch=*/true, std::move(mockSystemd)); auto services = util.restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_COLDBOOT); + cli::ServiceType::AGENT, + cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART); EXPECT_EQ(services.size(), 2); EXPECT_EQ(services[0], "fboss_hw_agent@0"); @@ -154,7 +156,7 @@ TEST(FbossServiceUtilTest, RestartService_SplitMode_Warmboot_MultipleHwAgents) { std::vector{0, 1}, /*multiSwitch=*/true, std::move(mockSystemd)); auto services = util.restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART); EXPECT_EQ(services.size(), 3); EXPECT_EQ(services[0], "fboss_hw_agent@0"); @@ -174,7 +176,7 @@ TEST(FbossServiceUtilTest, RestartService_PropagatesFailure) { EXPECT_THROW( util.restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT), + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART), std::runtime_error); } @@ -193,7 +195,7 @@ TEST(FbossServiceUtilTest, RestartService_ServiceFailsToStart) { EXPECT_THROW( util.restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT), + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART), std::runtime_error); } @@ -203,7 +205,7 @@ TEST(FbossServiceUtilTest, RestartService_ServiceFailsToStart) { // delegates to fbossServiceUtil_ without touching real systemd or thrift. // ============================================================ -// Test: applyServiceActions() delegates AGENT_WARMBOOT to +// Test: applyServiceActions() delegates SERVICE_RESTART to // FbossServiceUtil::restartService() TEST( ConfigSessionServiceTest, @@ -217,21 +219,21 @@ TEST( EXPECT_CALL( *mockPtr, restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT)) + cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART)) .WillOnce(::testing::Return(std::vector{"wedge_agent"})); TestableConfigSession session( "/tmp/test_session", "/tmp/test_system", std::move(mock)); std::map actions = { - {cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT}}; + {cli::ServiceType::AGENT, cli::ConfigActionLevel::SERVICE_RESTART}}; auto serviceNames = session.applyServiceActions(actions, hostInfo); ASSERT_EQ(serviceNames[cli::ServiceType::AGENT].size(), 1); EXPECT_EQ(serviceNames[cli::ServiceType::AGENT][0], "wedge_agent"); } -// Test: applyServiceActions() delegates AGENT_COLDBOOT to +// Test: applyServiceActions() delegates DISRUPTIVE_SERVICE_RESTART to // FbossServiceUtil::restartService() TEST( ConfigSessionServiceTest, @@ -245,7 +247,8 @@ TEST( EXPECT_CALL( *mockPtr, restartService( - cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_COLDBOOT)) + cli::ServiceType::AGENT, + cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART)) .WillOnce( ::testing::Return( std::vector{"fboss_hw_agent@0", "fboss_sw_agent"})); @@ -254,7 +257,8 @@ TEST( "/tmp/test_session", "/tmp/test_system", std::move(mock)); std::map actions = { - {cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_COLDBOOT}}; + {cli::ServiceType::AGENT, + cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART}}; auto serviceNames = session.applyServiceActions(actions, hostInfo); ASSERT_EQ(serviceNames[cli::ServiceType::AGENT].size(), 2); @@ -351,7 +355,8 @@ TEST( "localhost", "localhost-oob", folly::IPAddress("127.0.0.1")); std::map actions = { - {cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_COLDBOOT}}; + {cli::ServiceType::AGENT, + cli::ConfigActionLevel::DISRUPTIVE_SERVICE_RESTART}}; auto serviceNames = session.applyServiceActions(actions, hostInfo); diff --git a/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp index c0cb7b2a1543d..952c566675b38 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 = getInterfaceInfo(getRandomInterfacePortName()); + 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"; +} diff --git a/fboss/cli/fboss2/test/integration_test/ConfigQosDefaultPolicyTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigQosDefaultPolicyTest.cpp index 8f8aecc29eb43..6ac9d098e7b3e 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigQosDefaultPolicyTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigQosDefaultPolicyTest.cpp @@ -156,8 +156,8 @@ TEST_F(ConfigQosDefaultPolicyTest, SetAndDeleteDefaultPolicy) { EXPECT_THAT(delResult.stdout, ::testing::HasSubstr("Successfully removed")); commitConfig(); - // delete commits at AGENT_COLDBOOT; waitForRunningConfig tolerates the - // restart window (thrift errors count as condition-not-met). + // delete commits at DISRUPTIVE_SERVICE_RESTART; waitForRunningConfig + // tolerates the restart window (thrift errors count as condition-not-met). auto afterDelete = waitForRunningConfig( [](const folly::dynamic& config) { return !readDefaultPolicy(config).has_value();