[Nexthop][fboss2-dev] Unify agent/BGP config session + decouple ConfigSession header

fboss/cli/fboss2/cli_metadata.thrift

@@ -17,9 +17,11 @@ namespace cpp2 facebook.fboss.cli
 // changes.
 enum ConfigActionLevel {
   HITLESS = 0, // Can be applied with reloadConfig() - default
-  AGENT_WARMBOOT = 1, // Requires agent warmboot restart
+  // Requires a service restart that preserves state where possible. For the
+  // agent this is a warmboot (forwarding state retained); for bgpd (BGP++),
+  // which has no hitless reload, it is a plain service restart.
+  AGENT_WARMBOOT = 1,
   AGENT_COLDBOOT = 2, // Requires agent coldboot restart (clears ASIC state)
-  BGP_RESTART = 3, // Requires a restart of the bgpd (BGP++) service
 }
 
 // Identifier for different services that can be configured

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

fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp

@@ -59,11 +59,6 @@ CmdConfigSessionCommitTraits::RetType CmdConfigSessionCommit::queryClient(
               fmt::format("{} (warmboot)", serviceName));
         }
         break;
-      case cli::ConfigActionLevel::BGP_RESTART:
-        for (const auto& serviceName : serviceNamesList) {
-          restartedServices.push_back(fmt::format("{} (restart)", serviceName));
-        }
-        break;
       case cli::ConfigActionLevel::HITLESS:
         for (const auto& serviceName : serviceNamesList) {
           reloadedServices.push_back(serviceName);

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<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);
@@ -184,35 +157,35 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient(
     const utils::RevisionList& revisions) {
   auto& session = ConfigSession::getInstance();
   auto& git = session.getGit();
-  auto domains = allDomains(session);
+  auto domains = session.configDomains();
+
+  // A git path present in every commit (the agent config), used to validate a
+  // revision in getRevisionContent(). configDomains() lists the agent first.
+  std::string validationPath = domains.front().gitRelPath;
 
   // Modes 1 and 2 both diff each staged domain's session file against some
   // "base" (current live config for mode 1; a revision for mode 2). The only
   // difference is how the base content+label is obtained, so share the loop.
   auto diffStagedDomains =
       [&](const std::function<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;
       };
@@ -222,7 +195,7 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient(
     if (!session.hasActiveSession()) {
       return "No config session exists. Make a config change first.";
     }
-    return diffStagedDomains([&](const DiffDomain& d) {
+    return diffStagedDomains([&](const ConfigDomain& d) {
       return std::make_pair(
           readFileOrEmpty(d.systemPath), std::string("current live config"));
     });
@@ -233,8 +206,8 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient(
     if (!session.hasActiveSession()) {
       return "No config session exists. Make a config change first.";
     }
-    return diffStagedDomains([&](const DiffDomain& d) {
-      return getRevisionContent(revisions[0], d, git);
+    return diffStagedDomains([&](const ConfigDomain& d) {
+      return getRevisionContent(revisions[0], d, validationPath, git);
     });
   }
 
@@ -245,8 +218,8 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient(
     // when more than one domain is shown.
     std::vector<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
       }