Skip to content

[Nexthop][fboss2-dev] Support deleting an L3 interface with delete interface - #1496

Open
vybhav-nexthop wants to merge 4 commits into
facebook:mainfrom
nexthop-ai:delete-interface-svi
Open

[Nexthop][fboss2-dev] Support deleting an L3 interface with delete interface#1496
vybhav-nexthop wants to merge 4 commits into
facebook:mainfrom
nexthop-ai:delete-interface-svi

Conversation

@vybhav-nexthop

@vybhav-nexthop vybhav-nexthop commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Pre-submission checklist

  • 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
  • pre-commit run

Summary

What: delete interface <name|id> can now remove an L3/SVI interface, not just a physical port.

Why: the command previously only collected ports, so naming an L3 interface returned "No port found for the specified interface(s)". Generated configs frequently leave Interface.name unset, so those interfaces have no name at all and the interface ID is the only handle.

How:

Some background on how ports, VLANs, and L3 interfaces relate. SwitchConfig keeps them as separate lists — sw.ports, sw.vlans, and sw.interfaces (interfaces are where IP addresses live) — tied together like this:

Port ──< sw.vlanPorts >── VLAN ──── Interface(type=VLAN)    many VLANs per port; ≤1 SVI per VLAN
Port ─────────────────────────────  Interface(type=PORT)    routed port; exactly one interface
                                    Interface(loopback/virtual) — attached to nothing
  • A port can be a member of many VLANs (802.1Q trunking). Membership lives in the sw.vlanPorts join list — one {vlanID, logicalPort, emitTags} row per port-VLAN pair — and Port.ingressVlan classifies untagged frames.
  • A VLAN may have one VLAN-type interface, its SVI: the shared routed gateway for all of the VLAN's member ports. The SVI belongs to the VLAN (Interface.vlanID), not to any port — the VLAN a frame classifies into decides which SVI routes it.
  • A routed port skips the VLAN hop: exactly one PORT-type interface is bound to it directly via Interface.portID.
  • Loopback/virtual interfaces attach to no port or VLAN at all.

Until now delete interface resolved each name to a cfg::Port only, deleting the port via utility::removePortsFromConfig (which also prunes the interfaces only that port used). An SVI or a loopback never resolves to a port, hence the old "No port found" error.

What this PR does, in the order a command flows through it:

  1. Resolve. Every argument goes through the shared utils::InterfaceList resolver (from [Nexthop][fboss2-dev] allow addressing interfaces by port logical ID or interface ID #1471), which now tries, in order: port name → port logical ID (for numeric arguments) → interface name → interface ID. Each argument therefore lands as either a port or a portless L3 interface. A number that is both a port logical ID and an interface ID resolves to the port; a unit test pins that precedence.

  2. Split. CmdDeleteInterface partitions the resolved arguments into portsToDelete and interfacesToDelete. Ports keep the existing removePortsFromConfig path unchanged; portless interfaces go to the new InterfaceManager::deleteInterfaces.

  3. Refuse rather than break the config. deleteInterfaces validates the whole set before mutating anything, so a refusal leaves the session untouched. It refuses any delete that would leave a config the agent rejects — or crashes on — at apply time:

    • a PORT-type interface: its port would be left with no router interface (delete the port instead);
    • a tunnel's underlay interface: Tunnel.underlayIntfID is a required field, so there is nothing to clear (delete the tunnel first);
    • the last VLAN-type interface of a VLAN that still has an enabled member port: the agent rejects such a config outright.
  4. Order the two deletes. Interfaces are removed before ports so the refusal checks see the pre-delete state. portsToDelete is passed into the member-port check so a port removed by the same command does not count as keeping its VLAN alive — delete interface <svi> <its-only-port> is accepted rather than wrongly refused.

  5. Clean up back-pointers. A deleted VLAN interface's Vlan.intfID back-pointer is cleared (it carries no configuration of its own), and both kinds of delete commit hitlessly through the session's existing save path.

Stacked on #1471 (the InterfaceList port-logical-ID / interface-ID resolver) — review that first.

Test Plan

Unit — cmd_config_test, all CmdDelete* suites pass. The whole-interface fixtures (CmdDeleteWholeL3InterfaceTestFixture, CmdDeleteInterfaceIdCollisionTestFixture) cover: delete by interface ID, each refusal reason (port-router, tunnel underlay, VLAN with an enabled member port), the combined delete interface <svi> <port> accept path, and the port-vs-interface-ID precedence:

$ bazel-bin/.../cmd_config_test --gtest_filter='CmdDeleteWholeL3InterfaceTestFixture.*:CmdDeleteInterfaceIdCollisionTestFixture.*'
[  PASSED  ] 14 tests.

Integration — DeleteInterfaceTest on a T1 DUT

[  PASSED  ] DeleteInterfaceTest.DeleteLoopbackModeIdempotent
[  PASSED  ] DeleteInterfaceTest.DeleteLldpExpectedValue
[  PASSED  ] DeleteInterfaceTest.DeleteLldpExpectedValueIdempotent
[  PASSED  ] DeleteInterfaceTest.DeleteUnknownInterfaceIdFails   # delete-by-ID resolution, end-to-end

The positive whole-interface delete and every refusal reason are covered by the unit tests above rather than by integration tests: a PORT-type interface cannot be created through the CLI, and a session's port map is not rebuilt for interfaces staged later in the same session, so an integration test could only exercise those paths conditionally on the DUT's running config.

Review Findings

Pre-publication review (fboss-review, 6 reviewers) findings, all addressed before this PR:

  • Combined delete interface <svi> <its-only-port> was wrongly refused because the enabled-member-port check did not exclude ports deleted in the same command — fixed, with a regression test.
  • Missing unit coverage for naming a port and an interface in one command — added.
  • Duplicated test-fixture helpers — extracted into a shared base.
  • Two integration tests were skippable when the DUT config lacked a suitable interface — removed; the paths they covered are unit-tested unconditionally.

@vybhav-nexthop
vybhav-nexthop requested review from a team as code owners August 10, 2026 14:37
@meta-cla meta-cla Bot added the CLA Signed label Aug 10, 2026
benoit-nexthop and others added 2 commits August 10, 2026 15:12
InterfaceList now accepts a purely-numeric argument and resolves it as a
port logical ID or an interface ID, so e.g. `fboss2-dev config interface
2001 mtu 9000` works on the interface with ID 2001. Name lookups always
take precedence over ID lookups, consistently for both ports and
interfaces: port name -> port logical ID -> interface name -> interface ID.

Adds PortMap::getPortNameForLogicalId(), the reverse of the existing
getPortLogicalId().
delete interface can now remove an L3/SVI interface, not just a physical
port. A name resolving to a portless interface deletes the interface;
InterfaceManager refuses a delete that would dangle a reference or produce
a config the agent rejects.

Addressing an interface by a bare ID relies on the shared InterfaceList
resolver (port name -> port logical ID -> interface name -> interface ID)
rather than a delete-only flag, so a number that is both a port logical ID
and an interface ID resolves to the port. Pinned by a new unit test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants