bpf: switch to cilium/ebpf loader and native netlink calls #19159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
ti-mo
added
the
release-note/misc
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
ti-mo
added
sig/loader
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
|
/test |
|
/test |
|
/test |
|
Build finished. |
|
/test |
|
/test |
|
/test |
|
/test |
ti-mo
force-pushed
the
tb/ebpf-go
branch
2 times, most recently
from
aaf8fc2
to
4edb2f0
Compare
|
/test |
|
/test Job 'Cilium-PR-K8s-1.22-kernel-5.4' failed: Click to show.Test NameFailure OutputIf it is a flake and a GitHub issue doesn't already exist to track it, comment |
|
/test Job 'Cilium-PR-K8s-GKE' failed: Click to show.Test NameFailure OutputIf it is a flake and a GitHub issue doesn't already exist to track it, comment |
joestringer
requested changes
Jul 22, 2022
There was a problem hiding this comment.
The last commit seems to be rewriting FunctionQueue, which I don't think necessarily needs to happen to this degree but I guess it's fine. That said, there's a few things that could be improved with the new API, which should be fixed up if you are planning to rewrite that structure. Minor stuff, comments below.
I noticed a couple of odd things about the logging usage and errors which I think is important to address just to ensure that we don't start hiding real errors in production environments. I tried to highlight them to the best of my ability, hopefully it's not too much effort to try out those changes and see whether they break any expectations in the CI.
I didn't look too closely at the earlier commits since I had previously seen them and they seemed fine at that point.
As documented in the code, this patch adds wrappers around LoadCollectionSpec() and LoadCollection() to package bpf. For compatibility with iproute2 bpf_elf_map definitions, iproute2Compat() parses the unsupported extra tail bytes and interprets them to set the pinning flag and to attribute 'X/Y' sections containing tail calls to their respective map IDs and the indices within them. classifyProgramTypes() automatically detects attach types of Cilium's ELFs based on the names of the programs it contains. Signed-off-by: Timo Beckers <timo@isovalent.com>
inlineGlobalData() was added to inline map loads from .data into the bytecode in userspace. Once all references have been inlined, the map is removed from the spec to skip loading it into the kernel. Later on, global constants can be declared const so they end up in .rodata, where they can be found by CollectionSpec.RewriteConstants, which will replace the ELF templating logic. Signed-off-by: Timo Beckers <timo@isovalent.com>
This commit mainly modifies replaceDatapath() to invoke the cilium/ebpf ELF loader instead of shelling out to iproute2's tc and ip commands. Note that this patch focuses on a 1:1 conversion to minimize churn and to keep it reasonably reviewable. Many changes of larger magnitude are planned to make the datapath and loader provide an actual API that will serve as a foundation for testing infrastructure and higher-level packages. The goal here is to get the ball rolling. Programs are no longer being referred to using their ELF section names, but using their C function names instead. Changes to init.sh are notably excluded because they require additional care and attention, as well as adding CLI commands to the agent that wrap the Go loader code to replace tc/ip invocations. This will be tackled in follow-ups. `xdp bool` was removed from replaceDatapath(). All uses of xdp/xdpMode were either 'true, xdpMode' or 'false, ""', so this was reduced to a single flag xdpMode that is acted upon when non-zero. This simplifies logic and doesn't allow both parameters to contradict each other. Signed-off-by: Timo Beckers <timo@isovalent.com>
Start/FinalizeBPFFSMigration now take a *ebpf.CollectionSpec, which should be obtained using bpf.LoadCollectionSpec. This is a wrapper that contains iproute2 compatibility logic, including draining MapSpec.Extra, so this is no longer necessary. Signed-off-by: Timo Beckers <timo@isovalent.com>
Previously, different call sites emitted the same warning message, making it difficult to trace back where the error originated from. Signed-off-by: Timo Beckers <timo@isovalent.com>
With the addition of Go code that loads and attaches BPF programs, we're no longer using the BPF file/section as the tc filter name. Assume the filter names can also contain 'cilium'. Signed-off-by: Timo Beckers <timo@isovalent.com>
The context passed to ParseExternalRegenerationMetadata, which is the one that ends up propagating all the way down to ReloadDatapath and friends, can be Endpoint.aliveCtx if the ExternalRegenerationMetadata does not contain a parent context. Make sure this context is cancelled early on in the Endpoint teardown process to allow downstream logic (bpf prog load, netlink attach, ..) to bail out instead of trying to attach to resources that are already removed by the container runtime. Signed-off-by: Timo Beckers <timo@isovalent.com>
ctx is the canonical name of a context given to a Go function. Change this to make follow-up commits shorter. Signed-off-by: Timo Beckers <timo@isovalent.com>
When the CNI plugin gives up on creating an Endpoint, or more generally, when a Context passed into a loader function gets canceled, many functions in the call chain will return error. To avoid logging these errors as they occur in each part of the call chain, only emit them when they don't contain a context.Canceled. Exceeded deadlines should still be logged. Aside from cleaning up the logs, this also causes the CI log scanner to not trip over so many potential `level=error` lines that occur due to (CPU) resource constraints when compiling/loading/attaching many endpoint progs. Signed-off-by: Timo Beckers <timo@isovalent.com>
When multiple Endpoints with the same template config are being (re)generated, the 'active' one (with an in-flight clang invocation) might receive a context cancellation. When this happens, all other 'waiting' Endpoints would see fq.stopCh being closed, and since they all have different (non-cancelled) Contexts, they would all return from fq.Wait() without error. This results in many 'Could not locate previously compiled BPF template' errors in CI. This patch changes error handling by returning the error from the enqueued function instead of checking ctx.Err() explicitly in fq.Wait(). Wait() no longer takes a context since selecting on its done channel races with the write to fq.err. Signed-off-by: Timo Beckers <timo@isovalent.com>
This commit removes the serializer's retry behavior since it was left unused. The only queue that was ever created only had a single slot, and the only user of the queue (fetchOrCompile) would unconditionally stop the queue. In other words: a 1-slot queue that will only ever be used to process a single event. This aims to simplify the implementation so it can eventually be replaced by sync.Once or singleflight, which serve a similar purpose. Signed-off-by: Timo Beckers <timo@isovalent.com>
|
/test |
joestringer
approved these changes
Jul 27, 2022
ti-mo
added
the
ready-to-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description taken from the main commit. Best reviewed per commit.