From 7d92f8ae31d0246bd355a559a5c7cc918f7c1683 Mon Sep 17 00:00:00 2001 From: Vybhav Date: Tue, 4 Aug 2026 11:38:36 +0000 Subject: [PATCH] Add delete interface description and mtu Fill in the `delete interface` gaps where `config interface` can set an attribute but nothing could unset it. delete interface description Valueless reset of the optional sw.ports[*].description. delete interface mtu Valueless reset of the optional sw.interfaces[*].mtu. The agent falls back to Interface::kDefaultMtu when unset, so this is an interface-level reset rather than a port-level one. Both save with ConfigActionLevel::HITLESS, matching the config-side setters; SaiSwitch has no prohibited-change guard for these fields. `queue-config` is deliberately not included here: `delete interface queue-config` already clears portQueueConfigName upstream. --- .../delete/interface/CmdDeleteInterface.cpp | 44 ++++- .../delete/interface/CmdDeleteInterface.h | 11 +- .../test/config/CmdDeleteInterfaceTest.cpp | 107 +++++++++++- .../integration_test/DeleteInterfaceTest.cpp | 159 +++++++++++++++++- 4 files changed, 307 insertions(+), 14 deletions(-) diff --git a/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp b/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp index 6e2e1d7391637..f19f056a67c67 100644 --- a/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp +++ b/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp @@ -37,7 +37,7 @@ namespace { // config and delete commands cannot drift apart. const std::unordered_set kValuelessDeleteAttributes = [] { std::unordered_set attrs = { - "loopback-mode", "lookup-class", "queue-config"}; + "description", "loopback-mode", "lookup-class", "mtu", "queue-config"}; for (const auto& name : lldpAttrNames()) { attrs.insert(name); } @@ -53,7 +53,7 @@ const std::unordered_set kKnownDeleteAttributes = [] { }(); const std::string kValidDeleteAttrs = fmt::format( - "loopback-mode, lookup-class, queue-config, {}, ip-address, ipv6-address", + "description, loopback-mode, lookup-class, mtu, queue-config, {}, ip-address, ipv6-address", folly::join(", ", lldpAttrNames())); } // namespace @@ -177,9 +177,38 @@ CmdDeleteInterfaceTraits::RetType CmdDeleteInterface::queryClient( "No interface config found for: {}", folly::join(", ", missingNames))); } + } else if (attr == "mtu") { + // Interface-level reset: mtu is an optional field, and the agent falls + // back to Interface::kDefaultMtu when it is unset. + std::vector resetNames; + std::vector missingNames; + for (const utils::Intf& intf : interfaces) { + cfg::Interface* iface = intf.getInterface(); + if (!iface) { + missingNames.push_back(intf.name()); + continue; + } + if (iface->mtu().has_value()) { + iface->mtu().reset(); + changed = true; + } + resetNames.push_back(intf.name()); + } + if (!resetNames.empty()) { + results.push_back( + fmt::format( + "Successfully reset attribute 'mtu' for interface(s): {}", + folly::join(", ", resetNames))); + } + if (!missingNames.empty()) { + results.push_back( + fmt::format( + "No interface config found for: {}", + folly::join(", ", missingNames))); + } } else { - // Port-level valueless reset (loopback-mode, lookup-class, queue-config, - // lldp-expected-*). + // Port-level valueless reset (description, loopback-mode, lookup-class, + // queue-config, lldp-expected-*). std::vector resetNames; std::vector skippedNames; for (const utils::Intf& intf : interfaces) { @@ -188,7 +217,12 @@ CmdDeleteInterfaceTraits::RetType CmdDeleteInterface::queryClient( skippedNames.push_back(intf.name()); continue; } - if (attr == "loopback-mode") { + if (attr == "description") { + if (port->description().has_value()) { + port->description().reset(); + changed = true; + } + } else if (attr == "loopback-mode") { if (*port->loopbackMode() != cfg::PortLoopbackMode::NONE) { port->loopbackMode() = cfg::PortLoopbackMode::NONE; changed = true; diff --git a/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h b/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h index 9ae405f94f762..473b9ecdfe2ae 100644 --- a/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h +++ b/fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h @@ -25,9 +25,12 @@ namespace facebook::fboss { * Usage: delete interface [ [] ...] * * Valueless attributes (reset to default): - * loopback-mode, lookup-class, lldp-expected-value, lldp-expected-chassis, - * lldp-expected-ttl, lldp-expected-port-desc, lldp-expected-system-name, - * lldp-expected-system-desc + * description, loopback-mode, lookup-class, mtu, lldp-expected-value, + * lldp-expected-chassis, lldp-expected-ttl, lldp-expected-port-desc, + * lldp-expected-system-name, lldp-expected-system-desc + * + * mtu is an Interface field and unsets to the agent's default (1500 via + * Interface::kDefaultMtu); the rest are Port fields. * * Valued attributes (remove specific entry): * ip-address — remove an IPv4 address (e.g. 10.0.0.1/24) @@ -45,7 +48,7 @@ struct CmdDeleteInterfaceTraits : public WriteCommandTraits { cmd.add_option( "interface_delete_config", args, - " [loopback-mode|lookup-class|lldp-expected-*|ip-address |ipv6-address ]"); + " [description|loopback-mode|lookup-class|mtu|lldp-expected-*|ip-address |ipv6-address ]"); } using ObjectArgType = InterfaceDeleteConfig; using RetType = std::string; diff --git a/fboss/cli/fboss2/test/config/CmdDeleteInterfaceTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteInterfaceTest.cpp index e2927d7a7ad12..a1b927ddc3f8c 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteInterfaceTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteInterfaceTest.cpp @@ -42,6 +42,7 @@ class CmdDeleteInterfaceTestFixture : public CmdConfigTestBase { "speed": 100000, "loopbackMode": 1, "lookupClasses": [10, 11], + "description": "to-spine1", "expectedLLDPValues": { "2": "ge-0/0/0" } @@ -53,7 +54,23 @@ class CmdDeleteInterfaceTestFixture : public CmdConfigTestBase { "speed": 100000 } ], - "interfaces": [] + "interfaces": [ + { + "intfID": 1, + "routerID": 0, + "vlanID": 0, + "portID": 1, + "name": "eth1/1/1", + "mtu": 9000 + }, + { + "intfID": 2, + "routerID": 0, + "vlanID": 0, + "portID": 2, + "name": "eth1/2/1" + } + ] } })") {} @@ -197,6 +214,94 @@ TEST_F(CmdDeleteInterfaceTestFixture, queryClientMultipleAttrs) { EXPECT_EQ(ports[0].expectedLLDPValues()->count(cfg::LLDPTag::PORT), 0); } +// --------------------------------------------------------------------------- +// queryClient: clears the port description +// --------------------------------------------------------------------------- + +TEST_F(CmdDeleteInterfaceTestFixture, queryClientResetsDescription) { + setupTestableConfigSession(cmdPrefix_, "eth1/1/1 description"); + auto cmd = CmdDeleteInterface(); + auto deleteAttrs = InterfaceDeleteConfig({"eth1/1/1", "description"}); + + auto& ports = *ConfigSession::getInstance().getAgentConfig().sw()->ports(); + ASSERT_EQ(*ports[0].description(), "to-spine1"); + + auto result = cmd.queryClient(localhost(), deleteAttrs); + + EXPECT_THAT(result, HasSubstr("description")); + EXPECT_THAT(result, HasSubstr("eth1/1/1")); + EXPECT_FALSE(ports[0].description().has_value()); + // Other port attributes survive. + EXPECT_EQ(ports[0].expectedLLDPValues()->count(cfg::LLDPTag::PORT), 1); +} + +TEST_F(CmdDeleteInterfaceTestFixture, queryClientIdempotentNoDescription) { + setupTestableConfigSession(cmdPrefix_, "eth1/2/1 description"); + auto cmd = CmdDeleteInterface(); + auto deleteAttrs = InterfaceDeleteConfig({"eth1/2/1", "description"}); + + auto result = cmd.queryClient(localhost(), deleteAttrs); + + EXPECT_THAT(result, HasSubstr("eth1/2/1")); + auto& ports = *ConfigSession::getInstance().getAgentConfig().sw()->ports(); + EXPECT_FALSE(ports[1].description().has_value()); +} + +// --------------------------------------------------------------------------- +// queryClient: unsets the interface MTU (agent falls back to kDefaultMtu) +// --------------------------------------------------------------------------- + +TEST_F(CmdDeleteInterfaceTestFixture, queryClientResetsMtu) { + setupTestableConfigSession(cmdPrefix_, "eth1/1/1 mtu"); + auto cmd = CmdDeleteInterface(); + auto deleteAttrs = InterfaceDeleteConfig({"eth1/1/1", "mtu"}); + + auto& ifaces = + *ConfigSession::getInstance().getAgentConfig().sw()->interfaces(); + ASSERT_EQ(*ifaces[0].mtu(), 9000); + + auto result = cmd.queryClient(localhost(), deleteAttrs); + + EXPECT_THAT(result, HasSubstr("mtu")); + EXPECT_THAT(result, HasSubstr("eth1/1/1")); + EXPECT_FALSE(ifaces[0].mtu().has_value()); + // The sibling interface is untouched. + EXPECT_FALSE(ifaces[1].mtu().has_value()); +} + +TEST_F(CmdDeleteInterfaceTestFixture, queryClientIdempotentNoMtu) { + setupTestableConfigSession(cmdPrefix_, "eth1/2/1 mtu"); + auto cmd = CmdDeleteInterface(); + auto deleteAttrs = InterfaceDeleteConfig({"eth1/2/1", "mtu"}); + + auto result = cmd.queryClient(localhost(), deleteAttrs); + + // Assert on the success wording, not just the port name: the + // "No interface config found for: eth1/2/1" failure message also contains it. + EXPECT_THAT(result, HasSubstr("Successfully reset attribute 'mtu'")); + EXPECT_THAT(result, Not(HasSubstr("No interface config found"))); + auto& ifaces = + *ConfigSession::getInstance().getAgentConfig().sw()->interfaces(); + EXPECT_FALSE(ifaces[1].mtu().has_value()); +} + +// description is a Port field and mtu an Interface field, so a combined +// invocation has to touch both objects in one pass. +TEST_F(CmdDeleteInterfaceTestFixture, queryClientDescriptionAndMtuTogether) { + setupTestableConfigSession(cmdPrefix_, "eth1/1/1 description mtu"); + auto cmd = CmdDeleteInterface(); + auto deleteAttrs = InterfaceDeleteConfig({"eth1/1/1", "description", "mtu"}); + + auto result = cmd.queryClient(localhost(), deleteAttrs); + + EXPECT_THAT(result, HasSubstr("description")); + EXPECT_THAT(result, HasSubstr("mtu")); + + auto& config = ConfigSession::getInstance().getAgentConfig(); + EXPECT_FALSE((*config.sw()->ports())[0].description().has_value()); + EXPECT_FALSE((*config.sw()->interfaces())[0].mtu().has_value()); +} + // --------------------------------------------------------------------------- // InterfaceDeleteConfig constructor: rejects unknown attribute // --------------------------------------------------------------------------- diff --git a/fboss/cli/fboss2/test/integration_test/DeleteInterfaceTest.cpp b/fboss/cli/fboss2/test/integration_test/DeleteInterfaceTest.cpp index 9e10d60f8713f..ff4482eaad3f0 100644 --- a/fboss/cli/fboss2/test/integration_test/DeleteInterfaceTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/DeleteInterfaceTest.cpp @@ -14,8 +14,10 @@ * - The test must be run as root (or with appropriate permissions) */ +#include #include #include +#include #include #include #include @@ -52,6 +54,68 @@ class DeleteInterfaceTest : public Fboss2IntegrationTest { commitConfig(); } + void setDescription( + const std::string& interfaceName, + const std::string& value) { + // Remember the prior description so TearDown can put it back. Unlike + // loopback-mode / lldp, "delete" is not the right restore: a shared DUT + // port may already have had a real description before the test overwrote + // it. + if (!priorDescription_.has_value()) { + priorDescription_ = descriptionOf(interfaceName); + descriptionPort_ = interfaceName; + } + touched_.emplace_back(interfaceName, "description"); + auto result = + runCli({"config", "interface", interfaceName, "description", value}); + ASSERT_EQ(result.exitCode, 0) + << "Failed to set description=" << value << ": " << result.stderr; + commitConfig(); + } + + // Description of a port in the running config, or empty when unset. + std::string descriptionOf(const std::string& interfaceName) const { + auto config = getRunningConfig(); + for (const auto& port : config["sw"]["ports"]) { + if (port.count("name") && port["name"].asString() == interfaceName) { + return port.count("description") ? port["description"].asString() + : std::string(); + } + } + return {}; + } + + // mtu of the L3 interface belonging to a port, or 0 when unset. + int mtuOf(const std::string& interfaceName) const { + auto intfId = getInterfaceIdForPort(interfaceName); + auto config = getRunningConfig(); + for (const auto& intf : config["sw"]["interfaces"]) { + if (intf.count("intfID") && intf["intfID"].asInt() == intfId) { + return intf.count("mtu") ? static_cast(intf["mtu"].asInt()) : 0; + } + } + return 0; + } + + // Set mtu and record the prior value so TearDown can restore it. TearDown + // cannot simply `delete interface … mtu`: DUTs usually ship with a + // non-default mtu, and delete would leave it unset. + void setMtu(const std::string& interfaceName, int mtu) { + if (!priorMtu_.has_value()) { + priorMtu_ = mtuOf(interfaceName); + mtuPort_ = interfaceName; + } + auto result = runCli( + {"config", + "interface", + interfaceName, + "mtu", + folly::to(mtu)}); + ASSERT_EQ(result.exitCode, 0) + << "Failed to set mtu=" << mtu << ": " << result.stderr; + commitConfig(); + } + void deleteInterfaceAttrs( const std::string& interfaceName, const std::vector& attrs) { @@ -66,27 +130,86 @@ class DeleteInterfaceTest : public Fboss2IntegrationTest { } void TearDown() override { + bool changed = false; if (!touched_.empty()) { - bool reset = false; for (const auto& [interfaceName, attr] : touched_) { + // description is restored below from priorDescription_; skipping the + // delete avoids wiping a pre-existing DUT description. + if (attr == "description") { + continue; + } auto result = runCli({"delete", "interface", interfaceName, attr}); if (result.exitCode != 0) { XLOG(WARN) << "TearDown failed to delete " << attr << " on " << interfaceName << ": " << result.stderr; } else { - reset = true; + changed = true; } } - if (reset) { - commitConfig(); + } + if (priorDescription_.has_value() && !descriptionPort_.empty()) { + if (priorDescription_->empty()) { + auto result = + runCli({"delete", "interface", descriptionPort_, "description"}); + if (result.exitCode != 0) { + XLOG(WARN) << "TearDown failed to clear description on " + << descriptionPort_ << ": " << result.stderr; + } else { + changed = true; + } + } else { + auto result = runCli( + {"config", + "interface", + descriptionPort_, + "description", + *priorDescription_}); + if (result.exitCode != 0) { + XLOG(WARN) << "TearDown failed to restore description on " + << descriptionPort_ << ": " << result.stderr; + } else { + changed = true; + } } } + if (priorMtu_.has_value() && !mtuPort_.empty()) { + if (*priorMtu_ == 0) { + auto result = runCli({"delete", "interface", mtuPort_, "mtu"}); + if (result.exitCode != 0) { + XLOG(WARN) << "TearDown failed to clear mtu on " << mtuPort_ << ": " + << result.stderr; + } else { + changed = true; + } + } else { + auto result = runCli( + {"config", + "interface", + mtuPort_, + "mtu", + folly::to(*priorMtu_)}); + if (result.exitCode != 0) { + XLOG(WARN) << "TearDown failed to restore mtu on " << mtuPort_ << ": " + << result.stderr; + } else { + changed = true; + } + } + } + if (changed) { + commitConfig(); + } Fboss2IntegrationTest::TearDown(); } private: // (interfaceName, attr) pairs set by this test, deleted in TearDown std::vector> touched_; + // Prior description / mtu to restore after overwrite-based tests. + std::optional priorDescription_; + std::string descriptionPort_; + std::optional priorMtu_; + std::string mtuPort_; }; // --------------------------------------------------------------------------- @@ -153,6 +276,34 @@ TEST_F(DeleteInterfaceTest, DeleteLldpExpectedValue) { XLOG(INFO) << "TEST PASSED"; } +// --------------------------------------------------------------------------- +// Test: set description and mtu, then delete both in one invocation. +// +// description lives on cfg::Port and mtu on cfg::Interface, so a single +// command has to reach into both objects; covering them together exercises +// each reset path plus the multi-attribute dispatch in one commit cycle. +// +// Prior description and mtu are captured by setDescription / setMtu and +// restored in TearDown (delete alone is wrong for mtu: DUTs usually ship +// with a non-default value). +// --------------------------------------------------------------------------- + +TEST_F(DeleteInterfaceTest, DeleteDescriptionAndMtu) { + std::string ifName = getRandomInterfacePortName(); + XLOG(INFO) << "Using interface: " << ifName; + + setDescription(ifName, "fboss2-it-description"); + setMtu(ifName, 9000); + ASSERT_EQ(descriptionOf(ifName), "fboss2-it-description"); + ASSERT_EQ(mtuOf(ifName), 9000); + + deleteInterfaceAttrs(ifName, {"description", "mtu"}); + EXPECT_EQ(descriptionOf(ifName), std::string()) + << "description should be absent from the running config after delete"; + EXPECT_EQ(mtuOf(ifName), 0) + << "mtu should be absent from the running config after delete"; +} + // --------------------------------------------------------------------------- // Test: delete lldp-expected-value is idempotent // ---------------------------------------------------------------------------