diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index bf99733e85fa4..bf9d190c99419 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -717,6 +717,10 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/l2/CmdConfigL2.h fboss/cli/fboss2/commands/config/l2/learning_mode/CmdConfigL2LearningMode.cpp fboss/cli/fboss2/commands/config/l2/learning_mode/CmdConfigL2LearningMode.h + fboss/cli/fboss2/commands/config/mac/CmdConfigMac.cpp + fboss/cli/fboss2/commands/config/mac/CmdConfigMac.h + fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.cpp + fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.h fboss/cli/fboss2/commands/config/protocol/CmdConfigProtocol.cpp fboss/cli/fboss2/commands/config/protocol/CmdConfigProtocol.h fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.cpp diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index 670736e44deae..d4c22e6dcd045 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -9,6 +9,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportAccessVlanTest.cpp fboss/cli/fboss2/test/config/CmdConfigInterfaceTest.cpp fboss/cli/fboss2/test/config/CmdConfigL2LearningModeTest.cpp + fboss/cli/fboss2/test/config/CmdConfigMacAgingTimeTest.cpp fboss/cli/fboss2/test/config/CmdConfigQosBufferPoolTest.cpp fboss/cli/fboss2/test/config/CmdConfigReloadTest.cpp fboss/cli/fboss2/test/config/CmdConfigSessionCommitTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index 83db4e88430c1..2b227275f16be 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -13,6 +13,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigInterfaceMtuTest.cpp fboss/cli/fboss2/test/integration_test/ConfigInterfaceProfileTest.cpp fboss/cli/fboss2/test/integration_test/ConfigL2LearningModeTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigMacTest.cpp fboss/cli/fboss2/test/integration_test/ConfigPfcTest.cpp fboss/cli/fboss2/test/integration_test/ConfigPortQueueConfigTest.cpp fboss/cli/fboss2/test/integration_test/ConfigQosPolicyMapTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index c7b694f119301..a105ffeb6e229 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1025,6 +1025,8 @@ cpp_library( "commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp", "commands/config/l2/CmdConfigL2.cpp", "commands/config/l2/learning_mode/CmdConfigL2LearningMode.cpp", + "commands/config/mac/CmdConfigMac.cpp", + "commands/config/mac/aging_time/CmdConfigMacAgingTime.cpp", "commands/config/protocol/CmdConfigProtocol.cpp", "commands/config/protocol/bgp/BgpConfigSession.cpp", "commands/config/protocol/bgp/CmdConfigProtocolBgp.cpp", @@ -1123,6 +1125,8 @@ cpp_library( "commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h", "commands/config/l2/CmdConfigL2.h", "commands/config/l2/learning_mode/CmdConfigL2LearningMode.h", + "commands/config/mac/CmdConfigMac.h", + "commands/config/mac/aging_time/CmdConfigMacAgingTime.h", "commands/config/protocol/CmdConfigProtocol.h", "commands/config/protocol/bgp/BgpConfigSession.h", "commands/config/protocol/bgp/CmdConfigProtocolBgp.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 80ef97e9567a2..c9b3b3f02c8a2 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -22,6 +22,8 @@ #include "fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h" #include "fboss/cli/fboss2/commands/config/l2/CmdConfigL2.h" #include "fboss/cli/fboss2/commands/config/l2/learning_mode/CmdConfigL2LearningMode.h" +#include "fboss/cli/fboss2/commands/config/mac/CmdConfigMac.h" +#include "fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.h" #include "fboss/cli/fboss2/commands/config/protocol/CmdConfigProtocol.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/CmdConfigProtocolBgp.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" @@ -170,6 +172,20 @@ const CommandTree& kConfigCommandTree() { }}, }, + { + "config", + "mac", + "Configure MAC settings", + commandHandler, + argRegistrar, + {{ + "aging-time", + "Set MAC address aging time in seconds", + commandHandler, + argRegistrar, + }}, + }, + { "config", "protocol", diff --git a/fboss/cli/fboss2/commands/config/mac/CmdConfigMac.cpp b/fboss/cli/fboss2/commands/config/mac/CmdConfigMac.cpp new file mode 100644 index 0000000000000..40d098e9601f5 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/mac/CmdConfigMac.cpp @@ -0,0 +1,20 @@ +/* + * 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/mac/CmdConfigMac.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +namespace facebook::fboss { + +// Explicit template instantiation +template void CmdHandler::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/mac/CmdConfigMac.h b/fboss/cli/fboss2/commands/config/mac/CmdConfigMac.h new file mode 100644 index 0000000000000..b71aa6da2982a --- /dev/null +++ b/fboss/cli/fboss2/commands/config/mac/CmdConfigMac.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" + +namespace facebook::fboss { + +struct CmdConfigMacTraits : public WriteCommandTraits { + using ObjectArgType = utils::NoneArgType; + using RetType = std::string; +}; + +class CmdConfigMac : public CmdHandler { + public: + using ObjectArgType = CmdConfigMacTraits::ObjectArgType; + using RetType = CmdConfigMacTraits::RetType; + + RetType queryClient(const HostInfo& /* hostInfo */) { + throw std::runtime_error( + "Incomplete command, please use 'aging-time' subcommand"); + } + + void printOutput(const RetType& /* model */) {} +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.cpp b/fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.cpp new file mode 100644 index 0000000000000..dbbff510b4916 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.cpp @@ -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. + * + */ + +#include "fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +MacAgingTimeArg::MacAgingTimeArg(std::vector v) { + if (v.empty()) { + throw std::invalid_argument( + "MAC aging time in seconds is required (e.g. 300)"); + } + if (v.size() != 1) { + throw std::invalid_argument("Expected exactly one argument: seconds"); + } + + try { + seconds_ = folly::to(v[0]); + } catch (const folly::ConversionError&) { + throw std::invalid_argument( + "Invalid MAC aging time '" + v[0] + "': must be an integer"); + } + + if (seconds_ <= 0) { + throw std::invalid_argument( + "MAC aging time must be a positive integer, got " + v[0]); + } + + data_.push_back(v[0]); +} + +CmdConfigMacAgingTimeTraits::RetType CmdConfigMacAgingTime::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& agingTime) { + auto& session = ConfigSession::getInstance(); + auto& config = session.getAgentConfig(); + auto& swConfig = *config.sw(); + + int32_t newSeconds = agingTime.getSeconds(); + int32_t currentSeconds = *swConfig.switchSettings()->l2AgeTimerSeconds(); + + if (currentSeconds == newSeconds) { + return fmt::format( + "MAC aging time is already set to {} seconds", newSeconds); + } + + swConfig.switchSettings()->l2AgeTimerSeconds() = newSeconds; + + session.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + + return fmt::format( + "Successfully set MAC aging time to {} seconds", newSeconds); +} + +void CmdConfigMacAgingTime::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void +CmdHandler::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.h b/fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.h new file mode 100644 index 0000000000000..4515477e95249 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.h @@ -0,0 +1,51 @@ +/* + * 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/mac/CmdConfigMac.h" + +namespace facebook::fboss { + +class MacAgingTimeArg : public utils::BaseObjectArgType { + public: + /* implicit */ MacAgingTimeArg( // NOLINT(google-explicit-constructor) + std::vector v); + + int32_t getSeconds() const { + return seconds_; + } + + private: + int32_t seconds_{300}; +}; + +struct CmdConfigMacAgingTimeTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigMac; + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option("seconds", args, "MAC aging time in seconds (1-1000000)"); + } + using ObjectArgType = MacAgingTimeArg; + using RetType = std::string; +}; + +class CmdConfigMacAgingTime + : public CmdHandler { + public: + using ObjectArgType = CmdConfigMacAgingTimeTraits::ObjectArgType; + using RetType = CmdConfigMacAgingTimeTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& agingTime); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/BUCK b/fboss/cli/fboss2/test/config/BUCK index b14d20bcf0eca..ce0bb14cd1bcd 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -11,6 +11,7 @@ cpp_unittest( "CmdConfigInterfaceSwitchportAccessVlanTest.cpp", "CmdConfigInterfaceTest.cpp", "CmdConfigL2LearningModeTest.cpp", + "CmdConfigMacAgingTimeTest.cpp", "CmdConfigQosBufferPoolTest.cpp", "CmdConfigReloadTest.cpp", "CmdConfigSessionCommitTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigMacAgingTimeTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigMacAgingTimeTest.cpp new file mode 100644 index 0000000000000..587a733ceb98a --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigMacAgingTimeTest.cpp @@ -0,0 +1,133 @@ +/* + * 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/mac/aging_time/CmdConfigMacAgingTime.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" +#include "fboss/cli/fboss2/utils/PortMap.h" // NOLINT(misc-include-cleaner) + +using namespace ::testing; + +namespace facebook::fboss { + +// Seed mirrors the FBOSS default: switchSettings.l2AgeTimerSeconds = 300. +// The agent initialises this field to 300s when no explicit value is present +// in the running config. +class CmdConfigMacAgingTimeTestFixture : public CmdConfigTestBase { + public: + CmdConfigMacAgingTimeTestFixture() + : CmdConfigTestBase( + "fboss_mac_aging_test_%%%%-%%%%-%%%%-%%%%", + R"({ + "sw": { + "switchSettings": { + "l2AgeTimerSeconds": 300 + } + } +})") {} + + protected: + const std::string cmdPrefix_ = "config mac aging-time"; +}; + +// ============================================================================== +// MacAgingTimeArg validation tests +// ============================================================================== + +TEST_F(CmdConfigMacAgingTimeTestFixture, macAgingTimeArg_validSeconds) { + // Any strictly positive integer must parse without error and be accessible + // via getSeconds(). The boundary values 1 and a large number are included + // to confirm there is no hidden upper-bound clamp. + EXPECT_EQ(MacAgingTimeArg({"1"}).getSeconds(), 1); + EXPECT_EQ(MacAgingTimeArg({"300"}).getSeconds(), 300); + EXPECT_EQ(MacAgingTimeArg({"1000000"}).getSeconds(), 1000000); +} + +TEST_F(CmdConfigMacAgingTimeTestFixture, macAgingTimeArg_noArgs) { + // Passing an empty token list must throw because the required positional + // argument is missing. + EXPECT_THROW(MacAgingTimeArg({}), std::invalid_argument); +} + +TEST_F(CmdConfigMacAgingTimeTestFixture, macAgingTimeArg_tooManyArgs) { + // The argument accepts exactly one token; a second token must be rejected. + EXPECT_THROW(MacAgingTimeArg({"300", "600"}), std::invalid_argument); +} + +TEST_F(CmdConfigMacAgingTimeTestFixture, macAgingTimeArg_notAnInteger) { + // Non-numeric strings and floating-point notation must be rejected — the + // argument requires a whole-number count of seconds. + EXPECT_THROW(MacAgingTimeArg({"abc"}), std::invalid_argument); + EXPECT_THROW(MacAgingTimeArg({"30.5"}), std::invalid_argument); +} + +TEST_F(CmdConfigMacAgingTimeTestFixture, macAgingTimeArg_zeroOrNegative) { + // Zero and negative values must be rejected: a MAC aging timer of ≤ 0 + // seconds has no meaningful interpretation for the agent. + EXPECT_THROW(MacAgingTimeArg({"0"}), std::invalid_argument); + EXPECT_THROW(MacAgingTimeArg({"-1"}), std::invalid_argument); + EXPECT_THROW(MacAgingTimeArg({"-300"}), std::invalid_argument); +} + +// ============================================================================== +// queryClient() tests +// ============================================================================== + +TEST_F(CmdConfigMacAgingTimeTestFixture, queryClient_setsNewValue) { + // Changing l2AgeTimerSeconds from the seed value (300) to 600 must update + // the in-session config and return a confirmation string that contains the + // new value. + setupTestableConfigSession(cmdPrefix_, "600"); + CmdConfigMacAgingTime cmd; + HostInfo hostInfo("testhost"); + MacAgingTimeArg arg({"600"}); + + auto result = cmd.queryClient(hostInfo, arg); + EXPECT_THAT(result, HasSubstr("600")); + + auto& config = ConfigSession::getInstance().getAgentConfig(); + EXPECT_EQ(*config.sw()->switchSettings()->l2AgeTimerSeconds(), 600); +} + +TEST_F(CmdConfigMacAgingTimeTestFixture, queryClient_setsMinimumValue) { + // Setting aging-time to 1 (the minimum accepted value) must succeed and + // write the value through to switchSettings.l2AgeTimerSeconds. + setupTestableConfigSession(cmdPrefix_, "1"); + CmdConfigMacAgingTime cmd; + HostInfo hostInfo("testhost"); + MacAgingTimeArg arg({"1"}); + + auto result = cmd.queryClient(hostInfo, arg); + EXPECT_THAT(result, HasSubstr("1")); + + auto& config = ConfigSession::getInstance().getAgentConfig(); + EXPECT_EQ(*config.sw()->switchSettings()->l2AgeTimerSeconds(), 1); +} + +TEST_F(CmdConfigMacAgingTimeTestFixture, queryClient_noOpWhenAlreadySet) { + // Re-applying the current value (300 in the seed) must be detected as a + // no-op: the command should return a message containing "already" rather + // than writing a redundant change to the session. + setupTestableConfigSession(cmdPrefix_, "300"); + CmdConfigMacAgingTime cmd; + HostInfo hostInfo("testhost"); + MacAgingTimeArg arg({"300"}); + + auto result = cmd.queryClient(hostInfo, arg); + EXPECT_THAT(result, HasSubstr("already")); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 4c3731cddc0a4..f52f61710ebfd 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -21,6 +21,7 @@ cpp_binary( "ConfigInterfaceMtuTest.cpp", "ConfigInterfaceProfileTest.cpp", "ConfigL2LearningModeTest.cpp", + "ConfigMacTest.cpp", "ConfigPfcTest.cpp", "ConfigPortQueueConfigTest.cpp", "ConfigQosPolicyMapTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigMacTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigMacTest.cpp new file mode 100644 index 0000000000000..21941acf76c2b --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigMacTest.cpp @@ -0,0 +1,97 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for: + * fboss2-dev config mac aging-time + * + * Reads the current l2AgeTimerSeconds, applies CLI changes, verifies that + * each new value round-trips through the agent's running config, then + * restores the original. All commits are HITLESS (no agent restart between + * steps), unlike l2 learning-mode changes which require a coldboot. + */ + +#include +#include +#include +#include +#include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h" + +using namespace facebook::fboss; + +namespace { +constexpr int kDefaultAgingSeconds = 300; +} // namespace + +class ConfigMacTest : public Fboss2IntegrationTest { + protected: + // Read l2AgeTimerSeconds from the agent's running config. Returns + // kDefaultAgingSeconds when the field is absent (agent default). + int getAgingTime() const { + auto config = getRunningConfig(); + const auto& sw = config["sw"]; + if (!sw.count("switchSettings")) { + return kDefaultAgingSeconds; + } + const auto& settings = sw["switchSettings"]; + if (!settings.count("l2AgeTimerSeconds")) { + return kDefaultAgingSeconds; + } + return settings["l2AgeTimerSeconds"].asInt(); + } + + // Issue the CLI command and commit the session. No waitForAgentReady() + // is needed because aging-time changes are applied hitlessly by the SAI + // layer (SaiSwitch::setMacAgingSeconds, no ChangeProhibited guard). + void setAgingTime(int seconds) { + auto result = + runCli({"config", "mac", "aging-time", std::to_string(seconds)}); + ASSERT_EQ(result.exitCode, 0) << "aging-time CLI failed: " << result.stderr; + commitConfig(); + } +}; + +// Verify that aging-time can be updated and that the new value round-trips +// through the agent. A different target is chosen depending on the current +// value so the test is always exercising a real write. +TEST_F(ConfigMacTest, SetAndRestoreAgingTime) { + XLOG(INFO) << "[Step 1] Reading current MAC aging time..."; + int originalSeconds = getAgingTime(); + XLOG(INFO) << " Current: " << originalSeconds << "s"; + + // Pick a target distinct from the current value. + int newSeconds = (originalSeconds == 600) ? 300 : 600; + + XLOG(INFO) << "[Step 2] Setting aging-time to " << newSeconds << "s..."; + setAgingTime(newSeconds); + EXPECT_EQ(getAgingTime(), newSeconds); + + XLOG(INFO) << "[Step 3] Restoring original aging-time " << originalSeconds + << "s..."; + setAgingTime(originalSeconds); + EXPECT_EQ(getAgingTime(), originalSeconds); + + XLOG(INFO) << "TEST PASSED"; +} + +// Verify the minimum accepted value (1 second) is written and visible in +// the running config, then restore the original value. +TEST_F(ConfigMacTest, SetMinimumAgingTime) { + XLOG(INFO) << "[Step 1] Reading current MAC aging time..."; + int originalSeconds = getAgingTime(); + XLOG(INFO) << " Current: " << originalSeconds << "s"; + + if (originalSeconds == 1) { + XLOG(INFO) << " Already at minimum — skipping set step."; + } else { + XLOG(INFO) << "[Step 2] Setting aging-time to minimum (1s)..."; + setAgingTime(1); + EXPECT_EQ(getAgingTime(), 1); + + XLOG(INFO) << "[Step 3] Restoring original aging-time " << originalSeconds + << "s..."; + setAgingTime(originalSeconds); + EXPECT_EQ(getAgingTime(), originalSeconds); + } + + XLOG(INFO) << "TEST PASSED"; +}