Skip to content

fix: solve some verifier issues and introduce a bpf CI#135

Merged
Andreagit97 merged 13 commits into
mainfrom
test_bpf_versions
Dec 19, 2025
Merged

fix: solve some verifier issues and introduce a bpf CI#135
Andreagit97 merged 13 commits into
mainfrom
test_bpf_versions

Conversation

@Andreagit97
Copy link
Copy Markdown
Collaborator

@Andreagit97 Andreagit97 commented Dec 18, 2025

What this PR does / why we need it:

This PR solves some verifier issues and introduces a bpf CI

Which issue(s) this PR fixes
fixes #88

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@Andreagit97
refactor: use BPF_F_NO_PREALLOC for cgtracker_map
4e8f3c4 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
refactor: remove useless checks
92080d0 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
test: improve testing methods
efb2857 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
fix: set max entries to 65536 for string maps
f900622 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
fix: fix loading error on kernels < 5.11
95acf72 
max key size on kernels < 5.11 is always 512 so we need to rewrite
map specs in order to match this value

Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
Copy link
Copy Markdown
Collaborator Author

Andreagit97 commented Dec 18, 2025

I would like to test the new action I introduced, but I cannot due to

The action andreagit97/bpfvalidator@v0.3.0 is not allowed in neuvector/runtime-enforcer because all actions must be from a repository owned by your enterprise...

@holyspectral @dottorblaster, can you help me with this?

@Andreagit97 Andreagit97 force-pushed the test_bpf_versions branch 2 times, most recently from 900e85c to 9a501ce Compare December 18, 2025 16:18
@Andreagit97 Andreagit97 marked this pull request as draft December 18, 2025 16:25
@Andreagit97
Copy link
Copy Markdown
Collaborator Author

Andreagit97 commented Dec 18, 2025 •
edited
Loading

Just some thoughts:

  • [LOCAL DEVELOPMENT] I tried LVH locally, and I have to admit it's not super handy to use. You can start a VM, but then you have to inject a command inside it through ssh. On the side, virtme-ng allows you to execute on the fly a command in the VM and destroy it. Moreover, virtme-ng mounts your current fs over the VM as an overlay so that you can build your Go application directly inside the VM without installing anything; all the dependencies are there. So for local development, I believe virtme-ng is better at the moment (just my opinion of course)
  • [CI] The advantage of LVH is that the latest kernel images are built periodically by Cilium. virtme-ng rely on Ubuntu https://kernel.ubuntu.com/mainline/mainline that does more or less the same. What we miss with virtme-ng is RHEL support https://quay.io/repository/lvh-images/kernel-images?tab=tags, but at the moment, I'm not even sure we want to support it. LVH also comes with some support for building new kernel images. We can reuse this logic for virtme-ng if we want, in the end, virtme just requires a vmlinuz... TL;DR; LVH is probably better for the CI in the long term.

For now, I'm using bpfvalidator in CI because it is easier to set up and I'm more familiar with it. I have to admit I tried LVH in CI this morning, but I had some issues with the configuration... maybe in the near future we can switch to LVH, even if for now I'm not super sure it will bring a big additional value to what we have, maybe it could help us if we will build our own custom images...

A possible improvement to this PR could be to add an automatic job/dependabot that scrapes the Ubuntu mainline once per day and opens a PR with the updated image versions. Similar to what is done here https://github.com/cilium/pwru/blob/9ac30b44809321a96f189c2957d958a3122faee6/.github/workflows/test.yml#L61

@Andreagit97
Copy link
Copy Markdown
Collaborator Author

Andreagit97 commented Dec 18, 2025

Right now, the CI is running our ebpf tests inside each machine; it seems they don't work on some kernels, I need to check. Moreover, bpfvalidator is not reporting the failure in the output. I need to check this as well.

@holyspectral
Copy link
Copy Markdown
Collaborator

holyspectral commented Dec 18, 2025

I believe virtme-ng is better at the moment

I have a feeling that we might be able to use kernels downloaded from lvh, I will take a look to see if this is possible, but before that I have to setup virtme-ng...something is wrong in my env.

