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

s390x enablement WIP #3357

Merged
merged 9 commits into from
Aug 31, 2024
Merged

s390x enablement WIP #3357

merged 9 commits into from
Aug 31, 2024

Conversation

cfilleke
Copy link
Member

@cfilleke cfilleke commented Jul 25, 2024

What this PR does / why we need it:

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 #3356

Special notes for your reviewer:
WIP still needs rebase/merge conflict resolution

Release note:

enables s390x native build

Signed-off-by: cfilleke <[email protected]>
@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: 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 25, 2024
@kubevirt-bot
Copy link
Contributor

Hi @cfilleke. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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.

@awels
Copy link
Member

awels commented Jul 26, 2024

This is the one we want in, together with the fix for the unit test? I can waive the failing unit tests for this PR, so we can get this in so the other PR can pass. Although I think the other PR would have no issue merging even without this.

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Jul 26, 2024
@Davo911
Copy link
Contributor

Davo911 commented Jul 29, 2024

This is the one we want in, together with the fix for the unit test? I can waive the failing unit tests for this PR, so we can get this in so the other PR can pass. Although I think the other PR would have no issue merging even without this.

You're right, the unit test enablement can already be merged in without causing issues before this.
I'll convert it to a proper PR.

@cfilleke
Copy link
Member Author

If we're going to be using cross-compile with this, I'll need to merge the changes to the Dockerfile first; however, if we can build on the new cloud s390x external ci/cd cluster (and I would suggest we do that) then we need to configure that to build & register the build-the-builder container first; also have some questions about at what stage in the build-test lifecycle the make rpm-deps is executed. Thanks!

@cfilleke cfilleke marked this pull request as ready for review August 12, 2024 20:54
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2024
@kubevirt-bot kubevirt-bot requested review from aglitke and awels August 12, 2024 20:55
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2024
@dominikholler
Copy link

Building containerized-data-importer for s390x failed for me with

 final command line arguments: "build" "-o" "/remote-source/app/_out/cmd/cdi-importer/cdi-importer" "-tags" "strictfipsruntime" "-ldflags" "-extldflags $static_flag" "-ldflags" "-X 'kubevirt.io/containerized-data-importer/pkg/version.buildDate=2024-08-15T15:58:56Z'"
invoking real go binary
 # kubevirt.io/containerized-data-importer/pkg/util
 ../../pkg/util/util.go:116:9: invalid operation: int64(stat.Bavail) * stat.Bsize (mismatched types int64 and uint32)
Exited with: 1

@cfilleke
Copy link
Member Author

/retest

@kubevirt-bot
Copy link
Contributor

@cfilleke: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@cfilleke
Copy link
Member Author

/rehearse

@jschintag
Copy link
Contributor

/ok-to-test

@awels
Copy link
Member

awels commented Aug 26, 2024

Can you rebase? Seems like there are some issues there now.

@cfilleke
Copy link
Member Author

/test pull-cdi-verify-go-mod

@awels
Copy link
Member

awels commented Aug 26, 2024

To fix, run make generate and make generate-verify to ensure nothing is being generated

@cfilleke
Copy link
Member Author

Both make generate and make generate-verify on s390x require the existence of a kubevirt-cdi-bazel-builder container in the tagged manifest in quay.io which does not yet exist, as the code of this PR is part of the creation of that container.

The results of make generate and make generate-verify on s390x and x86 are in this gist: https://gist.github.com/cfilleke/3ab5bb4d386be007f70d9d97e574e5d4

pkg/importer/BUILD.bazel Outdated Show resolved Hide resolved
@cfilleke
Copy link
Member Author

cfilleke commented Aug 27, 2024

pull-cdi-unit-test fails with the same error that updating rpm/README.md in PR #3409 fails, so it's not clear that this would be resolved with a code change @awels

possibly they were both trying to run the same test at once, which caused both to fail?

@cfilleke
Copy link
Member Author

/test pull-cdi-unit-test

@cfilleke
Copy link
Member Author

/test pull-containerized-data-importer-e2e-nfs

1 similar comment
@cfilleke
Copy link
Member Author

/test pull-containerized-data-importer-e2e-nfs

@cfilleke
Copy link
Member Author

cfilleke commented Aug 28, 2024

All tests passed except 'tide' and this apears to now require only /lgtm /approve now @aglitke @alromeros @ShellyKa13 but it's still an XXL so please feel free to ask me questions. Thanks!

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2024
@kubevirt-bot kubevirt-bot added size/L and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL labels Aug 29, 2024
@coveralls
Copy link

coveralls commented Aug 29, 2024

Coverage Status

coverage: 59.144% (-0.003%) from 59.147%
when pulling 175826c on cfilleke:s390x-xc-rb
into bc7cf46 on kubevirt:main.

@cfilleke
Copy link
Member Author

/test pull-cdi-linter

@cfilleke
Copy link
Member Author

make all build test on s390x results here: https://gist.github.com/cfilleke/a4dcbad9a9ce9472ad3d74bdbedd4d62

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

@awels awels left a comment

Choose a reason for hiding this comment

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

/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2024
@kubevirt-bot kubevirt-bot merged commit e00c92a into kubevirt:main Aug 31, 2024
19 checks passed
universal-itengineer pushed a commit to deckhouse/3p-containerized-data-importer that referenced this pull request Oct 29, 2024
* s390x enablement

Signed-off-by: cfilleke <[email protected]>

* should resolve remaining merge conflicts

Signed-off-by: cfilleke <[email protected]>

* correct prev commit to resolve merge conflicts

Signed-off-by: cfilleke <[email protected]>

* to resolve generate-verify issue

Signed-off-by: cfilleke <[email protected]>

* provide registry container for s390x, vddk datasource stub fix and util data type fix

Signed-off-by: cfilleke <[email protected]>

* get the linter to fix things

Signed-off-by: cfilleke <[email protected]>

* fix linter issue

Signed-off-by: cfilleke <[email protected]>

---------

Signed-off-by: cfilleke <[email protected]>
Signed-off-by: cfillekes <[email protected]>
universal-itengineer pushed a commit to deckhouse/3p-containerized-data-importer that referenced this pull request Oct 29, 2024
* s390x enablement

Signed-off-by: cfilleke <[email protected]>

* should resolve remaining merge conflicts

Signed-off-by: cfilleke <[email protected]>

* correct prev commit to resolve merge conflicts

Signed-off-by: cfilleke <[email protected]>

* to resolve generate-verify issue

Signed-off-by: cfilleke <[email protected]>

* provide registry container for s390x, vddk datasource stub fix and util data type fix

Signed-off-by: cfilleke <[email protected]>

* get the linter to fix things

Signed-off-by: cfilleke <[email protected]>

* fix linter issue

Signed-off-by: cfilleke <[email protected]>

---------

Signed-off-by: cfilleke <[email protected]>
Signed-off-by: cfillekes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

s390x enablement
7 participants