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

replace virtme #1321

Merged
merged 2 commits into from May 10, 2024
Merged

replace virtme #1321

merged 2 commits into from May 10, 2024

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented Jan 30, 2024

ci: replace virtme with vimto

vimto is a lightweight wrapper around qemu with the sole purpose of 
executing Go unit tests. Use it instead of virtme, which has not had a
reliable upstream for a while.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

@lmb lmb force-pushed the vimto branch 5 times, most recently from 764b078 to 5c190b7 Compare February 2, 2024 13:12
@lmb lmb marked this pull request as ready for review February 2, 2024 13:42
@lmb lmb requested a review from a team as a code owner February 2, 2024 13:42
@lmb lmb requested a review from ti-mo February 2, 2024 13:43
@lmb lmb marked this pull request as draft February 2, 2024 13:44
@lmb
Copy link
Collaborator Author

lmb commented Feb 2, 2024

Test runs are suspiciously fast.

@paulcacheux
Copy link
Contributor

Not sure if fully applicable, but https://github.com/arighi/virtme-ng is actually supported, and is gaining traction after an article on LWN. Maybe something to look at if we want to replace virtme

@lmb lmb force-pushed the vimto branch 5 times, most recently from c29043a to 2daf2c1 Compare February 23, 2024 14:36
@lmb
Copy link
Collaborator Author

lmb commented Feb 23, 2024

@paulcacheux yeah, I'm aware of virtme-ng! I looked at it a while ago.

Pro:

  • Already works
  • Support for a variety of kernel sources
  • virtiofsd
  • gdb support?

Con:

  • Dependency on rust for init which makes it more difficult to install.
  • Doesn't allow distributing kernels as OCI images

So even if we use virtme-ng we need some wrapper code. vimto is easy to install when you're in the go ecosystem already, and I want to add go specific things like a delve wrapper.

Overall virtme-ng is probably the saner choice. The good news is that we can always rip out vimto if it turns out to be too much of a burden.

@lmb
Copy link
Collaborator Author

lmb commented Feb 23, 2024

Finally managed to get CI green. @ti-mo @dylandreimerink @rgo3 maybe you can give it a spin on your machines to see whether it works? (I know that Robin's nix setup is mysteriously broken.)

Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! Left a few nits and lifted the fix to the verifier test to #1367 and merged separately.

Note: you signed the middle commit with both emails.

We should also talk about vimto, who's going to develop and maintain it, and how we're going to organize it. It would be worth growing its userbase so more projects can benefit and contribute. Something to be discussed at the Cilium dev summit.

cmd/bpf2go/main_test.go Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@lmb
Copy link
Collaborator Author

lmb commented Mar 21, 2024

Current status is that vimto needs a bit more thinking. @ti-mo how about I upstream the non-CI changes so that I can use vimto on my machine to test? Currently stranded without a viable virtme and the changes themselves are useful either way I think.

Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

LGTM overall, just one thing I noticed

.vimto.toml Outdated Show resolved Hide resolved
@lmb lmb mentioned this pull request Mar 21, 2024
@lmb lmb force-pushed the vimto branch 4 times, most recently from 1c5da23 to b01edb9 Compare April 26, 2024 11:24
@lmb lmb marked this pull request as ready for review April 26, 2024 11:25
lmb and others added 2 commits April 29, 2024 16:04
vimto is a lightweight wrapper around qemu with the sole purpose of
executing Go unit tests. Use it instead of virtme, which has not
had a reliable upstream for a while.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb
Copy link
Collaborator Author

lmb commented Apr 29, 2024

Any concerns around merging this? It's at least as useable as virtme from my side.

@dylandreimerink
Copy link
Member

I had some issues running vimto locally initially. When I ran the command from the updated index.md to run vimto locally vimto -- go test ./... it fails with a lot of Error: exec: binary is not statically linked (did you build with CGO_ENABLED=0?) errors.
Turns out its because I didn't add CGO_ENABLED=0 when installing vimto itself go install lmb.io/vimto@latest. Adding so is very unusual. It would be nice if we can look into not requiring this. Or else add warnings in docs. Because I think more people will hit this.

Other than that, everything seems to be working! I say merge

@lmb lmb merged commit 65a17a9 into cilium:main May 10, 2024
17 checks passed
@lmb lmb deleted the vimto branch May 10, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants