Skip to content

[Nexthop][fboss2-dev] Add fboss2-dev delete copp subcommands (cpu-queue, reason) - #1466

Open
vybhav-nexthop wants to merge 3 commits into
facebook:mainfrom
nexthop-ai:delete-copp-subtree
Open

[Nexthop][fboss2-dev] Add fboss2-dev delete copp subcommands (cpu-queue, reason)#1466
vybhav-nexthop wants to merge 3 commits into
facebook:mainfrom
nexthop-ai:delete-copp-subtree

Conversation

@vybhav-nexthop

@vybhav-nexthop vybhav-nexthop commented Aug 5, 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-side counterparts for the copp config commands:

  • delete copp cpu-queue <id> — removes the whole sw.cpuQueues[] entry. Refused while rxReason mappings (rxReasonToQueueOrderedList) or cpu-traffic-policy actions (send-to-queue, user-defined-trap) still reference the queue id; the error names each referencing entry.
  • delete copp reason <reason-name> — removes the rxReasonToQueueOrderedList entry.

Why: The config-side copp commands landed without a way to unset what they set.

Notes

Whole-queue delete is not undoable by the CLI

delete copp cpu-queue <id> erases the entire cpuQueues[] entry, matching delete qos default-queue-config <id>, which erases a whole entry from defaultPortQueues — same list<PortQueue> shape.

The difference is on the config side. config qos default-queue-config can write reserved-bytes, shared-bytes, weight, scaling-factor, scheduling, stream-type, buffer-pool-name and AQM entries via utils::applyPortQueueConfig, so a deleted default queue can be rebuilt by hand. config copp cpu-queue writes only name and portQueueRate, so a deleted cpu-queue cannot.

PortQueue has 17 fields and the CLI can set two of them (name, and portQueueRate via rate-limit). streamType and scheduling are non-optional in thrift, so a recreated queue silently picks up their zero values. A platform agent.conf ships queue 9 as:

{ "id": 9, "name": "cpuQueue-high", "scheduling": 1, "streamType": 1 }

scheduling 1 is STRICT_PRIORITY, streamType 1 is MULTICAST. Delete it and recreate it and you get scheduling 0 (WEIGHTED_ROUND_ROBIN) and streamType 0 (UNICAST), with no command to set either back. The high-priority CPU queue would start round-robining.

So: delete a platform-provisioned cpu-queue only when you want it gone for good. Queues you created yourself are safe to delete and recreate.

To change a name or rate limit, no delete is involved — config overwrites in place:

fboss2-dev config copp cpu-queue 9 name some-name
fboss2-dev config copp cpu-queue 9 rate-limit pps 300

delete copp reason takes no queue id

Config is config copp reason <reason> queue <id>, delete is delete copp reason <reason>. rxReasonToQueueOrderedList holds at most one entry per reason — config overwrites an existing reason rather than appending a second entry — so the reason name alone identifies the entry, and delete looks it up that way:

// CmdDeleteCoppReason.cpp
auto it = std::find_if(
    list.begin(), list.end(), [&args](const cfg::PacketRxReasonToQueue& e) {
      return *e.rxReason() == args.getReason();
    });

Passing a queue id is rejected rather than ignored (->expected(1), plus the arg parser throws on a second token). The queue id shows up on the way out instead — Deleted reason ARP -> queue 9 mapping — read off the entry being removed.

Re-pointing a reason at a different queue is a config, not a delete: config copp reason arp queue 5 prints Updated reason ARP -> queue 5. Delete is for taking the reason off the CPU path entirely.

Clearing what blocks a queue delete

The refusal message lists each referrer: reason <NAME> for rxReason mappings, matcher '<name>' send-to-queue / matcher '<name>' user-defined-trap for cpu-traffic-policy actions (findQueueReferences(), CmdDeleteCoppCpuQueue.cpp). Only those two action types carry a queue id, so only they can block.

Clear reasons with delete copp reason <reason>; clearing a blocking action needs the cpu-traffic-policy action delete from #1379:

fboss2-dev delete copp cpu-traffic-policy match <matcher> action send-to-queue
fboss2-dev delete copp cpu-queue 9

