diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index ffd14d61e7f5c..d3f5360092927 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -768,6 +768,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp fboss/cli/fboss2/session/ConfigSession.h fboss/cli/fboss2/session/ConfigSession.cpp + fboss/cli/fboss2/session/Git.h + fboss/cli/fboss2/session/Git.cpp fboss/cli/fboss2/utils/InterfaceList.h fboss/cli/fboss2/utils/InterfaceList.cpp fboss/cli/fboss2/CmdListConfig.cpp diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index 1de3c9aed9a46..0fcd09dfd2f25 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -68,6 +68,7 @@ add_executable(fboss2_cmd_test # fboss/cli/fboss2/test/CmdShowTransceiverTest.cpp - excluded (depends on configerator bgp namespace) fboss/cli/fboss2/test/CmdStartPcapTest.cpp fboss/cli/fboss2/test/CmdStopPcapTest.cpp + fboss/cli/fboss2/test/GitTest.cpp fboss/cli/fboss2/test/PortMapTest.cpp ) diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index ed839a636e16e..38b689d11184c 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -932,6 +932,7 @@ cpp_library( "commands/config/session/CmdConfigSessionCommit.cpp", "commands/config/session/CmdConfigSessionDiff.cpp", "session/ConfigSession.cpp", + "session/Git.cpp", "utils/InterfaceList.cpp", ], headers = [ @@ -992,6 +993,7 @@ cpp_library( "commands/config/session/CmdConfigSessionCommit.h", "commands/config/session/CmdConfigSessionDiff.h", "session/ConfigSession.h", + "session/Git.h", "utils/InterfaceList.h", ], exported_deps = [ diff --git a/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp b/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp index e6a950462f6d0..2ad6bf9f5a9fb 100644 --- a/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp +++ b/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp @@ -9,120 +9,23 @@ */ #include "fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h" -#include -#include -#include -#include #include -#include -#include #include -#include #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/Table.h" -namespace fs = std::filesystem; - namespace facebook::fboss { namespace { -struct RevisionInfo { - int revisionNumber{}; - std::string owner; - int64_t commitTimeNsec{}; // Commit time in nanoseconds since epoch - std::string filePath; -}; - -// Get the username from a UID -std::string getUsername(uid_t uid) { - struct passwd* pw = getpwuid(uid); - if (pw) { - return std::string(pw->pw_name); - } - // If we can't resolve the username, return the UID as a string - return "UID:" + std::to_string(uid); -} - -// Format time as a human-readable string with milliseconds -std::string formatTime(int64_t timeNsec) { - // Convert nanoseconds to seconds and remaining nanoseconds - std::time_t timeSec = timeNsec / 1000000000; - long nsec = timeNsec % 1000000000; - - char buffer[100]; +// Format Unix timestamp (seconds) as a human-readable string +std::string formatTime(int64_t timeSec) { + char buffer[32]; tm timeinfo{}; - localtime_r(&timeSec, &timeinfo); + std::time_t time = timeSec; + localtime_r(&time, &timeinfo); std::strftime(buffer, sizeof(buffer), "%Y-%m-%d %H:%M:%S", &timeinfo); - - // Add milliseconds - long milliseconds = nsec / 1000000; - std::ostringstream oss; - oss << buffer << '.' << std::setfill('0') << std::setw(3) << milliseconds; - return oss.str(); -} - -// Collect all revision files from the CLI config directory -std::vector collectRevisions(const std::string& cliConfigDir) { - std::vector revisions; - - std::error_code ec; - if (!fs::exists(cliConfigDir, ec) || !fs::is_directory(cliConfigDir, ec)) { - // Directory doesn't exist or is not a directory - return revisions; - } - - for (const auto& entry : fs::directory_iterator(cliConfigDir, ec)) { - if (ec) { - continue; // Skip entries we can't read - } - - if (!entry.is_regular_file(ec)) { - continue; // Skip non-regular files - } - - std::string filename = entry.path().filename().string(); - int revNum = ConfigSession::extractRevisionNumber(filename); - - if (revNum < 0) { - continue; // Skip files that don't match our pattern - } - - // Get file metadata using statx to get birth time (creation time) - struct statx stx{}; - if (statx( - AT_FDCWD, entry.path().c_str(), 0, STATX_BTIME | STATX_UID, &stx) != - 0) { - continue; // Skip if we can't get file stats - } - - RevisionInfo info; - info.revisionNumber = revNum; - info.owner = getUsername(stx.stx_uid); - // Use birth time (creation time) if available, otherwise fall back to mtime - if (stx.stx_mask & STATX_BTIME) { - info.commitTimeNsec = - static_cast(stx.stx_btime.tv_sec) * 1000000000 + - stx.stx_btime.tv_nsec; - } else { - info.commitTimeNsec = - static_cast(stx.stx_mtime.tv_sec) * 1000000000 + - stx.stx_mtime.tv_nsec; - } - info.filePath = entry.path().string(); - - revisions.push_back(info); - } - - // Sort by revision number (ascending) - std::sort( - revisions.begin(), - revisions.end(), - [](const RevisionInfo& a, const RevisionInfo& b) { - return a.revisionNumber < b.revisionNumber; - }); - - return revisions; + return buffer; } } // namespace @@ -130,23 +33,27 @@ std::vector collectRevisions(const std::string& cliConfigDir) { CmdConfigHistoryTraits::RetType CmdConfigHistory::queryClient( const HostInfo& /* hostInfo */) { auto& session = ConfigSession::getInstance(); - const std::string cliConfigDir = session.getCliConfigDir(); + auto& git = session.getGit(); - auto revisions = collectRevisions(cliConfigDir); + // Get the commit history from Git for the CLI config file + auto commits = git.log(session.getCliConfigPath()); - if (revisions.empty()) { - return "No config revisions found in " + cliConfigDir; + if (commits.empty()) { + return "No config revisions found in Git history"; } // Build the table utils::Table table; - table.setHeader({"Revision", "Owner", "Commit Time"}); + table.setHeader({"Commit", "Author", "Commit Time", "Message"}); - for (const auto& rev : revisions) { + for (const auto& commit : commits) { + // Use short SHA1 (first 8 characters) + std::string shortSha = commit.sha1.substr(0, 8); table.addRow( - {"r" + std::to_string(rev.revisionNumber), - rev.owner, - formatTime(rev.commitTimeNsec)}); + {shortSha, + commit.authorName, + formatTime(commit.timestamp), + commit.subject}); } // Convert table to string diff --git a/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp b/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp index 2b3b04e07a0ff..595ae26f54415 100644 --- a/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp +++ b/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp @@ -21,28 +21,28 @@ CmdConfigRollbackTraits::RetType CmdConfigRollback::queryClient( // Validate arguments if (revisions.size() > 1) { throw std::invalid_argument( - "Too many arguments. Expected 0 or 1 revision specifier."); + "Too many arguments. Expected 0 or 1 commit SHA."); } if (!revisions.empty() && revisions[0] == "current") { throw std::invalid_argument( - "Cannot rollback to 'current'. Please specify a revision number like 'r42'."); + "Cannot rollback to 'current'. Please specify a commit SHA."); } try { - int newRevision; + std::string newCommitSha; if (revisions.empty()) { // No revision specified - rollback to previous revision - newRevision = session.rollback(hostInfo); + newCommitSha = session.rollback(hostInfo); } else { - // Specific revision specified - newRevision = session.rollback(hostInfo, revisions[0]); + // Specific commit SHA specified + newCommitSha = session.rollback(hostInfo, revisions[0]); } - if (newRevision) { - return "Successfully rolled back to r" + std::to_string(newRevision) + - " and config reloaded."; + if (!newCommitSha.empty()) { + return "Successfully rolled back. New commit: " + + newCommitSha.substr(0, 8) + ". Config reloaded."; } else { - return "Failed to create a new revision after rollback."; + return "Failed to create a new commit after rollback."; } } catch (const std::exception& ex) { throw std::runtime_error( diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp index 14d7300018799..fbed9488d33cf 100644 --- a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp @@ -48,23 +48,24 @@ CmdConfigSessionCommitTraits::RetType CmdConfigSessionCommit::queryClient( // Build message based on what actions were taken std::string message; + std::string shortSha = result.commitSha.substr(0, 7); if (restartedServices.empty() && reloadedServices.empty()) { - message = fmt::format( - "Config session committed successfully as r{}.", result.revision); + message = + fmt::format("Config session committed successfully as {}.", shortSha); } else if (restartedServices.empty()) { message = fmt::format( - "Config session committed successfully as r{} and config reloaded for {}.", - result.revision, + "Config session committed successfully as {} and config reloaded for {}.", + shortSha, folly::join(", ", reloadedServices)); } else if (reloadedServices.empty()) { message = fmt::format( - "Config session committed successfully as r{} and {} restarted.", - result.revision, + "Config session committed successfully as {} and {} restarted.", + shortSha, folly::join(", ", restartedServices)); } else { message = fmt::format( - "Config session committed successfully as r{}, {} restarted, and config reloaded for {}.", - result.revision, + "Config session committed successfully as {}, {} restarted, and config reloaded for {}.", + shortSha, folly::join(", ", restartedServices), folly::join(", ", reloadedServices)); } diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp index efb0fb447f8aa..8efe2a5244735 100644 --- a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.cpp @@ -11,44 +11,62 @@ #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" #include "fboss/cli/fboss2/session/ConfigSession.h" +#include #include -#include - -namespace fs = std::filesystem; namespace facebook::fboss { namespace { -// Helper function to resolve a revision specifier to a file path -// Note: Revision format validation is done in RevisionList constructor -std::string resolveRevisionPath( +// Helper function to get config content from a revision specifier +// Returns the content and a label for the revision +std::pair getRevisionContent( const std::string& revision, - const std::string& cliConfigDir, - const std::string& systemConfigPath) { - if (revision == "current") { - return systemConfigPath; - } + ConfigSession& session) { + auto& git = session.getGit(); + std::string cliConfigPath = session.getCliConfigPath(); - // Build the path (revision is already validated to be in "rN" format) - std::string revisionPath = cliConfigDir + "/agent-" + revision + ".conf"; - - // Check if the file exists - if (!fs::exists(revisionPath)) { - throw std::invalid_argument( - "Revision " + revision + " does not exist at " + revisionPath); + if (revision == "current") { + // Read the current live config (via the symlink or directly from cli path) + std::string content; + if (!folly::readFile(cliConfigPath.c_str(), content)) { + throw std::runtime_error( + "Failed to read current config from " + cliConfigPath); + } + return {content, "current live config"}; } - return revisionPath; + // Resolve the commit SHA and get the content from Git + std::string resolvedSha = git.resolveRef(revision); + std::string content = git.fileAtRevision(resolvedSha, "cli/agent.conf"); + return {content, revision.substr(0, 8)}; } -// Helper function to execute diff and return the result +// Helper function to execute diff on two strings and return the result std::string executeDiff( - const std::string& path1, - const std::string& path2, + const std::string& content1, + const std::string& content2, const std::string& label1, const std::string& label2) { try { + // Write content to temporary files for diff + std::string tmpFile1 = "/tmp/fboss2_diff_1_XXXXXX"; + std::string tmpFile2 = "/tmp/fboss2_diff_2_XXXXXX"; + + int fd1 = mkstemp(tmpFile1.data()); + int fd2 = mkstemp(tmpFile2.data()); + + if (fd1 < 0 || fd2 < 0) { + throw std::runtime_error("Failed to create temporary files for diff"); + } + + // Write content and close files + folly::writeFull(fd1, content1.data(), content1.size()); + folly::writeFull(fd2, content2.data(), content2.size()); + close(fd1); + close(fd2); + + // Run diff folly::Subprocess proc( std::vector{ "/usr/bin/diff", @@ -57,13 +75,17 @@ std::string executeDiff( label1, "--label", label2, - path1, - path2}, + tmpFile1, + tmpFile2}, folly::Subprocess::Options().pipeStdout().pipeStderr()); auto result = proc.communicate(); int returnCode = proc.wait().exitStatus(); + // Clean up temp files + unlink(tmpFile1.c_str()); + unlink(tmpFile2.c_str()); + // diff returns 0 if files are identical, 1 if different, 2 on error if (returnCode == 0) { return "No differences between " + label1 + " and " + label2 + "."; @@ -89,7 +111,6 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient( std::string systemConfigPath = session.getSystemConfigPath(); std::string sessionConfigPath = session.getSessionConfigPath(); - std::string cliConfigDir = session.getCliConfigDir(); // Mode 1: No arguments - diff session vs current live config if (revisions.empty()) { @@ -97,9 +118,21 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient( return "No config session exists. Make a config change first."; } + std::string currentContent; + if (!folly::readFile(systemConfigPath.c_str(), currentContent)) { + throw std::runtime_error( + "Failed to read current config from " + systemConfigPath); + } + + std::string sessionContent; + if (!folly::readFile(sessionConfigPath.c_str(), sessionContent)) { + throw std::runtime_error( + "Failed to read session config from " + sessionConfigPath); + } + return executeDiff( - systemConfigPath, - sessionConfigPath, + currentContent, + sessionContent, "current live config", "session config"); } @@ -110,28 +143,23 @@ CmdConfigSessionDiffTraits::RetType CmdConfigSessionDiff::queryClient( return "No config session exists. Make a config change first."; } - std::string revisionPath = - resolveRevisionPath(revisions[0], cliConfigDir, systemConfigPath); - std::string label = - revisions[0] == "current" ? "current live config" : revisions[0]; + auto [revContent, revLabel] = getRevisionContent(revisions[0], session); - return executeDiff( - revisionPath, sessionConfigPath, label, "session config"); + std::string sessionContent; + if (!folly::readFile(sessionConfigPath.c_str(), sessionContent)) { + throw std::runtime_error( + "Failed to read session config from " + sessionConfigPath); + } + + return executeDiff(revContent, sessionContent, revLabel, "session config"); } // Mode 3: Two arguments - diff between two revisions if (revisions.size() == 2) { - std::string path1 = - resolveRevisionPath(revisions[0], cliConfigDir, systemConfigPath); - std::string path2 = - resolveRevisionPath(revisions[1], cliConfigDir, systemConfigPath); - - std::string label1 = - revisions[0] == "current" ? "current live config" : revisions[0]; - std::string label2 = - revisions[1] == "current" ? "current live config" : revisions[1]; + auto [content1, label1] = getRevisionContent(revisions[0], session); + auto [content2, label2] = getRevisionContent(revisions[1], session); - return executeDiff(path1, path2, label1, label2); + return executeDiff(content1, content2, label1, label2); } // More than 2 arguments is an error diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index 03be1697b502e..2b09b0fd44e26 100644 --- a/fboss/cli/fboss2/session/ConfigSession.cpp +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -10,31 +10,24 @@ #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 #include #include -#include #include "fboss/agent/AgentDirectoryUtil.h" #include "fboss/cli/fboss2/gen-cpp2/cli_metadata_types.h" -#include "fboss/cli/fboss2/utils/CmdClientUtils.h" +#include "fboss/cli/fboss2/utils/CmdClientUtils.h" // NOLINT(misc-include-cleaner) #include "fboss/cli/fboss2/utils/PortMap.h" namespace fs = std::filesystem; @@ -60,8 +53,9 @@ void atomicSymlinkUpdate( // 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()); + auto now = std::chrono::system_clock::now().time_since_epoch(); + auto ns = std::chrono::duration_cast(now).count(); + std::string tmpLinkName = fmt::format("fboss2_tmp_{}_{}", getpid(), ns); fs::path tempSymlinkPath = symlinkFsPath.parent_path() / tmpLinkName; // Create new symlink with temporary name @@ -88,53 +82,6 @@ void atomicSymlinkUpdate( } } -/* - * 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()) { @@ -172,25 +119,6 @@ void ensureDirectoryExists(const std::string& dirPath) { } } -/* - * Get the current revision number by reading the symlink target. - * Returns -1 if unable to determine the current revision. - */ -int getCurrentRevisionNumber(const std::string& systemConfigPath) { - std::error_code ec; - - if (!fs::is_symlink(systemConfigPath, ec)) { - return -1; - } - - std::string target = fs::read_symlink(systemConfigPath, ec); - if (ec) { - return -1; - } - - return ConfigSession::extractRevisionNumber(target); -} - } // anonymous namespace /* @@ -222,31 +150,28 @@ std::string ConfigSession::readCommandLineFromProc() const { return folly::join(" ", args); } -ConfigSession::ConfigSession() { - username_ = getUsername(); - std::string homeDir = getHomeDirectory(); - +ConfigSession::ConfigSession() + : sessionConfigDir_(getHomeDirectory() + "/.fboss2"), + username_(getUsername()) { // 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(); + std::string coopDir = + fs::path(dirUtil.getConfigDirectory()).parent_path().string(); + systemConfigDir_ = coopDir; + git_ = std::make_unique(coopDir); initializeSession(); } ConfigSession::ConfigSession( - const std::string& sessionConfigPath, - const std::string& systemConfigPath, - const std::string& cliConfigDir) - : sessionConfigPath_(sessionConfigPath), - systemConfigPath_(systemConfigPath), - cliConfigDir_(cliConfigDir) { - username_ = getUsername(); + std::string sessionConfigDir, + std::string systemConfigDir) + : sessionConfigDir_(std::move(sessionConfigDir)), + systemConfigDir_(std::move(systemConfigDir)), + username_(getUsername()), + git_(std::make_unique(systemConfigDir_)) { initializeSession(); } @@ -270,19 +195,23 @@ void ConfigSession::setInstance(std::unique_ptr newInstance) { } std::string ConfigSession::getSessionConfigPath() const { - return sessionConfigPath_; + return sessionConfigDir_ + "/agent.conf"; } std::string ConfigSession::getSystemConfigPath() const { - return systemConfigPath_; + return systemConfigDir_ + "/agent.conf"; } std::string ConfigSession::getCliConfigDir() const { - return cliConfigDir_; + return systemConfigDir_ + "/cli"; +} + +std::string ConfigSession::getCliConfigPath() const { + return systemConfigDir_ + "/cli/agent.conf"; } bool ConfigSession::sessionExists() const { - return fs::exists(sessionConfigPath_); + return fs::exists(getSessionConfigPath()); } cfg::AgentConfig& ConfigSession::getAgentConfig() { @@ -338,7 +267,7 @@ void ConfigSession::saveConfig( // is flushed to disk before the atomic rename, preventing readers from // seeing partial/corrupted data. folly::writeFileAtomic( - sessionConfigPath_, prettyJson, 0644, folly::SyncType::WITH_SYNC); + getSessionConfigPath(), prettyJson, 0644, folly::SyncType::WITH_SYNC); // Automatically record the command from /proc/self/cmdline. // This ensures all config commands are tracked without requiring manual @@ -347,10 +276,13 @@ void ConfigSession::saveConfig( 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. - if (rawCmd.find("config ") == 0 && - (commands_.empty() || commands_.back() != rawCmd)) { - commands_.push_back(rawCmd); + // recorded as that'd be idempotent anyway. Strip any leading flags. + auto pos = rawCmd.find("config "); + if (pos != std::string::npos) { + std::string cmd = rawCmd.substr(pos); + if (commands_.empty() || commands_.back() != cmd) { + commands_.push_back(cmd); + } } // Update the required action metadata for this service @@ -360,29 +292,22 @@ void ConfigSession::saveConfig( saveMetadata(); } -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; +Git& ConfigSession::getGit() { + return *git_; +} - if (re2::RE2::FullMatch(filename, pattern, &revision)) { - return revision; - } - return -1; +const Git& ConfigSession::getGit() const { + return *git_; } std::string ConfigSession::getMetadataPath() const { // Store metadata in the same directory as session config - fs::path sessionPath(sessionConfigPath_); - return (sessionPath.parent_path() / "conf_metadata.json").string(); + return sessionConfigDir_ + "/cli_metadata.json"; +} + +std::string ConfigSession::getSystemMetadataPath() const { + // Store system metadata in the CLI config directory (Git-versioned) + return getCliConfigDir() + "/cli_metadata.json"; } std::string ConfigSession::getServiceName(cli::ServiceType service) { @@ -618,9 +543,10 @@ void ConfigSession::applyServiceActions( void ConfigSession::loadConfig() { std::string configJson; - if (!folly::readFile(sessionConfigPath_.c_str(), configJson)) { + std::string sessionConfigPath = getSessionConfigPath(); + if (!folly::readFile(sessionConfigPath.c_str(), configJson)) { throw std::runtime_error( - fmt::format("Failed to read config file: {}", sessionConfigPath_)); + fmt::format("Failed to read config file: {}", sessionConfigPath)); } apache::thrift::SimpleJSONSerializer::deserialize( @@ -636,10 +562,10 @@ void ConfigSession::loadConfig() { } void ConfigSession::initializeSession() { + initializeGit(); if (!sessionExists()) { - // Ensure the parent directory of the session config exists - fs::path sessionPath(sessionConfigPath_); - ensureDirectoryExists(sessionPath.parent_path().string()); + // Ensure the session config directory exists + ensureDirectoryExists(sessionConfigDir_); copySystemConfigToSession(); // Create initial empty metadata file for new sessions saveMetadata(); @@ -649,40 +575,37 @@ void ConfigSession::initializeSession() { } } -void ConfigSession::copySystemConfigToSession() { - // Resolve symlink if system config is a symlink - std::string sourceConfig = systemConfigPath_; - std::error_code ec; +void ConfigSession::initializeGit() { + // Initialize Git repository if it doesn't exist + if (!git_->isRepository()) { + ensureDirectoryExists(getCliConfigDir()); + git_->init(); + } - 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(); - } + // If the repository has no commits but the config file exists, + // create an initial commit to track the existing configuration + std::string cliConfigPath = getCliConfigPath(); + if (!git_->hasCommits() && fs::exists(cliConfigPath)) { + std::string systemConfigPath = getSystemConfigPath(); + git_->commit( + {cliConfigPath, systemConfigPath}, "Initial commit", username_, ""); } +} - // Read source config and write atomically to session config +void ConfigSession::copySystemConfigToSession() { + // 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. // WITH_SYNC ensures data is flushed to disk before the atomic rename. std::string configData; - if (!folly::readFile(sourceConfig.c_str(), configData)) { + std::string systemConfigPath = getSystemConfigPath(); + if (!folly::readFile(systemConfigPath.c_str(), configData)) { throw std::runtime_error( - fmt::format("Failed to read config from {}", sourceConfig)); + fmt::format("Failed to read config from {}", systemConfigPath)); } folly::writeFileAtomic( - sessionConfigPath_, configData, 0644, folly::SyncType::WITH_SYNC); + getSessionConfigPath(), configData, 0644, folly::SyncType::WITH_SYNC); } ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { @@ -691,56 +614,46 @@ ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { "No config session exists. Make a config change first."); } - ensureDirectoryExists(cliConfigDir_); + std::string cliConfigDir = getCliConfigDir(); + std::string cliConfigPath = getCliConfigPath(); + std::string sessionConfigPath = getSessionConfigPath(); + std::string systemConfigPath = getSystemConfigPath(); - // 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; + ensureDirectoryExists(cliConfigDir); - // 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) { + // Read the session config content + std::string sessionConfigData; + if (!folly::readFile(sessionConfigPath.c_str(), sessionConfigData)) { throw std::runtime_error( fmt::format( - "Failed to read symlink {}: {}", systemConfigPath_, ec.message())); + "Failed to read session config from {}", sessionConfigPath)); } - // 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())); + // Read the old config for rollback if needed + std::string oldConfigData; + if (fs::exists(cliConfigPath)) { + if (!folly::readFile(cliConfigPath.c_str(), oldConfigData)) { + throw std::runtime_error( + fmt::format("Failed to read CLI config from {}", cliConfigPath)); + } } // Copy the metadata file alongside the config revision - // e.g., agent-r123.conf -> agent-r123.metadata.json // This is required for rollback functionality std::string metadataPath = getMetadataPath(); std::string targetMetadataPath = - fmt::format("{}/agent-r{}.metadata.json", cliConfigDir_, revision); - fs::copy_file(metadataPath, targetMetadataPath, ec); + fmt::format("{}/cli_metadata.json", cliConfigDir); + std::error_code ec; + fs::copy_file( + metadataPath, + targetMetadataPath, + fs::copy_options::overwrite_existing, + ec); if (ec) { - // Clean up the revision file we created - fs::remove(targetConfigPath); + if (!oldConfigData.empty()) { + folly::writeFileAtomic( + cliConfigPath, oldConfigData, 0644, folly::SyncType::WITH_SYNC); + } throw std::runtime_error( fmt::format( "Failed to copy metadata to {}: {}", @@ -748,22 +661,42 @@ ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { ec.message())); } - // Atomically update the symlink to point to the new config - atomicSymlinkUpdate(systemConfigPath_, targetConfigPath); + // Atomically write the session config to the CLI config path + folly::writeFileAtomic( + cliConfigPath, sessionConfigData, 0644, folly::SyncType::WITH_SYNC); + + // Ensure the system config symlink points to the CLI config + atomicSymlinkUpdate(systemConfigPath, "cli/agent.conf"); // Apply the config based on the required action levels for each service // Copy requiredActions_ before we reset it - this will be returned in // CommitResult auto actions = requiredActions_; + // Apply the config based on the required action level + std::string commitSha; try { applyServiceActions(actions, hostInfo); - LOG(INFO) << "Config committed as revision r" << revision; + + // Create a Git commit with all changed files: + // - cli/agent.conf (the config file) + // - cli/cli_metadata.json (the metadata file) + // - agent.conf (the symlink, in case it was updated) + std::string commitMessage = fmt::format("Config commit by {}", username_); + commitSha = git_->commit( + {cliConfigPath, targetMetadataPath, systemConfigPath}, + commitMessage, + username_, + ""); + LOG(INFO) << "Config committed as " << commitSha.substr(0, 8); } catch (const std::exception& ex) { - // Rollback: atomically restore the old symlink, then re-apply actions + // Rollback: restore the old config, then re-apply actions // on the old config so services pick up the previous configuration try { - atomicSymlinkUpdate(systemConfigPath_, oldSymlinkTarget); + if (!oldConfigData.empty()) { + folly::writeFileAtomic( + cliConfigPath, oldConfigData, 0644, folly::SyncType::WITH_SYNC); + } applyServiceActions(actions, hostInfo); } catch (const std::exception& rollbackEx) { // If rollback also fails, include both errors in the message @@ -780,12 +713,13 @@ ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { } // Only remove the session config after everything succeeded - fs::remove(sessionConfigPath_, ec); + ec = std::error_code{}; + 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_, + sessionConfigPath, ec.message()); } @@ -794,111 +728,112 @@ ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { resetRequiredAction(service); } - return CommitResult{revision, actions}; + return CommitResult{commitSha, actions}; } -int ConfigSession::rollback(const HostInfo& hostInfo) { - // Get the current revision number - int currentRevision = getCurrentRevisionNumber(systemConfigPath_); - if (currentRevision <= 0) { - throw std::runtime_error( - "Cannot rollback: cannot determine the current revision from " + - systemConfigPath_); - } else if (currentRevision == 1) { +std::string ConfigSession::rollback(const HostInfo& hostInfo) { + // Get the commit history to find the previous commit + auto commits = git_->log(getCliConfigPath(), 2); + if (commits.size() < 2) { throw std::runtime_error( - "Cannot rollback: already at the first revision (r1)"); + "Cannot rollback: no previous revision available in Git history"); } - // Rollback to the previous revision - std::string targetRevision = "r" + std::to_string(currentRevision - 1); - return rollback(hostInfo, targetRevision); + // Rollback to the previous commit (second in the list) + return rollback(hostInfo, commits[1].sha1); } -int ConfigSession::rollback( +std::string ConfigSession::rollback( const HostInfo& hostInfo, - const std::string& revision) { - ensureDirectoryExists(cliConfigDir_); - - // Build the path to the target revision - std::string targetConfigPath = - fmt::format("{}/agent-{}.conf", cliConfigDir_, revision); - - // Check if the target revision exists - if (!fs::exists(targetConfigPath)) { - throw std::runtime_error( - fmt::format( - "Revision {} does not exist at {}", revision, targetConfigPath)); - } - - std::error_code ec; - - // Verify that the system config is a symlink - if (!fs::is_symlink(systemConfigPath_)) { - throw std::runtime_error( - fmt::format( - "{} is not a symlink. Expected it to be a symlink.", - systemConfigPath_)); + const std::string& commitSha) { + std::string cliConfigDir = getCliConfigDir(); + std::string cliConfigPath = getCliConfigPath(); + std::string systemConfigPath = getSystemConfigPath(); + + ensureDirectoryExists(cliConfigDir); + + // Resolve the commit SHA (in case it's a short SHA or ref) + std::string resolvedSha = git_->resolveRef(commitSha); + + // Get the config and metadata content from the target commit + // The paths in git are relative to the repo root + std::string targetConfigData = + git_->fileAtRevision(resolvedSha, "cli/agent.conf"); + std::string targetMetadataData = + git_->fileAtRevision(resolvedSha, "cli/cli_metadata.json"); + std::string metadataPath = fmt::format("{}/cli_metadata.json", cliConfigDir); + + // Read the current config for rollback if needed + std::string oldConfigData; + if (fs::exists(cliConfigPath)) { + if (!folly::readFile(cliConfigPath.c_str(), oldConfigData)) { + throw std::runtime_error( + fmt::format("Failed to read current config from {}", cliConfigPath)); + } } - - // Read the old symlink target in case we need to undo the rollback - std::string oldSymlinkTarget = fs::read_symlink(systemConfigPath_, ec); - if (ec) { - throw std::runtime_error( - fmt::format( - "Failed to read symlink {}: {}", systemConfigPath_, ec.message())); + std::string oldMetadataData; + if (fs::exists(metadataPath)) { + if (!folly::readFile(metadataPath.c_str(), oldMetadataData)) { + throw std::runtime_error( + fmt::format("Failed to read current metadata from {}", metadataPath)); + } } - // First, create a new revision with the same content as the target revision - auto [newRevisionPath, newRevision] = - createNextRevisionFile(fmt::format("{}/agent", cliConfigDir_)); - - // Copy the target config to the new revision file - fs::copy_file( - targetConfigPath, - newRevisionPath, - fs::copy_options::overwrite_existing, - ec); - if (ec) { - // Clean up the revision file we created - fs::remove(newRevisionPath); - throw std::runtime_error( - fmt::format( - "Failed to create new revision for rollback: {}", ec.message())); - } + // Atomically write the target config and metadata to the CLI directory + folly::writeFileAtomic( + cliConfigPath, targetConfigData, 0644, folly::SyncType::WITH_SYNC); + folly::writeFileAtomic( + metadataPath, targetMetadataData, 0644, folly::SyncType::WITH_SYNC); - // Atomically update the symlink to point to the new revision - atomicSymlinkUpdate(systemConfigPath_, newRevisionPath); + // Ensure the system config symlink points to the CLI config + atomicSymlinkUpdate(systemConfigPath, "cli/agent.conf"); - // Reload the config - if this fails, atomically undo the rollback + // Reload the config - if this fails, restore the old config and metadata // TODO: look at all the metadata files in the revision range and // decide whether or not we need to restart the agent based on the highest // action level in that range. + std::string newCommitSha; try { auto client = utils::createClient>( hostInfo); client->sync_reloadConfig(); + + // Create a Git commit for the rollback with all relevant files + std::string commitMessage = fmt::format( + "Rollback to {} by {}", resolvedSha.substr(0, 8), username_); + newCommitSha = git_->commit( + {cliConfigPath, metadataPath, systemConfigPath}, + commitMessage, + username_, + ""); + LOG(INFO) << "Rollback committed as " << newCommitSha.substr(0, 8); } catch (const std::exception& ex) { - // Rollback: atomically restore the old symlink + // Rollback: restore the old config and metadata try { - atomicSymlinkUpdate(systemConfigPath_, oldSymlinkTarget); + if (!oldConfigData.empty()) { + folly::writeFileAtomic( + cliConfigPath, oldConfigData, 0644, folly::SyncType::WITH_SYNC); + } + if (!oldMetadataData.empty()) { + folly::writeFileAtomic( + metadataPath, oldMetadataData, 0644, folly::SyncType::WITH_SYNC); + } } 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 symlink: {}", + "Failed to reload config: {}. Additionally, failed to restore the old config: {}", ex.what(), rollbackEx.what())); } throw std::runtime_error( fmt::format( - "Failed to reload config, symlink was rolled back automatically: {}", + "Failed to reload config, config was restored automatically: {}", ex.what())); } - // Successfully rolled back - LOG(INFO) << "Rollback committed as revision r" << newRevision; - return newRevision; + return newCommitSha; } } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/session/ConfigSession.h b/fboss/cli/fboss2/session/ConfigSession.h index fe4a572aa6214..f8036647cf851 100644 --- a/fboss/cli/fboss2/session/ConfigSession.h +++ b/fboss/cli/fboss2/session/ConfigSession.h @@ -14,8 +14,8 @@ #include #include #include "fboss/agent/gen-cpp2/agent_config_types.h" -#include "fboss/agent/if/gen-cpp2/ctrl_types.h" #include "fboss/cli/fboss2/gen-cpp2/cli_metadata_types.h" +#include "fboss/cli/fboss2/session/Git.h" #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fboss/cli/fboss2/utils/PortMap.h" @@ -26,8 +26,8 @@ namespace facebook::fboss { * * 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. + * configuration. It maintains a session file that can be edited and then + * atomically committed to the system configuration with Git version control. * * SINGLETON PATTERN: * ConfigSession is typically accessed via getInstance(), which currently @@ -48,35 +48,38 @@ namespace facebook::fboss { * 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 + * a. Atomically writes the session config to /etc/coop/cli/agent.conf + * b. Ensure /etc/coop/agent.conf is a symlink to /etc/coop/cli/agent.conf + * c. Creates a Git commit with the updated agent.conf and metadata + * d. Calls reloadConfig() on wedge_agent (or restarts it for + * AGENT_RESTART changes) * 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] + * 1. User runs: fboss2-dev config rollback [] * 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 + * a. Reads the target revision's agent.conf from Git history + * b. Atomically writes it to /etc/coop/cli/agent.conf + * c. Creates a new Git commit indicating the rollback + * d. Calls wedge_agent to reload the configuration (or restarts + * it if necessary) * * 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) + * - System config: /etc/coop/agent.conf (symlink to real config, Git-versioned) + * - CLI config: /etc/coop/cli/agent.conf (actual config file, Git-versioned) + * - Metadata: /etc/coop/cli/cli_metadata.json (commit metadata, Git-versioned) * - * Where is determined by AgentDirectoryUtil::getConfigDirectory() - * (typically /etc/coop, derived from the parent of the config directory path). + * VERSION CONTROL: + * The /etc/coop directory is a local Git repository. Each commit() creates + * a Git commit with the updated config. History is retrieved via git log, + * and rollback reads from Git history rather than using git revert. * * 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. + * multiple processes. */ class ConfigSession { public: @@ -90,31 +93,34 @@ class ConfigSession { // 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) + // Get the path to the system config file (/etc/coop/agent.conf symlink) std::string getSystemConfigPath() const; // Get the path to the CLI config directory (/etc/coop/cli) std::string getCliConfigDir() const; + // Get the path to the actual CLI config file (/etc/coop/cli/agent.conf) + std::string getCliConfigPath() const; + // Result of a commit operation struct CommitResult { - int revision; // The revision number that was committed + std::string commitSha; // The git commit SHA of the committed config // Maps each service to the action level that was applied during commit. // Services not in this map had no action taken. std::map actions; }; - // Atomically commit the session to /etc/coop/cli/agent-rN.conf, - // update the symlink /etc/coop/agent.conf to point to it. - // For HITLESS changes, also calls reloadConfig() on the agent. - // For AGENT_RESTART changes, does NOT call reloadConfig() - user must restart - // agent. Returns CommitResult with revision number and action level. + // Atomically commit the session to /etc/coop/cli/agent.conf and create a git + // commit. For HITLESS changes, also calls reloadConfig() on the agent. + // For AGENT_RESTART changes, restarts the agent via systemd. + // Returns CommitResult with git commit SHA and action level. CommitResult commit(const HostInfo& hostInfo); - // Rollback to a specific revision or to the previous revision - // Returns the revision that was rolled back to - int rollback(const HostInfo& hostInfo); - int rollback(const HostInfo& hostInfo, const std::string& revision); + // 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 + std::string rollback(const HostInfo& hostInfo); + std::string rollback(const HostInfo& hostInfo, const std::string& commitSha); // Check if a session exists bool sessionExists() const; @@ -133,9 +139,10 @@ class ConfigSession { // This combines saving the config and updating its associated metadata. void saveConfig(cli::ServiceType service, cli::ConfigActionLevel actionLevel); - // 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); + // Get the Git instance for this config session + // Used to access the Git repository for history, rollback, etc. + Git& getGit(); + const Git& getGit() const; // Update the required action level for the current session. // Tracks the highest action level across all config commands. @@ -159,10 +166,7 @@ class ConfigSession { protected: // Constructor for testing with custom paths - ConfigSession( - const std::string& sessionConfigPath, - const std::string& systemConfigPath, - const std::string& cliConfigDir); + ConfigSession(std::string sessionConfigDir, std::string systemConfigDir); // Set the singleton instance (for testing only) static void setInstance(std::unique_ptr instance); @@ -175,11 +179,13 @@ class ConfigSession { virtual std::string readCommandLineFromProc() const; private: - std::string sessionConfigPath_; - std::string systemConfigPath_; - std::string cliConfigDir_; + std::string sessionConfigDir_; // Typically ~/.fboss2 + std::string systemConfigDir_; // Typically /etc/coop std::string username_; + // Git instance for version control operations + std::unique_ptr git_; + // Lazy-initialized configuration and port map cfg::AgentConfig agentConfig_; std::unique_ptr portMap_; @@ -193,7 +199,10 @@ class ConfigSession { // List of commands executed in this session, persisted to disk std::vector commands_; - // Path to the metadata file (e.g., ~/.fboss2/metadata) + // Path to the system metadata file (in the Git repo) + std::string getSystemMetadataPath() const; + + // Path to the session metadata file (in the user's home directory) std::string getMetadataPath() const; // Load/save metadata (action levels and commands) from disk @@ -220,6 +229,9 @@ class ConfigSession { void initializeSession(); void copySystemConfigToSession(); void loadConfig(); + + // Initialize the Git repository if needed + void initializeGit(); }; } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/session/Git.cpp b/fboss/cli/fboss2/session/Git.cpp new file mode 100644 index 0000000000000..606cf9cb5ddab --- /dev/null +++ b/fboss/cli/fboss2/session/Git.cpp @@ -0,0 +1,312 @@ +/* + * 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/Git.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace fs = std::filesystem; + +namespace { + +// RAII class to temporarily set umask for group writeability +// This ensures that files and directories created by Git are group-writable. +class ScopedUmask { + public: + explicit ScopedUmask(mode_t newMask) { + oldMask_ = umask(newMask); + } + ~ScopedUmask() { + umask(oldMask_); + } + ScopedUmask(const ScopedUmask&) = delete; + ScopedUmask& operator=(const ScopedUmask&) = delete; + ScopedUmask(ScopedUmask&&) = delete; + ScopedUmask& operator=(ScopedUmask&&) = delete; + + private: + mode_t oldMask_; +}; + +// RAII class to acquire an exclusive flock on a file. +// The lock is automatically released when the file descriptor is closed, +// which also happens when the process exits (even on SIGKILL or crash). +class ScopedFileLock { + public: + explicit ScopedFileLock(const std::string& lockPath) { + fd_ = open(lockPath.c_str(), O_CREAT | O_RDWR, 0664); + if (fd_ < 0) { + throw std::runtime_error( + fmt::format( + "Failed to open lock file {}: {}", + lockPath, + folly::errnoStr(errno))); + } + if (flock(fd_, LOCK_EX) < 0) { + int savedErrno = errno; + close(fd_); + throw std::runtime_error( + fmt::format( + "Failed to acquire lock on {}: {}", + lockPath, + folly::errnoStr(savedErrno))); + } + } + ~ScopedFileLock() { + if (fd_ >= 0) { + flock(fd_, LOCK_UN); + close(fd_); + } + } + ScopedFileLock(const ScopedFileLock&) = delete; + ScopedFileLock& operator=(const ScopedFileLock&) = delete; + ScopedFileLock(ScopedFileLock&&) = delete; + ScopedFileLock& operator=(ScopedFileLock&&) = delete; + + private: + int fd_ = -1; +}; + +// Result of running a git command. +struct CommandResult { + std::string stdoutStr; + std::string stderrStr; + int exitStatus; +}; + +CommandResult runGitCommandWithStatus( + const std::string& repoPath, + const std::vector& args) { + // Use full path to git to avoid PATH issues in test environments + // Pass -c safe.directory= to handle cases where the repository + // is owned by a different user (e.g., /etc/coop owned by root) + std::vector fullArgs = { + "/usr/bin/git", "-c", "safe.directory=" + repoPath, "-C", repoPath}; + fullArgs.insert(fullArgs.end(), args.begin(), args.end()); + + try { + folly::Subprocess proc( + fullArgs, folly::Subprocess::Options().pipeStdout().pipeStderr()); + + auto output = proc.communicate(); + int exitStatus = proc.wait().exitStatus(); + + return {output.first, output.second, exitStatus}; + } catch (const std::exception& ex) { + throw std::runtime_error( + fmt::format("Failed to execute git command: {}", ex.what())); + } +} + +} // namespace + +namespace facebook::fboss { + +Git::Git(std::string repoPath) : repoPath_(std::move(repoPath)) {} + +std::string Git::getRepoPath() const { + return repoPath_; +} + +bool Git::isRepository() const { + auto result = runGitCommandWithStatus(repoPath_, {"rev-parse", "--git-dir"}); + return result.exitStatus == 0; +} + +void Git::init(const std::string& initialBranch) { + if (isRepository()) { + return; // Already a repository + } + + // Set umask to allow group write access (0002 means: keep all bits except + // "other write"). This ensures .git directory and its contents are + // group-writable when /etc/coop is group-writable (e.g., owned by root:admin) + ScopedUmask scopedUmask(0002); + + // Create the directory if it doesn't exist + std::error_code ec; + fs::create_directories(repoPath_, ec); + if (ec) { + throw std::runtime_error( + fmt::format( + "Failed to create directory {}: {}", repoPath_, ec.message())); + } + + // Initialize the repository with the specified initial branch + // Use --shared=group to make the repository group-writable + runGitCommand( + {"init", "--initial-branch=" + initialBranch, "--shared=group"}); + + // Configure user.name and user.email locally to avoid git config issues + // Use "fboss-cli" as the default identity for automated commits + runGitCommand({"config", "user.name", "fboss-cli"}); + runGitCommand({"config", "user.email", "fboss-cli@localhost"}); +} + +std::string Git::commit( + const std::vector& files, + const std::string& message, + const std::string& authorName, + const std::string& authorEmail) { + if (files.empty()) { + throw std::runtime_error("No files specified for commit"); + } + if (message.empty()) { + throw std::runtime_error("Commit message cannot be empty"); + } + + // Acquire an exclusive lock to serialize concurrent commits. + // This prevents race conditions where two processes could interleave + // their git add and git commit operations. + std::string lockPath = repoPath_ + "/.git/fboss2-commit.lock"; + ScopedFileLock lock(lockPath); + + // First, add the files to the index + // This handles both tracked and untracked files + std::vector addArgs = {"add", "--"}; + for (const auto& file : files) { + addArgs.push_back(file); + } + runGitCommand(addArgs); + + // Build the commit command + std::vector args = {"commit", "-m", message}; + + // If author is specified, use --author + if (!authorName.empty() || !authorEmail.empty()) { + std::string author = fmt::format( + "{} <{}>", + authorName.empty() ? "fboss-cli" : authorName, + authorEmail.empty() ? "fboss-cli@localhost" : authorEmail); + args.push_back("--author=" + author); + } + + runGitCommand(args); + + // Return the SHA of the new commit + return getHead(); +} + +std::vector Git::log(const std::string& filePath, size_t limit) + const { + // Use a custom format with null byte separators to parse reliably + // Format: SHA1, author_name, author_email, timestamp, subject + // Use %s (subject) instead of %B (body) to get just the first line + std::vector args = { + "log", "--format=%H%x00%an%x00%ae%x00%at%x00%s%x00", "--", filePath}; + + if (limit > 0) { + args.insert(args.begin() + 1, "-n"); + args.insert(args.begin() + 2, std::to_string(limit)); + } + + auto result = runGitCommandWithStatus(repoPath_, args); + if (result.exitStatus != 0) { + // No commits or file not in repo + return {}; + } + + std::vector commits; + + // Split output by null characters + std::vector parts; + folly::split('\0', result.stdoutStr, parts); + + // Each commit has 5 fields, so process in groups of 5 + // The last empty part after final \0 is ignored + for (size_t i = 0; i + 4 < parts.size(); i += 5) { + GitCommit commit; + + // Trim all fields - git log output can have newlines between commits + commit.sha1 = folly::trimWhitespace(parts[i]).str(); + commit.authorName = folly::trimWhitespace(parts[i + 1]).str(); + commit.authorEmail = folly::trimWhitespace(parts[i + 2]).str(); + + std::string timestampStr = folly::trimWhitespace(parts[i + 3]).str(); + if (timestampStr.empty()) { // Should never happen. + throw std::runtime_error( + fmt::format( + "Git log returned empty timestamp for commit {}", commit.sha1)); + } + commit.timestamp = std::stoll(timestampStr); + + commit.subject = folly::trimWhitespace(parts[i + 4]).str(); + + // Skip empty commits (can happen if there are extra null separators) + if (commit.sha1.empty()) { + continue; + } + + commits.push_back(std::move(commit)); + } + + return commits; +} + +std::string Git::fileAtRevision( + const std::string& revision, + const std::string& filePath) const { + // Use git show to get file contents at a specific revision + std::string ref = fmt::format("{}:{}", revision, filePath); + return runGitCommand({"show", ref}); +} + +std::string Git::resolveRef(const std::string& ref) const { + return runGitCommand({"rev-parse", folly::trimWhitespace(ref).str()}); +} + +std::string Git::getHead() const { + auto result = runGitCommandWithStatus(repoPath_, {"rev-parse", "HEAD"}); + if (result.exitStatus != 0) { + return ""; // No commits yet + } + return folly::trimWhitespace(result.stdoutStr).str(); +} + +bool Git::hasCommits() const { + // Check if HEAD can be resolved - if not, there are no commits + auto result = runGitCommandWithStatus(repoPath_, {"rev-parse", "HEAD"}); + return result.exitStatus == 0; +} + +std::string Git::runGitCommand(const std::vector& args) const { + auto result = runGitCommandWithStatus(repoPath_, args); + if (result.exitStatus != 0) { + std::string cmd = "git"; + for (const auto& arg : args) { + cmd += " " + arg; + } + throw std::runtime_error( + fmt::format( + "Git command failed: {} (exit {}): {}", + cmd, + result.exitStatus, + folly::trimWhitespace(result.stderrStr))); + } + return folly::trimWhitespace(result.stdoutStr).str(); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/session/Git.h b/fboss/cli/fboss2/session/Git.h new file mode 100644 index 0000000000000..8e59e1b28723d --- /dev/null +++ b/fboss/cli/fboss2/session/Git.h @@ -0,0 +1,146 @@ +/* + * 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 + +namespace facebook::fboss { + +/** + * Represents a single commit in the Git log. + */ +struct GitCommit { + std::string sha1; // Full 40-character SHA1 hash + std::string authorName; + std::string authorEmail; + int64_t timestamp; // Unix timestamp in seconds + std::string subject; // First line of commit message +}; + +/** + * Git provides a simple interface for Git operations in a local repository. + * + * This class is designed to be: + * 1. Easy to use for common operations like init, commit, log, and show + * 2. Easy to mock for unit tests + * 3. Thread-safe for concurrent usage (all operations are atomic) + * + * The commit() method uses git add followed by git commit. This two-step + * process is required to handle both tracked and untracked files, as git + * commit --include only works for already-tracked files. + */ +class Git { + public: + /** + * Create a Git instance for the specified repository path. + * @param repoPath The path to the Git repository (or where to create one) + */ + explicit Git(std::string repoPath); + ~Git() = default; + + // Non-copyable and non-movable + Git(const Git&) = delete; + Git& operator=(const Git&) = delete; + Git(Git&&) = delete; + Git& operator=(Git&&) = delete; + + /** + * Get the repository path. + */ + std::string getRepoPath() const; + + /** + * Check if the repository path is already a Git repository. + */ + bool isRepository() const; + + /** + * Initialize a new Git repository if one doesn't exist. + * If the repository already exists, this is a no-op. + * @param initialBranch The name of the initial branch (default: "main") + */ + void init(const std::string& initialBranch = "main"); + + /** + * Commit the specified files with the given message. + * This uses git commit --include to add files without using git add. + * + * @param files List of file paths (relative to repo root) to commit + * @param message Commit message + * @param authorName Author name (optional, uses system default if empty) + * @param authorEmail Author email (optional, uses system default if empty) + * @return The SHA1 of the new commit + * @throws std::runtime_error if the commit fails + */ + std::string commit( + const std::vector& files, + const std::string& message, + const std::string& authorName = "", + const std::string& authorEmail = ""); + + /** + * Get the commit log for a specific file, optionally limited to N entries. + * + * @param filePath Path to the file (relative to repo root) + * @param limit Maximum number of commits to return (0 = no limit) + * @return Vector of GitCommit objects, most recent first + */ + std::vector log(const std::string& filePath, size_t limit = 0) + const; + + /** + * Get the contents of a file at a specific revision. + * + * @param revision A Git revision: commit SHA (full or abbreviated), + * branch name, tag name, or other git syntax (e.g. HEAD~4) + * @param filePath Path to the file (relative to repo root) + * @return The file contents at that revision + * @throws std::runtime_error if the file or revision doesn't exist + */ + std::string fileAtRevision( + const std::string& revision, + const std::string& filePath) const; + + /** + * Get the full SHA for a commit reference (SHA, HEAD, branch name, etc). + * + * @param ref Git reference (commit SHA, HEAD, branch name, etc) + * @return Full SHA of the commit + * @throws std::runtime_error if the reference is invalid + */ + std::string resolveRef(const std::string& ref) const; + + /** + * Get the current HEAD commit SHA. + * @return Full SHA of HEAD, or empty string if repo has no commits + */ + std::string getHead() const; + + /** + * Check if the repository has any commits. + * @return true if the repository has at least one commit, false otherwise + */ + bool hasCommits() const; + + private: + /** + * Execute a git command and return its output. + * @param args Arguments to pass to git (not including 'git' itself) + * @return The stdout output of the command + * @throws std::runtime_error if the command fails + */ + std::string runGitCommand(const std::vector& args) const; + + std::string repoPath_; +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/BUCK b/fboss/cli/fboss2/test/BUCK index a9e689d9f839e..2ac5af3a57967 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -96,6 +96,7 @@ cpp_unittest( "CmdShowTransceiverTest.cpp", "CmdStartPcapTest.cpp", "CmdStopPcapTest.cpp", + "GitTest.cpp", "PortMapTest.cpp", ], # Config files for PortMapTest parameterized tests diff --git a/fboss/cli/fboss2/test/GitTest.cpp b/fboss/cli/fboss2/test/GitTest.cpp new file mode 100644 index 0000000000000..2f09a6f3e5ce1 --- /dev/null +++ b/fboss/cli/fboss2/test/GitTest.cpp @@ -0,0 +1,125 @@ +/* + * 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 +#include + +#include "fboss/cli/fboss2/session/Git.h" + +namespace fs = std::filesystem; + +namespace facebook::fboss { + +class GitTest : public ::testing::Test { + protected: + void SetUp() override { + // Create a unique temporary directory for each test + auto tempBase = fs::temp_directory_path(); + auto uniquePath = + boost::filesystem::unique_path("git_test_%%%%-%%%%-%%%%-%%%%"); + testRepoPath_ = tempBase / uniquePath.string(); + fs::create_directories(testRepoPath_); + } + + void TearDown() override { + // Clean up test directory + std::error_code ec; + if (fs::exists(testRepoPath_)) { + fs::remove_all(testRepoPath_, ec); + } + } + + void writeFile(const std::string& filename, const std::string& content) { + if (!folly::writeFile(content, (testRepoPath_ / filename).c_str())) { + throw std::runtime_error( + fmt::format( + "Failed to write file {}: {}", filename, folly::errnoStr(errno))); + } + } + + std::string readFile(const std::string& filename) { + std::string content; + if (!folly::readFile((testRepoPath_ / filename).c_str(), content)) { + throw std::runtime_error( + fmt::format( + "Failed to read file {}: {}", filename, folly::errnoStr(errno))); + } + return content; + } + + fs::path testRepoPath_; +}; + +TEST_F(GitTest, BasicOperations) { + Git git(testRepoPath_.string()); + + // Before init + EXPECT_FALSE(git.isRepository()); + + // After init + git.init(); + EXPECT_TRUE(git.isRepository()); + EXPECT_TRUE(fs::exists(testRepoPath_ / ".git")); + EXPECT_FALSE(git.hasCommits()); + EXPECT_TRUE(git.getHead().empty()); + + // First commit + writeFile("config.txt", "version 1"); + std::string sha1First = + git.commit({"config.txt"}, "Version 1", "User", "user@test.com"); + EXPECT_FALSE(sha1First.empty()); + EXPECT_EQ(40, sha1First.length()); // SHA1 is 40 hex characters + EXPECT_TRUE(git.hasCommits()); + EXPECT_EQ(sha1First, git.getHead()); + + // Check log after first commit + auto commits = git.log("config.txt"); + ASSERT_EQ(1, commits.size()); + EXPECT_EQ(sha1First, commits[0].sha1); + EXPECT_EQ("User", commits[0].authorName); + EXPECT_EQ("user@test.com", commits[0].authorEmail); + EXPECT_EQ("Version 1", commits[0].subject); + EXPECT_GT(commits[0].timestamp, 0); + + // Second commit - modify the same file + writeFile("config.txt", "version 2"); + std::string sha1Second = + git.commit({"config.txt"}, "Version 2", "User", "user@test.com"); + EXPECT_EQ(sha1Second, git.getHead()); + + // Check log with no limit (should return both commits, most recent first) + commits = git.log("config.txt"); + ASSERT_EQ(2, commits.size()); + EXPECT_EQ(sha1Second, commits[0].sha1); + EXPECT_EQ(sha1First, commits[1].sha1); + + // Check log with limit of 1 (should return only most recent) + auto limitedCommits = git.log("config.txt", 1); + ASSERT_EQ(1, limitedCommits.size()); + EXPECT_EQ(sha1Second, limitedCommits[0].sha1); + + // Retrieve file content from first commit + std::string contentAtFirst = git.fileAtRevision(sha1First, "config.txt"); + EXPECT_EQ("version 1", contentAtFirst); + + // Retrieve file content from second commit + std::string contentAtSecond = git.fileAtRevision(sha1Second, "config.txt"); + EXPECT_EQ("version 2", contentAtSecond); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/TestableConfigSession.h b/fboss/cli/fboss2/test/TestableConfigSession.h index 87e9fa90ec78c..0ffa469d43461 100644 --- a/fboss/cli/fboss2/test/TestableConfigSession.h +++ b/fboss/cli/fboss2/test/TestableConfigSession.h @@ -10,6 +10,9 @@ #pragma once +#include +#include + #include "fboss/cli/fboss2/session/ConfigSession.h" namespace facebook::fboss { @@ -19,10 +22,10 @@ namespace facebook::fboss { class TestableConfigSession : public ConfigSession { public: TestableConfigSession( - const std::string& sessionConfigPath, - const std::string& systemConfigPath, - const std::string& cliConfigDir) - : ConfigSession(sessionConfigPath, systemConfigPath, cliConfigDir) {} + std::string sessionConfigDir, + std::string systemConfigDir) + : ConfigSession(std::move(sessionConfigDir), std::move(systemConfigDir)) { + } // Expose protected setInstance() for testing using ConfigSession::setInstance; diff --git a/fboss/cli/fboss2/test/config/CmdConfigHistoryTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigHistoryTest.cpp index 41613852f8797..c177d16300349 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigHistoryTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigHistoryTest.cpp @@ -34,57 +34,72 @@ class CmdConfigHistoryTestFixture : public CmdConfigTestBase { }; TEST_F(CmdConfigHistoryTestFixture, historyListsRevisions) { - // Create revision files with valid config content - createTestConfig(getCliConfigDir() / "agent-r1.conf", initialConfigContents); - createTestConfig(getCliConfigDir() / "agent-r2.conf", initialConfigContents); - createTestConfig(getCliConfigDir() / "agent-r3.conf", initialConfigContents); + std::filesystem::path cliConfigPath = getCliConfigDir() / "agent.conf"; + + // Second commit + createTestConfig( + cliConfigPath, + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 200000}]}})"); + git().commit({"cli/agent.conf"}, "Second commit"); + + // Third commit + createTestConfig( + cliConfigPath, + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 300000}]}})"); + git().commit({"cli/agent.conf"}, "Third commit"); - // Initialize ConfigSession singleton with test paths setupTestableConfigSession(); - // Create and execute the command auto cmd = CmdConfigHistory(); auto result = cmd.queryClient(localhost()); - // Verify the output contains all three revisions - EXPECT_NE(result.find("r1"), std::string::npos); - EXPECT_NE(result.find("r2"), std::string::npos); - EXPECT_NE(result.find("r3"), std::string::npos); - - // Verify table headers are present - EXPECT_NE(result.find("Revision"), std::string::npos); - EXPECT_NE(result.find("Owner"), std::string::npos); + EXPECT_NE(result.find("Initial commit"), std::string::npos); + EXPECT_NE(result.find("Second commit"), std::string::npos); + EXPECT_NE(result.find("Third commit"), std::string::npos); + EXPECT_NE(result.find("Commit"), std::string::npos); + EXPECT_NE(result.find("Author"), std::string::npos); EXPECT_NE(result.find("Commit Time"), std::string::npos); + EXPECT_NE(result.find("Message"), std::string::npos); + + // Verify the timestamp is formatted correctly (not epoch). + // Git returns Unix timestamps in seconds, so if the code incorrectly + // treats them as nanoseconds, we'd see dates near the Unix epoch. + // Depending on timezone, epoch could show as 1970-01-01 or 1969-12-31. + EXPECT_EQ(result.find("1970-"), std::string::npos) + << "Timestamp appears to be incorrectly parsed (showing 1970 epoch)"; + EXPECT_EQ(result.find("1969-"), std::string::npos) + << "Timestamp appears to be incorrectly parsed (showing 1969 epoch)"; + // Check that the current year appears in the output + std::time_t now = std::time(nullptr); + std::tm tm{}; + localtime_r(&now, &tm); + std::string currentYear = std::to_string(1900 + tm.tm_year); + EXPECT_NE(result.find(currentYear + "-"), std::string::npos) + << "Expected timestamp with year " << currentYear << ", got: " << result; } TEST_F(CmdConfigHistoryTestFixture, historyIgnoresNonMatchingFiles) { - // Create valid revision files - createTestConfig(getCliConfigDir() / "agent-r1.conf", initialConfigContents); - createTestConfig(getCliConfigDir() / "agent-r2.conf", initialConfigContents); + std::filesystem::path cliConfigPath = getCliConfigDir() / "agent.conf"; - // Create files that should be ignored - createTestConfig(getCliConfigDir() / "agent.conf.bak", R"({"backup": true})"); - createTestConfig(getCliConfigDir() / "other-r1.conf", R"({"other": true})"); + // Second commit for cli/agent.conf createTestConfig( - getCliConfigDir() / "agent-r1.txt", R"({"wrong_ext": true})"); - createTestConfig(getCliConfigDir() / "agent-rX.conf", R"({"invalid": true})"); + cliConfigPath, + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 200000}]}})"); + git().commit({"cli/agent.conf"}, "Config update"); + + // Create and commit a different file (should not appear in history) + createTestConfig(getTestEtcDir() / "coop" / "other.txt", "other content"); + git().commit({"other.txt"}, "Other file commit"); - // Initialize ConfigSession singleton with test paths setupTestableConfigSession(); - // Create and execute the command auto cmd = CmdConfigHistory(); auto result = cmd.queryClient(localhost()); - // Verify only valid revisions are listed - EXPECT_NE(result.find("r1"), std::string::npos); - EXPECT_NE(result.find("r2"), std::string::npos); - - // Verify invalid files are not listed - EXPECT_EQ(result.find("agent.conf.bak"), std::string::npos); - EXPECT_EQ(result.find("other-r1.conf"), std::string::npos); - EXPECT_EQ(result.find("agent-r1.txt"), std::string::npos); - EXPECT_EQ(result.find("rX"), std::string::npos); + EXPECT_NE(result.find("Initial commit"), std::string::npos); + EXPECT_NE(result.find("Config update"), std::string::npos); + // The "Other file commit" should not appear since it doesn't touch agent.conf + EXPECT_EQ(result.find("Other file commit"), std::string::npos); } TEST_F(CmdConfigHistoryTestFixture, historyEmptyDirectory) { @@ -101,17 +116,11 @@ TEST_F(CmdConfigHistoryTestFixture, historyEmptyDirectory) { auto cmd = CmdConfigHistory(); auto result = cmd.queryClient(localhost()); - // Verify the output indicates no revisions found - EXPECT_NE(result.find("No config revisions found"), std::string::npos); - EXPECT_NE(result.find(getCliConfigDir().string()), std::string::npos); + // Verify the output indicates only an initial commit + EXPECT_NE(result.find("Initial commit"), std::string::npos); } -TEST_F(CmdConfigHistoryTestFixture, historyNonSequentialRevisions) { - // Create non-sequential revision files (e.g., after deletions) - createTestConfig(getCliConfigDir() / "agent-r1.conf", initialConfigContents); - createTestConfig(getCliConfigDir() / "agent-r5.conf", initialConfigContents); - createTestConfig(getCliConfigDir() / "agent-r10.conf", initialConfigContents); - +TEST_F(CmdConfigHistoryTestFixture, historyShowsCommitShas) { // Initialize ConfigSession singleton with test paths setupTestableConfigSession(); @@ -119,23 +128,62 @@ TEST_F(CmdConfigHistoryTestFixture, historyNonSequentialRevisions) { auto cmd = CmdConfigHistory(); auto result = cmd.queryClient(localhost()); - // Verify all revisions are listed in order - EXPECT_NE(result.find("r1"), std::string::npos); - EXPECT_NE(result.find("r5"), std::string::npos); - EXPECT_NE(result.find("r10"), std::string::npos); - - // Verify they appear in ascending order (r1 before r5 before r10) - auto pos_r1 = result.find("r1"); - auto pos_r5 = result.find("r5"); - auto pos_r10 = result.find("r10"); - EXPECT_LT(pos_r1, pos_r5); - EXPECT_LT(pos_r5, pos_r10); + // Verify the output contains a commit SHA (8 hex characters) + // The SHA should be in the first column + bool foundSha = false; + for (size_t i = 0; i + 7 < result.size(); ++i) { + bool isHex = true; + for (size_t j = 0; j < 8 && isHex; ++j) { + char c = result[i + j]; + if (!std::isxdigit(c)) { + isHex = false; + } + } + if (isHex) { + foundSha = true; + break; + } + } + EXPECT_TRUE(foundSha) << "Expected to find a commit SHA in the output"; +} + +TEST_F(CmdConfigHistoryTestFixture, historyMultipleCommits) { + std::filesystem::path cliConfigPath = getCliConfigDir() / "agent.conf"; + + // Create 5 more commits + for (int i = 2; i <= 6; ++i) { + createTestConfig( + cliConfigPath, + fmt::format( + R"({{"sw": {{"ports": [{{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": {}}}]}}}})", + i * 100000)); + git().commit({"cli/agent.conf"}, fmt::format("Commit {}", i)); + } + + setupTestableConfigSession(); + + auto cmd = CmdConfigHistory(); + auto result = cmd.queryClient(localhost()); + + EXPECT_NE(result.find("Commit 6"), std::string::npos); + EXPECT_NE(result.find("Commit 5"), std::string::npos); + EXPECT_NE(result.find("Commit 4"), std::string::npos); + EXPECT_NE(result.find("Commit 3"), std::string::npos); + EXPECT_NE(result.find("Commit 2"), std::string::npos); + EXPECT_NE(result.find("Initial commit"), std::string::npos); + + // Verify they appear in reverse chronological order (most recent first) + auto pos_6 = result.find("Commit 6"); + auto pos_5 = result.find("Commit 5"); + auto pos_initial = result.find("Initial commit"); + EXPECT_LT(pos_6, pos_5); + EXPECT_LT(pos_5, pos_initial); } TEST_F(CmdConfigHistoryTestFixture, printOutput) { auto cmd = CmdConfigHistory(); std::string tableOutput = - "Revision Owner Commit Time\nr1 user1 2024-01-01 12:00:00.000"; + "Commit Author Commit Time Message\nabcd1234 user1 2024-01-01 12:00:00 Initial commit"; // Redirect cout to capture output std::stringstream buffer; @@ -148,8 +196,8 @@ TEST_F(CmdConfigHistoryTestFixture, printOutput) { std::string output = buffer.str(); - EXPECT_NE(output.find("Revision"), std::string::npos); - EXPECT_NE(output.find("r1"), std::string::npos); + EXPECT_NE(output.find("Commit"), std::string::npos); + EXPECT_NE(output.find("abcd1234"), std::string::npos); EXPECT_NE(output.find("user1"), std::string::npos); } diff --git a/fboss/cli/fboss2/test/config/CmdConfigInterfaceDescriptionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigInterfaceDescriptionTest.cpp index a5c41f9541281..38ac17a82a23e 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigInterfaceDescriptionTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigInterfaceDescriptionTest.cpp @@ -42,7 +42,6 @@ class CmdConfigInterfaceDescriptionTestFixture : public CmdConfigTestBase { void SetUp() override { CmdConfigTestBase::SetUp(); - setupTestableConfigSession("config interface eth1/1/1 description", "test"); } }; diff --git a/fboss/cli/fboss2/test/config/CmdConfigSessionDiffTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigSessionDiffTest.cpp index ef8657000dbfe..87fc6505a8a1d 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigSessionDiffTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigSessionDiffTest.cpp @@ -97,9 +97,10 @@ TEST_F(CmdConfigSessionDiffTestFixture, diffDifferentConfigs) { TEST_F(CmdConfigSessionDiffTestFixture, diffSessionVsRevision) { setupTestableConfigSession(); - // Create a revision file + std::filesystem::path cliConfigPath = getCliConfigDir() / "agent.conf"; + // Create a commit with state: 1 createTestConfig( - getCliConfigDir() / "agent-r1.conf", + cliConfigPath, R"({ "sw": { "ports": [ @@ -112,6 +113,8 @@ TEST_F(CmdConfigSessionDiffTestFixture, diffSessionVsRevision) { } })"); + std::string commitSha = git().commit({cliConfigPath.string()}, "State 1"); + // Create session with different content std::filesystem::create_directories(getSessionConfigPath().parent_path()); createTestConfig( @@ -129,11 +132,12 @@ TEST_F(CmdConfigSessionDiffTestFixture, diffSessionVsRevision) { })"); auto cmd = CmdConfigSessionDiff(); - utils::RevisionList revisions(std::vector{"r1"}); + utils::RevisionList revisions(std::vector{commitSha}); auto result = cmd.queryClient(localhost(), revisions); - // Should show a diff between r1 and session (state changed from 1 to 2) + // Should show a diff between the commit and session (state changed from 1 to + // 2) EXPECT_NE(result.find("- \"state\": 1"), std::string::npos); EXPECT_NE(result.find("+ \"state\": 2"), std::string::npos); } @@ -141,9 +145,10 @@ TEST_F(CmdConfigSessionDiffTestFixture, diffSessionVsRevision) { TEST_F(CmdConfigSessionDiffTestFixture, diffTwoRevisions) { setupTestableConfigSession(); - // Create two different revision files + std::filesystem::path cliConfigPath = getCliConfigDir() / "agent.conf"; + // Create first commit with state: 2 createTestConfig( - getCliConfigDir() / "agent-r1.conf", + cliConfigPath, R"({ "sw": { "ports": [ @@ -155,9 +160,11 @@ TEST_F(CmdConfigSessionDiffTestFixture, diffTwoRevisions) { ] } })"); + std::string commit1 = git().commit({cliConfigPath.string()}, "Commit 1"); + // Create second commit with description added createTestConfig( - getCliConfigDir() / "agent-r2.conf", + cliConfigPath, R"({ "sw": { "ports": [ @@ -170,9 +177,12 @@ TEST_F(CmdConfigSessionDiffTestFixture, diffTwoRevisions) { ] } })"); + std::string commit2 = git().commit({cliConfigPath.string()}, "Commit 2"); + + setupTestableConfigSession(); auto cmd = CmdConfigSessionDiff(); - utils::RevisionList revisions(std::vector{"r1", "r2"}); + utils::RevisionList revisions(std::vector{commit1, commit2}); auto result = cmd.queryClient(localhost(), revisions); @@ -186,9 +196,10 @@ TEST_F(CmdConfigSessionDiffTestFixture, diffWithCurrentKeyword) { removeInitialRevisionFile(); setupTestableConfigSession(); - // Create a revision file + std::filesystem::path cliConfigPath = getCliConfigDir() / "agent.conf"; + // Create a commit with state: 1 createTestConfig( - getCliConfigDir() / "agent-r1.conf", + cliConfigPath, R"({ "sw": { "ports": [ @@ -200,14 +211,34 @@ TEST_F(CmdConfigSessionDiffTestFixture, diffWithCurrentKeyword) { ] } })"); + std::string commit1 = git().commit({cliConfigPath.string()}, "State 1"); + + // Update system config to state: 2 with speed (this is "current") + createTestConfig( + cliConfigPath, + R"({ + "sw": { + "ports": [ + { + "logicalID": 1, + "name": "eth1/1/1", + "state": 2, + "speed": 100000 + } + ] + } +})"); + git().commit({cliConfigPath.string()}, "Current state"); + + setupTestableConfigSession(); auto cmd = CmdConfigSessionDiff(); - utils::RevisionList revisions(std::vector{"r1", "current"}); + utils::RevisionList revisions(std::vector{commit1, "current"}); auto result = cmd.queryClient(localhost(), revisions); - // Should show a diff between r1 and current system config (state changed from - // 1 to 2, speed added) + // Should show a diff between commit1 and current system config (state changed + // from 1 to 2, speed added) EXPECT_NE(result.find("- \"state\": 1"), std::string::npos); EXPECT_NE(result.find("+ \"state\": 2"), std::string::npos); EXPECT_NE(result.find("+ \"speed\": 100000"), std::string::npos); @@ -217,17 +248,25 @@ TEST_F(CmdConfigSessionDiffTestFixture, diffNonexistentRevision) { setupTestableConfigSession(); auto cmd = CmdConfigSessionDiff(); - utils::RevisionList revisions(std::vector{"r999"}); + // Use a fake SHA that doesn't exist + utils::RevisionList revisions( + std::vector{"0000000000000000000000000000000000000000"}); // Should throw an exception for nonexistent revision - EXPECT_THROW(cmd.queryClient(localhost(), revisions), std::invalid_argument); + // Git throws runtime_error when the commit doesn't exist + EXPECT_THROW(cmd.queryClient(localhost(), revisions), std::runtime_error); } TEST_F(CmdConfigSessionDiffTestFixture, diffTooManyArguments) { setupTestableConfigSession(); auto cmd = CmdConfigSessionDiff(); - utils::RevisionList revisions(std::vector{"r1", "r2", "r3"}); + // Use fake SHAs for the test + utils::RevisionList revisions( + std::vector{ + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "cccccccccccccccccccccccccccccccccccccccc"}); // Should throw an exception for too many arguments EXPECT_THROW(cmd.queryClient(localhost(), revisions), std::invalid_argument); diff --git a/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp index 09cb11c61776a..2f0a381cbae20 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp @@ -1,25 +1,73 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. - -#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" - +/* + * 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 +#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" +#include "fboss/cli/fboss2/utils/PortMap.h" // NOLINT(misc-include-cleaner) namespace fs = std::filesystem; namespace facebook::fboss { -class ConfigSessionTestFixture : public CmdConfigTestBase { +class ConfigSessionTestFixture : public CmdHandlerTestBase { public: - ConfigSessionTestFixture() - : CmdConfigTestBase( - "fboss_test_%%%%-%%%%-%%%%-%%%%", - R"({ + 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"({ "sw": { "ports": [ { @@ -30,48 +78,86 @@ class ConfigSessionTestFixture : public CmdConfigTestBase { } ] } -})") {} +})"); + + // 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 = getTestHomeDir() / ".fboss2"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; EXPECT_FALSE(fs::exists(sessionDir)); // Creating a ConfigSession should create the directory and copy the config - // getSystemConfigPath() is already a symlink created in SetUp() - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.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 getSystemConfigPath() points to - // fs::read_symlink returns a relative path, so we need to resolve it - fs::path symlinkTarget = fs::read_symlink(getSystemConfigPath()); - fs::path actualConfigPath = - getSystemConfigPath().parent_path() / symlinkTarget; - std::string systemContent = readFile(actualConfigPath); + // Verify content was copied correctly (reads via symlink) + std::string systemContent = readFile(cliConfigPath); std::string sessionContent = readFile(sessionConfig); EXPECT_EQ(systemContent, sessionContent); } TEST_F(ConfigSessionTestFixture, sessionConfigModified) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; // Create a ConfigSession - // getSystemConfigPath() is already a symlink created in SetUp() - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); // Modify the session config through the ConfigSession API auto& config = session.getAgentConfig(); @@ -83,45 +169,37 @@ TEST_F(ConfigSessionTestFixture, sessionConfigModified) { // 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(getSystemConfigPath()); - fs::path actualConfigPath = - getSystemConfigPath().parent_path() / symlinkTarget; - std::string systemContent = readFile(actualConfigPath); + std::string systemContent = readFile(cliConfigPath); EXPECT_NE(sessionContent, systemContent); EXPECT_THAT(sessionContent, ::testing::HasSubstr("Modified port")); } TEST_F(ConfigSessionTestFixture, sessionCommit) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path cliConfigDir = getTestEtcDir() / "coop" / "cli"; - - // Verify old symlink exists (created in SetUp) - EXPECT_TRUE(fs::is_symlink(getSystemConfigPath())); - EXPECT_EQ( - fs::read_symlink(getSystemConfigPath()), cliConfigDir / "agent-r1.conf"); + fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; // Setup mock agent server setupMockedAgentServer(); EXPECT_CALL(getMockAgent(), reloadConfig()).Times(2); + std::string firstCommitSha; + std::string secondCommitSha; + // First commit: Create a ConfigSession and commit a change { - // getSystemConfigPath() is already a symlink to agent-r1.conf created in - // SetUp() TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - cliConfigDir.string()); + sessionDir.string(), systemConfigDir_.string()); + + // Simulate a CLI command being tracked + session.setCommandLine( + "config interface eth1/1/1 description First commit"); // Modify the session config 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); @@ -131,29 +209,29 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { // Verify session config no longer exists (removed after commit) EXPECT_FALSE(fs::exists(sessionConfig)); - // Verify new revision was created in cli directory - EXPECT_EQ(result.revision, 2); - fs::path targetConfig = cliConfigDir / "agent-r2.conf"; - EXPECT_TRUE(fs::exists(targetConfig)); - EXPECT_THAT(readFile(targetConfig), ::testing::HasSubstr("First commit")); + // Verify commit SHA was returned + EXPECT_FALSE(result.commitSha.empty()); + EXPECT_EQ(result.commitSha.length(), 40); // Full SHA1 is 40 chars + firstCommitSha = result.commitSha; // Verify metadata file was created alongside the config revision - fs::path targetMetadata = cliConfigDir / "agent-r2.metadata.json"; + fs::path targetMetadata = systemConfigDir_ / "cli" / "cli_metadata.json"; EXPECT_TRUE(fs::exists(targetMetadata)); - // Verify symlink was replaced and points to new revision - EXPECT_TRUE(fs::is_symlink(getSystemConfigPath())); - EXPECT_EQ(fs::read_symlink(getSystemConfigPath()), targetConfig); + // Verify system config was updated + EXPECT_THAT(readFile(cliConfigPath), ::testing::HasSubstr("First commit")); } - // Second commit: Create a new session and verify it's based on r2, not r1 + // Second commit: Create a new session and verify it's based on first commit { TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - cliConfigDir.string()); + sessionDir.string(), systemConfigDir_.string()); - // Verify the new session is based on r2 (the latest committed revision) + // Simulate a CLI command being tracked + session.setCommandLine( + "config interface eth1/1/1 description Second commit"); + + // Verify the new session is based on the latest committed revision auto& config = session.getAgentConfig(); auto& ports = *config.sw()->ports(); ASSERT_FALSE(ports.empty()); @@ -161,82 +239,77 @@ 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); // Commit the second change auto result = session.commit(localhost()); - // Verify new revision was created - EXPECT_EQ(result.revision, 3); - fs::path targetConfig = cliConfigDir / "agent-r3.conf"; - EXPECT_TRUE(fs::exists(targetConfig)); - EXPECT_THAT(readFile(targetConfig), ::testing::HasSubstr("Second commit")); + // Verify new commit SHA was returned + EXPECT_FALSE(result.commitSha.empty()); + EXPECT_NE(result.commitSha, firstCommitSha); + secondCommitSha = result.commitSha; // Verify metadata file was created alongside the config revision - fs::path targetMetadata = cliConfigDir / "agent-r3.metadata.json"; + fs::path targetMetadata = systemConfigDir_ / "cli" / "cli_metadata.json"; EXPECT_TRUE(fs::exists(targetMetadata)); - // Verify symlink was updated to point to r3 - EXPECT_TRUE(fs::is_symlink(getSystemConfigPath())); - EXPECT_EQ(fs::read_symlink(getSystemConfigPath()), targetConfig); + // Verify system config was updated + EXPECT_THAT(readFile(cliConfigPath), ::testing::HasSubstr("Second commit")); + + // Verify Git history has all commits + auto& git = session.getGit(); + auto commits = git.log(cliConfigPath.string()); + EXPECT_EQ(commits.size(), 3); // Initial + 2 commits - // Verify all revisions and their metadata files 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")); - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r2.metadata.json")); - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r3.metadata.json")); + // Verify metadata file was also committed to git + auto metadataCommits = git.log(targetMetadata.string()); + EXPECT_EQ(metadataCommits.size(), 2); // 2 commits } } // Ensure commit() works on a newly initialized session // This verifies that initializeSession() creates the metadata file TEST_F(ConfigSessionTestFixture, commitOnNewlyInitializedSession) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path cliConfigDir = getTestEtcDir() / "coop" / "cli"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path cliConfigDir = systemConfigDir_ / "cli"; // Setup mock agent server setupMockedAgentServer(); - // No config changes were made, so reloadConfig() should not be called - EXPECT_CALL(getMockAgent(), reloadConfig()).Times(0); + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(1); - // Create a new session and immediately commit it + // Create a new session // This tests that metadata file is created during session initialization - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - cliConfigDir.string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); // Verify metadata file was created during session initialization - fs::path metadataPath = sessionDir / "conf_metadata.json"; + fs::path metadataPath = sessionDir / "cli_metadata.json"; EXPECT_TRUE(fs::exists(metadataPath)); - // Make no changes to the session. It's initialized but that's it. + // Make a change so commit has something to commit + auto& config = session.getAgentConfig(); + 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, right now empty sessions still commmit a new - // revision (TODO: fix this so we don't create empty commits). + // Commit should succeed auto result = session.commit(localhost()); - EXPECT_EQ(result.revision, 2); + EXPECT_FALSE(result.commitSha.empty()); - // Verify metadata file was copied to revision directory - fs::path targetMetadata = cliConfigDir / "agent-r2.metadata.json"; + // Verify metadata file was copied to CLI config directory + fs::path targetMetadata = cliConfigDir / "cli_metadata.json"; EXPECT_TRUE(fs::exists(targetMetadata)); } TEST_F(ConfigSessionTestFixture, multipleChangesInOneSession) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; // Create a ConfigSession - // getSystemConfigPath() is already a symlink created in SetUp() - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); // Make first change auto& config = session.getAgentConfig(); @@ -261,16 +334,13 @@ TEST_F(ConfigSessionTestFixture, multipleChangesInOneSession) { } TEST_F(ConfigSessionTestFixture, sessionPersistsAcrossCommands) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; // Create first ConfigSession and modify config - // getSystemConfigPath() is already a symlink created in SetUp() { TestableConfigSession session1( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + sessionDir.string(), systemConfigDir_.string()); auto& config = session1.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -291,9 +361,7 @@ TEST_F(ConfigSessionTestFixture, sessionPersistsAcrossCommands) { // session { TestableConfigSession session2( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + sessionDir.string(), systemConfigDir_.string()); auto& config = session2.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -303,30 +371,22 @@ TEST_F(ConfigSessionTestFixture, sessionPersistsAcrossCommands) { } } -TEST_F(ConfigSessionTestFixture, symlinkRollbackOnFailure) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; +TEST_F(ConfigSessionTestFixture, configRollbackOnFailure) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path cliConfigDir = getTestEtcDir() / "coop" / "cli"; + fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; - // Verify old symlink exists (created in SetUp) - EXPECT_TRUE(fs::is_symlink(getSystemConfigPath())); - EXPECT_EQ( - fs::read_symlink(getSystemConfigPath()), cliConfigDir / "agent-r1.conf"); + // Save the original config content + std::string originalContent = readFile(cliConfigPath); - // Setup mock agent server to fail reloadConfig on first call (the commit), - // but succeed on second call (the rollback reload) + // Setup mock agent server to fail reloadConfig setupMockedAgentServer(); EXPECT_CALL(getMockAgent(), reloadConfig()) .WillOnce(::testing::Throw(std::runtime_error("Reload failed"))) .WillOnce(::testing::Return()); // Create a ConfigSession and try to commit - // getSystemConfigPath() is already a symlink to agent-r1.conf created in - // SetUp() - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - cliConfigDir.string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); auto& config = session.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -335,335 +395,228 @@ TEST_F(ConfigSessionTestFixture, symlinkRollbackOnFailure) { session.setCommandLine("config interface eth1/1/1 description Failed change"); session.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - // Commit should fail and rollback the symlink + // Commit should fail and rollback the config EXPECT_THROW(session.commit(localhost()), std::runtime_error); - // Verify symlink was rolled back to old target - EXPECT_TRUE(fs::is_symlink(getSystemConfigPath())); - EXPECT_EQ( - fs::read_symlink(getSystemConfigPath()), cliConfigDir / "agent-r1.conf"); + // Verify config was rolled back to original content + std::string currentContent = readFile(cliConfigPath); + EXPECT_EQ(currentContent, originalContent); // Verify session config still exists (not removed on failed commit) EXPECT_TRUE(fs::exists(sessionConfig)); } -TEST_F(ConfigSessionTestFixture, atomicRevisionCreation) { - fs::path cliConfigDir = getTestEtcDir() / "coop" / "cli"; +TEST_F(ConfigSessionTestFixture, concurrentCommits) { + fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; // 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}; + // Run two sequential commits to test Git commit functionality + // Note: Git doesn't handle truly concurrent commits well due to index.lock, + // so we run them sequentially to avoid race conditions. + std::string commitSha1; + std::string commitSha2; - auto commitTask = [&](const std::string& sessionName, - const std::string& description, - std::atomic& rev) { - fs::path sessionDir = getTestHomeDir() / sessionName; - fs::path sessionConfig = sessionDir / "agent.conf"; + // First commit + { + fs::path sessionDir = testHomeDir_ / ".fboss2_user1"; TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - cliConfigDir.string()); + sessionDir.string(), systemConfigDir_.string()); auto& config = session.getAgentConfig(); auto& ports = *config.sw()->ports(); ASSERT_FALSE(ports.empty()); - ports[0].description() = description; + ports[0].description() = "First commit"; session.setCommandLine( - "config interface eth1/1/1 description " + description); + "config interface eth1/1/1 description First commit"); session.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - rev = session.commit(localhost()).revision; - }; - - 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 = getTestEtcDir() / "coop" / "cli"; + auto result = session.commit(localhost()); + commitSha1 = result.commitSha; + } - // Setup mock agent server - // Either 1 or 2 commits might succeed depending on the race - setupMockedAgentServer(); - EXPECT_CALL(getMockAgent(), reloadConfig()).Times(testing::Between(1, 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. However, if one - // thread commits and deletes the session files before the other thread - // calls commit(), the second thread will get "No config session exists". - // This is a valid race outcome - the important thing is no crashes. - std::atomic revision1{0}; - std::atomic revision2{0}; - std::atomic thread1NoSession{false}; - std::atomic thread2NoSession{false}; - - auto commitTask = [&](const std::string& description, - std::atomic& rev, - std::atomic& noSession) { - // Both threads use the SAME session path - fs::path sessionDir = getTestHomeDir() / ".fboss2_shared"; - fs::path sessionConfig = sessionDir / "agent.conf"; + // Second commit + { + fs::path sessionDir = testHomeDir_ / ".fboss2_user2"; TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - cliConfigDir.string()); + sessionDir.string(), systemConfigDir_.string()); auto& config = session.getAgentConfig(); auto& ports = *config.sw()->ports(); ASSERT_FALSE(ports.empty()); - ports[0].description() = description; + ports[0].description() = "Second commit"; session.setCommandLine( - "config interface eth1/1/1 description " + description); + "config interface eth1/1/1 description Second commit"); session.saveConfig( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - try { - rev = session.commit(localhost()).revision; - } catch (const std::runtime_error& e) { - // If the other thread already committed and deleted the session files, - // we'll get "No config session exists" - this is a valid race outcome - if (folly::StringPiece(e.what()).contains("No config session exists")) { - noSession = true; - } else { - throw; // Re-throw unexpected errors - } - } - }; - - std::thread thread1( - commitTask, - "First commit", - std::ref(revision1), - std::ref(thread1NoSession)); - std::thread thread2( - commitTask, - "Second commit", - std::ref(revision2), - std::ref(thread2NoSession)); - - thread1.join(); - thread2.join(); - - // At least one commit should succeed - bool commit1Succeeded = revision1.load() != 0; - bool commit2Succeeded = revision2.load() != 0; - EXPECT_TRUE(commit1Succeeded || commit2Succeeded); - - // If both succeeded, they should have different revision numbers - if (commit1Succeeded && commit2Succeeded) { - 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")); - } else { - // One thread got "No config session exists" because the other committed - // first - EXPECT_TRUE(thread1NoSession.load() || thread2NoSession.load()); - // The successful commit should be r2 - int successfulRevision = - commit1Succeeded ? revision1.load() : revision2.load(); - EXPECT_EQ(successfulRevision, 2); - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r2.conf")); + auto result = session.commit(localhost()); + commitSha2 = result.commitSha; } - // The history command would list all three revisions with their metadata + // Both commits should succeed with different commit SHAs + EXPECT_FALSE(commitSha1.empty()); + EXPECT_FALSE(commitSha2.empty()); + EXPECT_NE(commitSha1, commitSha2); + + // Verify Git history contains both commits + Git git(systemConfigDir_.string()); + auto commits = git.log(cliConfigPath.string()); + EXPECT_GE(commits.size(), 3); // Initial + 2 commits } -TEST_F(ConfigSessionTestFixture, revisionNumberExtraction) { - // Test the revision number extraction logic - fs::path cliConfigDir = getTestEtcDir() / "coop" / "cli"; +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"; - // Create files with various revision numbers - createTestConfig(cliConfigDir / "agent-r1.conf", R"({})"); - createTestConfig(cliConfigDir / "agent-r42.conf", R"({})"); - createTestConfig(cliConfigDir / "agent-r999.conf", R"({})"); + // Setup mock agent server + setupMockedAgentServer(); + // 2 commits + 1 rollback = 3 reloadConfig calls + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(3); - // 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")); + // Create a session and make several commits to build history + std::string firstCommitSha; + std::string secondCommitSha; + { + TestableConfigSession session( + sessionDir.string(), systemConfigDir_.string()); - // 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); -} + // Simulate CLI command for first commit + session.setCommandLine( + "config interface eth1/1/1 description First version"); -TEST_F(ConfigSessionTestFixture, rollbackCreatesNewRevision) { - // This test actually calls the rollback() method with a specific revision - fs::path cliConfigDir = getTestEtcDir() / "coop" / "cli"; - fs::path symlinkPath = getTestEtcDir() / "coop" / "agent.conf"; - fs::path sessionConfigPath = getTestHomeDir() / ".fboss2" / "agent.conf"; + // First commit + auto& config1 = session.getAgentConfig(); + (*config1.sw()->ports())[0].description() = "First version"; + session.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + auto result1 = session.commit(localhost()); + firstCommitSha = result1.commitSha; - // Remove the regular file created by SetUp - if (fs::exists(symlinkPath)) { - fs::remove(symlinkPath); + // Second commit (need new session after commit) } + { + TestableConfigSession session( + sessionDir.string(), systemConfigDir_.string()); - // Create revision files (simulating previous commits) - createTestConfig(cliConfigDir / "agent-r1.conf", R"({"revision": 1})"); - createTestConfig(cliConfigDir / "agent-r2.conf", R"({"revision": 2})"); - createTestConfig(cliConfigDir / "agent-r3.conf", R"({"revision": 3})"); - - // Create symlink pointing to r3 (current revision) - fs::create_symlink(cliConfigDir / "agent-r3.conf", symlinkPath); - - // Verify initial state - EXPECT_TRUE(fs::is_symlink(symlinkPath)); - EXPECT_EQ(fs::read_symlink(symlinkPath), cliConfigDir / "agent-r3.conf"); + // Simulate CLI command for second commit + session.setCommandLine( + "config interface eth1/1/1 description Second version"); - // Setup mock agent server - setupMockedAgentServer(); + auto& config2 = session.getAgentConfig(); + (*config2.sw()->ports())[0].description() = "Second version"; + session.saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + auto result2 = session.commit(localhost()); + secondCommitSha = result2.commitSha; + } - // Expect reloadConfig to be called once - EXPECT_CALL(getMockAgent(), reloadConfig()).Times(1); + // Verify current content is "Second version" + EXPECT_THAT(readFile(cliConfigPath), ::testing::HasSubstr("Second version")); + // Verify current metadata contains second command + EXPECT_THAT( + readFile(metadataPath), ::testing::HasSubstr("description Second")); - // Create a testable ConfigSession with test paths - TestableConfigSession session( - sessionConfigPath.string(), symlinkPath.string(), cliConfigDir.string()); + // Now rollback to first commit + { + TestableConfigSession session( + sessionDir.string(), systemConfigDir_.string()); - // Call the actual rollback method to rollback to r1 - int newRevision = session.rollback(localhost(), "r1"); + std::string rollbackSha = session.rollback(localhost(), firstCommitSha); - // Verify rollback created a new revision (r4) - EXPECT_EQ(newRevision, 4); - EXPECT_TRUE(fs::is_symlink(symlinkPath)); - EXPECT_EQ(fs::read_symlink(symlinkPath), cliConfigDir / "agent-r4.conf"); - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r4.conf")); + // Verify rollback created a new commit + EXPECT_FALSE(rollbackSha.empty()); + EXPECT_NE(rollbackSha, firstCommitSha); + EXPECT_NE(rollbackSha, secondCommitSha); - // Verify r4 has same content as r1 (the target revision) - EXPECT_EQ( - readFile(cliConfigDir / "agent-r1.conf"), - readFile(cliConfigDir / "agent-r4.conf")); + // Verify config content is now "First version" + EXPECT_THAT(readFile(cliConfigPath), ::testing::HasSubstr("First version")); - // Verify old revisions still exist (rollback doesn't delete history) - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r1.conf")); - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r2.conf")); - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r3.conf")); -} + // Verify metadata was also rolled back to first version + std::string metadataContent = readFile(metadataPath); + EXPECT_THAT(metadataContent, ::testing::HasSubstr("description First")); + EXPECT_THAT( + metadataContent, + ::testing::Not(::testing::HasSubstr("description Second"))); -TEST_F(ConfigSessionTestFixture, rollbackToPreviousRevision) { - // This test actually calls the rollback() method without a revision argument - // to rollback to the previous revision - fs::path cliConfigDir = getTestEtcDir() / "coop" / "cli"; - fs::path symlinkPath = getTestEtcDir() / "coop" / "agent.conf"; - fs::path sessionConfigPath = getTestHomeDir() / ".fboss2" / "agent.conf"; + // Verify Git history has the rollback commit + auto& git = session.getGit(); + auto commits = git.log(cliConfigPath.string()); + EXPECT_EQ(commits.size(), 4); // Initial + 2 commits + rollback - // Remove the regular file created by SetUp - if (fs::exists(symlinkPath)) { - fs::remove(symlinkPath); + // Verify metadata file history + auto metadataCommits = git.log(metadataPath.string()); + EXPECT_EQ(metadataCommits.size(), 3); // 2 commits + rollback } +} - // Create revision files (simulating previous commits) - createTestConfig(cliConfigDir / "agent-r1.conf", R"({"revision": 1})"); - createTestConfig(cliConfigDir / "agent-r2.conf", R"({"revision": 2})"); - createTestConfig(cliConfigDir / "agent-r3.conf", R"({"revision": 3})"); - - // Create symlink pointing to r3 (current revision) - fs::create_symlink(cliConfigDir / "agent-r3.conf", symlinkPath); - - // Verify initial state - EXPECT_TRUE(fs::is_symlink(symlinkPath)); - EXPECT_EQ(fs::read_symlink(symlinkPath), cliConfigDir / "agent-r3.conf"); +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"; // Setup mock agent server setupMockedAgentServer(); + // 2 commits + 1 rollback = 3 reloadConfig calls + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(3); - // Expect reloadConfig to be called once - EXPECT_CALL(getMockAgent(), reloadConfig()).Times(1); + // Create commits to build history + { + TestableConfigSession session( + sessionDir.string(), systemConfigDir_.string()); - // Create a testable ConfigSession with test paths - TestableConfigSession session( - sessionConfigPath.string(), symlinkPath.string(), cliConfigDir.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()); - // Call the actual rollback method without a revision (should go to previous) - int newRevision = session.rollback(localhost()); + 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()); + } - // Verify rollback to previous revision created r4 with content from r2 - EXPECT_EQ(newRevision, 4); - EXPECT_TRUE(fs::is_symlink(symlinkPath)); - EXPECT_EQ(fs::read_symlink(symlinkPath), cliConfigDir / "agent-r4.conf"); - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r4.conf")); + // Verify current content is "Second version" + EXPECT_THAT(readFile(cliConfigPath), ::testing::HasSubstr("Second version")); - // Verify r4 has same content as r2 (the previous revision) - EXPECT_EQ( - readFile(cliConfigDir / "agent-r2.conf"), - readFile(cliConfigDir / "agent-r4.conf")); + // Rollback to previous commit (no argument) + { + TestableConfigSession session( + sessionDir.string(), systemConfigDir_.string()); + + std::string rollbackSha = session.rollback(localhost()); - // Verify old revisions still exist (rollback doesn't delete history) - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r1.conf")); - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r2.conf")); - EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r3.conf")); + // Verify rollback succeeded + EXPECT_FALSE(rollbackSha.empty()); + + // Verify content is now "First version" (from previous commit) + EXPECT_THAT(readFile(cliConfigPath), ::testing::HasSubstr("First version")); + } } TEST_F(ConfigSessionTestFixture, actionLevelDefaultIsHitless) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; // Create a ConfigSession - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); // Default action level should be HITLESS EXPECT_EQ( @@ -672,16 +625,12 @@ TEST_F(ConfigSessionTestFixture, actionLevelDefaultIsHitless) { } TEST_F(ConfigSessionTestFixture, actionLevelUpdateAndGet) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; // Create a ConfigSession - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); - // Update to AGENT_WARMBOOT + // Update to AGENT_RESTART session.updateRequiredAction( cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); @@ -692,16 +641,12 @@ TEST_F(ConfigSessionTestFixture, actionLevelUpdateAndGet) { } TEST_F(ConfigSessionTestFixture, actionLevelHigherTakesPrecedence) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; // Create a ConfigSession - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); - // Update to AGENT_WARMBOOT first + // Update to AGENT_RESTART first session.updateRequiredAction( cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); @@ -709,23 +654,19 @@ TEST_F(ConfigSessionTestFixture, actionLevelHigherTakesPrecedence) { session.updateRequiredAction( cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); - // Verify action level remains at AGENT_WARMBOOT + // Verify action level remains at AGENT_RESTART EXPECT_EQ( session.getRequiredAction(cli::ServiceType::AGENT), cli::ConfigActionLevel::AGENT_WARMBOOT); } TEST_F(ConfigSessionTestFixture, actionLevelReset) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; // Create a ConfigSession - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); - // Set to AGENT_WARMBOOT + // Set to AGENT_RESTART session.updateRequiredAction( cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); @@ -739,16 +680,13 @@ TEST_F(ConfigSessionTestFixture, actionLevelReset) { } TEST_F(ConfigSessionTestFixture, actionLevelPersistsToMetadataFile) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path metadataFile = sessionDir / "conf_metadata.json"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path metadataFile = sessionDir / "cli_metadata.json"; // Create a ConfigSession and set action level via saveConfig { TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + sessionDir.string(), systemConfigDir_.string()); // Load the config (required before saveConfig) session.getAgentConfig(); @@ -771,9 +709,10 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsToMetadataFile) { } TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path metadataFile = sessionDir / "conf_metadata.json"; + fs::path metadataFile = sessionDir / "cli_metadata.json"; + fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; // Create session directory and metadata file manually fs::create_directories(sessionDir); @@ -784,13 +723,10 @@ TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { // Also create the session config file (otherwise session will overwrite from // system) - fs::copy_file(getSystemConfigPath(), sessionConfig); + fs::copy_file(cliConfigPath, sessionConfig); // Create a ConfigSession - should load action level from metadata file - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); // Verify action level was loaded EXPECT_EQ( @@ -799,15 +735,12 @@ TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { } TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; // First session: set action level via saveConfig { TestableConfigSession session1( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + sessionDir.string(), systemConfigDir_.string()); // Load the config (required before saveConfig) session1.getAgentConfig(); @@ -819,9 +752,7 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { // Second session: verify action level was persisted { TestableConfigSession session2( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + sessionDir.string(), systemConfigDir_.string()); EXPECT_EQ( session2.getRequiredAction(cli::ServiceType::AGENT), @@ -830,16 +761,13 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { } TEST_F(ConfigSessionTestFixture, commandTrackingBasic) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path metadataFile = sessionDir / "conf_metadata.json"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path metadataFile = sessionDir / "cli_metadata.json"; // Create a ConfigSession, execute command, and verify persistence { TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + sessionDir.string(), systemConfigDir_.string()); // Initially, no commands should be recorded EXPECT_TRUE(session.getCommands().empty()); @@ -876,14 +804,10 @@ TEST_F(ConfigSessionTestFixture, commandTrackingBasic) { } TEST_F(ConfigSessionTestFixture, commandTrackingMultipleCommands) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; // Create a ConfigSession - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); // Execute multiple commands auto& config = session.getAgentConfig(); @@ -911,15 +835,12 @@ TEST_F(ConfigSessionTestFixture, commandTrackingMultipleCommands) { } TEST_F(ConfigSessionTestFixture, commandTrackingPersistsAcrossSessions) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; // First session: execute some commands { TestableConfigSession session1( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + sessionDir.string(), systemConfigDir_.string()); auto& config = session1.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -939,9 +860,7 @@ TEST_F(ConfigSessionTestFixture, commandTrackingPersistsAcrossSessions) { // Second session: verify commands were persisted { TestableConfigSession session2( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + sessionDir.string(), systemConfigDir_.string()); EXPECT_EQ(2, session2.getCommands().size()); EXPECT_EQ("config interface eth1/1/1 mtu 9000", session2.getCommands()[0]); @@ -952,14 +871,10 @@ TEST_F(ConfigSessionTestFixture, commandTrackingPersistsAcrossSessions) { } TEST_F(ConfigSessionTestFixture, commandTrackingClearedOnReset) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; - fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; // Create a ConfigSession and add some commands - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); auto& config = session.getAgentConfig(); auto& ports = *config.sw()->ports(); @@ -979,9 +894,10 @@ TEST_F(ConfigSessionTestFixture, commandTrackingClearedOnReset) { } TEST_F(ConfigSessionTestFixture, commandTrackingLoadsFromMetadataFile) { - fs::path sessionDir = getTestHomeDir() / ".fboss2"; + fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; - fs::path metadataFile = sessionDir / "conf_metadata.json"; + fs::path metadataFile = sessionDir / "cli_metadata.json"; + fs::path cliConfigPath = systemConfigDir_ / "cli" / "agent.conf"; // Create session directory and metadata file manually fs::create_directories(sessionDir); @@ -993,13 +909,10 @@ TEST_F(ConfigSessionTestFixture, commandTrackingLoadsFromMetadataFile) { metaFile.close(); // Also create the session config file - fs::copy_file(getSystemConfigPath(), sessionConfig); + fs::copy_file(cliConfigPath, sessionConfig); // Create a ConfigSession - should load commands from metadata file - TestableConfigSession session( - sessionConfig.string(), - getSystemConfigPath().string(), - (getTestEtcDir() / "coop" / "cli").string()); + TestableConfigSession session(sessionDir.string(), systemConfigDir_.string()); // Verify commands were loaded EXPECT_EQ(3, session.getCommands().size()); diff --git a/fboss/cli/fboss2/test/config/CmdConfigTestBase.cpp b/fboss/cli/fboss2/test/config/CmdConfigTestBase.cpp index 07192feeee5d9..46f63e965e44c 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigTestBase.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigTestBase.cpp @@ -6,22 +6,24 @@ #include #include +#include "fboss/cli/fboss2/session/Git.h" #include "fboss/cli/fboss2/test/TestableConfigSession.h" namespace facebook::fboss { -const std::string CmdConfigTestBase::initialAgentCliConfigFile = - "agent-r1.conf"; - -void CmdConfigTestBase::SetUp() { - CmdHandlerTestBase::SetUp(); - - // Create unique test directories - auto tempBase = std::filesystem::temp_directory_path(); - auto uniquePath = boost::filesystem::unique_path(uniquePath_); - testHomeDir_ = tempBase / (uniquePath.string() + "_home"); - testEtcDir_ = tempBase / (uniquePath.string() + "_etc"); - +CmdConfigTestBase::CmdConfigTestBase( + const std::string& uniquePath, + const std::string& initialConfigContent) + : uniquePath_(uniquePath), + initialConfigContent_(initialConfigContent), + testHomeDir_( + std::filesystem::temp_directory_path() / + (boost::filesystem::unique_path(uniquePath).string() + "_home")), + testEtcDir_( + std::filesystem::temp_directory_path() / + (boost::filesystem::unique_path(uniquePath).string() + "_etc")), + cliConfigDir_(testEtcDir_ / "coop" / "cli"), + git_((testEtcDir_ / "coop").string()) { std::error_code ec; if (std::filesystem::exists(testHomeDir_)) { std::filesystem::remove_all(testHomeDir_, ec); @@ -33,8 +35,6 @@ void CmdConfigTestBase::SetUp() { // Create test directories std::filesystem::create_directories(testHomeDir_); std::filesystem::create_directories(testEtcDir_ / "coop"); - - cliConfigDir_ = testEtcDir_ / "coop" / "cli"; std::filesystem::create_directories(cliConfigDir_); // Set environment variables @@ -42,16 +42,23 @@ void CmdConfigTestBase::SetUp() { setenv("USER", "testuser", 1); // Create a test system config file - std::filesystem::path initialRevision = - testEtcDir_ / "coop" / "cli" / initialAgentCliConfigFile; - createTestConfig(initialRevision, initialConfigContent_); + std::filesystem::path cliConfigPath = cliConfigDir_ / "agent.conf"; + createTestConfig(cliConfigPath, initialConfigContent_); // Create symlink systemConfigPath_ = testEtcDir_ / "coop" / "agent.conf"; - std::filesystem::create_symlink(initialRevision, systemConfigPath_); + std::filesystem::create_symlink(cliConfigPath, systemConfigPath_); // Create session config path sessionConfigPath_ = testHomeDir_ / ".fboss2" / "agent.conf"; + + // Initialize Git repository and create initial commit + git_.init(); + git_.commit({"cli/agent.conf"}, "Initial commit"); +} + +void CmdConfigTestBase::SetUp() { + CmdHandlerTestBase::SetUp(); } void CmdConfigTestBase::TearDown() { @@ -86,25 +93,22 @@ std::string CmdConfigTestBase::readFile(const std::filesystem::path& path) { void CmdConfigTestBase::setupTestableConfigSession() { TestableConfigSession::setInstance( std::make_unique( - sessionConfigPath_.string(), - systemConfigPath_.string(), - cliConfigDir_.string())); + (testHomeDir_ / ".fboss2").string(), + (testEtcDir_ / "coop").string())); } void CmdConfigTestBase::setupTestableConfigSession( const std::string& cmdPrefix, const std::string& cmdArgs) { auto testSession = std::make_unique( - sessionConfigPath_.string(), - systemConfigPath_.string(), - cliConfigDir_.string()); + (testHomeDir_ / ".fboss2").string(), (testEtcDir_ / "coop").string()); testSession->setCommandLine(fmt::format("{} {}", cmdPrefix, cmdArgs)); folly::split(' ', cmdArgs, cmdArgsList_, true); // true = ignore empty TestableConfigSession::setInstance(std::move(testSession)); } void CmdConfigTestBase::removeInitialRevisionFile() { - std::filesystem::remove(cliConfigDir_ / initialAgentCliConfigFile); + std::filesystem::remove(cliConfigDir_ / "agent.conf"); std::filesystem::remove(systemConfigPath_); createTestConfig(systemConfigPath_, initialConfigContent_); } diff --git a/fboss/cli/fboss2/test/config/CmdConfigTestBase.h b/fboss/cli/fboss2/test/config/CmdConfigTestBase.h index 2e290e2d6a8e6..6bc02f2d20291 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigTestBase.h +++ b/fboss/cli/fboss2/test/config/CmdConfigTestBase.h @@ -6,16 +6,15 @@ #include +#include "fboss/cli/fboss2/session/Git.h" + namespace facebook::fboss { class CmdConfigTestBase : public CmdHandlerTestBase { public: - static const std::string initialAgentCliConfigFile; - CmdConfigTestBase( const std::string& uniquePath, - const std::string& initialConfigContent) - : uniquePath_(uniquePath), initialConfigContent_(initialConfigContent) {} + const std::string& initialConfigContent); void SetUp() override; void TearDown() override; @@ -43,6 +42,10 @@ class CmdConfigTestBase : public CmdHandlerTestBase { return std::filesystem::exists(cliConfigDir_); } + Git& git() { + return git_; + } + void createTestConfig( const std::filesystem::path& path, const std::string& content); @@ -62,7 +65,7 @@ class CmdConfigTestBase : public CmdHandlerTestBase { void removeInitialRevisionFile(); private: - CmdConfigTestBase() = default; + CmdConfigTestBase() = delete; // All Config Tests should set these values in their constructors std::string uniquePath_{""}; @@ -78,5 +81,6 @@ class CmdConfigTestBase : public CmdHandlerTestBase { std::string cmdPrefix_; std::string cmdArgs_; std::vector cmdArgsList_; + Git git_; }; } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/utils/CmdUtils.cpp b/fboss/cli/fboss2/utils/CmdUtils.cpp index 6b78a7cbdda79..8bc53eafdbc6e 100644 --- a/fboss/cli/fboss2/utils/CmdUtils.cpp +++ b/fboss/cli/fboss2/utils/CmdUtils.cpp @@ -17,7 +17,6 @@ #endif #include -#include #include using namespace std::chrono; @@ -516,31 +515,27 @@ RevisionList::RevisionList(std::vector v) { continue; } - // Must be in the form "rN" where N is a positive integer - if (revision.empty() || revision[0] != 'r') { - throw std::invalid_argument( - "Invalid revision specifier: '" + revision + - "'. Expected 'rN' or 'current'"); - } - - // Extract the number part after 'r' - std::string revNum = revision.substr(1); - if (revNum.empty()) { - throw std::invalid_argument( - "Invalid revision specifier: '" + revision + - "'. Expected 'rN' or 'current'"); + // Accept Git commit SHAs (7-40 hex characters) or short refs + // Git SHAs are hexadecimal strings, typically 7-40 characters + bool isValidHex = !revision.empty() && revision.length() >= 7; + if (isValidHex) { + for (char c : revision) { + if (!std::isxdigit(c)) { + isValidHex = false; + break; + } + } } - // Validate that it's all digits - for (char c : revNum) { - if (!std::isdigit(c)) { - throw std::invalid_argument( - "Invalid revision number: '" + revision + - "'. Expected 'rN' or 'current'"); - } + if (isValidHex) { + data_.push_back(revision); + continue; } - data_.push_back(revision); + // If not a valid hex SHA, reject it + throw std::invalid_argument( + "Invalid revision specifier: '" + revision + + "'. Expected a Git commit SHA (7+ hex characters) or 'current'"); } }