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

Fix some test failures for bpf_nat_test.c #24534

Merged
merged 2 commits into from Mar 23, 2023

Conversation

YutaroHayakawa
Copy link
Member

@YutaroHayakawa YutaroHayakawa commented Mar 23, 2023

This PR fixes two test failures on the latest master.

BPF Unit Test failure for bpf_nat_test.c
#24212 introduces a known bug of Coverbee into the code base (cilium/coverbee#7). This only becomes a problem when we run the test with the affected code (the code includes nat.h or conntrack_map.h). We introduced the feature to disable coverage report per file to prevent CI from failing. The latest exclusion pattern is also introduced in #24212 (9a274db).

Yesterday, we merged the PR that introduces a new test file affected by the bug #18414 (07edf55), but we couldn't notice that because the BPF Unit test CI ran without the change introduced in #24212.

Coccicheck failure for bpf_nat_test.c
#24392 "fixed" the Coccicheck, which was not running correctly for a while. This was merged on May 21st. #18414 introduced the code which fails with Coccicheck but ran the check on May 20th, which was before the fix merged. That's why we couldn't notice the failure before the merge.

Fix some test failures for bpf_nat_test.c

@YutaroHayakawa YutaroHayakawa added kind/bug/CI This is a bug in the testing code. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Mar 23, 2023
This test is affected by coverbee's bug. Add it to the exclude pattern.

cilium/coverbee#7

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa changed the title bpf,test: Exclude coverage report for bpf_nat_tests.o bpf,test: Disable coverage report for bpf_nat_tests.o Mar 23, 2023
Coccicheck pointed out following

* file ./tests/bpf_nat_tests.c: missing __align_stack_8 on icmphdr on line 56
* file ./tests/bpf_nat_tests.c: missing __align_stack_8 on inner_l4 on line 114
* file ./tests/bpf_nat_tests.c: missing __align_stack_8 on in_l4hdr on line 431

Fix them by adding missing __align_stack_8.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa added the dont-merge/preview-only Only for preview or testing, don't merge it. label Mar 23, 2023
@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review March 23, 2023 06:19
@YutaroHayakawa YutaroHayakawa requested review from a team as code owners March 23, 2023 06:19
@YutaroHayakawa
Copy link
Member Author

BPF Unit Test and Cocchicheck recovered. Once reviews are in, I'll revert the preview commit.

@YutaroHayakawa YutaroHayakawa changed the title bpf,test: Disable coverage report for bpf_nat_tests.o Fix some test failure for bpf_nat_test.c Mar 23, 2023
@YutaroHayakawa YutaroHayakawa changed the title Fix some test failure for bpf_nat_test.c Fix some test failures for bpf_nat_test.c Mar 23, 2023
@YutaroHayakawa YutaroHayakawa removed the dont-merge/preview-only Only for preview or testing, don't merge it. label Mar 23, 2023
@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Mar 23, 2023

Reverted preview commit. Modified tests were passed, and reviews are in. I don't think it does make sense to run e2e tests since we only have changes to BPF Check related code.

@YutaroHayakawa YutaroHayakawa added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 23, 2023
@borkmann borkmann merged commit 4c6fd7e into cilium:master Mar 23, 2023
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug/CI This is a bug in the testing code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants