hillol-nexthop · Jul 24, 2026 · Jul 31, 2026 · Aug 19, 2026 · Aug 19, 2026 · Sep 4, 2026
@@ -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

@@ -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<std::string>{});
   // 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(

@@ -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,

@@ -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) " +

@@ -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) {}",

@@ -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 '{}'",

@@ -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);
 }

@@ -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 {}",

@@ -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 {}",

@@ -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.";

@@ -16,6 +16,7 @@
 #include <folly/String.h>
 #include <vector>
 #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:

@@ -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<DiffDomain> 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<std::string, std::string> 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<std::pair<std::string, std::string>(
-              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<std::pair<ConfigDomain, std::string>> 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<std::pair<std::string, std::string>> 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
       }

@@ -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 {}. "