ebpf icon indicating copy to clipboard operation
ebpf copied to clipboard

allow silencing feature test errors

Open lmb opened this issue 2 years ago • 1 comments

@arthurfabre reports that he'd like to be able to silence certain feature test errors, since his host kernel doesn't support CONFIG_FPROBE for example.

I'd like to keep the default behaviour of feature test tests (= when running locally) to throw an error. 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 https://github.com/cilium/ebpf/blob/8fceee5ca41b47a59739a14e2f6877886dc89023/internal/testutils/feature.go#L58-L62 check 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.

Originally posted by @lmb in https://github.com/cilium/ebpf/pull/812#discussion_r990077159

lmb avatar Oct 21 '22 11:10 lmb

Originally posted by @ti-mo in https://github.com/cilium/ebpf/pull/812#discussion_r994251090

[...] 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.

lmb avatar Oct 21 '22 11:10 lmb

@lmb I would like to work on this improvement.

I also feel testutils.CheckFeatureTest is better place to make this change. How about keeping EBPF_TEST_IGNORE_KERNEL_VERSION as list of feature names ( snake cased + lowercase ) to ignore so that multiple features can be silenced. eg: EBPF_TEST_IGNORE_KERNEL_VERSION="bpf_link_kprobe_multi,socket_filter".

abhipranay avatar Apr 25 '23 13:04 abhipranay

Hey, thanks for taking some time to work on this!

Changing CheckFeatureTest sounds good to me, as does splitting on ,. Why do you want to use the feature name though? I think it would be easier to compare against tb.Name(), so that you could do EBPF_TEST_IGNORE_KERNEL_VERSION=TestHaveBatchAPI,TestHaveObjName. That way you can just copy and paste from the go test output if necessary.

lmb avatar Apr 25 '23 13:04 lmb