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

add s390x support for kubevirt builder. #11097

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vamsikrishna-siddu
Copy link

@vamsikrishna-siddu vamsikrishna-siddu commented Jan 29, 2024

What this PR does / why we need it:
This pr adds the s390x support to kubevirt builder image.
The kubevirt builder image for s390x does not contain Bazel. We are not including the Bazel because at the moment there is no upstream official support of Bazel for s390x. We are working with Bazel community to get the support. But it will take lot of time to get the support as the Bazel community has other priorities and they are not willing to add the support now. We also thought to publish the natively compiled Bazel Binary for s390x, We were not able to do it because of legal issues. We will include the Bazel in the kubevirt builder once we get the Bazel support on s390x.
How we are proceeding for unit and E2E Tests.
Unit Test:
We will run the unit-tests using Golang . We will publish the kubevirt builder image without Bazel to the public and use that image to run the unit tests using make go-test.

E2E Test:
We will use our own kubevirt builder image (which contains Bazel) from our internal registry, and we will override the used image using environment variable to run the e2e tests.

cc: @brianmcarey

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:

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 s390x support for kubevirt builder

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has 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 Jan 29, 2024
@kubevirt-bot kubevirt-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M labels Jan 29, 2024
@kubevirt-bot
Copy link
Contributor

Hi @vamsikrishna-siddu. 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.

hack/builder/Dockerfile Outdated Show resolved Hide resolved
hack/builder/Dockerfile Outdated Show resolved Hide resolved
hack/builder/Dockerfile Outdated Show resolved Hide resolved
hack/builder/Dockerfile Outdated Show resolved Hide resolved
hack/builder/publish.sh Outdated Show resolved Hide resolved
@kubevirt-bot kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jan 30, 2024
@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 Jan 30, 2024
@fabiand
Copy link
Member

fabiand commented Jan 30, 2024

Howdy.

Please include an OWNERS file to specify who is owning s390 support.

Setting a

/hold

until the file is added.

For example: Add a new member to OWNERS in #11046 with a comment that they/she/he is covering for s390.

@dhiller @brianmcarey can help to do this modification

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2024
@brianmcarey
Copy link
Member

brianmcarey commented Jan 30, 2024

Howdy.

Please include an OWNERS file to specify who is owning s390 support.

Setting a

/hold

until the file is added.

For example: Add a new member to OWNERS in #11046 with a comment that they/she/he is covering for s390.

I think this is something we will have to do retroactively as these are some of the first s390x contributions - the contributors are not yet members of the kubevirt org and can't be added to any special interest groups as a result.

What do you think?

@dhiller @brianmcarey can help to do this modification

@vamsikrishna-siddu vamsikrishna-siddu force-pushed the kubevirt-s390x-builder branch 3 times, most recently from e3a7efd to d00de29 Compare January 31, 2024 05:02
brianmcarey added a commit to brianmcarey/project-infra that referenced this pull request Jan 31, 2024
There is an effort to enable support for s390x for KubeVirt[1].

This adds an s390x bootstrap image to our multiarch publish flow.

This bootstrap image does not include bazel as it will have to rely on
the s390x builder image[2] for bazel.

This image will allow us to run native s390x builds and unit tests on a
s390x cluster[3].

[1] kubevirt/kubevirt#10490
[2] kubevirt/kubevirt#11097
[3] kubevirt#3140

Signed-off-by: Brian Carey <bcarey@redhat.com>
hack/builder/Dockerfile Outdated Show resolved Hide resolved
@vamsikrishna-siddu
Copy link
Author

Hello @brianmcarey @alicefr As the Bazel support is not present for s390x. We have the below proposal for enabling the kubevirt on s390x. We want to know from your side whether we can go ahead with this proposal?

Unit Test
We will run the unit-tests using Golang until we get the Bazel support on s390x. We will publish the kubevirt builder image without Bazel to the public and use that image to run the unit tests using make go-test.

E2E Test
At present we can't publish Bazel binary for s390x. We will use our own kubevirt builder image (which contains Bazel) from our internal registry, and we will override the used image using environment variable to run the e2e tests.

@brianmcarey
Copy link
Member

Hello @brianmcarey @alicefr As the Bazel support is not present for s390x. We have the below proposal for enabling the kubevirt on s390x. We want to know from your side whether we can go ahead with this proposal?

Hi @vamsikrishna-siddu

Unit Test We will run the unit-tests using Golang until we get the Bazel support on s390x. We will publish the kubevirt builder image without Bazel to the public and use that image to run the unit tests using make go-test.

E2E Test At present we can't publish Bazel binary for s390x. We will use our own kubevirt builder image (which contains Bazel) from our internal registry, and we will override the used image using environment variable to run the e2e tests.

Would you be able to publish this builder image to quay.io ? In this case we could just use this image for both the unit test and e2e tests - this would satisfy CI. It would have to be documented that contributors wishing to build or test against s390x would have to use this builder image.

@dhiller @xpivarc - what do you think of this ?

Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

Actually looking at the commits again - there is further more tidying up required - the changes in this PR could be handled in a single commit.

For instance - the following commits are no longer relevant:

/lgtm cancel

@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2024
@vamsikrishna-siddu
Copy link
Author

Actually looking at the commits again - there is further more tidying up required - the changes in this PR could be handled in a single commit.

For instance - the following commits are no longer relevant:

/lgtm cancel

Hi @brianmcarey I put all the changes into a single commit and updated the PR. Please review it again. Thanks

Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

Thanks @vamsikrishna-siddu - just one small nit left and then should be good.

hack/builder/Dockerfile Outdated Show resolved Hide resolved
Copy link
Member

@brianmcarey brianmcarey 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 @vamsikrishna-siddu

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2024
@dhiller
Copy link
Contributor

dhiller commented Mar 15, 2024

Howdy.

Please include an OWNERS file to specify who is owning s390 support.

Setting a

/hold

until the file is added.

For example: Add a new member to OWNERS in #11046 with a comment that they/she/he is covering for s390.

@dhiller @brianmcarey can help to do this modification

@vamsikrishna-siddu the requested change wrt ownership is still missing. I know that this is currently not possible since you are not (yet) a member of the org, but someone in sig-buildsystem needs to own this.

So I would suggest you apply for KubeVirt membership (see here how to do this), and then we can add you to the org.

I believe we should make an exception for this case wrt/ contributions, and I think this is fair, since addition of a new build architecture is significant enough.

WDYT @fabiand @vladikr @davidvossel ?

@dhiller
Copy link
Contributor

dhiller commented Mar 15, 2024

Also WDYT @aburdenthehand ?

@dhiller
Copy link
Contributor

dhiller commented Mar 15, 2024

Howdy.
Please include an OWNERS file to specify who is owning s390 support.
Setting a
/hold
until the file is added.
For example: Add a new member to OWNERS in #11046 with a comment that they/she/he is covering for s390.
@dhiller @brianmcarey can help to do this modification

@vamsikrishna-siddu the requested change wrt ownership is still missing. I know that this is currently not possible since you are not (yet) a member of the org, but someone in sig-buildsystem needs to own this.

So I would suggest you apply for KubeVirt membership (see here how to do this), and then we can add you to the org.

I believe we should make an exception for this case wrt/ contributions, and I think this is fair, since addition of a new build architecture is significant enough.

WDYT @fabiand @vladikr @davidvossel ?

WRT sponsorship you can add @dhiller since I'm happy to sponsor it . maybe @brianmcarey also, I guess?

@vamsikrishna-siddu
Copy link
Author

Howdy.
Please include an OWNERS file to specify who is owning s390 support.
Setting a
/hold
until the file is added.
For example: Add a new member to OWNERS in #11046 with a comment that they/she/he is covering for s390.
@dhiller @brianmcarey can help to do this modification

@vamsikrishna-siddu the requested change wrt ownership is still missing. I know that this is currently not possible since you are not (yet) a member of the org, but someone in sig-buildsystem needs to own this.

