Skip to content

[Nexthop][fboss2-dev] Add delete switch admin-distance and icmpv4-unavailable-src-addr - #1523

Open
vybhav-nexthop wants to merge 2 commits into
facebook:mainfrom
nexthop-ai:delete-switch-admin-distance
Open

[Nexthop][fboss2-dev] Add delete switch admin-distance and icmpv4-unavailable-src-addr#1523
vybhav-nexthop wants to merge 2 commits into
facebook:mainfrom
nexthop-ai:delete-switch-admin-distance

Conversation

@vybhav-nexthop

@vybhav-nexthop vybhav-nexthop commented Aug 17, 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

Background: what admin distance is and what deleting an entry means

When two routing clients (BGP, OpenR, static config, ...) offer routes to the same prefix, admin distance decides who wins: the route with the lowest distance is programmed. sw.clientIdToAdminDistance maps a client-id to its distance, and the agent consults it through getAdminDistanceForClientId() each time it programs a route for that client.

Deleting a client's entry does not resurrect a per-client default: the thrift default map applies only when the whole field is absent from the config. A client with no entry gets AdminDistance::MAX_ADMIN_DISTANCE (255, least preferred), and the agent logs an error per lookup. Client-ids 1 through 4 (STATIC_ROUTE, INTERFACE_ROUTE, LINKLOCAL_ROUTE, REMOTE_INTERFACE_ROUTE) never consult the map at all; their distances are compile-time constants in the agent.

What: Two delete commands under a new delete switch branch, mirroring config switch:

fboss2-dev delete switch admin-distance <client-id>
fboss2-dev delete switch icmpv4-unavailable-src-addr
  • admin-distance removes the client's entry from sw.clientIdToAdminDistance. Saved at coldboot level, for the same reason as the config counterpart: the map is consulted only at route-program time, and existing routes are not re-stamped when it changes. Client-ids 1 through 4 are refused, mirroring the refusal in config switch admin-distance, since removing an entry the agent never reads would only pretend to do something.
  • icmpv4-unavailable-src-addr clears the optional sw.icmpV4UnavailableSrcAddress field, returning it to its unset default. Hitless.

Why: config switch admin-distance and config switch icmpv4-unavailable-src-addr could set these values but nothing could remove them.

How: Both leaves follow the single-positional-arg pattern of the existing delete commands. The client-id parsing and forbidden-client check are shared with the config command via parseAdminDistanceClientId() next to the existing forbiddenAdminDistanceClients(), so the config and delete commands cannot drift apart. The success message states the consequence: routes from the client will use MAX_ADMIN_DISTANCE (255) until a new entry is set, and a coldboot applies the change to existing routes.

Sample usage

$ fboss2-dev delete switch admin-distance 786
Successfully removed admin distance entry for client-id 786. Routes from this client will use MAX_ADMIN_DISTANCE (255). A coldboot is required to apply the change to existing routes.
$ fboss2-dev delete switch admin-distance 1
FBOSS does not allow removing the admin distance entry for client-id 1: STATIC_ROUTE is hardcoded to AdminDistance::STATIC_ROUTE in the agent
$ fboss2-dev delete switch icmpv4-unavailable-src-addr
Successfully removed ICMPv4 unavailable source address 192.0.2.1; the agent will use the RFC 7600 default (192.0.0.8)

Test Plan

Validation ran in our internal tree, which carries this same change. The upstream tree itself was not built locally; this PR's CI is the build verification.

Unit (cmd_config_test): arg validation (arity, non-integer, negative), forbidden-client refusal (ids 1 through 4 refused, entry kept), delete existing entry, delete missing entry throws, plus the icmpv4 set/clear paths:

[==========] 12 tests from 2 test suites ran. (1033 ms total)
[  PASSED  ] 12 tests.

Integration (ConfigAdminDistanceTest, run on a DUT): set an entry for an unused client-id, delete it, verify the running config returns exactly to baseline; forbidden client-ids rejected symmetrically by config and delete.

Review Findings

Pre-publication review (5 reviewers + verifier) raised one finding, fixed before push: the delete argument type duplicated the config command's client-id parse/validate block, now extracted into the shared parseAdminDistanceClientId().

@vybhav-nexthop
vybhav-nexthop requested review from a team as code owners August 17, 2026 13:19
@meta-cla meta-cla Bot added the CLA Signed label Aug 17, 2026
…c-addr

delete switch admin-distance <client-id> removes a routing client's
entry from sw.clientIdToAdminDistance. Once the entry is gone,
getAdminDistanceForClientId() finds no mapping and returns
AdminDistance::MAX_ADMIN_DISTANCE (255) for that client; the per-client
default in switch_config.thrift only applies when the whole map is
absent from the config. Saved at coldboot level because the map is
consulted only at route-program time and existing routes are not
re-stamped. Client-ids 1-4 (STATIC_ROUTE, INTERFACE_ROUTE,
LINKLOCAL_ROUTE, REMOTE_INTERFACE_ROUTE) are refused: their distances
are hardcoded in the agent, mirroring the refusal in config switch
admin-distance. The client-id parsing and forbidden-client check are
shared with the config command via parseAdminDistanceClientId().

delete switch icmpv4-unavailable-src-addr clears the optional
sw.icmpV4UnavailableSrcAddress field, returning it to its unset
default. Hitless.
@vybhav-nexthop
vybhav-nexthop force-pushed the delete-switch-admin-distance branch from a071842 to 34859c2 Compare August 17, 2026 13:30
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