-
Notifications
You must be signed in to change notification settings - Fork 24
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 for ubuntu and centos artifacts #203
base: main
Are you sure you want to change the base?
Add s390x for ubuntu and centos artifacts #203
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Hi @aditi-sharma-1. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
Signed-off-by: Aditi Sharma <[email protected]>
6e51630
to
c25d68a
Compare
@@ -28,27 +28,31 @@ var staticRegistry = []Entry{ | |||
Artifacts: []api.Artifact{ | |||
centosstream.New("9", "x86_64", &docs.UserData{Username: "cloud-user"}, defaultEnvVariables("u1.medium", "centos.stream9")), | |||
centosstream.New("9", "aarch64", &docs.UserData{Username: "cloud-user"}, defaultEnvVariables("u1.medium", "centos.stream9")), | |||
centosstream.New("9", "s390x", &docs.UserData{Username: "cloud-user"}, defaultEnvVariables("u1.medium", "centos.stream9")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do instancetypes in their current form apply to the s390x architecture? If not they should be dropped here and below. Same for the preferences.
Did you verify the created images work on s390x? (medius images publish
+ medius images verify
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @0xFelix ,
Sure, looking into it. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check if VMs created with these instancetypes / preferences work on s390x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @0xFelix ,
Yes, I have checked, and it's working. I am attaching the logs below for your reference. Thank you.
s390x-build.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first executed bin/medius images push --target-registry=localhost:5000 --dry-run=false --insecure-skip-tls --force --focus=ubuntu:22.04
, I encountered the following error:
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"
Since I was trying to build it locally on s390x, I commented out the section of code in cmd/medius/images/push.go
(from line 291 to line 304) that checks the quay.io manifest and modified the function to return true. I have included the git diff output in the logs for your reference. After making this change, the execution was successful. I have also done a docker pull
to verify the images (ubuntu:22.04 and centos-stream:9
) created for the s390x architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if you have any suggestions or specific requirements for this implementation. I appreciate your guidance! Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented out the section of code [...] that checks the quay.io manifest
There is a --source-registry
flag to change the source registry. That might have helped, or for next time.
Did you run medius images verify
to verify the images? I'm interested to see if the built images were able to boot successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @0xFelix ,
I am trying to execute medius images verify
but facing some errors. I'm currently investigating the issue. I will keep you updated about the progress.
Thank you for your understanding!
Makefile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put these changes into a separate commit please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @0xFelix, Thanks for the review. Sure, will make these changes in a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you.
…using Go's environment. This change will support building containerdisks for s390x, amd64, and arm64 and other architectures. Signed-off-by: Aditi Sharma <[email protected]>
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:
Signed-off-by: Aditi Sharma [email protected]