@holyspectral
Copy link
Copy Markdown
Collaborator

holyspectral commented Dec 18, 2025

okay it's the Ubuntu distribution of virtme-ng was broken. https://bugs.launchpad.net/ubuntu/+source/virtme-ng/+bug/2104184

Now I can use the kernel successfully via the steps here:

lvh kernels pull --dir _data 5.15-main
vng --run _data/5.15-main/boot/vmlinuz-5.15.197

It took roughly 35 seconds to download and enter the kernel.

@Andreagit97
Copy link
Copy Markdown
Collaborator Author

Andreagit97 commented Dec 18, 2025

yep exactly, if we want, we can use LVH to build and push images somewhere, and then run them with virtme-ng. we can also modify bpfvalidator to take as input a lvh built image and start virtme-ng for us

But yeah, still don't know what is the best choice for the CI 🤔 maybe we can end up using virtme-ng for local development and quick iteration, and LVH in CI if we want to reuse the infrastructure of cilium without building

@Andreagit97 Andreagit97 marked this pull request as ready for review December 19, 2025 12:16
@Andreagit97
Copy link
Copy Markdown
Collaborator Author

Andreagit97 commented Dec 19, 2025 •
edited
Loading

The main issue that I see at the moment is that ebpf tests could be flaky. What we do at the moment is:

  • We start the ebpf manager
  • We wait to see the first learning event in the channel (we use a goroutine that calls in a loop /usr/bin/true) -> Timeout of 10sec
  • When we are sure the manager has started, we begin the test

Currently, we are running four machines in parallel on a small GitHub runner, so a 10-second timeout could not be enough.

If we see they are too flaky, we can disable them and just check the verifier for now without running the test suite.

dottorblaster
dottorblaster approved these changes Dec 19, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM, I just left two comments that you can absolutely ignore 👍 (especially the opne about the constants)

Comment thread internal/bpf/manager.go Outdated
return err
}

// OK!
Copy link
Copy Markdown
Member

@dottorblaster dottorblaster Dec 19, 2025

Choose a reason for hiding this comment

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

LOL do you want to keep this comment?

Copy link
Copy Markdown
Collaborator Author

@Andreagit97 Andreagit97 Dec 19, 2025

Choose a reason for hiding this comment

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

done, thanks!

Comment thread internal/bpf/policy_values.go
Comment on lines +91 to +98
if s <= stringMapSize7 {
return stringMapSize7
}
if s <= stringMapSize8 {
return stringMapSize8
}
if s <= stringMapSize9 {
return stringMapSize9
Copy link
Copy Markdown
Member

@dottorblaster dottorblaster Dec 19, 2025

Choose a reason for hiding this comment

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

This is absolutely for the future but these constant names are very weird 😄

Is that bpf-idiomatic to have constants named like this?

Copy link
Copy Markdown
Collaborator Author

@Andreagit97 Andreagit97 Dec 19, 2025

Choose a reason for hiding this comment

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

nope, just random names, but I would probably do that in a cleanup PR

@Andreagit97
feat: check supported features instead of a minimal version
8dec0ef 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
fix: solve verifier issues on kernel < 5.13
f38c62d 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
ci: add bpfvalidator
0a99d05 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
ci: remove arm64 support for now
9587464 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
ci: move bpfvalidator action inside the repo for now
af2bc99 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
test: use a greater timeout for the CI
cf33a86 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
test: provide kernel version as a parameter
3bb6330 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
test: wait for the manager to start
86df41c 
Signed-off-by: Andrea Terzolo <andrea.terzolo@suse.com>
@Andreagit97
Copy link
Copy Markdown
Collaborator Author

Andreagit97 commented Dec 19, 2025

just triggered again the CI to see how it goes

@Andreagit97 Andreagit97 merged commit cef54cf into main Dec 19, 2025
12 checks passed
@dottorblaster dottorblaster deleted the test_bpf_versions branch December 19, 2025 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dottorblaster dottorblaster dottorblaster approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test ebpf support on different kernel versions

3 participants

@Andreagit97 @holyspectral @dottorblaster