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 rpms for s390x #11745

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

Add rpms for s390x #11745

wants to merge 1 commit into from

Conversation

jschintag
Copy link
Contributor

@jschintag jschintag commented Apr 18, 2024

Add sandbox and ldd_s390x definitions to rpm/BUILD.bazel. Declare rpmtrees with symlinks and capabilities in rpm/BUILD.bazel. Add centos stream9 repos for s390x.
Run make rpm-deps for s390x.

What this PR does

Adds the s390x rpm declarations to bazeldnf files, to allow for caching. Does not affect any code or builds, as this is not used by anything in kubevirt yet.
Split off from #10490

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:
qemu and libvirt versions are higher than x86 and arm64, as the old ones are no longer available on the centos mirror. They will be bumped by #11256

The following alternatives were considered:
Waiting for #11256 to merge first. However it is currently blocked.

Links to places where the discussion took place:

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

NONE

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. 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. sig/buildsystem Denotes an issue or PR that relates to changes in the build system. size/XXL labels Apr 18, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jean-edouard for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jschintag
Copy link
Contributor Author

/cc @andreabolognani

@andreabolognani I wanted to wait for #11256 before doing this, but since it is blocked i decided to go ahead.
I did not change the rpm versions in the script, but instead used enviroment variables to override it. Is that sufficient or should i add special, s390x only, temporary variables for this? My reason being that this way, when a fixed libvirt version is released you can just re-run make rpm-deps on your PR and we do not need to revert anything.

@andreabolognani
Copy link
Contributor

I wanted to wait for #11256 before doing this, but since it is blocked i decided to go ahead.
I did not change the rpm versions in the script, but instead used enviroment variables to override it. Is that sufficient or should i add special, s390x only, temporary variables for this? My reason being that this way, when a fixed libvirt version is released you can just re-run make rpm-deps on your PR and we do not need to revert anything.

@jschintag can you please explain the rationale for splitting this off the main s390x PR in the first place? Rushing to add s390x RPMs when nothing uses them yet doesn't make a whole lot of sense to me.

Now answering your question: I think we should have the variables, as explicit documentation of the fact that s390x is being treated in a special way. Undoing that as part of my PR is going to be trivial anyway, so don't even worry about that part :)

@jschintag
Copy link
Contributor Author

@jschintag can you please explain the rationale for splitting this off the main s390x PR in the first place? Rushing to add s390x RPMs when nothing uses them yet doesn't make a whole lot of sense to me.

Sure, the main point is to have them start being picked up by the caching job, so that i do not need to keep updating them. Additionally, it will make the main PR easier to review, as there is less that needs to be looked at.

Now answering your question: I think we should have the variables, as explicit documentation of the fact that s390x is being treated in a special way. Undoing that as part of my PR is going to be trivial anyway, so don't even worry about that part :)

Ok, than i will change it that way. I can also undo it myself after your PR merges.

@jschintag jschintag force-pushed the s390x-rpms branch 4 times, most recently from a92fe6e to 24866e3 Compare April 18, 2024 12:57
@jschintag jschintag changed the title WIP: Add rpms for s390x Add rpms for s390x Apr 18, 2024
@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 Apr 18, 2024
@andreabolognani
Copy link
Contributor

Sure, the main point is to have them start being picked up by the caching job, so that i do not need to keep updating them.

I'm not convinced this is a benefit. You could argue it's the exact opposite: we'd start mirroring to the Google Cloud storage RPMs that are going to become stale before the code that actually needs them is even merged. I won't actively stand in the way of this PR, but personally I would just drop it and let the work happening in #11256 and #10490 run its course. I understand this requires a bit more work on your side, but it still feels like the correct approach to me.

@jschintag
Copy link
Contributor Author

Sure, the main point is to have them start being picked up by the caching job, so that i do not need to keep updating them.

I'm not convinced this is a benefit. You could argue it's the exact opposite: we'd start mirroring to the Google Cloud storage RPMs that are going to become stale before the code that actually needs them is even merged. I won't actively stand in the way of this PR, but personally I would just drop it and let the work happening in #11256 and #10490 run its course. I understand this requires a bit more work on your side, but it still feels like the correct approach to me.

Well i think this does have advantages, though it does come with the risk of the packages growing stale before s390x lands upstream.
However it also means that when s390x support lands it will already be in sync with x86/aarch64 and not need to have newer package version because the old ones are no longer on the mirror. And it is not the case that the cached rpms would not be used, they would still support our s390x development effort. It would just not be used upstream (yet).
That being said i agree that there is not really a clear cut "best" approach to the topic.

@andreabolognani
Copy link
Contributor

However it also means that when s390x support lands it will already be in sync with x86/aarch64 and not need to have newer package version because the old ones are no longer on the mirror.

This would be a fairly convincing argument if this landed after #11256. I would have no objections at that point.

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented May 8, 2024

@jschintag: The following test 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-compute 24866e3 link true /test pull-kubevirt-e2e-k8s-1.29-sig-compute

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

Add sandbox and ldd_s390x definitions to rpm/BUILD.bazel.
Declare rpmtrees with symlinks and capabilities in rpm/BUILD.bazel.
Add centos stream9 repos for s390x.
Run make rpm-deps for s390x.

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>
@jschintag
Copy link
Contributor Author

@andreabolognani I have removed the special casing from this PR and updated the rpms.
What do you think, could we merge this so that no matter how long #10490 is taking for review, we will already have s390x rpms cached and in sync with the other arches?

targetcli
util-linux
which
"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now the same as testimage_main so there's no point in defining it. Make sure you update the caller below as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once that's been taken care of, I have no objections to the PR being merged.

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. release-note-none Denotes a PR that doesn't merit a release note. sig/buildsystem Denotes an issue or PR that relates to changes in the build system. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants