containerdisks icon indicating copy to clipboard operation
containerdisks copied to clipboard

Add s390x for ubuntu and centos artifacts

Open aditi-sharma-1 opened this issue 1 year ago • 2 comments

What this PR does / why we need it: This pull request adds s390x architecture in Containerdisks for Ubuntu and CentOS artifacts . These changes ensure compatibility and resolve build issues encountered on the s390x architecture.

Release note:

Add s390x artifacts

Signed-off-by: Aditi Sharma [email protected]

aditi-sharma-1 avatar Oct 11 '24 10:10 aditi-sharma-1

Hi @aditi-sharma-1. 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.

kubevirt-bot avatar Oct 11 '24 10:10 kubevirt-bot

Hi @lyarwood @0xFelix, thank you for your review and comments on the PR.

I co-work with @aditi-sharma-1 on the s390x efforts.

I take this opportunity to explain to you the importance and urgency of this PR and why we need it merged on the s390x side at the earliest.

The downstream tests (OCP virt - tier 0/1 tests), has a tight dependency expecting the container disks images for fedora, centos, ubuntu to be available from quay.io. We are ticking on the timeline to unblock the downstream tests and to have a clean run next week.

kindly, is it possible to consider the urgency of the PR and a possibility to merge the changes with the confidence that we got the testing covered from our local s390x verification? (Aditi can give you the logs again, please refer https://github.com/kubevirt/containerdisks/pull/203#discussion_r1801649950). Meanwhile, we are also motivated to provide you a s390x CI environment but that would take some time.

so, is it possible to push the changes forward with so called label 'untested-on-CI' so we mark it now and re-visit later with motivation?

Also, for the related changes for s390x/fedora in this PR, we have covered the local s390x verification for it inclusively from the common-instance types. so, these changes are also good to get piggy backed.

Please let me know your thoughts. Thank you.

LakshmiRavichandran1 avatar Nov 05 '24 13:11 LakshmiRavichandran1

I have no problem adding the containerdisks, but I would ask you to remove the environment variables for instancetypes and preferences from them for now, as they don't exist yet.

0xFelix avatar Nov 05 '24 15:11 0xFelix

Hi @lyarwood @0xFelix, thank you for your review and comments on the PR.

I co-work with @aditi-sharma-1 on the s390x efforts.

I take this opportunity to explain to you the importance and urgency of this PR and why we need it merged on the s390x side at the earliest.

The downstream tests (OCP virt - tier 0/1 tests), has a tight dependency expecting the container disks images for fedora, centos, ubuntu to be available from quay.io. We are ticking on the timeline to unblock the downstream tests and to have a clean run next week.

kindly, is it possible to consider the urgency of the PR and a possibility to merge the changes with the confidence that we got the testing covered from our local s390x verification? (Aditi can give you the logs again, please refer #203 (comment)). Meanwhile, we are also motivated to provide you a s390x CI environment but that would take some time.

so, is it possible to push the changes forward with so called label 'untested-on-CI' so we mark it now and re-visit later with motivation?

Also, for the related changes for s390x/fedora in this PR, we have covered the local s390x verification for it inclusively from the common-instance types. so, these changes are also good to get piggy backed.

Please let me know your thoughts. Thank you.

Yeah appreciated, lets land this PR without the instance type and preference envs as suggested by @0xFelix and come back later once we have CI runners to ensure coverage. @aditi-sharma-1 can you update the PR and we will get it merged tomorrow?

lyarwood avatar Nov 05 '24 15:11 lyarwood

Hi @0xFelix, I have added the unit tests for the CentOS and Ubuntu artifacts.

I am currently working on the Fedora artifact and will raise a separate PR for it soon. This PR focuses on the Ubuntu and CentOS artifacts.

I’ve also attached the logs for building and verification of the Ubuntu and CentOS artifacts on s390x for your reference.

Thank you!

containerdisks_building&verification_onS390x.txt

aditi-sharma-1 avatar Nov 08 '24 09:11 aditi-sharma-1

/test all

0xFelix avatar Nov 08 '24 15:11 0xFelix

/approve cancel

Can you please check the CI failures? They are related.

0xFelix avatar Nov 08 '24 15:11 0xFelix

/retest-required

aditi-sharma-1 avatar Nov 09 '24 09:11 aditi-sharma-1

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

In response to this:

/retest-required

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.

kubevirt-bot avatar Nov 09 '24 09:11 kubevirt-bot

/ok-to-test

aditi-sharma-1 avatar Nov 09 '24 09:11 aditi-sharma-1

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

In response to this:

/ok-to-test

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.

kubevirt-bot avatar Nov 09 '24 09:11 kubevirt-bot

/test all

0xFelix avatar Nov 11 '24 10:11 0xFelix

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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 avatar Nov 11 '24 14:11 kubevirt-bot

/hold

time="2024-11-11T14:39:35Z" level=error msg="error introspecting image \"quay.io/containerdisks/ubuntu:24.04\": Error parsing manifest for image: choosing image instance: no image found in manifest list for architecture \"s390x\", variant \"\", OS \"linux\"" name=ubuntu version=24.04
time="2024-11-11T14:39:35Z" level=error msg="error introspecting image \"quay.io/containerdisks/ubuntu:22.04\": Error parsing manifest for image: choosing image instance: no image found in manifest list for architecture \"s390x\", variant \"\", OS \"linux\"" name=ubuntu version=22.04

I don't recall hitting this with the arm64 images? Looking now.

lyarwood avatar Nov 11 '24 16:11 lyarwood

Reproduces locally FWIW....

$ make medius
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/medius kubevirt.io/containerdisks/cmd/medius
$ bin/medius images push --target-registry=localhost:5000 --dry-run=false --insecure-skip-tls --focus=ubuntu:22.04
INFO[0005] Latest containerdisk checksum: "55c687a9a242fab7b0ec89ac69f9def77696c4e160e6f640879a0b0031a08318"  name=ubuntu version=22.04
INFO[0008] Latest containerdisk checksum: "2edb369b85141fbeff6c87c7d92e08f315ec236f639235e35429e4930d98e2de"  name=ubuntu version=22.04
ERRO[0009] error introspecting image "quay.io/containerdisks/ubuntu:22.04": Error parsing manifest for image: choosing image instance: no image found in manifest list for architecture "s390x", variant "", OS "linux"  name=ubuntu version=22.04
INFO[0009] Writing results file                         
FATA[0009] error introspecting image "quay.io/containerdisks/ubuntu:22.04": Error parsing manifest for image: choosing image instance: no image found in manifest list for architecture "s390x", variant "", OS "linux" 

lyarwood avatar Nov 11 '24 16:11 lyarwood

The error is coming from github.com/containers/image/v5, quick and dirty workaround below works for me for now:

git diff
diff --git a/cmd/medius/images/push.go b/cmd/medius/images/push.go
index 36455d34..71a5f1ea 100644
--- a/cmd/medius/images/push.go
+++ b/cmd/medius/images/push.go
@@ -176,6 +176,8 @@ func (b *buildAndPublish) handleMetadataError(imageName string, err error) error
                b.Log.Info("Tag does not yet exist, it will be created")
        case repository.IsTagUnknownError(err):
                b.Log.Info("Tag is gone but seems to have existed already, it will be created")
+       case repository.IsArchUnknownError(err):
+               b.Log.Info("Image with arch does not exist yet, it will be created")
        default:
                return fmt.Errorf("error introspecting image %q: %v", imageName, err)
        }
diff --git a/pkg/repository/repository.go b/pkg/repository/repository.go
index 4bb19f99..1bd9795c 100644
--- a/pkg/repository/repository.go
+++ b/pkg/repository/repository.go
@@ -159,6 +159,10 @@ func IsTagUnknownError(err error) bool {
        return false
 }
 
+func IsArchUnknownError(err error) bool {
+       return strings.Contains(err.Error(), "no image found in manifest list for architecture")
+}
+
 func getErrorCode(err error) errcode.ErrorCoder {
        for {
                if unwrapped := errors.Unwrap(err); unwrapped != nil {
$ make medius
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/medius kubevirt.io/containerdisks/cmd/medius

$ bin/medius images push --target-registry=localhost:5000 --dry-run=false --insecure-skip-tls --focus=ubuntu:22.04
INFO[0005] Latest containerdisk checksum: "55c687a9a242fab7b0ec89ac69f9def77696c4e160e6f640879a0b0031a08318"  name=ubuntu version=22.04
INFO[0010] Latest containerdisk checksum: "2edb369b85141fbeff6c87c7d92e08f315ec236f639235e35429e4930d98e2de"  name=ubuntu version=22.04
INFO[0011] Image with arch does not exist yet, it will be created  name=ubuntu version=22.04
INFO[0011] Rebuild needed, downloading "https://cloud-images.ubuntu.com/releases/22.04/release/ubuntu-22.04-server-cloudimg-amd64.img" ...  name=ubuntu version=22.04
INFO[0017] Building containerdisk ...                    name=ubuntu version=22.04
INFO[0019] Rebuild needed, downloading "https://cloud-images.ubuntu.com/releases/22.04/release/ubuntu-22.04-server-cloudimg-arm64.img" ...  name=ubuntu version=22.04
INFO[0024] Building containerdisk ...                    name=ubuntu version=22.04
INFO[0026] Rebuild needed, downloading "https://cloud-images.ubuntu.com/releases/22.04/release/ubuntu-22.04-server-cloudimg-s390x.img" ...  name=ubuntu version=22.04
INFO[0031] Building containerdisk ...                    name=ubuntu version=22.04
INFO[0032] Pushing localhost:5000/ubuntu:22.04-2411111627 image index  name=ubuntu version=22.04
INFO[0034] Pushing localhost:5000/ubuntu:22.04 image index  name=ubuntu version=22.04
INFO[0034] Writing results file

$ podman search --list-tags --no-trunc localhost:5000/ubuntu
NAME                   TAG
localhost:5000/ubuntu  22.04-2411111627
localhost:5000/ubuntu  22.04

$ podman pull --arch s390x localhost:5000/ubuntu:22.04
Trying to pull localhost:5000/ubuntu:22.04...
Getting image source signatures
Copying blob sha256:e7cd03db0bb75f3780e80df734c614f2dea4e4e6a3a45e9315b379edd29a9238
Copying config sha256:ae7d5c8454fc402b2db723838bd94f2e25e84e92a9b6d12f0f921a775893a6e8
Writing manifest to image destination
ae7d5c8454fc402b2db723838bd94f2e25e84e92a9b6d12f0f921a775893a6e8

lyarwood avatar Nov 11 '24 16:11 lyarwood

/hold cancel

lyarwood avatar Nov 11 '24 16:11 lyarwood

/lgtm

lyarwood avatar Nov 11 '24 17:11 lyarwood

Hi @0xFelix, @lyarwood

Thank you for reviewing and merging the PR. I really appreciate the feedback and collaboration, and look forward to contributing more!!

aditi-sharma-1 avatar Nov 12 '24 06:11 aditi-sharma-1

The error is coming from github.com/containers/image/v5, quick and dirty workaround below works for me for now

@lyarwood Thats more of a bug on our side. I knew there was handling for unknown tags in medius (hence my comment), but I did not know that unknown archs have to be handled separately. Thanks for addressing it!

Thanks for you contribution @aditi-sharma-1! :)

0xFelix avatar Nov 12 '24 09:11 0xFelix

The error is coming from github.com/containers/image/v5, quick and dirty workaround below works for me for now

@lyarwood Thats more of a bug on our side. I knew there was handling for unknown tags in medius (hence my comment), but I did not know that unknown archs have to be handled separately. Thanks for addressing it!

Yeah I think that error in github.com/containers/image/v5 is new however as I don't recall it being an issue with the arm64 disks, still good to get this landed finally.

lyarwood avatar Nov 12 '24 09:11 lyarwood