tcproute: add support for round-robin load balancing #148

aryan9600 · 2023-12-01T15:11:47Z

Add support for round-robin load balancing for TCPRoute. This enables mutliple backend references in a UDPRoute object, with traffic being distributed to each backend in a round-robin fashion.

A new BPF map TCP_CONNECTIONS was introduced to help keep track of active TCP connections, with its key storing the clien <ip:port> identifier and the value storing the backend's <ip:port> along with the state of the connection.

Fixes #119

Follow up items:

modify TCP_CONNECTIONS to be more generic; get rid of BLIXT_CONNTRACK and refactor udp ingress and icmp egress to use the new generic map

shaneutt

This looks great, thank you for jumping on this!

Many of my comments are about small improvements or idioms but nothing major. While I think in the future we can probably take a look at other TCP implementations and take some inspiration there, I think it's reasonable to have this as a first step which gives us something that works and then work iteratively up from here.

I'm specifically appreciative of the comments you've been adding, these can be very helpful to future readers of the code and make our codebase more inviting/inclusive for the community.

As an aside and unrelated to any specific bit of code or suggestion: just a general reminder that we're in a memory constrained environment of 512 bytes stack memory with ebpf programs, which felt kind of relevant to me while reading this PR as we've added quite a bit to those programs here. I don't think there's any immediate action to take, as I don't think we're anywhere near in danger of exhausting that limit right now but just something to think about as we continue to add functionality as if we can do some of our optimizations up front we can potentially reduce the size of refactors later.

.github/workflows/test.yaml

config/tests/tcproute-rr/kustomization.yaml

config/tests/tcproute-rr/patch.yaml

dataplane/api-server/src/server.rs

dataplane/ebpf/src/ingress/tcp.rs

dataplane/ebpf/src/utils.rs

internal/dataplane/client/utils.go

shaneutt

/cc @astoycos for additional review.

astoycos

Looks really good @aryan9600!!! Mostly nit's and some questions for yah which might result in some follow up issues

Thanks for all the hard work here!

.github/workflows/test.yaml

astoycos · 2023-12-06T17:12:25Z

dataplane/ebpf/src/egress/tcp.rs