So I would suggest you apply for KubeVirt membership (see here how to do this), and then we can add you to the org.

I believe we should make an exception for this case wrt/ contributions, and I think this is fair, since addition of a new build architecture is significant enough.

WDYT @fabiand @vladikr @davidvossel ?

Hi @dhiller I am successfully now a member of kubevirt organization. can you please add me to the required special interest group. Thanks

@dhiller
Copy link
Contributor

dhiller commented Mar 27, 2024

Howdy.
Please include an OWNERS file to specify who is owning s390 support.
Setting a
/hold
until the file is added.
For example: Add a new member to OWNERS in #11046 with a comment that they/she/he is covering for s390.
@dhiller @brianmcarey can help to do this modification

@vamsikrishna-siddu the requested change wrt ownership is still missing. I know that this is currently not possible since you are not (yet) a member of the org, but someone in sig-buildsystem needs to own this.
So I would suggest you apply for KubeVirt membership (see here how to do this), and then we can add you to the org.
I believe we should make an exception for this case wrt/ contributions, and I think this is fair, since addition of a new build architecture is significant enough.
WDYT @fabiand @vladikr @davidvossel ?

Hi @dhiller I am successfully now a member of kubevirt organization. can you please add me to the required special interest group. Thanks

Hey @vamsikrishna-siddu I'd suggest you create a PR against the OWNERS_ALIASES file in kubevirt/kubevirt, adding you to the sig-buildsystem-reviewers group, maybe with a comment after your name to make clear what you are owning.

@dhiller
Copy link
Contributor

dhiller commented Apr 8, 2024

/release-note-edit add s390x support for kubevirt builder

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed 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 Apr 8, 2024
Barakmor1 pushed a commit to Barakmor1/project-infra that referenced this pull request Apr 24, 2024
…virt#3197)

* bootstrap,s390x: Add s390x support to multiarch bootstrap build

There is an effort to enable support for s390x for KubeVirt[1].

This adds an s390x bootstrap image to our multiarch publish flow.

This bootstrap image does not include bazel as it will have to rely on
the s390x builder image[2] for bazel.

This image will allow us to run native s390x builds and unit tests on a
s390x cluster[3].

[1] kubevirt/kubevirt#10490
[2] kubevirt/kubevirt#11097
[3] kubevirt#3140

Signed-off-by: Brian Carey <bcarey@redhat.com>

* bootstrap: Remove older version of Bazel from bootstrap

Signed-off-by: Brian Carey <bcarey@redhat.com>

---------

Signed-off-by: Brian Carey <bcarey@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label May 27, 2024
@kubevirt-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@vamsikrishna-siddu
Copy link
Author

Hi @fabiand @dhiller ,
with the recent communication in google-groups regarding this PR #11614 . I got that you are planning to implement working-groups to specify the stakeholders for the respective arches. I assume it will take some time to get that feature. meanwhile is it possible to merge this Kubevirt builder pr as it helps us to enable CI and kubevirt on s390x. Once the working-groups feature is out. We can add the contributors there.

@fabiand
Copy link
Member

fabiand commented May 29, 2024

@vamsikrishna-siddu yes, working groups.

It's not a technical feature, it's more about defining ownerhsip, and specific owners, for archtiectures.

Please join the call to speed up this process.

@vamsikrishna-siddu vamsikrishna-siddu force-pushed the kubevirt-s390x-builder branch 2 times, most recently from cde4059 to ce3e5df Compare June 5, 2024 03:29
Signed-off-by: VamsikrishnaSiddu <vamsikrishna.siddu@ibm.com>
@kubevirt-bot
Copy link
Contributor

@vamsikrishna-siddu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-e2e-k8s-1.29-sig-storage 82abac1 link true /test pull-kubevirt-e2e-k8s-1.29-sig-storage
pull-kubevirt-conformance-arm64 96e4378 link false /test pull-kubevirt-conformance-arm64

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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/network sig/storage size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants