Skip to content

[Nexthop][fboss2-dev] Add delete interface description and mtu #1495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vybhav-nexthop wants to merge 1 commit into
base: main
Choose a base branch
from
+307 −14
Open

[Nexthop][fboss2-dev] Add delete interface description and mtu #1495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 39 additions & 5 deletions fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ namespace {
// config and delete commands cannot drift apart.
const std::unordered_set<std::string> kValuelessDeleteAttributes = [] {
std::unordered_set<std::string> attrs = {
"loopback-mode", "lookup-class", "queue-config"};
"description", "loopback-mode", "lookup-class", "mtu", "queue-config"};
for (const auto& name : lldpAttrNames()) {
attrs.insert(name);
}
Expand All @@ -53,7 +53,7 @@ const std::unordered_set<std::string> 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
Expand Down Expand Up @@ -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<std::string> resetNames;
std::vector<std::string> 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<std::string> resetNames;
std::vector<std::string> skippedNames;
for (const utils::Intf& intf : interfaces) {
Expand All @@ -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;
Expand Down
11 changes: 7 additions & 4 deletions fboss/cli/fboss2/commands/delete/interface/CmdDeleteInterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@ namespace facebook::fboss {
* Usage: delete interface <port-list> [<attr> [<value>] ...]
*
* 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 <cidr> — remove an IPv4 address (e.g. 10.0.0.1/24)
Expand All @@ -45,7 +48,7 @@ struct CmdDeleteInterfaceTraits : public WriteCommandTraits {
cmd.add_option(
"interface_delete_config",
args,
"<port-list> [loopback-mode|lookup-class|lldp-expected-*|ip-address <cidr>|ipv6-address <cidr>]");
"<port-list> [description|loopback-mode|lookup-class|mtu|lldp-expected-*|ip-address <cidr>|ipv6-address <cidr>]");
}
using ObjectArgType = InterfaceDeleteConfig;
using RetType = std::string;
Expand Down
107 changes: 106 additions & 1 deletion fboss/cli/fboss2/test/config/CmdDeleteInterfaceTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class CmdDeleteInterfaceTestFixture : public CmdConfigTestBase {
"speed": 100000,
"loopbackMode": 1,
"lookupClasses": [10, 11],
"description": "to-spine1",
"expectedLLDPValues": {
"2": "ge-0/0/0"
}
Expand All @@ -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"
}
]
}
})") {}

Expand Down Expand Up @@ -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
// ---------------------------------------------------------------------------
Expand Down
Loading
Loading