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

ci: containerdisks job for CentOS Stream on s390x architecture #3966

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

Conversation

nestoracunablanco
Copy link
Contributor

@nestoracunablanco nestoracunablanco commented Feb 19, 2025

This job verifies any changes related to container disks for CentOS
Stream on the s390x architecture. Given that the qcow image may
contain bugs, it is crucial to identify any issues in the early
stages.

Release note:

NONE

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Feb 19, 2025
@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 lyarwood for approval. For more information see the 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

@jcanocan
Copy link
Contributor

Could you please add some explanation why this is needed, and what will be the use of new CI?

This job verifies any changes related to container disks for CentOS
Stream on the s390x architecture. Given that the qcow image may
contain bugs, it is crucial to identify any issues in the early
stages.

Signed-off-by: Nestor Acuna Blanco <[email protected]>
@nestoracunablanco nestoracunablanco force-pushed the ci/centosStreamJobContainerdisks branch from d2223ff to 0480cb0 Compare February 19, 2025 14:48
@nestoracunablanco
Copy link
Contributor Author

nestoracunablanco commented Feb 19, 2025

Could you please add some explanation why this is needed, and what will be the use of new CI?

@jcanocan, done. Please let me know if you need any further explanation. To provide a bit more context:

  • The cluster prow-s390x-workloads is managed on IBM's end.
  • You may choose to believe me or not, but some qcow2 files for Alpine and Fedora on the s390x architecture have been buggy. This underscores the importance of testing in the early stages.

@jcanocan
Copy link
Contributor

Could you please add some explanation why this is needed, and what will be the use of new CI?

@jcanocan, done. Please let me know if you need any further explanation. To provide a bit more context:

Great. Thanks.

* The cluster prow-s390x-workloads is managed on IBM's end.

* You may choose to believe me or not, but some qcow2 files for Alpine and Fedora on the s390x architecture have been buggy. This underscores the importance of testing in the early stages.

Don't worry, I believe you! 😊 Yeah, I agree.

@jcanocan
Copy link
Contributor

I'm asking because I see we already have the pipeline pull-containerdisks-pipeline-centos-stream-9-s390x that it is always executed. Won't that be enough? Or, am I missing something?
Maybe we can just change the current to execute in that pipeline all centosstream versions.
Or the other way around, drop pull-containerdisks-pipeline-centos-stream-9-s390x and get this one in.

@nestoracunablanco
Copy link
Contributor Author

I understand your point now! The centos-9 and centos-9-s390x jobs run for all pull requests across all architectures (specifically, only centos-9). Their purpose is to test for any changes in the tooling that might impact the build of the container disks.

The centos and potentially future centos-s390x jobs are primarily intended to test all CentOS distributions (likely centos-9 and centos-10) in case there are modifications to the CentOS-related files.

One important aspect for us is maintaining test parity. If we create a job for every pull request for centos-9 and centos-10 for s390x, we should ensure the same applies for amd64. I can prepare another pull request for this if you’d like.

@jcanocan
Copy link
Contributor

Thinking about it twice, I think it would be nice to have both pull-containerdisks-pipeline-centos-stream-9-s390x and the proposed in this PR pull-containerdisks-pipeline-centosstream-s390x pipelines. This way we have the same ones in both archs. Apologizes for the noise @nestoracunablanco.

What do you think @0xFelix?

@0xFelix
Copy link
Member

0xFelix commented Feb 24, 2025

I'm still not convinced that we need to run these pipelines with multiple archs.
This pipeline is responsible for testing the medius / containerdisks tool. It is not resposible for verifying the built containerdisks.

The periodic pipeline is responsible for building, verifying and pushing new containerdisks / qcow images. So even if this job would fail for whatever reason, as long as the periodic pipeline runs, the containerdisks are updated anyway. If we want to ensure that the containerdisks we push really work, we need to extend the periodic pipeline, to verify all arches of freshly built containerdisks before pushing them.

@0xFelix
Copy link
Member

0xFelix commented Feb 24, 2025

/hold

@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 Feb 24, 2025
@nestoracunablanco
Copy link
Contributor Author

@0xFelix, I agree that the periodic job needs enhancement, and I have a related commit ready to be pushed. However, what is the downside of testing it before a change is merged? The periodic pipeline tests the main branch, and if it fails, the container disks are not pushed (currently verifies only amd64). In contrast, this CI job ensures that any changes that could break the CentOS Stream images on s390x do not get merged.

I believe both pipelines—the OS-specific one and the future periodic job—serve important purposes.

@0xFelix
Copy link
Member

0xFelix commented Feb 27, 2025

Why should an image in the same format work if it is built on amd64 but break if is built on s390x? In general the possibility is there, but I don't see the value of this lane. Our periodic pipeline also only runs on amd64.

I still think the right way is to verify images on all architectures before publishing them in the periodic pipeline.

@nestoracunablanco
Copy link
Contributor Author

@0xFelix, you are absolutely right about the periodic pipeline! I plan to implement it in the near future, along with additional changes.

Regarding the current situation: For instance, Fedora 40 is not functioning on s390x at the moment, and there have also been issues with Alpine Linux qcow images. The periodic pipeline will help us identify issues, but it will do so only after changes have already been merged into the main branch. By implementing checks at the pull request (PR) level, we can prevent the merging of problematic code and catch issues before they become part of the main codebase.

What is the downside of early error detection? After all, we are all human and prone to making mistakes.

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. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants