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

Introduce riscv64 CI container #106

Merged

Conversation

TimePrinciple
Copy link
Collaborator

@TimePrinciple TimePrinciple commented Jul 29, 2024

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:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@TimePrinciple
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

roypat commented Aug 1, 2024

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
Copy link
Collaborator Author

TimePrinciple commented Aug 1, 2024

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 added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 2, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

Signed-off-by: Ruoqing He <[email protected]>
Copy link
Collaborator

@roypat roypat left a comment

Choose a reason for hiding this comment

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

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 :)

build_rootfs_riscv64.sh Outdated Show resolved Hide resolved
.github/workflows/docker-publish.yml Outdated Show resolved Hide resolved
build_rootfs_riscv64.sh Outdated Show resolved Hide resolved
@TimePrinciple TimePrinciple force-pushed the introduce-riscv-ci-container branch 2 times, most recently from ff88ffa to 364a81b Compare August 2, 2024 11:22
@TimePrinciple
Copy link
Collaborator Author

TimePrinciple commented Aug 2, 2024

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
Copy link
Collaborator

roypat commented Aug 2, 2024

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

riscv64/start_in_qemu.sh Outdated Show resolved Hide resolved
@TimePrinciple
Copy link
Collaborator Author

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

TimePrinciple added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 2, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

Signed-off-by: Ruoqing He <[email protected]>
@stefano-garzarella
Copy link
Member

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.

@TimePrinciple TimePrinciple force-pushed the introduce-riscv-ci-container branch 2 times, most recently from 6377783 to e498223 Compare August 6, 2024 01:18
TimePrinciple added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 6, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

Signed-off-by: Ruoqing He <[email protected]>
@stefano-garzarella
Copy link
Member

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

@TimePrinciple TimePrinciple force-pushed the introduce-riscv-ci-container branch 3 times, most recently from d5e3ac4 to f11bcae Compare August 7, 2024 09:58
TimePrinciple added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 8, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

Signed-off-by: Ruoqing He <[email protected]>
TimePrinciple added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 8, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

Signed-off-by: Ruoqing He <[email protected]>
TimePrinciple added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 9, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

Signed-off-by: Ruoqing He <[email protected]>
Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

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

Just minor things, the rest LGTM.

.github/workflows/docker-publish.yml Outdated Show resolved Hide resolved
build_container.sh Outdated Show resolved Hide resolved
TimePrinciple added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 27, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

Signed-off-by: Ruoqing He <[email protected]>
@stefano-garzarella
Copy link
Member

@TimePrinciple LGTM, please can you rebase?

Add build scripts for v6.10 riscv64 kernel, qemu-system-riscv64 and
opensbi required to boot qemu-system inside docker container.

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

This is the third draft proceeds rust-vmm#101, rust-vmm#104. It is expected to be
replaced by the second draft rust-vmm#104 in the future which standardise
`riscv64`.

Signed-off-by: Ruoqing He <[email protected]>
@TimePrinciple
Copy link
Collaborator Author

@TimePrinciple LGTM, please can you rebase?

Yes, already rebased :)

@stefano-garzarella stefano-garzarella merged commit d0c516f into rust-vmm:main Aug 27, 2024
3 checks passed
TimePrinciple added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 27, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

The container version is updated to v39 to enable CI on RISC-V platform.

Signed-off-by: Ruoqing He <[email protected]>
TimePrinciple added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 27, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

The container version is updated to v40 to enable CI on RISC-V platform.

Signed-off-by: Ruoqing He <[email protected]>
@stefano-garzarella
Copy link
Member

@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
Copy link
Member

@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!

TimePrinciple added a commit to TimePrinciple/rust-vmm-ci that referenced this pull request Aug 30, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

The container version is updated to v44 to enable CI on RISC-V platform.

Signed-off-by: Ruoqing He <[email protected]>
@TimePrinciple TimePrinciple deleted the introduce-riscv-ci-container branch August 30, 2024 17:34
stefano-garzarella pushed a commit to rust-vmm/rust-vmm-ci that referenced this pull request Sep 2, 2024
Introduce logic necessary for generating YAML needed by BuildKite, which
are designed to work with image introduced in
rust-vmm/rust-vmm-container#106.

The container version is updated to v44 to enable CI on RISC-V platform.

Signed-off-by: Ruoqing He <[email protected]>
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.

3 participants