8000 bpf/complexity-tests: bpf_xdp coverage improvements by gentoo-root · Pull Request #38561 · cilium/cilium · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

bpf/complexity-tests: bpf_xdp coverage improvements #38561

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

Merged
merged 3 commits into from
Apr 12, 2025

Conversation

gentoo-root
Copy link
Contributor
@gentoo-root gentoo-root commented Mar 27, 2025

ci-e2e-upgrade test may fail on kernel 5.15 due to verifier complexity limit, while ci-verifier passes in all tests. Increase the coverage of the latter by adding that specific scenario and its extended version.

Link to that specific occurrence: https://github.com/cilium/cilium/actions/runs/14091707307/job/39498542748?pr=38110

Fix a typo EVENTS_MAP_RATE_LIMIT -> EVENTS_MAP_RATE_LIMIT_PER_SECOND that led to missing coverage for rate limiting on BPF events map (Cc: @siwiutki, #29711).

@gentoo-root gentoo-root added the release-note/ci This PR makes changes to the CI. label Mar 27, 2025
@gentoo-root gentoo-root requested a review from a team as a code owner March 27, 2025 15:19
@gentoo-root
Copy link
Contributor Author

Moving to draft for now, apparently more config options should be enabled in the test, as it still fails in the E2E test while passing ci-verifier:

https://github.com/cilium/cilium/actions/runs/14115207803/job/39543635184

@gentoo-root gentoo-root marked this pull request as draft March 27, 2025 21:22
@gentoo-root gentoo-root force-pushed the pr/max/complexity-test branch from c6627c9 to ce252f3 Compare March 28, 2025 16:37
@gentoo-root gentoo-root changed the title bpf/complexity-tests: Test egress gateway with bpf_xdp bpf/complexity-tests: bpf_xdp coverage improvements Mar 28, 2025
@gentoo-root gentoo-root force-pushed the pr/max/complexity-test branch from ce252f3 to 9b9aa5b Compare March 28, 2025 16:55
@gentoo-root
Copy link
Contributor Author

/test

@gentoo-root gentoo-root marked this pull request as ready for review March 28, 2025 17:34
@gentoo-root gentoo-root force-pushed the pr/max/complexity-test branch from 9b9aa5b to 6a7d8d6 Compare March 28, 2025 18:36
@gentoo-root gentoo-root requested a review from a team as a code owner March 28, 2025 18:36
@gentoo-root gentoo-root requested a review from rgo3 March 28, 2025 18:36
@gentoo-root
Copy link
Contributor Author

/test

1 similar comment
@gentoo-root
Copy link
Contributor Author

/test

@siwiutki
Copy link
Contributor
siwiutki commented Apr 2, 2025

Thanks for catching that. I think at some point when working on #29711, I renamed EVENTS_MAP_RATE_LIMIT_PER_SECOND to EVENTS_MAP_RATE_LIMIT to make the naming consistent with the other parameter, EVENTS_MAP_BURST_LIMIT, but forgot to change it in bpf/complexity-tests and since everything was still green, I didn't double-check.

Might be worth to have some sanity check in CI to verify if defines in those complexity tests actually affect anything.

Copy link
Member
@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nonblocking: maybe reverse the order of 3rd and 2nd patch, so as to ensure git-bisectibility?

Overall LGTM

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 10, 2025
@gentoo-root gentoo-root force-pushed the pr/max/complexity-test branch from 3b3ebec to 6382ddf Compare April 10, 2025 11:16
@gentoo-root
Copy link
Contributor Author

/test

ci-e2e-upgrade test may fail on kernel 5.15 due to verifier complexity
limit, while ci-verifier passes in all tests. Increase the coverage of
the latter by adding that specific scenario and its extended version.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
After enabling coverage for EVENTS_MAP_RATE_LIMIT (next commit), clang
sometimes uninlines ratelimit_check_and_take (marked as inline, but not
__always_inline), which produces this verification error in bpf_host
and bpf_lxc on kernel 5.4:

Error: program tail_ipv4_to_endpoint: load program: invalid argument: tail_calls are not allowed in programs with bpf-to-bpf calls

Mark ratelimit_check_and_take as __always_inline to suppress this
behavior.

Fixes: 93f1619 ("bpf: Add ratelimiting feature and use it for ICMPv6 responses")
Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
The correct define name is EVENTS_MAP_RATE_LIMIT, not
EVENTS_MAP_RATE_LIMIT_PER_SECOND. Fix the typo to add coverage for the
rate limiting feature.

Fixes: b7a26eb ("Add rate limiting on BPF events map")
Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root gentoo-root force-pushed the pr/max/complexity-test branch from 6382ddf to cc17ebd Compare April 10, 2025 12:46
@gentoo-root
Copy link
Contributor Author

/test

@joestringer joestringer added this pull request to the merge queue Apr 12, 2025
Merged via the queue into main with commit 1e0778b Apr 12, 2025
303 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0