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.

Sign up for GitHub

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

Jump to bottom

runtime-rs: make compilation for QEMU on s390x #8454

Merged
merged 3 commits into from Feb 2, 2024
Merged

runtime-rs: make compilation for QEMU on s390x #8454

merged 3 commits into from Feb 2, 2024

Conversation

BbolroC
Copy link
Member

@BbolroC BbolroC commented Nov 15, 2023 •
edited

This PR is to make runtime-rs compilable with QEMU on s390x. This is just a base work for the s390x enablement. There must be a few more steps (implementation for s390x-specific) to run a container with runtime-rs.

Fixes: #8446

Signed-off-by: Hyounggyu Choi Hyounggyu.Choi@ibm.com

@katacontainersbot katacontainersbot added the size/large Task of significant size label Nov 15, 2023
@Apokleos Apokleos self-requested a review November 19, 2023 07:39
Apokleos
Apokleos reviewed Nov 19, 2023
View reviewed changes
Copy link
Contributor

@Apokleos Apokleos left a comment

Choose a reason for hiding this comment

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

Hi @BbolroC, Thx for your work.
Can we break this PR down into smaller commits? The commit messages don't seem to match the code changes. For example, a lot of the code changes don't seem to be related to "make compilation for QEMU on s390x".

src/runtime-rs/crates/hypervisor/src/lib.rs Show resolved Hide resolved
@BbolroC
Copy link
Member Author

BbolroC commented Nov 20, 2023

Hi @BbolroC, Thx for your work. Can we break this PR down into smaller commits? The commit messages don't seem to match the code changes. For example, a lot of the code changes don't seem to be related to "make compilation for QEMU on s390x".

Yes, I will definitely. Thanks for the comment. I also got the similar comment from @gkurz.

But at the moment this PR has lots of conflict with #8185 (upcoming QEMU implementation) which also depends on #8414. I will start re-working this PR after getting the confirmation for going ahead from @pmores . Thanks. 😉

cmaf
cmaf reviewed Nov 20, 2023
View reviewed changes
Copy link
Contributor

@cmaf cmaf left a comment

Choose a reason for hiding this comment

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

Hi @BbolroC, one initial question. I would also be grateful if you could break this into smaller commits.

src/runtime-rs/crates/hypervisor/src/dragonball/inner.rs Outdated Show resolved Hide resolved
@katacontainersbot katacontainersbot added size/medium Average sized task and removed size/large Task of significant size labels Jan 31, 2024
@BbolroC BbolroC force-pushed the compile-with-qemu-s390x branch 2 times, most recently from 7818a9a to 5f3e900 Compare January 31, 2024 17:41
pmores
pmores reviewed Feb 1, 2024
View reviewed changes
src/runtime-rs/crates/runtimes/virt_container/src/lib.rs
use hypervisor::ch::CloudHypervisor;
#[cfg(feature = "cloud-hypervisor")]
#[cfg(all(feature = "cloud-hypervisor", not(target_arch = "s390x")))]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love for the resulting code to have fewer conditional compilation lines but frankly, I don't see how to achieve that at the moment, without a major refactor to group individual hypervisors' code more closely together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I support your preference. Refactoring will be necessary (potentially not a small scale, I think), and this could be effectively handled by one of the Rust experts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely out of scope for this PR, to be handled by a subsequent one I agree. I approved already regardless of this.

pmores
pmores approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@pmores pmores left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @BbolroC !

gkurz
gkurz requested changes Feb 1, 2024
View reviewed changes
Copy link
Member

@gkurz gkurz left a comment

Choose a reason for hiding this comment

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

It is certain that you had to enable the compilation first in order to hit the build issues and fix them. Now that this code is going to be merged into main, please consider bisectability : in order for the enablement patch to not break the build, it should be applied after the fixes, i.e. last in the PR.

@BbolroC
runtime-rs: Exclude hypervisors plugins except QEMU for s390x
56aef37 
Dragonball and cloud-hypervisor are not supported on s390x.
We need to exclude the plugins for these hypervisors from compilation.

Signed-off-by: Hyounggyu Choi <Hyounggyu.Choi@ibm.com>
@BbolroC
runtime-rs: Use Persist::restore() of QEMU for VirtSandbox
8fcee6e 
It fails to compile virt_container because Dragonball is only
used in the implementation of the trait method Persist::restore().
As the hypervisor is not compiled on s390x and QEMU implements
the trait method, this commit is to let the method use QEMUi's.

Signed-off-by: Hyounggyu Choi <Hyounggyu.Choi@ibm.com>
@BbolroC
Copy link
Member Author

BbolroC commented Feb 1, 2024

/test

@BbolroC
runtime-rs: Allow compilation for s390x
bb6f507 
Until now, runtime-rs couldn't be compiled on s390x.
We need to lift those restrictions in Makefile first.

Fixes: kata-containers#8446

Signed-off-by: Hyounggyu Choi <Hyounggyu.Choi@ibm.com>
@BbolroC
Copy link
Member Author

BbolroC commented Feb 1, 2024

/test

@BbolroC
Copy link
Member Author

BbolroC commented Feb 2, 2024 •
edited

It is certain that you had to enable the compilation first in order to hit the build issues and fix them. Now that this code is going to be merged into main, please consider bisectability : in order for the enablement patch to not break the build, it should be applied after the fixes, i.e. last in the PR.

I have finished studying git bisect and got what it means and implies on finding a bad commit. The enablement patch has been moved to the latest place. Thanks for the suggestion.

gkurz
gkurz approved these changes Feb 2, 2024
View reviewed changes
Copy link
Member

@gkurz gkurz left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for understanding the git bisect related request @BbolroC ! 😃

@gkurz gkurz merged commit d1a26ea into kata-containers:main Feb 2, 2024
287 of 292 checks passed
@BbolroC BbolroC deleted the compile-with-qemu-s390x branch February 2, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmaf cmaf cmaf left review comments

@Apokleos Apokleos Apokleos left review comments

@pmores pmores pmores approved these changes

@gkurz gkurz gkurz approved these changes

Assignees
No one assigned
Labels
ok-to-test runtime-rs size/medium Average sized task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

runtime-rs: make compilation for QEMU on s390x
7 participants
@BbolroC @gkurz @cmaf @Apokleos @pmores @bergwolf @katacontainersbot