[Nexthop][fboss2-dev] Ip in Ip tunnel#1140
Open
hillol-nexthop wants to merge 3 commits intofacebook:mainfrom
Open
[Nexthop][fboss2-dev] Ip in Ip tunnel#1140hillol-nexthop wants to merge 3 commits intofacebook:mainfrom
hillol-nexthop wants to merge 3 commits intofacebook:mainfrom
Conversation
# Summary
Adds a `VlanManager` class that auto-creates a VLAN (and its required
`cfg::Interface`) in the session config whenever a `config vlan <id>`
subcommand references a VLAN that does not yet exist.
Previously, `config vlan <id> static-mac add` and
`config vlan <id> port taggingMode` would throw an error if the VLAN
was not already present in the running config, forcing operators to
create the VLAN manually first. Now these commands create the VLAN
on demand and print a one-line notice when they do:
```
Created VLAN <id>
```
The agent requires every VLAN to have a matching `cfg::Interface` entry
or it rejects the commit. `VlanManager::createVlan` therefore creates
both objects atomically:
- `cfg::Vlan` with `id`, `name = "Vlan<id>"`, `routable = false`
- `cfg::Interface` with `intfID = id`, `vlanID = id`,
`name = "fboss<id>"`, `routerID = 0`, `ipAddresses = []`
For `static-mac delete` on a non-existent VLAN the command now returns
an idempotent "does not exist" message instead of throwing, consistent
with the delete-on-non-existent pattern used elsewhere.
Also fixes a latent bug in `ConfigSession::saveConfig` where a `CHECK`
would fatally abort when run in a test context with no CLI arguments on
the process command line. The inline comment already described the
correct behaviour as "gracefully skip"; the CHECK is removed to match.
# Test Plan
Unit tests (`fboss2_cmd_config_test --gtest_filter='VlanManager*'`):
```
[==========] Running 12 tests from 1 test suite.
[----------] 12 tests from VlanManagerTestFixture
[ RUN ] VlanManagerTestFixture.createVlanNewVlan
[ OK ] VlanManagerTestFixture.createVlanNewVlan (44 ms)
[ RUN ] VlanManagerTestFixture.createVlanAlreadyExists
[ OK ] VlanManagerTestFixture.createVlanAlreadyExists (40 ms)
[ RUN ] VlanManagerTestFixture.createVlanSetsCorrectId
[ OK ] VlanManagerTestFixture.createVlanSetsCorrectId (40 ms)
[ RUN ] VlanManagerTestFixture.createVlanSetsAutoGeneratedName
[ OK ] VlanManagerTestFixture.createVlanSetsAutoGeneratedName (49 ms)
[ RUN ] VlanManagerTestFixture.createVlanSetsRoutableFalse
[ OK ] VlanManagerTestFixture.createVlanSetsRoutableFalse (44 ms)
[ RUN ] VlanManagerTestFixture.createVlanDoesNotSaveConfig
[ OK ] VlanManagerTestFixture.createVlanDoesNotSaveConfig (45 ms)
[ RUN ] VlanManagerTestFixture.createVlanAlsoCreatesInterface
[ OK ] VlanManagerTestFixture.createVlanAlsoCreatesInterface (45 ms)
[ RUN ] VlanManagerTestFixture.createVlanDoesNotDuplicateInterface
[ OK ] VlanManagerTestFixture.createVlanDoesNotDuplicateInterface (37 ms)
[ RUN ] VlanManagerTestFixture.getOrCreateVlanReturnsExistingVlan
[ OK ] VlanManagerTestFixture.getOrCreateVlanReturnsExistingVlan (41 ms)
[ RUN ] VlanManagerTestFixture.getOrCreateVlanCreatesNewVlan
Created VLAN 777
[ OK ] VlanManagerTestFixture.getOrCreateVlanCreatesNewVlan (40 ms)
[ RUN ] VlanManagerTestFixture.getOrCreateVlanPrintsMessageForNewVlan
[ OK ] VlanManagerTestFixture.getOrCreateVlanPrintsMessageForNewVlan (37 ms)
[ RUN ] VlanManagerTestFixture.getOrCreateVlanNoPrintForExistingVlan
[ OK ] VlanManagerTestFixture.getOrCreateVlanNoPrintForExistingVlan (40 ms)
[----------] 12 tests from VlanManagerTestFixture (508 ms total)
[ PASSED ] 12 tests.
```
End-to-end integration tests on minipack3
(`fboss2_integration_test --gtest_filter='ConfigVlanStaticMacTest.*'`):
```
[==========] Running 3 tests from 1 test suite.
[----------] 3 tests from ConfigVlanStaticMacTest
[ RUN ] ConfigVlanStaticMacTest.StaticMacAddDeleteOnExistingVlan
[ OK ] ConfigVlanStaticMacTest.StaticMacAddDeleteOnExistingVlan (111 ms)
[ RUN ] ConfigVlanStaticMacTest.AutoCreateVlanInSession
[ OK ] ConfigVlanStaticMacTest.AutoCreateVlanInSession (133 ms)
[ RUN ] ConfigVlanStaticMacTest.DeleteOnNonExistentVlanIsIdempotent
[ OK ] ConfigVlanStaticMacTest.DeleteOnNonExistentVlanIsIdempotent (11 ms)
[----------] 3 tests from ConfigVlanStaticMacTest (256 ms total)
[ PASSED ] 3 tests.
```
## Sample usage
Add a static MAC to a brand-new VLAN — the CLI creates both the VLAN
and its interface automatically, then shows the diff before committing:
```
$ fboss2 config vlan 3998 static-mac add AA:BB:CC:DD:EE:FF eth1/1/1
Created VLAN 3998
Successfully added static MAC entry: MAC AA:BB:CC:DD:EE:FF -> VLAN 3998, Port eth1/1/1 (ID 9)
$ fboss2 config session diff
--- current live config
+++ session config
@@ -2168,6 +2168,17 @@
+ {
+ "intfID": 3998,
+ "ipAddresses": [],
+ "name": "fboss3998",
+ "routerID": 0,
+ "vlanID": 3998
+ }
@@ -5899,6 +5910,11 @@
+ {
+ "egressLogicalPortID": 9,
+ "macAddress": "AA:BB:CC:DD:EE:FF",
+ "vlanID": 3998
+ }
@@ -7779,6 +7795,13 @@
+ {
+ "id": 3998,
+ "name": "Vlan3998",
+ "routable": false
+ }
```
Delete on a non-existent VLAN is idempotent (no VLAN is created):
```
$ fboss2 config vlan 9999 static-mac delete AA:BB:CC:DD:EE:FF
Static MAC entry for MAC AA:BB:CC:DD:EE:FF on VLAN 9999 does not exist
```
Adds 'fboss2-dev config tunnel ip-in-ip' and 'delete tunnel ip-in-ip' along with the supporting session/handler plumbing: - New command tree under commands/config/tunnel/ip_in_ip and commands/delete/tunnel/ip_in_ip for create / per-attribute update / whole-tunnel delete / per-attribute reset. - ConfigSession: drop pre-commit underlay-intf-id validation (defer to agent), fix EPERM when metadata file is root-owned. - VlanManager-driven underlay setup in integration tests, replacing the crash-prone probe. - Integration tests (TunnelIpInIpTest.cpp): single fixture covering the full positive + negative matrix for both config and delete, with fixture-level auto-cleanup. Drops the duplicated Config/Delete files and the obsolete ConfigSessionRestart / ConfigInterfaceSpeed tests. - Unit tests under test/config for the new command handlers. - Misc: clang-tidy include cleanup (IPAddressV6.h). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hillol-nexthop
force-pushed
the
ip-in-ip-tunnel
branch
from
224d55e to
9d18b71
Compare
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.
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runBased on #1091
Summary
Implements
fboss2-dev config tunnel ip-in-ipandfboss2-dev delete tunnel ip-in-ipCLI commands for configuring and removing IP-in-IP tunnel attributes on a per-tunnel basis.Config commands
Attribute names and enum values are matched case-insensitively.
Delete commands
Resettable attributes:
source,ttl-mode,dscp-mode,ecn-mode,termination-type. Required fields (destination,underlay-intf-id) cannot be reset individually — delete the whole tunnel instead. Deleting a non-existent tunnel is idempotent (returns success).Implementation Details
fboss/cli/fboss2/commands/config/tunnel/:CmdConfigTunnel— top-levelconfig tunnelsubcommandCmdConfigTunnelIpInIp—ip-in-ip <tunnel-id>selector that accepts attribute key/value pairs in a single invocationfboss/cli/fboss2/commands/delete/tunnel/:CmdDeleteTunnel— top-leveldelete tunnelsubcommandCmdDeleteTunnelIpInIp—ip-in-ip <tunnel-id> [<attr> ...]for whole-tunnel delete or per-attribute resetfolly::IPAddress; enum modes validated against thrift enum strings with helpful error messages;maskis reserved as a sub-keyword and rejected as a tunnel ID.CmdListConfigandCmdSubcommands; CMake and BUCK updated.CmdConfigTunnelIpInIpTest.cpp,CmdDeleteTunnelIpInIpTest.cpp): cover all attribute commands, including invalid inputs, case-insensitive parsing, andmask-as-tunnel-ID rejection.ConfigTunnelIpInIpTest.cpp,DeleteTunnelIpInIpTest.cpp): end-to-end set/verify of multi-attribute config, auto-create, full-tunnel delete, idempotent delete, and per-attribute reset against a live agent.Test Plan
Unit and integration tests pass.