Skip to content

[Nexthop][fboss2-dev] Add delete qos policy subcommands - #1497

Open
vybhav-nexthop wants to merge 3 commits into
facebook:mainfrom
nexthop-ai:delete-qos-policy
Open

[Nexthop][fboss2-dev] Add delete qos policy subcommands#1497
vybhav-nexthop wants to merge 3 commits into
facebook:mainfrom
nexthop-ai:delete-qos-policy

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. Adds the delete counterpart to the existing config qos policy family:

fboss2-dev delete qos policy <name>
fboss2-dev delete qos policy <name> map dscp <dscp-value>
fboss2-dev delete qos policy <name> map mpls-exp <exp>
fboss2-dev delete qos policy <name> map dot1p <pcp>
fboss2-dev delete qos policy <name> map tc-to-queue <tc>
fboss2-dev delete qos policy <name> map pfc-pri-to-queue <pri>
fboss2-dev delete qos policy <name> map tc-to-pg <tc>
fboss2-dev delete qos policy <name> map pfc-pri-to-pg <pri>

Why. The QoS policy object and its mappings could be added but never taken
away. The map delete covers every QosMap type CmdConfigQosPolicyMap writes —
anything less leaves one-way doors: settable via CLI with no CLI way back.

How.

Whole-policy delete refuses when the policy is still named by
dataPlaneTrafficPolicy or cpuTrafficPolicy.trafficPolicy, through either
defaultQosPolicy or portIdToQosPolicy, and reports the exact referring
field. These are plain strings in thrift, so nothing else stops a delete from
leaving a dangling reference. Refusing rather than cascading is deliberate:
clearing the referrer would change forwarding on ports the user did not name.

The three list maps (dscpMaps, expMaps, pcpMaps) are structurally
identical — groups keyed by internalTrafficClass, not flat maps — so they
share one erase helper: the codepoint is removed from the group that lists it.
A group left with no ingress codepoints and no egress rewrite carries no
information and is dropped; a group that still has the rewrite is kept — the
two halves are independent.

The four simple maps (trafficClassToQueueId, pfcPriorityToQueueId,
trafficClassToPgId, pfcPriorityToPgId) are plain key erases. The optional
ones are reset once their last key goes, returning the field to its unset
default rather than leaving an empty map behind.

The policy-by-name scan moves into a shared utils::findQosPolicy
(commands/config/qos/QosPolicyUtils.h, alongside the existing
PortQueueConfigUtils), replacing three separate copies — one in each new
delete command and the hand-rolled loop in CmdConfigQosPolicyMap. The name
argument reuses QosPolicyName from the config side for the same reason, the
way delete qos queuing-policy reuses QueuingPolicyName.

Test Plan

24 unit tests in CmdDeleteQosPolicyTest.cpp, seeded with one deletable
policy carrying every map type and one policy referenced by
dataPlaneTrafficPolicy.defaultQosPolicy. Per-kind coverage: delete an
existing entry, delete a missing entry errors, the emptied-optional reset,
and value-range validation (dscp 0-63, everything else 3-bit).

[==========] 24 tests from 1 test suite ran. (1378 ms total)
[  PASSED  ] 24 tests.

Full cmd_config_test suite, unfiltered, no regressions:

//fboss/cli/fboss2/test/config:cmd_config_test    PASSED in 46.6s
Executed 1 out of 1 test: 1 test passes.

Integration test — test/integration_test/DeleteQosPolicyTest.cpp, one test,
self-cleaning and positive-flow only (missing-entry/missing-policy errors are
unit-tested): create a scratch policy carrying a dscp mapping and a
tc-to-queue mapping, delete the dscp entry and assert trafficClassToQueueId
is untouched, delete the tc-to-queue entry, delete the policy.

Review Findings

Pre-publication review (fboss-review) is clean after fixes: the three refusal
branches (portIdToQosPolicy, cpuTrafficPolicy, and the no-qosMap guard) gained
unit coverage, and the DSCP bounds and find-policy-or-throw were de-duplicated
into QosPolicyUtils.

Adds the delete counterpart to the existing `config qos policy` family:

  fboss2-dev delete qos policy <name>
  fboss2-dev delete qos policy <name> map dscp <dscp-value>
  fboss2-dev delete qos policy <name> map tc-to-queue <tc>

Whole-policy delete refuses when the policy is still named by
dataPlaneTrafficPolicy or cpuTrafficPolicy.trafficPolicy, via either
defaultQosPolicy or portIdToQosPolicy. The error reports the referring field
so the operator knows what to unset first. Cascading instead would change
forwarding on ports the user did not name.

dscpMaps groups codepoints under an internalTrafficClass, so removing a DSCP
value erases it from the group that lists it. A group left with no ingress
codepoints and no fromTrafficClassToDscp rewrite carries no information and is
dropped; one that still has the egress rewrite is kept.

tc-to-queue is a plain map erase. Only dscp and tc-to-queue are removable; the
other map types the config command writes are rejected by name.

The policy-by-name scan and the DSCP bounds are shared with the config side via
QosPolicyUtils, so both agree on what "the policy called X" means and on the
valid DSCP range.
The config command writes all seven QosMap types but only dscp and
tc-to-queue were deletable, leaving the other five as one-way doors:
settable via CLI with no CLI way back. Adds mpls-exp, dot1p,
pfc-pri-to-queue, tc-to-pg and pfc-pri-to-pg using the same tokens as
the config command.

List maps (dscp/exp/pcp) share one erase helper: the codepoint is
removed from whichever entry's ingress list carries it, and entries
left with no codepoints and no egress rewrite are dropped. Optional
simple maps are reset once their last key is erased, returning the
field to its unset default.
@vybhav-nexthop

Copy link
Copy Markdown
Contributor Author

Extended the map delete to every QosMap type the config command writes (mpls-exp, dot1p, pfc-pri-to-queue, tc-to-pg, pfc-pri-to-pg) — the remaining types were settable via CLI with no CLI way back. Same tokens as the config command; unit tests cover each kind plus the emptied-optional reset behavior.

Missing-entry and missing-policy errors are unit-tested; the DUT run
sticks to the positive flow: create, dismantle entry by entry, remove.
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.

1 participant