Enable Multi-Platform Support for kubemacpool Image Builds #441

ashokpariya0 · 2024-11-25T16:32:08Z

These changes enable building and pushing kubemacpool container images for multiple platforms (amd64, s390x, arm64) from a single Dockerfile. Enhanced multi-platform support in the build process by adding a PLATFORMS argument in the Makefile for amd64, s390x, and arm64 architectures. Multi-platform build support is provided for both Docker and Podman container runtimes.

What this PR does / why we need it:
The upstream kubemacpool image (link) currently supports only the amd64 architecture. The proposed changes in this PR enable multi-platform support for the kubemacpool image, broadening its compatibility to include additional architectures.

Special notes for your reviewer:

Multiplatform Build Instruction: https://gist.github.com/ashokpariya0/33cf22401d17153b7f91b40295397ccd

Multi-platform build support is enabled for both Docker and Podman container runtimes, and the necessary changes have been made accordingly.
The PLATFORMS variable defines the target platforms for building the manager image, allowing support for multiple architectures.

To use this feature, you need to:

For Docker:

Have access to docker buildx. More information: [Docker Buildx Documentation](https://docs.docker.com/build/building/multi-platform/)
Ensure that BuildKit is enabled. More information: [Docker BuildKit Enhancements](https://docs.docker.com/develop/develop-images/build_enhancements/)
For Podman:

No additional dependencies are required to build multi-platform images. Podman supports this feature natively.

Release note:

None

kubevirt-bot · 2024-11-25T16:32:18Z

Hi @ashokpariya0. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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.

oshoval · 2024-12-01T07:29:12Z

cc @RamLavi

RamLavi

Hey @ashokpariya0 , thank you for this change.
I added some comments, please take a look

RamLavi · 2024-12-01T09:28:14Z

Makefile

+# export PLATFORMS=linux/arm64,linux/s390x,linux/amd64
+ARCH := $(shell uname -m | sed 's/x86_64/amd64/')
+DOCKER_BUILDER ?= kubemacpool-docker-builder
+KUBEMACPOOL_IMAGE_TAGGED_1 := ${REGISTRY}/${IMG}:${IMAGE_TAG}


please change KUBEMACPOOL_IMAGE_TAGGED_1 --> KUBEMACPOOL_IMAGE_TAGGED
and KUBEMACPOOL_IMAGE_TAGGED_2 --> KUBEMACPOOL_IMAGE_GIT_TAGGED
the current naming is not good, but it is still better than meaningless enumeration.

Please change it in the appropriate commit. There should be no existence of KUBEMACPOOL_IMAGE_TAGGED_1 during any commit

RamLavi · 2024-12-01T09:29:20Z

Makefile

-	$(OCI_BIN) push ${TLS_SETTING} ${REGISTRY}/${IMG}:${IMAGE_TAG}
-	$(OCI_BIN) tag ${REGISTRY}/${IMG}:${IMAGE_TAG} ${REGISTRY}/${IMG}:${IMAGE_GIT_TAG}
-	$(OCI_BIN) push ${TLS_SETTING} ${REGISTRY}/${IMG}:${IMAGE_GIT_TAG}
+ifeq ($(OCI_BIN),podman)


why is there no docker counterpart?
In any case, if we don't want to support pushing with docker, we should be explicit, failing the operation.

I'm not sure I understand this. In the existing code (https://github.com/k8snetworkplumbingwg/kubemacpool/blob/main/Makefile#L94-L98), I don't see any check to prevent pushing the image in the Docker case either. Just for clarification—docker buildx build combines both build and push into a single command, so there isn't a separate push command. This is the way it works.

Let me rephrase: in the existing code (https://github.com/k8snetworkplumbingwg/kubemacpool/blob/main/Makefile#L94-L98) you can either do docker/podman push/tag using the OCI_BIN env var.
Now however, you can only do it with podman:

ifeq ($(OCI_BIN),podman) push,tag,etc... endif ...

my original question is - why are you limiting it like this?

For building multi-platform images using docker buildx build, the build and push operations are combined into a single command, Therefore, the separate push and tag logic, which currently applies only to podman, isn't needed for docker when using buildx.

theoretically it is possible to split even for docker single arch / multi arch ?
(i see here --push flag is used so i wonder)
https://github.com/k8snetworkplumbingwg/kubemacpool/pull/441/files#diff-8a6127c87c24bc27e5595386a152b920da0266e3b6d185a3e632cab440abf09bR10-R18

not saying we need to, first trying to understand

doing so gives advantage of more flexibility and symmetry

Currently it's not possible for multi arch images:
Below is the reason:

With docker buildx, the build result is stored only in the build cache by default. There are two options to handle the result, which I've already tested: --push and --load.

The --push flag pushes the built image to a container registry.
The --load flag loads the resulting image into the local Docker daemon. However, the --load option currently only supports single-platform images or simpler images. It does not work with multi-platform builds (i.e., manifest lists).
When using the --load flag with a multi-platform build, you'll encounter the following error:

docker exporter does not currently support exporting manifest lists

thanks for the explanation
theoretically for single arch, it is possible then right?
it is nice to have, as it will allow to just build instead build + push, but i am not sure it worths the effort, and all the other repos have it combined already
moreover we encourage podman over docker

it is Ram's decision here anyhow

Correct, but with single arch we can directly use docker build , for multiplatform we need to use docker buildx build and we have to do like this

RamLavi · 2024-12-01T09:33:49Z

build/Dockerfile

@@ -1,4 +1,21 @@
+ARG BUILD_ARCH=amd64


please separate into 2 commits:

moving repo go build to container (+1 btw)

Adding multi arch thing..

You did so but the order of the commits is wrong.
Please separate to 2 commits. The first commit only moves the repo go build to a container, setting the ground to the main commit. The second does the main thing this PR is meant to do which is adding the multi arch

RamLavi · 2024-12-01T09:39:29Z

hack/build-kubemacpool-podman.sh

@@ -0,0 +1,25 @@
+#!/bin/bash
+
+if [ -z "$ARCH" ] || [ -z "$PLATFORMS" ] || [ -z "$KUBEMACPOOL_IMAGE_TAGGED_1" ]; then


I'm missing KUBEMACPOOL_IMAGE_TAGGED_2 in the podman script. Is this intentional?
I would want both tag to be there. having both latest and sha-specific tags allow us to be flexible picking the desired image when deployed from other operators like CNAO.

Yes, both tags are properly handled.
For multi-architecture support, we create a single manifest and tag it with both the git_sha_tag and the latest tag before pushing. This process, managed in the Makefile, ensures that both tags are properly handled.

RamLavi · 2024-12-01T09:48:12Z

Makefile

-container: manager
-	$(OCI_BIN) build build/ -t ${REGISTRY}/${IMG}:${IMAGE_TAG}
+container:
+ifeq ($(OCI_BIN),podman)


I prefer not over-complicating the Makefile with these conditions.
please call to a build-multiarch-kubemacpool script that deals with both podman and docker cases.

RamLavi · 2024-12-01T09:52:46Z

hack/build-kubemacpool-podman.sh

+        --no-cache \
+        --build-arg BUILD_ARCH="$ARCH" \
+        --platform "$platform" \
+        --manifest "${KUBEMACPOOL_IMAGE_TAGGED_1}" \


again what happened to KUBEMACPOOL_IMAGE_TAGGED_2 tagging?

As mentioned earlier, this script creates only a single Podman manifest(this how multi-platform build works with podman), while the tagging with git_sha is handled in the Makefile. so both tags are properly handled.

oshoval · 2024-12-01T09:58:51Z

Thanks for the review Ram

Please note that imo we should not merge any new PRs about cross compilation until
kubevirt/cluster-network-addons-operator#1923 (comment) issue is solved

We must make sure we have local machines that can run it (for me personally since the new machine isn't accessible right now, the old machine will take time to upgrade)
Alternative, we need to keep old docker support for now - EDIT machines back to business
CNAO merged PR might have some problems (some errors that are mentioned, need to check if they are important)
and also not sure how all the lanes run - EDIT - Vanilla main is fine on all lanes, and the error is just an error we ignore (rm when image not found) iiuc.

Even if this effort is important, we should not rush it until it is done correctly please.

ashokpariya0 · 2024-12-01T12:16:14Z

kubevirt/cluster-network-addons-operator#1923 (comment)
Hey @ashokpariya0 , thank you for this change. I added some comments, please take a look

Okay, noted! I'll make the necessary updates.

kubevirt-bot · 2024-12-02T07:12:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ramlavi. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

oshoval

some comments (fast pass)
thanks

oshoval · 2024-12-02T07:16:00Z

Makefile

@@ -6,6 +6,16 @@ IMAGE_GIT_TAG ?= $(shell git describe --abbrev=8 --tags)
 IMG ?= $(REPO)/kubemacpool
 OCI_BIN ?= $(shell if podman ps >/dev/null 2>&1; then echo podman; elif docker ps >/dev/null 2>&1; then echo docker; fi)
 TLS_SETTING := $(if $(filter $(OCI_BIN),podman),--tls-verify=false,)
+PLATFORMS ?= linux/amd64,linux/s390x


imo need to check benchmark before doing both
or best to just use the auto detect and have optional all flag as raised on CNAO please

Done, Added auto detach and all flag option in latest commit.

oshoval · 2024-12-02T07:16:35Z

build/Dockerfile

+FROM --platform=linux/${BUILD_ARCH} quay.io/centos/centos:stream9 AS builder
+RUN dnf install -y tar gzip jq && dnf clean all
+RUN ARCH=$(uname -m | sed 's/x86_64/amd64/') && \
+    GO_VERSION=$(curl -L -s "https://go.dev/dl/?mode=json" | jq -r '.[0].version') && \


better to get go version from go.mod please
easier to maintain

oshoval · 2024-12-02T07:16:50Z

hack/build-kubemacpool-docker.sh

+
+IFS=',' read -r -a PLATFORM_LIST <<< "$PLATFORMS"
+
+BUILD_ARGS="--no-cache --build-arg BUILD_ARCH=$ARCH -f build/Dockerfile -t $KUBEMACPOOL_IMAGE_TAGGED -t $KUBEMACPOOL_IMAGE_GIT_TAGGED . --push"


why no cache?

oshoval · 2024-12-02T07:17:14Z

hack/build-kubemacpool-podman.sh

+
+for platform in "${PLATFORM_LIST[@]}"; do
+    podman build \
+        --no-cache \


why no cache?

It seems --no-cache is recommend approach. Without --no-cache, the build is faster because it reuses previous layers. But this can sometimes cause outdated or incorrect dependencies to be used. Using --no-cache forces a fresh build, making sure all steps in the Dockerfile run again without using old layers. Since we already remove the old manifest before creating a new one, I think --no-cache is the best choice. However, I'm open to other suggestions.

imho we can omit it please and use caching, docker caching seems too fundamental to have bugs beside corner cases
we use caching so far and it is fine iiuc
speed is important (of course unless we find in the future it is buggy and then can report / workaround that)

oshoval · 2024-12-02T07:17:39Z

hack/init-buildx.sh

+  if ! docker buildx > /dev/null 2>&1; then
+     mkdir -p ~/.docker/cli-plugins
+     BUILDX_VERSION=$(curl -s https://api.github.com/repos/docker/buildx/releases/latest | jq -r .tag_name)
+     curl -L https://github.com/docker/buildx/releases/download/"${BUILDX_VERSION}"/buildx-"${BUILDX_VERSION}".linux-amd64 --output ~/.docker/cli-plugins/docker-buildx


is it fine it has hardcoded linux-amd64 ?

good catch, fixed it.

thanks, saw this on another PR of this effort (commented there)

ashokpariya0 · 2024-12-02T14:23:57Z

@RamLavi @oshoval I have addressed all the review comments and responded to the questions. Please take a look.

oshoval · 2024-12-02T14:27:29Z

build/Dockerfile

+    rm go${GO_VERSION}.${BUILDOS}-${BUILDARCH}.tar.gz
+
+ENV PATH=$PATH:/usr/local/go/bin
+COPY . .


please consider adding a RUN go mod download before the copy, to improve layer building

ref k8snetworkplumbingwg/ovs-cni#336

oshoval · 2024-12-02T14:37:53Z

build/Dockerfile

+ENV PATH=$PATH:/usr/local/go/bin
+COPY . .
+
+RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -o build/_output/bin/manager github.com/k8snetworkplumbingwg/kubemacpool/cmd/manager


is it just me or it builds remote sources which is not correct ?
i would expect something like
go build -o build/_output/bin/manager ./cmd/manager

(unless it compiles based on module name, not remote)

No, this is correct only,
Please check

kubemacpool/Makefile

Line 87 in f61b413

CGO_ENABLED=0 GOOS=linux GOARCH=amd64 $(GO) build -o $(BIN_DIR)/manager github.com/k8snetworkplumbingwg/kubemacpool/cmd/manager

thanks,
imho it is better to be explicit with what we build please
./cmd/manager
EDIT - but no need in this PR if this was the case already

oshoval · 2024-12-02T14:42:16Z

hack/build-kubemacpool-docker.sh

+else
+    ./hack/init-buildx.sh "$DOCKER_BUILDER"
+    docker buildx build --platform "$PLATFORMS" $BUILD_ARGS
+    docker buildx rm "$DOCKER_BUILDER" || true


worth to add please redir of stderr to /dev/null, else the error would always be marked on CI spyglass (same please for all other repos where this effort was done)

(as done on hack/build-kubemacpool-podman.sh)

ashokpariya0 · 2024-12-03T09:09:26Z

@RamLavi @oshoval All the review comments have been addressed. Please take a look.

ashokpariya0 · 2024-12-06T04:48:45Z

@RamLavi @oshoval I have addressed all the review comments. Please have a look.
cc @phoracek @ormergi

RamLavi

please reorder the PR so that it is easier to review

RamLavi · 2024-12-10T10:22:05Z

Makefile

-	$(OCI_BIN) push ${TLS_SETTING} ${REGISTRY}/${IMG}:${IMAGE_TAG}
-	$(OCI_BIN) tag ${REGISTRY}/${IMG}:${IMAGE_TAG} ${REGISTRY}/${IMG}:${IMAGE_GIT_TAG}
-	$(OCI_BIN) push ${TLS_SETTING} ${REGISTRY}/${IMG}:${IMAGE_GIT_TAG}
+ifeq ($(OCI_BIN),podman)


Let me rephrase: in the existing code (https://github.com/k8snetworkplumbingwg/kubemacpool/blob/main/Makefile#L94-L98) you can either do docker/podman push/tag using the OCI_BIN env var.
Now however, you can only do it with podman:

ifeq ($(OCI_BIN),podman) push,tag,etc... endif ...

my original question is - why are you limiting it like this?

RamLavi · 2024-12-10T10:24:20Z

build/Dockerfile

@@ -1,4 +1,21 @@
+ARG BUILD_ARCH=amd64


You did so but the order of the commits is wrong.
Please separate to 2 commits. The first commit only moves the repo go build to a container, setting the ground to the main commit. The second does the main thing this PR is meant to do which is adding the multi arch

RamLavi · 2024-12-10T10:28:42Z

Makefile

+# export PLATFORMS=linux/arm64,linux/s390x,linux/amd64
+ARCH := $(shell uname -m | sed 's/x86_64/amd64/')
+DOCKER_BUILDER ?= kubemacpool-docker-builder
+KUBEMACPOOL_IMAGE_TAGGED_1 := ${REGISTRY}/${IMG}:${IMAGE_TAG}


Please change it in the appropriate commit. There should be no existence of KUBEMACPOOL_IMAGE_TAGGED_1 during any commit

…plumbingwg#440) The origin-kube-rbac-proxy image is not compatible with the s390x architecture. This change deploys an image that is multi arch supported. Signed-off-by: Ashok Pariya <[email protected]>

These changes provide multi-platform build support for the Kubemacpool CNI, enabling builds for Docker and Podman container runtimes. Signed-off-by: Ashok Pariya [email protected]

kubevirt-bot added release-note-none dco-signoff: yes labels Nov 25, 2024

kubevirt-bot added the size/L label Nov 25, 2024

kubevirt-bot requested review from ormergi and phoracek November 25, 2024 16:32

ashokpariya0 mentioned this pull request Nov 25, 2024

Enable multi-architecture support, specifically for s390x, in CNAO and the associated CNIs. kubevirt/cluster-network-addons-operator#1916

Open

RamLavi requested changes Dec 1, 2024

View reviewed changes

kubevirt-bot assigned RamLavi Dec 1, 2024

ashokpariya0 force-pushed the enable-multi-platform-kubemacpool branch from 1d5d286 to 33e4a6b Compare December 2, 2024 07:12

oshoval reviewed Dec 2, 2024

View reviewed changes

ashokpariya0 force-pushed the enable-multi-platform-kubemacpool branch 2 times, most recently from 621f919 to 44f660a Compare December 2, 2024 14:15

oshoval reviewed Dec 2, 2024

View reviewed changes

ashokpariya0 force-pushed the enable-multi-platform-kubemacpool branch from 44f660a to 18f484e Compare December 2, 2024 16:38

RamLavi requested changes Dec 10, 2024

View reviewed changes

ashokpariya0 force-pushed the enable-multi-platform-kubemacpool branch from 18f484e to fa19101 Compare December 11, 2024 10:13

ashokpariya0 added 2 commits December 11, 2024 11:01

feat: use multi arch supported image for kube-rbac-proxy. (k8snetwork…

7ee47db

…plumbingwg#440) The origin-kube-rbac-proxy image is not compatible with the s390x architecture. This change deploys an image that is multi arch supported. Signed-off-by: Ashok Pariya <[email protected]>

Add Multi-Platform support for Kubemacpool Image using make command.

bc8283b

These changes provide multi-platform build support for the Kubemacpool CNI, enabling builds for Docker and Podman container runtimes. Signed-off-by: Ashok Pariya [email protected]

ashokpariya0 force-pushed the enable-multi-platform-kubemacpool branch from fa19101 to bc8283b Compare December 11, 2024 11:05

		@@ -0,0 +1,25 @@
		#!/bin/bash

		if [ -z "$ARCH" ] \|\| [ -z "$PLATFORMS" ] \|\| [ -z "$KUBEMACPOOL_IMAGE_TAGGED_1" ]; then


		IFS=',' read -r -a PLATFORM_LIST <<< "$PLATFORMS"

		BUILD_ARGS="--no-cache --build-arg BUILD_ARCH=$ARCH -f build/Dockerfile -t $KUBEMACPOOL_IMAGE_TAGGED -t $KUBEMACPOOL_IMAGE_GIT_TAGGED . --push"

Enable Multi-Platform Support for kubemacpool Image Builds #441

Are you sure you want to change the base?

Enable Multi-Platform Support for kubemacpool Image Builds #441

Conversation

ashokpariya0 commented Nov 25, 2024 • edited Loading

kubevirt-bot commented Nov 25, 2024

oshoval commented Dec 1, 2024

RamLavi left a 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

oshoval Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 10, 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

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

oshoval commented Dec 1, 2024 • edited Loading

ashokpariya0 commented Dec 1, 2024

kubevirt-bot commented Dec 2, 2024

oshoval left a 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

Choose a reason for hiding this comment

oshoval Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

ashokpariya0 commented Dec 2, 2024

oshoval Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

oshoval Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokpariya0 commented Dec 3, 2024

ashokpariya0 commented Dec 6, 2024

RamLavi left a 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

ashokpariya0 commented Nov 25, 2024 •

edited

Loading

oshoval Dec 10, 2024 •

edited

Loading

oshoval Dec 10, 2024 •

edited

Loading

oshoval commented Dec 1, 2024 •

edited

Loading

oshoval Dec 2, 2024 •

edited

Loading

oshoval Dec 2, 2024 •

edited

Loading

oshoval Dec 2, 2024 •

edited

Loading

oshoval Dec 3, 2024 •

edited

Loading

oshoval Dec 2, 2024 •

edited

Loading