[CORE-10974] Add bandwidth QoS controls #9881

coutinhop · 2025-02-21T01:41:44Z

Add bandwidth limiting for ingress and egress, implemented via tc qdisc configuration based on the {ingress,egress} bandwidth and burst fields of the QoSControls struct in WorkloadEndpoint. In kubernetes, they come from annotations on the kubernetes pods.

Felix FV tests added to verify that the tc qdisc configuration is properly added and that the bandwidth is properly limited with iperf3.

Some code borrowed from the kubernetes bandwidth plugin (credit given).

Description

Related issues/PRs

Todos

Tests
Documentation
Release note

Release Note

TBD

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 bandwidth limiting for ingress and egress, implemented via `tc qdisc` configuration based on the {ingress,egress} bandwidth and burst fields of the QoSControls struct in WorkloadEndpoint. In kubernetes, they come from annotations on the kubernetes pods. Felix FV tests added to verify that the `tc qdisc` configuration is properly added and that the bandwidth is properly limited with `iperf3`. Some code borrowed from the kubernetes bandwidth plugin (credit given).

nelljerram

Some questions and corrections...

nelljerram · 2025-02-21T13:44:08Z

felix/dataplane/linux/bwp/ifb_creator.go

+
+	err := netlink.LinkAdd(&netlink.Ifb{
+		LinkAttrs: linkAttrs,
+	})


I'm surprised there's nothing here to say what type of link this is. (i.e. the TYPE value in man ip-link.) Does it default to dummy?

Sorry, I'm not sure I fully follow. The type is indicated by using the netlink.Ifb{} struct, the netlink code for it is:

// Ifb links are advanced dummy devices for packet filtering type Ifb struct { LinkAttrs } func (ifb *Ifb) Attrs() *LinkAttrs { return &ifb.LinkAttrs } func (ifb *Ifb) Type() string { return "ifb" }

Or am I missing something else? Apologies if I am

Ah right, I missed that b. That sounds fine.

nelljerram · 2025-02-21T13:45:39Z

felix/dataplane/linux/bwp/ifb_creator.go

+func CreateIngressQdisc(rateInBits, burstInBits uint64, hostDeviceName string) error {
+	hostDevice, err := netlink.LinkByName(hostDeviceName)
+	if err != nil {
+		return fmt.Errorf("get host device: %s", err)


Will likely be helpful to include hostDeviceName in this error message.

(maybe above in CreateIfb too)

In fact, please can you consider this everywhere you've added new errors?

Will do, thanks!

nelljerram · 2025-02-21T13:47:45Z

felix/dataplane/linux/bwp/ifb_creator.go

+		return fmt.Errorf("get host device: %s", err)
+	}
+
+	// check if host device has a ingres qdisc


sp: ingress

nelljerram · 2025-02-21T13:51:30Z

felix/dataplane/linux/bwp/ifb_creator.go

+		return fmt.Errorf("create ifb qdisc: %s", err)
+	}
+	return nil
+}


Why is there so much asymmetry between the ingress and egress coding?

From my understanding, we can only add one tbf qdisc per interface, and it only works for ingress. So for egress, the workaround is to create an ingress qdisc, an ifb interface, mirror the egress traffic from the original interface to the ifb, and then add the tbf qdisc to the ifb interface...

nelljerram · 2025-02-21T13:53:06Z

felix/dataplane/linux/bwp/ifb_creator.go

