-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade Cilium's eBPF library version to 0.16 #4397
Conversation
3d516ee
to
b00d33b
Compare
s/Cilium/Cilium's eBPF library/ |
b00d33b
to
92948b3
Compare
f72e3cd
to
978b517
Compare
I've pushed two different commits to illustrate the points being discussed, but if everything is accepted, I intend to squash-merge them as a single commit (happy to amend the PR for that). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelroquetto how is this solving a bug? You use the libcontainer module in your program and can't update the cilium dep to that version there due to runc using the old one? Or can you clarify that?
If that is the case, too bad cilium doesn't provide a 1.x release :(
} | ||
|
||
if useReplaceProg { | ||
attachProgramOptions.Anchor = link.ReplaceProgram(oldProgs[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not adding the flag too, as it was done before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are now handled by the underlying code and are no longer explicitly allowed. See https://github.com/cilium/ebpf/blob/15d4f245e548eb00383aa88a24da934f0d26fc43/link/program.go#L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. For the next time, pointing this out in the commit/PR makes the review simpler :)
@rata you're spont on! We have a situation in which one of our projects (Grafana Beyla) requires fixes from more recent Cilium's eBPF releases (notoriously cilium/ebpf#1348). We then have another project, Grafana Alloy, which depends both on Beyla and runc, and as you correctly guessed, we can't update the cilium dependency. :( |
(@wildum can perhaps offer further context) |
That's correct, and on a more generic case, I expect all projects that have a dependency to |
The breaking change is from cilium/ebpf#1163 and it first appeared in v0.13.0 (which is probably the reason why dependabot never opened a PR to upgrade it). @cyphar PTAL |
Please squash commits |
d893c36
to
7bf75e1
Compare
@AkihiroSuda done |
@rafaelroquetto due to #4393 merge (which vendored golang.org/x/sys) this needs a rebase. |
7bf75e1
to
c82711c
Compare
e55604b
to
e3a1e1c
Compare
@kolyshkin done - interestingly it no longer complains and requires the change from Thank you all again for taking time to review this. |
Signed-off-by: Rafael Roquetto <[email protected]>
e3a1e1c
to
216175a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; thanks!
@kolyshkin @AkihiroSuda would this PR qualify to be cherry-picked on the 1.1 branch? would love to pay down some debt in kubernetes/kubernetes w.r.t dependencies if we can move up to newer cilium |
Yes, feel free to submit a cherrypick PR |
1.1 still uses go 1.18, so that would not be simple. As of today, I'd rather release 1.2.0-final out the door (and use it in k8s). |
@kolyshkin that sounds awesome! |
It seems that this caused a regression of #3008 because while cilium/ebpf@417f8a2 was handled for the actual implementation of |
Upgrade the cilium version to 0.16 (latest). There are many important fixes in between (e.g. cilium/ebpf#1348) that would be nice to have downstream.
This PR basically updates the version of the cilium package and its dependencies, and fixes libcontainer/cgroups/devices/ebpf_linux.go to reflext the new API.
Disclaimer: I am not well acquainted with the codebase, so apologies in advance for anything stupid I may be doing.
Resolves: #4396