diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index d3f5360092927..a822d2e1faba3 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -766,6 +766,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp + fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.h + fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.cpp fboss/cli/fboss2/session/ConfigSession.h fboss/cli/fboss2/session/ConfigSession.cpp fboss/cli/fboss2/session/Git.h diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index adf6d463341cf..0b4d9a2c3338e 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -931,6 +931,7 @@ cpp_library( "commands/config/rollback/CmdConfigRollback.cpp", "commands/config/session/CmdConfigSessionCommit.cpp", "commands/config/session/CmdConfigSessionDiff.cpp", + "commands/config/session/CmdConfigSessionRebase.cpp", "session/ConfigSession.cpp", "session/Git.cpp", "utils/InterfaceList.cpp", @@ -992,6 +993,7 @@ cpp_library( "commands/config/rollback/CmdConfigRollback.h", "commands/config/session/CmdConfigSessionCommit.h", "commands/config/session/CmdConfigSessionDiff.h", + "commands/config/session/CmdConfigSessionRebase.h", "session/ConfigSession.h", "session/Git.h", "utils/InterfaceList.h", diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 2f4da898563f3..7b00d5ef920c0 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -12,6 +12,7 @@ // Current linter doesn't properly handle the template functions which need the // following headers +// NOLINTBEGIN(misc-include-cleaner) // @lint-ignore-every CLANGTIDY facebook-unused-include-check #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" @@ -85,6 +86,8 @@ #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" +#include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.h" +// NOLINTEND(misc-include-cleaner) namespace facebook::fboss { @@ -112,6 +115,8 @@ template void CmdHandler::run(); template void CmdHandler::run(); +template void +CmdHandler::run(); template void CmdHandler::run(); template void CmdHandler::run(); diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 8aaf235a631ce..7c2f4679da451 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -83,6 +83,7 @@ #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" +#include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.h" namespace facebook::fboss { @@ -637,6 +638,12 @@ const CommandTree& kConfigCommandTree() { "Show diff between configs (session vs live, session vs revision, or revision vs revision)", commandHandler, argTypeHandler, + }, + { + "rebase", + "Rebase session changes onto current HEAD", + commandHandler, + argTypeHandler, }}, }, diff --git a/fboss/cli/fboss2/cli_metadata.thrift b/fboss/cli/fboss2/cli_metadata.thrift index 214a9467a10e3..774c7dee5dc49 100644 --- a/fboss/cli/fboss2/cli_metadata.thrift +++ b/fboss/cli/fboss2/cli_metadata.thrift @@ -36,4 +36,7 @@ struct ConfigSessionMetadata { // List of CLI commands executed in this session, in chronological order. // Each entry is the full command string (e.g., "config interface eth1/1/1 mtu 9000"). 2: list commands; + // Git commit SHA that this session is based on. Used to detect if someone + // else committed changes while this session was in progress. + 3: string base; } diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.cpp b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.cpp new file mode 100644 index 0000000000000..3abb5de2da19b --- /dev/null +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.cpp @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.h" +#include +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +CmdConfigSessionRebaseTraits::RetType CmdConfigSessionRebase::queryClient( + const HostInfo& /* hostInfo */) { + auto& session = ConfigSession::getInstance(); + session.rebase(); // raises a runtime_error if we fail + return "Session successfully rebased onto current HEAD. You can now commit."; +} + +void CmdConfigSessionRebase::printOutput(const RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.h b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.h new file mode 100644 index 0000000000000..994eecb2a31ac --- /dev/null +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.h @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +struct CmdConfigSessionRebaseTraits : public WriteCommandTraits { + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE; + using ObjectArgType = std::monostate; // no arg + using RetType = std::string; +}; + +class CmdConfigSessionRebase + : public CmdHandler { + public: + using ObjectArgType = CmdConfigSessionRebaseTraits::ObjectArgType; + using RetType = CmdConfigSessionRebaseTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index 2b09b0fd44e26..00372121053e7 100644 --- a/fboss/cli/fboss2/session/ConfigSession.cpp +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -12,7 +12,9 @@ #include #include +#include #include +#include #include #include #include @@ -20,14 +22,31 @@ #include #include #include +#include #include +#include +#include +#include #include #include +#include +#include +#include #include +#include +#include #include +#include +#include #include "fboss/agent/AgentDirectoryUtil.h" +#include "fboss/agent/gen-cpp2/agent_config_types.h" +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/agent/if/gen-cpp2/FbossCtrl.h" +#include "fboss/agent/if/gen-cpp2/FbossCtrlAsyncClient.h" #include "fboss/cli/fboss2/gen-cpp2/cli_metadata_types.h" -#include "fboss/cli/fboss2/utils/CmdClientUtils.h" // NOLINT(misc-include-cleaner) +#include "fboss/cli/fboss2/session/Git.h" +#include "fboss/cli/fboss2/utils/CmdClientUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" #include "fboss/cli/fboss2/utils/PortMap.h" namespace fs = std::filesystem; @@ -119,6 +138,117 @@ void ensureDirectoryExists(const std::string& dirPath) { } } +// Maximum number of conflicts to report before truncating with "and more" +constexpr size_t kMaxConflicts = 10; + +// Add a conflict to the list, appending "and more" when we hit the limit. +void addConflict(std::vector& conflicts, std::string conflict) { + conflicts.push_back(std::move(conflict)); + if (conflicts.size() == kMaxConflicts - 1) { + conflicts.emplace_back("and more"); + } +} + +/* + * Perform a recursive 3-way merge of JSON objects. + * + * @param base The original/base version + * @param head The version that was changed by someone else (current HEAD) + * @param session The version with the user's changes + * @param path Current path in the JSON tree (for conflict reporting) + * @param conflicts Vector to collect conflict paths (capped at kMaxConflicts) + * @return The merged JSON, preferring session changes over head when safe + * (the return value must be ignored when "conflicts" is not empty) + */ +folly::dynamic threeWayMerge( + const folly::dynamic& base, + const folly::dynamic& head, + const folly::dynamic& session, + const std::string& path, + std::vector& conflicts) { + // If we've already hit max conflicts, stop recursing + if (conflicts.size() >= kMaxConflicts) { + return session; + } + + // Note: folly::dynamic::operator== does deep comparison which is O(n) for the + // entire subtree. We compare subtrees O(n) times leading to O(n²) complexity. + // While suboptimal, benchmarking showed ~11ms for a 41k line config file, + // which is acceptable for CLI usage, given how simple the implementation is. + + // If session equals base, user didn't change this - use head's version + if (session == base) { + return head; + } + + // If head equals base, other user didn't change this - use session's version + if (head == base) { + return session; + } + + // Both changed - if they made the same change, that's fine + if (head == session) { + return session; + } + + // Both changed differently - need to handle based on type + if (base.isObject() && head.isObject() && session.isObject()) { + // Recursively merge objects + folly::dynamic result = folly::dynamic::object; + + // Collect all keys from all three versions + std::set allKeys; + for (const auto& kv : base.items()) { + allKeys.insert(kv.first.asString()); + } + for (const auto& kv : head.items()) { + allKeys.insert(kv.first.asString()); + } + for (const auto& kv : session.items()) { + allKeys.insert(kv.first.asString()); + } + + for (const auto& key : allKeys) { + std::string childPath = + path.empty() ? key : fmt::format("{}.{}", path, key); + + // Get values from each version (null if not present) + folly::dynamic baseVal = base.getDefault(key, nullptr); + folly::dynamic headVal = head.getDefault(key, nullptr); + folly::dynamic sessionVal = session.getDefault(key, nullptr); + + folly::dynamic mergedVal = + threeWayMerge(baseVal, headVal, sessionVal, childPath, conflicts); + + // Don't include null values (represents deletion) + if (!mergedVal.isNull()) { + result[key] = std::move(mergedVal); + } + } + return result; + } + + if (base.isArray() && head.isArray() && session.isArray()) { + // For arrays, we can try element-by-element merge if sizes match + if (base.size() == head.size() && base.size() == session.size()) { + folly::dynamic result = folly::dynamic::array; + for (size_t i = 0; i < base.size(); ++i) { + std::string childPath = fmt::format("{}[{}]", path, i); + result.push_back( + threeWayMerge(base[i], head[i], session[i], childPath, conflicts)); + } + return result; + } + // Array sizes differ - this is a conflict + addConflict(conflicts, path + " (array size mismatch)"); + return session; // Return session's version, but report conflict + } + + // Scalar values that both changed differently - conflict + addConflict(conflicts, path); + return session; // Return session's version, but report conflict +} + } // anonymous namespace /* @@ -150,9 +280,10 @@ std::string ConfigSession::readCommandLineFromProc() const { return folly::join(" ", args); } -ConfigSession::ConfigSession() - : sessionConfigDir_(getHomeDirectory() + "/.fboss2"), - username_(getUsername()) { +ConfigSession::ConfigSession() { + username_ = getUsername(); + std::string homeDir = getHomeDirectory(); + // Use AgentDirectoryUtil to get the config directory path // getConfigDirectory() returns /etc/coop/agent, so we get the parent to get // /etc/coop @@ -160,6 +291,7 @@ ConfigSession::ConfigSession() std::string coopDir = fs::path(dirUtil.getConfigDirectory()).parent_path().string(); + sessionConfigDir_ = homeDir + "/.fboss2"; systemConfigDir_ = coopDir; git_ = std::make_unique(coopDir); initializeSession(); @@ -272,9 +404,10 @@ void ConfigSession::saveConfig( // 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(); - CHECK(!rawCmd.empty()) - << "saveConfig() called with no command line arguments"; // 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 "); @@ -292,6 +425,10 @@ void ConfigSession::saveConfig( saveMetadata(); } +void ConfigSession::saveConfig() { + saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); +} + Git& ConfigSession::getGit() { return *git_; } @@ -347,6 +484,7 @@ void ConfigSession::loadMetadata() { facebook::thrift::format_adherence::LENIENT); requiredActions_ = *metadata.action(); commands_ = *metadata.commands(); + base_ = *metadata.base(); } catch (const std::exception& ex) { // If JSON parsing fails, keep defaults LOG(WARNING) << "Failed to parse metadata file: " << ex.what(); @@ -361,6 +499,7 @@ void ConfigSession::saveMetadata() { cli::ConfigSessionMetadata metadata; metadata.action() = requiredActions_; metadata.commands() = commands_; + metadata.base() = base_; folly::dynamic json = facebook::thrift::to_dynamic( metadata, facebook::thrift::dynamic_format::PORTABLE); @@ -453,12 +592,45 @@ void ConfigSession::restartService( fs::path filePath(file); std::error_code ec; fs::create_directories(filePath.parent_path(), ec); + if (ec) { + throw std::runtime_error( + fmt::format( + "Failed to create directory for coldboot file {}: {}", + file, + ec.message())); + } // Create the file (touch equivalent) std::ofstream touchFile(file); - touchFile.close(); + if (!touchFile.good()) { + // If we failed due to permissions, try using sudo touch + int savedErrno = errno; + if (savedErrno == EACCES || savedErrno == EPERM) { + try { + folly::Subprocess touchProc( + {"/usr/bin/sudo", "/usr/bin/touch", file}); + touchProc.waitChecked(); + } catch (const std::exception& ex) { + throw std::runtime_error( + fmt::format( + "Failed to create coldboot file {} (permission denied, sudo touch also failed): {}", + file, + ex.what())); + } + } else { + throw std::runtime_error( + fmt::format( + "Failed to create coldboot file {}: {}", + file, + folly::errnoStr(savedErrno))); + } + } else { + touchFile.close(); + } if (!fs::exists(file)) { throw std::runtime_error( - fmt::format("Failed to create coldboot file: {}", file)); + fmt::format( + "Failed to create coldboot file {}: file does not exist after creation", + file)); } } @@ -542,6 +714,12 @@ void ConfigSession::applyServiceActions( } void ConfigSession::loadConfig() { + // If session file doesn't exist (e.g., after a commit), re-initialize + // the session by copying from system config. + if (!sessionExists()) { + initializeSession(); + } + std::string configJson; std::string sessionConfigPath = getSessionConfigPath(); if (!folly::readFile(sessionConfigPath.c_str(), configJson)) { @@ -564,10 +742,21 @@ void ConfigSession::loadConfig() { void ConfigSession::initializeSession() { initializeGit(); if (!sessionExists()) { + // 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; + // Ensure the session config directory exists ensureDirectoryExists(sessionConfigDir_); copySystemConfigToSession(); - // Create initial empty metadata file for new sessions + // Capture the current git HEAD as the base for this session. + // This is used to detect if someone else committed changes while this + // session was in progress. + base_ = git_->getHead(); + // Create initial metadata file for new sessions saveMetadata(); } else { // Load metadata from disk (survives across CLI invocations) @@ -592,7 +781,7 @@ void ConfigSession::initializeGit() { } } -void ConfigSession::copySystemConfigToSession() { +void ConfigSession::copySystemConfigToSession() const { // Read system config and write atomically to session config // This ensures that readers never see a partially written file - they either // see the old file or the new file, never a mix. @@ -614,6 +803,20 @@ ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { "No config session exists. Make a config change first."); } + // Check if someone else committed changes while this session was in progress + std::string currentHead = git_->getHead(); + if (!base_.empty() && currentHead != base_) { + throw std::runtime_error( + fmt::format( + "Cannot commit: the system configuration has changed since this " + "session was started. Your session was based on commit {}, but the " + "current HEAD is {}. Run 'config session rebase' to rebase your " + "changes onto the current configuration, or discard your session " + "and start over.", + base_.substr(0, 7), + currentHead.substr(0, 7))); + } + std::string cliConfigDir = getCliConfigDir(); std::string cliConfigPath = getCliConfigPath(); std::string sessionConfigPath = getSessionConfigPath(); @@ -727,10 +930,79 @@ ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { for (const auto& [service, level] : actions) { resetRequiredAction(service); } + base_ = commitSha; + // Force config reload from system config on next access + configLoaded_ = false; return CommitResult{commitSha, actions}; } +void ConfigSession::rebase() { + if (!sessionExists()) { + throw std::runtime_error( + "No config session exists. Make a config change first."); + } + + std::string currentHead = git_->getHead(); + + // If base is empty or already matches HEAD, nothing to rebase + if (base_.empty() || base_ == currentHead) { + throw std::runtime_error( + "No rebase needed: session is already based on the current HEAD."); + } + + // Get the three versions of the config: + // 1. Base config (what the session was originally based on) + // 2. Current HEAD config (what someone else committed) + // 3. Session config (user's changes) + std::string cliConfigRelPath = "cli/agent.conf"; + std::string baseConfig = git_->fileAtRevision(base_, cliConfigRelPath); + std::string headConfig = git_->fileAtRevision(currentHead, cliConfigRelPath); + + std::string sessionConfigPath = getSessionConfigPath(); + std::string sessionConfig; + if (!folly::readFile(sessionConfigPath.c_str(), sessionConfig)) { + throw std::runtime_error( + fmt::format( + "Failed to read session config from {}", sessionConfigPath)); + } + + // Parse all three as JSON + folly::dynamic baseJson = folly::parseJson(baseConfig); + folly::dynamic headJson = folly::parseJson(headConfig); + folly::dynamic sessionJson = folly::parseJson(sessionConfig); + + // Perform a 3-way merge + // For each key in session that differs from base, apply to head + // If head also changed the same key differently, that's a conflict + std::vector conflicts; + folly::dynamic mergedJson = + threeWayMerge(baseJson, headJson, sessionJson, "", conflicts); + + if (!conflicts.empty()) { + std::string conflictList; + for (const auto& conflict : conflicts) { + conflictList += "\n - " + conflict; + } + throw std::runtime_error( + fmt::format( + "Rebase failed due to conflicts at the following paths:{}", + conflictList)); + } + + // Write the merged config to the session file + std::string mergedConfigStr = folly::toPrettyJson(mergedJson); + folly::writeFileAtomic( + sessionConfigPath, mergedConfigStr, 0644, folly::SyncType::WITH_SYNC); + + // Update the base to current HEAD + base_ = currentHead; + saveMetadata(); + + // Reload the config into memory + loadConfig(); +} + std::string ConfigSession::rollback(const HostInfo& hostInfo) { // Get the commit history to find the previous commit auto commits = git_->log(getCliConfigPath(), 2); diff --git a/fboss/cli/fboss2/session/ConfigSession.h b/fboss/cli/fboss2/session/ConfigSession.h index f8036647cf851..302205f71ef2b 100644 --- a/fboss/cli/fboss2/session/ConfigSession.h +++ b/fboss/cli/fboss2/session/ConfigSession.h @@ -11,8 +11,8 @@ #include #include -#include #include +#include #include "fboss/agent/gen-cpp2/agent_config_types.h" #include "fboss/cli/fboss2/gen-cpp2/cli_metadata_types.h" #include "fboss/cli/fboss2/session/Git.h" @@ -116,6 +116,13 @@ class ConfigSession { // Returns CommitResult with git commit SHA and action level. CommitResult commit(const HostInfo& hostInfo); + // Rebase the session onto the current HEAD. + // This is needed when someone else has committed changes while this session + // was in progress. It computes the diff between the base config and the + // session config, then applies that diff on top of the current HEAD. + // Throws std::runtime_error if there are conflicts that cannot be resolved. + void rebase(); + // Rollback to a specific revision (git commit SHA) or to the previous // revision Returns the git commit SHA of the new commit created for the // rollback @@ -138,6 +145,8 @@ class ConfigSession { // (if the new level is higher than the current one). // This combines saving the config and updating its associated metadata. void saveConfig(cli::ServiceType service, cli::ConfigActionLevel actionLevel); + // Save the configuration for AGENT service with HITLESS action level. + void saveConfig(); // Get the Git instance for this config session // Used to access the Git repository for history, rollback, etc. @@ -199,6 +208,11 @@ class ConfigSession { // List of commands executed in this session, persisted to disk std::vector commands_; + // Git commit SHA that this session is based on (captured when session is + // created). Used to detect if someone else committed changes while this + // session was in progress. + std::string base_; + // Path to the system metadata file (in the Git repo) std::string getSystemMetadataPath() const; @@ -227,7 +241,7 @@ class ConfigSession { // Initialize the session (creates session config file if it doesn't exist) void initializeSession(); - void copySystemConfigToSession(); + void copySystemConfigToSession() const; void loadConfig(); // Initialize the Git repository if needed diff --git a/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp index bb3fc156eb0d7..9f690bbe1fb14 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp @@ -1,72 +1,33 @@ -/* - * Copyright (c) 2004-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - * - */ - -#include +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include "fboss/cli/fboss2/gen-cpp2/cli_metadata_types.h" +#include "fboss/cli/fboss2/session/Git.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" + +#include #include #include #include #include #include #include -#include -#include #include -#include +#include + +#include -#include "fboss/cli/fboss2/gen-cpp2/cli_metadata_types.h" -#include "fboss/cli/fboss2/session/ConfigSession.h" -#include "fboss/cli/fboss2/session/Git.h" -#include "fboss/cli/fboss2/test/CmdHandlerTestBase.h" #include "fboss/cli/fboss2/test/TestableConfigSession.h" namespace fs = std::filesystem; namespace facebook::fboss { -class ConfigSessionTestFixture : public CmdHandlerTestBase { +class ConfigSessionTestFixture : public CmdConfigTestBase { public: - void SetUp() override { - CmdHandlerTestBase::SetUp(); - - // Create unique test directories for each test to avoid conflicts when - // running tests in parallel - auto tempBase = fs::temp_directory_path(); - auto uniquePath = - boost::filesystem::unique_path("fboss_test_%%%%-%%%%-%%%%-%%%%"); - testHomeDir_ = tempBase / (uniquePath.string() + "_home"); - testEtcDir_ = tempBase / (uniquePath.string() + "_etc"); - - // Clean up any previous test artifacts (shouldn't exist with unique names) - std::error_code ec; - if (fs::exists(testHomeDir_)) { - fs::remove_all(testHomeDir_, ec); - } - if (fs::exists(testEtcDir_)) { - fs::remove_all(testEtcDir_, ec); - } - - // Create test directories - // Structure: systemConfigDir_ = /etc/coop (git repo root) - // - agent.conf (symlink -> cli/agent.conf) - // - cli/agent.conf (actual config file) - fs::create_directories(testHomeDir_); - systemConfigDir_ = testEtcDir_ / "coop"; - fs::create_directories(systemConfigDir_ / "cli"); - - // Set environment variables - setenv("HOME", testHomeDir_.c_str(), 1); - setenv("USER", "testuser", 1); - - // Create the actual config file at cli/agent.conf - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; - createTestConfig(cliConfigPath, R"({ + ConfigSessionTestFixture() + : CmdConfigTestBase( + "fboss_test_%%%%-%%%%-%%%%-%%%%", + R"({ "sw": { "ports": [ { @@ -74,70 +35,28 @@ class ConfigSessionTestFixture : public CmdHandlerTestBase { "name": "eth1/1/1", "state": 2, "speed": 100000 + }, + { + "logicalID": 2, + "name": "eth1/1/2", + "state": 2, + "speed": 100000 } ] } -})"); - - // Create symlink at /etc/coop/agent.conf -> cli/agent.conf - fs::create_symlink("cli/agent.conf", systemConfigDir_ / "agent.conf"); - - // Initialize Git repository and create initial commit - Git git(systemConfigDir_.string()); - git.init(); - git.commit({cliConfigPath.string()}, "Initial commit"); - } - - void TearDown() override { - // Clean up test directories - // Use error_code to avoid throwing exceptions in TearDown - std::error_code ec; - if (fs::exists(testHomeDir_)) { - fs::remove_all(testHomeDir_, ec); - if (ec) { - std::cerr << "Warning: Failed to remove " << testHomeDir_ << ": " - << ec.message() << std::endl; - } - } - if (fs::exists(testEtcDir_)) { - fs::remove_all(testEtcDir_, ec); - if (ec) { - std::cerr << "Warning: Failed to remove " << testEtcDir_ << ": " - << ec.message() << std::endl; - } - } - - CmdHandlerTestBase::TearDown(); - } - - protected: - void createTestConfig(const fs::path& path, const std::string& content) { - std::ofstream file(path); - file << content; - file.close(); - } - - std::string readFile(const fs::path& path) { - std::ifstream file(path); - std::stringstream buffer; - buffer << file.rdbuf(); - return buffer.str(); - } - - fs::path testHomeDir_; - fs::path testEtcDir_; - fs::path systemConfigDir_; // /etc/coop (git repo root) +})") {} }; TEST_F(ConfigSessionTestFixture, sessionInitialization) { // Initially, session directory should not exist - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; EXPECT_FALSE(fs::exists(sessionDir)); // Creating a ConfigSession should create the directory and copy the config - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Verify the directory was created EXPECT_TRUE(fs::exists(sessionDir)); @@ -151,12 +70,13 @@ TEST_F(ConfigSessionTestFixture, sessionInitialization) { } TEST_F(ConfigSessionTestFixture, sessionConfigModified) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; // Create a ConfigSession - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Modify the session config through the ConfigSession API auto& config = session.getAgentConfig(); @@ -174,9 +94,9 @@ TEST_F(ConfigSessionTestFixture, sessionConfigModified) { } TEST_F(ConfigSessionTestFixture, sessionCommit) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; // Setup mock agent server setupMockedAgentServer(); @@ -188,7 +108,7 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { // First commit: Create a ConfigSession and commit a change { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Simulate a CLI command being tracked session.setCommandLine( @@ -199,6 +119,8 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { auto& ports = *config.sw()->ports(); ASSERT_FALSE(ports.empty()); ports[0].description() = "First commit"; + session.setCommandLine( + "config interface eth1/1/1 description First commit"); session.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); @@ -214,7 +136,8 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { firstCommitSha = result.commitSha; // Verify metadata file was created alongside the config revision - fs::path targetMetadata = systemConfigDir_ / "cli" / "cli_metadata.json"; + fs::path targetMetadata = + getTestEtcDir() / "coop" / "cli" / "cli_metadata.json"; EXPECT_TRUE(fs::exists(targetMetadata)); // Verify system config was updated @@ -224,7 +147,7 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { // Second commit: Create a new session and verify it's based on first commit { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Simulate a CLI command being tracked session.setCommandLine( @@ -238,6 +161,8 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { // Make another change to the same port ports[0].description() = "Second commit"; + session.setCommandLine( + "config interface eth1/1/1 description Second commit"); session.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); @@ -250,7 +175,8 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { secondCommitSha = result.commitSha; // Verify metadata file was created alongside the config revision - fs::path targetMetadata = systemConfigDir_ / "cli" / "cli_metadata.json"; + fs::path targetMetadata = + getTestEtcDir() / "coop" / "cli" / "cli_metadata.json"; EXPECT_TRUE(fs::exists(targetMetadata)); // Verify system config was updated @@ -270,8 +196,8 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { // Ensure commit() works on a newly initialized session // This verifies that initializeSession() creates the metadata file TEST_F(ConfigSessionTestFixture, commitOnNewlyInitializedSession) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; - fs::path cliConfigDir = systemConfigDir_ / "cli"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; + fs::path cliConfigDir = getTestEtcDir() / "coop" / "cli"; // Setup mock agent server setupMockedAgentServer(); @@ -279,7 +205,8 @@ TEST_F(ConfigSessionTestFixture, commitOnNewlyInitializedSession) { // Create a new session // This tests that metadata file is created during session initialization - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Verify metadata file was created during session initialization fs::path metadataPath = sessionDir / "cli_metadata.json"; @@ -290,8 +217,6 @@ TEST_F(ConfigSessionTestFixture, commitOnNewlyInitializedSession) { auto& ports = *config.sw()->ports(); ASSERT_FALSE(ports.empty()); ports[0].description() = "Test change for commit"; - session.setCommandLine( - "config interface eth1/1/1 description Test change for commit"); session.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); // Commit should succeed @@ -304,11 +229,12 @@ TEST_F(ConfigSessionTestFixture, commitOnNewlyInitializedSession) { } TEST_F(ConfigSessionTestFixture, multipleChangesInOneSession) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; // Create a ConfigSession - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Make first change auto& config = session.getAgentConfig(); @@ -333,13 +259,13 @@ TEST_F(ConfigSessionTestFixture, multipleChangesInOneSession) { } TEST_F(ConfigSessionTestFixture, sessionPersistsAcrossCommands) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; // Create first ConfigSession and modify config { TestableConfigSession session1( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); auto& config = session1.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -360,7 +286,7 @@ TEST_F(ConfigSessionTestFixture, sessionPersistsAcrossCommands) { // session { TestableConfigSession session2( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); auto& config = session2.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -371,21 +297,23 @@ TEST_F(ConfigSessionTestFixture, sessionPersistsAcrossCommands) { } TEST_F(ConfigSessionTestFixture, configRollbackOnFailure) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; // Save the original config content std::string originalContent = readFile(cliConfigPath); - // Setup mock agent server to fail reloadConfig + // Setup mock agent server to fail reloadConfig on first call (the commit), + // but succeed on second call (the rollback reload) setupMockedAgentServer(); EXPECT_CALL(getMockAgent(), reloadConfig()) .WillOnce(::testing::Throw(std::runtime_error("Reload failed"))) .WillOnce(::testing::Return()); // Create a ConfigSession and try to commit - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); auto& config = session.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -406,7 +334,7 @@ TEST_F(ConfigSessionTestFixture, configRollbackOnFailure) { } TEST_F(ConfigSessionTestFixture, concurrentCommits) { - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; // Setup mock agent server setupMockedAgentServer(); @@ -420,17 +348,15 @@ TEST_F(ConfigSessionTestFixture, concurrentCommits) { // First commit { - fs::path sessionDir = testHomeDir_ / ".fboss2_user1"; + fs::path sessionDir = getTestHomeDir() / ".fboss2_user1"; TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); auto& config = session.getAgentConfig(); auto& ports = *config.sw()->ports(); ASSERT_FALSE(ports.empty()); ports[0].description() = "First commit"; - session.setCommandLine( - "config interface eth1/1/1 description First commit"); session.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); @@ -440,17 +366,15 @@ TEST_F(ConfigSessionTestFixture, concurrentCommits) { // Second commit { - fs::path sessionDir = testHomeDir_ / ".fboss2_user2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2_user2"; TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); auto& config = session.getAgentConfig(); auto& ports = *config.sw()->ports(); ASSERT_FALSE(ports.empty()); ports[0].description() = "Second commit"; - session.setCommandLine( - "config interface eth1/1/1 description Second commit"); session.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); @@ -464,16 +388,17 @@ TEST_F(ConfigSessionTestFixture, concurrentCommits) { EXPECT_NE(commitSha1, commitSha2); // Verify Git history contains both commits - Git git(systemConfigDir_.string()); + Git git((getTestEtcDir() / "coop").string()); auto commits = git.log(cliConfigPath.string()); EXPECT_GE(commits.size(), 3); // Initial + 2 commits } TEST_F(ConfigSessionTestFixture, rollbackToSpecificCommit) { // This test calls the rollback() method with a specific commit SHA - fs::path sessionDir = testHomeDir_ / ".fboss2"; - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; - fs::path metadataPath = systemConfigDir_ / "cli" / "cli_metadata.json"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; + fs::path metadataPath = + getTestEtcDir() / "coop" / "cli" / "cli_metadata.json"; // Setup mock agent server setupMockedAgentServer(); @@ -485,7 +410,7 @@ TEST_F(ConfigSessionTestFixture, rollbackToSpecificCommit) { std::string secondCommitSha; { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Simulate CLI command for first commit session.setCommandLine( @@ -503,7 +428,7 @@ TEST_F(ConfigSessionTestFixture, rollbackToSpecificCommit) { } { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Simulate CLI command for second commit session.setCommandLine( @@ -526,7 +451,7 @@ TEST_F(ConfigSessionTestFixture, rollbackToSpecificCommit) { // Now rollback to first commit { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); std::string rollbackSha = session.rollback(localhost(), firstCommitSha); @@ -559,8 +484,8 @@ TEST_F(ConfigSessionTestFixture, rollbackToSpecificCommit) { TEST_F(ConfigSessionTestFixture, rollbackToPreviousCommit) { // This test calls the rollback() method without a commit SHA argument // to rollback to the previous commit - fs::path sessionDir = testHomeDir_ / ".fboss2"; - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; // Setup mock agent server setupMockedAgentServer(); @@ -570,24 +495,20 @@ TEST_F(ConfigSessionTestFixture, rollbackToPreviousCommit) { // Create commits to build history { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); auto& config1 = session.getAgentConfig(); (*config1.sw()->ports())[0].description() = "First version"; - session.setCommandLine( - "config interface eth1/1/1 description First version"); session.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); session.commit(localhost()); } { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); auto& config2 = session.getAgentConfig(); (*config2.sw()->ports())[0].description() = "Second version"; - session.setCommandLine( - "config interface eth1/1/1 description Second version"); session.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); session.commit(localhost()); @@ -599,7 +520,7 @@ TEST_F(ConfigSessionTestFixture, rollbackToPreviousCommit) { // Rollback to previous commit (no argument) { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); std::string rollbackSha = session.rollback(localhost()); @@ -612,10 +533,11 @@ TEST_F(ConfigSessionTestFixture, rollbackToPreviousCommit) { } TEST_F(ConfigSessionTestFixture, actionLevelDefaultIsHitless) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; // Create a ConfigSession - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Default action level should be HITLESS EXPECT_EQ( @@ -624,12 +546,13 @@ TEST_F(ConfigSessionTestFixture, actionLevelDefaultIsHitless) { } TEST_F(ConfigSessionTestFixture, actionLevelUpdateAndGet) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; // Create a ConfigSession - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); - // Update to AGENT_RESTART + // Update to AGENT_WARMBOOT session.updateRequiredAction( cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); @@ -640,12 +563,13 @@ TEST_F(ConfigSessionTestFixture, actionLevelUpdateAndGet) { } TEST_F(ConfigSessionTestFixture, actionLevelHigherTakesPrecedence) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; // Create a ConfigSession - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); - // Update to AGENT_RESTART first + // Update to AGENT_WARMBOOT first session.updateRequiredAction( cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); @@ -653,19 +577,20 @@ TEST_F(ConfigSessionTestFixture, actionLevelHigherTakesPrecedence) { session.updateRequiredAction( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - // Verify action level remains at AGENT_RESTART + // Verify action level remains at AGENT_WARMBOOT EXPECT_EQ( session.getRequiredAction(cli::ServiceType::AGENT), cli::ConfigActionLevel::AGENT_WARMBOOT); } TEST_F(ConfigSessionTestFixture, actionLevelReset) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; // Create a ConfigSession - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); - // Set to AGENT_RESTART + // Set to AGENT_WARMBOOT session.updateRequiredAction( cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); @@ -679,17 +604,16 @@ TEST_F(ConfigSessionTestFixture, actionLevelReset) { } TEST_F(ConfigSessionTestFixture, actionLevelPersistsToMetadataFile) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path metadataFile = sessionDir / "cli_metadata.json"; // Create a ConfigSession and set action level via saveConfig { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Load the config (required before saveConfig) session.getAgentConfig(); - session.setCommandLine("config interface eth1/1/1 description Test"); session.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); } @@ -708,10 +632,10 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsToMetadataFile) { } TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; fs::path metadataFile = sessionDir / "cli_metadata.json"; - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; // Create session directory and metadata file manually fs::create_directories(sessionDir); @@ -725,7 +649,8 @@ TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { fs::copy_file(cliConfigPath, sessionConfig); // Create a ConfigSession - should load action level from metadata file - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Verify action level was loaded EXPECT_EQ( @@ -734,16 +659,15 @@ TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { } TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; // First session: set action level via saveConfig { TestableConfigSession session1( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Load the config (required before saveConfig) session1.getAgentConfig(); - session1.setCommandLine("config interface eth1/1/1 description Test"); session1.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); } @@ -751,7 +675,7 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { // Second session: verify action level was persisted { TestableConfigSession session2( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); EXPECT_EQ( session2.getRequiredAction(cli::ServiceType::AGENT), @@ -760,13 +684,13 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { } TEST_F(ConfigSessionTestFixture, commandTrackingBasic) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path metadataFile = sessionDir / "cli_metadata.json"; // Create a ConfigSession, execute command, and verify persistence { TestableConfigSession session( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Initially, no commands should be recorded EXPECT_TRUE(session.getCommands().empty()); @@ -803,10 +727,11 @@ TEST_F(ConfigSessionTestFixture, commandTrackingBasic) { } TEST_F(ConfigSessionTestFixture, commandTrackingMultipleCommands) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; // Create a ConfigSession - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Execute multiple commands auto& config = session.getAgentConfig(); @@ -834,12 +759,12 @@ TEST_F(ConfigSessionTestFixture, commandTrackingMultipleCommands) { } TEST_F(ConfigSessionTestFixture, commandTrackingPersistsAcrossSessions) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; // First session: execute some commands { TestableConfigSession session1( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); auto& config = session1.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -859,7 +784,7 @@ TEST_F(ConfigSessionTestFixture, commandTrackingPersistsAcrossSessions) { // Second session: verify commands were persisted { TestableConfigSession session2( - sessionDir.string(), systemConfigDir_.string()); + sessionDir.string(), (getTestEtcDir() / "coop").string()); EXPECT_EQ(2, session2.getCommands().size()); EXPECT_EQ("config interface eth1/1/1 mtu 9000", session2.getCommands()[0]); @@ -870,10 +795,11 @@ TEST_F(ConfigSessionTestFixture, commandTrackingPersistsAcrossSessions) { } TEST_F(ConfigSessionTestFixture, commandTrackingClearedOnReset) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; // Create a ConfigSession and add some commands - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); auto& config = session.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -893,10 +819,10 @@ TEST_F(ConfigSessionTestFixture, commandTrackingClearedOnReset) { } TEST_F(ConfigSessionTestFixture, commandTrackingLoadsFromMetadataFile) { - fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionDir = getTestHomeDir() / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; fs::path metadataFile = sessionDir / "cli_metadata.json"; - fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; // Create session directory and metadata file manually fs::create_directories(sessionDir); @@ -911,7 +837,8 @@ TEST_F(ConfigSessionTestFixture, commandTrackingLoadsFromMetadataFile) { fs::copy_file(cliConfigPath, sessionConfig); // Create a ConfigSession - should load commands from metadata file - TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); // Verify commands were loaded EXPECT_EQ(3, session.getCommands().size()); @@ -920,4 +847,267 @@ TEST_F(ConfigSessionTestFixture, commandTrackingLoadsFromMetadataFile) { EXPECT_EQ("cmd3", session.getCommands()[2]); } +// Test that concurrent sessions are detected and rejected +// Scenario: user1 and user2 both start sessions based on the same commit, +// user1 commits first, then user2 tries to commit and should fail. +TEST_F(ConfigSessionTestFixture, concurrentSessionConflict) { + fs::path sessionDir1 = getTestHomeDir() / ".fboss2_user1"; + fs::path sessionDir2 = getTestHomeDir() / ".fboss2_user2"; + + // Setup mock agent server + setupMockedAgentServer(); + // Only user1's commit should succeed, so only 1 reloadConfig call + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(1); + + // User1 creates a session (captures current HEAD as base) + TestableConfigSession session1( + sessionDir1.string(), (getTestEtcDir() / "coop").string()); + + // User2 also creates a session at the same time (same base) + TestableConfigSession session2( + sessionDir2.string(), (getTestEtcDir() / "coop").string()); + + // User1 makes a change and commits + auto& config1 = session1.getAgentConfig(); + (*config1.sw()->ports())[0].description() = "User1 change"; + session1.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + auto result1 = session1.commit(localhost()); + EXPECT_FALSE(result1.commitSha.empty()); + + // User2 makes a different change + auto& config2 = session2.getAgentConfig(); + (*config2.sw()->ports())[0].description() = "User2 change"; + session2.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + + // User2 tries to commit but should fail because user1 already committed + EXPECT_THROW( + { + try { + session2.commit(localhost()); + } catch (const std::runtime_error& e) { + // Verify the error message mentions the conflict + EXPECT_THAT( + e.what(), + ::testing::HasSubstr("system configuration has changed")); + throw; + } + }, + std::runtime_error); + + // Verify that only user1's change is in the system config + Git git((getTestEtcDir() / "coop").string()); + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; + std::string content; + EXPECT_TRUE(folly::readFile(cliConfigPath.c_str(), content)); + EXPECT_THAT(content, ::testing::HasSubstr("User1 change")); + EXPECT_THAT(content, ::testing::Not(::testing::HasSubstr("User2 change"))); +} + +TEST_F(ConfigSessionTestFixture, rebaseSuccessNoConflict) { + // Test successful rebase when user2's changes don't conflict with user1's + fs::path sessionDir1 = getTestHomeDir() / ".fboss2_user1"; + fs::path sessionDir2 = getTestHomeDir() / ".fboss2_user2"; + + setupMockedAgentServer(); + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(2); + + // User1 creates a session + TestableConfigSession session1( + sessionDir1.string(), (getTestEtcDir() / "coop").string()); + + // User2 also creates a session at the same time (same base) + TestableConfigSession session2( + sessionDir2.string(), (getTestEtcDir() / "coop").string()); + + // User1 changes port[0] description and commits + auto& config1 = session1.getAgentConfig(); + (*config1.sw()->ports())[0].description() = "User1 change"; + session1.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + auto result1 = session1.commit(localhost()); + EXPECT_FALSE(result1.commitSha.empty()); + + // User2 changes port[1] description (non-conflicting - different port) + auto& config2 = session2.getAgentConfig(); + ASSERT_GE(config2.sw()->ports()->size(), 2) << "Need at least 2 ports"; + (*config2.sw()->ports())[1].description() = "User2 change"; + session2.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + + // User2 tries to commit but fails due to stale base + EXPECT_THROW(session2.commit(localhost()), std::runtime_error); + + // User2 rebases - should succeed since changes don't conflict + EXPECT_NO_THROW(session2.rebase()); + + // Now user2 can commit + auto result2 = session2.commit(localhost()); + EXPECT_FALSE(result2.commitSha.empty()); + + // Verify both changes are in the final config + Git git((getTestEtcDir() / "coop").string()); + fs::path cliConfigPath = getTestEtcDir() / "coop" / "cli" / "agent.conf"; + std::string content; + EXPECT_TRUE(folly::readFile(cliConfigPath.c_str(), content)); + EXPECT_THAT(content, ::testing::HasSubstr("User1 change")); + EXPECT_THAT(content, ::testing::HasSubstr("User2 change")); +} + +TEST_F(ConfigSessionTestFixture, rebaseFailsOnConflict) { + // Test that rebase fails when user2's changes conflict with user1's + fs::path sessionDir1 = getTestHomeDir() / ".fboss2_user1"; + fs::path sessionDir2 = getTestHomeDir() / ".fboss2_user2"; + + setupMockedAgentServer(); + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(1); + + // User1 creates a session + TestableConfigSession session1( + sessionDir1.string(), (getTestEtcDir() / "coop").string()); + + // User2 also creates a session at the same time (same base) + TestableConfigSession session2( + sessionDir2.string(), (getTestEtcDir() / "coop").string()); + + // User1 changes port[0] description to "User1 change" + auto& config1 = session1.getAgentConfig(); + (*config1.sw()->ports())[0].description() = "User1 change"; + session1.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + auto result1 = session1.commit(localhost()); + EXPECT_FALSE(result1.commitSha.empty()); + + // User2 changes the SAME port[0] description to "User2 change" (conflict!) + auto& config2 = session2.getAgentConfig(); + (*config2.sw()->ports())[0].description() = "User2 change"; + session2.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + + // User2 tries to rebase but should fail due to conflict + EXPECT_THROW( + { + try { + session2.rebase(); + } catch (const std::runtime_error& e) { + EXPECT_THAT(e.what(), ::testing::HasSubstr("conflict")); + throw; + } + }, + std::runtime_error); +} + +TEST_F(ConfigSessionTestFixture, rebaseNotNeeded) { + // Test that rebase throws when session is already up-to-date + fs::path sessionDir = getTestHomeDir() / ".fboss2"; + + setupMockedAgentServer(); + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(0); + + TestableConfigSession session( + sessionDir.string(), (getTestEtcDir() / "coop").string()); + + // Make a change but don't commit yet + auto& config = session.getAgentConfig(); + (*config.sw()->ports())[0].description() = "My change"; + session.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + + // Try to rebase - should fail because we're already on HEAD + EXPECT_THROW( + { + try { + session.rebase(); + } catch (const std::runtime_error& e) { + EXPECT_THAT(e.what(), ::testing::HasSubstr("No rebase needed")); + throw; + } + }, + std::runtime_error); +} + +// Tests 3-way merge algorithm through rebase() covering: +// - Only session changed (head == base) +// - Only head changed (session == base) +// - Both changed to same value (no conflict) +// - Both changed to different values (conflict) +TEST_F(ConfigSessionTestFixture, threeWayMergeScenarios) { + fs::path sessionDir1 = getTestHomeDir() / ".fboss2_user1"; + fs::path sessionDir2 = getTestHomeDir() / ".fboss2_user2"; + 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); + + // Scenario 1: Only session changed, head unchanged + // User1 commits, User2 changes different field - should merge cleanly + { + TestableConfigSession session1( + sessionDir1.string(), (getTestEtcDir() / "coop").string()); + TestableConfigSession session2( + sessionDir2.string(), (getTestEtcDir() / "coop").string()); + + (*session1.getAgentConfig().sw()->ports())[0].name() = "port0_renamed"; + session1.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + session1.commit(localhost()); + + (*session2.getAgentConfig().sw()->ports())[1].description() = "port1_desc"; + session2.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + EXPECT_NO_THROW(session2.rebase()); + session2.commit(localhost()); + + std::string content; + EXPECT_TRUE(folly::readFile(cliConfigPath.c_str(), content)); + EXPECT_THAT(content, ::testing::HasSubstr("port0_renamed")); + EXPECT_THAT(content, ::testing::HasSubstr("port1_desc")); + } + + // Scenario 2: Both changed same field to identical value - no conflict + { + TestableConfigSession session1( + sessionDir1.string(), (getTestEtcDir() / "coop").string()); + TestableConfigSession session2( + sessionDir2.string(), (getTestEtcDir() / "coop").string()); + + (*session1.getAgentConfig().sw()->ports())[0].description() = "same_value"; + session1.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + session1.commit(localhost()); + + (*session2.getAgentConfig().sw()->ports())[0].description() = "same_value"; + session2.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + EXPECT_NO_THROW(session2.rebase()); + session2.commit(localhost()); + + std::string content; + EXPECT_TRUE(folly::readFile(cliConfigPath.c_str(), content)); + EXPECT_THAT(content, ::testing::HasSubstr("same_value")); + } + + // Scenario 3: Both changed same field to different values - conflict + { + TestableConfigSession session1( + sessionDir1.string(), (getTestEtcDir() / "coop").string()); + TestableConfigSession session2( + sessionDir2.string(), (getTestEtcDir() / "coop").string()); + + (*session1.getAgentConfig().sw()->ports())[0].description() = "user1_value"; + session1.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + session1.commit(localhost()); + + (*session2.getAgentConfig().sw()->ports())[0].description() = "user2_value"; + session2.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + EXPECT_THROW( + { + try { + session2.rebase(); + } catch (const std::runtime_error& e) { + EXPECT_THAT(e.what(), ::testing::HasSubstr("conflict")); + throw; + } + }, + std::runtime_error); + } +} + } // namespace facebook::fboss diff --git a/fboss/oss/cli_tests/test_config_concurrent_sessions.py b/fboss/oss/cli_tests/test_config_concurrent_sessions.py new file mode 100644 index 0000000000000..0a04703433141 --- /dev/null +++ b/fboss/oss/cli_tests/test_config_concurrent_sessions.py @@ -0,0 +1,155 @@ +#!/usr/bin/env python3 +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the BSD-style license found in the +# LICENSE file in the root directory of this source tree. + +""" +End-to-end test for concurrent session conflict detection and rebase. + +This test simulates two users making changes to the configuration at the same +time by using different HOME directories for each "user". It verifies: + +1. User1 can start a session and commit changes +2. User2's commit fails if User1 committed while User2's session was open +3. User2 can rebase their changes onto User1's commit +4. After rebase, User2 can commit successfully + +Requirements: +- FBOSS agent must be running with a valid configuration +- The test must be run as root (or with appropriate permissions) +""" + +import os +import subprocess +import sys +import tempfile + +from cli_test_lib import ( + find_first_eth_interface, + get_fboss_cli, + get_interface_info, +) + + +def run_cli_as_user( + home_dir: str, args: list[str], check: bool = True +) -> subprocess.CompletedProcess: + """Run CLI with a specific HOME directory to simulate a different user.""" + cli = get_fboss_cli() + cmd = [cli] + args + env = os.environ.copy() + env["HOME"] = home_dir + print(f"[User HOME={home_dir}] Running: {' '.join(args)}") + result = subprocess.run(cmd, capture_output=True, text=True, env=env) + if check and result.returncode != 0: + print(f" Failed with code {result.returncode}") + print(f" stdout: {result.stdout}") + print(f" stderr: {result.stderr}") + raise RuntimeError(f"Command failed: {' '.join(cmd)}") + return result + + +def main() -> int: + print("=" * 60) + print("CLI E2E Test: Concurrent Session Conflict Detection") + print("=" * 60) + + # Step 1: Find an interface to test with + print("\n[Step 1] Finding interfaces to test...") + interface = find_first_eth_interface() + print(f" Using interface: {interface.name} (VLAN: {interface.vlan})") + + original_description = interface.description + print(f" Original description: '{original_description}'") + + # Create temporary home directories for two simulated users + with tempfile.TemporaryDirectory( + prefix="user1_" + ) as user1_home, tempfile.TemporaryDirectory(prefix="user2_") as user2_home: + + try: + # Step 2: User1 starts a session + print("\n[Step 2] User1 starts a session...") + run_cli_as_user( + user1_home, + ["config", "interface", interface.name, "description", "User1_change"], + ) + print(" User1 session started with description change") + + # Step 3: User2 also starts a session (same base commit) + print("\n[Step 3] User2 starts a session...") + run_cli_as_user( + user2_home, + ["config", "interface", interface.name, "description", "User2_change"], + ) + print(" User2 session started with description change") + + # Step 4: User1 commits first + print("\n[Step 4] User1 commits first...") + run_cli_as_user(user1_home, ["config", "session", "commit"]) + print(" User1 commit succeeded") + + # Verify User1's change is applied + info = get_interface_info(interface.name) + if info.description != "User1_change": + print(f" ERROR: Expected 'User1_change', got '{info.description}'") + return 1 + print(f" Verified: Description is now '{info.description}'") + + # Step 5: User2 tries to commit - should fail + print("\n[Step 5] User2 tries to commit (should fail)...") + result = run_cli_as_user( + user2_home, ["config", "session", "commit"], check=False + ) + print(f" Return code: {result.returncode}") + print(f" stderr: {result.stderr[:300] if result.stderr else '(empty)'}") + # Check for conflict message in stderr (exit code may incorrectly be 0) + if "system configuration has changed" not in result.stderr: + print(f" ERROR: Expected conflict message in stderr") + return 1 + print(" User2's commit correctly rejected with conflict message") + + # Step 6: User2 rebases + print("\n[Step 6] User2 rebases (should fail - conflicting changes)...") + result = run_cli_as_user( + user2_home, ["config", "session", "rebase"], check=False + ) + print(f" Return code: {result.returncode}") + print(f" stderr: {result.stderr[:300] if result.stderr else '(empty)'}") + # This should fail because both users changed the same field + # Check both stdout and stderr for conflict message + output = (result.stdout or "") + (result.stderr or "") + if "conflict" in output.lower(): + print(" Rebase correctly detected conflict") + else: + print(" Note: Rebase may have succeeded or failed with other error") + + print("\n" + "=" * 60) + print("TEST PASSED") + print("=" * 60) + return 0 + + finally: + # Cleanup: Restore original description + print(f"\n[Cleanup] Restoring original description...") + real_home = os.environ.get("HOME", "/root") + try: + run_cli_as_user( + real_home, + [ + "config", + "interface", + interface.name, + "description", + original_description or "", + ], + ) + run_cli_as_user(real_home, ["config", "session", "commit"]) + print(f" Restored description to '{original_description}'") + except Exception as e: + print(f" Warning: Cleanup failed: {e}") + + +if __name__ == "__main__": + sys.exit(main())