rust-vmm / rust-vmm-container

Container with all dependencies required for running rust-vmm crates integration tests.
Apache License 2.0
65 stars 32 forks source link

Introduce riscv64 CI container #106

Closed TimePrinciple closed 2 weeks ago

TimePrinciple commented 1 month ago

Summary of the PR

This work was inspired by the work done by @endeneer in PR #91, and is the third draft proceeds #101, #104. It is expected to be replaced by #104 in the future.

Add build scripts for v6.10 riscv64 kernel, qemu-system-riscv64, opensbi and rootfs required to boot qemu-system. And an entrypoint to forward the commands accepted to qemu-system inside the container.

With this approach, we are able to run tests inside qemu-system, while preserving the original output as much as possbile with ssh.

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

TimePrinciple commented 1 month ago

To illustrate, I've drawn an image which describes the entire workflow of how RISC-V CI is completed on a x86_64 runner.

RustVMM RISC-V CI (5)

TimePrinciple commented 1 month ago

Here is a gif showing how the entire workflow works with docker running inside a x86_64 buildkite-agent. With a little tinkering on .buildkite/autogenerate_pipeline.py in rust-vmm-ci, the whole system works seamlessly and produces original output just like CI on x86_64 and arm64.

rust-vmm-ci-demonstration

roypat commented 1 month ago

Here is a gif showing how the entire workflow works with docker running inside a x86_64 buildkite-agent. With a little tinkering on .buildkite/autogenerate_pipeline.py in rust-vmm-ci, the whole system works seamlessly and produces original output just like CI on x86_64 and arm64.

rust-vmm-ci-demonstration

Very cool!! Could you do a draft PR to rust-vmm-ci with the changes needed for this? :o

TimePrinciple commented 1 month ago

Here is a gif showing how the entire workflow works with docker running inside a x86_64 buildkite-agent. With a little tinkering on .buildkite/autogenerate_pipeline.py in rust-vmm-ci, the whole system works seamlessly and produces original output just like CI on x86_64 and arm64.

rust-vmm-ci-demonstration

Very cool!! Could you do a draft PR to rust-vmm-ci with the changes needed for this? :o

Surely, I have the required work done in rust-vmm-ci already. I will raise the PR the first thing tomorrow morning in accordance of this work.

TimePrinciple commented 1 month ago

Overall, this looks very reasonable to me, modulo some minor comments.

One thing we need to keep in mind though (and this might be more of a comment for rust-vmm/rust-vmm-ci#159 and not a blocker here) is that we need a way to selectively enable the riscv64 CI checks on a per repository basis. Currently, if we merged this PR and rust-vmm-ci#159, it'd enable rustv64 checks on all repositories. I suspect that for a bunch of them, it will not work immediately (e.g. the rootfs compiled here doesn't contain all of the vhost-user stuffs, I suspect). The fact that not all rust-vmm repos are initially supported is fine - we can iterate as we go, just start with kvm-bindings and kvm-ioctls, and add whatever is needed for the other repositories as we actually add riscv64 support to them. But it does mean that we'll need some sort of "repository allowlist", maybe some sort of .platforms file in the repo root that lists for which platforms CI should be enabled?

cc @stefano-garzarella @stsquad for opinions on this :)

Very reasonable, that means we also need logic in that python script of detecting which repository it's running in. Possibly by looking at Cargo.toml I suppose 🤔, I will give it a try。

Or should the allow_list be present inside every repository independently. My question is what should be the controlling unit of enabling/disabling CI for a specific platform in a specific repo, the repo itself (.platform in repo root approach) or the rust-vmm-ci.

roypat commented 1 month ago

Overall, this looks very reasonable to me, modulo some minor comments. One thing we need to keep in mind though (and this might be more of a comment for rust-vmm/rust-vmm-ci#159 and not a blocker here) is that we need a way to selectively enable the riscv64 CI checks on a per repository basis. Currently, if we merged this PR and rust-vmm-ci#159, it'd enable rustv64 checks on all repositories. I suspect that for a bunch of them, it will not work immediately (e.g. the rootfs compiled here doesn't contain all of the vhost-user stuffs, I suspect). The fact that not all rust-vmm repos are initially supported is fine - we can iterate as we go, just start with kvm-bindings and kvm-ioctls, and add whatever is needed for the other repositories as we actually add riscv64 support to them. But it does mean that we'll need some sort of "repository allowlist", maybe some sort of .platforms file in the repo root that lists for which platforms CI should be enabled? cc @stefano-garzarella @stsquad for opinions on this :)

Very reasonable, that means we also need logic in that python script of detecting which repository it's running in. Possibly by looking at Cargo.toml I suppose 🤔, I will give it a try。

Or should the allow_list be present inside every repository independently. My question is what should be the controlling unit of enabling/disabling CI for a specific repo, the repo itself (.platform in repo root approach) or the rust-vmm-ci.

I'm somewhat inclined to have it in each repository (with the default being x86 + arm for repos that don't have the file), as that will allow us to just enable the riscv64 CI via a PR to the repository on which we want to enable it (instead of making a change in rust-vmm-ci and then waiting for dependabot to propagate it), but I'd like to hear some more opinions on this

TimePrinciple commented 1 month ago

The output of last build suggests that aemu and other dependencies might yet support riscv64, so I decided to skip steps for vhost-users at the time being, prioritize the riscv64 support for core crates like kvm-bindings and kvm-ioctls.

截屏2024-08-03 00 47 25
stefano-garzarella commented 1 month ago

Overall, this looks very reasonable to me, modulo some minor comments. One thing we need to keep in mind though (and this might be more of a comment for rust-vmm/rust-vmm-ci#159 and not a blocker here) is that we need a way to selectively enable the riscv64 CI checks on a per repository basis. Currently, if we merged this PR and rust-vmm-ci#159, it'd enable rustv64 checks on all repositories. I suspect that for a bunch of them, it will not work immediately (e.g. the rootfs compiled here doesn't contain all of the vhost-user stuffs, I suspect). The fact that not all rust-vmm repos are initially supported is fine - we can iterate as we go, just start with kvm-bindings and kvm-ioctls, and add whatever is needed for the other repositories as we actually add riscv64 support to them. But it does mean that we'll need some sort of "repository allowlist", maybe some sort of .platforms file in the repo root that lists for which platforms CI should be enabled? cc @stefano-garzarella @stsquad for opinions on this :)

Yeah +1 on this!

Very reasonable, that means we also need logic in that python script of detecting which repository it's running in. Possibly by looking at Cargo.toml I suppose 🤔, I will give it a try。 Or should the allow_list be present inside every repository independently. My question is what should be the controlling unit of enabling/disabling CI for a specific repo, the repo itself (.platform in repo root approach) or the rust-vmm-ci.

I'm somewhat inclined to have it in each repository (with the default being x86 + arm for repos that don't have the file), as that will allow us to just enable the riscv64 CI via a PR to the repository on which we want to enable it (instead of making a change in rust-vmm-ci and then waiting for dependabot to propagate it), but I'd like to hear some more opinions on this

I was also thinking something like this, it should be easy to implement and also to activate.

stefano-garzarella commented 1 month ago

@endeneer can you take a look at this PR? WDYT about closing #91 and merge this.

stefano-garzarella commented 2 weeks ago

@TimePrinciple LGTM, please can you rebase?

TimePrinciple commented 2 weeks ago

@TimePrinciple LGTM, please can you rebase?

Yes, already rebased :)

stefano-garzarella commented 2 weeks ago

@TimePrinciple our workflow is failing signing the riscv image: https://github.com/rust-vmm/rust-vmm-container/actions/runs/10581126625/job/29317670656

Can you take a look?

stefano-garzarella commented 2 weeks ago

@TimePrinciple our workflow is failing signing the riscv image: https://github.com/rust-vmm/rust-vmm-container/actions/runs/10581126625/job/29317670656

Can you take a look?

Ops, I just saw the new #112 PR for that, thanks!