[v3.30] fix(QoS): Use QdiscReplace() instead of QdiscAdd()#11984
Merged
coutinhop merged 2 commits intoMar 4, 2026
Merged
Conversation
Use QdiscReplace() instead of QdiscAdd() so that adding the TBF qdiscs needed for QoS controls with tc does not error out when there is an existing non-default (handle != 0) qdisc on the interface for any reason. Add a test case to the felix FVs to cover this. Also, enable felix debug logging on the QoS felix FVs, and remove overzealous Skip() that was resulting in no test cases running on iptables/nftables modes.
coutinhop
added
release-note-required
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Felix QoS bandwidth qdisc programming to use QdiscReplace() (instead of QdiscAdd()) so ingress/egress TBF configuration succeeds even when a workload interface already has a non-default root qdisc handle, and adds FV coverage for that scenario.
Changes:
- Switch ingress bandwidth qdisc setup to a create-or-update flow using
netlink.QdiscReplace(). - Update egress qdisc paths to use replace semantics for TBF creation/update.
- Extend Felix FV QoS tests with a case covering a pre-existing non-default root qdisc handle, and enable debug logging for these FVs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| felix/fv/qos_controls_test.go | Adds FV coverage for non-default existing root qdisc handles; enables Felix debug logging in these tests. |
| felix/dataplane/linux/qos_controls.go | Routes ingress QoS qdisc add/update through a single create-or-update helper. |
| felix/dataplane/linux/qos/qos.go | Replaces TBF qdisc add/update with QdiscReplace() and consolidates ingress helpers. |
Comment on lines
+331
to
+336
| By("Setting 10Mbps limit and 100Mbps peakrate for ingress on workload 1") | ||
| w[1].WorkloadEndpoint.Spec.QoSControls = &internalapi.QoSControls{ | ||
| IngressBandwidth: 10000000, | ||
| IngressBurst: 300000000, | ||
| IngressPeakrate: 100000000, | ||
| } |
There was a problem hiding this comment.
internalapi.QoSControls is referenced here but internalapi is not imported anywhere in this file (and other test cases in this file use api.QoSControls). This will not compile as-is; switch this to api.QoSControls (consistent with the rest of the FV) or add the correct import/alias if a different type is intended.
nelljerram
approved these changes
Mar 4, 2026
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.
Cherry-pick history
Use QdiscReplace() instead of QdiscAdd() so that adding the TBF qdiscs needed for QoS controls with tc does not error out when there is an existing non-default (handle != 0) qdisc on the interface for any reason.
Add a test case to the felix FVs to cover this.
Also, enable felix debug logging on the QoS felix FVs, and remove overzealous Skip() that was resulting in no test cases running on iptables/nftables modes.
Description
Related issues/PRs
Release Note