bpf: create and open maps using cilium/ebpf
#22693
Conversation
|
/test |
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
|
This pull request has been automatically marked as stale because it |
github-actions
bot
added
the
stale
|
This pull request has not seen any activity since it was marked stale. |
ti-mo
added
release-note/misc
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
|
@ti-mo Sorry never got around to reviewing this, are you planning to reopen? |
No worries, it's a draft. :) Will pick this up again in a few weeks. |
ti-mo
force-pushed
the
tb/ebpf-maps
branch
3 times, most recently
from
75f2f0e
to
dfe6a86
Compare
|
/test |
|
/test |
|
@YutaroHayakawa I've had to make some changes to the way per-cluster CT maps are loaded from a pin, PTAL. I think pinning inner maps there is not strictly necessary and creates quite some accidental complexity around managing pins. Also, 4 * 256 syscalls to attempt opening all possible pins on every single GC cycle. Map-in-map reads need RCU, but surely a few BPF_MAP_GET_NEXT_KEY calls on the outer map aren't that problematic? At least the latter would just block instead of hammering BPF_OBJ_GETs, leading to lower CPU usage? |
|
/test |
|
/test |
|
/test-1.26-net-next Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed: Click to show.Test NameFailure OutputJenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/79/ If it is a flake and a GitHub issue doesn't already exist to track it, comment Then please upload the Jenkins artifacts to that issue. |
|
net-next is hitting #25524 |
|
Broadly I think the direction that this PR is pushing the codebase is the right path, reducing Cilium-specific syscall related code and relying more on the cilium/ebpf library 🎉 From a quick glance, the |
|
/test Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed: Click to show.Test NameFailure OutputJenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/117/ If it is a flake and a GitHub issue doesn't already exist to track it, comment Then please upload the Jenkins artifacts to that issue. |
ti-mo
added
the
ready-to-merge
|
Marked ready-to-merge as Travis' arm64 job is stalling on |
ti-mo
removed
the
ready-to-merge
When running `V=0 make ... --quiet`, the `go test ./...` executions will never show up, and all output is redirected to /dev/null. This makes it hard to see what's going on. On Travis, this can cause timeouts due to the job emitting no output. Signed-off-by: Timo Beckers <timo@isovalent.com>
We're having issues with Travis jobs timing out due to no output being generated for over 10 minutes. Integration tests run with V=0 and --quiet barely generate any output, since all 'go test' output is hidden, and Makefile steps are also hidden. Signed-off-by: Timo Beckers <timo@isovalent.com>
MapFdFromID was unused in the codebase, remove it. Also remove unused command args to bpf(). The remainder acts as a list of behaviour to port to use cilium/ebpf. Signed-off-by: Timo Beckers <timo@isovalent.com>
Errors returned from OpenOrCreateMap don't necessarily contain any identifying context about the map being created. Wrap all errors encountered during map creation with the map/subsystem name. Signed-off-by: Timo Beckers <timo@isovalent.com>
This commit removes the bool return value from OpenOrCreate() that indicates whether or not a Map was recreated in the process. It was only checked in tests and generally didn't provide much value or actionable feedback, only inflating API surface. Signed-off-by: Timo Beckers <timo@isovalent.com>
This is the first in a series of patches that aims to remove all uses of unix.Syscall in the cilium/cilium codebase in favour of the cilium/ebpf APIs. CreateMap() was unexported and renamed to createMap(). It now calls directly into ebpf.NewMapWithOptions and takes care of passing MapOptions when specified (for passing a PinPath). OpenOrCreateMap() now takes an ebpf.MapSpec and a pinDir and had most of its logic removed as compatibility checks are already taken care of by ebpf-go. NewMap() no longer takes innerID. A new function NewMapWithInnerSpec() was introduced to provide an ebpf.MapSpec to use for the inner map definition during map creation. Signed-off-by: Timo Beckers <timo@isovalent.com>
The innerID argument was only used by 2 maps that needed inner maps. Those were migrated to NewMapWithInnerSpec() in a prior commit, when the argument was already blanked out. This commit removes it. Signed-off-by: Timo Beckers <timo@isovalent.com>
OpenMap() now calls ebpf.LoadPinnedMap and must be given an asbolute path. bpf.ObjGet() was removed as it is now unused. Signed-off-by: Timo Beckers <timo@isovalent.com>
- Remove the bpf.Map.fd field and all accesses to it - Rename bpf.Map.GetFd() to FD() and perform all map FD accesses through it Signed-off-by: Timo Beckers <timo@isovalent.com>
A map is considered in 'parallel' mode when it has been unpinned, recreated and re-pinned, but the old map is still referred to by living endpoint programs until those eventually get phased out by replacing their progs. Rename OpenParallel() to Recreate() as it's a confusing term from a data/bpf standpoint, as there are no double-writes going on. The Map.NonPersistent flag is a deferred way of saying "always remove the map's pin when creating". Remove this bit of state, instead explicitly calling Recreate() for opening the Map. Signed-off-by: Timo Beckers <timo@isovalent.com>
…t map By changing the underlying map abstraction to cilium/ebpf, opening a map from a pin no longer returns an error chain containing a os.PathError, only a unix.ENOENT. Return errors from getClusterMap() instead of a vague 'nil, nil'. Check for ENOENT in getAllClusterMaps() instead of checking the first return value. Stop checking the first return value in multiple places. Also removed redundant mapType argument from PerClusterCTMap.newInnerMap(). Update tests to perform error checking first before accessing any other return value. Signed-off-by: Timo Beckers <timo@isovalent.com>
|
/test |
This PR puts
cilium/ebpf.Mapat the core of Cilium'sbpf.Maptype, see #22513 for more context. Best reviewed per commit given its size. If you've been tagged for review and don't feel comfortable, please let me know so we can reassign. Don't hesitate to ping me offline for discussion or more context.Part of: #22513.