-
Notifications
You must be signed in to change notification settings - Fork 71
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
Fix: multiarch build by using crosscompilation #336
base: main
Are you sure you want to change the base?
Fix: multiarch build by using crosscompilation #336
Conversation
Hi @ykulazhenkov. Thanks for your PR. I'm waiting for a k8snetworkplumbingwg 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-sigs/prow repository. |
bb3536d
to
ce4b397
Compare
Additional context is in this message Example of the job with the code from this PR: |
@@ -27,9 +27,5 @@ jobs: | |||
with: | |||
context: . | |||
push: false | |||
# no need to explicitly set goarch, | |||
# correct arch will be selected for each build platform | |||
build-args: | |
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.
no need to set this anymore. Default for the goarch= build-arg is empty string.
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.
Nice thanks
cmd/Dockerfile
Outdated
@@ -1,35 +1,37 @@ | |||
FROM quay.io/centos/centos:stream9 as builder | |||
FROM --platform=$BUILDPLATFORM docker.io/golang:1.23 AS builder |
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.
go ver is hardcoded here instead taking from go.mod
i believe we can live with it, just raising for awareness in case you think otherwsie
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 it is totally acceptable (and even beneficial) to use a newer version of Golang to build the binary. The 1.23 tag will provide us with the latest patch release of 1.23, including all fixes to known CVEs. When Go 1.24 is released, we can update the builder image to the latest version but keep 1.22 in the go.mod file. This approach allows us to be flexible about the compiler version (for downstream consumers who rebuild the image), while the “official” image build always uses a “fresh” version.
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.
It requires changing on more than one place, which isnt what we strive to do usually,
only rarely we need a different compiler than a language level imho
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.
Ok, let's use go.mod if this is standard practice in kubevirt-related projects. I updated PR to rollback to explicit go download.
ce4b397
to
4222e29
Compare
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.
Awesome thank you
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oshoval, ykulazhenkov 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 |
|
||
COPY . . | ||
RUN go mod download |
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.
curious, why we need this suddenly ?
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.
First, we copy go.mod
and go.sum
to download dependencies explicitly (go mod download
), then we copy other code files. This optimizes image rebuilds when only source code changes, allowing Docker to use cached dependencies.
This construction is employed to prevent re-downloading the dependencies whenever possible.
This is pretty common practice. For example, kubebuilder will generate Dockerfile that use such optimization https://github.com/kubernetes-sigs/kubebuilder/blob/cbc6e383c342f1337ab37ee4aa0755957a01f9c7/pkg/plugins/golang/v4/scaffolds/internal/templates/dockerfile.go#L46
@ykulazhenkov We should update the GOARCH ?= amd64 line (as seen in this Makefile) to: @oshoval changes look good to me, assuming the idea is to provide multiplatform support only through GitHub Actions, exclusively for the Docker container runtime, and not through make commands or for the Podman container runtime. |
thanks at the end we will do need also images that are multi platform though isnt it? (as the issue / PRs you work on) |
Yes, we can consider adding multiplatform support using a Makefile(using make commands) for both Docker and Podman in the future. This would allow us to build images on our local machine, in a specific environment, or within any CI system, giving us more control over the build process and environment. |
Use quay.io/centos/centos:stream9 with the –platform=$BUILDPLATFORM flag, in this case the image is pulled for the builder host's current architecture. The BUILDOS and BUILDARCH args are used to download the right go binary for the build platform. Cross-compilation occurs in the builder image using TARGETOS and TARGETARCH build arguments to determine the target OS/arch. These args are set automatically by the multiarch-build process (with docker buildx). The final container image (registry.access.redhat.com/ubi9/ubi-minimal) is pulled for the correct target architecture, such as amd64 or arm64. This update fixes support for multiarch builds and speeds up image building, as cross-compilation is faster than compiling on a non-native platform. Signed-off-by: Yury Kulazhenkov <[email protected]>
4222e29
to
d6a30e9
Compare
done |
What this PR does / why we need it:
Use quay.io/centos/centos:stream9 with the –platform=$BUILDPLATFORM flag,
in this case the image is pulled for the builder host's current architecture.
The BUILDOS and BUILDARCH args are used to download the right go binary for
the build platform.
Cross-compilation occurs in the builder image using TARGETOS and TARGETARCH build arguments to determine the target OS/arch.
These args are set automatically by the multiarch-build process (with docker buildx).
The final container image (registry.access.redhat.com/ubi9/ubi-minimal) is pulled for the correct target architecture, such as amd64 or arm64.
This update fixes support for multiarch builds and speeds up image building, as cross-compilation is faster than compiling on a non-native platform.
Special notes for your reviewer:
Fixes support for multiarch builds added in #307
Multiarch build was broken by #319
Release note: