From 0f56ea6767369dac0029547b251e3ab30901a001 Mon Sep 17 00:00:00 2001 From: Manoharan Sundaramoorthy Date: Mon, 12 Jan 2026 19:55:23 +0530 Subject: [PATCH 1/3] 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 | 11 + cmake/CliFboss2Test.cmake | 1 + fboss/cli/fboss2/BUCK | 13 + fboss/cli/fboss2/CmdHandlerImplConfig.cpp | 5 + fboss/cli/fboss2/CmdListConfig.cpp | 16 + fboss/cli/fboss2/CmdSubcommands.cpp | 7 + fboss/cli/fboss2/cli_metadata.thrift | 35 ++ .../fboss2/commands/config/qos/CmdConfigQos.h | 35 ++ .../buffer_pool/CmdConfigQosBufferPool.cpp | 155 +++++++++ .../qos/buffer_pool/CmdConfigQosBufferPool.h | 67 ++++ .../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 | 311 ++++++++++++++++++ .../cli/fboss2/test/CmdConfigSessionTest.cpp | 181 +++++++++- fboss/cli/fboss2/utils/CmdUtilsCommon.h | 1 + 17 files changed, 1117 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/CmdConfigQosBufferPool.cpp create mode 100644 fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h create mode 100644 fboss/cli/fboss2/test/CmdConfigQosBufferPoolTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index f8cf4dadbd293..ab46dac2e6fcd 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 @@ -588,6 +595,9 @@ 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.cpp + fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h fboss/cli/fboss2/commands/config/history/CmdConfigHistory.cpp fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h @@ -605,6 +615,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 b868a398f2fc8..f3f68a8f20401 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -38,6 +38,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 35a21bd461767..7563b64c11f63 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( @@ -792,6 +801,7 @@ cpp_library( "commands/config/history/CmdConfigHistory.cpp", "commands/config/interface/CmdConfigInterfaceDescription.cpp", "commands/config/interface/CmdConfigInterfaceMtu.cpp", + "commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp", "commands/config/rollback/CmdConfigRollback.cpp", "commands/config/session/CmdConfigSessionCommit.cpp", "commands/config/session/CmdConfigSessionDiff.cpp", @@ -805,6 +815,8 @@ 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/rollback/CmdConfigRollback.h", "commands/config/session/CmdConfigSessionCommit.h", "commands/config/session/CmdConfigSessionDiff.h", @@ -815,6 +827,7 @@ cpp_library( "fbsource//third-party/fmt:fmt", "fbsource//third-party/glog:glog", "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 7822a84641ab5..290831c6c86be 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -19,6 +19,8 @@ #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/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -40,5 +42,8 @@ template void CmdHandler::run(); template void CmdHandler::run(); +template void CmdHandler::run(); +template void +CmdHandler::run(); } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 05e070f392c0c..78e4ad709d3ee 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -17,6 +17,8 @@ #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/rollback/CmdConfigRollback.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionCommit.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" @@ -57,6 +59,20 @@ const CommandTree& kConfigCommandTree() { }}, }, + { + "config", + "qos", + "Configure QoS settings", + commandHandler, + argTypeHandler, + {{ + "buffer-pool", + "Configure buffer pool settings", + commandHandler, + argTypeHandler, + }}, + }, + { "config", "session", diff --git a/fboss/cli/fboss2/CmdSubcommands.cpp b/fboss/cli/fboss2/CmdSubcommands.cpp index 94bcd70ad244c..24221d3d6b753 100644 --- a/fboss/cli/fboss2/CmdSubcommands.cpp +++ b/fboss/cli/fboss2/CmdSubcommands.cpp @@ -232,6 +232,13 @@ 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_config", + args, + " [ ...] where is one " + "of: shared-bytes, headroom-bytes, reserved-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/CmdConfigQosBufferPool.cpp b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp new file mode 100644 index 0000000000000..25b5b131d24d8 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp @@ -0,0 +1,155 @@ +/* + * 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/CmdConfigQosBufferPool.h" + +#include +#include +#include +#include +#include +#include +#include + +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +namespace { + +const std::set kValidAttrs = { + "shared-bytes", + "headroom-bytes", + "reserved-bytes", +}; + +void validatePoolName(const std::string& name) { + // 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."); + } +} + +int32_t parseBytes(const std::string& value) { + try { + int32_t bytes = folly::to(value); + if (bytes < 0) { + throw std::invalid_argument( + fmt::format("Bytes value must be non-negative, got: {}", bytes)); + } + return bytes; + } catch (const folly::ConversionError&) { + throw std::invalid_argument(fmt::format("Invalid bytes value: {}", value)); + } +} + +} // namespace + +BufferPoolConfig::BufferPoolConfig(std::vector v) { + if (v.empty()) { + throw std::invalid_argument( + "Expected: [ ...] where is " + "one of: shared-bytes, headroom-bytes, reserved-bytes"); + } + + // First argument is the pool name + name_ = v[0]; + validatePoolName(name_); + data_.push_back(name_); + + // Remaining arguments are key-value pairs + if (v.size() < 3) { + throw std::invalid_argument( + "Expected at least one attribute-value pair after pool name. " + "Valid attributes are: shared-bytes, headroom-bytes, reserved-bytes"); + } + + if ((v.size() - 1) % 2 != 0) { + throw std::invalid_argument( + "Attribute-value pairs must come in pairs. Got odd number of " + "arguments after pool name."); + } + + for (size_t i = 1; i < v.size(); i += 2) { + const std::string& attr = v[i]; + const std::string& value = v[i + 1]; + + if (kValidAttrs.find(attr) == kValidAttrs.end()) { + throw std::invalid_argument( + fmt::format( + "Unknown attribute: '{}'. Valid attributes are: {}", + attr, + folly::join(", ", kValidAttrs))); + } + + attributes_.emplace_back(attr, value); + data_.push_back(attr); + data_.push_back(value); + } +} + +CmdConfigQosBufferPoolTraits::RetType CmdConfigQosBufferPool::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& config) { + const std::string& poolName = config.getName(); + + 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(); + + // Get or create the buffer pool config + auto it = bufferPoolConfigs.find(poolName); + if (it == bufferPoolConfigs.end()) { + // Create a new buffer pool config with default sharedBytes + cfg::BufferPoolConfig newConfig; + newConfig.sharedBytes() = 0; + bufferPoolConfigs[poolName] = std::move(newConfig); + it = bufferPoolConfigs.find(poolName); + } + + cfg::BufferPoolConfig& targetConfig = it->second; + + // Process each attribute-value pair + for (const auto& [attr, value] : config.getAttributes()) { + int32_t bytes = parseBytes(value); + if (attr == "shared-bytes") { + targetConfig.sharedBytes() = bytes; + } else if (attr == "headroom-bytes") { + targetConfig.headroomBytes() = bytes; + } else if (attr == "reserved-bytes") { + targetConfig.reservedBytes() = bytes; + } + } + + // Save the updated config - buffer pool changes require agent restart + session.saveConfig(cli::ConfigActionLevel::AGENT_RESTART); + + return fmt::format("Successfully configured buffer-pool '{}'", poolName); +} + +void CmdConfigQosBufferPool::printOutput(const RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // 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..6a79242ba0f2a --- /dev/null +++ b/fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h @@ -0,0 +1,67 @@ +/* + * 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/CmdUtilsCommon.h" + +namespace facebook::fboss { + +// Custom type for buffer pool configuration +// Parses: [ ...] +// where attr is one of: shared-bytes, headroom-bytes, reserved-bytes +class BufferPoolConfig : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BufferPoolConfig(std::vector v); + + const std::string& getName() const { + return name_; + } + + const std::vector>& getAttributes() + const { + return attributes_; + } + + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_POOL_NAME; + + private: + std::string name_; + std::vector> attributes_; +}; + +struct CmdConfigQosBufferPoolTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigQos; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_BUFFER_POOL_NAME; + using ObjectArgType = BufferPoolConfig; + using RetType = std::string; +}; + +class CmdConfigQosBufferPool + : public CmdHandler { + public: + using ObjectArgType = CmdConfigQosBufferPoolTraits::ObjectArgType; + using RetType = CmdConfigQosBufferPoolTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& config); + + 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 a88ce899d0ab0..8ba400a718b87 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -67,6 +67,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..b5dc507888064 --- /dev/null +++ b/fboss/cli/fboss2/test/CmdConfigQosBufferPoolTest.cpp @@ -0,0 +1,311 @@ +/* + * 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/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 BufferPoolConfig argument validation +TEST_F(CmdConfigQosBufferPoolTestFixture, bufferPoolConfigValidation) { + // Valid config with one attribute + EXPECT_NO_THROW(BufferPoolConfig({"ingress_pool", "shared-bytes", "1000"})); + EXPECT_NO_THROW( + BufferPoolConfig({"egress-lossy-pool", "headroom-bytes", "2000"})); + EXPECT_NO_THROW(BufferPoolConfig({"Pool1", "reserved-bytes", "3000"})); + + // Valid config with multiple attributes + EXPECT_NO_THROW(BufferPoolConfig( + {"test_pool", "shared-bytes", "1000", "headroom-bytes", "2000"})); + EXPECT_NO_THROW(BufferPoolConfig( + {"test_pool", + "shared-bytes", + "1000", + "headroom-bytes", + "2000", + "reserved-bytes", + "3000"})); + + // Empty should throw + EXPECT_THROW(BufferPoolConfig({}), std::invalid_argument); + + // Name only (no attributes) should throw + EXPECT_THROW(BufferPoolConfig({"pool1"}), std::invalid_argument); + + // Name with only one arg (odd number after name) should throw + EXPECT_THROW( + BufferPoolConfig({"pool1", "shared-bytes"}), std::invalid_argument); + + // Invalid pool names - must start with letter + EXPECT_THROW( + BufferPoolConfig({"123pool", "shared-bytes", "1000"}), + std::invalid_argument); + EXPECT_THROW( + BufferPoolConfig({"_pool", "shared-bytes", "1000"}), + std::invalid_argument); + + // Invalid attribute name + EXPECT_THROW( + BufferPoolConfig({"pool1", "invalid-attr", "1000"}), + std::invalid_argument); + + // Note: Invalid bytes values (negative, non-numeric) are validated in + // queryClient, not in the constructor. This allows the command to be + // constructed and provide better error messages at execution time. +} + +// Test BufferPoolConfig getters +TEST_F(CmdConfigQosBufferPoolTestFixture, bufferPoolConfigGetters) { + BufferPoolConfig config( + {"test_pool", "shared-bytes", "1000", "headroom-bytes", "2000"}); + + EXPECT_EQ(config.getName(), "test_pool"); + + const auto& attrs = config.getAttributes(); + ASSERT_EQ(attrs.size(), 2); + EXPECT_EQ(attrs[0].first, "shared-bytes"); + EXPECT_EQ(attrs[0].second, "1000"); + EXPECT_EQ(attrs[1].first, "headroom-bytes"); + EXPECT_EQ(attrs[1].second, "2000"); +} + +// 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 = CmdConfigQosBufferPool(); + BufferPoolConfig config({"test_pool", "shared-bytes", "50000"}); + + auto result = cmd.queryClient(localhost(), config); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully configured")); + EXPECT_THAT(result, ::testing::HasSubstr("test_pool")); + + // Verify the config was actually modified + auto& agentConfig = ConfigSession::getInstance().getAgentConfig(); + auto& switchConfig = *agentConfig.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 = CmdConfigQosBufferPool(); + BufferPoolConfig config({"headroom_pool", "headroom-bytes", "10000"}); + + auto result = cmd.queryClient(localhost(), config); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully configured")); + EXPECT_THAT(result, ::testing::HasSubstr("headroom_pool")); + + // Verify the config was actually modified + auto& agentConfig = ConfigSession::getInstance().getAgentConfig(); + auto& switchConfig = *agentConfig.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 = CmdConfigQosBufferPool(); + BufferPoolConfig config({"reserved_pool", "reserved-bytes", "20000"}); + + auto result = cmd.queryClient(localhost(), config); + + EXPECT_THAT(result, ::testing::HasSubstr("Successfully configured")); + EXPECT_THAT(result, ::testing::HasSubstr("reserved_pool")); + + // Verify the config was actually modified + auto& agentConfig = ConfigSession::getInstance().getAgentConfig(); + auto& switchConfig = *agentConfig.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 with multiple attributes +TEST_F(CmdConfigQosBufferPoolTestFixture, updateExistingBufferPool) { + TestableConfigSession::setInstance( + std::make_unique( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string())); + + auto cmd = CmdConfigQosBufferPool(); + + // Set all attributes in one command + BufferPoolConfig config( + {"existing_pool", + "shared-bytes", + "30000", + "headroom-bytes", + "5000", + "reserved-bytes", + "2000"}); + cmd.queryClient(localhost(), config); + + // Verify all values are set correctly + auto& agentConfig = ConfigSession::getInstance().getAgentConfig(); + auto& switchConfig = *agentConfig.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 buffer-pool command +TEST_F(CmdConfigQosBufferPoolTestFixture, printOutputBufferPool) { + auto cmd = CmdConfigQosBufferPool(); + std::string successMessage = "Successfully configured buffer-pool 'my_pool'"; + + 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 2f2eb34d5a614..53adaa39a9132 100644 --- a/fboss/cli/fboss2/utils/CmdUtilsCommon.h +++ b/fboss/cli/fboss2/utils/CmdUtilsCommon.h @@ -66,6 +66,7 @@ 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, }; template From c64dd21c1bac9eb5e947c07267da8f2df001e785 Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Mon, 5 Jan 2026 21:22:01 +0000 Subject: [PATCH 2/3] 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 ab46dac2e6fcd..1c50cf34d25d7 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -595,6 +595,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.cpp fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index f3f68a8f20401..fbd094efa1048 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -38,6 +38,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 7563b64c11f63..b5672da637526 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -801,6 +801,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/CmdConfigQosBufferPool.cpp", "commands/config/rollback/CmdConfigRollback.cpp", "commands/config/session/CmdConfigSessionCommit.cpp", @@ -815,6 +816,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/rollback/CmdConfigRollback.h", diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 290831c6c86be..515f4dbb239e4 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -19,6 +19,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/rollback/CmdConfigRollback.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 78e4ad709d3ee..697111d4daa13 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/rollback/CmdConfigRollback.h" @@ -56,6 +59,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 24221d3d6b753..f457b94d73513 100644 --- a/fboss/cli/fboss2/CmdSubcommands.cpp +++ b/fboss/cli/fboss2/CmdSubcommands.cpp @@ -239,6 +239,9 @@ CLI::App* CmdSubcommands::addCommand( " [ ...] where is one " "of: shared-bytes, headroom-bytes, reserved-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..e2af310574b50 --- /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&) { + 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 8ba400a718b87..3c0398c53b8cb 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -67,6 +67,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 53adaa39a9132..384deef7a7cdd 100644 --- a/fboss/cli/fboss2/utils/CmdUtilsCommon.h +++ b/fboss/cli/fboss2/utils/CmdUtilsCommon.h @@ -67,6 +67,7 @@ enum class ObjectArgTypeId : uint8_t { OBJECT_ARG_TYPE_ID_INTERFACE_LIST, OBJECT_ARG_TYPE_ID_REVISION_LIST, OBJECT_ARG_TYPE_ID_BUFFER_POOL_NAME, + OBJECT_ARG_TYPE_VLAN_ID, }; template From 2ce7e5f21828a2c294722ebfb278c5ab0578f96c Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Fri, 9 Jan 2026 22:19:41 +0000 Subject: [PATCH 3/3] 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 fbd094efa1048..f1d9dbcce2274 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -33,6 +33,7 @@ gtest_discover_tests(fboss2_framework_test) # 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 @@ -43,6 +44,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 @@ -54,7 +57,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 3c0398c53b8cb..f2ec51dd9d63e 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -73,6 +73,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