The deprecated rxReasonToCPUQueue map is ignored entirely (per review — the field is no longer used).

Test Plan

Unit tests — 13 added by this PR (7 in CmdDeleteCoppCpuQueueTestFixture, 6 across the CmdDeleteCoppReason* fixtures), all passing:

$ bazel-bin/fboss/cli/fboss2/test/config/cmd_config_test --gtest_filter='CmdDeleteCoppCpuQueueTestFixture.*:CmdDeleteCoppReason*'
[==========] Running 13 tests from 4 test suites.
[----------] 7 tests from CmdDeleteCoppCpuQueueTestFixture (685 ms total)
[----------] 4 tests from CmdDeleteCoppReasonTestFixture (310 ms total)
[----------] 1 test from CmdDeleteCoppReasonNoPolicyFixture (99 ms total)
[----------] 1 test from CmdDeleteCoppReasonNoListFixture (89 ms total)
[==========] 13 tests from 4 test suites ran. (1185 ms total)
[  PASSED  ] 13 tests.

Integration tests on a freshly imaged T1 device — 2/2 pass)
One end-to-end test per command, both unconditional .
Each test creates what it deletes (config copp reason creates cpuTrafficPolicy and rxReasonToQueueOrderedList when absent) and removes it again, so nothing touches a live CoPP policy.

$ ./fboss2_integration_test --gtest_filter='DeleteCoppTest.*'
[==========] Running 2 tests from 1 test suite.
[ RUN      ] DeleteCoppTest.DeleteWholeCpuQueue
DeleteCoppTest.cpp:119] Creating scratch cpu-queue 3
[       OK ] DeleteCoppTest.DeleteWholeCpuQueue (185 ms)
[ RUN      ] DeleteCoppTest.DeleteReasonMapping
DeleteCoppTest.cpp:150] Mapping reason eapol -> scratch cpu-queue 3
DeleteCoppTest.cpp:161] Deleting reason eapol
DeleteCoppTest.cpp:170] Removing scratch cpu-queue 3
[       OK ] DeleteCoppTest.DeleteReasonMapping (242 ms)
[==========] 2 tests from 1 test suite ran. (427 ms total)
[  PASSED  ] 2 tests.

$ fboss2-dev config session diff        # device left unchanged
No differences between current live config and session config.

Sample usage on the same device — note the refusal list shrinks as the referencing reason is deleted:

$ fboss2-dev delete copp cpu-queue 9
localhost: Thrift call failed: 'Cannot delete cpu-queue 9: still referenced by
reason NDP, reason ARP_RESPONSE, reason BGP, reason BGPV6, reason LACP, reason ARP.
Delete those references first.'

$ fboss2-dev delete copp reason arp
Deleted reason ARP -> queue 9 mapping

$ fboss2-dev config session diff
--- current live config
+++ session config
@@ -146,10 +146,6 @@
         {
           "queueId": 1,
           "rxReason": 0
-        },
-        {
-          "queueId": 9,
-          "rxReason": 1
-        }
       ]
     },

$ fboss2-dev delete copp cpu-queue 9
localhost: Thrift call failed: 'Cannot delete cpu-queue 9: still referenced by
reason NDP, reason ARP_RESPONSE, reason BGP, reason BGPV6, reason LACP.
Delete those references first.'

$ fboss2-dev config session clear
Config session cleared successfully.

Review Findings

Pre-publication review (7 reviewers + verifier, confidence >= 0.7) surfaced these findings; all are addressed in this PR:

  • shared parsing helpers moved to a dedicated CoppUtils.{h,cpp} instead of living in the config command header
  • user-defined-trap refusal branch gained a unit test
  • duplicated not-found throw in the reason delete collapsed into one helper
  • integration-test reason candidates use cfg::PacketRxReason enum values instead of bare ints

@vybhav-nexthop
vybhav-nexthop requested review from a team as code owners August 5, 2026 10:18
@meta-cla meta-cla Bot added the CLA Signed label Aug 5, 2026
@meta-codesync

meta-codesync Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

This pull request has been imported. If you are a Meta employee, you can view this in D114872682. (Because this pull request was imported automatically, there will not be any future comments.)

