Enable multi-platform support for bridge marker image builds #77

ashokpariya0 · 2024-11-25T12:10:19Z

These changes enable building and pushing bridge marker 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 bridge-marker image ([link](https://quay.io/repository/kubevirt/bridge-marker?tab=tags&tag=latest)) currently supports only the amd64 architecture. The proposed changes in this PR enable multi-platform support for the bridge-marker image, broadening its compatibility to include additional architectures.

Special notes for your reviewer:
Multiplatform build instruction: https://gist.github.com/ashokpariya0/30d5efc0e9525c266ef9e65fe4291860

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-25T12:10:30Z

Hi @ashokpariya0. 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.

oshoval

Thanks looks great

oshoval · 2024-12-01T08:41:23Z

Makefile

@@ -1,6 +1,16 @@
 REGISTRY ?= quay.io/kubevirt
 IMAGE_TAG ?= latest
 IMAGE_GIT_TAG ?= $(shell git describe --abbrev=8 --tags)
+PLATFORMS ?= linux/amd64,linux/s390x


lets see please we are good to with having s390x by default, or better do the auto detection trick as done on cnao please
it is important to have fast deployment generally speaking imho

oshoval · 2024-12-01T08:42:40Z

Makefile

+MARKER_IMAGE_TAGGED_1 := ${REGISTRY}/bridge-marker:${IMAGE_TAG}
+MARKER_IMAGE_TAGGED_2 := ${REGISTRY}/bridge-marker:${IMAGE_GIT_TAG}


please renamed to MARKER_IMAGE_TAGGED
and MARKER_IMAGE_GIT_TAGGED

oshoval · 2024-12-01T08:43:31Z

Makefile

+build-multiarch-marker-podman:
+	ARCH=$(ARCH) PLATFORMS=$(PLATFORMS) MARKER_IMAGE_TAGGED_1=$(MARKER_IMAGE_TAGGED_1) MARKER_IMAGE_TAGGED_2=$(MARKER_IMAGE_TAGGED_2) ./hack/build-marker-podman.sh
+
+.PHONY: build build-multiarch-marker-docker build-multiarch-marker-podman format docker-build docker-push manifests cluster-up cluster-down cluster-sync vendor marker tools


please use one per line

oshoval · 2024-12-01T08:44:44Z

build/Dockerfile

+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') && \
+    curl -L "https://go.dev/dl/${GO_VERSION}.linux-${ARCH}.tar.gz" -o go.tar.gz && \


lets take go version from go.mod please, as done on other network repos
easier to maintain

Addressed in latest commit.

oshoval · 2024-12-01T08:46:22Z

build/Dockerfile

+    rm go.tar.gz
+ENV PATH=$PATH:/usr/local/go/bin
+WORKDIR /go/src/bridge-marker
+COPY . .


please see k8snetworkplumbingwg/ovs-cni#336
nice to copy the go.mod / go.sum, and then have a layer of go mod download
and just at the end the copy . . in order to have docker layers that enable caching better

oshoval · 2024-12-01T08:51:47Z

hack/build-marker-docker.sh

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


why --no-cache ?
we do need it at least locally usually

Removed in latest commit.

oshoval · 2024-12-01T08:52:17Z

hack/build-marker-podman.sh

+
+IFS=',' read -r -a PLATFORM_LIST <<< "$PLATFORMS"
+
+# Remove any existing manifests and images


please drop comments all over, unless non trivial

oshoval · 2024-12-01T08:52:40Z

hack/build-marker-podman.sh

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


why no cache ?
speed is important please

oshoval · 2024-12-01T08:53:18Z

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


linux-amd64 hardcoded, is it fine? or we want multi support
what if compiled directly on non amd64, (not cross) ?

oshoval · 2024-12-01T08:55:32Z

hack/install-go.sh

@@ -2,7 +2,8 @@

 destination=$1
 version=$(grep "^go " go.mod |awk '{print $2}')
-tarball=go$version.linux-amd64.tar.gz
+arch=$(uname -m | sed 's/x86_64/amd64/')


lets add auto detection please ?
like kubevirt/cluster-network-addons-operator#1928

Added auto detection as default mode.

shouldn't it be ? (unless for go binary the naming are different)
uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/'

Yes added for arm64.

ashokpariya0 · 2024-12-03T07:36:05Z

@oshoval All review comments have been addressed, Please take look.

These changes enable building and pushing bridge marker 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. Signed-off-by: Ashok Pariya <[email protected]>

oshoval

Looks great, thank you

oshoval · 2024-12-03T08:55:31Z

@RamLavi @qinqon
please take a look

oshoval · 2024-12-03T09:40:44Z

hack/install-go.sh

@@ -2,7 +2,8 @@

 destination=$1
 version=$(grep "^go " go.mod |awk '{print $2}')
-tarball=go$version.linux-amd64.tar.gz
+arch=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/')


kubevirt/kubesecondarydns@d03ce8e

here the logic is bit different (for s390x), are both fine ? or one should be fixed
thanks

kubevirt/kubesecondarydns@d03ce8e

Both are same for s390x case.
Both approaches are fine. However, one thing to note is that linux/ppc64le will fail with [this commit](kubevirt/kubesecondarydns@d03ce8e), as it explicitly supports only amd64, arm64, and s390x.

thank you
i think we dont support ppc64le atm, please raise it there as well if you want (for visibility)

thank you i think we dont support ppc64le atm, please raise it there as well if you want (for visibility)

okay, I can see for ipam controller (https://github.com/kubevirt/ipam-extensions/blob/925bee6139751fcb241a435bae8b79ce1dfce595/Makefile#L121C50-L121C63)

yes but i dont think we need to support ppc64le generally speaking on other sig network repos
@phoracek please keep me honest here, i think we discussed it once

EDIT - anyhow, imho, even if we need it, it should be on a follow-up all over,
not to be mixed with the current effort which lays the foundations and support for new archs already

@oshoval Just to inform you, I noticed that linux/ppc64le is supported for components like the bridge-operator, OVS, Multus CNI, etc.

oshoval · 2024-12-03T10:48:40Z

build/Dockerfile

+ENV PATH=$PATH:/usr/local/go/bin
+RUN go mod download
+COPY . .
+RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -o build/_output/bin/marker github.com/kubevirt/bridge-marker/cmd/marker


imho it is better to be explicit with what we build please
./cmd/marker

EDIT - as we spoke, since it was like that, lets keep it

oshoval · 2024-12-10T08:58:28Z

Thanks
/lgtm

@phoracek
can you please take a look ?

phoracek · 2024-12-10T09:08:32Z

/retest

phoracek

/approve

kubevirt-bot · 2024-12-10T13:31:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phoracek

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~~ [phoracek]

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

oshoval · 2024-12-10T13:41:31Z

Thanks
need to find what creates release and update it as well / issue a release right ?
(unless part of it was already done)

ashokpariya0 · 2024-12-10T13:46:48Z

Thanks need to find what creates release and update it as well / issue a release right ? (unless part of it was already done)

Yes we need that info as we have to do export PLATFORMS=all before we build and push images:
More details is available on https://gist.github.com/ashokpariya0/30d5efc0e9525c266ef9e65fe4291860

oshoval · 2024-12-10T13:54:35Z

Hi @phoracek
is it possible that it doesn't have automation and you release this one manually ?

oshoval · 2024-12-10T15:12:45Z

Please see here the post submit
https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/logs/main-bridge-marker/1866476397598347264
https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/files/jobs/kubevirt/bridge-marker/bridge-marker-postsubmits.yaml

Thanks Petr

Once you add the "all", we need to tag and then it will release with tag

ashokpariya0 · 2024-12-10T15:27:52Z

Please see here the post submit https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/logs/main-bridge-marker/1866476397598347264 https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/files/jobs/kubevirt/bridge-marker/bridge-marker-postsubmits.yaml

Thanks Petr

Once you add the "all", we need to tag and then it will release with tag

Great! Can I create a PR to add export PLATFORMS=all in the above YAML file under the project-infra repository?

oshoval · 2024-12-10T15:31:31Z

sure thanks

ashokpariya0 · 2024-12-11T05:47:19Z

Please see here the post submit https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/logs/main-bridge-marker/1866476397598347264 https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/files/jobs/kubevirt/bridge-marker/bridge-marker-postsubmits.yaml

Thanks Petr

Once you add the "all", we need to tag and then it will release with tag

PR link: kubevirt/project-infra#3819

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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 25, 2024

kubevirt-bot requested review from phoracek and RamLavi November 25, 2024 12:10

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

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

oshoval reviewed Dec 1, 2024

View reviewed changes

ashokpariya0 force-pushed the enable-multi-platform-bridge-marker branch from b59b33f to d1c0939 Compare December 3, 2024 07:30

ashokpariya0 force-pushed the enable-multi-platform-bridge-marker branch from d1c0939 to 022e5e2 Compare December 3, 2024 07:49

oshoval approved these changes Dec 3, 2024

View reviewed changes

kubevirt-bot assigned oshoval Dec 3, 2024

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

oshoval reviewed Dec 3, 2024

View reviewed changes

oshoval mentioned this pull request Dec 3, 2024

ci: Enable building for s390x and arm64 kubevirt/kubesecondarydns#73

Merged

oshoval reviewed Dec 3, 2024

View reviewed changes

phoracek reviewed Dec 10, 2024

View reviewed changes

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

kubevirt-bot merged commit 892ccdd into kubevirt:main Dec 10, 2024
4 checks passed

		MARKER_IMAGE_TAGGED_1 := ${REGISTRY}/bridge-marker:${IMAGE_TAG}
		MARKER_IMAGE_TAGGED_2 := ${REGISTRY}/bridge-marker:${IMAGE_GIT_TAG}


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

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


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

		# Remove any existing manifests and images

Enable multi-platform support for bridge marker image builds #77

Enable multi-platform support for bridge marker image builds #77

Conversation

ashokpariya0 commented Nov 25, 2024 • edited Loading

kubevirt-bot commented Nov 25, 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

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 1, 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

ashokpariya0 commented Dec 3, 2024

oshoval left a comment

Choose a reason for hiding this comment

oshoval commented Dec 3, 2024

oshoval Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

ashokpariya0 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

oshoval Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

oshoval commented Dec 10, 2024

phoracek commented Dec 10, 2024

phoracek left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Dec 10, 2024

oshoval commented Dec 10, 2024 • edited Loading

ashokpariya0 commented Dec 10, 2024

oshoval commented Dec 10, 2024 • edited Loading

oshoval commented Dec 10, 2024 • edited Loading

ashokpariya0 commented Dec 10, 2024

oshoval commented Dec 10, 2024

ashokpariya0 commented Dec 11, 2024

ashokpariya0 commented Nov 25, 2024 •

edited

Loading

oshoval Dec 1, 2024 •

edited

Loading

oshoval Dec 3, 2024 •

edited

Loading

ashokpariya0 Dec 3, 2024 •

edited

Loading

oshoval Dec 3, 2024 •

edited

Loading

oshoval Dec 3, 2024 •

edited

Loading

oshoval Dec 3, 2024 •

edited

Loading

oshoval commented Dec 10, 2024 •

edited

Loading

oshoval commented Dec 10, 2024 •

edited

Loading

oshoval commented Dec 10, 2024 •

edited

Loading