[Nexthop][fboss2-dev] Add interface loopback-mode config command#1096
Closed
hillol-nexthop wants to merge 5 commits into
Closed
[Nexthop][fboss2-dev] Add interface loopback-mode config command#1096hillol-nexthop wants to merge 5 commits into
hillol-nexthop wants to merge 5 commits into
Conversation
2 tasks
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
2 times, most recently
from
a5eec2c to
02393bc
Compare
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
5 times, most recently
from
083c86f to
fad4539
Compare
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
6 times, most recently
from
fb593d4 to
fb719bf
Compare
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
fb719bf to
6a4216c
Compare
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
6a4216c to
533d229
Compare
Contributor
|
@joseph5wu has imported this pull request. If you are a Meta employee, you can view this in D108790790. |
joseph5wu
requested changes
Jun 16, 2026
joseph5wu
left a comment
joseph5wu
left a comment
Contributor
There was a problem hiding this comment.
Overall it looks good to me.
Please address my comments
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
fd91825 to
ce4e1a8
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Add two new fboss2-dev command families for managing per-interface IPv6 NDP / Router Advertisement configuration: config interface <intf> [...] ipv6 ndp <attr> [<value>] [...] delete interface <intf> [...] ipv6 ndp <attr> [...] Supported attributes: ra-interval, ra-lifetime, hop-limit, prefix-valid-lifetime, prefix-preferred-lifetime, managed-config-flag, other-config-flag, ra-address. `config` writes into cfg::Interface::ndp in the config session; `delete` resets attributes to their thrift schema defaults (and clears the optional ra-address). Both commands validate all attributes before mutating anything, reject duplicate attributes, and error out on names with no associated L3 interface. The attribute name set is shared between the two commands via ndpAttrNames() so they cannot drift. `delete interface` is introduced as a namespace-level command whose behavior lives in its subcommands; invoking it without a subcommand reports an error. Test infra: getInterfaceIdForPort() and getNdpConfig() helpers added to Fboss2IntegrationTest; integration tests restore the NDP attributes they touch in TearDown so the shared test switch stays clean on failure. Test plan: - Built fboss2-dev, cmd_config_test, and fboss2_integration_test - cmd_config_test unit tests: all 54 NDP tests pass (CmdConfigInterfaceIpv6Ndp* + CmdDeleteInterfaceIpv6Ndp*) - Integration: ConfigInterfaceIpv6NdpTest.* + DeleteInterfaceIpv6NdpTest.* (10 tests) pass against a live agent on an NH-4010 switch
Extend the fboss2-dev `config interface` and `delete interface`
commands with per-port attributes:
config interface <port-list> loopback-mode <none|PHY|NIF|MAC>
config interface <port-list> flow-control-rx|flow-control-tx
<enable|disable>
config interface <port-list> lldp-expected-<value|chassis|ttl|
port-desc|system-name|system-desc> <value>
config interface <port-list> type routed-port
config interface <port-list> shutdown | no-shutdown
delete interface <port-list> loopback-mode (reset to NONE)
delete interface <port-list> lldp-expected-* (remove the LLDP entry)
shutdown / no-shutdown are valueless attribute tokens (they consume no
value), implemented as a kValuelessAttributes set in the token parser.
`type routed-port` converts a port to a routed L3 port (portType
INTERFACE_PORT, routable, ingressVlan 0, vlanPorts membership removed).
Shared infrastructure (keeps config and delete from drifting apart):
- InterfaceAttrUtils.h holds the single source of truth for the
lldp-expected-* name -> cfg::LLDPTag mapping (lldpAttrToTag()); both
commands derive their attribute-name sets and valid-attribute help
strings from it.
- InterfaceAttrArgsBase holds the common <port-list> [<attr> [<value>]]
token parsing and accessors; InterfacesConfig and
InterfaceDeleteConfig derive from it and only supply their
attribute predicates and extra validation.
- Both commands track a `changed` flag and skip the config
write/commit cycle when no port or interface was actually modified
(an LLDP delete only counts as a change when an entry was erased,
loopback-mode delete only when the mode was not already NONE).
- delete interface reports only the interfaces actually reset and
calls out interfaces with no backing port instead of claiming
success for them.
Integration tests restore the attributes they touch in TearDown (via
`config ... disable` or `delete interface ...`) so the shared test
switch is left clean even when a test fails midway, following the
ConfigInterfaceIpv6NdpTest touched_/TearDown pattern. Four tests are
DISABLED because committing loopback-mode=PHY crashes the hw_agent on
NH-4010 + Broadcom SAI (SaiPortManager::changePortImpl does not catch
the SaiApiError); re-enable once the agent contains that error on the
change-port path.
Test plan:
- Built fboss2-dev, cmd_config_test, and fboss2_integration_test
- cmd_config_test unit tests pass (CmdConfigInterfaceTest +
CmdDeleteInterfaceTest cover parsing, validation, apply/reset and
idempotency for all new attributes)
- Integration: ConfigInterfaceFlowControlTest,
ConfigInterfaceLldpExpectedValueTest, ConfigInterfaceLoopbackModeTest,
ConfigInterfaceTypeTest, DeleteInterfaceTest pass against a live
agent on an NH-4010 switch
Summary: **Pre-submission checklist** - [x] I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running `pip install -r requirements-dev.txt && pre-commit install` - [x] `pre-commit run` Based on facebook#1072 Adds two new fboss2-dev CLI commands for configuring and deleting IPv6 Neighbor Discovery (NDP) settings on interfaces: - `config interface <intf> ipv6 ndp <attr> [<value>] ...` — set NDP attributes - `delete interface <intf> ipv6 ndp <attr> ...` — reset NDP attributes to defaults **Supported attributes:** `ra-interval`, `ra-lifetime`, `hop-limit`, `prefix-valid-lifetime`, `prefix-preferred-lifetime`, `managed-config-flag`, `other-config-flag`, `ra-address` - Added `CmdConfigInterfaceIpv6` and `CmdConfigInterfaceIpv6Ndp` under `commands/config/interface/ipv6/` to handle NDP configuration via `cfg::Interface::ndp()`. - Added `CmdDeleteInterface`, `CmdDeleteInterfaceIpv6`, and `CmdDeleteInterfaceIpv6Ndp` under `commands/delete/interface/` to reset NDP attributes to their defaults. - `NdpConfigAttrs` parses multi-attribute token lists; `NdpDeleteAttrs` validates a list of attribute names to reset. - Both commands use `ConfigSession` for config persistence. No Thrift RPCs are made — NDP is a pure config operation. - Registered both command trees in `CmdListConfig.cpp` and added `OBJECT_ARG_TYPE_ID_NDP_CONFIG_ATTRS` / `OBJECT_ARG_TYPE_ID_NDP_DELETE_ATTRS` to `CmdUtilsCommon.h`. - Updated both `BUCK` and `cmake/CliFboss2.cmake` build files. Pull Request resolved: facebook#1095 Test Plan: **Unit tests:** `test/config/CmdConfigInterfaceIpv6NdpTest.cpp`, `test/config/CmdDeleteInterfaceIpv6NdpTest.cpp` - Valid attribute set/reset per attribute - Invalid value format → `std::invalid_argument` - Out-of-range hop-limit (0-255) - Multi-port update - Multiple attributes in one command - Idempotent delete (no NDP config on interface) - Case-insensitive attribute names **Integration tests:** `test/integration_test/ConfigInterfaceIpv6NdpTest.cpp`, `test/integration_test/DeleteInterfaceIpv6NdpTest.cpp` ``` /tmp/fboss2_integration_test --gtest_filter=ConfigInterfaceIpv6Ndp*:DeleteInterfaceIpv6Ndp* ``` Reviewed By: srikrishnagopu Differential Revision: D108637984 Pulled By: joseph5wu fbshipit-source-id: 60c0cc8335e9707a626773b5a7d53587ea60e07b
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
caea1f0 to
5002a10
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
5002a10 to
41edde4
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
41edde4 to
8587a0c
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
8587a0c to
19ca214
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Differential Revision: D108702250 fbshipit-source-id: bf8d99eab63bde39ecc624638f70ce2e80d3c83f
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
19ca214 to
0bba0bb
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
aacb1f0 to
247cfe7
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
247cfe7 to
dc72ccd
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
…rg parsing Introduce utils::MultiArgsConfigType as a command-agnostic base for CLI argument types of the form '<object-names> [<attr> [<value>] ...]'. It owns the token-splitting / attribute-value parsing (parseTokens) and the attribute accessors, with the object/attribute specifics supplied through virtual hooks (isKnownAttribute, isValuelessAttribute, objectKind, attrKind, validAttrs) instead of std::function callbacks threaded through every parseTokens call. InterfaceAttrArgsBase now derives from MultiArgsConfigType and only adds the interface-specific InterfaceList resolution (objectKind() -> 'interface'). InterfacesConfig and InterfaceDeleteConfig override the attribute hooks rather than defining static predicates passed into parseTokens. This removes the per-call std::function plumbing and makes the multi-arg parsing reusable by future 'config/delete <object> ...' commands, per PR review feedback (joseph5wu). No behavior change: parseTokens emits the same error messages (objectKind parameterizes the object noun). Verified by the fboss2 interface config/delete integration tests on gold218 (19/19 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hillol-nexthop
force-pushed
the
interface-loopback-mode
branch
from
dc72ccd to
b5a8d87
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Contributor
|
@joseph5wu merged this pull request in 5454028. |
meta-codesync Bot
pushed a commit
that referenced
this pull request
Jun 25, 2026
Summary: Based on #1096 **Pre-submission checklist** - [x] I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running `pip install -r requirements-dev.txt && pre-commit install` - [x] `pre-commit run` This PR implements trunk VLAN management functionality for the FBOSS CLI, allowing users to add and remove VLANs from trunk ports. ``` config interface <port-list> switchport trunk allowed vlan add <vlan-id-list> config interface <port-list> switchport trunk allowed vlan remove <vlan-id-list> ``` Examples: ``` # Add VLANs 100, 200, 300 to trunk port eth1/1/1 fboss2-dev config interface eth1/1/1 switchport trunk allowed vlan add 100,200,300 # Add VLAN 100 to multiple ports fboss2-dev config interface eth1/1/1-eth1/1/4 switchport trunk allowed vlan add 100 # Remove VLAN 200 from trunk port fboss2-dev config interface eth1/1/1 switchport trunk allowed vlan remove 200 ``` **Implementation Details** Adding VLANs: Creates VlanPort entries with emitTags=true (tagged traffic) for each port-VLAN combination Removing VLANs: Removes the corresponding VlanPort entries from the configuration Validation: VLANs must exist in the configuration with an associated interface (SVI) Ports must exist in the hardware platform mapping VLAN IDs must be in the valid range (1-4094) Idempotency: Adding an existing VLAN or removing a non-existent VLAN reports "No changes made" without error **Files Changed** fboss/cli/fboss2/commands/config/interface/switchport/trunk/ - New command hierarchy fboss/cli/fboss2/utils/CmdUtils.h/cpp - Added TrunkVlanAction argument type fboss/cli/fboss2/CmdList.cpp - Registered new commands fboss/cli/fboss2/CmdArgsLists.h - Added argument type mapping Pull Request resolved: #1097 Test Plan: AddAndRemoveTrunkVlan AddMultipleVlans AddExistingVlan RemoveVlansOneByOne RemoveNonExistingVlan AddVlanToMultiplePorts RemoveVlanFromMultiplePorts AddMultipleVlansToMultiplePorts Reviewed By: srikrishnagopu Differential Revision: D109641943 Pulled By: joseph5wu fbshipit-source-id: fd17343967b99269e20ce87738c628ee28d7ab4e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on #1095
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
1. New
config interfaceattributes — gives operators direct CLI control over port properties previously only configurable by editing switch config JSON.New config command syntax:
2. Consolidated
delete interfacecommands — replaces separate per-attribute subcommand classes (CmdDeleteInterfaceLoopbackMode,CmdDeleteInterfaceLldpExpectedValue) with a singleCmdDeleteInterfacehandler, mirroring theCmdConfigInterfacepattern.New delete command syntax:
Implementation
All attributes map to existing fields on
cfg::Port— no thrift schema changes.| Attribute | Thrift field | Notes |
|--------|-----------|-------------|-------|
loopback-mode|cfg::Port.loopbackMode(field 17) | NONE/PHY/NIF/MAC, case-insensitive |flow-control-rx|cfg::Port.pause.rx(field 13) | enable/disable |flow-control-tx|cfg::Port.pause.tx(field 13) | enable/disable |lldp-expected-*|cfg::Port.expectedLLDPValues(field 20) | 6 tag variants |type routed-port|cfg::Port.portType(field 27) | also sets routable=true, clears ingressVlan |shutdown/no-shutdown|cfg::Port.state(field 2) | valueless flags |InterfaceDeleteAttrsarg type parses<ports> <attr> [<attr>...]— same pattern asInterfacesConfigbut all attributes are valueless (reset-to-default semantics)operator const InterfaceList&()implicit conversion preserves theipv6 ndsubcommand chainlldp-expected-*tag variants (onlylldp-expected-valuewas supported before)Test plan
fboss2-dev config interface eth1/1/1 loopback-mode PHY— sets loopback modefboss2-dev delete interface eth1/1/1 loopback-mode— resets to NONEfboss2-dev delete interface eth1/1/1 loopback-mode lldp-expected-chassis— resets both in one callfboss2-dev config interface eth1/1/1 shutdown/no-shutdown— disables/enables portfboss2-dev delete interface eth1/1/1 ipv6 nd ra-interval— ipv6 nd subcommand still worksCmdDeleteInterfaceTest(7 cases),CmdConfigInterfaceLoopbackModeTest, etc.