diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index ce64af07dd670..d0891c04e7737 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -575,6 +575,10 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.cpp fboss/cli/fboss2/commands/config/CmdConfigReload.h fboss/cli/fboss2/commands/config/CmdConfigReload.cpp + fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h + fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp + fboss/cli/fboss2/session/ConfigSession.h + fboss/cli/fboss2/session/ConfigSession.cpp fboss/cli/fboss2/CmdListConfig.cpp fboss/cli/fboss2/CmdHandlerImplConfig.cpp ) diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index bdeb5d5c9ee2e..3d4391cb82dee 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -5,6 +5,7 @@ add_executable(fboss2_cmd_test fboss/cli/fboss2/test/TestMain.cpp fboss/cli/fboss2/test/CmdConfigAppliedInfoTest.cpp fboss/cli/fboss2/test/CmdConfigReloadTest.cpp + fboss/cli/fboss2/test/CmdConfigSessionTest.cpp fboss/cli/fboss2/test/CmdSetPortStateTest.cpp fboss/cli/fboss2/test/CmdShowAclTest.cpp fboss/cli/fboss2/test/CmdShowAgentSslTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 3b2d2fc1f11d3..4ce0b60027c1b 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -771,10 +771,14 @@ cpp_library( "CmdListConfig.cpp", "commands/config/CmdConfigAppliedInfo.cpp", "commands/config/CmdConfigReload.cpp", + "commands/config/session/CmdConfigSessionCommit.cpp", + "session/ConfigSession.cpp", ], headers = [ "commands/config/CmdConfigAppliedInfo.h", "commands/config/CmdConfigReload.h", + "commands/config/session/CmdConfigSessionCommit.h", + "session/ConfigSession.h", ], exported_deps = [ ":cmd-handler", diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 6df477995b8cc..d4217e43fa286 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -12,11 +12,14 @@ #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" +#include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" namespace facebook::fboss { template void CmdHandler::run(); template void CmdHandler::run(); +template void +CmdHandler::run(); } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index d59c576e5e576..e59256deb5fbc 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -13,6 +13,7 @@ #include "fboss/cli/fboss2/CmdHandler.h" #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" +#include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" namespace facebook::fboss { @@ -24,6 +25,18 @@ const CommandTree& kConfigCommandTree() { commandHandler, argTypeHandler}, + { + "config", + "session", + "Manage config session", + {{ + "commit", + "Commit the current config session", + commandHandler, + argTypeHandler, + }}, + }, + {"config", "reload", "Reload agent configuration", diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp new file mode 100644 index 0000000000000..eef1f9e4cae7e --- /dev/null +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp @@ -0,0 +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 "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +CmdConfigSessionCommitTraits::RetType CmdConfigSessionCommit::queryClient( + const HostInfo& hostInfo) { + auto& session = ConfigSession::getInstance(); + + if (!session.sessionExists()) { + return "No config session exists. Make a config change first."; + } + + int revision = session.commit(hostInfo); + return "Config session committed successfully as r" + + std::to_string(revision) + " and config reloaded."; +} + +void CmdConfigSessionCommit::printOutput(const RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h new file mode 100644 index 0000000000000..49cf4e1c301d7 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h @@ -0,0 +1,37 @@ +/* + * 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 "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/utils/CmdClientUtils.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" + +namespace facebook::fboss { + +struct CmdConfigSessionCommitTraits : public WriteCommandTraits { + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE; + using ObjectArgType = std::monostate; + using RetType = std::string; +}; + +class CmdConfigSessionCommit + : public CmdHandler { + public: + using ObjectArgType = CmdConfigSessionCommitTraits::ObjectArgType; + using RetType = CmdConfigSessionCommitTraits::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 new file mode 100644 index 0000000000000..ecf11bb06a86b --- /dev/null +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -0,0 +1,441 @@ +/* + * 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/session/ConfigSession.h" + +#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/cli/fboss2/utils/CmdClientUtils.h" +#include "fboss/cli/fboss2/utils/PortMap.h" + +namespace fs = std::filesystem; + +namespace facebook::fboss { + +namespace { // anonymous namespace + +/* + * Atomically update a symlink to point to a new target. + * This creates a temporary symlink and then atomically renames it over the + * existing symlink, ensuring there's no window where the symlink doesn't exist. + * + * @param symlinkPath The path to the symlink to update + * @param newTarget The new target for the symlink + * @throws std::runtime_error if the operation fails + */ +void atomicSymlinkUpdate( + const std::string& symlinkPath, + const std::string& newTarget) { + std::error_code ec; + fs::path symlinkFsPath(symlinkPath); + + // Generate a unique temporary path in the same directory as the target + // symlink, we'll then atomically rename it to the final symlink name. + std::string tmpLinkName = + fmt::format("fboss2_tmp_{}", boost::filesystem::unique_path().string()); + fs::path tempSymlinkPath = symlinkFsPath.parent_path() / tmpLinkName; + + // Create new symlink with temporary name + fs::create_symlink(newTarget, tempSymlinkPath, ec); + if (ec) { + throw std::runtime_error( + fmt::format( + "Failed to create temporary symlink {} to {}: {}", + tempSymlinkPath.string(), + newTarget, + ec.message())); + } + + // Atomically replace the old symlink with the new one + fs::rename(tempSymlinkPath, symlinkPath, ec); + if (ec) { + // Clean up temp symlink + fs::remove(tempSymlinkPath); + throw std::runtime_error( + fmt::format( + "Failed to atomically update symlink {}: {}", + symlinkPath, + ec.message())); + } +} + +/* + * Atomically create the next revision file for a given prefix. + * This function finds the next available revision number (starting from 1) + * and atomically creates a file with that revision number using O_CREAT|O_EXCL. + * This ensures that concurrent commits will get different revision numbers. + * + * @param pathPrefix The path prefix (e.g., "/etc/coop/cli/agent") + * @return A pair containing the path to the newly created revision file + * (e.g., "/etc/coop/cli/agent-r1.conf") and the revision number + * @throws std::runtime_error if unable to create a revision file after many + * attempts + */ +std::pair createNextRevisionFile( + const std::string& pathPrefix) { + // Try up to 100000 revision numbers to handle concurrent commits + // In practice, we should find one quickly + for (int revision = 1; revision <= 100000; ++revision) { + std::string revisionPath = fmt::format("{}-r{}.conf", pathPrefix, revision); + + // Try to atomically create the file with O_CREAT | O_EXCL + // This will fail if the file already exists, ensuring atomicity + int fd = open(revisionPath.c_str(), O_CREAT | O_EXCL | O_WRONLY, 0644); + + if (fd >= 0) { + // Successfully created the file - close it and return the path + close(fd); + return {revisionPath, revision}; + } + + // If errno is EEXIST, the file already exists - try the next revision + if (errno != EEXIST) { + // Some other error occurred + throw std::runtime_error( + fmt::format( + "Failed to create revision file {}: {}", + revisionPath, + folly::errnoStr(errno))); + } + + // File exists, try next revision number + } + + throw std::runtime_error( + "Failed to create revision file after 100000 attempts. " + "This likely indicates a problem with the filesystem or too many revisions."); +} + +std::string getUsername() { + const char* user = std::getenv("USER"); + if (user != nullptr && !std::string(user).empty()) { + return std::string(user); + } + + // If USER env var is not set, get username from UID + uid_t uid = getuid(); + struct passwd* pw = getpwuid(uid); + if (pw == nullptr) { + throw std::runtime_error( + "Failed to get username: USER environment variable not set and " + "getpwuid() failed"); + } + return std::string(pw->pw_name); +} + +std::string getHomeDirectory() { + const char* home = std::getenv("HOME"); + if (home == nullptr || std::string(home).empty()) { + throw std::runtime_error("HOME environment variable not set"); + } + return std::string(home); +} + +void ensureDirectoryExists(const std::string& dirPath) { + std::error_code ec; + fs::create_directories(dirPath, ec); + // create_directories returns false if the directory already exists, but + // that's not an error. Only throw if there's an actual error code set. + if (ec) { + throw std::runtime_error( + fmt::format( + "Failed to create directory {}: {}", dirPath, ec.message())); + } +} + +} // anonymous namespace + +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 + AgentDirectoryUtil dirUtil; + fs::path configDir = fs::path(dirUtil.getConfigDirectory()).parent_path(); + + sessionConfigPath_ = homeDir + "/.fboss2/agent.conf"; + systemConfigPath_ = (configDir / "agent.conf").string(); + cliConfigDir_ = (configDir / "cli").string(); + + initializeSession(); +} + +ConfigSession::ConfigSession( + const std::string& sessionConfigPath, + const std::string& systemConfigPath, + const std::string& cliConfigDir) + : sessionConfigPath_(sessionConfigPath), + systemConfigPath_(systemConfigPath), + cliConfigDir_(cliConfigDir) { + username_ = getUsername(); + initializeSession(); +} + +ConfigSession& ConfigSession::getInstance() { + static ConfigSession instance; + return instance; +} + +std::string ConfigSession::getSessionConfigPath() const { + return sessionConfigPath_; +} + +std::string ConfigSession::getSystemConfigPath() const { + return systemConfigPath_; +} + +bool ConfigSession::sessionExists() const { + return fs::exists(sessionConfigPath_); +} + +cfg::AgentConfig& ConfigSession::getAgentConfig() { + if (!configLoaded_) { + loadConfig(); + } + return agentConfig_; +} + +const cfg::AgentConfig& ConfigSession::getAgentConfig() const { + if (!configLoaded_) { + throw std::runtime_error( + "Config not loaded yet. Call getAgentConfig() (non-const) first."); + } + return agentConfig_; +} + +utils::PortMap& ConfigSession::getPortMap() { + if (!configLoaded_) { + loadConfig(); + } + return *portMap_; +} + +const utils::PortMap& ConfigSession::getPortMap() const { + if (!configLoaded_) { + throw std::runtime_error( + "Config not loaded yet. Call getPortMap() (non-const) first."); + } + return *portMap_; +} + +void ConfigSession::saveConfig() { + if (!configLoaded_) { + throw std::runtime_error("No config loaded to save"); + } + + // We need to do a round-trip through serialize -> parse -> toPrettyJson + // because SimpleJSONSerializer handles Thrift maps with integer keys + // (like clientIdToAdminDistance) by converting them to strings. + // If we use facebook::thrift::to_dynamic() directly, the integer keys + // are preserved as integers in the folly::dynamic object, which causes + // folly::toPrettyJson() to fail because JSON objects require string keys. + std::string json = + apache::thrift::SimpleJSONSerializer::serialize( + agentConfig_); + std::string prettyJson = folly::toPrettyJson(folly::parseJson(json)); + + // Use folly::writeFileAtomic with sync to avoid race conditions when multiple + // threads/processes write to the same session file. WITH_SYNC ensures data + // is flushed to disk before the atomic rename, preventing readers from + // seeing partial/corrupted data. + folly::writeFileAtomic( + sessionConfigPath_, prettyJson, 0644, folly::SyncType::WITH_SYNC); +} + +int ConfigSession::extractRevisionNumber(const std::string& filenameOrPath) { + // Extract just the filename if a full path was provided + std::string filename = filenameOrPath; + size_t lastSlash = filenameOrPath.rfind('/'); + if (lastSlash != std::string::npos) { + filename = filenameOrPath.substr(lastSlash + 1); + } + + // Pattern: agent-rN.conf where N is a positive integer + // Using RE2 instead of std::regex to avoid stack overflow issues (GCC bug) + static const re2::RE2 pattern(R"(^agent-r(\d+)\.conf$)"); + int revision = -1; + + if (re2::RE2::FullMatch(filename, pattern, &revision)) { + return revision; + } + return -1; +} + +void ConfigSession::loadConfig() { + std::string configJson; + if (!folly::readFile(sessionConfigPath_.c_str(), configJson)) { + throw std::runtime_error( + fmt::format("Failed to read config file: {}", sessionConfigPath_)); + } + + apache::thrift::SimpleJSONSerializer::deserialize( + configJson, agentConfig_); + + // Handle the legacy case where config might be a bare SwitchConfig + if (*agentConfig_.sw() == cfg::SwitchConfig()) { + apache::thrift::SimpleJSONSerializer::deserialize( + configJson, *agentConfig_.sw()); + } + portMap_ = std::make_unique(agentConfig_); + configLoaded_ = true; +} + +void ConfigSession::initializeSession() { + if (!sessionExists()) { + // Ensure the parent directory of the session config exists + fs::path sessionPath(sessionConfigPath_); + ensureDirectoryExists(sessionPath.parent_path().string()); + copySystemConfigToSession(); + } +} + +void ConfigSession::copySystemConfigToSession() { + // Resolve symlink if system config is a symlink + std::string sourceConfig = systemConfigPath_; + std::error_code ec; + + if (LIKELY(fs::is_symlink(systemConfigPath_, ec))) { + sourceConfig = fs::read_symlink(systemConfigPath_, ec).string(); + if (ec) { + throw std::runtime_error( + fmt::format( + "Failed to read symlink {}: {}", + systemConfigPath_, + ec.message())); + } + // If the symlink is relative, make it absolute relative to the system + // config directory + if (!fs::path(sourceConfig).is_absolute()) { + fs::path systemConfigDir = fs::path(systemConfigPath_).parent_path(); + sourceConfig = (systemConfigDir / sourceConfig).string(); + } + } + + // Read source 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. + // WITH_SYNC ensures data is flushed to disk before the atomic rename. + std::string configData; + if (!folly::readFile(sourceConfig.c_str(), configData)) { + throw std::runtime_error( + fmt::format("Failed to read config from {}", sourceConfig)); + } + + folly::writeFileAtomic( + sessionConfigPath_, configData, 0644, folly::SyncType::WITH_SYNC); +} + +int ConfigSession::commit(const HostInfo& hostInfo) { + if (!sessionExists()) { + throw std::runtime_error( + "No config session exists. Make a config change first."); + } + + ensureDirectoryExists(cliConfigDir_); + + // Atomically create the next revision file + // This ensures concurrent commits get different revision numbers + auto [targetConfigPath, revision] = + createNextRevisionFile(fmt::format("{}/agent", cliConfigDir_)); + std::error_code ec; + + // Read the old symlink target for rollback if needed + std::string oldSymlinkTarget; + if (!fs::is_symlink(systemConfigPath_)) { + throw std::runtime_error( + fmt::format( + "{} is not a symlink. Expected it to be a symlink.", + systemConfigPath_)); + } + oldSymlinkTarget = fs::read_symlink(systemConfigPath_, ec); + if (ec) { + throw std::runtime_error( + fmt::format( + "Failed to read symlink {}: {}", systemConfigPath_, ec.message())); + } + + // Copy session config to the atomically-created revision file + // Overwrite the empty file that was created by createNextRevisionFile + fs::copy_file( + sessionConfigPath_, + targetConfigPath, + fs::copy_options::overwrite_existing, + ec); + if (ec) { + // Clean up the revision file we created + fs::remove(targetConfigPath); + throw std::runtime_error( + fmt::format( + "Failed to copy session config to {}: {}", + targetConfigPath, + ec.message())); + } + + // Atomically update the symlink to point to the new config + atomicSymlinkUpdate(systemConfigPath_, targetConfigPath); + + // Reload the config - if this fails, rollback the symlink atomically + try { + auto client = + utils::createClient>( + hostInfo); + client->sync_reloadConfig(); + LOG(INFO) << "Config committed as revision r" << revision; + } catch (const std::exception& ex) { + // Rollback: atomically restore the old symlink + try { + atomicSymlinkUpdate(systemConfigPath_, oldSymlinkTarget); + } catch (const std::exception& rollbackEx) { + // If rollback also fails, include both errors in the message + throw std::runtime_error( + fmt::format( + "Failed to reload config: {}. Additionally, failed to rollback the config: {}", + ex.what(), + rollbackEx.what())); + } + throw std::runtime_error( + fmt::format( + "Failed to reload config, config was rolled back automatically: {}", + ex.what())); + } + + // Only remove the session config after everything succeeded + fs::remove(sessionConfigPath_, ec); + if (ec) { + // Log warning but don't fail - the commit succeeded + LOG(WARNING) << fmt::format( + "Failed to remove session config {}: {}", + sessionConfigPath_, + ec.message()); + } + + return revision; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/session/ConfigSession.h b/fboss/cli/fboss2/session/ConfigSession.h new file mode 100644 index 0000000000000..b07e9962c1702 --- /dev/null +++ b/fboss/cli/fboss2/session/ConfigSession.h @@ -0,0 +1,144 @@ +/* + * 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/agent/gen-cpp2/agent_config_types.h" +#include "fboss/agent/if/gen-cpp2/ctrl_types.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss::utils { +class PortMap; +} + +namespace facebook::fboss { + +/** + * ConfigSession manages configuration editing sessions for the fboss2 CLI. + * + * OVERVIEW: + * ConfigSession provides a session-based workflow for editing FBOSS agent + * configuration. It maintains one or more session files that can be edited + * and then atomically committed to the system configuration. + * + * SINGLETON PATTERN: + * ConfigSession is typically accessed via getInstance(), which currently + * returns a per-UNIX-user singleton. The singleton is created on first + * access and persists across CLI commands until commit() is called. + * + * TYPICAL USAGE: + * Most CLI commands follow this pattern: + * 1. Get the singleton: auto& session = ConfigSession::getInstance(); + * 2. Access the config: auto& config = session.getAgentConfig(); + * 3. Modify the config: config.sw()->ports()[0].name() = "eth0"; + * 4. Save changes: session.saveConfig(); + * + * The changes are saved to the session file (currently ~/.fboss2/agent.conf) + * but are NOT applied to the running system until explicitly committed. + * + * COMMIT FLOW: + * To apply session changes to the system: + * 1. User runs: fboss2 config session commit + * 2. ConfigSession::commit() is called, which: + * a. Determines the next revision number (e.g., r5) + * b. Atomically writes session config to /etc/coop/cli/agent-r5.conf + * c. Atomically updates the /etc/coop/agent.conf symlink to point to + * agent-r5.conf and calls reloadConfig() on the wedge_agent to + * reload its configuration + * 3. The session file is cleared (ready for next edit session) + * + * ROLLBACK FLOW: + * To revert to a previous configuration: + * 1. User runs: fboss2 config rollback [rN] + * 2. ConfigSession::rollback() is called, which: + * a. Identifies the target revision (previous or specified) + * b. Atomically updates /etc/coop/agent.conf symlink to point to + * agent-rN.conf c. Calls wedge_agent to reload the configuration + * + * CONFIGURATION FILES: + * - Session file: ~/.fboss2/agent.conf (per-user, temporary edits) + * - System config: /agent.conf (symlink to current revision) + * - Revision files: /cli/agent-rN.conf (committed configs) + * + * Where is determined by AgentDirectoryUtil::getConfigDirectory() + * (typically /etc/coop, derived from the parent of the config directory path). + * + * THREAD SAFETY: + * ConfigSession is NOT thread-safe. It is designed for single-threaded CLI + * command execution. The code is safe in face of concurrent usage from + * multiple processes, e.g. two users trying to commit a config at the same + * time should not lead to a partially committed config or any process being + * able to read a partially written file. + */ +class ConfigSession { + public: + ConfigSession(); + ~ConfigSession() = default; + + // Get or create the current config session + // If no session exists, copies /etc/coop/agent.conf to ~/.fboss2/agent.conf + static ConfigSession& getInstance(); + + // Get the path to the session config file (~/.fboss2/agent.conf) + std::string getSessionConfigPath() const; + + // Get the path to the system config file (/etc/coop/agent.conf) + std::string getSystemConfigPath() const; + + // Atomically commit the session to /etc/coop/cli/agent-rN.conf, + // update the symlink /etc/coop/agent.conf to point to it, and reload config. + // Returns the revision number that was committed if the commit was + // successful. + int commit(const HostInfo& hostInfo); + + // Check if a session exists + bool sessionExists() const; + + // Get the parsed agent configuration + cfg::AgentConfig& getAgentConfig(); + const cfg::AgentConfig& getAgentConfig() const; + + // Get the PortMap for port-to-interface lookups + utils::PortMap& getPortMap(); + const utils::PortMap& getPortMap() const; + + // Save the configuration back to the session file + void saveConfig(); + + // Extract revision number from a filename or path like "agent-r42.conf" + // Returns -1 if the filename doesn't match the expected pattern + static int extractRevisionNumber(const std::string& filenameOrPath); + + protected: + // Constructor for testing with custom paths + ConfigSession( + const std::string& sessionConfigPath, + const std::string& systemConfigPath, + const std::string& cliConfigDir); + + private: + std::string sessionConfigPath_; + std::string systemConfigPath_; + std::string cliConfigDir_; + std::string username_; + + // Lazy-initialized configuration and port map + cfg::AgentConfig agentConfig_; + std::unique_ptr portMap_; + bool configLoaded_ = false; + + // Initialize the session (creates session config file if it doesn't exist) + void initializeSession(); + void copySystemConfigToSession(); + void loadConfig(); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/BUCK b/fboss/cli/fboss2/test/BUCK index 1a2060ec9188d..35813c9014337 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -8,6 +8,7 @@ cpp_library( headers = [ "CmdHandlerTestBase.h", "MockClients.h", + "TestableConfigSession.h", ], exported_deps = [ "fbsource//third-party/googletest:gmock", @@ -55,6 +56,7 @@ cpp_unittest( srcs = [ "CmdConfigAppliedInfoTest.cpp", "CmdConfigReloadTest.cpp", + "CmdConfigSessionTest.cpp", "CmdGetPcapTest.cpp", "CmdSetPortStateTest.cpp", "CmdShowAclTest.cpp", diff --git a/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp b/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp new file mode 100644 index 0000000000000..8b08a3ca1c950 --- /dev/null +++ b/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp @@ -0,0 +1,533 @@ +/* + * 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 +#include +#include +#include +#include +#include +#include +#include + +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/CmdHandlerTestBase.h" +#include "fboss/cli/fboss2/test/TestableConfigSession.h" +#include "fboss/cli/fboss2/utils/PortMap.h" + +namespace fs = std::filesystem; + +namespace facebook::fboss { + +class ConfigSessionTestFixture : public CmdHandlerTestBase { + 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 + fs::create_directories(testHomeDir_); + fs::create_directories(testEtcDir_ / "coop"); + fs::create_directories(testEtcDir_ / "coop" / "cli"); + + // Set environment variables + setenv("HOME", testHomeDir_.c_str(), 1); + setenv("USER", "testuser", 1); + + // Create a test system config file as agent-r1.conf in the cli directory + // and create a symlink at agent.conf pointing to it + fs::path initialRevision = testEtcDir_ / "coop" / "cli" / "agent-r1.conf"; + createTestConfig(initialRevision, R"({ + "sw": { + "ports": [ + { + "logicalID": 1, + "name": "eth1/1/1", + "state": 2, + "speed": 100000 + } + ] + } +})"); + + // Create symlink at agent.conf pointing to agent-r1.conf + // Use an absolute path for the symlink target so it works in tests + systemConfigPath_ = testEtcDir_ / "coop" / "agent.conf"; + fs::create_symlink(initialRevision, systemConfigPath_); + } + + 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 systemConfigPath_; +}; + +TEST_F(ConfigSessionTestFixture, sessionInitialization) { + // Initially, session directory should not exist + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + EXPECT_FALSE(fs::exists(sessionDir)); + + // Creating a ConfigSession should create the directory and copy the config + // systemConfigPath_ is already a symlink created in SetUp() + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Verify the directory was created + EXPECT_TRUE(fs::exists(sessionDir)); + EXPECT_TRUE(session.sessionExists()); + EXPECT_TRUE(fs::exists(sessionConfig)); + + // Verify content was copied correctly + // Read the actual file that systemConfigPath_ points to + // fs::read_symlink returns a relative path, so we need to resolve it + fs::path symlinkTarget = fs::read_symlink(systemConfigPath_); + fs::path actualConfigPath = systemConfigPath_.parent_path() / symlinkTarget; + std::string systemContent = readFile(actualConfigPath); + std::string sessionContent = readFile(sessionConfig); + EXPECT_EQ(systemContent, sessionContent); +} + +TEST_F(ConfigSessionTestFixture, sessionConfigModified) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // Create a ConfigSession + // systemConfigPath_ is already a symlink created in SetUp() + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Modify the session config through the ConfigSession API + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + ports[0].description() = "Modified port"; + session.saveConfig(); + + // Verify session config is modified + std::string sessionContent = readFile(sessionConfig); + // fs::read_symlink returns a relative path, so we need to resolve it + fs::path symlinkTarget = fs::read_symlink(systemConfigPath_); + fs::path actualConfigPath = systemConfigPath_.parent_path() / symlinkTarget; + std::string systemContent = readFile(actualConfigPath); + EXPECT_NE(sessionContent, systemContent); + EXPECT_THAT(sessionContent, ::testing::HasSubstr("Modified port")); +} + +TEST_F(ConfigSessionTestFixture, sessionCommit) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path cliConfigDir = testEtcDir_ / "coop" / "cli"; + + // Verify old symlink exists (created in SetUp) + EXPECT_TRUE(fs::is_symlink(systemConfigPath_)); + EXPECT_EQ( + fs::read_symlink(systemConfigPath_), cliConfigDir / "agent-r1.conf"); + + // Setup mock agent server + setupMockedAgentServer(); + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(2); + + // First commit: Create a ConfigSession and commit a change + { + // systemConfigPath_ is already a symlink to agent-r1.conf created in + // SetUp() + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + cliConfigDir.string()); + + // Modify the session config + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + ports[0].description() = "First commit"; + session.saveConfig(); + + // Commit the session + int revision = session.commit(localhost()); + + // Verify session config no longer exists (removed after commit) + EXPECT_FALSE(fs::exists(sessionConfig)); + + // Verify new revision was created in cli directory + EXPECT_EQ(revision, 2); + fs::path targetConfig = cliConfigDir / "agent-r2.conf"; + EXPECT_TRUE(fs::exists(targetConfig)); + EXPECT_THAT(readFile(targetConfig), ::testing::HasSubstr("First commit")); + + // Verify symlink was replaced and points to new revision + EXPECT_TRUE(fs::is_symlink(systemConfigPath_)); + EXPECT_EQ(fs::read_symlink(systemConfigPath_), targetConfig); + } + + // Second commit: Create a new session and verify it's based on r2, not r1 + { + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + cliConfigDir.string()); + + // Verify the new session is based on r2 (the latest committed revision) + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + EXPECT_EQ(*ports[0].description(), "First commit"); + + // Make another change to the same port + ports[0].description() = "Second commit"; + session.saveConfig(); + + // Commit the second change + int revision = session.commit(localhost()); + + // Verify new revision was created + EXPECT_EQ(revision, 3); + fs::path targetConfig = cliConfigDir / "agent-r3.conf"; + EXPECT_TRUE(fs::exists(targetConfig)); + EXPECT_THAT(readFile(targetConfig), ::testing::HasSubstr("Second commit")); + + // Verify symlink was updated to point to r3 + EXPECT_TRUE(fs::is_symlink(systemConfigPath_)); + EXPECT_EQ(fs::read_symlink(systemConfigPath_), targetConfig); + + // Verify all revisions exist + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r1.conf")); + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r2.conf")); + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r3.conf")); + } +} + +TEST_F(ConfigSessionTestFixture, multipleChangesInOneSession) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // Create a ConfigSession + // systemConfigPath_ is already a symlink created in SetUp() + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Make first change + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + ports[0].description() = "Change 1"; + session.saveConfig(); + EXPECT_THAT(readFile(sessionConfig), ::testing::HasSubstr("Change 1")); + + // Make second change + ports[0].description() = "Change 2"; + session.saveConfig(); + EXPECT_THAT(readFile(sessionConfig), ::testing::HasSubstr("Change 2")); + + // Make third change + ports[0].description() = "Change 3"; + session.saveConfig(); + EXPECT_THAT(readFile(sessionConfig), ::testing::HasSubstr("Change 3")); +} + +TEST_F(ConfigSessionTestFixture, sessionPersistsAcrossCommands) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // Create first ConfigSession and modify config + // systemConfigPath_ is already a symlink created in SetUp() + { + TestableConfigSession session1( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + auto& config = session1.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + ports[0].description() = "Persistent change"; + session1.saveConfig(); + } + + // Verify session persists (file still exists with same content) + EXPECT_TRUE(fs::exists(sessionConfig)); + EXPECT_THAT( + readFile(sessionConfig), ::testing::HasSubstr("Persistent change")); + + // Create another ConfigSession to simulate another command reading the + // session + { + TestableConfigSession session2( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + auto& config = session2.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + // Verify the change persisted + EXPECT_EQ(*ports[0].description(), "Persistent change"); + } +} + +TEST_F(ConfigSessionTestFixture, symlinkRollbackOnFailure) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path cliConfigDir = testEtcDir_ / "coop" / "cli"; + + // Verify old symlink exists (created in SetUp) + EXPECT_TRUE(fs::is_symlink(systemConfigPath_)); + EXPECT_EQ( + fs::read_symlink(systemConfigPath_), cliConfigDir / "agent-r1.conf"); + + // Setup mock agent server to fail reloadConfig + setupMockedAgentServer(); + EXPECT_CALL(getMockAgent(), reloadConfig()) + .WillOnce(::testing::Throw(std::runtime_error("Reload failed"))); + + // Create a ConfigSession and try to commit + // systemConfigPath_ is already a symlink to agent-r1.conf created in SetUp() + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + cliConfigDir.string()); + + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + ports[0].description() = "Failed change"; + session.saveConfig(); + + // Commit should fail and rollback the symlink + EXPECT_THROW(session.commit(localhost()), std::runtime_error); + + // Verify symlink was rolled back to old target + EXPECT_TRUE(fs::is_symlink(systemConfigPath_)); + EXPECT_EQ( + fs::read_symlink(systemConfigPath_), cliConfigDir / "agent-r1.conf"); + + // Verify session config still exists (not removed on failed commit) + EXPECT_TRUE(fs::exists(sessionConfig)); +} + +TEST_F(ConfigSessionTestFixture, atomicRevisionCreation) { + fs::path cliConfigDir = testEtcDir_ / "coop" / "cli"; + + // Setup mock agent server + setupMockedAgentServer(); + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(2); + + // Run two concurrent commits to test atomic revision creation + // Each thread uses a separate session config path (simulating different + // users) Both threads will try to commit at the same time, and the atomic + // file creation (O_CREAT | O_EXCL) should ensure they get different revision + // numbers without conflicts + std::atomic revision1{0}; + std::atomic revision2{0}; + + auto commitTask = [&](const std::string& sessionName, + const std::string& description, + std::atomic& rev) { + fs::path sessionDir = testHomeDir_ / sessionName; + fs::path sessionConfig = sessionDir / "agent.conf"; + + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + cliConfigDir.string()); + + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + ports[0].description() = description; + session.saveConfig(); + + rev = session.commit(localhost()); + }; + + std::thread thread1( + commitTask, ".fboss2_user1", "First commit", std::ref(revision1)); + std::thread thread2( + commitTask, ".fboss2_user2", "Second commit", std::ref(revision2)); + + thread1.join(); + thread2.join(); + + // Both commits should succeed with different revision numbers + EXPECT_NE(revision1.load(), 0); + EXPECT_NE(revision2.load(), 0); + EXPECT_NE(revision1.load(), revision2.load()); + + // Both should be either r2 or r3 (one gets r2, the other gets r3) + EXPECT_TRUE( + (revision1.load() == 2 && revision2.load() == 3) || + (revision1.load() == 3 && revision2.load() == 2)); + + // Both revision files should exist + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r2.conf")); + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r3.conf")); + + // Verify the content of each revision matches what was committed + std::string r2Content = readFile(cliConfigDir / "agent-r2.conf"); + std::string r3Content = readFile(cliConfigDir / "agent-r3.conf"); + EXPECT_TRUE( + (r2Content.find("First commit") != std::string::npos && + r3Content.find("Second commit") != std::string::npos) || + (r2Content.find("Second commit") != std::string::npos && + r3Content.find("First commit") != std::string::npos)); +} + +TEST_F(ConfigSessionTestFixture, concurrentSessionCreationSameUser) { + fs::path cliConfigDir = testEtcDir_ / "coop" / "cli"; + + // Setup mock agent server + setupMockedAgentServer(); + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(2); + + // Test concurrent session creation and commits for the SAME user + // This tests the race conditions in: + // 1. ensureDirectoryExists() - concurrent directory creation + // 2. copySystemConfigToSession() - concurrent session file creation + // 3. saveConfig() - concurrent writes to the same session file + // 4. atomicSymlinkUpdate() - concurrent symlink updates + // + // Note: When two threads share the same session file, they race to modify it. + // The atomic operations ensure no crashes or corruption, but both commits + // might have the same content if one thread's saveConfig() overwrites the + // other's changes. This is expected behavior - the important thing is that + // both commits succeed without crashes. + std::atomic revision1{0}; + std::atomic revision2{0}; + + auto commitTask = [&](const std::string& description, std::atomic& rev) { + // Both threads use the SAME session path + fs::path sessionDir = testHomeDir_ / ".fboss2_shared"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + cliConfigDir.string()); + + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + ports[0].description() = description; + session.saveConfig(); + + rev = session.commit(localhost()); + }; + + std::thread thread1(commitTask, "First commit", std::ref(revision1)); + std::thread thread2(commitTask, "Second commit", std::ref(revision2)); + + thread1.join(); + thread2.join(); + + // Both commits should succeed with different revision numbers + EXPECT_NE(revision1.load(), 0); + EXPECT_NE(revision2.load(), 0); + EXPECT_NE(revision1.load(), revision2.load()); + + // Both should be either r2 or r3 (one gets r2, the other gets r3) + EXPECT_TRUE( + (revision1.load() == 2 && revision2.load() == 3) || + (revision1.load() == 3 && revision2.load() == 2)); + + // Both revision files should exist + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r2.conf")); + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r3.conf")); + + // The history command would list all three revisions with their metadata +} + +TEST_F(ConfigSessionTestFixture, revisionNumberExtraction) { + // Test the revision number extraction logic + fs::path cliConfigDir = testEtcDir_ / "coop" / "cli"; + + // Create files with various revision numbers + createTestConfig(cliConfigDir / "agent-r1.conf", R"({})"); + createTestConfig(cliConfigDir / "agent-r42.conf", R"({})"); + createTestConfig(cliConfigDir / "agent-r999.conf", R"({})"); + + // Verify files exist + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r1.conf")); + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r42.conf")); + EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r999.conf")); + + // Test extractRevisionNumber() method + EXPECT_EQ( + ConfigSession::extractRevisionNumber( + (cliConfigDir / "agent-r1.conf").string()), + 1); + EXPECT_EQ( + ConfigSession::extractRevisionNumber( + (cliConfigDir / "agent-r42.conf").string()), + 42); + EXPECT_EQ( + ConfigSession::extractRevisionNumber( + (cliConfigDir / "agent-r999.conf").string()), + 999); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/TestableConfigSession.h b/fboss/cli/fboss2/test/TestableConfigSession.h new file mode 100644 index 0000000000000..4838656d3db37 --- /dev/null +++ b/fboss/cli/fboss2/test/TestableConfigSession.h @@ -0,0 +1,28 @@ +/* + * 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 "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +// Test-only derived class that exposes the protected constructor and methods +// This allows tests to inject custom paths and control the singleton instance +class TestableConfigSession : public ConfigSession { + public: + TestableConfigSession( + const std::string& sessionConfigPath, + const std::string& systemConfigPath, + const std::string& cliConfigDir) + : ConfigSession(sessionConfigPath, systemConfigPath, cliConfigDir) {} +}; + +} // namespace facebook::fboss