[CORE-10975] Add QoS packet rate limit #9947

coutinhop · 2025-03-07T15:36:36Z

Description

Add packet rate limiting QoS control implementation, using iptables (using '-m limit' and a new mark) and nftables (using 'limit rate over') rules to limit ingress and/or egress packet rate based on workload QoSControls (from pod annotations in k8s).

Related issues/PRs

Todos

Tests
Documentation
Release note

Release Note

Add packet rate limiting QoS control to workloads.

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

docs-pr-required: This change requires a change to the documentation that has not been completed yet.
docs-completed: This change has all necessary documentation completed.
docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

release-note-required: This PR has user-facing changes. Most PRs should have this label.
release-note-not-required: This PR has no user-facing changes.

Other optional labels:

cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

Add packet rate limiting QoS control implementation, using iptables (using '-m limit' and a new mark) and nftables (using 'limit rate over') rules to limit ingress and/or egress packet rate based on workload QoSControls (from pod annotations in k8s).

nelljerram

Looks great. Just a few minor points to address.

In terms of dataplane support - something that I should have checked before! -

Is it correct that we're supporting iptables and nftables, but not eBPF?
Is that the case for the bandwidth control as well?

nelljerram · 2025-03-07T17:56:21Z

felix/dataplane/driver.go

@@ -123,6 +123,8 @@ func StartDataplaneDriver(
 		markScratch0, _ = markBitsManager.NextSingleBitMark()
 		markScratch1, _ = markBitsManager.NextSingleBitMark()

+		markLimitPacketRate, _ = markBitsManager.NextSingleBitMark()


This may be absolutely fine, but can you check with the relevant people - probably @fasaxc and @tomastigera - in case we need to be more careful with using up new mark bits? It might be possible to reuse an existing one, if the places where they are used, for different features, do not overlap with each other.

Considering we invariably use the mark in 2 consecutive iptables rules, I suppose we could reuse an existing 'scratch' mark instead of allocating a new one, but then I think we'd need a 3rd rule to clear the mark (considering it could/would be used for other purposes as well, as we're only using the mark to "bypass" the iptables -m limit module limitation of not being able to drop packets above the limit, we could clear the mark in a rule immediately after the one dropping the packets without the mark). @nelljerram do you think that's a better approach?

I'd prefer @fasaxc or @tomastigera to give an opinion about this, as I don't know how careful we need to be with mark bit usage. One possible situation is that there isn't any real shortage in OSS, but there might be when we get around to merging this into calico-private...

nelljerram · 2025-03-07T18:11:26Z

felix/fv/qos_controls_test.go

