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

test: print logical instruction count per program #26641

Merged
merged 1 commit into from Jul 19, 2023

Conversation

ti-mo
Copy link
Contributor

@ti-mo ti-mo commented Jul 5, 2023

As it says on the tin, echo instruction count for all programs, both if the Collection fails to load, as well as at the end of the last line of the verifier log on a successful load.

Example output of successful load:

=== RUN   TestVerifier/bpf_host_1
    verifier_test.go:216: tail_srv6_reply: processed 277 insns (limit 1000000) ... mark_read 8 (201 unverified insns)
...

Example output of unsuccessful load:

=== RUN   TestVerifier/bpf_lxc_1
    verifier_test.go:201: Full verifier log at bpf_lxc_1_verifier.log
    verifier_test.go:203: BPF unverified instruction count per program:
    verifier_test.go:205: tail_handle_ipv4: 1048 insns

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 5, 2023
@julianwiedmann
Copy link
Member

Looks good:
https://github.com/cilium/cilium/actions/runs/5463861410/jobs/9945121909#step:4:171

imho we could even add this to the log output when no error occurs (for before/after comparison). But happy to do that as follow-on.

@ti-mo ti-mo force-pushed the tb/verifier-test-insn-count branch from 03c3667 to ce8b1f9 Compare July 5, 2023 17:53
@ti-mo
Copy link
Contributor Author

ti-mo commented Jul 5, 2023

imho we could even add this to the log output when no error occurs (for before/after comparison). But happy to do that as follow-on.

No, good call, was an extra line of changes. :) Might as well change it while we're at it.

@ti-mo ti-mo changed the title test: print logical instruction count per program on verifier errors test: print logical instruction count per program Jul 5, 2023
@ti-mo ti-mo force-pushed the tb/verifier-test-insn-count branch from ce8b1f9 to 534b62f Compare July 5, 2023 18:12
@julianwiedmann
Copy link
Member

imho we could even add this to the log output when no error occurs (for before/after comparison). But happy to do that as follow-on.

No, good call, was an extra line of changes. :) Might as well change it while we're at it.

nice, thank you! lgtm.

@ti-mo ti-mo added the release-note/ci This PR makes changes to the CI. label Jul 18, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 18, 2023
As it says on the tin, echo instruction count for all programs, both if the
Collection fails to load, as well as at the end of the last line of the
verifier log on a successful load.

Example output of successful load:
```
=== RUN   TestVerifier/bpf_host_1
    verifier_test.go:216: tail_srv6_reply: processed 277 insns (limit 1000000) ... mark_read 8 (201 unverified insns)
...
```

Example output of unsuccessful load:

```
=== RUN   TestVerifier/bpf_lxc_1
    verifier_test.go:201: Full verifier log at bpf_lxc_1_verifier.log
    verifier_test.go:203: BPF unverified instruction count per program:
    verifier_test.go:205: tail_handle_ipv4: 1048 insns
```

Also output the original `err` containing the name of the prog that caused
the verifier error, which was accidentally removed by 1833964
("test/verifier: don't dump full log into test output"). It's now
documented why it's included.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo ti-mo force-pushed the tb/verifier-test-insn-count branch from 534b62f to 6fb37c1 Compare July 18, 2023 13:19
@ti-mo ti-mo marked this pull request as ready for review July 18, 2023 13:21
@ti-mo ti-mo requested review from a team as code owners July 18, 2023 13:21
@ti-mo ti-mo requested review from lmb and christarazi July 18, 2023 13:21
@ti-mo
Copy link
Contributor Author

ti-mo commented Jul 18, 2023

/test

@christarazi christarazi added the area/CI-improvement Topic or proposal to improve the Continuous Integration workflow label Jul 18, 2023
@ti-mo ti-mo added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 19, 2023
@ti-mo
Copy link
Contributor Author

ti-mo commented Jul 19, 2023

Gateway and ingress conformance tests didn't skip for some reason. Flagged this internally. Marking ready.

@aditighag aditighag merged commit db0e2c2 into cilium:main Jul 19, 2023
55 checks passed
@julianwiedmann
Copy link
Member

Think it would be good to have this in v1.14 as well, labeling.

@julianwiedmann julianwiedmann added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Aug 16, 2023
@ti-mo ti-mo deleted the tb/verifier-test-insn-count branch August 16, 2023 15:23
@tklauser tklauser mentioned this pull request Aug 22, 2023
22 tasks
@tklauser tklauser added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Aug 22, 2023
@tklauser tklauser mentioned this pull request Aug 25, 2023
3 tasks
@joestringer joestringer added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. 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.

None yet

7 participants