@@ -68,6 +71,39 @@ pub fn handle_tcp_egress(ctx: TcContext) -> Result<i32, i64> {
    unsafe { (*tcp_hdr).check = 0 };

    // TODO: connection tracking cleanup https://github.com/kubernetes-sigs/blixt/issues/85


So the TODO was to have proper conntrack mechanisms, i.e connection tracktion entries should time out at some point if there's and error in the flow etc

dataplane/loader/src/main.rs

dataplane/ebpf/src/egress/tcp.rs

dataplane/common/src/lib.rs

dataplane/ebpf/src/ingress/tcp.rs

dataplane/common/src/lib.rs

astoycos · 2023-12-08T18:17:59Z

dataplane/api-server/src/server.rs

@@ -51,6 +54,40 @@ impl BackendService {
        backends_map.remove(&key)?;
        let mut gateway_indexes_map = self.gateway_indexes_map.lock().await;
        gateway_indexes_map.remove(&key)?;
+
+        // Delete all entries in our tcp connection tracking map that this backend


Do we need to think about adding a "timeout" at some point so that stale threads are automatically pruned?

are you talking about deleting stale TCP connections based on a certain period of inactivity? this got me thinking about whether we should (and how) deal with TCP keepalive?

Correct, I'm not even sure we need that mechanism yet just thinking about it, Let's defer for a follow up.

Add support for round-robin load balancing for `TCPRoute`. This enables mutliple backend references in a UDPRoute object, with traffic being distributed to each backend in a round-robin fashion. A new BPF map `TCP_CONNECTIONS` was introduced to help keep track of active TCP connections, with its key storing the client <ip:port> identifier and the value storing the backend's <ip:port> along with the state of the connection. Signed-off-by: Sanskar Jaiswal <[email protected]>

Signed-off-by: Sanskar Jaiswal <[email protected]>

astoycos

Looks great thanks @aryan9600!

k8s-ci-robot · 2023-12-11T20:33:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aryan9600, astoycos

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [astoycos]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Not applicable anymore

astoycos · 2023-12-11T20:33:48Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 1, 2023

k8s-ci-robot requested review from astoycos and mlavacca December 1, 2023 15:11

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 1, 2023

aryan9600 force-pushed the tcp-rr branch from b6989e7 to e790e9d Compare December 1, 2023 15:32

shaneutt added this to the v0.3.0 - More Fundamentals milestone Dec 1, 2023

shaneutt self-requested a review December 1, 2023 15:43

aryan9600 force-pushed the tcp-rr branch 2 times, most recently from bb64438 to 2f41083 Compare December 4, 2023 17:03

shaneutt previously requested changes Dec 5, 2023

View reviewed changes

shaneutt self-assigned this Dec 5, 2023

shaneutt added area/dataplane area/controlplane area/maintenance priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Dec 5, 2023

kubernetes-sigs deleted a comment from aryan9600 Dec 6, 2023

shaneutt reviewed Dec 6, 2023

View reviewed changes

aryan9600 force-pushed the tcp-rr branch from 2f41083 to 651d029 Compare December 7, 2023 07:18

kubernetes-sigs deleted a comment from aryan9600 Dec 7, 2023

aryan9600 force-pushed the tcp-rr branch from 651d029 to 7ad7bdf Compare December 7, 2023 11:47

aryan9600 requested a review from shaneutt December 7, 2023 11:59

astoycos reviewed Dec 8, 2023

View reviewed changes

aryan9600 force-pushed the tcp-rr branch from 7ad7bdf to 277f483 Compare December 11, 2023 19:40

aryan9600 added 3 commits December 11, 2023 19:50

tcproute: add tests for round-robin load balancing

7065fe4

Signed-off-by: Sanskar Jaiswal <[email protected]>

ci: disable bpfd integration tests due to change upstream

4acd90b

Signed-off-by: Sanskar Jaiswal <[email protected]>

aryan9600 force-pushed the tcp-rr branch from 277f483 to 4acd90b Compare December 11, 2023 19:51

astoycos approved these changes Dec 11, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2023

k8s-ci-robot assigned astoycos Dec 11, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2023

k8s-ci-robot merged commit fa647b5 into kubernetes-sigs:main Dec 11, 2023
11 checks passed

aryan9600 mentioned this pull request Dec 12, 2023

Consolidate connection tracking BPF maps across programs #153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tcproute: add support for round-robin load balancing #148

tcproute: add support for round-robin load balancing #148

aryan9600 commented Dec 1, 2023 •

edited

Loading

shaneutt left a comment •

edited

Loading

shaneutt left a comment •

edited

Loading

astoycos left a comment

astoycos Dec 6, 2023

astoycos Dec 8, 2023

aryan9600 Dec 8, 2023

astoycos Dec 8, 2023

astoycos left a comment

k8s-ci-robot commented Dec 11, 2023

astoycos commented Dec 11, 2023

		@@ -68,6 +71,39 @@ pub fn handle_tcp_egress(ctx: TcContext) -> Result<i32, i64> {
		unsafe { (*tcp_hdr).check = 0 };

		// TODO: connection tracking cleanup https://github.com/kubernetes-sigs/blixt/issues/85

tcproute: add support for round-robin load balancing #148

tcproute: add support for round-robin load balancing #148

Conversation

aryan9600 commented Dec 1, 2023 • edited Loading

shaneutt left a comment • edited Loading

Choose a reason for hiding this comment

shaneutt left a comment • edited Loading

Choose a reason for hiding this comment

astoycos left a comment

Choose a reason for hiding this comment

astoycos Dec 6, 2023

Choose a reason for hiding this comment

astoycos Dec 8, 2023

Choose a reason for hiding this comment

aryan9600 Dec 8, 2023

Choose a reason for hiding this comment

astoycos Dec 8, 2023

Choose a reason for hiding this comment

astoycos left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 11, 2023

astoycos commented Dec 11, 2023

aryan9600 commented Dec 1, 2023 •

edited

Loading

shaneutt left a comment •

edited

Loading

shaneutt left a comment •

edited

Loading