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

bpf unit tests: Run tests on changes to pks/bpf/** #25911

Merged
merged 1 commit into from Jun 6, 2023

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jun 5, 2023

The Go program used to run the BPF unit tests has a few dependencies on other Cilium packages, namely:

  • pkg/bpf
  • pkg/byteorder
  • pkg/datapath/link
  • pkg/monitor

In the past, we nearly merged a change that would break the unit tests, because the CI skipped the unit tests job for the related PR. The reason was that the changes didn't touch BPF files (or the runner's sources), but the way we collect programs and perform relocations from an ELF file in pkg/bpf/collection.go.

To avoid repeating this in the future, let's mark pkg/bpf/** updates as a trigger for the unit tests. The other dependency packages could be added too, but they seem less critical (and less likely to break the tests), so we leave them aside for now.

Reference: #25837 (review)

The Go program used to run the BPF unit tests has a few dependencies on
other Cilium packages, namely:

  - pkg/bpf
  - pkg/byteorder
  - pkg/datapath/link
  - pkg/monitor

In the past, we nearly merged a change that would break the unit tests,
because the CI skipped the unit tests job for the related PR. The reason
was that the changes didn't touch BPF files (or the runner's sources),
but the way we collect programs and perform relocations from an ELF file
in pkg/bpf/collection.go.

To avoid repeating this in the future, let's mark pkg/bpf/** updates as
a trigger for the unit tests. The other dependency packages could be
added too, but they seem less critical (and less likely to break the
tests), so we leave them aside for now.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet added area/CI Continuous Integration testing issue or flake sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/ci This PR makes changes to the CI. labels Jun 5, 2023
@qmonnet qmonnet requested review from a team as code owners June 5, 2023 16:55
@qmonnet qmonnet requested review from aanm and nebril June 5, 2023 16:55
@qmonnet
Copy link
Member Author

qmonnet commented Jun 6, 2023

Travis hit a documented flake. BPF checks workflow is still valid (The workflow didn't crash. The test itself was skipped because none of the relevant files were touched). No need for the full CI suite, I'm marking as ready-to-merge.

@qmonnet qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 6, 2023
@dylandreimerink
Copy link
Member

Agreed, travis is hitting a flake in the new BGP tests: failed to start BGP server for virtual router with local ASN 64125: failed starting BGP server: listen tcp4 0.0.0.0:45450: bind: address already in use. Unrelated to this change.

@dylandreimerink dylandreimerink merged commit ebf4e61 into cilium:main Jun 6, 2023
48 of 49 checks passed
@qmonnet qmonnet deleted the pr/bpf-unit-tests-deps branch June 6, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake 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. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants