[Nexthop][fboss2-dev] Add 'config vlan <id> name <value>' CLI command#1324
Open
benoit-nexthop wants to merge 1 commit into
Open
[Nexthop][fboss2-dev] Add 'config vlan <id> name <value>' CLI command#1324benoit-nexthop wants to merge 1 commit into
benoit-nexthop wants to merge 1 commit into
Conversation
Adds a new `config vlan <id> name <value>` command to configure
the name attribute of a VLAN. The implementation follows the
`CmdConfigInterface` pattern: a single handler (`CmdConfigVlanConfig`)
dispatches on attribute name, so adding future attributes (e.g.
`routable`) requires only a new subcommand entry in `CmdListConfig.cpp`
and a new `else if` branch in `queryClient` — no new handler class.
If the specified VLAN does not already exist, the command auto-creates
it (via `VlanManager::getOrCreateVlan`) and prints "Created VLAN <id>"
before configuring the attribute — consistent with all other
`config vlan` subcommands.
The auto-create test uses VLAN 3100 (not 3998/3999) because
`getTableIdForNpu` maps IDs in [3000, 3152] to Linux routing table IDs
in [101, 253]. Values above 3152 overflow the 253-table limit and crash
`TunManager` with `Check failed: tableId <= 253`.
```
Note: Google Test filter = *VlanConfig*
[==========] Running 9 tests from 1 test suite.
[----------] 9 tests from CmdConfigVlanConfigTestFixture
[ RUN ] CmdConfigVlanConfigTestFixture.setNameOnExistingVlan
[ OK ] CmdConfigVlanConfigTestFixture.setNameOnExistingVlan (171 ms)
[ RUN ] CmdConfigVlanConfigTestFixture.setNameUpdatesSwConfig
[ OK ] CmdConfigVlanConfigTestFixture.setNameUpdatesSwConfig (308 ms)
[ RUN ] CmdConfigVlanConfigTestFixture.setNameOnSecondVlan
[ OK ] CmdConfigVlanConfigTestFixture.setNameOnSecondVlan (195 ms)
[ RUN ] CmdConfigVlanConfigTestFixture.setNameOnNonExistentVlanAutoCreates
[ OK ] CmdConfigVlanConfigTestFixture.setNameOnNonExistentVlanAutoCreates (105 ms)
[ RUN ] CmdConfigVlanConfigTestFixture.setNameDoesNotAffectOtherVlans
[ OK ] CmdConfigVlanConfigTestFixture.setNameDoesNotAffectOtherVlans (99 ms)
[ RUN ] CmdConfigVlanConfigTestFixture.vlanNameArgGetAttrName
[ OK ] CmdConfigVlanConfigTestFixture.vlanNameArgGetAttrName (29 ms)
[ RUN ] CmdConfigVlanConfigTestFixture.vlanNameArgGetValue
[ OK ] CmdConfigVlanConfigTestFixture.vlanNameArgGetValue (39 ms)
[ RUN ] CmdConfigVlanConfigTestFixture.vlanNameArgRejectsMissingValue
[ OK ] CmdConfigVlanConfigTestFixture.vlanNameArgRejectsMissingValue (33 ms)
[ RUN ] CmdConfigVlanConfigTestFixture.vlanNameArgRejectsTooManyArgs
[ OK ] CmdConfigVlanConfigTestFixture.vlanNameArgRejectsTooManyArgs (36 ms)
[----------] 9 tests from CmdConfigVlanConfigTestFixture (1020 ms total)
[==========] 9 tests from 1 test suite ran. (1020 ms total)
[ PASSED ] 9 tests.
```
```
Note: Google Test filter = ConfigVlanName*
[==========] Running 2 tests from 1 test suite.
[----------] 2 tests from ConfigVlanNameTest
[ RUN ] ConfigVlanNameTest.SetNameOnExistingVlan
I0325 14:00:25 [Step 1] Finding an eth interface...
I0325 14:00:25 Using interface eth1/1/1 (VLAN 2122)
I0325 14:00:25 [Step 2] Reading current VLAN 2122 name...
I0325 14:00:25 Current name: vlan2122
I0325 14:00:25 [Step 3] Setting VLAN 2122 name to "TestVlanName"...
I0325 14:00:25 Output: {"localhost":"Successfully configured VLAN 2122: name=\"TestVlanName\""}
I0325 14:00:25 [Step 4] Committing...
I0325 14:00:25 Committed.
I0325 14:00:25 [Step 5] Restoring original name "vlan2122"...
I0325 14:00:25 Restored. TEST PASSED
[ OK ] ConfigVlanNameTest.SetNameOnExistingVlan (375 ms)
[ RUN ] ConfigVlanNameTest.AutoCreateVlanAndSetName
I0325 14:00:25 [Step 0] Ensuring VLAN 3100 is absent from running config...
I0325 14:00:25 [Cleanup] VLAN 3100 not present — no cleanup needed.
I0325 14:00:25 [Step 1] Setting name on non-existent VLAN 3100...
I0325 14:00:25 Output: Created VLAN 3100
I0325 14:00:25 {"localhost":"Successfully configured VLAN 3100: name=\"TestAutoCreateVlan\""}
I0325 14:00:25 [Step 2] Committing session...
I0325 14:00:25 Config committed.
I0325 14:00:25 [Step 3] Cleaning up VLAN 3100...
I0325 14:00:25 Cleanup complete. TEST PASSED
[ OK ] ConfigVlanNameTest.AutoCreateVlanAndSetName (262 ms)
[----------] 2 tests from ConfigVlanNameTest (637 ms total)
[==========] 2 tests from 1 test suite ran. (638 ms total)
[ PASSED ] 2 tests.
```
```
fboss2-dev config vlan 2122 name myvlan
fboss2-dev config session commit
fboss2-dev config vlan 3100 name newvlan
fboss2-dev config session commit
```
benoit-nexthop
force-pushed
the
vlan-config-name
branch
from
0bfa7f9 to
ceec311
Compare
joseph5wu
requested changes
Jun 30, 2026
| case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE: | ||
| case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_REVISION_LIST: | ||
| case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_VLAN_ID: | ||
| default: |
Contributor
There was a problem hiding this comment.
Please don't use default cause this will stop the code to exposing missed utils::ObjectArgTypeId enum
| // A VLAN ID used to test auto-create; cleaned up before each run | ||
| static constexpr int kNewVlanId = 3999; | ||
| // A VLAN ID used to test auto-create; cleaned up before each run. | ||
| // Must be in range 3000-3152 so that getTableIdForNpu() maps it to a |
Contributor
There was a problem hiding this comment.
Why do we need to limit the vlan based on the table id for TunManager?
https://github.com/facebook/fboss/blob/main/fboss/agent/TunManager.cpp#L448
We do have special rules for assigning vlans for our data center mainly due to how our DNE wants to design the interfaces(uplinks and downlinks).
I don't think we should limit vlan ranges since agent code doesn't do that either:
https://github.com/facebook/fboss/blob/main/fboss/agent/ApplyThriftConfig.cpp#L3534
So maybe just stick to standard 1–4094?
| @@ -0,0 +1,91 @@ | |||
| /* | |||
Contributor
There was a problem hiding this comment.
Why do we need a new CmdConfigVlanConfig.h for vlan name?
Other object just stacks the direct attributes in their own CmdConfigXXX.h,
like: https://github.com/facebook/fboss/blob/main/fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h#L44
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.
Summary
Adds a new
config vlan <id> name <value>command to configurethe name attribute of a VLAN. The implementation follows the
CmdConfigInterfacepattern: a single handler (CmdConfigVlanConfig)dispatches on attribute name, so adding future attributes (e.g.
routable) requires only a new subcommand entry inCmdListConfig.cppand a new
else ifbranch inqueryClient— no new handler class.If the specified VLAN does not already exist, the command auto-creates
it (via
VlanManager::getOrCreateVlan) and prints "Created VLAN "before configuring the attribute — consistent with all other
config vlansubcommands.The auto-create test uses VLAN 3100 (not 3998/3999) because
getTableIdForNpumaps IDs in [3000, 3152] to Linux routing table IDsin [101, 253]. Values above 3152 overflow the 253-table limit and crash
TunManagerwithCheck failed: tableId <= 253.Test Plan
New unit tests
New end-to-end integration tests
Sample usage