-
Notifications
You must be signed in to change notification settings - Fork 28
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
Implementing aarch64 image build capabilities #475
Conversation
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.
Absolutely insane to me how small this patch is. I haven't tested it yet but will shortly. I just added the one quick comment about the comments in the Dockerfile.
Dockerfile
Outdated
@@ -27,6 +27,7 @@ FROM opensuse/leap:15.5 | |||
# 4. RPM resolution logic | |||
# 5. Embedded artefact registry | |||
# 6. Network configuration | |||
# 7. RAW image modification on aarch64 |
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.
This isn't needed here. The intention was to explain the contents of this one big ass RUN line, but the aarch64 stuff is done at a different level after this.
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.
Changes look good, added some comments which I’m curious to hear your thoughts on. The big unknown here is testing this on a variety of builds (especially with RPMs and embedded images) to see what breaks and what doesn’t.
ROOT_PART=/dev/sda3 | ||
|
||
# Make the necessarry adaptations for aarch64 | ||
if [[ $(uname -m) == "aarch64" ]]; then |
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 think we should use the variable that’s set to the arch in the image definition file and then template it in as it allows us to remove this if check and just rely on a single variable which is a little bit safer.
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 vote for doing this too - we should be able to simplify the script and move everything to Go code.
# Make the necessarry adaptations for aarch64 | ||
if [[ $(uname -m) == "aarch64" ]]; then | ||
if ! test -f /dev/kvm; then | ||
export LIBGUESTFS_BACKEND_SETTINGS=force_tcg |
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 personally believe that this should be the default behavior. Getting KVM to work in a container (in my experience) has been very error prone and while investigating the x86 EIB builds, I found that it resorts to “tcg” anyway so my opinion is that we just make use force_tcg as the default and avoid KVM.
Dockerfile
Outdated
fi | ||
|
||
# Clean up the repos to reduce size | ||
RUN zypper clean -a |
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.
This needs to be done in the same layer as the installation, otherwise it will still be there.
Not sure whether this should be covered by this PR, but have we tested whether the RPM resolution logic works with the suggested implementation? I did a quick test on an aarch64 Tumbleweed with your changes + an EIB image using |
# guestfish looks for very specific locations on the filesystem for UEFI firmware | ||
# and also expects the boot kernel to be a portable executable (PE), not ELF. | ||
RUN if [[ $(uname -m) == "aarch64" ]]; then \ | ||
zypper install -y qemu-uefi-aarch64; \ |
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.
How much space does this package take up? If it's trivial, it may make sense to go ahead and install the aarch64 emulator in the x86 image as well to make working on cross complications easier in the future.
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.
This is something we already do in the downstream builds.
Tested RPM resolution logic on an arm system using arm SLE Micro. Image was built successful. |
Resolved in #490. |
In this PR I introduce the ability to use an aarch64 system with EIB, that allows the user to generate either RAW or ISO images with the same architecture, i.e. it's not possible to cross-build with these patches (e.g. x86_64 image on aarch64, or vice versa) as it's a) not possible to currently use the target architecture for the type of images that we cache, nor the hauler/nmc binary architectures, and b) not possible to currently spin up guestfish with a different qemu architecture to be able to run the image modification tools, but I think this is a good step forward to enabling customers to build aarch64 images at this time.
Note that we will need to rebase on
leap:15.6
for ISO images, as the aarch64 ISO's need a newer version ofxorriso
to be able to repack the aarch64 ISO. I've tested these changes on both aarch64 and x86_64 based systems for both raw and iso (albeit with the previous comment in mind, I tested that pathway on 15.6).Credit for this largely goes to @dbw7 and @diconico07 for their prior investigation and inputs.
Fixes: #193