+			}
+
+			It("should limit packet rate correctly", func() {
+				format.MaxLength = 1000000


I think this is going to affect all following tests. Personally, I'm a fan of putting format.MaxLength = 0 somewhere, like in libcalico-go/lib/clientv3/tier_e2e_test.go:

func init() { // Stop Gomega from chopping off diffs in logs. format.MaxLength = 0 }

(IOW, my only concern is that it looks like this might be a local-only setting, but in fact it won't be.)

good point! I blindly copied what happened in another test case I saw, but I agree this should be improved. Will do

I would personally favour setting it globally for the whole FV test. I don't think there's any significant downside - because tests aren't supposed to fail! - and it's just annoying when you do get a failure and then the output is truncated.

nelljerram · 2025-03-07T18:14:28Z

felix/fv/qos_controls_test.go

+				By("Running iperf3 client on workload 1")
+				out, err := w[1].ExecOutput("iperf3", "-c", w[0].IP, "-O5", "-M1000", "-J")
+				Expect(err).NotTo(HaveOccurred())
+				baselineRate, err := getRateFromJsonOutput(out)


Note, this is in bits per sec.

this is noted in the Info log a few lines below it:

calico/felix/fv/qos_controls_test.go

Line 189 in 483cc0e

log.Infof("iperf client rate with no packet rate limit (bps): %v", baselineRate)

should I add a comment in addition to that?

If you add a comment to explain the 800000 just below, I don't think we need a comment for this point as well. What was confusing was the combination of not explaining the 800000 and not indicating the units of getRateFromJsonOutput.

felix/fv/qos_controls_test.go

nelljerram · 2025-03-07T18:23:26Z

felix/fv/qos_controls_test.go

+
+				By("Waiting for the config to appear in 'iptables-save' on workload 0")
+				// ingress config should be present
+				Eventually(getRules(0), "10s", "1s").Should(And(MatchRegexp(`-A cali-tw-`+regexp.QuoteMeta(w[0].InterfaceName)+` .* -m comment --comment "Mark packets within ingress packet rate limit" -m limit --limit `+regexp.QuoteMeta("100/sec")+` -j MARK --set-xmark 0x\d+/0x\d+`), MatchRegexp(`-A cali-tw-`+regexp.QuoteMeta(w[0].InterfaceName)+` .* -m comment --comment "Drop packets over ingress packet rate limit" -m mark ! --mark 0x\d+/0x\d+ -j DROP`)))


In this check, iptables-save -c will be called only once, returning a string, and then that same string will be analysed up to 10 times. If it fails the first time, it won't make any difference to keep trying for the next 10s.

What you really want is:

getRules := func(felixId int) func() string { return func() { out, err := tc.Felixes[felixId].ExecOutput("iptables-save", "-c") log.Infof("iptables-save -c output:\n%v", out) Expect(err).NotTo(HaveOccurred()) return out } }

nelljerram · 2025-03-07T18:26:20Z

felix/fv/qos_controls_test.go

+				ingressLimitedRate, err := getRateFromJsonOutput(out)
+				Expect(err).NotTo(HaveOccurred())
+				log.Infof("iperf client rate with ingress packet rate limit on server (bps): %v", ingressLimitedRate)
+				// Expect the limited rate to be below an estimated desired rate (1000 byte packets * 8 bits/byte * 100 packets/s = 800000 bps)


Ah nice. Please explain that relation above as well.

nelljerram · 2025-03-07T18:26:46Z

felix/fv/qos_controls_test.go

+				Expect(err).NotTo(HaveOccurred())
+				log.Infof("iperf client rate with ingress packet rate limit on server (bps): %v", ingressLimitedRate)
+				// Expect the limited rate to be below an estimated desired rate (1000 byte packets * 8 bits/byte * 100 packets/s = 800000 bps)
+				Expect(ingressLimitedRate).To(BeNumerically("<=", 800000.0))


Should there be a bit of leeway here, e.g. * 1.2

In my experience, this upperbound was never even close to hitting when I ran the test (the max was typically 640kbps), but I suppose it doesn't hurt to have some margin, will add!

nelljerram · 2025-03-07T18:50:27Z

felix/rules/endpoints_test.go

@@ -1359,6 +1369,82 @@ var _ = Describe("Endpoints", func() {
 					},
 				}))
 			})