+	// tc qdisc add dev link root tbf
+	//		rate netConf.BandwidthLimits.Rate
+	//		burst netConf.BandwidthLimits.Burst
+	if rateInBits <= 0 {


Vars are unsigned, so I think it would be clearer to write == 0 here.

nelljerram · 2025-02-21T14:39:14Z

felix/fv/qos_controls_test.go

+				Expect(err).NotTo(HaveOccurred())
+				log.Infof("iperf client rate with no limit (bps): %v", baselineRate)
+				// Expect the baseline rate to be much greater than the desired limit
+				Expect(baselineRate).To(BeNumerically(">=", 100000.0*100))


Why the *100 factor?

When running the test locally on my machine, I was getting roughly 35Gbps traffic when not limiting. I just figured this would be a sanity check to make sure the "fixed" 100kbps used when enforcing the bandwidth limits is not too close to the non-limited bandwidth. (I figured this might vary depending on the conditions when running the test)

nelljerram · 2025-02-21T14:41:50Z

felix/fv/qos_controls_test.go

+				// ingress config should be present
+				Eventually(getQdisc, "10s", "1s").Should(MatchRegexp(`qdisc tbf \d+: dev ` + regexp.QuoteMeta(w[1].InterfaceName) + ` root refcnt \d+ rate ` + regexp.QuoteMeta("100Kbit")))
+				// egress config should not be present
+				Eventually(getQdisc, "10s", "1s").ShouldNot(MatchRegexp(`qdisc tbf \d+: dev bwcali.* root refcnt \d+ rate ` + regexp.QuoteMeta("100Kbit")))


No reason for Eventually here. Should probably be just Expect.

It does need Eventually, as it takes a quantum of time for the tc qdisc configuration to appear once the WorkloadEndpoint is updated. (granted, I didn't spend much time fine tuning this, 10s is probably too much)

nelljerram · 2025-02-21T14:42:44Z

felix/fv/qos_controls_test.go

+
+				out, err = w[1].ExecOutput("iperf3", "-c", w[0].IP, "-O5", "-J")
+				Expect(err).NotTo(HaveOccurred())
+				egressLimitedRate, err := getRateFromJsonOutput(out)


nit: ingress-limited, not egress-

this is embarrassing, my ingress and egress var names were switched, thanks for catching it!

nelljerram · 2025-02-21T14:44:13Z

felix/fv/qos_controls_test.go

+				Expect(err).NotTo(HaveOccurred())
+				log.Infof("iperf client rate with ingress limit (bps): %v", egressLimitedRate)
+				// Expect the ingress limited rate to be much greater than the desired limit, but smaller than baseline (with a 20% margin)
+				Expect(egressLimitedRate).To(BeNumerically(">=", 100000.0*100))


I don't understand the logic of this check. Why "Expect the ingress limited rate to be much greater than the desired limit"? What is "the desired limit"?

yes, this was a mistake, I'll update the test to run iperf3 in the correct direction and have the proper expectation

nelljerram · 2025-02-21T14:46:15Z

felix/fv/qos_controls_test.go

+				// ingress config should not be present
+				Eventually(getQdisc, "10s", "1s").ShouldNot(MatchRegexp(`qdisc tbf \d+: dev ` + regexp.QuoteMeta(w[1].InterfaceName) + ` root refcnt \d+ rate ` + regexp.QuoteMeta("100Kbit")))
+				// egress config should be present
+				Eventually(getQdisc, "10s", "1s").Should(And(MatchRegexp(`qdisc ingress ffff: dev `+regexp.QuoteMeta(w[1].InterfaceName)+` parent ffff:fff1`), MatchRegexp(`qdisc tbf \d+: dev bwcali.* root refcnt \d+ rate `+regexp.QuoteMeta("100Kbit"))))


same as above

- Improve error messages - Fix ingress bandwidth testing - Other misc corrections

It will be replaced by felix functionality. Needs to merge in sync with projectcalico/calico#9881.

coutinhop requested review from fasaxc and nelljerram February 21, 2025 01:41

coutinhop self-assigned this Feb 21, 2025

coutinhop requested a review from a team as a code owner February 21, 2025 01:41

marvin-tigera added this to the Calico v3.30.0 milestone Feb 21, 2025

marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Feb 21, 2025

coutinhop force-pushed the pedro-CORE-10974 branch 2 times, most recently from 3e04ce5 to 60759ea Compare February 21, 2025 02:11

nelljerram requested changes Feb 21, 2025

View reviewed changes

coutinhop added 2 commits February 21, 2025 10:26

Address review comments:

b5a9295

- Improve error messages - Fix ingress bandwidth testing - Other misc corrections

Fix error logging on ifb teardown

32c284a

coutinhop added a commit to coutinhop/operator that referenced this pull request Feb 22, 2025

[CORE-10974] Remove bandwidth plugin chaining

b39da31

It will be replaced by felix functionality. Needs to merge in sync with projectcalico/calico#9881.

coutinhop mentioned this pull request Feb 22, 2025

[CORE-10974] Remove bandwidth plugin chaining tigera/operator#3784

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-10974] Add bandwidth QoS controls #9881

[CORE-10974] Add bandwidth QoS controls #9881

coutinhop commented Feb 21, 2025

nelljerram left a comment

nelljerram Feb 21, 2025

coutinhop Feb 21, 2025

nelljerram Feb 21, 2025

nelljerram Feb 21, 2025

nelljerram Feb 21, 2025

nelljerram Feb 21, 2025

coutinhop Feb 21, 2025

nelljerram Feb 21, 2025

nelljerram Feb 21, 2025

coutinhop Feb 21, 2025

nelljerram Feb 21, 2025

nelljerram Feb 21, 2025

coutinhop Feb 21, 2025

nelljerram Feb 21, 2025

coutinhop Feb 21, 2025

nelljerram Feb 21, 2025

coutinhop Feb 21, 2025

nelljerram Feb 21, 2025

coutinhop Feb 21, 2025

nelljerram Feb 21, 2025

[CORE-10974] Add bandwidth QoS controls #9881

Are you sure you want to change the base?

[CORE-10974] Add bandwidth QoS controls #9881

Conversation

coutinhop commented Feb 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment