-
Notifications
You must be signed in to change notification settings - Fork 715
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
link: Allow kprobe multi to be disabled in kernel #812
Conversation
51066fa
to
7c6a2b2
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.
shouldn't the if err != nil
check be left here?
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.
🤦 Yes, my bad. Good catch. Fixed.
7c6a2b2
to
e6fba81
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.
Thanks! You're hitting this when trying to run the tests on your laptop?
link/kprobe_multi.go
Outdated
@@ -138,7 +138,7 @@ func (kml *kprobeMultiLink) Unpin() error { | |||
return fmt.Errorf("unpin kprobe_multi: %w", ErrNotSupported) | |||
} | |||
|
|||
var haveBPFLinkKprobeMulti = internal.FeatureTest("bpf_link_kprobe_multi", "5.18", func() error { | |||
var haveBPFLinkKprobeMulti = internal.FeatureTest("bpf_link_kprobe_multi", "0.0", func() error { |
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.
I think this version number should stay, otherwise our CI will miss this test failing.
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.
If we don't remove the version number, tests fail for me with:
Feature 'bpf_link_kprobe_multi' isn't supported even though kernel v5.19.11 is newer than v5.18
Maybe we can add some magic (-X
define or a build tag) so the version number checks happen in CI tests, but not locally? I can give it a go if that sounds sane.
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.
Hmm, I'd like to keep the default (= when running locally) to throw an error when this happens. Otherwise developing a new feature test becomes cumbersome.
How about an environment variable that contains separated feature test names for which to ignore an error result? Basically, in
ebpf/internal/testutils/feature.go
Lines 58 to 62 in 8fceee5
if ufe.MinimumVersion.Less(kernelVersion) { | |
tb.Helper() | |
tb.Fatalf("Feature '%s' isn't supported even though kernel %s is newer than %s", | |
ufe.Name, kernelVersion, ufe.MinimumVersion) | |
} |
tb.Name()
against an env var and call tb.Skip()
. Brownie points if we only split the env var once or so.
This means you have to opt in to ignore the result, but you can stick that variable in your .bashrc
or something. If your kernel changes and the test automagically isn't skipped anymore.
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.
I also think the version number should stay, Build tags are a hard nack from me, and ldflags are quite cumbersome to specify.
We've also recently adopted envs in cilium/cilium
to gate test behaviours. All in all, I think it's the lesser evil, because they can be set ambiently and are easy to slap onto any command, as there might be arbitrary tools/scripts in between your shell and the actual test binaries reading the env.
I'd probably go with a single env here (like EBPF_TEST_IGNORE_KERNEL_VERSION
?) that basically trusts the verdict of our haveBPFXYZ()
functions and doesn't execute the kernel version check. We could add this kind of skip in testutils.CheckFeatureTest()
or testutils.checkKernelVersion()
, although the latter might cause more unintended side effects.
As long as the default doesn't change and we don't relax CI.
Yup I'm hitting it locally on my laptop with the stock debian sid kernel (5.19.11 right now). |
Kprobe multi support is gated on CONFIG_FPROBE, without it creating the link fails with EOPNOTSUPP: https://elixir.bootlin.com/linux/v6.0.3/source/include/linux/fprobe.h#L64 Unfortunately the standard Debian sid kernel does not have it enabled. Update the kprobe multi test to handle EOPNOTSUPP. Co-developed-by: Lorenz Bauer <i@lmb.io>
I've dropped the minimum version change, and used
|
Kprobe multi support is gated on CONFIG_FPROBE, without it creating the link fails with ENOTSUP: https://elixir.bootlin.com/linux/latest/source/include/linux/fprobe.h#L56
Unfortunately the standard Debian sid kernel does not have it enabled.
Update the kprobe multi test to handle ENOTSUP, and drop the minimum kernel version check, tests otherwise fail with:
Feature 'bpf_link_kprobe_multi' isn't supported even though kernel v5.19.11 is newer than v5.18