From 5d3de93b5817c2c28e2a420f5b0afcc568dc35a5 Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Tue, 23 Dec 2025 16:48:10 +0000 Subject: [PATCH 1/9] Add a `fboss2 config rollback` command. --- cmake/CliFboss2.cmake | 2 + fboss/cli/fboss2/BUCK | 2 + fboss/cli/fboss2/CmdHandlerImplConfig.cpp | 2 + fboss/cli/fboss2/CmdListConfig.cpp | 7 ++ .../config/rollback/CmdConfigRollback.cpp | 57 +++++++++ .../config/rollback/CmdConfigRollback.h | 39 ++++++ fboss/cli/fboss2/session/ConfigSession.cpp | 115 ++++++++++++++++++ fboss/cli/fboss2/session/ConfigSession.h | 5 + .../cli/fboss2/test/CmdConfigSessionTest.cpp | 107 ++++++++++++++++ 9 files changed, 336 insertions(+) create mode 100644 fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp create mode 100644 fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 6a5872c93f967..caea3ddb0ce67 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -575,6 +575,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.cpp fboss/cli/fboss2/commands/config/CmdConfigReload.h fboss/cli/fboss2/commands/config/CmdConfigReload.cpp + fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h + fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 4c08a6a7311e2..fddd9ae247522 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -771,6 +771,7 @@ cpp_library( "CmdListConfig.cpp", "commands/config/CmdConfigAppliedInfo.cpp", "commands/config/CmdConfigReload.cpp", + "commands/config/rollback/CmdConfigRollback.cpp", "commands/config/session/CmdConfigSessionCommit.cpp", "commands/config/session/CmdConfigSessionDiff.cpp", "session/ConfigSession.cpp", @@ -778,6 +779,7 @@ cpp_library( headers = [ "commands/config/CmdConfigAppliedInfo.h", "commands/config/CmdConfigReload.h", + "commands/config/rollback/CmdConfigRollback.h", "commands/config/session/CmdConfigSessionCommit.h", "commands/config/session/CmdConfigSessionDiff.h", "session/ConfigSession.h", diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 6c93601afdc7a..6c394fcaafb35 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -12,6 +12,7 @@ #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" +#include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -20,6 +21,7 @@ namespace facebook::fboss { template void CmdHandler::run(); template void CmdHandler::run(); +template void CmdHandler::run(); template void CmdHandler::run(); template void diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 231cef7c5990c..acdbc61d78c6e 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -13,6 +13,7 @@ #include "fboss/cli/fboss2/CmdHandler.h" #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" +#include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -49,6 +50,12 @@ const CommandTree& kConfigCommandTree() { "Reload agent configuration", commandHandler, argTypeHandler}, + + {"config", + "rollback", + "Rollback to a previous config revision", + commandHandler, + argTypeHandler}, }; sort(root.begin(), root.end()); return root; diff --git a/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp b/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp new file mode 100644 index 0000000000000..2b3b04e07a0ff --- /dev/null +++ b/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +CmdConfigRollbackTraits::RetType CmdConfigRollback::queryClient( + const HostInfo& hostInfo, + const utils::RevisionList& revisions) { + auto& session = ConfigSession::getInstance(); + + // Validate arguments + if (revisions.size() > 1) { + throw std::invalid_argument( + "Too many arguments. Expected 0 or 1 revision specifier."); + } + + if (!revisions.empty() && revisions[0] == "current") { + throw std::invalid_argument( + "Cannot rollback to 'current'. Please specify a revision number like 'r42'."); + } + + try { + int newRevision; + if (revisions.empty()) { + // No revision specified - rollback to previous revision + newRevision = session.rollback(hostInfo); + } else { + // Specific revision specified + newRevision = session.rollback(hostInfo, revisions[0]); + } + if (newRevision) { + return "Successfully rolled back to r" + std::to_string(newRevision) + + " and config reloaded."; + } else { + return "Failed to create a new revision after rollback."; + } + } catch (const std::exception& ex) { + throw std::runtime_error( + "Failed to rollback config: " + std::string(ex.what())); + } +} + +void CmdConfigRollback::printOutput(const RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h b/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h new file mode 100644 index 0000000000000..e03baab404dff --- /dev/null +++ b/fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/utils/CmdClientUtils.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" + +namespace facebook::fboss { + +struct CmdConfigRollbackTraits : public WriteCommandTraits { + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_REVISION_LIST; + using ObjectArgType = utils::RevisionList; + using RetType = std::string; +}; + +class CmdConfigRollback + : public CmdHandler { + public: + using ObjectArgType = CmdConfigRollbackTraits::ObjectArgType; + using RetType = CmdConfigRollbackTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const utils::RevisionList& revisions); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index e9232a33b216f..a9a4d959fb124 100644 --- a/fboss/cli/fboss2/session/ConfigSession.cpp +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -167,6 +167,25 @@ 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 ConfigSession::ConfigSession() { @@ -456,4 +475,100 @@ int ConfigSession::commit(const HostInfo& hostInfo) { return revision; } +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) { + throw std::runtime_error( + "Cannot rollback: already at the first revision (r1)"); + } + + // Rollback to the previous revision + std::string targetRevision = "r" + std::to_string(currentRevision - 1); + return rollback(hostInfo, targetRevision); +} + +int ConfigSession::rollback( + const HostInfo& hostInfo, + const std::string& revision) { + ensureDirectoryExists(cliConfigDir_); + + // Build the path to the target revision + std::string targetConfigPath = cliConfigDir_ + "/agent-" + revision + ".conf"; + + // Check if the target revision exists + if (!fs::exists(targetConfigPath)) { + throw std::runtime_error( + "Revision " + revision + " does not exist at " + targetConfigPath); + } + + std::error_code ec; + + // Verify that the system config is a symlink + if (!fs::is_symlink(systemConfigPath_)) { + throw std::runtime_error( + systemConfigPath_ + " is not a symlink. Expected it to be a symlink."); + } + + // 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( + "Failed to read symlink " + systemConfigPath_ + ": " + ec.message()); + } + + // 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 update the symlink to point to the new revision + atomicSymlinkUpdate(systemConfigPath_, newRevisionPath); + + // Reload the config - if this fails, atomically undo the rollback + try { + auto client = + utils::createClient>( + hostInfo); + client->sync_reloadConfig(); + } catch (const std::exception& ex) { + // Rollback: atomically restore the old symlink + try { + atomicSymlinkUpdate(systemConfigPath_, oldSymlinkTarget); + } catch (const std::exception& rollbackEx) { + // If rollback also fails, include both errors in the message + throw std::runtime_error( + fmt::format( + "Failed to reload config: {}. Additionally, failed to rollback the symlink: {}", + ex.what(), + rollbackEx.what())); + } + throw std::runtime_error( + fmt::format( + "Failed to reload config, symlink was rolled back automatically: {}", + ex.what())); + } + + // Successfully rolled back + LOG(INFO) << "Rollback committed as revision r" << newRevision; + return newRevision; +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/session/ConfigSession.h b/fboss/cli/fboss2/session/ConfigSession.h index 5f962dad5c874..7838262eaae35 100644 --- a/fboss/cli/fboss2/session/ConfigSession.h +++ b/fboss/cli/fboss2/session/ConfigSession.h @@ -102,6 +102,11 @@ class ConfigSession { // successful. int 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); + // Check if a session exists bool sessionExists() const; diff --git a/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp b/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp index 8b08a3ca1c950..1dfb390be1ef0 100644 --- a/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp +++ b/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp @@ -530,4 +530,111 @@ TEST_F(ConfigSessionTestFixture, revisionNumberExtraction) { 999); } +TEST_F(ConfigSessionTestFixture, rollbackCreatesNewRevision) { + // This test actually calls the rollback() method with a specific revision + fs::path cliConfigDir = testEtcDir_ / "coop" / "cli"; + fs::path symlinkPath = testEtcDir_ / "coop" / "agent.conf"; + fs::path sessionConfigPath = testHomeDir_ / ".fboss2" / "agent.conf"; + + // Remove the regular file created by SetUp + if (fs::exists(symlinkPath)) { + fs::remove(symlinkPath); + } + + // 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"); + + // Setup mock agent server + setupMockedAgentServer(); + + // Expect reloadConfig to be called once + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(1); + + // Create a testable ConfigSession with test paths + TestableConfigSession session( + sessionConfigPath.string(), symlinkPath.string(), cliConfigDir.string()); + + // Call the actual rollback method to rollback to r1 + int newRevision = session.rollback(localhost(), "r1"); + + // 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 r4 has same content as r1 (the target revision) + EXPECT_EQ( + readFile(cliConfigDir / "agent-r1.conf"), + readFile(cliConfigDir / "agent-r4.conf")); + + // 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")); +} + +TEST_F(ConfigSessionTestFixture, rollbackToPreviousRevision) { + // This test actually calls the rollback() method without a revision argument + // to rollback to the previous revision + fs::path cliConfigDir = testEtcDir_ / "coop" / "cli"; + fs::path symlinkPath = testEtcDir_ / "coop" / "agent.conf"; + fs::path sessionConfigPath = testHomeDir_ / ".fboss2" / "agent.conf"; + + // Remove the regular file created by SetUp + if (fs::exists(symlinkPath)) { + fs::remove(symlinkPath); + } + + // 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"); + + // Setup mock agent server + setupMockedAgentServer(); + + // Expect reloadConfig to be called once + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(1); + + // Create a testable ConfigSession with test paths + TestableConfigSession session( + sessionConfigPath.string(), symlinkPath.string(), cliConfigDir.string()); + + // Call the actual rollback method without a revision (should go to previous) + int newRevision = session.rollback(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 r4 has same content as r2 (the previous revision) + EXPECT_EQ( + readFile(cliConfigDir / "agent-r2.conf"), + readFile(cliConfigDir / "agent-r4.conf")); + + // 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")); +} + } // namespace facebook::fboss From 575d2113a9ddc66c5b3615ba34509e7742f20916 Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Tue, 23 Dec 2025 16:56:42 +0000 Subject: [PATCH 2/9] Add a `fboss2 config history` command. --- cmake/CliFboss2.cmake | 2 + cmake/CliFboss2Test.cmake | 1 + fboss/cli/fboss2/BUCK | 2 + fboss/cli/fboss2/CmdHandlerImplConfig.cpp | 2 + fboss/cli/fboss2/CmdListConfig.cpp | 7 + .../config/history/CmdConfigHistory.cpp | 162 +++++++++++ .../config/history/CmdConfigHistory.h | 37 +++ fboss/cli/fboss2/test/BUCK | 1 + .../cli/fboss2/test/CmdConfigHistoryTest.cpp | 263 ++++++++++++++++++ 9 files changed, 477 insertions(+) create mode 100644 fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp create mode 100644 fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h create mode 100644 fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index caea3ddb0ce67..f9ead328e24fb 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -575,6 +575,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.cpp fboss/cli/fboss2/commands/config/CmdConfigReload.h fboss/cli/fboss2/commands/config/CmdConfigReload.cpp + fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h + fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.cpp fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index e50f1dd335b8e..cbb89bdfd0405 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -4,6 +4,7 @@ add_executable(fboss2_cmd_test fboss/cli/fboss2/test/TestMain.cpp fboss/cli/fboss2/test/CmdConfigAppliedInfoTest.cpp + fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp fboss/cli/fboss2/test/CmdConfigReloadTest.cpp fboss/cli/fboss2/test/CmdConfigSessionDiffTest.cpp fboss/cli/fboss2/test/CmdConfigSessionTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index fddd9ae247522..86c53d4bf7fcf 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -771,6 +771,7 @@ cpp_library( "CmdListConfig.cpp", "commands/config/CmdConfigAppliedInfo.cpp", "commands/config/CmdConfigReload.cpp", + "commands/config/history/CmdConfigHistory.cpp", "commands/config/rollback/CmdConfigRollback.cpp", "commands/config/session/CmdConfigSessionCommit.cpp", "commands/config/session/CmdConfigSessionDiff.cpp", @@ -779,6 +780,7 @@ cpp_library( headers = [ "commands/config/CmdConfigAppliedInfo.h", "commands/config/CmdConfigReload.h", + "commands/config/history/CmdConfigHistory.h", "commands/config/rollback/CmdConfigRollback.h", "commands/config/session/CmdConfigSessionCommit.h", "commands/config/session/CmdConfigSessionDiff.h", diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 6c394fcaafb35..6b2f8ac117f64 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -12,6 +12,7 @@ #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" +#include "fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -21,6 +22,7 @@ namespace facebook::fboss { template void CmdHandler::run(); template void CmdHandler::run(); +template void CmdHandler::run(); template void CmdHandler::run(); template void CmdHandler::run(); diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index acdbc61d78c6e..254e99acbfcff 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -13,6 +13,7 @@ #include "fboss/cli/fboss2/CmdHandler.h" #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" +#include "fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -27,6 +28,12 @@ const CommandTree& kConfigCommandTree() { commandHandler, argTypeHandler}, + {"config", + "history", + "Show history of committed config revisions", + commandHandler, + argTypeHandler}, + { "config", "session", diff --git a/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp b/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp new file mode 100644 index 0000000000000..b5c3755bd38b6 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp @@ -0,0 +1,162 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/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]; + tm timeinfo{}; + localtime_r(&timeSec, &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; +} + +} // namespace + +CmdConfigHistoryTraits::RetType CmdConfigHistory::queryClient( + const HostInfo& hostInfo) { + auto& session = ConfigSession::getInstance(); + const std::string cliConfigDir = session.getCliConfigDir(); + + auto revisions = collectRevisions(cliConfigDir); + + if (revisions.empty()) { + return "No config revisions found in " + cliConfigDir; + } + + // Build the table + utils::Table table; + table.setHeader({"Revision", "Owner", "Commit Time"}); + + for (const auto& rev : revisions) { + table.addRow( + {"r" + std::to_string(rev.revisionNumber), + rev.owner, + formatTime(rev.commitTimeNsec)}); + } + + // Convert table to string + std::ostringstream oss; + oss << table; + return oss.str(); +} + +void CmdConfigHistory::printOutput(const RetType& tableOutput) { + std::cout << tableOutput << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h b/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h new file mode 100644 index 0000000000000..44008e5e28c7d --- /dev/null +++ b/fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h @@ -0,0 +1,37 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/utils/CmdClientUtils.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" + +namespace facebook::fboss { + +struct CmdConfigHistoryTraits : public WriteCommandTraits { + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE; + using ObjectArgType = std::monostate; + using RetType = std::string; +}; + +class CmdConfigHistory + : public CmdHandler { + public: + using ObjectArgType = CmdConfigHistoryTraits::ObjectArgType; + using RetType = CmdConfigHistoryTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo); + + void printOutput(const RetType& tableOutput); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/BUCK b/fboss/cli/fboss2/test/BUCK index 797d2da0b80c8..b13fe353aa698 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -56,6 +56,7 @@ cpp_unittest( name = "cmd_test", srcs = [ "CmdConfigAppliedInfoTest.cpp", + "CmdConfigHistoryTest.cpp", "CmdConfigReloadTest.cpp", "CmdConfigSessionDiffTest.cpp", "CmdConfigSessionTest.cpp", diff --git a/fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp b/fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp new file mode 100644 index 0000000000000..5db56df97a947 --- /dev/null +++ b/fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp @@ -0,0 +1,263 @@ +// (c) Facebook, Inc. and its affiliates. Confidential and proprietary. + +#include +#include +#include +#include + +#include "fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/CmdHandlerTestBase.h" +#include "fboss/cli/fboss2/test/TestableConfigSession.h" +#include "fboss/cli/fboss2/utils/PortMap.h" + +#include +#include + +namespace fs = std::filesystem; + +using namespace ::testing; + +namespace facebook::fboss { + +class CmdConfigHistoryTestFixture : public CmdHandlerTestBase { + public: + fs::path testHomeDir_; + fs::path testEtcDir_; + fs::path systemConfigPath_; + fs::path sessionConfigPath_; + fs::path cliConfigDir_; + + 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( + "fboss2_config_history_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; + fs::remove_all(testHomeDir_, ec); + fs::remove_all(testEtcDir_, ec); + + // Create fresh test directories + fs::create_directories(testHomeDir_); + fs::create_directories(testEtcDir_); + + // Set up paths + systemConfigPath_ = testEtcDir_ / "agent.conf"; + sessionConfigPath_ = testHomeDir_ / ".fboss2" / "agent.conf"; + cliConfigDir_ = testEtcDir_ / "coop" / "cli"; + + // Create CLI config directory + fs::create_directories(cliConfigDir_); + + // Create a default system config + createTestConfig( + systemConfigPath_, + R"({ + "sw": { + "ports": [ + { + "logicalID": 1, + "name": "eth1/1/1", + "state": 2, + "speed": 100000 + } + ] + } +})"); + } + + void TearDown() override { + // Reset the singleton to ensure tests don't interfere with each other + TestableConfigSession::setInstance(nullptr); + // Clean up test directories (use error_code to avoid exceptions) + std::error_code ec; + fs::remove_all(testHomeDir_, ec); + fs::remove_all(testEtcDir_, ec); + CmdHandlerTestBase::TearDown(); + } + + void createTestConfig(const fs::path& path, const std::string& content) { + fs::create_directories(path.parent_path()); + 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(); + } + + void initializeTestSession() { + // Ensure system config exists before initializing session + if (!fs::exists(systemConfigPath_)) { + createTestConfig( + systemConfigPath_, + R"({ + "sw": { + "ports": [ + { + "logicalID": 1, + "name": "eth1/1/1", + "state": 2, + "speed": 100000 + } + ] + } +})"); + } + + // Ensure the parent directory of session config exists + fs::create_directories(sessionConfigPath_.parent_path()); + + // Initialize ConfigSession singleton with test paths + TestableConfigSession::setInstance( + std::make_unique( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string())); + } +}; + +TEST_F(CmdConfigHistoryTestFixture, historyListsRevisions) { + // Create revision files with valid config content + createTestConfig( + cliConfigDir_ / "agent-r1.conf", + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 100000}]}})"); + createTestConfig( + cliConfigDir_ / "agent-r2.conf", + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 100000}]}})"); + createTestConfig( + cliConfigDir_ / "agent-r3.conf", + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 100000}]}})"); + + // Initialize ConfigSession singleton with test paths + initializeTestSession(); + + // 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("Commit Time"), std::string::npos); +} + +TEST_F(CmdConfigHistoryTestFixture, historyIgnoresNonMatchingFiles) { + // Create valid revision files + createTestConfig( + cliConfigDir_ / "agent-r1.conf", + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 100000}]}})"); + createTestConfig( + cliConfigDir_ / "agent-r2.conf", + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 100000}]}})"); + + // Create files that should be ignored + createTestConfig(cliConfigDir_ / "agent.conf.bak", R"({"backup": true})"); + createTestConfig(cliConfigDir_ / "other-r1.conf", R"({"other": true})"); + createTestConfig(cliConfigDir_ / "agent-r1.txt", R"({"wrong_ext": true})"); + createTestConfig(cliConfigDir_ / "agent-rX.conf", R"({"invalid": true})"); + + // Initialize ConfigSession singleton with test paths + initializeTestSession(); + + // 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); +} + +TEST_F(CmdConfigHistoryTestFixture, historyEmptyDirectory) { + // Directory exists but has no revision files + EXPECT_TRUE(fs::exists(cliConfigDir_)); + + // Initialize ConfigSession singleton with test paths + initializeTestSession(); + + // Create and execute the command + 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(cliConfigDir_.string()), std::string::npos); +} + +TEST_F(CmdConfigHistoryTestFixture, historyNonSequentialRevisions) { + // Create non-sequential revision files (e.g., after deletions) + createTestConfig( + cliConfigDir_ / "agent-r1.conf", + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 100000}]}})"); + createTestConfig( + cliConfigDir_ / "agent-r5.conf", + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 100000}]}})"); + createTestConfig( + cliConfigDir_ / "agent-r10.conf", + R"({"sw": {"ports": [{"logicalID": 1, "name": "eth1/1/1", "state": 2, "speed": 100000}]}})"); + + // Initialize ConfigSession singleton with test paths + initializeTestSession(); + + // Create and execute the command + 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); +} + +TEST_F(CmdConfigHistoryTestFixture, printOutput) { + auto cmd = CmdConfigHistory(); + std::string tableOutput = + "Revision Owner Commit Time\nr1 user1 2024-01-01 12:00:00.000"; + + // Redirect cout to capture output + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + + cmd.printOutput(tableOutput); + + // Restore cout + std::cout.rdbuf(old); + + 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("user1"), std::string::npos); +} + +} // namespace facebook::fboss From 323ea6eac9f92e3746bc140d316b60aa35b79a77 Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Tue, 23 Dec 2025 17:06:05 +0000 Subject: [PATCH 3/9] Add a `fboss2 config interface description ` command. Add some helper code to process interface-list arguments. --- cmake/CliFboss2.cmake | 5 ++ cmake/CliFboss2Test.cmake | 1 + fboss/cli/fboss2/BUCK | 5 ++ fboss/cli/fboss2/CmdHandler.h | 2 + fboss/cli/fboss2/CmdHandlerImplConfig.cpp | 6 ++ fboss/cli/fboss2/CmdListConfig.cpp | 16 ++++ fboss/cli/fboss2/CmdSubcommands.cpp | 3 + .../config/interface/CmdConfigInterface.h | 38 ++++++++ .../CmdConfigInterfaceDescription.cpp | 48 ++++++++++ .../interface/CmdConfigInterfaceDescription.h | 43 +++++++++ fboss/cli/fboss2/test/BUCK | 1 + .../CmdConfigInterfaceDescriptionTest.cpp | 90 +++++++++++++++++++ fboss/cli/fboss2/utils/CmdUtilsCommon.h | 1 + fboss/cli/fboss2/utils/InterfaceList.cpp | 65 ++++++++++++++ fboss/cli/fboss2/utils/InterfaceList.h | 79 ++++++++++++++++ 15 files changed, 403 insertions(+) create mode 100644 fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h create mode 100644 fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.cpp create mode 100644 fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h create mode 100644 fboss/cli/fboss2/test/CmdConfigInterfaceDescriptionTest.cpp create mode 100644 fboss/cli/fboss2/utils/InterfaceList.cpp create mode 100644 fboss/cli/fboss2/utils/InterfaceList.h diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index f9ead328e24fb..e4348aba326d0 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -479,6 +479,8 @@ add_library(fboss2_lib fboss/cli/fboss2/utils/PortMap.cpp fboss/cli/fboss2/utils/Table.cpp fboss/cli/fboss2/utils/HostInfo.h + fboss/cli/fboss2/utils/InterfaceList.h + fboss/cli/fboss2/utils/InterfaceList.cpp fboss/cli/fboss2/utils/FilterOp.h fboss/cli/fboss2/utils/AggregateOp.h fboss/cli/fboss2/utils/AggregateUtils.h @@ -575,6 +577,9 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.cpp fboss/cli/fboss2/commands/config/CmdConfigReload.h fboss/cli/fboss2/commands/config/CmdConfigReload.cpp + fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h + fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h + fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.cpp fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index cbb89bdfd0405..9f6551dd3e1f6 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -5,6 +5,7 @@ add_executable(fboss2_cmd_test fboss/cli/fboss2/test/TestMain.cpp fboss/cli/fboss2/test/CmdConfigAppliedInfoTest.cpp fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp + fboss/cli/fboss2/test/CmdConfigInterfaceDescriptionTest.cpp fboss/cli/fboss2/test/CmdConfigReloadTest.cpp fboss/cli/fboss2/test/CmdConfigSessionDiffTest.cpp fboss/cli/fboss2/test/CmdConfigSessionTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 86c53d4bf7fcf..bb73e49db0668 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -146,6 +146,7 @@ cpp_library( name = "cmd-common-utils", srcs = [ "utils/CmdUtilsCommon.cpp", + "utils/InterfaceList.cpp", ], headers = [ "commands/clear/CmdClearUtils.h", @@ -154,6 +155,7 @@ cpp_library( "utils/CmdUtilsCommon.h", "utils/FilterUtils.h", "utils/HostInfo.h", + "utils/InterfaceList.h", ], exported_deps = [ ":cmd-global-options", @@ -772,6 +774,7 @@ cpp_library( "commands/config/CmdConfigAppliedInfo.cpp", "commands/config/CmdConfigReload.cpp", "commands/config/history/CmdConfigHistory.cpp", + "commands/config/interface/CmdConfigInterfaceDescription.cpp", "commands/config/rollback/CmdConfigRollback.cpp", "commands/config/session/CmdConfigSessionCommit.cpp", "commands/config/session/CmdConfigSessionDiff.cpp", @@ -781,6 +784,8 @@ cpp_library( "commands/config/CmdConfigAppliedInfo.h", "commands/config/CmdConfigReload.h", "commands/config/history/CmdConfigHistory.h", + "commands/config/interface/CmdConfigInterface.h", + "commands/config/interface/CmdConfigInterfaceDescription.h", "commands/config/rollback/CmdConfigRollback.h", "commands/config/session/CmdConfigSessionCommit.h", "commands/config/session/CmdConfigSessionDiff.h", diff --git a/fboss/cli/fboss2/CmdHandler.h b/fboss/cli/fboss2/CmdHandler.h index 37614c4670e48..74b4bc507ab98 100644 --- a/fboss/cli/fboss2/CmdHandler.h +++ b/fboss/cli/fboss2/CmdHandler.h @@ -169,6 +169,8 @@ class CmdHandler { RetType result; try { result = queryClientHelper(hostInfo); + } catch (std::invalid_argument const& err) { + errStr = folly::to("Invalid argument: ", err.what()); } catch (std::exception const& err) { errStr = folly::to("Thrift call failed: '", err.what(), "'"); } diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 6b2f8ac117f64..9b940aeae417d 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -13,6 +13,8 @@ #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" #include "fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -22,6 +24,10 @@ namespace facebook::fboss { template void CmdHandler::run(); template void CmdHandler::run(); +template void CmdHandler::run(); +template void CmdHandler< + CmdConfigInterfaceDescription, + CmdConfigInterfaceDescriptionTraits>::run(); template void CmdHandler::run(); template void CmdHandler::run(); template void diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 254e99acbfcff..d06c7fb4370b2 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -14,6 +14,8 @@ #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" #include "fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -34,6 +36,20 @@ const CommandTree& kConfigCommandTree() { commandHandler, argTypeHandler}, + { + "config", + "interface", + "Configure interface settings", + commandHandler, + argTypeHandler, + {{ + "description", + "Set interface description", + commandHandler, + argTypeHandler, + }}, + }, + { "config", "session", diff --git a/fboss/cli/fboss2/CmdSubcommands.cpp b/fboss/cli/fboss2/CmdSubcommands.cpp index 199f4b00a6c6b..d1232311a866d 100644 --- a/fboss/cli/fboss2/CmdSubcommands.cpp +++ b/fboss/cli/fboss2/CmdSubcommands.cpp @@ -219,6 +219,9 @@ CLI::App* CmdSubcommands::addCommand( case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_FAN_PWM: subCmd->add_option("pwm", args, "Fan PWM (0..100) or 'disable'"); break; + case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_INTERFACE_LIST: + subCmd->add_option("interfaces", args, "Interface(s)"); + break; case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_REVISION_LIST: subCmd->add_option( "revisions", args, "Revision(s) in the form 'rN' or 'current'"); diff --git a/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h new file mode 100644 index 0000000000000..5550bfae6f22a --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" + +namespace facebook::fboss { + +struct CmdConfigInterfaceTraits : public WriteCommandTraits { + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_PORT_LIST; + using ObjectArgType = std::vector; + using RetType = std::string; +}; + +class CmdConfigInterface + : public CmdHandler { + public: + RetType queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& /* interfaceNames */) { + throw std::runtime_error( + "Incomplete command, please use one of the subcommands"); + } + + void printOutput(const RetType& /* model */) {} +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.cpp b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.cpp new file mode 100644 index 0000000000000..73a9f82998f78 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.cpp @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" + +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +CmdConfigInterfaceDescriptionTraits::RetType +CmdConfigInterfaceDescription::queryClient( + const HostInfo& hostInfo, + const utils::InterfaceList& interfaces, + const ObjectArgType& description) { + if (interfaces.empty()) { + throw std::invalid_argument("No interface name provided"); + } + + std::string descriptionStr = description.data()[0]; + + // Update description for all resolved ports + for (const utils::Intf& intf : interfaces) { + cfg::Port* port = intf.getPort(); + if (port) { + port->description() = descriptionStr; + } + } + + // Save the updated config + ConfigSession::getInstance().saveConfig(); + + std::string interfaceList = folly::join(", ", interfaces.getNames()); + return "Successfully set description for interface(s) " + interfaceList; +} + +void CmdConfigInterfaceDescription::printOutput(const RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h new file mode 100644 index 0000000000000..2858dc85aee6f --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" +#include "fboss/cli/fboss2/utils/InterfaceList.h" + +namespace facebook::fboss { + +struct CmdConfigInterfaceDescriptionTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigInterface; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + using ObjectArgType = utils::Message; + using RetType = std::string; +}; + +class CmdConfigInterfaceDescription : public CmdHandler< + CmdConfigInterfaceDescription, + CmdConfigInterfaceDescriptionTraits> { + public: + using ObjectArgType = CmdConfigInterfaceDescriptionTraits::ObjectArgType; + using RetType = CmdConfigInterfaceDescriptionTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const utils::InterfaceList& interfaces, + const ObjectArgType& description); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/BUCK b/fboss/cli/fboss2/test/BUCK index b13fe353aa698..9046f3d987072 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -57,6 +57,7 @@ cpp_unittest( srcs = [ "CmdConfigAppliedInfoTest.cpp", "CmdConfigHistoryTest.cpp", + "CmdConfigInterfaceDescriptionTest.cpp", "CmdConfigReloadTest.cpp", "CmdConfigSessionDiffTest.cpp", "CmdConfigSessionTest.cpp", diff --git a/fboss/cli/fboss2/test/CmdConfigInterfaceDescriptionTest.cpp b/fboss/cli/fboss2/test/CmdConfigInterfaceDescriptionTest.cpp new file mode 100644 index 0000000000000..59b226e5d783d --- /dev/null +++ b/fboss/cli/fboss2/test/CmdConfigInterfaceDescriptionTest.cpp @@ -0,0 +1,90 @@ +// (c) Facebook, Inc. and its affiliates. Confidential and proprietary. + +#include +#include +#include + +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" + +using namespace ::testing; + +namespace facebook::fboss { + +class CmdConfigInterfaceDescriptionTestFixture : public ::testing::Test { + public: + void SetUp() override {} +}; + +TEST_F(CmdConfigInterfaceDescriptionTestFixture, printOutputSuccess) { + auto cmd = CmdConfigInterfaceDescription(); + std::string successMessage = + "Successfully set description for interface(s) eth1/1/1 to \"Test description\" and reloaded config"; + + // Redirect cout to capture output + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + + cmd.printOutput(successMessage); + + // Restore cout + std::cout.rdbuf(old); + + std::string output = buffer.str(); + std::string expectedOutput = + "Successfully set description for interface(s) eth1/1/1 to \"Test description\" and reloaded config\n"; + + EXPECT_EQ(output, expectedOutput); +} + +TEST_F(CmdConfigInterfaceDescriptionTestFixture, printOutputMultiplePorts) { + auto cmd = CmdConfigInterfaceDescription(); + std::string successMessage = + "Successfully set description for interface(s) eth1/1/1, eth1/2/1 to \"Multi-port test\" and reloaded config"; + + // Redirect cout to capture output + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + + cmd.printOutput(successMessage); + + // Restore cout + std::cout.rdbuf(old); + + std::string output = buffer.str(); + std::string expectedOutput = + "Successfully set description for interface(s) eth1/1/1, eth1/2/1 to \"Multi-port test\" and reloaded config\n"; + + EXPECT_EQ(output, expectedOutput); +} + +TEST_F(CmdConfigInterfaceDescriptionTestFixture, errorOnNonExistentPort) { + auto cmd = CmdConfigInterfaceDescription(); + + // Test that attempting to set description on a non-existent port throws an + // error This is important because we cannot create arbitrary ports - they + // must exist in the platform mapping (hardware configuration) + + // Note: This test would require mocking the queryClient method to test the + // actual error behavior. For now, we just verify the printOutput works + // correctly. + std::string errorMessage = + "Port(s) not found in configuration: eth1/99/1. Ports must exist in the " + "hardware platform mapping and be defined in the configuration before " + "setting their description."; + + // Redirect cout to capture output + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + + cmd.printOutput(errorMessage); + + // Restore cout + std::cout.rdbuf(old); + + std::string output = buffer.str(); + std::string expectedOutput = errorMessage + "\n"; + + EXPECT_EQ(output, expectedOutput); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/utils/CmdUtilsCommon.h b/fboss/cli/fboss2/utils/CmdUtilsCommon.h index 340b77dde69ee..b56c0808ed8fd 100644 --- a/fboss/cli/fboss2/utils/CmdUtilsCommon.h +++ b/fboss/cli/fboss2/utils/CmdUtilsCommon.h @@ -59,6 +59,7 @@ enum class ObjectArgTypeId : uint8_t { OBJECT_ARG_TYPE_ID_MIRROR_LIST, OBJECT_ARG_TYPE_LINK_DIRECTION, OBJECT_ARG_TYPE_FAN_PWM, + OBJECT_ARG_TYPE_ID_INTERFACE_LIST, OBJECT_ARG_TYPE_ID_REVISION_LIST, }; diff --git a/fboss/cli/fboss2/utils/InterfaceList.cpp b/fboss/cli/fboss2/utils/InterfaceList.cpp new file mode 100644 index 0000000000000..6c6e4292dffb6 --- /dev/null +++ b/fboss/cli/fboss2/utils/InterfaceList.cpp @@ -0,0 +1,65 @@ +/* + * 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/utils/InterfaceList.h" +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/utils/PortMap.h" + +namespace facebook::fboss::utils { + +InterfaceList::InterfaceList(std::vector names) + : names_(std::move(names)) { + // Get the PortMap from the session + auto& portMap = ConfigSession::getInstance().getPortMap(); + + // Resolve names to Intf objects + std::vector notFound; + + for (const auto& name : names_) { + Intf intf; + + // First try to look up as a port name + cfg::Port* port = portMap.getPort(name); + if (port) { + intf.setPort(port); + // Also try to get the associated interface + auto interfaceId = portMap.getInterfaceIdForPort(name); + if (interfaceId) { + cfg::Interface* interface = portMap.getInterface(*interfaceId); + if (interface) { + intf.setInterface(interface); + } + } + } else { + // If not found as a port name, try as an interface name + cfg::Interface* interface = portMap.getInterfaceByName(name); + if (interface) { + intf.setInterface(interface); + } + } + + if (!intf.isValid()) { + notFound.push_back(name); + } else { + data_.push_back(intf); + } + } + + if (!notFound.empty()) { + throw std::invalid_argument( + "Port(s) or interface(s) not found in configuration: " + + folly::join(", ", notFound) + + ". Ports must exist in the hardware platform mapping and be defined " + "in the configuration before they can be configured."); + } +} + +} // namespace facebook::fboss::utils diff --git a/fboss/cli/fboss2/utils/InterfaceList.h b/fboss/cli/fboss2/utils/InterfaceList.h new file mode 100644 index 0000000000000..d2b6f2ede565b --- /dev/null +++ b/fboss/cli/fboss2/utils/InterfaceList.h @@ -0,0 +1,79 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "fboss/agent/if/gen-cpp2/ctrl_types.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" + +namespace facebook::fboss::utils { + +/* + * Intf represents a unified interface/port object that can contain + * a pointer to a cfg::Port, a cfg::Interface, or both. + */ +class Intf { + public: + Intf() : port_(nullptr), interface_(nullptr) {} + + /* Get the Port pointer (may be nullptr). */ + cfg::Port* getPort() const { + return port_; + } + + /* Get the Interface pointer (may be nullptr). */ + cfg::Interface* getInterface() const { + return interface_; + } + + /* Set the Port pointer. */ + void setPort(cfg::Port* port) { + port_ = port; + } + + /* Set the Interface pointer. */ + void setInterface(cfg::Interface* interface) { + interface_ = interface; + } + + /* Check if this Intf has either a Port or Interface. */ + bool isValid() const { + return port_ != nullptr || interface_ != nullptr; + } + + private: + cfg::Port* port_; + cfg::Interface* interface_; +}; + +/* + * InterfaceList resolves port/interface names to Intf objects. + * For each name, it looks up both the port and the interface. + * First tries to look up as a port name, then as an interface name. + */ +class InterfaceList : public BaseObjectArgType { + public: + /* implicit */ InterfaceList(std::vector names); + + /* Get the original names provided by the user. */ + const std::vector& getNames() const { + return names_; + } + + const static ObjectArgTypeId id = + ObjectArgTypeId::OBJECT_ARG_TYPE_ID_INTERFACE_LIST; + + private: + std::vector names_; +}; + +} // namespace facebook::fboss::utils From 6f63db5faf302523253ed64a7820219e45d16b33 Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Tue, 23 Dec 2025 17:15:49 +0000 Subject: [PATCH 4/9] Add a `fboss2 config interface mtu ` command. --- cmake/CliFboss2.cmake | 2 + cmake/CliFboss2Test.cmake | 1 + fboss/cli/fboss2/BUCK | 2 + fboss/cli/fboss2/CmdHandlerImplConfig.cpp | 3 + fboss/cli/fboss2/CmdListConfig.cpp | 17 ++- fboss/cli/fboss2/CmdSubcommands.cpp | 3 + .../interface/CmdConfigInterfaceMtu.cpp | 48 +++++++ .../config/interface/CmdConfigInterfaceMtu.h | 77 +++++++++++ fboss/cli/fboss2/test/BUCK | 1 + .../fboss2/test/CmdConfigInterfaceMtuTest.cpp | 129 ++++++++++++++++++ fboss/cli/fboss2/utils/CmdUtilsCommon.h | 1 + 11 files changed, 279 insertions(+), 5 deletions(-) create mode 100644 fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.cpp create mode 100644 fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h create mode 100644 fboss/cli/fboss2/test/CmdConfigInterfaceMtuTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index e4348aba326d0..72d469054b26a 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -580,6 +580,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.cpp + fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h + fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.cpp fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index 9f6551dd3e1f6..264ea0b904ade 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -6,6 +6,7 @@ add_executable(fboss2_cmd_test fboss/cli/fboss2/test/CmdConfigAppliedInfoTest.cpp fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp fboss/cli/fboss2/test/CmdConfigInterfaceDescriptionTest.cpp + fboss/cli/fboss2/test/CmdConfigInterfaceMtuTest.cpp fboss/cli/fboss2/test/CmdConfigReloadTest.cpp fboss/cli/fboss2/test/CmdConfigSessionDiffTest.cpp fboss/cli/fboss2/test/CmdConfigSessionTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index bb73e49db0668..6a53b568b0abf 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -775,6 +775,7 @@ cpp_library( "commands/config/CmdConfigReload.cpp", "commands/config/history/CmdConfigHistory.cpp", "commands/config/interface/CmdConfigInterfaceDescription.cpp", + "commands/config/interface/CmdConfigInterfaceMtu.cpp", "commands/config/rollback/CmdConfigRollback.cpp", "commands/config/session/CmdConfigSessionCommit.cpp", "commands/config/session/CmdConfigSessionDiff.cpp", @@ -786,6 +787,7 @@ cpp_library( "commands/config/history/CmdConfigHistory.h", "commands/config/interface/CmdConfigInterface.h", "commands/config/interface/CmdConfigInterfaceDescription.h", + "commands/config/interface/CmdConfigInterfaceMtu.h", "commands/config/rollback/CmdConfigRollback.h", "commands/config/session/CmdConfigSessionCommit.h", "commands/config/session/CmdConfigSessionDiff.h", diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 9b940aeae417d..133893761ad65 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -15,6 +15,7 @@ #include "fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -28,6 +29,8 @@ template void CmdHandler::run(); template void CmdHandler< CmdConfigInterfaceDescription, CmdConfigInterfaceDescriptionTraits>::run(); +template void +CmdHandler::run(); template void CmdHandler::run(); template void CmdHandler::run(); template void diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index d06c7fb4370b2..05e070f392c0c 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -16,6 +16,7 @@ #include "fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -43,11 +44,17 @@ const CommandTree& kConfigCommandTree() { commandHandler, argTypeHandler, {{ - "description", - "Set interface description", - commandHandler, - argTypeHandler, - }}, + "description", + "Set interface description", + commandHandler, + argTypeHandler, + }, + { + "mtu", + "Set interface MTU", + commandHandler, + argTypeHandler, + }}, }, { diff --git a/fboss/cli/fboss2/CmdSubcommands.cpp b/fboss/cli/fboss2/CmdSubcommands.cpp index d1232311a866d..dd84da46788be 100644 --- a/fboss/cli/fboss2/CmdSubcommands.cpp +++ b/fboss/cli/fboss2/CmdSubcommands.cpp @@ -219,6 +219,9 @@ CLI::App* CmdSubcommands::addCommand( case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_FAN_PWM: subCmd->add_option("pwm", args, "Fan PWM (0..100) or 'disable'"); break; + case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_MTU: + subCmd->add_option("mtu", args, "MTU value (68-9216)"); + break; case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_INTERFACE_LIST: subCmd->add_option("interfaces", args, "Interface(s)"); break; diff --git a/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.cpp b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.cpp new file mode 100644 index 0000000000000..a7344c63f7e01 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.cpp @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" + +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +CmdConfigInterfaceMtuTraits::RetType CmdConfigInterfaceMtu::queryClient( + const HostInfo& hostInfo, + const utils::InterfaceList& interfaces, + const CmdConfigInterfaceMtuTraits::ObjectArgType& mtuValue) { + // Extract the MTU value (validation already done in MtuValue constructor) + int32_t mtu = mtuValue.getMtu(); + + // Update MTU for all resolved interfaces + for (const utils::Intf& intf : interfaces) { + cfg::Interface* interface = intf.getInterface(); + if (interface) { + interface->mtu() = mtu; + } + } + + // Save the updated config + ConfigSession::getInstance().saveConfig(); + + std::string interfaceList = folly::join(", ", interfaces.getNames()); + std::string message = "Successfully set MTU for interface(s) " + + interfaceList + " to " + std::to_string(mtu); + + return message; +} + +void CmdConfigInterfaceMtu::printOutput( + const CmdConfigInterfaceMtuTraits::RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h new file mode 100644 index 0000000000000..15773e0ba5c0e --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h @@ -0,0 +1,77 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" +#include "fboss/cli/fboss2/utils/InterfaceList.h" + +namespace facebook::fboss { + +// Custom type for MTU argument with validation +class MtuValue : public utils::BaseObjectArgType { + public: + /* implicit */ MtuValue(std::vector v) { + if (v.empty()) { + throw std::invalid_argument("MTU value is required"); + } + if (v.size() != 1) { + throw std::invalid_argument( + "Expected single MTU value, got: " + folly::join(", ", v)); + } + + try { + int32_t mtu = folly::to(v[0]); + if (mtu < 68 || mtu > 9216) { + throw std::invalid_argument( + "MTU must be between 68 and 9216 inclusive, got: " + + std::to_string(mtu)); + } + data_.push_back(mtu); + } catch (const folly::ConversionError& e) { + throw std::invalid_argument("Invalid MTU value: " + v[0]); + } + } + + int32_t getMtu() const { + return data_[0]; + } + + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_MTU; +}; + +struct CmdConfigInterfaceMtuTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigInterface; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_MTU; + using ObjectArgType = MtuValue; + using RetType = std::string; +}; + +class CmdConfigInterfaceMtu + : public CmdHandler { + public: + using ObjectArgType = CmdConfigInterfaceMtuTraits::ObjectArgType; + using RetType = CmdConfigInterfaceMtuTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const utils::InterfaceList& interfaces, + const ObjectArgType& mtu); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/BUCK b/fboss/cli/fboss2/test/BUCK index 9046f3d987072..c4ef46f504bc1 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -58,6 +58,7 @@ cpp_unittest( "CmdConfigAppliedInfoTest.cpp", "CmdConfigHistoryTest.cpp", "CmdConfigInterfaceDescriptionTest.cpp", + "CmdConfigInterfaceMtuTest.cpp", "CmdConfigReloadTest.cpp", "CmdConfigSessionDiffTest.cpp", "CmdConfigSessionTest.cpp", diff --git a/fboss/cli/fboss2/test/CmdConfigInterfaceMtuTest.cpp b/fboss/cli/fboss2/test/CmdConfigInterfaceMtuTest.cpp new file mode 100644 index 0000000000000..ca2a4adaa8207 --- /dev/null +++ b/fboss/cli/fboss2/test/CmdConfigInterfaceMtuTest.cpp @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" +#include + +namespace facebook::fboss { + +class CmdConfigInterfaceMtuTestFixture : public ::testing::Test {}; + +TEST_F(CmdConfigInterfaceMtuTestFixture, printOutputSuccess) { + auto cmd = CmdConfigInterfaceMtu(); + std::string successMessage = + "Successfully set MTU for interface(s) eth1/1/1 to 1500. Run 'fboss2 config session commit' to apply changes."; + + // Redirect cout to capture output + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + + cmd.printOutput(successMessage); + + // Restore cout + std::cout.rdbuf(old); + + std::string output = buffer.str(); + std::string expectedOutput = + "Successfully set MTU for interface(s) eth1/1/1 to 1500. Run 'fboss2 config session commit' to apply changes.\n"; + + EXPECT_EQ(output, expectedOutput); +} + +TEST_F(CmdConfigInterfaceMtuTestFixture, printOutputMultipleInterfaces) { + auto cmd = CmdConfigInterfaceMtu(); + std::string successMessage = + "Successfully set MTU for interface(s) eth1/1/1, eth1/2/1 to 9000. Run 'fboss2 config session commit' to apply changes."; + + // Redirect cout to capture output + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + + cmd.printOutput(successMessage); + + // Restore cout + std::cout.rdbuf(old); + + std::string output = buffer.str(); + std::string expectedOutput = + "Successfully set MTU for interface(s) eth1/1/1, eth1/2/1 to 9000. Run 'fboss2 config session commit' to apply changes.\n"; + + EXPECT_EQ(output, expectedOutput); +} + +TEST_F(CmdConfigInterfaceMtuTestFixture, errorOnNonExistentInterface) { + auto cmd = CmdConfigInterfaceMtu(); + + // Test that attempting to set MTU on a non-existent interface throws an error + // This is important because we cannot create arbitrary interfaces - they must + // be defined in the configuration + std::string errorMessage = + "Interface(s) not found in configuration: eth1/99/1. Interfaces must be " + "defined in the configuration before setting their MTU."; + + // Redirect cout to capture output + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + + cmd.printOutput(errorMessage); + + // Restore cout + std::cout.rdbuf(old); + + std::string output = buffer.str(); + std::string expectedOutput = errorMessage + "\n"; + + EXPECT_EQ(output, expectedOutput); +} + +TEST_F(CmdConfigInterfaceMtuTestFixture, errorOnInvalidMtuTooLow) { + auto cmd = CmdConfigInterfaceMtu(); + + // Test that MTU validation works for values below minimum + std::string errorMessage = + "MTU must be between 68 and 9216 inclusive, got: 67"; + + // Redirect cout to capture output + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + + cmd.printOutput(errorMessage); + + // Restore cout + std::cout.rdbuf(old); + + std::string output = buffer.str(); + std::string expectedOutput = errorMessage + "\n"; + + EXPECT_EQ(output, expectedOutput); +} + +TEST_F(CmdConfigInterfaceMtuTestFixture, errorOnInvalidMtuTooHigh) { + auto cmd = CmdConfigInterfaceMtu(); + + // Test that MTU validation works for values above maximum + std::string errorMessage = + "MTU must be between 68 and 9216 inclusive, got: 9217"; + + // Redirect cout to capture output + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + + cmd.printOutput(errorMessage); + + // Restore cout + std::cout.rdbuf(old); + + std::string output = buffer.str(); + std::string expectedOutput = errorMessage + "\n"; + + EXPECT_EQ(output, expectedOutput); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/utils/CmdUtilsCommon.h b/fboss/cli/fboss2/utils/CmdUtilsCommon.h index b56c0808ed8fd..028f8e8cf94d9 100644 --- a/fboss/cli/fboss2/utils/CmdUtilsCommon.h +++ b/fboss/cli/fboss2/utils/CmdUtilsCommon.h @@ -59,6 +59,7 @@ enum class ObjectArgTypeId : uint8_t { OBJECT_ARG_TYPE_ID_MIRROR_LIST, OBJECT_ARG_TYPE_LINK_DIRECTION, OBJECT_ARG_TYPE_FAN_PWM, + OBJECT_ARG_TYPE_MTU, OBJECT_ARG_TYPE_ID_INTERFACE_LIST, OBJECT_ARG_TYPE_ID_REVISION_LIST, }; From 96c09a332e2d3bb5a73ddef5f7373390ecca8bed Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Wed, 7 Jan 2026 20:02:53 +0000 Subject: [PATCH 5/9] Add some end-to-end tests for the CLI --- fboss/oss/cli_tests/cli_test_lib.py | 241 ++++++++++++++++++ fboss/oss/cli_tests/test_cli_test_lib.py | 149 +++++++++++ .../test_config_interface_description.py | 96 +++++++ .../cli_tests/test_config_interface_mtu.py | 122 +++++++++ fboss/oss/scripts/run_scripts/run_test.py | 121 +++++++++ 5 files changed, 729 insertions(+) create mode 100644 fboss/oss/cli_tests/cli_test_lib.py create mode 100644 fboss/oss/cli_tests/test_cli_test_lib.py create mode 100644 fboss/oss/cli_tests/test_config_interface_description.py create mode 100644 fboss/oss/cli_tests/test_config_interface_mtu.py diff --git a/fboss/oss/cli_tests/cli_test_lib.py b/fboss/oss/cli_tests/cli_test_lib.py new file mode 100644 index 0000000000000..be38ebcf1ec2b --- /dev/null +++ b/fboss/oss/cli_tests/cli_test_lib.py @@ -0,0 +1,241 @@ +#!/usr/bin/env python3 +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the BSD-style license found in the +# LICENSE file in the root directory of this source tree. + +""" +Common library for CLI end-to-end tests. + +This module provides shared utilities for CLI tests including: +- Finding and running the fboss2-dev CLI binary +- Parsing interface information from CLI output +- Running shell commands with proper error handling +""" + +import json +import os +import subprocess +import time +from dataclasses import dataclass +from typing import Any, Callable, Optional + + +@dataclass +class Interface: + """Represents a network interface from the output of 'show interface'.""" + + name: str + status: str + speed: str + vlan: Optional[int] + mtu: int + addresses: list[str] # IPv4 and IPv6 addresses assigned to the interface + description: str + + @staticmethod + def from_json(data: dict[str, Any]) -> "Interface": + """ + Parse interface data from JSON into an Interface object. + + The JSON format from 'fboss2 --fmt json show interface' is: + { + "name": "eth1/1/1", + "description": "...", + "status": "down", + "speed": "800G", + "vlan": 2001, + "mtu": 1500, + "prefixes": [{"ip": "10.0.0.0", "prefixLength": 24}, ...], + ... + } + """ + # Convert prefixes to "ip/prefixLength" format + prefixes = data.get("prefixes", []) + addresses = [f"{p['ip']}/{p['prefixLength']}" for p in prefixes] + + return Interface( + name=data["name"], + status=data["status"], + speed=data["speed"], + vlan=data.get("vlan"), + mtu=data["mtu"], + addresses=addresses, + description=data.get("description", ""), + ) + + +# CLI binary path - can be overridden via FBOSS_CLI_PATH environment variable +_FBOSS_CLI = None + + +def get_fboss_cli() -> str: + """ + Get the path to the FBOSS CLI binary. + + The path can be overridden by setting the FBOSS_CLI_PATH environment variable. + Example: FBOSS_CLI_PATH=/tmp/fboss2-dev python3 test_config_interface_mtu.py + """ + global _FBOSS_CLI + if _FBOSS_CLI is not None: + return _FBOSS_CLI + + # Check if path is specified via environment variable + env_path = os.environ.get("FBOSS_CLI_PATH") + if env_path: + expanded = os.path.expanduser(env_path) + if os.path.isfile(expanded) and os.access(expanded, os.X_OK): + _FBOSS_CLI = expanded + print(f" Using CLI from FBOSS_CLI_PATH: {_FBOSS_CLI}") + return _FBOSS_CLI + else: + raise RuntimeError( + f"FBOSS_CLI_PATH is set to '{env_path}' but the file does not exist " + "or is not executable" + ) + + # Default locations (only fboss2-dev has config commands) + candidates = ( + "/opt/fboss/bin/fboss2-dev", + "fboss2-dev", + ) + + for candidate in candidates: + if os.path.isabs(candidate): + if os.path.isfile(candidate) and os.access(candidate, os.X_OK): + _FBOSS_CLI = candidate + return _FBOSS_CLI + else: + # Check if it's in PATH + result = subprocess.run( + ["which", candidate], capture_output=True, text=True + ) + if result.returncode == 0: + _FBOSS_CLI = result.stdout.strip() + return _FBOSS_CLI + + raise RuntimeError( + "Could not find fboss2-dev CLI binary. " + "Set FBOSS_CLI_PATH environment variable to specify the path." + ) + + +def run_cmd(cmd: list[str], check: bool = True) -> subprocess.CompletedProcess: + """Run a command and return the result.""" + print(f"Running: {' '.join(cmd)}") + result = subprocess.run(cmd, capture_output=True, text=True) + if check and result.returncode != 0: + print(f"Command failed with return code {result.returncode}") + print(f"stdout: {result.stdout}") + print(f"stderr: {result.stderr}") + raise RuntimeError(f"Command failed: {' '.join(cmd)}") + return result + + +def run_cli(args: list[str], check: bool = True) -> dict[str, Any]: + """Run the fboss2-dev CLI with the given arguments. + + The --fmt json flag is automatically prepended to all commands. + Returns the parsed JSON output as a dict. + """ + cli = get_fboss_cli() + cmd = [cli, "--fmt", "json"] + args + print(f"[CLI] Running: {' '.join(args)}") + start_time = time.time() + result = subprocess.run(cmd, capture_output=True, text=True) + elapsed = time.time() - start_time + print(f"[CLI] Completed in {elapsed:.2f}s: {' '.join(args)}") + if check and result.returncode != 0: + print(f"Command failed with return code {result.returncode}") + print(f"stdout: {result.stdout}") + print(f"stderr: {result.stderr}") + raise RuntimeError(f"Command failed: {' '.join(cmd)}") + return json.loads(result.stdout) if result.stdout.strip() else {} + + +def _get_interfaces(interface_name: Optional[str] = None) -> dict[str, Interface]: + """ + Get interface information from 'fboss2-dev show interface [name]'. + + Args: + interface_name: Optional interface name. If None, gets all interfaces. + + Returns a dict mapping interface name to Interface object. + """ + args = ["show", "interface"] + if interface_name is not None: + args.append(interface_name) + + data = run_cli(args) + + # The JSON has a host key (e.g., "127.0.0.1") containing the interfaces + interfaces: dict[str, Interface] = {} + for host_data in data.values(): + for intf_data in host_data.get("interfaces", []): + intf = Interface.from_json(intf_data) + assert intf.name not in interfaces, f"Duplicate interface name: {intf.name}" + interfaces[intf.name] = intf + + return interfaces + + +def get_all_interfaces() -> dict[str, Interface]: + """ + Get all interface information from 'fboss2-dev show interface'. + Returns a dict mapping interface name to Interface object. + """ + return _get_interfaces() + + +def get_interface_info(interface_name: str) -> Interface: + """ + Get interface information from 'fboss2-dev show interface '. + Returns an Interface object. + """ + interfaces = _get_interfaces(interface_name) + + if interface_name not in interfaces: + raise RuntimeError(f"Could not find interface {interface_name} in output") + + return interfaces[interface_name] + + +def find_interfaces(predicate: Callable[[Interface], bool]) -> list[Interface]: + """ + Find all interfaces matching the given predicate. + + Args: + predicate: A callable that takes an Interface and returns True + if the interface should be included in the results. + + Returns: + A list of Interface objects for all matching interfaces. + + This calls the CLI only once via get_all_interfaces(). + """ + all_interfaces = get_all_interfaces() + return [intf for intf in all_interfaces.values() if predicate(intf)] + + +def find_first_eth_interface() -> Interface: + """ + Find the first suitable ethernet interface. + Returns an Interface object. + + Only returns ethernet interfaces (starting with 'eth') with a valid VLAN > 1. + """ + + def is_valid_eth_interface(intf: Interface) -> bool: + return intf.name.startswith("eth") and intf.vlan is not None and intf.vlan > 1 + + matches = find_interfaces(is_valid_eth_interface) + + if not matches: + raise RuntimeError("No suitable ethernet interface found with VLAN > 1") + + return matches[0] + + +def commit_config() -> None: + """Commit the current configuration session.""" + run_cli(["config", "session", "commit"]) diff --git a/fboss/oss/cli_tests/test_cli_test_lib.py b/fboss/oss/cli_tests/test_cli_test_lib.py new file mode 100644 index 0000000000000..6e9c7f257898a --- /dev/null +++ b/fboss/oss/cli_tests/test_cli_test_lib.py @@ -0,0 +1,149 @@ +#!/usr/bin/env python3 +# Copyright (c) Meta Platforms, Inc. and affiliates. +# 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. + +""" +Unit tests for cli_test_lib.py parsing logic. + +These tests use static CLI output samples to verify the Interface parsing +without requiring a live switch connection. +""" + +import unittest +from unittest.mock import patch + +from cli_test_lib import get_all_interfaces, get_interface_info + + +# Sample parsed JSON data from 'fboss2 --fmt json show interface' +SAMPLE_JSON_DATA = { + "127.0.0.1": { + "interfaces": [ + { + "name": "eth1/1/1", + "description": "this", + "status": "down", + "speed": "800G", + "vlan": 2001, + "mtu": 1500, + "prefixes": [ + {"ip": "10.0.0.0", "prefixLength": 24}, + {"ip": "2400::", "prefixLength": 64}, + {"ip": "fe80::b4db:91ff:fe95:ff07", "prefixLength": 64}, + ], + }, + { + "name": "eth1/2/1", + "description": "Another test description", + "status": "up", + "speed": "200G", + "vlan": 2003, + "mtu": 9216, + "prefixes": [ + {"ip": "11.0.0.0", "prefixLength": 24}, + {"ip": "2401::", "prefixLength": 64}, + ], + }, + { + "name": "eth1/3/1", + "description": "", + "status": "down", + "speed": "400G", + "vlan": 2005, + "mtu": 9000, + "prefixes": [], + }, + ] + } +} + + +class TestGetAllInterfaces(unittest.TestCase): + """Tests for get_all_interfaces() with mocked CLI.""" + + @patch("cli_test_lib.run_cli") + def test_get_all_interfaces(self, mock_run_cli): + """Test parsing all interfaces.""" + mock_run_cli.return_value = SAMPLE_JSON_DATA + + interfaces = get_all_interfaces() + + self.assertEqual(len(interfaces), 3) + self.assertIn("eth1/1/1", interfaces) + self.assertIn("eth1/2/1", interfaces) + self.assertIn("eth1/3/1", interfaces) + + @patch("cli_test_lib.run_cli") + def test_parse_interface_fields(self, mock_run_cli): + """Test that interface fields are correctly parsed.""" + mock_run_cli.return_value = SAMPLE_JSON_DATA + + interfaces = get_all_interfaces() + + intf = interfaces["eth1/1/1"] + self.assertEqual(intf.name, "eth1/1/1") + self.assertEqual(intf.status, "down") + self.assertEqual(intf.speed, "800G") + self.assertEqual(intf.vlan, 2001) + self.assertEqual(intf.mtu, 1500) + self.assertEqual( + intf.addresses, + ["10.0.0.0/24", "2400::/64", "fe80::b4db:91ff:fe95:ff07/64"], + ) + self.assertEqual(intf.description, "this") + + @patch("cli_test_lib.run_cli") + def test_parse_interface_no_addresses(self, mock_run_cli): + """Test parsing an interface with no addresses.""" + mock_run_cli.return_value = SAMPLE_JSON_DATA + + interfaces = get_all_interfaces() + + intf = interfaces["eth1/3/1"] + self.assertEqual(intf.name, "eth1/3/1") + self.assertEqual(intf.addresses, []) + + @patch("cli_test_lib.run_cli") + def test_empty_data(self, mock_run_cli): + """Test that empty data returns empty dict.""" + mock_run_cli.return_value = {} + + interfaces = get_all_interfaces() + self.assertEqual(interfaces, {}) + + @patch("cli_test_lib.run_cli") + def test_empty_interfaces(self, mock_run_cli): + """Test that empty interfaces list returns empty dict.""" + mock_run_cli.return_value = {"127.0.0.1": {"interfaces": []}} + + interfaces = get_all_interfaces() + self.assertEqual(interfaces, {}) + + +class TestGetInterfaceInfo(unittest.TestCase): + """Tests for get_interface_info() with mocked CLI.""" + + @patch("cli_test_lib.run_cli") + def test_get_interface_info(self, mock_run_cli): + """Test getting a single interface.""" + mock_run_cli.return_value = SAMPLE_JSON_DATA + + intf = get_interface_info("eth1/1/1") + + self.assertEqual(intf.name, "eth1/1/1") + self.assertEqual(intf.mtu, 1500) + + @patch("cli_test_lib.run_cli") + def test_get_interface_info_not_found(self, mock_run_cli): + """Test error when interface not found.""" + mock_run_cli.return_value = SAMPLE_JSON_DATA + + with self.assertRaises(RuntimeError): + get_interface_info("nonexistent") + + +if __name__ == "__main__": + unittest.main() diff --git a/fboss/oss/cli_tests/test_config_interface_description.py b/fboss/oss/cli_tests/test_config_interface_description.py new file mode 100644 index 0000000000000..a55701b33c966 --- /dev/null +++ b/fboss/oss/cli_tests/test_config_interface_description.py @@ -0,0 +1,96 @@ +#!/usr/bin/env python3 +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the BSD-style license found in the +# LICENSE file in the root directory of this source tree. + +""" +End-to-end test for the 'fboss2-dev config interface description ' command. + +This test: +1. Picks an interface from the running system +2. Gets the current description +3. Sets a new description +4. Verifies the description was set correctly via 'fboss2-dev show interface' +5. Restores the original description + +Requirements: +- FBOSS agent must be running with a valid configuration +- The test must be run as root (or with appropriate permissions) +""" + +import sys + +from cli_test_lib import ( + commit_config, + find_first_eth_interface, + get_interface_info, + run_cli, +) + + +def get_interface_description(interface_name: str) -> str: + """Get the current description for an interface.""" + info = get_interface_info(interface_name) + return info.description + + +def set_interface_description(interface_name: str, description: str) -> None: + """Set the description for an interface and commit the change.""" + run_cli(["config", "interface", interface_name, "description", description]) + commit_config() + + +def main() -> int: + print("=" * 60) + print("CLI E2E Test: config interface description ") + print("=" * 60) + + # Step 1: Get an interface to test with + print("\n[Step 1] Finding an interface to test...") + interface = find_first_eth_interface() + print(f" Using interface: {interface.name} (VLAN: {interface.vlan})") + + # Step 2: Get the current description + print("\n[Step 2] Getting current description...") + original_description = get_interface_description(interface.name) + print(f" Current description: '{original_description}'") + + # Step 3: Set a new description + test_description = "CLI_E2E_TEST_DESCRIPTION" + if original_description == test_description: + test_description = "CLI_E2E_TEST_DESCRIPTION_ALT" + print(f"\n[Step 3] Setting description to '{test_description}'...") + set_interface_description(interface.name, test_description) + print(f" Description set to '{test_description}'") + + # Step 4: Verify description via 'show interface' + print("\n[Step 4] Verifying description via 'show interface'...") + actual_description = get_interface_description(interface.name) + if actual_description != test_description: + print( + f" ERROR: Expected description '{test_description}', got '{actual_description}'" + ) + return 1 + print(f" Verified: Description is '{actual_description}'") + + # Step 5: Restore original description + print(f"\n[Step 5] Restoring original description ('{original_description}')...") + set_interface_description(interface.name, original_description) + print(f" Restored description to '{original_description}'") + + # Verify restoration + restored_description = get_interface_description(interface.name) + if restored_description != original_description: + print( + f" WARNING: Restoration may have failed. Current: '{restored_description}'" + ) + + print("\n" + "=" * 60) + print("TEST PASSED") + print("=" * 60) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/fboss/oss/cli_tests/test_config_interface_mtu.py b/fboss/oss/cli_tests/test_config_interface_mtu.py new file mode 100644 index 0000000000000..f860758cfd5ab --- /dev/null +++ b/fboss/oss/cli_tests/test_config_interface_mtu.py @@ -0,0 +1,122 @@ +#!/usr/bin/env python3 +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the BSD-style license found in the +# LICENSE file in the root directory of this source tree. + +""" +End-to-end test for the 'fboss2-dev config interface mtu ' command. + +This test: +1. Picks an interface from the running system +2. Gets the current MTU value +3. Sets a new MTU value +4. Verifies the MTU was set correctly via 'fboss2-dev show interface' +5. Verifies the MTU on the kernel interface via 'ip link' +6. Restores the original MTU + +Requirements: +- FBOSS agent must be running with a valid configuration +- The test must be run as root (or with appropriate permissions) +""" + +import json +import sys + +from cli_test_lib import ( + commit_config, + find_first_eth_interface, + get_interface_info, + run_cli, + run_cmd, +) + + +def get_interface_mtu(interface_name: str) -> int: + """Get the current MTU for an interface.""" + info = get_interface_info(interface_name) + if info.mtu is None: + raise RuntimeError(f"Could not find MTU for interface {interface_name}") + return info.mtu + + +def get_kernel_interface_mtu(vlan_id: int) -> int: + """Get the MTU of the kernel interface (fboss) using 'ip -json link'.""" + kernel_intf = f"fboss{vlan_id}" + result = run_cmd(["ip", "-json", "link", "show", kernel_intf], check=False) + + if result.returncode != 0: + print(f"Warning: Kernel interface {kernel_intf} not found") + return -1 + + data = json.loads(result.stdout) + if not data: + raise RuntimeError(f"No data returned for kernel interface {kernel_intf}") + + mtu = data[0]["mtu"] + assert mtu, "MTU can't be zero" + return mtu + + +def set_interface_mtu(interface_name: str, mtu: int) -> None: + """Set the MTU for an interface and commit the change.""" + run_cli(["config", "interface", interface_name, "mtu", str(mtu)]) + commit_config() + + +def main() -> int: + print("=" * 60) + print("CLI E2E Test: config interface mtu ") + print("=" * 60) + + # Step 1: Get an interface to test with + print("\n[Step 1] Finding an interface to test...") + interface = find_first_eth_interface() + print(f" Using interface: {interface.name} (VLAN: {interface.vlan})") + + # Step 2: Get the current MTU + print("\n[Step 2] Getting current MTU...") + original_mtu = get_interface_mtu(interface.name) + print(f" Current MTU: {original_mtu}") + + # Step 3: Set a new MTU (toggle between 1500 and 9000) + new_mtu = 9000 if original_mtu != 9000 else 1500 + print(f"\n[Step 3] Setting MTU to {new_mtu}...") + set_interface_mtu(interface.name, new_mtu) + print(f" MTU set to {new_mtu}") + + # Step 4: Verify MTU via 'show interface' + print("\n[Step 4] Verifying MTU via 'show interface'...") + actual_mtu = get_interface_mtu(interface.name) + if actual_mtu != new_mtu: + print(f" ERROR: Expected MTU {new_mtu}, got {actual_mtu}") + return 1 + print(f" Verified: MTU is {actual_mtu}") + + # Step 5: Verify kernel interface MTU + print("\n[Step 5] Verifying kernel interface MTU...") + assert interface.vlan is not None # Guaranteed by find_first_eth_interface + kernel_mtu = get_kernel_interface_mtu(interface.vlan) + if kernel_mtu > 0: + if kernel_mtu != new_mtu: + print(f" ERROR: Kernel MTU is {kernel_mtu}, expected {new_mtu}") + return 1 + print( + f" Verified: Kernel interface fboss{interface.vlan} has MTU {kernel_mtu}" + ) + else: + print(f" Skipped: Kernel interface fboss{interface.vlan} not found") + + # Step 6: Restore original MTU + print(f"\n[Step 6] Restoring original MTU ({original_mtu})...") + set_interface_mtu(interface.name, original_mtu) + print(f" Restored MTU to {original_mtu}") + + print("\n" + "=" * 60) + print("TEST PASSED") + print("=" * 60) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/fboss/oss/scripts/run_scripts/run_test.py b/fboss/oss/scripts/run_scripts/run_test.py index c9a581a99b0a7..ebf603bab8f7b 100755 --- a/fboss/oss/scripts/run_scripts/run_test.py +++ b/fboss/oss/scripts/run_scripts/run_test.py @@ -189,6 +189,7 @@ SUB_CMD_QSFP = "qsfp" SUB_CMD_LINK = "link" SUB_CMD_SAI_AGENT = "sai_agent" +SUB_CMD_CLI = "cli" SUB_ARG_AGENT_RUN_MODE = "--agent-run-mode" SUB_ARG_AGENT_RUN_MODE_MONO = "mono" SUB_ARG_AGENT_RUN_MODE_MULTI = "multi_switch" @@ -1290,6 +1291,119 @@ def _filter_tests(self, tests: List[str]) -> List[str]: return tests_to_run +class CliTestRunner: + """ + Runner for CLI end-to-end tests. + + Unlike the gtest-based test runners, CLI tests are simple Python tests + that run CLI commands and verify output. They test the CLI tool itself + (fboss2-dev) on a running FBOSS instance. + """ + + CLI_TEST_DIR = "./share/cli_tests" + + def run_test(self, args): + """Run CLI end-to-end tests""" + print("Running CLI end-to-end tests...") + + # Find and run test scripts + test_dir = self.CLI_TEST_DIR + if not os.path.isdir(test_dir): + print(f"CLI test directory not found: {test_dir}") + print("No CLI tests to run.") + return + + # Get list of test scripts + test_scripts = [] + for filename in sorted(os.listdir(test_dir)): + if filename.startswith("test_") and filename.endswith(".py"): + test_scripts.append(os.path.join(test_dir, filename)) + + if not test_scripts: + print(f"No CLI test scripts found in {test_dir}") + return + + # Apply filter if specified + if args.filter: + filtered_scripts = [] + for script in test_scripts: + script_name = os.path.basename(script) + if args.filter in script_name: + filtered_scripts.append(script) + test_scripts = filtered_scripts + if not test_scripts: + print(f"No tests match filter: {args.filter}") + return + + # Run each test script + passed = 0 + failed = 0 + failed_tests = [] + test_times = {} # Track time for each test + total_start_time = time.time() + + for test_script in test_scripts: + test_name = os.path.basename(test_script) + print(f"\n########## Running CLI test: {test_name}") + + test_start_time = time.time() + try: + result = subprocess.run( + ["python3", test_script], + capture_output=True, + text=True, + timeout=300, # 5 minute timeout per test + ) + test_elapsed = time.time() - test_start_time + test_times[test_name] = test_elapsed + + if result.returncode == 0: + print(f"[ PASSED ] {test_name} ({test_elapsed:.1f}s)") + passed += 1 + else: + print(f"[ FAILED ] {test_name} ({test_elapsed:.1f}s)") + print(f"stdout: {result.stdout}") + print(f"stderr: {result.stderr}") + failed += 1 + failed_tests.append(test_name) + + except subprocess.TimeoutExpired as e: + test_elapsed = time.time() - test_start_time + test_times[test_name] = test_elapsed + print(f"[ TIMEOUT ] {test_name} ({test_elapsed:.1f}s)") + if e.stdout: + print(f"stdout: {e.stdout}") + if e.stderr: + print(f"stderr: {e.stderr}") + failed += 1 + failed_tests.append(test_name) + except Exception as e: + test_elapsed = time.time() - test_start_time + test_times[test_name] = test_elapsed + print(f"[ ERROR ] {test_name}: {e} ({test_elapsed:.1f}s)") + failed += 1 + failed_tests.append(test_name) + + total_elapsed = time.time() - total_start_time + + # Print summary + print("\n" + "=" * 60) + print("CLI Test Summary") + print("=" * 60) + print(f" Passed: {passed}") + print(f" Failed: {failed}") + print(f" Total: {passed + failed}") + print(f" Time: {total_elapsed:.1f}s") + + if failed_tests: + print("\nFailed tests:") + for test in failed_tests: + print(f" - {test} ({test_times.get(test, 0):.1f}s)") + + if failed > 0: + sys.exit(1) + + if __name__ == "__main__": _check_working_dir() # Set env variables for FBOSS @@ -1486,6 +1600,13 @@ def _filter_tests(self, tests: List[str]) -> List[str]: sai_agent_test_parser.set_defaults(func=sai_agent_test_runner.run_test) sai_agent_test_runner.add_subcommand_arguments(sai_agent_test_parser) + # Add subparser for CLI end-to-end tests + cli_test_parser = subparsers.add_parser( + SUB_CMD_CLI, help="run CLI end-to-end tests" + ) + cli_test_runner = CliTestRunner() + cli_test_parser.set_defaults(func=cli_test_runner.run_test) + # Parse the args args = ap.parse_known_args() args = ap.parse_args(args[1], args[0]) From 77f6f22befc14ec713fb1bceb1ee65d5418a4480 Mon Sep 17 00:00:00 2001 From: Manoharan Sundaramoorthy Date: Mon, 12 Jan 2026 19:55:23 +0530 Subject: [PATCH 6/9] Add buffer-pool configuration support 1. Some of the configuration commands like QoS buffer pool modification requires an agent restart and some are hitless. Inorder to take appropriate action during the commit, track the highest impact action by storing the same in `$HOME/.fboss2/action_level` at the end of each configuration change. The proposed change only prints a warning if the action needs to be taken. Later this can be modified to perform the action automatically 2. Add QoS buffer pool configuration commands ``` - fboss2 config qos buffer-pool shared-bytes - fboss2 config qos buffer-pool headroom-bytes - fboss2 config qos buffer-pool reserved-bytes ``` 1. Updated existing UT for action information 2. Added new tests for QoS buffer bool configurations. ``` [admin@fboss101 ~]$ ~/benoit/fboss2-dev config qos buffer-pool testpool headroom-bytes 1024 Successfully set headroom-bytes for buffer-pool 'testpool' to 1024 [admin@fboss101 ~]$ ~/benoit/fboss2-dev config session diff --- current live config +++ session config @@ -121,6 +121,12 @@ "arpAgerInterval": 5, "arpRefreshSeconds": 20, "arpTimeoutSeconds": 60, + "bufferPoolConfigs": { + "testpool": { + "headroomBytes": 1024, + "sharedBytes": 0 + } + }, "clientIdToAdminDistance": { "0": 20, "1": 1, [admin@fboss101 ~]$ ll ~/.fboss2 total 216 -rw-r--r-- 1 admin admin 213283 Jan 13 05:15 agent.conf -rw-r--r-- 1 admin admin 42 Jan 13 05:15 conf_metadata.json [admin@fboss101 ~]$ cat ~/.fboss2/conf_metadata.json { "action": { "WEDGE_AGENT": "AGENT_RESTART" } } ``` --- cmake/CliFboss2.cmake | 16 + cmake/CliFboss2Test.cmake | 1 + fboss/cli/fboss2/BUCK | 18 + fboss/cli/fboss2/CmdHandlerImplConfig.cpp | 17 + fboss/cli/fboss2/CmdListConfig.cpp | 37 +++ fboss/cli/fboss2/CmdSubcommands.cpp | 6 + fboss/cli/fboss2/cli_metadata.thrift | 35 ++ .../fboss2/commands/config/qos/CmdConfigQos.h | 35 ++ .../qos/buffer_pool/BufferPoolConfigUtils.h | 86 +++++ .../qos/buffer_pool/CmdConfigQosBufferPool.h | 80 +++++ .../CmdConfigQosBufferPoolHeadroomBytes.cpp | 40 +++ .../CmdConfigQosBufferPoolHeadroomBytes.h | 45 +++ .../CmdConfigQosBufferPoolReservedBytes.cpp | 40 +++ .../CmdConfigQosBufferPoolReservedBytes.h | 45 +++ .../CmdConfigQosBufferPoolSharedBytes.cpp | 40 +++ .../CmdConfigQosBufferPoolSharedBytes.h | 77 +++++ .../config/session/CmdConfigSessionCommit.cpp | 19 +- fboss/cli/fboss2/session/ConfigSession.cpp | 224 ++++++++++++- fboss/cli/fboss2/session/ConfigSession.h | 66 +++- fboss/cli/fboss2/test/BUCK | 1 + .../test/CmdConfigQosBufferPoolTest.cpp | 314 ++++++++++++++++++ .../cli/fboss2/test/CmdConfigSessionTest.cpp | 181 +++++++++- fboss/cli/fboss2/utils/CmdUtilsCommon.h | 2 + 23 files changed, 1394 insertions(+), 31 deletions(-) create mode 100644 fboss/cli/fboss2/cli_metadata.thrift create mode 100644 fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h create mode 100644 fboss/cli/fboss2/commands/config/qos/buffer_pool/BufferPoolConfigUtils.h create mode 100644 fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h create mode 100644 fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.cpp create mode 100644 fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h create mode 100644 fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.cpp create mode 100644 fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h create mode 100644 fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.cpp create mode 100644 fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h create mode 100644 fboss/cli/fboss2/test/CmdConfigQosBufferPoolTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 72d469054b26a..ee8d8846759bd 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -3,6 +3,13 @@ # In general, libraries and binaries in fboss/foo/bar are built by # cmake/FooBar.cmake +add_fbthrift_cpp_library( + cli_metadata + fboss/cli/fboss2/cli_metadata.thrift + OPTIONS + json +) + add_fbthrift_cpp_library( cli_model fboss/cli/fboss2/cli.thrift @@ -582,6 +589,14 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.cpp fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.cpp + fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h + fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h + fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h + fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.cpp + fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h + fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.cpp + fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h + fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.cpp fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h @@ -597,6 +612,7 @@ add_library(fboss2_config_lib ) target_link_libraries(fboss2_config_lib + cli_metadata fboss2_lib agent_dir_util ) diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index 264ea0b904ade..22629f620329c 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -7,6 +7,7 @@ add_executable(fboss2_cmd_test fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp fboss/cli/fboss2/test/CmdConfigInterfaceDescriptionTest.cpp fboss/cli/fboss2/test/CmdConfigInterfaceMtuTest.cpp + fboss/cli/fboss2/test/CmdConfigQosBufferPoolTest.cpp fboss/cli/fboss2/test/CmdConfigReloadTest.cpp fboss/cli/fboss2/test/CmdConfigSessionDiffTest.cpp fboss/cli/fboss2/test/CmdConfigSessionTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 6a53b568b0abf..9b8c44a700ca9 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -15,6 +15,15 @@ thrift_library( thrift_srcs = {"cli.thrift": []}, ) +thrift_library( + name = "cli_metadata", + languages = [ + "cpp2", + ], + thrift_cpp2_options = "json", + thrift_srcs = {"cli_metadata.thrift": []}, +) + # NOTE: all of the actual command tree is managed inside CmdList.cpp # CmdList.h defines the data structure cpp_library( @@ -776,6 +785,9 @@ cpp_library( "commands/config/history/CmdConfigHistory.cpp", "commands/config/interface/CmdConfigInterfaceDescription.cpp", "commands/config/interface/CmdConfigInterfaceMtu.cpp", + "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.cpp", + "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.cpp", + "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.cpp", "commands/config/rollback/CmdConfigRollback.cpp", "commands/config/session/CmdConfigSessionCommit.cpp", "commands/config/session/CmdConfigSessionDiff.cpp", @@ -788,6 +800,11 @@ cpp_library( "commands/config/interface/CmdConfigInterface.h", "commands/config/interface/CmdConfigInterfaceDescription.h", "commands/config/interface/CmdConfigInterfaceMtu.h", + "commands/config/qos/CmdConfigQos.h", + "commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h", + "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h", + "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h", + "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h", "commands/config/rollback/CmdConfigRollback.h", "commands/config/session/CmdConfigSessionCommit.h", "commands/config/session/CmdConfigSessionDiff.h", @@ -796,6 +813,7 @@ cpp_library( exported_deps = [ "fbsource//third-party/fmt:fmt", "fbsource//third-party/re2:re2", + ":cli_metadata-cpp2-types", ":cmd-common-utils", ":cmd-handler", ":fboss2-lib", diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 133893761ad65..656ec92256473 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -16,6 +16,11 @@ #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" +#include "fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -37,5 +42,17 @@ template void CmdHandler::run(); template void CmdHandler::run(); +template void CmdHandler::run(); +template void +CmdHandler::run(); +template void CmdHandler< + CmdConfigQosBufferPoolSharedBytes, + CmdConfigQosBufferPoolSharedBytesTraits>::run(); +template void CmdHandler< + CmdConfigQosBufferPoolHeadroomBytes, + CmdConfigQosBufferPoolHeadroomBytesTraits>::run(); +template void CmdHandler< + CmdConfigQosBufferPoolReservedBytes, + CmdConfigQosBufferPoolReservedBytesTraits>::run(); } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 05e070f392c0c..b7c213c90ae62 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -17,6 +17,11 @@ #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" +#include "fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -57,6 +62,38 @@ const CommandTree& kConfigCommandTree() { }}, }, + { + "config", + "qos", + "Configure QoS settings", + commandHandler, + argTypeHandler, + {{ + "buffer-pool", + "Configure buffer pool settings", + commandHandler, + argTypeHandler, + {{ + "shared-bytes", + "Set buffer pool shared bytes", + commandHandler, + argTypeHandler, + }, + { + "headroom-bytes", + "Set buffer pool headroom bytes", + commandHandler, + argTypeHandler, + }, + { + "reserved-bytes", + "Set buffer pool reserved bytes", + commandHandler, + argTypeHandler, + }}, + }}, + }, + { "config", "session", diff --git a/fboss/cli/fboss2/CmdSubcommands.cpp b/fboss/cli/fboss2/CmdSubcommands.cpp index dd84da46788be..35a4d3078a424 100644 --- a/fboss/cli/fboss2/CmdSubcommands.cpp +++ b/fboss/cli/fboss2/CmdSubcommands.cpp @@ -229,6 +229,12 @@ CLI::App* CmdSubcommands::addCommand( subCmd->add_option( "revisions", args, "Revision(s) in the form 'rN' or 'current'"); break; + case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_POOL_NAME: + subCmd->add_option("buffer_pool_name", args, "Buffer pool name"); + break; + case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_BYTES: + subCmd->add_option("bytes", args, "Buffer size in bytes"); + break; case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_UNINITIALIZE: case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE: break; diff --git a/fboss/cli/fboss2/cli_metadata.thrift b/fboss/cli/fboss2/cli_metadata.thrift new file mode 100644 index 0000000000000..18b82b687d92e --- /dev/null +++ b/fboss/cli/fboss2/cli_metadata.thrift @@ -0,0 +1,35 @@ +/* + * 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. + * + */ + +package "facebook.com/fboss/cli" + +namespace cpp2 facebook.fboss.cli + +// Action level required for config changes to take effect. +// Used to track the highest impact action needed when committing config +// changes. +enum ConfigActionLevel { + HITLESS = 0, // Can be applied with reloadConfig() - default + AGENT_RESTART = 1, // Requires agent restart +} + +// Identifier for different agents that can be configured +enum AgentType { + WEDGE_AGENT = 1, +} + +// Metadata stored alongside the session configuration file. +// This metadata tracks state that needs to persist across CLI invocations +// within a single config session. +struct ConfigSessionMetadata { + // Maps each agent to the required action level for pending config changes. + // Agents not in this map default to HITLESS. + 1: map action; +} diff --git a/fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h b/fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h new file mode 100644 index 0000000000000..382bcf7219879 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h @@ -0,0 +1,35 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" + +namespace facebook::fboss { + +struct CmdConfigQosTraits : public WriteCommandTraits { + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE; + using ObjectArgType = std::monostate; + using RetType = std::string; +}; + +class CmdConfigQos : public CmdHandler { + public: + RetType queryClient(const HostInfo& /* hostInfo */) { + throw std::runtime_error( + "Incomplete command, please use one of the subcommands"); + } + + void printOutput(const RetType& /* model */) {} +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/qos/buffer_pool/BufferPoolConfigUtils.h b/fboss/cli/fboss2/commands/config/qos/buffer_pool/BufferPoolConfigUtils.h new file mode 100644 index 0000000000000..ed3655fd7a0b6 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/BufferPoolConfigUtils.h @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#include + +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +/** + * Helper function to set a buffer pool configuration field. + * + * This function handles the common logic for all buffer pool configuration + * commands (shared-bytes, headroom-bytes, reserved-bytes): + * - Gets or creates the buffer pool config map + * - Gets or creates the specific buffer pool entry + * - Calls the setter to update the specific field + * - Saves the config + * - Updates the required action level to AGENT_RESTART + * + * @param poolName The name of the buffer pool to configure + * @param fieldName The name of the field being set (for the success message) + * @param value The value to set + * @param setter A lambda that sets the specific field on the BufferPoolConfig. + * For new configs, it receives a reference to the new config. + * For existing configs, it receives a reference to the existing + * config. + * @return A success message string + */ +template +std::string setBufferPoolConfigField( + const std::string& poolName, + const std::string& fieldName, + int32_t value, + SetterFn setter) { + auto& session = ConfigSession::getInstance(); + auto& agentConfig = session.getAgentConfig(); + auto& switchConfig = *agentConfig.sw(); + + // Get or create the bufferPoolConfigs map + if (!switchConfig.bufferPoolConfigs()) { + switchConfig.bufferPoolConfigs() = + std::map{}; + } + + auto& bufferPoolConfigs = *switchConfig.bufferPoolConfigs(); + + // Check if the buffer pool exists + auto it = bufferPoolConfigs.find(poolName); + if (it == bufferPoolConfigs.end()) { + // Create a new buffer pool config + // Note: sharedBytes is required, so we default it to 0 + cfg::BufferPoolConfig newConfig; + newConfig.sharedBytes() = 0; + setter(newConfig); + bufferPoolConfigs[poolName] = std::move(newConfig); + } else { + // Update the existing buffer pool config + setter(it->second); + } + + // Save the updated config and update the required action level + // Buffer pool changes always require agent restart + session.saveConfig(cli::ConfigActionLevel::AGENT_RESTART); + + return fmt::format( + "Successfully set {} for buffer-pool '{}' to {}", + fieldName, + poolName, + value); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h new file mode 100644 index 0000000000000..0fbe2ed725d3f --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h @@ -0,0 +1,80 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" + +namespace facebook::fboss { + +// Custom type for buffer pool name argument +class BufferPoolName : public utils::BaseObjectArgType { + public: + /* implicit */ BufferPoolName(std::vector v) { + if (v.empty()) { + throw std::invalid_argument("Buffer pool name is required"); + } + if (v.size() != 1) { + throw std::invalid_argument( + "Expected single buffer pool name, got: " + folly::join(", ", v)); + } + const auto& name = v[0]; + // Valid pool name: starts with letter, alphanumeric + underscore/hyphen, + // 1-64 chars + static const re2::RE2 kValidPoolNamePattern( + "^[a-zA-Z][a-zA-Z0-9_-]{0,63}$"); + if (!re2::RE2::FullMatch(name, kValidPoolNamePattern)) { + throw std::invalid_argument( + "Invalid buffer pool name: '" + name + + "'. Name must start with a letter, contain only alphanumeric " + "characters, underscores, or hyphens, and be 1-64 characters long."); + } + data_.push_back(name); + } + + const std::string& getName() const { + return data_[0]; + } + + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_POOL_NAME; +}; + +struct CmdConfigQosBufferPoolTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigQos; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_POOL_NAME; + using ObjectArgType = BufferPoolName; + using RetType = std::string; +}; + +class CmdConfigQosBufferPool + : public CmdHandler { + public: + using ObjectArgType = CmdConfigQosBufferPoolTraits::ObjectArgType; + using RetType = CmdConfigQosBufferPoolTraits::RetType; + + RetType queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& /* bufferPoolName */) { + throw std::runtime_error( + "Incomplete command, please use one of the subcommands: " + "shared-bytes, headroom-bytes, reserved-bytes"); + } + + void printOutput(const RetType& /* model */) {} +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.cpp b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.cpp new file mode 100644 index 0000000000000..1f9ec074a8e5a --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.cpp @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h" + +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/BufferPoolConfigUtils.h" + +namespace facebook::fboss { + +CmdConfigQosBufferPoolHeadroomBytesTraits::RetType +CmdConfigQosBufferPoolHeadroomBytes::queryClient( + const HostInfo& /* hostInfo */, + const BufferPoolName& bufferPoolName, + const CmdConfigQosBufferPoolHeadroomBytesTraits::ObjectArgType& + headroomBytesValue) { + const std::string& poolName = bufferPoolName.getName(); + int32_t headroomBytes = headroomBytesValue.getValue(); + + return setBufferPoolConfigField( + poolName, + "headroom-bytes", + headroomBytes, + [headroomBytes](cfg::BufferPoolConfig& config) { + config.headroomBytes() = headroomBytes; + }); +} + +void CmdConfigQosBufferPoolHeadroomBytes::printOutput( + const CmdConfigQosBufferPoolHeadroomBytesTraits::RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h new file mode 100644 index 0000000000000..1a73017d8a228 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" + +namespace facebook::fboss { + +struct CmdConfigQosBufferPoolHeadroomBytesTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigQosBufferPool; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_BYTES; + using ObjectArgType = BufferBytesValue; + using RetType = std::string; +}; + +class CmdConfigQosBufferPoolHeadroomBytes + : public CmdHandler< + CmdConfigQosBufferPoolHeadroomBytes, + CmdConfigQosBufferPoolHeadroomBytesTraits> { + public: + using ObjectArgType = + CmdConfigQosBufferPoolHeadroomBytesTraits::ObjectArgType; + using RetType = CmdConfigQosBufferPoolHeadroomBytesTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BufferPoolName& bufferPoolName, + const ObjectArgType& headroomBytesValue); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.cpp b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.cpp new file mode 100644 index 0000000000000..c3257d99ff878 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.cpp @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h" + +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/BufferPoolConfigUtils.h" + +namespace facebook::fboss { + +CmdConfigQosBufferPoolReservedBytesTraits::RetType +CmdConfigQosBufferPoolReservedBytes::queryClient( + const HostInfo& /* hostInfo */, + const BufferPoolName& bufferPoolName, + const CmdConfigQosBufferPoolReservedBytesTraits::ObjectArgType& + reservedBytesValue) { + const std::string& poolName = bufferPoolName.getName(); + int32_t reservedBytes = reservedBytesValue.getValue(); + + return setBufferPoolConfigField( + poolName, + "reserved-bytes", + reservedBytes, + [reservedBytes](cfg::BufferPoolConfig& config) { + config.reservedBytes() = reservedBytes; + }); +} + +void CmdConfigQosBufferPoolReservedBytes::printOutput( + const CmdConfigQosBufferPoolReservedBytesTraits::RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h new file mode 100644 index 0000000000000..a6ee985ca5b0f --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" + +namespace facebook::fboss { + +struct CmdConfigQosBufferPoolReservedBytesTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigQosBufferPool; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_BYTES; + using ObjectArgType = BufferBytesValue; + using RetType = std::string; +}; + +class CmdConfigQosBufferPoolReservedBytes + : public CmdHandler< + CmdConfigQosBufferPoolReservedBytes, + CmdConfigQosBufferPoolReservedBytesTraits> { + public: + using ObjectArgType = + CmdConfigQosBufferPoolReservedBytesTraits::ObjectArgType; + using RetType = CmdConfigQosBufferPoolReservedBytesTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BufferPoolName& bufferPoolName, + const ObjectArgType& reservedBytesValue); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.cpp b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.cpp new file mode 100644 index 0000000000000..b7af1527045a9 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.cpp @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h" + +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/BufferPoolConfigUtils.h" + +namespace facebook::fboss { + +CmdConfigQosBufferPoolSharedBytesTraits::RetType +CmdConfigQosBufferPoolSharedBytes::queryClient( + const HostInfo& /* hostInfo */, + const BufferPoolName& bufferPoolName, + const CmdConfigQosBufferPoolSharedBytesTraits::ObjectArgType& + sharedBytesValue) { + const std::string& poolName = bufferPoolName.getName(); + int32_t sharedBytes = sharedBytesValue.getValue(); + + return setBufferPoolConfigField( + poolName, + "shared-bytes", + sharedBytes, + [sharedBytes](cfg::BufferPoolConfig& config) { + config.sharedBytes() = sharedBytes; + }); +} + +void CmdConfigQosBufferPoolSharedBytes::printOutput( + const CmdConfigQosBufferPoolSharedBytesTraits::RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h new file mode 100644 index 0000000000000..caa1a92bab520 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h @@ -0,0 +1,77 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" + +namespace facebook::fboss { + +// Custom type for buffer bytes argument with validation +class BufferBytesValue : public utils::BaseObjectArgType { + public: + /* implicit */ BufferBytesValue(std::vector v) { + if (v.empty()) { + throw std::invalid_argument("Buffer bytes value is required"); + } + if (v.size() != 1) { + throw std::invalid_argument( + "Expected single buffer bytes value, got: " + folly::join(", ", v)); + } + + try { + int32_t bytes = folly::to(v[0]); + if (bytes < 0) { + throw std::invalid_argument( + "Buffer bytes must be non-negative, got: " + std::to_string(bytes)); + } + data_.push_back(bytes); + } catch (const folly::ConversionError& e) { + throw std::invalid_argument("Invalid buffer bytes value: " + v[0]); + } + } + + int32_t getValue() const { + return data_[0]; + } + + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_BYTES; +}; + +struct CmdConfigQosBufferPoolSharedBytesTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigQosBufferPool; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_BYTES; + using ObjectArgType = BufferBytesValue; + using RetType = std::string; +}; + +class CmdConfigQosBufferPoolSharedBytes + : public CmdHandler< + CmdConfigQosBufferPoolSharedBytes, + CmdConfigQosBufferPoolSharedBytesTraits> { + public: + using ObjectArgType = CmdConfigQosBufferPoolSharedBytesTraits::ObjectArgType; + using RetType = CmdConfigQosBufferPoolSharedBytesTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BufferPoolName& bufferPoolName, + const ObjectArgType& sharedBytesValue); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp index eef1f9e4cae7e..9c40594da2320 100644 --- a/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp +++ b/fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.cpp @@ -9,6 +9,8 @@ */ #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" + +#include #include "fboss/cli/fboss2/session/ConfigSession.h" namespace facebook::fboss { @@ -21,9 +23,20 @@ CmdConfigSessionCommitTraits::RetType CmdConfigSessionCommit::queryClient( return "No config session exists. Make a config change first."; } - int revision = session.commit(hostInfo); - return "Config session committed successfully as r" + - std::to_string(revision) + " and config reloaded."; + auto result = session.commit(hostInfo); + + std::string message; + if (result.actionLevel == cli::ConfigActionLevel::AGENT_RESTART) { + message = fmt::format( + "Config session committed successfully as r{} and wedge_agent restarted.", + result.revision); + } else { + message = fmt::format( + "Config session committed successfully as r{} and config reloaded.", + result.revision); + } + + return message; } void CmdConfigSessionCommit::printOutput(const RetType& logMsg) { diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index a9a4d959fb124..1bb720f1cf4f6 100644 --- a/fboss/cli/fboss2/session/ConfigSession.cpp +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -16,19 +16,24 @@ #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/PortMap.h" @@ -281,7 +286,9 @@ const utils::PortMap& ConfigSession::getPortMap() const { return *portMap_; } -void ConfigSession::saveConfig() { +void ConfigSession::saveConfig( + std::optional actionLevel, + cli::AgentType agent) { if (!configLoaded_) { throw std::runtime_error("No config loaded to save"); } @@ -303,6 +310,11 @@ void ConfigSession::saveConfig() { // seeing partial/corrupted data. folly::writeFileAtomic( sessionConfigPath_, prettyJson, 0644, folly::SyncType::WITH_SYNC); + + // If an action level was provided, update the required action metadata + if (actionLevel.has_value()) { + updateRequiredAction(*actionLevel, agent); + } } int ConfigSession::extractRevisionNumber(const std::string& filenameOrPath) { @@ -324,6 +336,158 @@ int ConfigSession::extractRevisionNumber(const std::string& filenameOrPath) { return -1; } +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(); +} + +std::string ConfigSession::getServiceName(cli::AgentType agent) { + switch (agent) { + case cli::AgentType::WEDGE_AGENT: + return "wedge_agent"; + } + throw std::runtime_error("Unknown agent type"); +} + +void ConfigSession::loadActionLevel() { + std::string metadataPath = getMetadataPath(); + // Note: We don't initialize requiredActions_ here since getRequiredAction() + // returns HITLESS by default for agents not in the map, and + // updateRequiredAction() handles adding new agents. + + if (!fs::exists(metadataPath)) { + return; + } + + std::string content; + if (!folly::readFile(metadataPath.c_str(), content)) { + // If we can't read the file, keep defaults + return; + } + + // Parse JSON with symbolic enum names using fbthrift's folly_dynamic API + // LENIENT adherence allows parsing both string names and integer values + try { + folly::dynamic json = folly::parseJson(content); + cli::ConfigSessionMetadata metadata; + facebook::thrift::from_dynamic( + metadata, + json, + facebook::thrift::dynamic_format::PORTABLE, + facebook::thrift::format_adherence::LENIENT); + requiredActions_ = *metadata.action(); + } catch (const std::exception& ex) { + // If JSON parsing fails, keep defaults + LOG(WARNING) << "Failed to parse metadata file: " << ex.what(); + } +} + +void ConfigSession::saveActionLevel() { + std::string metadataPath = getMetadataPath(); + + // Build Thrift metadata struct and serialize to JSON with symbolic enum names + // Using PORTABLE format for human-readable enum names instead of integers + cli::ConfigSessionMetadata metadata; + metadata.action() = requiredActions_; + + folly::dynamic json = facebook::thrift::to_dynamic( + metadata, facebook::thrift::dynamic_format::PORTABLE); + std::string prettyJson = folly::toPrettyJson(json); + folly::writeFileAtomic( + metadataPath, prettyJson, 0644, folly::SyncType::WITH_SYNC); +} + +void ConfigSession::updateRequiredAction( + cli::ConfigActionLevel actionLevel, + cli::AgentType agent) { + // Initialize to HITLESS if not present + if (requiredActions_.find(agent) == requiredActions_.end()) { + requiredActions_[agent] = cli::ConfigActionLevel::HITLESS; + } + + // Only update if the new action level is higher (more impactful) + if (static_cast(actionLevel) > + static_cast(requiredActions_[agent])) { + requiredActions_[agent] = actionLevel; + saveActionLevel(); + } +} + +cli::ConfigActionLevel ConfigSession::getRequiredAction( + cli::AgentType agent) const { + auto it = requiredActions_.find(agent); + if (it != requiredActions_.end()) { + return it->second; + } + return cli::ConfigActionLevel::HITLESS; +} + +void ConfigSession::resetRequiredAction(cli::AgentType agent) { + requiredActions_[agent] = cli::ConfigActionLevel::HITLESS; + + // If all agents are HITLESS, remove the file entirely + bool allHitless = true; + for (const auto& [a, level] : requiredActions_) { + if (level != cli::ConfigActionLevel::HITLESS) { + allHitless = false; + break; + } + } + if (allHitless) { + std::string metadataPath = getMetadataPath(); + std::error_code ec; + fs::remove(metadataPath, ec); + // Ignore errors - file might not exist + } else { + // Only save if there are remaining agents with non-HITLESS levels + saveActionLevel(); + } +} + +void ConfigSession::restartAgent(cli::AgentType agent) { + std::string serviceName = getServiceName(agent); + + LOG(INFO) << "Restarting " << serviceName << " via systemd..."; + + // Use systemctl to restart the service + // Using folly::Subprocess with explicit args avoids shell injection + try { + folly::Subprocess restartProc( + {"/usr/bin/systemctl", "restart", serviceName}); + restartProc.waitChecked(); + } catch (const std::exception& ex) { + throw std::runtime_error( + fmt::format("Failed to restart {}: {}", serviceName, ex.what())); + } + + // Wait for the service to be active (up to 60 seconds) + constexpr int maxWaitSeconds = 60; + constexpr int pollIntervalMs = 500; + int waitedMs = 0; + + while (waitedMs < maxWaitSeconds * 1000) { + try { + folly::Subprocess checkProc( + {"/usr/bin/systemctl", "is-active", "--quiet", serviceName}); + checkProc.waitChecked(); + // If waitChecked() doesn't throw, the service is active + LOG(INFO) << serviceName << " is now active"; + return; + } catch (const folly::CalledProcessError&) { + // Service not active yet, keep waiting + } + std::this_thread::sleep_for(std::chrono::milliseconds(pollIntervalMs)); + waitedMs += pollIntervalMs; + } + + throw std::runtime_error( + fmt::format( + "{} did not become active within {} seconds", + serviceName, + maxWaitSeconds)); +} + void ConfigSession::loadConfig() { std::string configJson; if (!folly::readFile(sessionConfigPath_.c_str(), configJson)) { @@ -350,6 +514,8 @@ void ConfigSession::initializeSession() { ensureDirectoryExists(sessionPath.parent_path().string()); copySystemConfigToSession(); } + // Load the action level from disk (survives across CLI invocations) + loadActionLevel(); } void ConfigSession::copySystemConfigToSession() { @@ -388,7 +554,7 @@ void ConfigSession::copySystemConfigToSession() { sessionConfigPath_, configData, 0644, folly::SyncType::WITH_SYNC); } -int ConfigSession::commit(const HostInfo& hostInfo) { +ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { if (!sessionExists()) { throw std::runtime_error( "No config session exists. Make a config change first."); @@ -437,28 +603,46 @@ int ConfigSession::commit(const HostInfo& hostInfo) { // Atomically update the symlink to point to the new config atomicSymlinkUpdate(systemConfigPath_, targetConfigPath); - // Reload the config - if this fails, rollback the symlink atomically + // Check the required action level for this commit + auto actionLevel = getRequiredAction(cli::AgentType::WEDGE_AGENT); + + // Apply the config based on the required action level try { - auto client = - utils::createClient>( - hostInfo); - client->sync_reloadConfig(); - LOG(INFO) << "Config committed as revision r" << revision; + if (actionLevel == cli::ConfigActionLevel::AGENT_RESTART) { + // For AGENT_RESTART changes, restart the agent via systemd + // This will cause the agent to pick up the new config on startup + restartAgent(cli::AgentType::WEDGE_AGENT); + LOG(INFO) << "Config committed as revision r" << revision + << " (agent restarted)"; + } else { + // For HITLESS changes, use reloadConfig() which applies without restart + auto client = utils::createClient< + apache::thrift::Client>(hostInfo); + client->sync_reloadConfig(); + LOG(INFO) << "Config committed as revision r" << revision + << " (config reloaded)"; + } } catch (const std::exception& ex) { // Rollback: atomically restore the old symlink try { atomicSymlinkUpdate(systemConfigPath_, oldSymlinkTarget); + // If this was an AGENT_RESTART change, we need to restart the agent again + // so it picks up the old config (in case the restart was partially + // successful before failing) + if (actionLevel == cli::ConfigActionLevel::AGENT_RESTART) { + restartAgent(cli::AgentType::WEDGE_AGENT); + } } catch (const std::exception& rollbackEx) { // If rollback also fails, include both errors in the message throw std::runtime_error( fmt::format( - "Failed to reload config: {}. Additionally, failed to rollback the config: {}", + "Failed to apply config: {}. Additionally, failed to rollback the config: {}", ex.what(), rollbackEx.what())); } throw std::runtime_error( fmt::format( - "Failed to reload config, config was rolled back automatically: {}", + "Failed to apply config, config was rolled back automatically: {}", ex.what())); } @@ -472,7 +656,10 @@ int ConfigSession::commit(const HostInfo& hostInfo) { ec.message()); } - return revision; + // Reset action level after successful commit + resetRequiredAction(cli::AgentType::WEDGE_AGENT); + + return CommitResult{revision, actionLevel}; } int ConfigSession::rollback(const HostInfo& hostInfo) { @@ -498,12 +685,14 @@ int ConfigSession::rollback( ensureDirectoryExists(cliConfigDir_); // Build the path to the target revision - std::string targetConfigPath = cliConfigDir_ + "/agent-" + revision + ".conf"; + std::string targetConfigPath = + fmt::format("{}/agent-{}.conf", cliConfigDir_, revision); // Check if the target revision exists if (!fs::exists(targetConfigPath)) { throw std::runtime_error( - "Revision " + revision + " does not exist at " + targetConfigPath); + fmt::format( + "Revision {} does not exist at {}", revision, targetConfigPath)); } std::error_code ec; @@ -511,14 +700,17 @@ int ConfigSession::rollback( // Verify that the system config is a symlink if (!fs::is_symlink(systemConfigPath_)) { throw std::runtime_error( - systemConfigPath_ + " is not a symlink. Expected it to be a symlink."); + fmt::format( + "{} is not a symlink. Expected it to be a symlink.", + systemConfigPath_)); } // 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( - "Failed to read symlink " + systemConfigPath_ + ": " + ec.message()); + fmt::format( + "Failed to read symlink {}: {}", systemConfigPath_, ec.message())); } // First, create a new revision with the same content as the target revision diff --git a/fboss/cli/fboss2/session/ConfigSession.h b/fboss/cli/fboss2/session/ConfigSession.h index 7838262eaae35..4e67e769366a0 100644 --- a/fboss/cli/fboss2/session/ConfigSession.h +++ b/fboss/cli/fboss2/session/ConfigSession.h @@ -9,10 +9,13 @@ #pragma once +#include #include +#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/utils/HostInfo.h" namespace facebook::fboss::utils { @@ -96,11 +99,21 @@ class ConfigSession { // Get the path to the CLI config directory (/etc/coop/cli) std::string getCliConfigDir() const; + // Result of a commit operation + struct CommitResult { + int revision; // The revision number that was committed + cli::ConfigActionLevel actionLevel; // The action level that was required + // Note: configReloaded can be inferred from actionLevel: + // - HITLESS: config was reloaded via reloadConfig() + // - AGENT_RESTART: agent was restarted via systemd + }; + // Atomically commit the session to /etc/coop/cli/agent-rN.conf, - // update the symlink /etc/coop/agent.conf to point to it, and reload config. - // Returns the revision number that was committed if the commit was - // successful. - int commit(const HostInfo& hostInfo); + // 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. + CommitResult commit(const HostInfo& hostInfo); // Rollback to a specific revision or to the previous revision // Returns the revision that was rolled back to @@ -118,13 +131,36 @@ class ConfigSession { utils::PortMap& getPortMap(); const utils::PortMap& getPortMap() const; - // Save the configuration back to the session file - void saveConfig(); + // Save the configuration back to the session file. + // If actionLevel is provided, also updates the required action level + // for the specified agent (if the new level is higher than the current one). + // This combines saving the config and updating its associated metadata. + void saveConfig( + std::optional actionLevel = std::nullopt, + cli::AgentType agent = cli::AgentType::WEDGE_AGENT); // 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); + // Update the required action level for the current session. + // Tracks the highest action level across all config commands. + // Higher action levels take precedence (AGENT_RESTART > HITLESS). + // The agent parameter specifies which agent this action level applies to. + // Currently only WEDGE_AGENT is supported; future agents will be added. + void updateRequiredAction( + cli::ConfigActionLevel actionLevel, + cli::AgentType agent = cli::AgentType::WEDGE_AGENT); + + // Get the current required action level for the session + // The agent parameter specifies which agent to get the action level for. + cli::ConfigActionLevel getRequiredAction( + cli::AgentType agent = cli::AgentType::WEDGE_AGENT) const; + + // Reset the required action level to HITLESS (called after successful commit) + // The agent parameter specifies which agent to reset the action level for. + void resetRequiredAction(cli::AgentType agent = cli::AgentType::WEDGE_AGENT); + protected: // Constructor for testing with custom paths ConfigSession( @@ -146,6 +182,24 @@ class ConfigSession { std::unique_ptr portMap_; bool configLoaded_ = false; + // Track the highest action level required for pending config changes per + // agent. Persisted to disk so it survives across CLI invocations within a + // session. + std::map requiredActions_; + + // Path to the metadata file (e.g., ~/.fboss2/metadata) + std::string getMetadataPath() const; + + // Load/save action levels from/to disk + void loadActionLevel(); + void saveActionLevel(); + + // Restart an agent via systemd and wait for it to be active + void restartAgent(cli::AgentType agent); + + // Get the systemd service name for an agent + static std::string getServiceName(cli::AgentType agent); + // Initialize the session (creates session config file if it doesn't exist) void initializeSession(); void copySystemConfigToSession(); diff --git a/fboss/cli/fboss2/test/BUCK b/fboss/cli/fboss2/test/BUCK index c4ef46f504bc1..41ef684bbb1ec 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -59,6 +59,7 @@ cpp_unittest( "CmdConfigHistoryTest.cpp", "CmdConfigInterfaceDescriptionTest.cpp", "CmdConfigInterfaceMtuTest.cpp", + "CmdConfigQosBufferPoolTest.cpp", "CmdConfigReloadTest.cpp", "CmdConfigSessionDiffTest.cpp", "CmdConfigSessionTest.cpp", diff --git a/fboss/cli/fboss2/test/CmdConfigQosBufferPoolTest.cpp b/fboss/cli/fboss2/test/CmdConfigQosBufferPoolTest.cpp new file mode 100644 index 0000000000000..4d57caa3dba92 --- /dev/null +++ b/fboss/cli/fboss2/test/CmdConfigQosBufferPoolTest.cpp @@ -0,0 +1,314 @@ +/* + * 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 "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.h" +#include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.h" +#include "fboss/cli/fboss2/test/CmdHandlerTestBase.h" +#include "fboss/cli/fboss2/test/TestableConfigSession.h" +#include "fboss/cli/fboss2/utils/PortMap.h" + +namespace fs = std::filesystem; + +namespace facebook::fboss { + +class CmdConfigQosBufferPoolTestFixture : public CmdHandlerTestBase { + public: + void SetUp() override { + CmdHandlerTestBase::SetUp(); + + // Create unique test directories + auto tempBase = fs::temp_directory_path(); + auto uniquePath = + boost::filesystem::unique_path("fboss_bp_test_%%%%-%%%%-%%%%-%%%%"); + testHomeDir_ = tempBase / (uniquePath.string() + "_home"); + testEtcDir_ = tempBase / (uniquePath.string() + "_etc"); + + std::error_code ec; + if (fs::exists(testHomeDir_)) { + fs::remove_all(testHomeDir_, ec); + } + if (fs::exists(testEtcDir_)) { + fs::remove_all(testEtcDir_, ec); + } + + // Create test directories + fs::create_directories(testHomeDir_); + fs::create_directories(testEtcDir_ / "coop"); + fs::create_directories(testEtcDir_ / "coop" / "cli"); + + // Set environment variables + setenv("HOME", testHomeDir_.c_str(), 1); + setenv("USER", "testuser", 1); + + // Create a test system config file + fs::path initialRevision = testEtcDir_ / "coop" / "cli" / "agent-r1.conf"; + createTestConfig(initialRevision, R"({ + "sw": { + "ports": [ + { + "logicalID": 1, + "name": "eth1/1/1", + "state": 2, + "speed": 100000 + } + ] + } +})"); + + // Create symlink + systemConfigPath_ = testEtcDir_ / "coop" / "agent.conf"; + fs::create_symlink(initialRevision, systemConfigPath_); + + // Create session config path + sessionConfigPath_ = testHomeDir_ / ".fboss2" / "agent.conf"; + cliConfigDir_ = testEtcDir_ / "coop" / "cli"; + } + + void TearDown() override { + // Reset the singleton to ensure tests don't interfere with each other + TestableConfigSession::setInstance(nullptr); + + std::error_code ec; + if (fs::exists(testHomeDir_)) { + fs::remove_all(testHomeDir_, ec); + } + if (fs::exists(testEtcDir_)) { + fs::remove_all(testEtcDir_, ec); + } + CmdHandlerTestBase::TearDown(); + } + + protected: + void createTestConfig(const fs::path& path, const std::string& content) { + std::ofstream file(path); + file << content; + file.close(); + } + + std::string readFile(const fs::path& path) { + std::ifstream file(path); + std::stringstream buffer; + buffer << file.rdbuf(); + return buffer.str(); + } + + fs::path testHomeDir_; + fs::path testEtcDir_; + fs::path systemConfigPath_; + fs::path sessionConfigPath_; + fs::path cliConfigDir_; +}; + +// Test BufferPoolName argument validation +TEST_F(CmdConfigQosBufferPoolTestFixture, bufferPoolNameValidation) { + // Valid names - alphanumeric with underscores and hyphens, starting with + // letter + EXPECT_NO_THROW(BufferPoolName({"ingress_pool"})); + EXPECT_NO_THROW(BufferPoolName({"egress-lossy-pool"})); + EXPECT_NO_THROW(BufferPoolName({"Pool1"})); + EXPECT_NO_THROW(BufferPoolName({"a"})); // single character + EXPECT_NO_THROW(BufferPoolName({"default"})); + + // Empty name should throw + EXPECT_THROW(BufferPoolName({}), std::invalid_argument); + + // Multiple names should throw + EXPECT_THROW(BufferPoolName({"pool1", "pool2"}), std::invalid_argument); + + // Invalid names - must start with letter + EXPECT_THROW(BufferPoolName({"123pool"}), std::invalid_argument); + EXPECT_THROW(BufferPoolName({"_pool"}), std::invalid_argument); + EXPECT_THROW(BufferPoolName({"-pool"}), std::invalid_argument); + + // Invalid names - no spaces or special characters + EXPECT_THROW(BufferPoolName({"pool name"}), std::invalid_argument); + EXPECT_THROW(BufferPoolName({"pool.name"}), std::invalid_argument); + EXPECT_THROW(BufferPoolName({"pool@name"}), std::invalid_argument); + + // Invalid names - empty string + EXPECT_THROW(BufferPoolName({""}), std::invalid_argument); +} + +// Test BufferBytesValue argument validation +TEST_F(CmdConfigQosBufferPoolTestFixture, bufferBytesValueValidation) { + // Valid positive value + BufferBytesValue validValue({"1000"}); + EXPECT_EQ(validValue.getValue(), 1000); + + // Valid zero value + BufferBytesValue zeroValue({"0"}); + EXPECT_EQ(zeroValue.getValue(), 0); + + // Empty value should throw + EXPECT_THROW(BufferBytesValue({}), std::invalid_argument); + + // Negative value should throw + EXPECT_THROW(BufferBytesValue({"-100"}), std::invalid_argument); + + // Non-numeric value should throw + EXPECT_THROW(BufferBytesValue({"abc"}), std::invalid_argument); + + // Multiple values should throw + EXPECT_THROW(BufferBytesValue({"100", "200"}), std::invalid_argument); +} + +// Test shared-bytes command creates buffer pool config +TEST_F(CmdConfigQosBufferPoolTestFixture, sharedBytesCreatesBufferPool) { + TestableConfigSession::setInstance( + std::make_unique( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string())); + + auto cmd = CmdConfigQosBufferPoolSharedBytes(); + BufferPoolName poolName({"test_pool"}); + BufferBytesValue sharedBytes({"50000"}); + + auto result = cmd.queryClient(localhost(), poolName, sharedBytes); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully set shared-bytes")); + EXPECT_THAT(result, ::testing::HasSubstr("test_pool")); + EXPECT_THAT(result, ::testing::HasSubstr("50000")); + + // Verify the config was actually modified + auto& config = ConfigSession::getInstance().getAgentConfig(); + auto& switchConfig = *config.sw(); + ASSERT_TRUE(switchConfig.bufferPoolConfigs().has_value()); + + auto& bufferPoolConfigs = *switchConfig.bufferPoolConfigs(); + auto it = bufferPoolConfigs.find("test_pool"); + ASSERT_NE(it, bufferPoolConfigs.end()); + EXPECT_EQ(*it->second.sharedBytes(), 50000); +} + +// Test headroom-bytes command creates buffer pool config +TEST_F(CmdConfigQosBufferPoolTestFixture, headroomBytesCreatesBufferPool) { + TestableConfigSession::setInstance( + std::make_unique( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string())); + + auto cmd = CmdConfigQosBufferPoolHeadroomBytes(); + BufferPoolName poolName({"headroom_pool"}); + BufferBytesValue headroomBytes({"10000"}); + + auto result = cmd.queryClient(localhost(), poolName, headroomBytes); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully set headroom-bytes")); + EXPECT_THAT(result, ::testing::HasSubstr("headroom_pool")); + EXPECT_THAT(result, ::testing::HasSubstr("10000")); + + // Verify the config was actually modified + auto& config = ConfigSession::getInstance().getAgentConfig(); + auto& switchConfig = *config.sw(); + ASSERT_TRUE(switchConfig.bufferPoolConfigs().has_value()); + + auto& bufferPoolConfigs = *switchConfig.bufferPoolConfigs(); + auto it = bufferPoolConfigs.find("headroom_pool"); + ASSERT_NE(it, bufferPoolConfigs.end()); + EXPECT_EQ(*it->second.sharedBytes(), 0); // Default value + ASSERT_TRUE(it->second.headroomBytes().has_value()); + EXPECT_EQ(*it->second.headroomBytes(), 10000); +} + +// Test reserved-bytes command creates buffer pool config +TEST_F(CmdConfigQosBufferPoolTestFixture, reservedBytesCreatesBufferPool) { + TestableConfigSession::setInstance( + std::make_unique( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string())); + + auto cmd = CmdConfigQosBufferPoolReservedBytes(); + BufferPoolName poolName({"reserved_pool"}); + BufferBytesValue reservedBytes({"20000"}); + + auto result = cmd.queryClient(localhost(), poolName, reservedBytes); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully set reserved-bytes")); + EXPECT_THAT(result, ::testing::HasSubstr("reserved_pool")); + EXPECT_THAT(result, ::testing::HasSubstr("20000")); + + // Verify the config was actually modified + auto& config = ConfigSession::getInstance().getAgentConfig(); + auto& switchConfig = *config.sw(); + ASSERT_TRUE(switchConfig.bufferPoolConfigs().has_value()); + + auto& bufferPoolConfigs = *switchConfig.bufferPoolConfigs(); + auto it = bufferPoolConfigs.find("reserved_pool"); + ASSERT_NE(it, bufferPoolConfigs.end()); + EXPECT_EQ(*it->second.sharedBytes(), 0); // Default value + ASSERT_TRUE(it->second.reservedBytes().has_value()); + EXPECT_EQ(*it->second.reservedBytes(), 20000); +} + +// Test updating an existing buffer pool +TEST_F(CmdConfigQosBufferPoolTestFixture, updateExistingBufferPool) { + TestableConfigSession::setInstance( + std::make_unique( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string())); + + // First, create a buffer pool with shared-bytes + auto sharedCmd = CmdConfigQosBufferPoolSharedBytes(); + BufferPoolName poolName({"existing_pool"}); + BufferBytesValue sharedBytes({"30000"}); + sharedCmd.queryClient(localhost(), poolName, sharedBytes); + + // Then, add headroom-bytes to the same pool + auto headroomCmd = CmdConfigQosBufferPoolHeadroomBytes(); + BufferBytesValue headroomBytes({"5000"}); + headroomCmd.queryClient(localhost(), poolName, headroomBytes); + + // Finally, add reserved-bytes to the same pool + auto reservedCmd = CmdConfigQosBufferPoolReservedBytes(); + BufferBytesValue reservedBytes({"2000"}); + reservedCmd.queryClient(localhost(), poolName, reservedBytes); + + // Verify all values are set correctly + auto& config = ConfigSession::getInstance().getAgentConfig(); + auto& switchConfig = *config.sw(); + ASSERT_TRUE(switchConfig.bufferPoolConfigs().has_value()); + + auto& bufferPoolConfigs = *switchConfig.bufferPoolConfigs(); + auto it = bufferPoolConfigs.find("existing_pool"); + ASSERT_NE(it, bufferPoolConfigs.end()); + EXPECT_EQ(*it->second.sharedBytes(), 30000); + ASSERT_TRUE(it->second.headroomBytes().has_value()); + EXPECT_EQ(*it->second.headroomBytes(), 5000); + ASSERT_TRUE(it->second.reservedBytes().has_value()); + EXPECT_EQ(*it->second.reservedBytes(), 2000); +} + +// Test printOutput for shared-bytes command +TEST_F(CmdConfigQosBufferPoolTestFixture, printOutputSharedBytes) { + auto cmd = CmdConfigQosBufferPoolSharedBytes(); + std::string successMessage = + "Successfully set shared-bytes for buffer-pool 'my_pool' to 50000"; + + std::stringstream buffer; + std::streambuf* old = std::cout.rdbuf(buffer.rdbuf()); + cmd.printOutput(successMessage); + std::cout.rdbuf(old); + + EXPECT_EQ(buffer.str(), successMessage + "\n"); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp b/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp index 1dfb390be1ef0..b04b7decb27e0 100644 --- a/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp +++ b/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp @@ -207,13 +207,13 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { session.saveConfig(); // Commit the session - int revision = session.commit(localhost()); + auto result = session.commit(localhost()); // Verify session config no longer exists (removed after commit) EXPECT_FALSE(fs::exists(sessionConfig)); // Verify new revision was created in cli directory - EXPECT_EQ(revision, 2); + 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")); @@ -241,10 +241,10 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { session.saveConfig(); // Commit the second change - int revision = session.commit(localhost()); + auto result = session.commit(localhost()); // Verify new revision was created - EXPECT_EQ(revision, 3); + 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")); @@ -402,7 +402,7 @@ TEST_F(ConfigSessionTestFixture, atomicRevisionCreation) { ports[0].description() = description; session.saveConfig(); - rev = session.commit(localhost()); + rev = session.commit(localhost()).revision; }; std::thread thread1( @@ -475,7 +475,7 @@ TEST_F(ConfigSessionTestFixture, concurrentSessionCreationSameUser) { ports[0].description() = description; session.saveConfig(); - rev = session.commit(localhost()); + rev = session.commit(localhost()).revision; }; std::thread thread1(commitTask, "First commit", std::ref(revision1)); @@ -637,4 +637,173 @@ TEST_F(ConfigSessionTestFixture, rollbackToPreviousRevision) { EXPECT_TRUE(fs::exists(cliConfigDir / "agent-r3.conf")); } +TEST_F(ConfigSessionTestFixture, actionLevelDefaultIsHitless) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // Create a ConfigSession + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Default action level should be HITLESS + EXPECT_EQ( + session.getRequiredAction(cli::AgentType::WEDGE_AGENT), + cli::ConfigActionLevel::HITLESS); +} + +TEST_F(ConfigSessionTestFixture, actionLevelUpdateAndGet) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // Create a ConfigSession + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Update to AGENT_RESTART + session.updateRequiredAction( + cli::ConfigActionLevel::AGENT_RESTART, cli::AgentType::WEDGE_AGENT); + + // Verify the action level was updated + EXPECT_EQ( + session.getRequiredAction(cli::AgentType::WEDGE_AGENT), + cli::ConfigActionLevel::AGENT_RESTART); +} + +TEST_F(ConfigSessionTestFixture, actionLevelHigherTakesPrecedence) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // Create a ConfigSession + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Update to AGENT_RESTART first + session.updateRequiredAction( + cli::ConfigActionLevel::AGENT_RESTART, cli::AgentType::WEDGE_AGENT); + + // Try to "downgrade" to HITLESS - should be ignored + session.updateRequiredAction( + cli::ConfigActionLevel::HITLESS, cli::AgentType::WEDGE_AGENT); + + // Verify action level remains at AGENT_RESTART + EXPECT_EQ( + session.getRequiredAction(cli::AgentType::WEDGE_AGENT), + cli::ConfigActionLevel::AGENT_RESTART); +} + +TEST_F(ConfigSessionTestFixture, actionLevelReset) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // Create a ConfigSession + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Set to AGENT_RESTART + session.updateRequiredAction( + cli::ConfigActionLevel::AGENT_RESTART, cli::AgentType::WEDGE_AGENT); + + // Reset the action level + session.resetRequiredAction(cli::AgentType::WEDGE_AGENT); + + // Verify action level was reset to HITLESS + EXPECT_EQ( + session.getRequiredAction(cli::AgentType::WEDGE_AGENT), + cli::ConfigActionLevel::HITLESS); +} + +TEST_F(ConfigSessionTestFixture, actionLevelPersistsToMetadataFile) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path metadataFile = sessionDir / "conf_metadata.json"; + + // Create a ConfigSession and set action level + { + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Set to AGENT_RESTART + session.updateRequiredAction( + cli::ConfigActionLevel::AGENT_RESTART, cli::AgentType::WEDGE_AGENT); + } + + // Verify metadata file exists and has correct JSON format + EXPECT_TRUE(fs::exists(metadataFile)); + std::string content = readFile(metadataFile); + + // Parse the JSON and verify structure - uses symbolic enum names + folly::dynamic json = folly::parseJson(content); + EXPECT_TRUE(json.isObject()); + EXPECT_TRUE(json.count("action")); + EXPECT_TRUE(json["action"].isObject()); + EXPECT_TRUE(json["action"].count("WEDGE_AGENT")); + EXPECT_EQ(json["action"]["WEDGE_AGENT"].asString(), "AGENT_RESTART"); +} + +TEST_F(ConfigSessionTestFixture, actionLevelLoadsFromMetadataFile) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path metadataFile = sessionDir / "conf_metadata.json"; + + // Create session directory and metadata file manually + fs::create_directories(sessionDir); + std::ofstream metaFile(metadataFile); + // Use symbolic enum names for human readability + metaFile << R"({"action":{"WEDGE_AGENT":"AGENT_RESTART"}})"; + metaFile.close(); + + // Also create the session config file (otherwise session will overwrite from + // system) + fs::copy_file(systemConfigPath_, sessionConfig); + + // Create a ConfigSession - should load action level from metadata file + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Verify action level was loaded + EXPECT_EQ( + session.getRequiredAction(cli::AgentType::WEDGE_AGENT), + cli::ConfigActionLevel::AGENT_RESTART); +} + +TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // First session: set action level + { + TestableConfigSession session1( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + session1.updateRequiredAction( + cli::ConfigActionLevel::AGENT_RESTART, cli::AgentType::WEDGE_AGENT); + } + + // Second session: verify action level was persisted + { + TestableConfigSession session2( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + EXPECT_EQ( + session2.getRequiredAction(cli::AgentType::WEDGE_AGENT), + cli::ConfigActionLevel::AGENT_RESTART); + } +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/utils/CmdUtilsCommon.h b/fboss/cli/fboss2/utils/CmdUtilsCommon.h index 028f8e8cf94d9..a7ee723252ed8 100644 --- a/fboss/cli/fboss2/utils/CmdUtilsCommon.h +++ b/fboss/cli/fboss2/utils/CmdUtilsCommon.h @@ -62,6 +62,8 @@ enum class ObjectArgTypeId : uint8_t { OBJECT_ARG_TYPE_MTU, OBJECT_ARG_TYPE_ID_INTERFACE_LIST, OBJECT_ARG_TYPE_ID_REVISION_LIST, + OBJECT_ARG_TYPE_ID_BUFFER_POOL_NAME, + OBJECT_ARG_TYPE_ID_BUFFER_BYTES, }; template From 737626190fd82b432fe0a90a3c8eb9eefb7b203c Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Mon, 5 Jan 2026 21:22:01 +0000 Subject: [PATCH 7/9] Add a `fboss2 config interface switchport access vlan ` command. This doesn't yet automatically create the VLAN if it doesn't exist. --- cmake/CliFboss2.cmake | 4 + cmake/CliFboss2Test.cmake | 1 + fboss/cli/fboss2/BUCK | 4 + fboss/cli/fboss2/CmdHandlerImplConfig.cpp | 12 + fboss/cli/fboss2/CmdListConfig.cpp | 22 ++ fboss/cli/fboss2/CmdSubcommands.cpp | 3 + .../switchport/CmdConfigInterfaceSwitchport.h | 42 +++ .../CmdConfigInterfaceSwitchportAccess.h | 43 +++ ...CmdConfigInterfaceSwitchportAccessVlan.cpp | 54 ++++ .../CmdConfigInterfaceSwitchportAccessVlan.h | 82 ++++++ fboss/cli/fboss2/test/BUCK | 1 + ...onfigInterfaceSwitchportAccessVlanTest.cpp | 244 ++++++++++++++++++ fboss/cli/fboss2/utils/CmdUtilsCommon.h | 1 + 13 files changed, 513 insertions(+) create mode 100644 fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h create mode 100644 fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h create mode 100644 fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp create mode 100644 fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h create mode 100644 fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index ee8d8846759bd..f27b04561c96e 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -589,6 +589,10 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.cpp fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.cpp + fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h + fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h + fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h + fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index 22629f620329c..de0c6689bfed2 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -7,6 +7,7 @@ add_executable(fboss2_cmd_test fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp fboss/cli/fboss2/test/CmdConfigInterfaceDescriptionTest.cpp fboss/cli/fboss2/test/CmdConfigInterfaceMtuTest.cpp + fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp fboss/cli/fboss2/test/CmdConfigQosBufferPoolTest.cpp fboss/cli/fboss2/test/CmdConfigReloadTest.cpp fboss/cli/fboss2/test/CmdConfigSessionDiffTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 9b8c44a700ca9..c2eee6fc2c4d0 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -785,6 +785,7 @@ cpp_library( "commands/config/history/CmdConfigHistory.cpp", "commands/config/interface/CmdConfigInterfaceDescription.cpp", "commands/config/interface/CmdConfigInterfaceMtu.cpp", + "commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp", "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.cpp", "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolReservedBytes.cpp", "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolSharedBytes.cpp", @@ -800,6 +801,9 @@ cpp_library( "commands/config/interface/CmdConfigInterface.h", "commands/config/interface/CmdConfigInterfaceDescription.h", "commands/config/interface/CmdConfigInterfaceMtu.h", + "commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h", + "commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h", + "commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h", "commands/config/qos/CmdConfigQos.h", "commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h", "commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h", diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 656ec92256473..a8b284515abd1 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -16,6 +16,9 @@ #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h" #include "fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h" #include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" #include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h" @@ -36,6 +39,15 @@ template void CmdHandler< CmdConfigInterfaceDescriptionTraits>::run(); template void CmdHandler::run(); +template void CmdHandler< + CmdConfigInterfaceSwitchport, + CmdConfigInterfaceSwitchportTraits>::run(); +template void CmdHandler< + CmdConfigInterfaceSwitchportAccess, + CmdConfigInterfaceSwitchportAccessTraits>::run(); +template void CmdHandler< + CmdConfigInterfaceSwitchportAccessVlan, + CmdConfigInterfaceSwitchportAccessVlanTraits>::run(); template void CmdHandler::run(); template void CmdHandler::run(); template void diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index b7c213c90ae62..6fd92e4c9079f 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -17,6 +17,9 @@ #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h" #include "fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h" #include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" #include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPoolHeadroomBytes.h" @@ -59,6 +62,25 @@ const CommandTree& kConfigCommandTree() { "Set interface MTU", commandHandler, argTypeHandler, + }, + { + "switchport", + "Configure switchport settings", + commandHandler, + argTypeHandler, + {{ + "access", + "Configure access mode settings", + commandHandler, + argTypeHandler, + {{ + "vlan", + "Set access VLAN (ingressVlan) for the interface", + commandHandler, + argTypeHandler< + CmdConfigInterfaceSwitchportAccessVlanTraits>, + }}, + }}, }}, }, diff --git a/fboss/cli/fboss2/CmdSubcommands.cpp b/fboss/cli/fboss2/CmdSubcommands.cpp index 35a4d3078a424..2003d0f906e83 100644 --- a/fboss/cli/fboss2/CmdSubcommands.cpp +++ b/fboss/cli/fboss2/CmdSubcommands.cpp @@ -235,6 +235,9 @@ CLI::App* CmdSubcommands::addCommand( case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_BYTES: subCmd->add_option("bytes", args, "Buffer size in bytes"); break; + case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_VLAN_ID: + subCmd->add_option("vlan_id", args, "VLAN ID (1-4094)"); + break; case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_UNINITIALIZE: case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE: break; diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h b/fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h new file mode 100644 index 0000000000000..c32a5eb3e0f32 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" +#include "fboss/cli/fboss2/utils/InterfaceList.h" + +namespace facebook::fboss { + +struct CmdConfigInterfaceSwitchportTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigInterface; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE; + using ObjectArgType = std::monostate; + using RetType = std::string; +}; + +class CmdConfigInterfaceSwitchport : public CmdHandler< + CmdConfigInterfaceSwitchport, + CmdConfigInterfaceSwitchportTraits> { + public: + RetType queryClient( + const HostInfo& /* hostInfo */, + const utils::InterfaceList& /* interfaces */) { + throw std::runtime_error( + "Incomplete command, please use one of the subcommands"); + } + + void printOutput(const RetType& /* model */) {} +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h b/fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h new file mode 100644 index 0000000000000..0eb3ce70b3556 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" +#include "fboss/cli/fboss2/utils/InterfaceList.h" + +namespace facebook::fboss { + +struct CmdConfigInterfaceSwitchportAccessTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigInterfaceSwitchport; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE; + using ObjectArgType = std::monostate; + using RetType = std::string; +}; + +class CmdConfigInterfaceSwitchportAccess + : public CmdHandler< + CmdConfigInterfaceSwitchportAccess, + CmdConfigInterfaceSwitchportAccessTraits> { + public: + RetType queryClient( + const HostInfo& /* hostInfo */, + const utils::InterfaceList& /* interfaces */) { + throw std::runtime_error( + "Incomplete command, please use one of the subcommands"); + } + + void printOutput(const RetType& /* model */) {} +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp new file mode 100644 index 0000000000000..fe4cd1c17df53 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp @@ -0,0 +1,54 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h" + +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +CmdConfigInterfaceSwitchportAccessVlanTraits::RetType +CmdConfigInterfaceSwitchportAccessVlan::queryClient( + const HostInfo& hostInfo, + const utils::InterfaceList& interfaces, + const CmdConfigInterfaceSwitchportAccessVlanTraits::ObjectArgType& + vlanIdValue) { + if (interfaces.empty()) { + throw std::invalid_argument("No interface name provided"); + } + + // Extract the VLAN ID (validation already done in VlanIdValue constructor) + int32_t vlanId = vlanIdValue.getVlanId(); + + // Update ingressVlan for all resolved ports + for (const utils::Intf& intf : interfaces) { + cfg::Port* port = intf.getPort(); + if (port) { + port->ingressVlan() = vlanId; + } + } + + // Save the updated config + ConfigSession::getInstance().saveConfig(); + + std::string interfaceList = folly::join(", ", interfaces.getNames()); + std::string message = "Successfully set access VLAN for interface(s) " + + interfaceList + " to " + std::to_string(vlanId); + + return message; +} + +void CmdConfigInterfaceSwitchportAccessVlan::printOutput( + const CmdConfigInterfaceSwitchportAccessVlanTraits::RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h new file mode 100644 index 0000000000000..218032da34b8a --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h @@ -0,0 +1,82 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" +#include "fboss/cli/fboss2/utils/InterfaceList.h" + +namespace facebook::fboss { + +// Custom type for VLAN ID argument with validation +class VlanIdValue : public utils::BaseObjectArgType { + public: + /* implicit */ VlanIdValue(std::vector v) { + if (v.empty()) { + throw std::invalid_argument("VLAN ID is required"); + } + if (v.size() != 1) { + throw std::invalid_argument( + "Expected single VLAN ID, got: " + folly::join(", ", v)); + } + + try { + int32_t vlanId = folly::to(v[0]); + // VLAN IDs are typically 1-4094 (0 and 4095 are reserved) + if (vlanId < 1 || vlanId > 4094) { + throw std::invalid_argument( + "VLAN ID must be between 1 and 4094 inclusive, got: " + + std::to_string(vlanId)); + } + data_.push_back(vlanId); + } catch (const folly::ConversionError& e) { + throw std::invalid_argument("Invalid VLAN ID: " + v[0]); + } + } + + int32_t getVlanId() const { + return data_[0]; + } + + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_VLAN_ID; +}; + +struct CmdConfigInterfaceSwitchportAccessVlanTraits + : public WriteCommandTraits { + using ParentCmd = CmdConfigInterfaceSwitchportAccess; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_VLAN_ID; + using ObjectArgType = VlanIdValue; + using RetType = std::string; +}; + +class CmdConfigInterfaceSwitchportAccessVlan + : public CmdHandler< + CmdConfigInterfaceSwitchportAccessVlan, + CmdConfigInterfaceSwitchportAccessVlanTraits> { + public: + using ObjectArgType = + CmdConfigInterfaceSwitchportAccessVlanTraits::ObjectArgType; + using RetType = CmdConfigInterfaceSwitchportAccessVlanTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const utils::InterfaceList& interfaces, + const ObjectArgType& vlanId); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/BUCK b/fboss/cli/fboss2/test/BUCK index 41ef684bbb1ec..a5e9e90bc50bf 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -59,6 +59,7 @@ cpp_unittest( "CmdConfigHistoryTest.cpp", "CmdConfigInterfaceDescriptionTest.cpp", "CmdConfigInterfaceMtuTest.cpp", + "CmdConfigInterfaceSwitchportAccessVlanTest.cpp", "CmdConfigQosBufferPoolTest.cpp", "CmdConfigReloadTest.cpp", "CmdConfigSessionDiffTest.cpp", diff --git a/fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp b/fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp new file mode 100644 index 0000000000000..5a426a939b473 --- /dev/null +++ b/fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp @@ -0,0 +1,244 @@ +/* + * 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 "fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h" +#include "fboss/cli/fboss2/test/CmdHandlerTestBase.h" +#include "fboss/cli/fboss2/test/TestableConfigSession.h" +#include "fboss/cli/fboss2/utils/PortMap.h" + +namespace fs = std::filesystem; + +using namespace ::testing; + +namespace facebook::fboss { + +class CmdConfigInterfaceSwitchportAccessVlanTestFixture + : public CmdHandlerTestBase { + public: + void SetUp() override { + CmdHandlerTestBase::SetUp(); + + // Create unique test directories + auto tempBase = fs::temp_directory_path(); + auto uniquePath = boost::filesystem::unique_path( + "fboss_switchport_test_%%%%-%%%%-%%%%-%%%%"); + testHomeDir_ = tempBase / (uniquePath.string() + "_home"); + testEtcDir_ = tempBase / (uniquePath.string() + "_etc"); + + std::error_code ec; + if (fs::exists(testHomeDir_)) { + fs::remove_all(testHomeDir_, ec); + } + if (fs::exists(testEtcDir_)) { + fs::remove_all(testEtcDir_, ec); + } + + // Create test directories + fs::create_directories(testHomeDir_); + fs::create_directories(testEtcDir_ / "coop"); + fs::create_directories(testEtcDir_ / "coop" / "cli"); + + // Set environment variables + setenv("HOME", testHomeDir_.c_str(), 1); + setenv("USER", "testuser", 1); + + // Create a test system config file with ports + fs::path initialRevision = testEtcDir_ / "coop" / "cli" / "agent-r1.conf"; + createTestConfig(initialRevision, R"({ + "sw": { + "ports": [ + { + "logicalID": 1, + "name": "eth1/1/1", + "state": 2, + "speed": 100000, + "ingressVlan": 1 + }, + { + "logicalID": 2, + "name": "eth1/2/1", + "state": 2, + "speed": 100000, + "ingressVlan": 1 + } + ] + } +})"); + + // Create symlink + systemConfigPath_ = testEtcDir_ / "coop" / "agent.conf"; + fs::create_symlink(initialRevision, systemConfigPath_); + + // Create session config path + sessionConfigPath_ = testHomeDir_ / ".fboss2" / "agent.conf"; + cliConfigDir_ = testEtcDir_ / "coop" / "cli"; + } + + void TearDown() override { + std::error_code ec; + if (fs::exists(testHomeDir_)) { + fs::remove_all(testHomeDir_, ec); + } + if (fs::exists(testEtcDir_)) { + fs::remove_all(testEtcDir_, ec); + } + CmdHandlerTestBase::TearDown(); + } + + protected: + void createTestConfig(const fs::path& path, const std::string& content) { + std::ofstream file(path); + file << content; + file.close(); + } + + fs::path testHomeDir_; + fs::path testEtcDir_; + fs::path systemConfigPath_; + fs::path sessionConfigPath_; + fs::path cliConfigDir_; +}; + +// Tests for VlanIdValue validation + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdValidMin) { + VlanIdValue vlanId({"1"}); + EXPECT_EQ(vlanId.getVlanId(), 1); +} + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdValidMax) { + VlanIdValue vlanId({"4094"}); + EXPECT_EQ(vlanId.getVlanId(), 4094); +} + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdValidMid) { + VlanIdValue vlanId({"100"}); + EXPECT_EQ(vlanId.getVlanId(), 100); +} + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdZeroInvalid) { + EXPECT_THROW(VlanIdValue({"0"}), std::invalid_argument); +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdTooHighInvalid) { + EXPECT_THROW(VlanIdValue({"4095"}), std::invalid_argument); +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdNegativeInvalid) { + EXPECT_THROW(VlanIdValue({"-1"}), std::invalid_argument); +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdNonNumericInvalid) { + EXPECT_THROW(VlanIdValue({"abc"}), std::invalid_argument); +} + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdEmptyInvalid) { + EXPECT_THROW(VlanIdValue({}), std::invalid_argument); +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdMultipleValuesInvalid) { + EXPECT_THROW(VlanIdValue({"100", "200"}), std::invalid_argument); +} + +// Test error message format +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdOutOfRangeErrorMessage) { + try { + VlanIdValue({"9999"}); + FAIL() << "Expected std::invalid_argument"; + } catch (const std::invalid_argument& e) { + std::string errorMsg = e.what(); + EXPECT_THAT(errorMsg, HasSubstr("VLAN ID must be between 1 and 4094")); + EXPECT_THAT(errorMsg, HasSubstr("9999")); + } +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdNonNumericErrorMessage) { + try { + VlanIdValue({"notanumber"}); + FAIL() << "Expected std::invalid_argument"; + } catch (const std::invalid_argument& e) { + std::string errorMsg = e.what(); + EXPECT_THAT(errorMsg, HasSubstr("Invalid VLAN ID")); + EXPECT_THAT(errorMsg, HasSubstr("notanumber")); + } +} + +// Tests for queryClient + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + queryClientSetsIngressVlanMultiplePorts) { + TestableConfigSession session( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string()); + + auto cmd = CmdConfigInterfaceSwitchportAccessVlan(); + VlanIdValue vlanId({"2001"}); + + // Create InterfaceList from port names + utils::InterfaceList interfaces({"eth1/1/1", "eth1/2/1"}); + + auto result = cmd.queryClient(localhost(), interfaces, vlanId); + + EXPECT_THAT(result, HasSubstr("Successfully set access VLAN")); + EXPECT_THAT(result, HasSubstr("eth1/1/1")); + EXPECT_THAT(result, HasSubstr("eth1/2/1")); + EXPECT_THAT(result, HasSubstr("2001")); + + // Verify the ingressVlan was updated for both ports + auto& config = session.getAgentConfig(); + auto& switchConfig = *config.sw(); + auto& ports = *switchConfig.ports(); + for (const auto& port : ports) { + if (*port.name() == "eth1/1/1" || *port.name() == "eth1/2/1") { + EXPECT_EQ(*port.ingressVlan(), 2001); + } + } +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + queryClientThrowsOnEmptyInterfaceList) { + TestableConfigSession session( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string()); + + auto cmd = CmdConfigInterfaceSwitchportAccessVlan(); + VlanIdValue vlanId({"100"}); + + // Empty InterfaceList is valid to construct but queryClient should throw + utils::InterfaceList emptyInterfaces({}); + EXPECT_THROW( + cmd.queryClient(localhost(), emptyInterfaces, vlanId), + std::invalid_argument); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/utils/CmdUtilsCommon.h b/fboss/cli/fboss2/utils/CmdUtilsCommon.h index a7ee723252ed8..7a1987f0d9701 100644 --- a/fboss/cli/fboss2/utils/CmdUtilsCommon.h +++ b/fboss/cli/fboss2/utils/CmdUtilsCommon.h @@ -64,6 +64,7 @@ enum class ObjectArgTypeId : uint8_t { OBJECT_ARG_TYPE_ID_REVISION_LIST, OBJECT_ARG_TYPE_ID_BUFFER_POOL_NAME, OBJECT_ARG_TYPE_ID_BUFFER_BYTES, + OBJECT_ARG_TYPE_VLAN_ID, }; template From 379e043da2d7606dfde5c80fb70e011b8b9fc74d Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Fri, 9 Jan 2026 22:19:41 +0000 Subject: [PATCH 8/9] Add a unit tests checking the CLI command tree. A recent merge introduced a duplicate command by mistake (bad merge on my part) and this escaped because of lack of test coverage. Also make sure we keep `cmake/CliFboss2Test.cmake` sorted. --- cmake/CliFboss2Test.cmake | 4 ++- fboss/cli/fboss2/test/BUCK | 1 + fboss/cli/fboss2/test/CmdListConfigTest.cpp | 30 +++++++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 fboss/cli/fboss2/test/CmdListConfigTest.cpp diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index de0c6689bfed2..138a6fcf05c0a 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -2,6 +2,7 @@ # cmd_test - Command tests from BUCK file add_executable(fboss2_cmd_test + fboss/cli/fboss2/oss/CmdListConfig.cpp fboss/cli/fboss2/test/TestMain.cpp fboss/cli/fboss2/test/CmdConfigAppliedInfoTest.cpp fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp @@ -12,6 +13,8 @@ add_executable(fboss2_cmd_test fboss/cli/fboss2/test/CmdConfigReloadTest.cpp fboss/cli/fboss2/test/CmdConfigSessionDiffTest.cpp fboss/cli/fboss2/test/CmdConfigSessionTest.cpp + fboss/cli/fboss2/test/CmdGetPcapTest.cpp + fboss/cli/fboss2/test/CmdListConfigTest.cpp fboss/cli/fboss2/test/CmdSetPortStateTest.cpp fboss/cli/fboss2/test/CmdShowAclTest.cpp fboss/cli/fboss2/test/CmdShowAgentSslTest.cpp @@ -23,7 +26,6 @@ add_executable(fboss2_cmd_test fboss/cli/fboss2/test/CmdShowL2Test.cpp fboss/cli/fboss2/test/CmdShowLldpTest.cpp fboss/cli/fboss2/test/CmdShowNdpTest.cpp - fboss/cli/fboss2/test/CmdGetPcapTest.cpp fboss/cli/fboss2/test/CmdShowAggregatePortTest.cpp fboss/cli/fboss2/test/CmdShowCpuPortTest.cpp fboss/cli/fboss2/test/CmdShowExampleTest.cpp diff --git a/fboss/cli/fboss2/test/BUCK b/fboss/cli/fboss2/test/BUCK index a5e9e90bc50bf..68e4952697687 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -65,6 +65,7 @@ cpp_unittest( "CmdConfigSessionDiffTest.cpp", "CmdConfigSessionTest.cpp", "CmdGetPcapTest.cpp", + "CmdListConfigTest.cpp", "CmdSetPortStateTest.cpp", "CmdShowAclTest.cpp", "CmdShowAgentSslTest.cpp", diff --git a/fboss/cli/fboss2/test/CmdListConfigTest.cpp b/fboss/cli/fboss2/test/CmdListConfigTest.cpp new file mode 100644 index 0000000000000..72823dd6afeca --- /dev/null +++ b/fboss/cli/fboss2/test/CmdListConfigTest.cpp @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include + +#include "fboss/cli/fboss2/CmdList.h" +#include "fboss/cli/fboss2/CmdSubcommands.h" + +namespace facebook::fboss { + +// This test verifies that the command trees can be successfully registered +// with CLI11 without throwing CLI::OptionAlreadyAdded exceptions due to +// duplicate subcommand names. +TEST(CmdListConfigTest, noDuplicateSubcommands) { + CLI::App app{"Test CLI"}; + + // This will throw CLI::OptionAlreadyAdded if there are duplicate subcommands + EXPECT_NO_THROW( + CmdSubcommands().init( + app, kCommandTree(), kAdditionalCommandTree(), kSpecialCommands())); +} + +} // namespace facebook::fboss From 42fe595367fc7402f5d2f02b81c91303cd4afaaf Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Tue, 13 Jan 2026 17:08:09 +0000 Subject: [PATCH 9/9] Save the config commands to CLI metadata and in session commit. Now every config command is saved in the CLI session metadata so we can easily tell what commands were used in a given session. The metadata is now also saved along the config when we commit the session. A future commit will make rollback also rely on this metadata to decide whether or not to restart the agent. --- fboss/cli/fboss2/cli_metadata.thrift | 3 + fboss/cli/fboss2/session/ConfigSession.cpp | 97 +++++- fboss/cli/fboss2/session/ConfigSession.h | 16 +- .../cli/fboss2/test/CmdConfigSessionTest.cpp | 316 ++++++++++++++++-- fboss/cli/fboss2/test/TestableConfigSession.h | 3 + 5 files changed, 394 insertions(+), 41 deletions(-) diff --git a/fboss/cli/fboss2/cli_metadata.thrift b/fboss/cli/fboss2/cli_metadata.thrift index 18b82b687d92e..97247c320fc7d 100644 --- a/fboss/cli/fboss2/cli_metadata.thrift +++ b/fboss/cli/fboss2/cli_metadata.thrift @@ -32,4 +32,7 @@ struct ConfigSessionMetadata { // Maps each agent to the required action level for pending config changes. // Agents not in this map default to HITLESS. 1: map action; + // List of CLI commands executed in this session, in chronological order. + // Each entry is the full command string (e.g., "config interface eth1/1/1 mtu 9000"). + 2: list commands; } diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index 1bb720f1cf4f6..45fccb8b571d9 100644 --- a/fboss/cli/fboss2/session/ConfigSession.cpp +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -191,6 +192,35 @@ int getCurrentRevisionNumber(const std::string& systemConfigPath) { return ConfigSession::extractRevisionNumber(target); } +/* + * Read the command line from /proc/self/cmdline, skipping argv[0]. + * Returns the command arguments as a space-separated string, + * e.g., "config interface eth1/1/1 mtu 9000" + */ +std::string readCommandLineFromProc() { + std::ifstream file("/proc/self/cmdline"); + if (!file) { + throw std::runtime_error( + fmt::format( + "Failed to open /proc/self/cmdline: {}", folly::errnoStr(errno))); + } + + std::vector args; + std::string arg; + bool first = true; + while (std::getline(file, arg, '\0')) { + if (first) { + // Skip argv[0] (program name) + first = false; + continue; + } + if (!arg.empty()) { + args.push_back(arg); + } + } + return folly::join(" ", args); +} + } // anonymous namespace ConfigSession::ConfigSession() { @@ -298,7 +328,7 @@ void ConfigSession::saveConfig( // (like clientIdToAdminDistance) by converting them to strings. // If we use facebook::thrift::to_dynamic() directly, the integer keys // are preserved as integers in the folly::dynamic object, which causes - // folly::toPrettyJson() to fail because JSON objects require string keys. + // folly::toPrettyJson() to fail because JSON objects requires string keys. std::string json = apache::thrift::SimpleJSONSerializer::serialize( agentConfig_); @@ -311,10 +341,26 @@ void ConfigSession::saveConfig( folly::writeFileAtomic( sessionConfigPath_, prettyJson, 0644, folly::SyncType::WITH_SYNC); - // If an action level was provided, update the required action metadata + // Automatically record the command from /proc/self/cmdline. + // This ensures all config commands are tracked without requiring manual + // instrumentation in each command implementation. + std::string rawCmd = readCommandLineFromProc(); + CHECK(!rawCmd.empty()) + << "saveConfig() called with no command line arguments"; + // Only record if this is a config command and not already the last one + // recorded as that'd be idempotent anyway. + if (rawCmd.find("config ") == 0 && + (commands_.empty() || commands_.back() != rawCmd)) { + commands_.push_back(rawCmd); + } + + // If an action level was provided, update the required action level if (actionLevel.has_value()) { updateRequiredAction(*actionLevel, agent); } + + // Save command history and action levels to metadata + saveMetadata(); } int ConfigSession::extractRevisionNumber(const std::string& filenameOrPath) { @@ -350,7 +396,7 @@ std::string ConfigSession::getServiceName(cli::AgentType agent) { throw std::runtime_error("Unknown agent type"); } -void ConfigSession::loadActionLevel() { +void ConfigSession::loadMetadata() { std::string metadataPath = getMetadataPath(); // Note: We don't initialize requiredActions_ here since getRequiredAction() // returns HITLESS by default for agents not in the map, and @@ -377,19 +423,21 @@ void ConfigSession::loadActionLevel() { facebook::thrift::dynamic_format::PORTABLE, facebook::thrift::format_adherence::LENIENT); requiredActions_ = *metadata.action(); + commands_ = *metadata.commands(); } catch (const std::exception& ex) { // If JSON parsing fails, keep defaults LOG(WARNING) << "Failed to parse metadata file: " << ex.what(); } } -void ConfigSession::saveActionLevel() { +void ConfigSession::saveMetadata() { std::string metadataPath = getMetadataPath(); // Build Thrift metadata struct and serialize to JSON with symbolic enum names // Using PORTABLE format for human-readable enum names instead of integers cli::ConfigSessionMetadata metadata; metadata.action() = requiredActions_; + metadata.commands() = commands_; folly::dynamic json = facebook::thrift::to_dynamic( metadata, facebook::thrift::dynamic_format::PORTABLE); @@ -410,7 +458,6 @@ void ConfigSession::updateRequiredAction( if (static_cast(actionLevel) > static_cast(requiredActions_[agent])) { requiredActions_[agent] = actionLevel; - saveActionLevel(); } } @@ -425,6 +472,7 @@ cli::ConfigActionLevel ConfigSession::getRequiredAction( void ConfigSession::resetRequiredAction(cli::AgentType agent) { requiredActions_[agent] = cli::ConfigActionLevel::HITLESS; + commands_.clear(); // If all agents are HITLESS, remove the file entirely bool allHitless = true; @@ -441,7 +489,17 @@ void ConfigSession::resetRequiredAction(cli::AgentType agent) { // Ignore errors - file might not exist } else { // Only save if there are remaining agents with non-HITLESS levels - saveActionLevel(); + saveMetadata(); + } +} + +const std::vector& ConfigSession::getCommands() const { + return commands_; +} + +void ConfigSession::addCommand(const std::string& command) { + if (!command.empty() && (commands_.empty() || commands_.back() != command)) { + commands_.push_back(command); } } @@ -513,9 +571,12 @@ void ConfigSession::initializeSession() { fs::path sessionPath(sessionConfigPath_); ensureDirectoryExists(sessionPath.parent_path().string()); copySystemConfigToSession(); + // Create initial empty metadata file for new sessions + saveMetadata(); + } else { + // Load metadata from disk (survives across CLI invocations) + loadMetadata(); } - // Load the action level from disk (survives across CLI invocations) - loadActionLevel(); } void ConfigSession::copySystemConfigToSession() { @@ -600,6 +661,23 @@ ConfigSession::CommitResult ConfigSession::commit(const HostInfo& hostInfo) { ec.message())); } + // 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); + if (ec) { + // Clean up the revision file we created + fs::remove(targetConfigPath); + throw std::runtime_error( + fmt::format( + "Failed to copy metadata to {}: {}", + targetMetadataPath, + ec.message())); + } + // Atomically update the symlink to point to the new config atomicSymlinkUpdate(systemConfigPath_, targetConfigPath); @@ -735,6 +813,9 @@ int ConfigSession::rollback( atomicSymlinkUpdate(systemConfigPath_, newRevisionPath); // Reload the config - if this fails, atomically undo the rollback + // 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. try { auto client = utils::createClient>( diff --git a/fboss/cli/fboss2/session/ConfigSession.h b/fboss/cli/fboss2/session/ConfigSession.h index 4e67e769366a0..32e14c83cb2ac 100644 --- a/fboss/cli/fboss2/session/ConfigSession.h +++ b/fboss/cli/fboss2/session/ConfigSession.h @@ -161,6 +161,9 @@ class ConfigSession { // The agent parameter specifies which agent to reset the action level for. void resetRequiredAction(cli::AgentType agent = cli::AgentType::WEDGE_AGENT); + // Get the list of commands executed in this session + const std::vector& getCommands() const; + protected: // Constructor for testing with custom paths ConfigSession( @@ -171,6 +174,10 @@ class ConfigSession { // Set the singleton instance (for testing only) static void setInstance(std::unique_ptr instance); + // Add a command to the history (for testing only) + // This allows tests to simulate command tracking without /proc/self/cmdline + void addCommand(const std::string& command); + private: std::string sessionConfigPath_; std::string systemConfigPath_; @@ -187,12 +194,15 @@ class ConfigSession { // session. std::map requiredActions_; + // List of commands executed in this session, persisted to disk + std::vector commands_; + // Path to the metadata file (e.g., ~/.fboss2/metadata) std::string getMetadataPath() const; - // Load/save action levels from/to disk - void loadActionLevel(); - void saveActionLevel(); + // Load/save metadata (action levels and commands) from disk + void loadMetadata(); + void saveMetadata(); // Restart an agent via systemd and wait for it to be active void restartAgent(cli::AgentType agent); diff --git a/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp b/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp index b04b7decb27e0..0f02feb097cf0 100644 --- a/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp +++ b/fboss/cli/fboss2/test/CmdConfigSessionTest.cpp @@ -218,6 +218,10 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { EXPECT_TRUE(fs::exists(targetConfig)); EXPECT_THAT(readFile(targetConfig), ::testing::HasSubstr("First commit")); + // Verify metadata file was created alongside the config revision + fs::path targetMetadata = cliConfigDir / "agent-r2.metadata.json"; + EXPECT_TRUE(fs::exists(targetMetadata)); + // Verify symlink was replaced and points to new revision EXPECT_TRUE(fs::is_symlink(systemConfigPath_)); EXPECT_EQ(fs::read_symlink(systemConfigPath_), targetConfig); @@ -249,17 +253,57 @@ TEST_F(ConfigSessionTestFixture, sessionCommit) { EXPECT_TRUE(fs::exists(targetConfig)); EXPECT_THAT(readFile(targetConfig), ::testing::HasSubstr("Second commit")); + // Verify metadata file was created alongside the config revision + fs::path targetMetadata = cliConfigDir / "agent-r3.metadata.json"; + EXPECT_TRUE(fs::exists(targetMetadata)); + // Verify symlink was updated to point to r3 EXPECT_TRUE(fs::is_symlink(systemConfigPath_)); EXPECT_EQ(fs::read_symlink(systemConfigPath_), targetConfig); - // Verify all revisions exist + // 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")); } } +// Ensure commit() works on a newly initialized session +// This verifies that initializeSession() creates the metadata file +TEST_F(ConfigSessionTestFixture, commitOnNewlyInitializedSession) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path cliConfigDir = testEtcDir_ / "coop" / "cli"; + + // Setup mock agent server + setupMockedAgentServer(); + EXPECT_CALL(getMockAgent(), reloadConfig()).Times(1); + + // Create a new session and immediately commit it + // This tests that metadata file is created during session initialization + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + cliConfigDir.string()); + + // Verify metadata file was created during session initialization + fs::path metadataPath = sessionDir / "conf_metadata.json"; + EXPECT_TRUE(fs::exists(metadataPath)); + + // Make no changes to the session. It's initialized but that's it. + + // Commit should succeed, right now empty sessions still commmit a new + // revision (TODO: fix this so we don't create empty commits). + auto result = session.commit(localhost()); + EXPECT_EQ(result.revision, 2); + + // Verify metadata file was copied to revision directory + fs::path targetMetadata = cliConfigDir / "agent-r2.metadata.json"; + EXPECT_TRUE(fs::exists(targetMetadata)); +} + TEST_F(ConfigSessionTestFixture, multipleChangesInOneSession) { fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; @@ -441,8 +485,9 @@ TEST_F(ConfigSessionTestFixture, concurrentSessionCreationSameUser) { fs::path cliConfigDir = testEtcDir_ / "coop" / "cli"; // Setup mock agent server + // Either 1 or 2 commits might succeed depending on the race setupMockedAgentServer(); - EXPECT_CALL(getMockAgent(), reloadConfig()).Times(2); + 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: @@ -452,14 +497,18 @@ TEST_F(ConfigSessionTestFixture, concurrentSessionCreationSameUser) { // 4. atomicSymlinkUpdate() - concurrent symlink updates // // Note: When two threads share the same session file, they race to modify it. - // The atomic operations ensure no crashes or corruption, but both commits - // might have the same content if one thread's saveConfig() overwrites the - // other's changes. This is expected behavior - the important thing is that - // both commits succeed without crashes. + // 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) { + auto commitTask = [&](const std::string& description, + std::atomic& rev, + std::atomic& noSession) { // Both threads use the SAME session path fs::path sessionDir = testHomeDir_ / ".fboss2_shared"; fs::path sessionConfig = sessionDir / "agent.conf"; @@ -475,28 +524,58 @@ TEST_F(ConfigSessionTestFixture, concurrentSessionCreationSameUser) { ports[0].description() = description; session.saveConfig(); - rev = session.commit(localhost()).revision; + 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::thread thread2(commitTask, "Second commit", std::ref(revision2)); + 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(); - // 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")); + // 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")); + } // The history command would list all three revisions with their metadata } @@ -725,16 +804,16 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsToMetadataFile) { fs::path sessionConfig = sessionDir / "agent.conf"; fs::path metadataFile = sessionDir / "conf_metadata.json"; - // Create a ConfigSession and set action level + // Create a ConfigSession and set action level via saveConfig { TestableConfigSession session( sessionConfig.string(), systemConfigPath_.string(), (testEtcDir_ / "coop" / "cli").string()); - // Set to AGENT_RESTART - session.updateRequiredAction( - cli::ConfigActionLevel::AGENT_RESTART, cli::AgentType::WEDGE_AGENT); + // Load the config (required before saveConfig) + session.getAgentConfig(); + session.saveConfig(cli::ConfigActionLevel::AGENT_RESTART); } // Verify metadata file exists and has correct JSON format @@ -782,15 +861,16 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { fs::path sessionDir = testHomeDir_ / ".fboss2"; fs::path sessionConfig = sessionDir / "agent.conf"; - // First session: set action level + // First session: set action level via saveConfig { TestableConfigSession session1( sessionConfig.string(), systemConfigPath_.string(), (testEtcDir_ / "coop" / "cli").string()); - session1.updateRequiredAction( - cli::ConfigActionLevel::AGENT_RESTART, cli::AgentType::WEDGE_AGENT); + // Load the config (required before saveConfig) + session1.getAgentConfig(); + session1.saveConfig(cli::ConfigActionLevel::AGENT_RESTART); } // Second session: verify action level was persisted @@ -806,4 +886,180 @@ TEST_F(ConfigSessionTestFixture, actionLevelPersistsAcrossSessions) { } } +TEST_F(ConfigSessionTestFixture, commandTrackingBasic) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path metadataFile = sessionDir / "conf_metadata.json"; + + // Create a ConfigSession, execute command, and verify persistence + { + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Initially, no commands should be recorded + EXPECT_TRUE(session.getCommands().empty()); + + // Simulate a command and save config + session.addCommand("config interface eth1/1/1 description Test change"); + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + ports[0].description() = "Test change"; + session.saveConfig(); + + // Verify command was recorded in memory + EXPECT_EQ(1, session.getCommands().size()); + EXPECT_EQ( + "config interface eth1/1/1 description Test change", + session.getCommands()[0]); + } + + // Verify metadata file exists and has commands persisted + EXPECT_TRUE(fs::exists(metadataFile)); + std::string content = readFile(metadataFile); + + // Parse the JSON and verify structure + folly::dynamic json = folly::parseJson(content); + EXPECT_TRUE(json.isObject()); + EXPECT_TRUE(json.count("commands")); + EXPECT_TRUE(json["commands"].isArray()); + EXPECT_EQ(1, json["commands"].size()); + EXPECT_EQ( + "config interface eth1/1/1 description Test change", + json["commands"][0].asString()); +} + +TEST_F(ConfigSessionTestFixture, commandTrackingMultipleCommands) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // Create a ConfigSession + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Execute multiple commands + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + + session.addCommand("config interface eth1/1/1 mtu 9000"); + ports[0].description() = "First change"; + session.saveConfig(); + + session.addCommand("config interface eth1/1/1 description Test"); + ports[0].description() = "Second change"; + session.saveConfig(); + + session.addCommand("config interface eth1/1/1 speed 100G"); + ports[0].description() = "Third change"; + session.saveConfig(); + + // Verify all commands were recorded in order + EXPECT_EQ(3, session.getCommands().size()); + EXPECT_EQ("config interface eth1/1/1 mtu 9000", session.getCommands()[0]); + EXPECT_EQ( + "config interface eth1/1/1 description Test", session.getCommands()[1]); + EXPECT_EQ("config interface eth1/1/1 speed 100G", session.getCommands()[2]); +} + +TEST_F(ConfigSessionTestFixture, commandTrackingPersistsAcrossSessions) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // First session: execute some commands + { + TestableConfigSession session1( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + auto& config = session1.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + + session1.addCommand("config interface eth1/1/1 mtu 9000"); + ports[0].description() = "First change"; + session1.saveConfig(); + + session1.addCommand("config interface eth1/1/1 description Test"); + ports[0].description() = "Second change"; + session1.saveConfig(); + } + + // Second session: verify commands were persisted + { + TestableConfigSession session2( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + EXPECT_EQ(2, session2.getCommands().size()); + EXPECT_EQ("config interface eth1/1/1 mtu 9000", session2.getCommands()[0]); + EXPECT_EQ( + "config interface eth1/1/1 description Test", + session2.getCommands()[1]); + } +} + +TEST_F(ConfigSessionTestFixture, commandTrackingClearedOnReset) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + + // Create a ConfigSession and add some commands + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + auto& config = session.getAgentConfig(); + auto& ports = *config.sw()->ports(); + ASSERT_FALSE(ports.empty()); + + session.addCommand("config interface eth1/1/1 mtu 9000"); + ports[0].description() = "Test change"; + session.saveConfig(); + + EXPECT_EQ(1, session.getCommands().size()); + + // Reset the action level (which also clears commands) + session.resetRequiredAction(cli::AgentType::WEDGE_AGENT); + + // Verify commands were cleared + EXPECT_TRUE(session.getCommands().empty()); +} + +TEST_F(ConfigSessionTestFixture, commandTrackingLoadsFromMetadataFile) { + fs::path sessionDir = testHomeDir_ / ".fboss2"; + fs::path sessionConfig = sessionDir / "agent.conf"; + fs::path metadataFile = sessionDir / "conf_metadata.json"; + + // Create session directory and metadata file manually + fs::create_directories(sessionDir); + std::ofstream metaFile(metadataFile); + metaFile << R"({ + "action": {"WEDGE_AGENT": "HITLESS"}, + "commands": ["cmd1", "cmd2", "cmd3"] + })"; + metaFile.close(); + + // Also create the session config file + fs::copy_file(systemConfigPath_, sessionConfig); + + // Create a ConfigSession - should load commands from metadata file + TestableConfigSession session( + sessionConfig.string(), + systemConfigPath_.string(), + (testEtcDir_ / "coop" / "cli").string()); + + // Verify commands were loaded + EXPECT_EQ(3, session.getCommands().size()); + EXPECT_EQ("cmd1", session.getCommands()[0]); + EXPECT_EQ("cmd2", session.getCommands()[1]); + EXPECT_EQ("cmd3", session.getCommands()[2]); +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/TestableConfigSession.h b/fboss/cli/fboss2/test/TestableConfigSession.h index 1f2179dcf6c10..5141ee0560716 100644 --- a/fboss/cli/fboss2/test/TestableConfigSession.h +++ b/fboss/cli/fboss2/test/TestableConfigSession.h @@ -26,6 +26,9 @@ class TestableConfigSession : public ConfigSession { // Expose protected setInstance() for testing using ConfigSession::setInstance; + + // Expose protected addCommand() for testing + using ConfigSession::addCommand; }; } // namespace facebook::fboss