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

Enable building and running kubevirt on s390x #10490

Merged
merged 7 commits into from
Jun 10, 2024
Merged

Enable building and running kubevirt on s390x #10490

merged 7 commits into from
Jun 10, 2024

Conversation

jschintag
Copy link
Contributor

@jschintag jschintag commented Sep 27, 2023 •
edited
Loading

What this PR does / why we need it:
Enable building and running kubevirt on IBM Z Platform.

The Purpose of this Draft PR is not to merge it as is, but rather have a discussion about the Changes and start gathering Feedback from the community.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
Since this is a WIP PR, here are my open TODOs for the PR:

TODOs:

  • Test all changed build scripts on s390x
  • Rework Graphic device
  • Disable ACPI by default on s390x
  • Add s390x to tests
  • Enable cross-compiling
  • Do not build every container image on s390x, as some base images are not available
  • Cleanup commits (Squash, wrong author etc.)
  • Rebase PR on main
  • (Optional) Split PR into multiple smaller PRs
  • Write Release Note
  • Checklist below

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

Add support for building and running kubevirt on s390x.

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 27, 2023
@kubevirt-bot kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 27, 2023
@kubevirt-bot
Copy link
Contributor

Hi @jschintag. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kubevirt-bot kubevirt-bot added size/XXL kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/build-change Categorizes PRs as related to changing build files of virt-* components labels Sep 27, 2023
@cfilleke cfilleke mentioned this pull request Sep 28, 2023
Open
@cfilleke
Copy link
Contributor

Starting point to splitting this into multiple smaller PRs is to generate issues for the parts that can be upstreamed relatively independently of each other, e.g:

  1. document the building of the build image
  2. changes necessary to build the s390x build image and publish in a private registry
  3. tag and manifest the builder it in the default registry and repository with the other builder images
  4. keep the library dependency versions consistent across the multi-arch builds of kubevirt (rpms)
  5. provide code changes needed for s390x cross-compile of kubevirt for test and release, with dev includes and libraries pinned to tested/approved versions consistent across all four machine architectures
  6. provide test cases for s390x; two categories: System Z specific vs a subset of multi-arch tests. Document exceptions.
  7. script changes to tag and manifest RC kubevirt images upstream for test and release

@alicefr
Copy link
Member

alicefr commented Sep 29, 2023

@cfilleke @jschintag please take into account that we'll need also CI integration. Please, take a look to the kubevirtci and project-infra. The CI needs to be integrated in prow and KVM capable. Ideally, the CI host shouldn't be z/VM.

andreabolognani
andreabolognani reviewed Oct 24, 2023
View reviewed changes
Copy link
Contributor

@andreabolognani andreabolognani left a comment

Choose a reason for hiding this comment

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

@jschintag the bulk of the changes to the Go code look sane, but I have some concerns around the build system situation.

As a "meta" comment, I understand that this is a WIP, but I still think it would be great if you could reorganize things a bit to make reviewers' lives easier. As it is today, there are things that are introduced halfway through the series only to be ripped out later, bits that are iterated over multiple times... It just makes things more confusing that they need to be IMO.

Rebasing on top of main would also be great: at this point, it has diverged quite a bit from the base of your branch.

cmd/virt-launcher/virt-launcher.go Outdated Show resolved Hide resolved
pkg/virt-api/webhooks/s390x.go Outdated Show resolved Hide resolved
hack/build-go.sh Show resolved Hide resolved
hack/builder/Dockerfile Outdated Show resolved Hide resolved
bazel/toolchain/s390x-none-linux-gnu/cc_toolchain_config.bzl Outdated Show resolved Hide resolved
hack/builder/Dockerfile Outdated Show resolved Hide resolved
@jschintag
Copy link
Contributor Author

As a "meta" comment, I understand that this is a WIP, but I still think it would be great if you could reorganize things a bit to make reviewers' lives easier. As it is today, there are things that are introduced halfway through the series only to be ripped out later, bits that are iterated over multiple times... It just makes things more confusing that they need to be IMO.

Rebasing on top of main would also be great: at this point, it has diverged quite a bit from the base of your branch.

@andreabolognani Thank you for your review.

I am currently working on squashing all the commits into something more presentable, and i will also rebase it as part of that. If i don't run into errors with the rebase, i will have that done this week.

My goal with this PR was mainly to raise awareness about our effort and get early feedback. I think your preference would be to have CI first, before merging the code. Is that correct?

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Oct 25, 2023
@andreabolognani
Copy link
Contributor

I am currently working on squashing all the commits into something more presentable, and i will also rebase it as part of that. If i don't run into errors with the rebase, i will have that done this week.

The new version looks a lot more manageable already, thanks! Looking forward to the rebased one :)

My goal with this PR was mainly to raise awareness about our effort and get early feedback. I think your preference would be to have CI first, before merging the code. Is that correct?

Not my call to make, so I'll let someone else answer :)

@alicefr
Copy link
Member

alicefr commented Oct 26, 2023

I am currently working on squashing all the commits into something more presentable, and i will also rebase it as part of that. If i don't run into errors with the rebase, i will have that done this week.

My goal with this PR was mainly to raise awareness about our effort and get early feedback. I think your preference would be to have CI first, before merging the code. Is that correct?

I would strongly encourage you to concentrate on the CI. It might take some time and I would say it is a must before merging something. For the SEV support, the PR was blocked for several months even if the code was ready because we were lacking the CI.

/cc @brianmcarey @dhiller

@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2023
@jschintag
Copy link
Contributor Author

I pushed the rebase, but so far it has at the least broken virt-handler, so i will have to look at fixing it.

@alicefr For the CI work, my colleagues are working on it in parallel.

@andreabolognani
Copy link
Contributor

@jschintag can you please explain the purpose of the fix-rpm-deps.sh script you've just introduced?

@andreabolognani
Copy link
Contributor

The rebase looks sane by the way, I'm surprised that virt-handler would no longer work after it but I'm sure you'll figure out the reason :)

@andreabolognani
Copy link
Contributor

@jschintag please at least improve the comment as requested here since you're going to need to rebase anyway.

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2024
@vamsikrishna-siddu vamsikrishna-siddu mentioned this pull request Jun 6, 2024
Closed
18 tasks
@vamsikrishna-siddu
Copy link
Contributor

vamsikrishna-siddu commented Jun 6, 2024 •
edited
Loading

Hello reviewers, We are not able to rebase this PR due to the unavailability of the @jschintag . So, I have raised this new #12066 PR with the same changes by rebasing with the latest changes and addressing this comment. #10490 (comment)
can you please review it?

@jschintag
BUILD.bazel: Enable s390x build target
36e451b 
Add s390x container select statements.
Enable s390x virtctl builds.

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>
@jschintag
Add s390x to build scripts
19eaa24 
Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>
@jschintag
Docs: Expand explanation for sandbox updates when onboarding new arches
d8aa5cf 
Add explanation on how to onboard new arches with the new bazel sandbox
mechanic in place.

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>
@jschintag
Bazel: Add targets for s390x container images
09bb867 
Skip cirros and fedora-with-test-tooling, as they are not build for s390x.
Skip images needed for e2e tests.

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>
@jschintag
Multiarch: Refactor if-else structs to switch-case
965e539 
Refactor if-else for selecting the right architecture code to switch-case.
Ensure arm64 specific cases are more clearly recognizable as such.

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>
@jschintag
Enable s390x VMs for kubevirt
49509be 
Add s390x Machine Type.
Enable and add IsS390X functions.
Set default disk type as virtio for s390x.
Disable ROM tuning on s390x.
Disable USB on s390x.
Set device model to virtio instead of none for ccw devices.
Do not set virtio-(non)-transitional model type for ccw devices.
Add s390x to supported architectures in node-labeller.
Use default CPU mode implementation for s390x.
Disable ACPI Feature by default on s390x.
Disable usbredir for s390x.

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>
@jschintag
s390x: Skip virtctl version unit test
b8e71ea 
The virtctl version unit test depends on the version information being injected
during the compile step. This does not happen when running it via
`make go-test`.

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>
@kubevirt-bot kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 10, 2024
@jschintag
Copy link
Contributor Author

I'm back from vacation and i have rebased the PR. Additionally i have added the requested extended comment.
I have already pushed the changes, though i'm also going to build it locally and run the unit tests on s390x to be sure that nothing broke with the rebase.

/cc @alicefr @andreabolognani

Thank you for your review and sorry that i did not give push rights before leaving for vacation. I thought a single week wouldn't make that much of a difference...

@alicefr
Copy link
Member

alicefr commented Jun 10, 2024

/lgtm
/hold to let @andreabolognani give a final look.

@kubevirt-bot kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 10, 2024
@kubevirt-commenter-bot
Copy link

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

@jschintag
Copy link
Contributor Author

I could build and deploy the PR on s390x without problems. Unit tests also passed.

@alicefr
Copy link
Member

alicefr commented Jun 10, 2024

/test pull-kubevirt-e2e-k8s-1.30-sig-storage

@andreabolognani
Copy link
Contributor

/lgtm
/hold cancel

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2024
@kubevirt-bot kubevirt-bot merged commit 630fc5b into kubevirt:main Jun 10, 2024
39 checks passed
@jschintag jschintag deleted the enable-s390x branch June 11, 2024 07:02
@andreabolognani
Copy link
Contributor

@jschintag congrats! 👏

@Rurouni-Jiaxin Rurouni-Jiaxin mentioned this pull request Aug 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbrueckner hbrueckner hbrueckner left review comments

@xpivarc xpivarc xpivarc left review comments

@dankenigsberg dankenigsberg dankenigsberg requested changes

@kbidarkar kbidarkar Awaiting requested review from kbidarkar

@rthallisey rthallisey Awaiting requested review from rthallisey

@brianmcarey brianmcarey Awaiting requested review from brianmcarey

@dhiller dhiller Awaiting requested review from dhiller

@zhlhahaha zhlhahaha Awaiting requested review from zhlhahaha

@alicefr alicefr Awaiting requested review from alicefr

@andreabolognani andreabolognani Awaiting requested review from andreabolognani

Assignees

@dankenigsberg dankenigsberg

@andreabolognani andreabolognani

@alicefr alicefr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/virtctl dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/build-change Categorizes PRs as related to changing build files of virt-* components lgtm Indicates that a PR is ready to be merged. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/buildsystem Denotes an issue or PR that relates to changes in the build system. sig/compute sig/network sig/storage size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.