Skip to content
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

Initial fixes to support multi-arch container building #2524

Merged

Conversation

haimgel
Copy link
Contributor

@haimgel haimgel commented Dec 10, 2023

Change Overview

Here are some assorted fixes to make it possible to build a multi-arch (amd64/arm64) container for Kanister:

  1. A tiny fix in the "build" container Dockerfile: it should not constrain the Docker apt repo to amd64 arch only, since arm64 binaries are available there as well. This change makes it possible to build a multi-arch "build" container.
  2. Pass ARCH from the Makefile to "goreleaser" and override whatever is specified in goreleaser config file.
  3. Revert one change in Specify platform as linux/${ARCH} when cross-building docker images #2503: The license extractor image should not be pinned to the architecture of the container that is being built, they are not related. We should be using the native architecture of the host if possible.

Pull request type

Please check the type of change your PR introduces:

  • 🐛 Bugfix

Issues

This is the first step for #2254: I didn't address the CI part of it.

Test Plan

  • 💪 Manual

I built a multi-arch image manually on M1 Mac and verified that it works:

# Build the "build" image first
docker build build --platform Linux/amd64,Linux/arm64 -t ghcr.io/haimgel/kanisterio-build:v0.0.25 docker/build
docker push ghcr.io/haimgel/kanisterio-build:v0.0.25

# Build the separate images for ARM64 and AMD64
make build-controller BUILD_IMAGE=ghcr.io/haimgel/kanisterio-build:v0.0.25 ARCH=arm64 
make container BUILD_IMAGE=ghcr.io/haimgel/kanisterio-build:v0.0.25 REGISTRY=ghcr.io/haimgel \
    IMAGE_NAME=kanisterio-controller ARCH=arm64 VERSION=temp-arm64

make build-controller BUILD_IMAGE=ghcr.io/haimgel/kanisterio-build:v0.0.25 ARCH=amd64 
make container BUILD_IMAGE=ghcr.io/haimgel/kanisterio-build:v0.0.25 REGISTRY=ghcr.io/haimgel \ 
    IMAGE_NAME=kanisterio-controller ARCH=amd64 VERSION=temp-amd64

# Unify them into one multi-arch image
manifest-tool push from-args --platforms Linux/amd64,linux/arm64 \
	--template ghcr.io/haimgel/kanisterio-controller:temp-ARCH \
	--target ghcr.io/haimgel/kanisterio-controller:feb02e

Docker debian repo has AMD64 and ARM64 images both. Do not contstrain it, so that a multi-platform build image can be built.
1. Pass architecture from the Makefile and override the hardcoded AMD64 in the goreleaser file.
2. AMD64 artifacts have a _v1 suffix, ARM64 artifacts don't: relas the "cp" source glob to support both.
…ilt image architecture:

It's perfectly fine to use the native license extractor arch even when building a non-native architecture image.
@haimgel
Copy link
Contributor Author

haimgel commented Dec 12, 2023

I agree to the DCO for all the commits in this PR.

@hairyhum
Copy link
Contributor

Since this PR addresses Dockerfiles, should it also include things like https://github.com/kanisterio/kanister/pull/2524/files#diff-c925a4382a9e81d94aabbaa91b5e3492cadc2d4f56bc2098bff1a86ae8cf0c67R21 ?
There are few binaries like that being downloaded in Dockerfiles.

@haimgel
Copy link
Contributor Author

haimgel commented Dec 19, 2023

Since this PR addresses Dockerfiles, should it also include things like https://github.com/kanisterio/kanister/pull/2524/files#diff-c925a4382a9e81d94aabbaa91b5e3492cadc2d4f56bc2098bff1a86ae8cf0c67R21 ? There are few binaries like that being downloaded in Dockerfiles.

Good point, it should! I missed Kind because it's not used when building release images. I pushed a commit to address this. I also verified that other dependencies of this image are available for both amd64 and arm64.

I didn't address multi-arch issues in other support Dockerfiles in the repo: i'd rather focus on the controller image and its dependencies first.

@@ -18,7 +18,8 @@ COPY --from=alpine/helm:3.12.2 /usr/bin/helm /usr/local/bin/

COPY --from=golangci/golangci-lint:v1.51.2 /usr/bin/golangci-lint /usr/local/bin/

RUN wget -O /usr/local/bin/kind https://github.com/kubernetes-sigs/kind/releases/download/v0.18.0/kind-linux-amd64 \
RUN ARCH=$(uname -m | sed -e 's/aarch64/arm64/') && \
wget -O /usr/local/bin/kind https://github.com/kubernetes-sigs/kind/releases/download/v0.18.0/kind-linux-${ARCH} \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resolves into https://github.com/kubernetes-sigs/kind/releases/download/v0.18.0/kind-linux-x86_64 for amd64, so it doesn't exist.
Maybe getting it from the input argument instead of uname would be a better idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resolves into https://github.com/kubernetes-sigs/kind/releases/download/v0.18.0/kind-linux-x86_64 for amd64, so it doesn't exist. Maybe getting it from the input argument instead of uname would be a better idea.

Sorry about that!

I updated to use the TARGETPLATFORM buildx argument. Buildx was already set up in the GH workflow for the build container, and I also added the platform parameter there to build for both platforms.

@hairyhum
Copy link
Contributor

@haimgel
Copy link
Contributor Author

haimgel commented Jan 7, 2024

@hairyhum, thanks for approving my PR! What else is needed to bring it over the finish line and merge?

@hairyhum hairyhum added the kueue label Jan 8, 2024
@hairyhum hairyhum merged commit bc4a5f8 into kanisterio:master Jan 9, 2024
12 of 13 checks passed
@hairyhum
Copy link
Contributor

hairyhum commented Jan 9, 2024

@haimgel automated merge didn't work because of changes in the workflows. Merged manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants