loader: attach programs using tcx #30103

rgo3 · 2024-01-04T13:29:35Z

For more detailed descriptions, please refer to the individual commits.

On a high level, this PR:

attaches TC progs using bpf_link (tcx) respecting upgrade and downgrade paths
uses per-endpoint bpffs dirs, at e.g. /sys/fs/bpf/cilium/endpoints/12345/links/cil_to_container
adds netns-driven tests for attaching skb progs via tcx
adds a Helm flag (enableTXC) to optionally disable the feature to ease integration with other tools

pkg/datapath/loader/netlink.go

rgo3 · 2024-01-05T11:13:25Z

Connectivity tests fail because currently the TCX API in cilium/ebpf doesn't do a feature test, so on kernels without TCX support creating links fails and we don't fall back to the old behavior. Opened cilium/ebpf#1294 to add a feature test in ebpf-go.

The build every commit action will continue to fail because I've separate commits for vendoring the lib changes and fixing the query programs API breakage. I'll keep them separate so the fix can easily be applied to the renovate PR that updates the ebpf-go dependency.

rgo3 · 2024-02-27T00:14:43Z

/test

rgo3 · 2024-02-27T10:46:59Z

/test

ti-mo · 2024-03-06T16:12:46Z

/test

jrfastab · 2024-04-30T16:30:08Z

Some minor comments/questions about the code otherwise lgtm.

learnitall

API changes LGTM, thanks!

This commit adds the necessary infrastructure to attach bpf programs operating on sk_buff using the kernel's new tcx hook. Enabling the functionality in the agent's endpoint attachment path happens in a follow-up commit. Signed-off-by: Robin Gögge <r.goegge@isovalent.com> Co-authored-by: Timo Beckers <timo@isovalent.com>

This commit puts the tcx logic in the endpoint attachment path and gates it behind a new --enable-tcx agent flag. A follow-up commit will use the flag in the Helm charts' configmap. attachSKBProgram() now takes a bool to indicate if the user has requested tcx attachments and seamlessly migrates programs between tcx and legacy attachment modes in both directions. Of course, this process is contingent on no other tcx programs being attached to the interface, as that disables legacy tc execution. Signed-off-by: Timo Beckers <timo@isovalent.com>

This commit adds the 'bpf.enableTCX' Helm value to allow disabling tcx attachments if external tooling integrating with Cilium hasn't caught up yet, as attaching a tcx program to an interface disables the legacy tc ingress/egress pipelines. The agent upgrades and downgrades interfaces seamlessly based on tcx being enabled or not, so any existing workloads are migrated automatically at runtime if the config flag is changed and the agent restarted. Rebooting the node is not necessary. Signed-off-by: Timo Beckers <timo@isovalent.com>

Extend the agent API to indicate whether Cilium is actually using tcx or relying on legacy tc so that this can be displayed in `cilium status`. Status when tcx is active: # kubectl exec cilium-4m7nq -- cilium-dbg status [...] BandwidthManager: Disabled Routing: Network: Tunnel [geneve] Host: Legacy Attach Mode: TCX Masquerading: IPTables [IPv4: Enabled, IPv6: Enabled] [...] Status when inactive: # kubectl exec cilium-4m7nq -- cilium-dbg status [...] BandwidthManager: Disabled Routing: Network: Tunnel [geneve] Host: Legacy Attach Mode: Legacy TC Masquerading: IPTables [IPv4: Enabled, IPv6: Enabled] [...] Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Add a note on tcx for the 1.16 upgrade guide. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

ti-mo · 2024-05-02T09:06:21Z

/test

joestringer · 2024-05-02T20:23:21Z

I wonder if this is something we should elevate to release-note/major given the potential performance impacts. This seems like a great thing to tout in the release blog.

julianwiedmann · 2024-05-03T06:59:24Z

👋 looking at https://github.com/cilium/cilium/actions/runs/8931592322. What's the current upgrade/downgrade impact of this feature - in particular without #32228 backported? Do we need to switch some of the bpf-next downgrade CI tests to bpf.enableTCX=false, until v1.15 is sorted?

borkmann · 2024-05-03T09:14:47Z

👋 looking at https://github.com/cilium/cilium/actions/runs/8931592322. What's the current upgrade/downgrade impact of this feature - in particular without #32228 backported? Do we need to switch some of the bpf-next downgrade CI tests to bpf.enableTCX=false, until v1.15 is sorted?

I just did the backport of mentioned PR in #32337 . The ci-ipsec-upgrade test I've seen sporadically fail on various kernels though, not only bpf-next.

borkmann · 2024-05-03T09:22:10Z

What's the current upgrade/downgrade impact of this feature - in particular without #32228 backported?

Without the backport there is a tiny race for downgrade where tcx gets removed and legacy tc installed so for a short window there is nothing attached. With the backport it's atomic. The upgrade is also atomic, that is, tcx is installed and then tc legacy removed (if tcx is installed successfully it takes precedence and legacy tc is not run). Lets see if the backport helps.

borkmann · 2024-05-03T10:13:20Z

I just did the backport of mentioned PR in #32337 . The ci-ipsec-upgrade test I've seen sporadically fail on various kernels though, not only bpf-next.

(fyi, merged to v1.15 branch now.)

julianwiedmann · 2024-05-03T10:44:57Z

What's the current upgrade/downgrade impact of this feature - in particular without #32228 backported?

Without the backport there is a tiny race for downgrade where tcx gets removed and legacy tc installed so for a short window there is nothing attached. With the backport it's atomic. The upgrade is also atomic, that is, tcx is installed and then tc legacy removed (if tcx is installed successfully it takes precedence and legacy tc is not run). Lets see if the backport helps.

I'm at attempt 7 for the bpf-next downgrade from main to v1.15 in https://github.com/cilium/cilium/actions/runs/8927670014, which includes the txc work. And as the workflow is meant to be sensitive enough for such tiny blips, I'm very inclined to blame this race window ;).

Can we please flip the default to bpf.enableTCX=false, until the next v1.15 patch release is through and we can offer a smooth downgrade?

borkmann · 2024-05-03T12:25:34Z

What's the current upgrade/downgrade impact of this feature - in particular without #32228 backported?

Without the backport there is a tiny race for downgrade where tcx gets removed and legacy tc installed so for a short window there is nothing attached. With the backport it's atomic. The upgrade is also atomic, that is, tcx is installed and then tc legacy removed (if tcx is installed successfully it takes precedence and legacy tc is not run). Lets see if the backport helps.

I'm at attempt 7 for the bpf-next downgrade from main to v1.15 in https://github.com/cilium/cilium/actions/runs/8927670014, which includes the txc work. And as the workflow is meant to be sensitive enough for such tiny blips, I'm very inclined to blame this race window ;).

Can we please flip the default to bpf.enableTCX=false, until the next v1.15 patch release is through and we can offer a smooth downgrade?

Ah yes, I thought the downgrade tests would test against latest v1.15 branch, not patch release. Cc @ti-mo lets default to false until then wdyt?

borkmann · 2024-05-03T13:45:01Z

(PR is here for checking #32342 )

rgo3 · 2024-05-03T15:38:13Z

@julianwiedmann I've retriggered the job you referenced previously and it is now passing: https://github.com/cilium/cilium/actions/runs/8927670014/job/24559664765. Your 7 runs prior to that pulled a v1.15 image before we merged the seamless downgrade path 4h ago. So I don't think bpf.enableTCX=false is necessary anymore. I'll also rerun it a couple times more to gather some additional data points.

julianwiedmann · 2024-05-03T16:22:23Z

@julianwiedmann I've retriggered the job you referenced previously and it is now passing: https://github.com/cilium/cilium/actions/runs/8927670014/job/24559664765. Your 7 runs prior to that pulled a v1.15 image before we merged the seamless downgrade path 4h ago. So I don't think bpf.enableTCX=false is necessary anymore. I'll also rerun it a couple times more to gather some additional data points.

🤷 I give up on understanding the downgrade tests. So the minor downgrade is running against the previous minor's branch tip, and not the previous minor's current release. And from a quick git archaeology, that was always the case? Then we should be good indeed. Sorry for the noise. Thank you @rgo3 for keeping me honest!

rgo3 · 2024-05-03T16:31:15Z

I feel the same 😅.

But yes, I believe and have seen in the CI logs that we install previous minor's branch tip, in this case v1.15. To me that also makes the most sense for the minor tests, given the previous minor's branch tip will be the code we support minor downgrades to once released. Otherwise, we would have to backport things, wait until it's merged and released, and only then can move forward with the PR for main (something we did in the past, including this PR), which always felt quite tedious to me.

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 4, 2024

rgo3 commented Jan 4, 2024

View reviewed changes

pkg/datapath/loader/netlink.go Outdated Show resolved Hide resolved

rgo3 force-pushed the tcx-for-cilium branch from 87dec5f to bab617b Compare January 10, 2024 10:49

rgo3 force-pushed the tcx-for-cilium branch from bab617b to ef36b81 Compare January 29, 2024 18:05

rgo3 force-pushed the tcx-for-cilium branch from ef36b81 to bb62443 Compare February 27, 2024 00:08

rgo3 changed the title ~~Tcx for cilium~~ loader: attach TC programs using bpf_link/TCX Feb 27, 2024

rgo3 force-pushed the tcx-for-cilium branch 2 times, most recently from 07ae5db to 876668e Compare February 27, 2024 10:02

ti-mo force-pushed the tcx-for-cilium branch from 876668e to 526eb19 Compare March 6, 2024 16:08

ti-mo added sig/loader Impacts the loading of BPF programs into the kernel. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Mar 6, 2024

maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 6, 2024

maintainer-s-little-helper bot added this to Needs backport from main in 1.15.2 Mar 6, 2024

maintainer-s-little-helper bot added this to Needs backport from main in 1.13.13 Mar 6, 2024

maintainer-s-little-helper bot added this to Needs backport from main in 1.14.8 Mar 6, 2024

ti-mo changed the title ~~loader: attach TC programs using bpf_link/TCX~~ loader: attach programs using tcx Mar 6, 2024

jrajahalme removed this from Needs backport from main in 1.15.2 Mar 13, 2024

thorn3r removed this from Needs backport from main in 1.13.13 Mar 13, 2024

thorn3r added this to Needs backport from main in 1.13.13 Mar 13, 2024

thorn3r added this to Needs backport from main in 1.13.14 Mar 13, 2024

learnitall approved these changes Apr 30, 2024

View reviewed changes

lambdanis approved these changes May 1, 2024

View reviewed changes

tklauser approved these changes May 2, 2024

View reviewed changes

rgo3 and others added 5 commits May 2, 2024 11:05

cilium, docs: Add note to upgrade guide on tcx

598c933

Add a note on tcx for the 1.16 upgrade guide. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

ti-mo force-pushed the tcx-for-cilium branch from dec2f6a to 598c933 Compare May 2, 2024 09:05

ti-mo enabled auto-merge May 2, 2024 09:08

ti-mo added this pull request to the merge queue May 2, 2024

Merged via the queue into cilium:main with commit 3f22794 May 2, 2024
63 of 64 checks passed

ti-mo deleted the tcx-for-cilium branch May 2, 2024 10:21

borkmann added release-note/major This PR introduces major new functionality to Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels May 3, 2024

julianwiedmann mentioned this pull request May 3, 2024

CI: test-conn-disrupt-client failed due to interrupted traffic during upgrade/downgrade #28088

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loader: attach programs using tcx #30103

loader: attach programs using tcx #30103

rgo3 commented Jan 4, 2024 •

edited by ti-mo

Loading

rgo3 commented Jan 5, 2024

rgo3 commented Feb 27, 2024

rgo3 commented Feb 27, 2024

ti-mo commented Mar 6, 2024

jrfastab commented Apr 30, 2024

learnitall left a comment

ti-mo commented May 2, 2024

joestringer commented May 2, 2024

julianwiedmann commented May 3, 2024 •

edited

Loading

borkmann commented May 3, 2024

borkmann commented May 3, 2024

borkmann commented May 3, 2024

julianwiedmann commented May 3, 2024 •

edited

Loading

borkmann commented May 3, 2024

borkmann commented May 3, 2024 •

edited

Loading

rgo3 commented May 3, 2024 •

edited

Loading

julianwiedmann commented May 3, 2024

rgo3 commented May 3, 2024

loader: attach programs using tcx #30103

loader: attach programs using tcx #30103

Conversation

rgo3 commented Jan 4, 2024 • edited by ti-mo Loading

rgo3 commented Jan 5, 2024

rgo3 commented Feb 27, 2024

rgo3 commented Feb 27, 2024

ti-mo commented Mar 6, 2024

jrfastab commented Apr 30, 2024

learnitall left a comment

Choose a reason for hiding this comment

ti-mo commented May 2, 2024

joestringer commented May 2, 2024

julianwiedmann commented May 3, 2024 • edited Loading

borkmann commented May 3, 2024

borkmann commented May 3, 2024

borkmann commented May 3, 2024

julianwiedmann commented May 3, 2024 • edited Loading

borkmann commented May 3, 2024

borkmann commented May 3, 2024 • edited Loading

rgo3 commented May 3, 2024 • edited Loading

julianwiedmann commented May 3, 2024

rgo3 commented May 3, 2024

rgo3 commented Jan 4, 2024 •

edited by ti-mo

Loading

julianwiedmann commented May 3, 2024 •

edited

Loading

julianwiedmann commented May 3, 2024 •

edited

Loading

borkmann commented May 3, 2024 •

edited

Loading

rgo3 commented May 3, 2024 •

edited

Loading