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
support for arm64 #454
support for arm64 #454
Conversation
Hi @Prashanth684. Thanks for your PR. I'm waiting for a code-ready member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
We do not have the equipment to confirm this works. Can you provide tests, or a means to ensure consistency/stability for follow-up changes we make to the general SNC codebase. |
/ok-to-test |
so "we" as in the multi-arch team at RedHat use ARM systems that we loan from beaker. That being said long term, the plan is to have CI on ARM starting mainly with AWS but we could also look to extend it to have a CI setup for crc which would be necessary. Let me bring this up for further discussion in our forums. |
/test e2e-snc |
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.
If these are the only changes needed to have snc running successfully on aarch64 hardware, then I'm happy to see it's so straightforward :)
Only remaining question is how we do test this.
Fwiw, if no additional changes are needed, I'd be fine with merging this even if aarch64 is unsupported from our end.
@Prashanth684 Have you tested this PR, like generated bundle for arm and able to run those even on beaker infra? |
good point @praveenkumar . the single node cluster itself came up fine, but the createdisk.sh command to create the images pulls amd64 images from quay which will not work. can you point us to how to build these images so we can build them manifest listed for amd64 and arm64? these are the images in question:
|
The dnsmasq image is there https://github.com/code-ready/crc/tree/master/images/dnsmasq Some of these images can be built on osbs, I'd have to check if it can be used to generate aarch64 images. |
@Prashanth684 The images are built on osbs (Thanks to @cfergeau) and uploaded to quay so it should pull platform specific. |
many thanks again @cfergeau ! i will use these and run tests again and let you know. |
4ab11e9
to
f3a1872
Compare
with the developer preview of ARM available for 4.9, adding support in it for SNC so we can work toward having crc support for macs
ok. updated and rebased. i was able to generate a crc bundle. @praveenkumar @cfergeau any docs on how i can run crc with a custom bundle? |
@Prashanth684 if you build crc from git,
|
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.
Looks good to me, a few questions/suggestions, but nothing blocking.
@@ -66,7 +66,7 @@ echo "Setting OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE to ${OPENSHIFT_INSTALL_RE | |||
# Extract openshift-install binary if not present in current directory | |||
if test -z ${OPENSHIFT_INSTALL-}; then | |||
echo "Extracting OpenShift baremetal installer binary" | |||
${OC} adm release -a ${OPENSHIFT_PULL_SECRET_PATH} extract ${OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE} --command=openshift-baremetal-install --to . | |||
${OC} adm release extract -a ${OPENSHIFT_PULL_SECRET_PATH} ${OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE} --command=openshift-baremetal-install --to . |
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.
oh, oc
became strict about parameter order?
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.
yes looks like it....this used to work for me before too
createdisk.sh
Outdated
@@ -86,7 +86,7 @@ kernel_release=$(${SSH} core@api.${CRC_VM_NAME}.${BASE_DOMAIN} -- 'uname -r') | |||
kernel_cmd_line=$(${SSH} core@api.${CRC_VM_NAME}.${BASE_DOMAIN} -- 'cat /proc/cmdline') | |||
|
|||
# SCP the vmlinuz/initramfs from VM to Host in provided folder. | |||
${SCP} core@api.${CRC_VM_NAME}.${BASE_DOMAIN}:/boot/ostree/${BASE_OS}-${ostree_hash}/* $1 | |||
${SCP} -r core@api.${CRC_VM_NAME}.${BASE_DOMAIN}:/boot/ostree/${BASE_OS}-${ostree_hash}/* $1 |
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.
The commands from ostree_hash=
to here could actually be wrapped in a if [ -n "${SNC_GENERATE_MACOS_BUNDLE}" ]
as it's not going to be useful on linux/Windows.
I don't think hyperkit (which crc uses on macOS) works on M1 macs, so it might be a bit early to generate macOS bundles by default in snc.
Could you at least expand the commit log a bit, is this causing an error if we omit the -r
? Which additional files do we need from /boot/ostree
which are in subdirs?
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.
so for aarch64 the kernel pulls in DTB - https://src.fedoraproject.org/rpms/kernel/blob/f34/f/kernel.spec#_1533 as part of the bootloading process which is the extra directory that requires the -r.
Yes, so looks like hyperkit is not yet available for arm64 so I would just want to get these changes in for now so that in the future when it is available we can start on the CRC side of things.
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.
changed to have this block under the conditional.
/retest |
So i did try and the pre-flight check failed with finding the virtualization device which doesn't seem to be present on any hardware we have today. So, i think for now I would just like to get this in so we at least have support to create bundles so we can investigate more when we get an ARM system with virtualization hardware or hopefully an M1 which we can test with. But again as @cfergeau pointed out hyperkit is not supported for arm64 so we will also have to wait for that. |
arm64 contains some dirs in the ostree content
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau, Prashanth684 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 |
/retest |
@Prashanth684: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
with the developer preview of ARM available for 4.9, adding support in it for SNC so we can work toward having crc support for macs