[Nexthop][fboss2-dev] Add fboss2-dev delete load-balancing subcommands - #1494
Open
vybhav-nexthop wants to merge 2 commits into
Open
[Nexthop][fboss2-dev] Add fboss2-dev delete load-balancing subcommands#1494vybhav-nexthop wants to merge 2 commits into
vybhav-nexthop wants to merge 2 commits into
Conversation
delete load-balancing ecmp|lag removes the matching LoadBalancer entry from sw.loadBalancers. Removal is applied hitlessly: SaiSwitch dispatches LoadBalancersDelta removals to SaiSwitchManager::removeLoadBalancer with no ChangeProhibited guard, the same delta path the config load-balancing subcommands use.
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
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 runSummary
What: Adds
fboss2-dev delete load-balancing ecmpanddelete load-balancing lag, removing the matchingLoadBalancerentry fromsw.loadBalancers. Errors out if the entry is not configured.Why: The config side (
config load-balancing ecmp|lag <attr> <value>) can create and mutate load-balancer entries but nothing can remove one; an emptyloadBalancerslist is the valid unconfigured state.How: New
CmdDeleteLoadBalancingparent + ecmp/lag leaf handlers sharing aremoveLoadBalancer()helper. Removal commits hitlessly — SaiSwitch dispatchesLoadBalancersDeltaremovals toSaiSwitchManager::removeLoadBalancerThe integration test covers ecmp delete + restore end-to-end. LAG delete shares the same implementation and is covered by unit tests
Test Plan
Unit (6 new for
CmdDeleteLoadBalancing, full suite green):Integration on a Nexthop device (delete → verify gone → restore via config CLI → verify match, all hitless):
Sample usage on the device:
Review Findings
Pre-publication review (11-reviewer sweep + verifier, confidence >= 0.7). Two findings, both fixed in this PR before publication:
DeleteLoadBalancingTest.cppASSERT_TRUE. A mid-restore failure left the device with no ECMP load-balancer, since the failure path only discards the uncommitted session.stageEcmpRestore(); the test arms a pending restore before the delete commit and clears it after the restore commits, with aTearDown()that puts the entry back if the test aborted in between.LoadBalancingTestUtils.hcfg::HashingAlgorithm/cfg::IPv4Field/cfg::IPv6Field/cfg::TransportField/cfg::MPLSField. This also splits the ipv4/ipv6 handling, which previously shared a branch that could returnflow-labelfor an ipv4 field.