Delete-side counterparts for the copp config commands:

  delete copp cpu-queue <id>       - remove the whole sw.cpuQueues[]
      entry; refused while rxReason mappings (the ordered list or the
      deprecated rxReasonToCPUQueue map, which the agent still reads as
      a fallback) or matchToAction actions (send-to-queue,
      user-defined-trap) still reference the queue id, naming each
      referencing entry
  delete copp reason <reason-name> - remove the
      rxReasonToQueueOrderedList entry

Per-attribute deletes (cpu-queue <id> name, cpu-queue <id> rate-limit)
are left for a follow-up.

Queue-id parsing/lookup and reason-name parsing move out of
CmdConfigCopp.cpp into the copp_cpu_queue and copp_reason namespaces in
a new CoppUtils.{h,cpp} so the config and delete trees share one
implementation. Both handlers save with ConfigActionLevel::HITLESS,
matching the config-side handlers.

This also introduces the `delete copp` parent command node, which does
not exist upstream yet (facebook#1379 adds it too, for the cpu-traffic-policy
delete; whichever lands second is a trivial merge).
@facebook-github-tools

Copy link
Copy Markdown

@vybhav-nexthop has updated the pull request. You must reimport the pull request before landing.

@facebook-github-tools

Copy link
Copy Markdown

@vybhav-nexthop has updated the pull request. You must reimport the pull request before landing.

@joseph5wu joseph5wu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main comment for this PR to fix is to remove the logic of handling rxReasonToCPUQueue as we already deprecated this field. Let me know once you remove the relevant code for this field.

Besides, when reviewing this PR, it made me think we might need 3 more improvements related to copp and you can address them in the future PRs respectively:

  1. Rename cpu_queue to just queue. It's already under copp, so queue implicitly mean cpu_queue, so we don't need to be verbose about that;
  2. About your comment in the summary that config copp cpu-queue can only support changing name and rate_limit. FBOSS supports cpu queue just as port queue, can you make another PR to make sure config for cpu queue with the missing attributes?
  3. Since rxReasonToQueueOrderedList is actually an ordered list, I think we need to adjust the current config copp reason <reason-name> queue <id> to allow specify order rather than always push_back to the list:
    https://github.com/facebook/fboss/blob/main/fboss/cli/fboss2/commands/config/copp/CmdConfigCopp.cpp#L276

cc: @benoit-nexthop


namespace facebook::fboss {

namespace copp_cpu_queue {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create another PR to simplify the subcommand here to just be copp_queue or replace all the cpu_queue with just queue?
copp already means control plane so the queue used in here already means cpu queue, I think we can make the whole command less verbose.
I think these should work:

fboss2-dev config copp queue XXX
fboss2-dev delete copp queue XXX

Comment on lines +56 to +68
// The deprecated rxReasonToCPUQueue map is still honored by the agent as
// a fallback when the ordered list is unset, so a queue referenced only
// there is still live.
if (policy.rxReasonToCPUQueue().has_value()) {
for (const auto& [rxReason, mappedQueueId] : *policy.rxReasonToCPUQueue()) {
if (mappedQueueId == queueId) {
references.push_back(
fmt::format(
"reason {} (deprecated rxReasonToCPUQueue map)",
apache::thrift::util::enumNameSafe(rxReason)));
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can ignore this field as we don't use it internally anymore.
Besides we don't support setting the reason to the deprecated field in
https://github.com/facebook/fboss/blob/main/fboss/cli/fboss2/commands/config/copp/CmdConfigCopp.cpp#L253
I think we should just stop using rxReasonToCPUQueue at all

Comment on lines +204 to +207
// The deprecated rxReasonToCPUQueue map form is still read by the agent as
// a fallback when rxReasonToQueueOrderedList is unset, so a queue referenced
// only there must also refuse deletion. The CLI never writes this map; it
// only appears in configs authored outside the CLI.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's stop supporting this deprecated field: rxReasonToCPUQueue

The field is deprecated and no longer honored internally, so a queue
referenced only by the legacy map is not live; stop treating it as a
blocking reference and drop the legacy-map fixture.
@facebook-github-tools

Copy link
Copy Markdown

@vybhav-nexthop has updated the pull request. You must reimport the pull request before landing.

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