-
Notifications
You must be signed in to change notification settings - Fork 167
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
chore(pkg/bpf): Replace libbpfgo with cilium/ebpf #1438
base: main
Are you sure you want to change the base?
Conversation
5fa556a
to
42303d8
Compare
@rootfs @dave-tucker which release is this targeted for? |
I don't have a target in mind. But I doubt it will be ready for review until Friday at the earliest. |
08d0a91
to
d031ddd
Compare
I thought I was close, but I think I found an issue in #1443 that causes failures when perf events can't be opened for some reason. I've cherry-picked that here to see if it makes CI green - it applies to the libbpfgo code to so it's another PR. |
🤖 SeineSailor Here's a concise summary of the pull request changes: Key Modifications:
Impact on Codebase:
Observations and Suggestions:
Overall, this pull request appears to be a significant refactoring effort to modernize the Kepler project's dependencies and internal implementation. A thorough review and testing of the changes are necessary to ensure the codebase remains stable and maintainable. |
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!
@dave-tucker we may want to look at this crash before merging.
|
I get the same using the
|
@sthaha I just force pushed to fix a small bug in the cleanup logic (forgetting to clear the I've tried locally on Fedora 40 with both running the binary and compose and can't reproduce. I've also tried on RHEL 9.4 and RHEL 9.2 and still can't reproduce. I'm also not seeing anything in the logs stating that any of the eBPF probes couldn't be loaded 🤔 TL;DR I'm a bit stuck on fixing this as I can't reproduce the issue. |
I tried with the latest, commit 6d8539b and runs fine. |
This is ready to go. I'm holding off doing a rebase though until #1481 has merged so marking this as a draft until then. |
klog.Infof("Number of CPUs: %d", numCPU) | ||
for _, m := range specs.Maps { | ||
// Only resize maps that have a MaxEntries of NUM_CPUS constant | ||
if m.MaxEntries == 128 { |
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.
can we check condition by map name prefix if not by map name? the size comparison does not reflect why are we resizing the map as per number of cpus.
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.
Well, 128 is indeed the magic NUM_CPUS
constant that we define on the eBPF side.
If you use NUM_CPUS
as your max-entries value, the contract is that userspace will resize your map to the number of CPUs.
I prefer this to keeping a list of map names and/or prefixes around which could get stale
Snyk checks are failing, but not due to changes in this PR: That comes in via: $ go mod graph | grep gopkg.in/yaml.v1
howett.net/plist@v1.0.0 gopkg.in/yaml.v1@v1.0.0-20140924161607-9f9df34309c0 Which is used by: $ go mod why howett.net/plist
# howett.net/plist
github.com/sustainable-computing-io/kepler/cmd/validator
github.com/jaypipes/ghw
github.com/jaypipes/ghw/pkg/block
howett.net/plist The other issues are either in:
|
I can't see the details of the failure ... but, I wonder why the other PRs pass CI while this fails 🤔 |
I think anything that touches |
@dave-tucker can you sync this PR with main? |
Done! |
d64c96f
to
1aac060
Compare
5fa77d5
to
138a422
Compare
LGTM |
274f339
to
a00fa0d
Compare
cilium/ebpf is a pure Go eBPF package and is used in a number of popular cloud-native projects. The benefits to Kepler are: 1. Bytecode is built using bpf2go and the C and Go structs are kept in sync automatically 2. There is no requirement for Cgo anymore and therefore no requirement to have libbpf or libelf installed to compile and/or to be dynamically linked at runtime 3. Simplified packaging as the correct bytecode is contained within the kepler binary Overall I'm happy with this change, but there is only one thing that bugs me. We have to check in the bytecode object files (e.g kepler.bpfel.o) or the Go tooling (go lint/go vet) complains about the missing files. I couldn't reliably get `go generate ./...` to work to compile these files in CI. This is something which should be relatively easy to fix in the Makefile/CI environment before we cut a release. Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
cilium/ebpf is a pure Go eBPF package and is used in a number of popular
cloud-native projects. The benefits to Kepler are:
sync automatically
to have libbpf or libelf installed to compile and/or to be
dynamically linked at runtime
kepler binary
Overall I'm happy with this change, but there is only one thing that
bugs me.
We have to check in the bytecode object files (e.g kepler.bpfel.o) or
the Go tooling (go lint/go vet) complains about the missing files.
I couldn't reliably get
go generate ./...
to work to compile thesefiles in CI. This is something which should be relatively easy to fix
in the Makefile/CI environment before we cut a release.
Depends-On: #1435
Fixes: #1424