+
+			It("should render a workload endpoint with packet rate limiting QoSControls", func() {
+				format.MaxLength = 1000000


same comment about this as in FV test

nelljerram · 2025-03-07T18:53:17Z

manifests/ocp/custom-resources.yaml

Shouldn't be in this PR.

oh, I think this showed up after running make generate, will remove

coutinhop · 2025-03-07T22:56:26Z

Looks great. Just a few minor points to address.

In terms of dataplane support - something that I should have checked before! -
* Is it correct that we're supporting iptables and nftables, but not eBPF?

* Is that the case for the bandwidth control as well?

Thanks so much! Yes, currently all 3 planned controls will only support iptables and nftables, but not eBPF. This control (and the future connection limit control) relies on iptables/nftables itself, and will need to be implemented differently on eBPF. The bandwidth control has an incompatibility between the tc qdisc that the control needs and one that the eBPF dataplane uses to attach BPF programs to interfaces. So yes, in short, this will support iptables/nftables, but not eBPF (at least for now).

… client) Also address review comments: - fix usage of format.MaxLength - remove manifests/ocp/custom-resources.yaml

coutinhop · 2025-03-10T20:30:23Z

felix/fv/infrastructure/infra_k8s.go

 	args := []string{
 		"-v", os.Getenv("CERTS_PATH") + ":/home/user/certs", // Mount in location of certificates.
+		"-v", pwd + "/../../libcalico-go/config/crd:/crds", // Mount in location of CRDs.


I'm not entirely sure but I was having failures almost every time when trying to copy the CRDs into the container (maybe related to low disk space on my machine), mounting as volume fixed it

OK with me if it doesn't cause other problems.

nelljerram

Thanks, this looks great. One place where a little more commenting would be useful.

nelljerram · 2025-03-11T15:55:52Z

felix/fv/infrastructure/infra_k8s.go

 	args := []string{
 		"-v", os.Getenv("CERTS_PATH") + ":/home/user/certs", // Mount in location of certificates.
+		"-v", pwd + "/../../libcalico-go/config/crd:/crds", // Mount in location of CRDs.


OK with me if it doesn't cause other problems.

nelljerram · 2025-03-11T16:01:17Z

felix/fv/qos_controls_test.go

+		if err != nil {
+			continue
+		}
+		time.Sleep(retryInterval)


Can you add some commenting here to explain why we retry even when an interation succeeds?

huh, to be honest, we don't need to, I'll move the sleep so it only happens when we're continueing (i.e. an actual retry)
thanks for catching this!

Ah, I wondered that too 😄

coutinhop added release-note-required Change has user-facing impact (no matter how small) docs-not-required Docs not required for this change labels Mar 7, 2025

coutinhop requested review from fasaxc and nelljerram March 7, 2025 15:36

coutinhop self-assigned this Mar 7, 2025

coutinhop requested a review from a team as a code owner March 7, 2025 15:36

marvin-tigera added this to the Calico v3.30.0 milestone Mar 7, 2025

coutinhop mentioned this pull request Mar 7, 2025

[CORE-10975] Add QoS packet rate limit #9938

Merged

3 tasks

nelljerram requested changes Mar 7, 2025

View reviewed changes

coutinhop added 4 commits March 10, 2025 12:01

Fix test flakiness (infra fixes and adding a retry mechanism to iperf…

5a9daab

… client) Also address review comments: - fix usage of format.MaxLength - remove manifests/ocp/custom-resources.yaml

Use (and clear) scratch firewall mark instead of allocating new one

3948b76

revert mark changes in tests

56156c6

fix ut

52227d2

coutinhop commented Mar 10, 2025

View reviewed changes

fix test for NFTables mode

b067464

nelljerram approved these changes Mar 11, 2025

View reviewed changes

Adjust retry mechanism for iperf client in test

8684f7b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-10975] Add QoS packet rate limit #9947

[CORE-10975] Add QoS packet rate limit #9947

coutinhop commented Mar 7, 2025

nelljerram left a comment

nelljerram Mar 7, 2025

coutinhop Mar 7, 2025

nelljerram Mar 10, 2025

nelljerram Mar 7, 2025

nelljerram Mar 7, 2025

coutinhop Mar 7, 2025

nelljerram Mar 10, 2025

nelljerram Mar 7, 2025

coutinhop Mar 7, 2025

nelljerram Mar 10, 2025

nelljerram Mar 7, 2025

nelljerram Mar 7, 2025

nelljerram Mar 7, 2025

coutinhop Mar 7, 2025

nelljerram Mar 7, 2025

nelljerram Mar 7, 2025

coutinhop Mar 7, 2025

coutinhop commented Mar 7, 2025

coutinhop Mar 10, 2025

nelljerram Mar 11, 2025

nelljerram left a comment

nelljerram Mar 11, 2025

nelljerram Mar 11, 2025

coutinhop Mar 11, 2025

nelljerram Mar 11, 2025

[CORE-10975] Add QoS packet rate limit #9947

Are you sure you want to change the base?

[CORE-10975] Add QoS packet rate limit #9947

Conversation

coutinhop commented Mar 7, 2025

Description

Related issues/PRs

Todos

Release Note

Reminder for the reviewer

nelljerram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coutinhop commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelljerram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment