feat: build multi-architecture images in by default #157

nestoracunablanco · 2024-07-16T14:32:49Z

This feature utilizes cross-compiling to generate binary files for the amd64 and s390x architectures, followed by the creation of images for both architectures.

What this PR does / why we need it: multiarch images enablement

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Release note:

openshift-ci · 2024-07-16T14:32:59Z

Hi @nestoracunablanco. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

akrejcir · 2024-07-16T14:49:48Z

Makefile

+	podman push ${IMG_REPOSITORY}:amd64-${IMG_TAG} && \
+	podman push ${IMG_REPOSITORY}:s390x-${IMG_TAG} && \
+	podman manifest push ${IMG}


I'm not familiar with multi-arch images. When the image is pulled, is the arch determined by the system where the podman is running? Can I just call podman run ${IMG} ?

It can be made this way, but this does not look like it to me. These docs should help: https://docs.podman.io/en/latest/markdown/podman-manifest.1.html#examples

@akrejcir, after you have added the manifest and the images to it, you can simply use 'podman run', and it will automatically select the appropriate architecture during the pull.

@0xFelix, you are correct. However, for this to work, the host must have the capability to execute non-native binaries. As I am not familiar with the specifics of your CI workers, I chose a more generic approach.

akrejcir · 2024-07-16T14:51:58Z

Dockerfile


+# amd64 target image
+FROM --platform=amd64 registry.access.redhat.com/ubi8/ubi-micro as amd64-build


Can the --platform be a parameter passed to podman when building? We could then avoid code duplication.

Yes, according to the docs this is possible.

0xFelix · 2024-07-16T15:00:52Z

@nestoracunablanco While we are at it, can you also add a build for arm64?

nestoracunablanco · 2024-07-17T11:56:36Z

@0xFelix, I have made the code in the Dockerfile more generic and adjusted the Makefile accordingly. I hope you find it more to your liking now.

akrejcir · 2024-07-18T08:48:51Z

/ok-to-test

0xFelix · 2024-07-18T12:32:25Z

Dockerfile

 RUN microdnf install -y make tar gzip which && microdnf clean all
+RUN export ARCH=$(uname -m | sed 's/x86_64/amd64/'); curl -L https://go.dev/dl/go1.21.6.linux-${ARCH}.tar.gz | tar -C /usr/local -xzf -


Suggested change

RUN export ARCH=$(uname -m | sed 's/x86_64/amd64/'); curl -L https://go.dev/dl/go1.21.6.linux-${ARCH}.tar.gz | tar -C /usr/local -xzf -

RUN ARCH=$(uname -m | sed 's/x86_64/amd64/') curl -L https://go.dev/dl/go1.21.6.linux-${ARCH}.tar.gz | tar -C /usr/local -xzf -

This change does not work with my podman version. How did you test it?

Ah yes, I see.

It would only work with something like this:

RUN ARCH=$(uname -m | sed 's/x86_64/amd64/') bash -c 'curl -L https://go.dev/dl/go1.21.6.linux-${ARCH}.tar.gz | tar -C /usr/local -xzf -'

Let's leave it like it is.

0xFelix · 2024-07-18T12:32:45Z

Dockerfile

-ENV PATH=$PATH:/usr/local/go/bin
+ARG TARGET_ARCH=amd64
+FROM registry.access.redhat.com/ubi8/ubi-minimal as base
+ARG TARGET_ARCH


Is it necessary to redefine TARGET_ARCH here?

Yes, it works with my Podman without any issues. However, it appears that in the container engine used in the CI, it does not work as expected. Essentially, if we do not reactivate the ARG in every stage, it will not be readable in the FROM statement on line 34. It took me a while to figure out why the tests were failing.

0xFelix · 2024-07-18T12:33:35Z

Dockerfile

-# Build
-RUN . ${ENVFILE}; CGO_ENABLED=0 GOOS=linux GOARCH=${ARCH} GO111MODULE=on make build
+# compile for the TARGET_ARCH
+RUN CGO_ENABLED=0 GOOS=linux GOARCH=${TARGET_ARCH} GO111MODULE=on make build


Suggested change

RUN CGO_ENABLED=0 GOOS=linux GOARCH=${TARGET_ARCH} GO111MODULE=on make build

RUN CGO_ENABLED=0 GOOS=linux GOARCH=${TARGET_ARCH} make build

That should be the default.

0xFelix · 2024-07-18T12:35:18Z

Makefile

@@ -24,11 +24,17 @@ build: fmt vet

 .PHONY: build-container
 build-container: fmt vet test
-	podman build -t ${IMG} .
+	podman manifest rm ${IMG} || true && \
+	podman build --build-arg TARGET_ARCH=amd64 --manifest=${IMG} -t ${IMG_REPOSITORY}:amd64-${IMG_TAG} . && \


Do we really need the additional $arch-${IMG_TAG} tags? IMO we can omit them. The manifest keeps track of all images.

You are correct that the manifest keeps track of them. However, when we upload to a container registry, these images can appear without any tagging. This could prevent accidental deletion. I can make changes if this is a concern for you.

Sounds like a bug in those registries to me if they don't make use of manifests? I'd remove the additional tags.

0xFelix · 2024-07-19T07:56:06Z

Dockerfile



-FROM registry.access.redhat.com/ubi8/ubi-minimal
+FROM --platform=linux/${TARGET_ARCH} registry.access.redhat.com/ubi8/ubi-micro


Can we keep ubi-minimal here?

You are making a good point. Here is why I changed it to ubi-micro:

When a container image is built, the FROM instruction checks if the image exists in the cache. If it does not, it downloads it.

The issue arises when the downloaded image is from a different architecture, causing the builder image instructions to fail.

In other words, the initial call of make build-container may succeed, but subsequent calls would fail.

Could you please explain why you chose the ubi-minimal image?

I was not aware that ubi-micro exists. It may be better than ubi-minimal, because it is smaller.
We should look into it and maybe use it.

If it is a technical problem to keep ubi-minimal, then it's ok to use ubi-micro.

If we keep ubi-micro, could we use it across all build stages?

@0xFelix, the same argument as before applies here. The underlying issue here is not about micro versus minimal. It is about how podman manages and utilizes existing images before building a new one.

For instance, consider a scenario where there is a tagged image as registry.access.redhat.com/ubi8/ubi-micro from a different architecture in your system, and you have the following Dockerfile:

FROM registry.access.redhat.com/ubi8/ubi-micro RUN uname -m

The image build will fail because you are attempting to execute a binary from an incompatible architecture.

Therefore, I propose the approach of keeping the builder steps aligned with the host architecture, cross-compiling, using a different image source, while the target images are tailored for the target architecture.

Thank you for the explanation.

I don't quite understand, that means depending on the images I have on my host the build might fail? In this case it only works because ubi-micro needs to be pulled when using ubi-minimal in the stages before? Or does this only apply to multi-stage builds? From what you say it sounds like a common issue though?

Hey @0xFelix, if the ubi-minimal image on your system is from your architecture, it should work without any issues. This is because each time we build the image using ubi-micro, we specify the architecture using --platform=linux/${TARGET_ARCH}.

The example I mentioned 4 days ago was just to explain why I opted for this approach. The main issue here arises because podman or docker tries to use an existing image for building without checking if the architecture matches. As you mentioned, this is a common problem.

I tried it myself, and even though I got it working, for the builder we need to stay on ubi-minimal, because ubi-micro has no dnf to install dependencies of the build. But for the final image we should keep it.

akrejcir · 2024-07-23T11:46:00Z

Looks good to me.

/approve

kubevirt-bot · 2024-07-23T11:46:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrejcir

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:

~~OWNERS~~ [akrejcir]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

0xFelix · 2024-07-25T08:22:45Z

Dockerfile

-RUN echo "export ARCH=`uname -m | sed 's/x86_64/amd64/'`" >> ${ENVFILE}
-ENV PATH=$PATH:/usr/local/go/bin
+ARG TARGET_ARCH=amd64
+FROM registry.access.redhat.com/ubi8/ubi-minimal as base


Suggested change

FROM registry.access.redhat.com/ubi8/ubi-minimal as base

FROM registry.access.redhat.com/ubi9/ubi-minimal as base

Use the latest ubi here?

0xFelix · 2024-07-25T08:25:22Z

Dockerfile

+FROM base as builder

-RUN . ${ENVFILE}; curl -L https://go.dev/dl/go1.22.4.linux-${ARCH}.tar.gz | tar -C /usr/local -xzf -
+# in multistage builds the ARG must be renewed
+ARG TARGET_ARCH


Can we drop this extra FROM and ARG?

0xFelix · 2024-07-25T08:25:59Z

Dockerfile

+# in multistage builds the ARG must be renewed
+ARG TARGET_ARCH


Is it required to renew it here although it is not used below?

With the last change is no longer a requirement. Deleted.

0xFelix · 2024-07-25T08:26:41Z

Makefile

@@ -24,11 +24,14 @@ build: fmt vet

 .PHONY: build-container
 build-container: fmt vet test
-	podman build -t ${IMG} .
+	podman manifest rm ${IMG} || true && \
+	podman build --build-arg TARGET_ARCH=amd64 --manifest=${IMG} . && \


Can you add --platform args here to prevent the issue you experienced above?

That could potentially lead to additional issues. For instance, consider a scenario where your host can run applications designed for a different architecture. If this happens and the build fails silently due to incorrect parameters, the build process will run very slowly.

0xFelix · 2024-07-25T08:27:39Z

Dockerfile



-FROM registry.access.redhat.com/ubi8/ubi-minimal
+FROM --platform=linux/${TARGET_ARCH} registry.access.redhat.com/ubi8/ubi-micro


I tried it myself, and even though I got it working, for the builder we need to stay on ubi-minimal, because ubi-micro has no dnf to install dependencies of the build. But for the final image we should keep it.

nestoracunablanco · 2024-08-05T11:08:07Z

Hello @0xFelix, could you please specify if there are any aspects of the PR that require clarification? Additionally, could you indicate the level of detail you need?

This feature utilizes cross-compiling to generate binary files for the amd64, s390x and arm64 architectures, followed by the creation of images. Signed-off-by: Nestor Acuna-Blanco <nestor.acuna@ibm.com>

0xFelix

/lgtm

Thanks for adding the explanation.

kubevirt-bot · 2024-08-15T09:29:48Z

@akrejcir: new pull request created: #179

In response to this:

/cherry-pick release-v0.6

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 added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 16, 2024

kubevirt-bot requested review from 0xFelix and jcanocan July 16, 2024 14:32

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 16, 2024

kubevirt-bot added the size/M label Jul 16, 2024

akrejcir reviewed Jul 16, 2024

View reviewed changes

nestoracunablanco force-pushed the fix/multiarchMakefile branch from 5671f18 to db0b49e Compare July 17, 2024 11:42

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 18, 2024

nestoracunablanco force-pushed the fix/multiarchMakefile branch 2 times, most recently from ab7ae3c to 52a0b1c Compare July 18, 2024 10:01

0xFelix reviewed Jul 18, 2024

View reviewed changes

nestoracunablanco force-pushed the fix/multiarchMakefile branch 2 times, most recently from 7e7beca to 6a76eb4 Compare July 18, 2024 16:57

0xFelix reviewed Jul 19, 2024

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2024

nestoracunablanco force-pushed the fix/multiarchMakefile branch from 6a76eb4 to db0c550 Compare July 22, 2024 15:12

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2024

0xFelix reviewed Jul 25, 2024

View reviewed changes

nestoracunablanco force-pushed the fix/multiarchMakefile branch from db0c550 to 1a4900b Compare July 26, 2024 08:42

feat: build multi-architecture images in by default

ece5884

This feature utilizes cross-compiling to generate binary files for the amd64, s390x and arm64 architectures, followed by the creation of images. Signed-off-by: Nestor Acuna-Blanco <nestor.acuna@ibm.com>

nestoracunablanco force-pushed the fix/multiarchMakefile branch from 1a4900b to ece5884 Compare August 7, 2024 09:52

0xFelix approved these changes Aug 7, 2024

View reviewed changes

kubevirt-bot assigned 0xFelix Aug 7, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2024

kubevirt-bot merged commit 4e58179 into kubevirt:main Aug 7, 2024
5 checks passed

nestoracunablanco deleted the fix/multiarchMakefile branch August 7, 2024 11:42

kubevirt-bot mentioned this pull request Aug 15, 2024

[release-v0.6] feat: build multi-architecture images in by default #179

Closed


		# amd64 target image
		FROM --platform=amd64 registry.access.redhat.com/ubi8/ubi-micro as amd64-build

		RUN microdnf install -y make tar gzip which && microdnf clean all
		RUN export ARCH=$(uname -m \| sed 's/x86_64/amd64/'); curl -L https://go.dev/dl/go1.21.6.linux-${ARCH}.tar.gz \| tar -C /usr/local -xzf -

	RUN export ARCH=$(uname -m \| sed 's/x86_64/amd64/'); curl -L https://go.dev/dl/go1.21.6.linux-${ARCH}.tar.gz \| tar -C /usr/local -xzf -
	RUN ARCH=$(uname -m \| sed 's/x86_64/amd64/') curl -L https://go.dev/dl/go1.21.6.linux-${ARCH}.tar.gz \| tar -C /usr/local -xzf -

	RUN CGO_ENABLED=0 GOOS=linux GOARCH=${TARGET_ARCH} GO111MODULE=on make build
	RUN CGO_ENABLED=0 GOOS=linux GOARCH=${TARGET_ARCH} make build



		FROM registry.access.redhat.com/ubi8/ubi-minimal
		FROM --platform=linux/${TARGET_ARCH} registry.access.redhat.com/ubi8/ubi-micro

	FROM registry.access.redhat.com/ubi8/ubi-minimal as base
	FROM registry.access.redhat.com/ubi9/ubi-minimal as base

		# in multistage builds the ARG must be renewed
		ARG TARGET_ARCH

feat: build multi-architecture images in by default #157

feat: build multi-architecture images in by default #157

Conversation

nestoracunablanco commented Jul 16, 2024

openshift-ci bot commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xFelix commented Jul 16, 2024

nestoracunablanco commented Jul 17, 2024 • edited Loading

akrejcir commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nestoracunablanco Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nestoracunablanco Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir commented Jul 23, 2024

kubevirt-bot commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nestoracunablanco commented Aug 5, 2024

0xFelix left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Aug 15, 2024

nestoracunablanco commented Jul 17, 2024 •

edited

Loading

nestoracunablanco Jul 19, 2024 •

edited

Loading

nestoracunablanco Jul 19, 2024 •

edited

Loading