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
cilium: improve 16bit ifindex probe on old kernels #32466
base: main
Are you sure you want to change the base?
Conversation
RFC I don't think the unit is enough. I would think adding a check in integration/e2e that checks the flag is correctly detected would be a better approach than a unit (which I am not even sure is actually correct for backports as is). Please advise |
f8ba39f
to
4733a89
Compare
/test |
|
||
func TestDetectHaveFibIfindex(t *testing.T) { | ||
testutils.PrivilegedTest(t) | ||
testutils.SkipOnOldKernel(t, "5.10", "bpf: Always return target ifindex in bpf_fib_lookup") |
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.
Why? On old kernels it should still work, but return an error saying that the feature is not supported, no?
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.
Humm.. HaveFibIfindex
call will return err
= ebpf.ErrNotSupported
, so no, the test - as written here - shouldn't succeed for Kernels that don't have the patch.
But as I say in the comment, I see little value on this unit, as you don't have much control on which Kernel the CI is actually running. I think a much better approach would be to run it as part of the e2e or integr, where tests are run against specific kernels, making sure detection (true/false) is correct based on the specific kernel version.
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.
Good question, I don't actually know how we test these. ci-runtime only runs on whatever CI has.
a362f74
to
3c8f17e
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.
There are a couple of vet errors due to fmt.Errorf
.
3c8f17e
to
0d9575c
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.
Still a CI failure unfortunately: https://github.com/cilium/cilium/actions/runs/9077690092/job/24943135667?pr=32466#step:14:35
You might need a
|
I was discussing with Marc a bit on how this might affect downgrades. Considering the scenario where a 1.16 Cilium would detect But Marc also made the fair point that we didn't consider this downgrade aspect when merging #27622. Which also produced such CT states, that were not safe for downgrade. |
Based on my earlier work to bump the minimum kernel version we know that at least ubuntu 20.04 lts started on a 5.4 kernel. But the current version in repos is 5.10: https://packages.ubuntu.com/search?suite=focal&searchon=names&keywords=linux-image Probably this would only people that hand roll their kernels / distros and are on an old version? |
Previously, `HaveFibIfindex` probe checked the presence of commit d1c362e1dd68 in the Linux kernel indirectly, by verifying the presence of the redirect helpers (which were merged alongside). Since `d1c362e1dd68` has been backported to older kernels - but not helpers -, this commit improves `HaveFibIfindex` detection by running a live probe. Fixes: cilium#27642 Signed-off-by: Marc Suñé <[email protected]>
0d9575c
to
ab0a12a
Compare
Previously,
HaveFibIfindex
probe checked the presence of commitd1c362e1dd68
in the Linux kernel indirectly, by verifying the presence of the redirect helpers (which were merged alongside).Since
d1c362e1dd68
has been backported to older kernels - but not helpers -, this commit improvesHaveFibIfindex
detection by running a live probe.Fixes: #27642