Skip to content
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

bcc: 0.24.0 -> 0.25.0; bpftrace: 0.15.0 -> 0.16.0 #189078

Merged
merged 6 commits into from
Dec 4, 2022

Conversation

martinetd
Copy link
Member

Description of changes

upgrade to latest bcc/bpftrace (bcc upgrade breaks older bpftrace, so PR grouped together)

I've tested the tools still work with default llvm 11, but it's old so upgrading it makes more sense to have better tested code paths

I've also tested the PR with #187978 (libbpf 1.0) -- the tools work with either version of libbpf

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@superherointj superherointj changed the title bcc 0.24.0 -> 0.25.0 / bpftrace 0.15.0 -> 0.16.0 bcc: 0.24.0 -> 0.25.0; bpftrace: 0.15.0 -> 0.16.0 Aug 31, 2022
@Mic92
Copy link
Member

Mic92 commented Sep 1, 2022

@ofborg build bpftrace linuxPackages.oci-seccomp-bpf-hook

@Mic92
Copy link
Member

Mic92 commented Sep 1, 2022

It appears that bpftrace patches no longer applies: https://logs.nix.ci/?attempt_id=ab0981dc-bf6d-478a-a0b7-82fad974f116&key=nixos%2Fnixpkgs.189078

...
...
/nix/store/drg8q3m0qkr5dkmdvzd9xvxvgmbbacmg-go-md2man-2.0.2/bin/go-md2man -in oci-seccomp-bpf-hook.md -out oci-seccomp-bpf-hook.1
make[1]: Leaving directory '/build/source/docs'
GO111MODULE=on go build -mod=vendor -mod=vendor -o bin/oci-seccomp-bpf-hook -ldflags "-X main.version=v1.2.6" github.com/containers/oci-seccomp-bpf-hook
unpacking sources
unpacking source archive /nix/store/qhfambdxj83m1bc240c87bkf30m5pjzv-source
source root is source
patching sources
applying patch /nix/store/fxvvv7qqkpgrmw6mz0ssf3r8ly84jpky-binutils-2.39.patch
patching file CMakeLists.txt
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file CMakeLists.txt.rej
patching file cmake/FindLibBfd.cmake
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file cmake/FindLibBfd.cmake.rej
patching file src/bfd-disasm.cpp
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
2 out of 2 hunks ignored -- saving rejects to file src/bfd-disasm.cpp.rej
# github.com/iovisor/gobpf/bcc
vendor/github.com/iovisor/gobpf/bcc/module.go:230:132: not enough arguments in call to (_C2func_bcc_func_load)
        have (unsafe.Pointer, _Ctype_int, *_Ctype_char, *_Ctype_struct_bpf_insn, _Ctype_int, *_Ctype_char, _Ctype_uint, _Ctype_int, *_Ctype_char, _Ctype_uint, nil)
        want (unsafe.Pointer, _Ctype_int, *_Ctype_char, *_Ctype_struct_bpf_insn, _Ctype_int, *_Ctype_char, _Ctype_uint, _Ctype_int, *_Ctype_char, _Ctype_uint, *_Ctype_char, _Ctype_int)
make: *** [Makefile:49: binary] Error 2
error: builder for '/nix/store/mjwyx1afanfnypamvmh4d70497cs3iz6-oci-seccomp-bpf-hook-1.2.6.drv' failed with exit code 2;
       last 10 log lines:
       > make -C docs PREFIX=/usr/local
       > make[1]: Entering directory '/build/source/docs'
       > /nix/store/drg8q3m0qkr5dkmdvzd9xvxvgmbbacmg-go-md2man-2.0.2/bin/go-md2man -in oci-seccomp-bpf-hook.md -out oci-seccomp-bpf-hook.1
       > make[1]: Leaving directory '/build/source/docs'
       > GO111MODULE=on go build -mod=vendor -mod=vendor -o bin/oci-seccomp-bpf-hook -ldflags "-X main.version=v1.2.6" github.com/containers/oci-seccomp-bpf-hook
       > # github.com/iovisor/gobpf/bcc
       > vendor/github.com/iovisor/gobpf/bcc/module.go:230:132: not enough arguments in call to (_C2func_bcc_func_load)
       >     have (unsafe.Pointer, _Ctype_int, *_Ctype_char, *_Ctype_struct_bpf_insn, _Ctype_int, *_Ctype_char, _Ctype_uint, _Ctype_int, *_Ctype_char, _Ctype_uint, nil)
       >    want (unsafe.Pointer, _Ctype_int, *_Ctype_char, *_Ctype_struct_bpf_insn, _Ctype_int, *_Ctype_char, _Ctype_uint, _Ctype_int, *_Ctype_char, _Ctype_uint, *_Ctype_char, _Ctype_int)
       > make: *** [Makefile:49: binary] Error 2
       For full logs, run 'nix log /nix/store/mjwyx1afanfnypamvmh4d70497cs3iz6-oci-seccomp-bpf-hook-1.2.6.drv'.
error: builder for '/nix/store/afhmbqbf5q3rd9b45i7gir8f7p7vj0g9-bpftrace-0.16.0.drv' failed with exit code 1;
       last 10 log lines:
       > patching file cmake/FindLibBfd.cmake
       > Reversed (or previously applied) patch detected!  Assume -R? [n]
       > Apply anyway? [n]
       > Skipping patch.
       > 1 out of 1 hunk ignored -- saving rejects to file cmake/FindLibBfd.cmake.rej
       > patching file src/bfd-disasm.cpp
       > Reversed (or previously applied) patch detected!  Assume -R? [n]
       > Apply anyway? [n]
       > Skipping patch.
       > 2 out of 2 hunks ignored -- saving rejects to file src/bfd-disasm.cpp.rej
       For full logs, run 'nix log /nix/store/afhmbqbf5q3rd9b45i7gir8f7p7vj0g9-bpftrace-0.16.0.drv'.
error: build of '/nix/store/afhmbqbf5q3rd9b45i7gir8f7p7vj0g9-bpftrace-0.16.0.drv', '/nix/store/mjwyx1afanfnypamvmh4d70497cs3iz6-oci-seccomp-bpf-hook-1.2.6.drv' failed
       > Apply anyway? [n],       > Skipping patch.,       > 1 out of 1 hunk ignored -- saving rejects to file cmake/FindLibBfd.cmake.rej,       > patching file src/bfd-disasm.cpp,       > Reversed (or previously applied) patch detected!  Assume -R? [n],       > Apply anyway? [n],       > Skipping patch.,       > 2 out of 2 hunks ignored -- saving rejects to file src/bfd-disasm.cpp.rej,       For full logs, run 'nix log /nix/store/afhmbqbf5q3rd9b45i7gir8f7p7vj0g9-bpftrace-0.16.0.drv'.,error: build of '/nix/store/afhmbqbf5q3rd9b45i7gir8f7p7vj0g9-bpftrace-0.16.0.drv', '/nix/store/mjwyx1afanfnypamvmh4d70497cs3iz6-oci-seccomp-bpf-hook-1.2.6.drv' failed

Also linuxPackages.oci-seccomp-bpf-hook seems to be no longer compatible with bcc. Is there maybe an upstream fix / new release for that?

@martinetd
Copy link
Member Author

It appears that bpftrace patches no longer applies: https://logs.nix.ci/?attempt_id=ab0981dc-bf6d-478a-a0b7-82fad974f116&key=nixos%2Fnixpkgs.189078

ah, I had missed the binutils patch had already been merged as I'm on slightly older tree to spare my tiny nix-store disk for tests... And it obviously didn't merge conflict... Thanks for the heads up.

Also made me take another look and we no longer need the mass rename for man pages, and quite some more cleanup was possible so here's an update for this.

I've never looked at linuxPackages.oci-seccomp-bpf-hook before, I'll check what changed later tonight.

@martinetd martinetd force-pushed the bpftrace branch 2 times, most recently from adf0131 to 6267b0d Compare September 1, 2022 08:06
@martinetd
Copy link
Member Author

I've never looked at linuxPackages.oci-seccomp-bpf-hook before, I'll check what changed later tonight.

vendor/github.com/iovisor/gobpf/bcc/module.go:230:132: not enough arguments in call to (_C2func_bcc_func_load)

Ok so that's the same problem bpftrace 0.15.0 had, bcc changed the arguments to bcc_func_load() between 0.24 and 0.25.
Something similar to bpftrace/bpftrace#2340 (comment) can be done to keep compat or we can just convert it.

containers/oci-seccomp-bpf-hook#100 has been opened there I'll help a bit on it...

@Mic92
Copy link
Member

Mic92 commented Sep 5, 2022

I've never looked at linuxPackages.oci-seccomp-bpf-hook before, I'll check what changed later tonight.

vendor/github.com/iovisor/gobpf/bcc/module.go:230:132: not enough arguments in call to (_C2func_bcc_func_load)

Ok so that's the same problem bpftrace 0.15.0 had, bcc changed the arguments to bcc_func_load() between 0.24 and 0.25. Something similar to iovisor/bpftrace#2340 (comment) can be done to keep compat or we can just convert it.

containers/oci-seccomp-bpf-hook#100 has been opened there I'll help a bit on it...

So should we wait a bit with the upgrade? Looks like iovisor/gobpf#311 might have chance of being merged soon.

@martinetd
Copy link
Member Author

So should we wait a bit with the upgrade? Looks like iovisor/gobpf#311 might have chance of being merged soon.

Yeah, this isn't in any real hurry -- after looking there really isn't much to do that isn't either already done or horrible, so I've subscribed to both this and the oci-seccomp-bpf-hook issues and will update this when either have moved.

There doesn't seem to be any "on hold" kind of label, if I missed it feel free to add it :)

(and I'll remove the fetchpatch input as pointed out by @SuperSandro2000 with the hook fix)

@SuperSandro2000
Copy link
Member

(and I'll remove the fetchpatch input as pointed out by @SuperSandro2000 with the hook fix)

you didn't push it though

@martinetd
Copy link
Member Author

There's no hook fix yet. I don't see any point in making ofborg rebuild everything for something we're not merging.

@martinetd
Copy link
Member Author

Updated to also use libbpf_1 ; didn't retest but iirc I tested back then and it should be ok.

the gobpf issue hasn't moved in over a month though with no comment from the maintainer (schu) at all, and no new commits for over a year.
I'm not sure what we can do at this point, I'd rather not duplicate the bcc package as well... So I guess this PR is going to stay on hold for an indefinite amount of time :(

@martinetd
Copy link
Member Author

(marked as draft until oci-seccomp-bpf-hook gets fixed -- the libbpfgo issue got fixed so just need that update used now...)

@martinetd martinetd marked this pull request as ready for review November 5, 2022 01:39
@ofborg ofborg bot requested a review from saschagrunert November 5, 2022 01:43
@martinetd
Copy link
Member Author

I got oci-seccomp-bpf-hook updated with the unreleased gobpf, and they released a new version with that -- updating it in nixpkgs at least builds but I have no way of checking. @saschagrunert could you give it a spin? afaik the update could be done separately as unlike bpftrace the newer oci-seccomp-bpf-hook should work with bcc 0.24; happy to split the PR if that helps but it'll need checking with new bcc anyway...

Note I also moved it away from linuxPackages -- it doesn't depend on kernel (I assume that's a leftover from when bcc used to); I've added an inherit like bcc/bpftrace for backwards compatibility. I don't really care about this so happy to drop that commit if someone has strong feelings either way.

With this I see no problem with bcc 0.25, so I've removed the draft.

@saschagrunert
Copy link
Member

@martinetd the hook should work with bcc 0.25.0: containers/oci-seccomp-bpf-hook#104

I had no chance to test this out yet, though.

@martinetd
Copy link
Member Author

Well, yes, I'm the author of that PR ;) But I don't have any setup to test/have no idea how the hook should actually work...

From the "change default to libbpf_1" draft PR discussion it doesn't look like we'll be moving this forward before the 22.11 branch, so we've got a bit of time here to test this if you can/have time.
Otherwise I guess we can just merge this after the branching and trust the code change works.

@saschagrunert
Copy link
Member

Well, yes, I'm the author of that PR ;)

Oops, I did not notice that! 😁

Otherwise I guess we can just merge this after the branching and trust the code change works.

Yes, I think that should be feasible in this case.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
note bcc 0.25.0 breaks bpftrace 0.15.0, so this needs to be together
with bcc upgrade
the default llvm on linux is llvm11, which works but is old and has some deprecation
warnings when building bcc (and possibly some features disabled for bpftrace).

Just use the latest current version
fixes compatibility with bcc 0.25
oci-seccomp-bpf-hook does not depend on kernel and does not need to be a
linuxPackage attribute.

keep inherited attribute for backwards compability
at the moment the used go-epbf version not compatible with our bcc version.
@ofborg ofborg bot requested a review from flokli December 4, 2022 15:20
@Mic92 Mic92 merged commit a587815 into NixOS:master Dec 4, 2022
@martinetd
Copy link
Member Author

Thanks! Hadn't realized grafana also had an ebpf exporter mode...

(I've now rebased #199830 making libbpf_1 the default and marked it ready)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants