-
Notifications
You must be signed in to change notification settings - Fork 35
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
add multiarch support for kubevirt tasks #698
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Basavaraju-G The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @Basavaraju-G. 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 Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
Hi @ksimon1 would you please review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your commit looks to be mangled (author, message). Can you please correct that?
@@ -7,7 +7,7 @@ metadata: | |||
tekton.dev/categories: Automation | |||
tekton.dev/tags: kubevirt | |||
tekton.dev/displayName: "KubeVirt disk virt customize" | |||
tekton.dev/platforms: "linux/amd64" | |||
tekton.dev/platforms: "linux/amd64,linux/arm64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need quay.io/kubevirt/libguestfs-tools
for s390x
to add it here, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @0xFelix currently we dont have s390x image of quay.io/kubevirt/libguestfs-tools
, and we are working on it to have s390x multiarch image. once the "quay.io/kubevirt/libguestfs-tools" have multi arch s390x image, i will add it in separate PR. is this okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
scripts/build-release-images.sh
Outdated
@@ -12,11 +12,19 @@ SCRIPT_DIR="$(dirname "$(readlink -f "$0")")" | |||
source "${SCRIPT_DIR}/release-var.sh" | |||
source "${SCRIPT_DIR}/common.sh" | |||
|
|||
# add qemu-user-static | |||
sudo podman run --rm --privileged docker.io/multiarch/qemu-user-static --reset -p yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does that do? Also, we should try to not use images on Docker hub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @0xFelix Here we are trying to use qemu-user-emulation to build multiarch images on x86 native platform. i was taken this from here https://github.com/kubevirt/kubevirt/blob/add278ad94ca13c112b8da83f41160f7371501b5/hack/builder/build.sh#L19 to cross building on x86 platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the qemu-user-static image available on quay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where this is registered as a runner for foreign binaries though? Does it happen in the container? Also, why not make it conditional like in the file you've linked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the qemu-user-static image available on quay?
No its not there in quay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where this is registered as a runner for foreign binaries though? Does it happen in the container? Also, why not make it conditional like in the file you've linked?
No, it wont happen in container. sure i will make a check before running it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where this is registered as a runner for foreign binaries though? Does it happen in the container? Also, why not make it conditional like in the file you've linked?
Hi @0xFelix added condition to install if not enabled.
a4c89d2
to
cc44132
Compare
fixed |
@Basavaraju-G are you planning to add test runners for e2e tests on s390x? |
@ksimon1 yes we have it in planning. we have some kubevirt projects in https://prow.ci.kubevirt.io/ are running s390x e2e jobs in here https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/logs/publish-kubevirtci-s390x/1902662300989394944. |
@Basavaraju-G I tried to trigger it locally and it is failing on:
|
HI @ksimon1 may i know what version of podman your using it? and may i know which OS are you building on it(RHEL/Ubuntu)? i was able to build with github actions job. build log here https://github.com/Basavaraju-G/kubevirt-tekton-tasks/actions/runs/13896150271/job/38877127084. |
cc44132
to
9e3f342
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it looks okay from what I can tell
/ok-to-test
@@ -7,7 +7,7 @@ metadata: | |||
tekton.dev/categories: Automation | |||
tekton.dev/tags: kubevirt | |||
tekton.dev/displayName: "KubeVirt disk virt customize" | |||
tekton.dev/platforms: "linux/amd64" | |||
tekton.dev/platforms: "linux/amd64,linux/arm64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
/retest |
@@ -7,7 +7,7 @@ metadata: | |||
tekton.dev/categories: Automation | |||
tekton.dev/tags: kubevirt | |||
tekton.dev/displayName: "KubeVirt cleanup VM" | |||
tekton.dev/platforms: "linux/amd64" | |||
tekton.dev/platforms: "linux/amd64,linux/s390x,linux/arm64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes has to go to manifests in templates
folder and then run make generate-yaml-tasks
What this PR does / why we need it:
As of now Tekton kubevirt tasks supported only on x86/amd64. so this PR will add support for s390x, arm64 cpu platforms.
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:
Release note: