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

Enable Multi-Platform Support for kubemacpool Image Builds #441

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,19 @@ 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,)
PLATFORM_LIST ?= linux/amd64,linux/s390x,linux/arm64
ARCH := $(shell uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/')
PLATFORMS ?= linux/${ARCH}
PLATFORMS := $(if $(filter all,$(PLATFORMS)),$(PLATFORM_LIST),$(PLATFORMS))
# Define the platforms for building a multi-platform image.
# Example:
# PLATFORMS ?= linux/amd64,linux/arm64,linux/s390x
# Alternatively, you can set the PLATFORMS variable using:
# export PLATFORMS=linux/arm64,linux/s390x,linux/amd64
# or export PLATFORMS=all to automatically include all supported platforms.
DOCKER_BUILDER ?= kubemacpool-docker-builder
KUBEMACPOOL_IMAGE_TAGGED := ${REGISTRY}/${IMG}:${IMAGE_TAG}
KUBEMACPOOL_IMAGE_GIT_TAGGED := ${REGISTRY}/${IMG}:${IMAGE_GIT_TAG}

BIN_DIR = $(CURDIR)/build/_output/bin/

Expand Down Expand Up @@ -83,18 +96,17 @@ generate: generate-go generate-deploy generate-test generate-external
check: $(GO)
./hack/check.sh

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

# Build the docker image
container: manager
$(OCI_BIN) build build/ -t ${REGISTRY}/${IMG}:${IMAGE_TAG}
container:
./hack/build-multiarch-kubemacpool.sh $(ARCH) $(PLATFORMS) $(KUBEMACPOOL_IMAGE_TAGGED) $(KUBEMACPOOL_IMAGE_GIT_TAGGED) $(DOCKER_BUILDER) $(OCI_BIN)

# Push the docker image
docker-push:
$(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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oshoval oshoval Dec 10, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@oshoval oshoval Dec 10, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I see now. I don't like the asymmetry of it, but having build+push to the local repo isn't such a bad thing.
Thanks for the explanation

podman manifest push ${TLS_SETTING} ${KUBEMACPOOL_IMAGE_TAGGED} ${KUBEMACPOOL_IMAGE_TAGGED}
podman tag ${KUBEMACPOOL_IMAGE_TAGGED} ${KUBEMACPOOL_IMAGE_GIT_TAGGED}
podman manifest push ${TLS_SETTING} ${KUBEMACPOOL_IMAGE_GIT_TAGGED} ${KUBEMACPOOL_IMAGE_GIT_TAGGED}
endif

cluster-up:
./cluster/up.sh
Expand Down Expand Up @@ -128,7 +140,6 @@ vendor: $(GO)
fmt \
vet \
check \
manager \
container \
push \
cluster-up \
Expand Down
31 changes: 29 additions & 2 deletions build/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,31 @@
ARG BUILD_ARCH=amd64
Copy link
Member

Choose a reason for hiding this comment

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

please separate into 2 commits:

  1. moving repo go build to container (+1 btw)
  2. Adding multi arch thing..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

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

FROM --platform=linux/${BUILD_ARCH} quay.io/centos/centos:stream9 AS builder
ARG TARGETOS
ARG TARGETARCH
ENV TARGETOS=${TARGETOS:-linux}
ENV TARGETARCH=${TARGETARCH:-amd64}

ARG BUILDOS
ARG BUILDARCH
ENV BUILDOS=${BUILDOS:-linux}
ENV BUILDARCH=${BUILDARCH:-amd64}

WORKDIR /go/src/kubemacpool
RUN dnf install -y wget && dnf clean all
COPY go.mod .
COPY go.sum .
RUN GO_VERSION=$(sed -En 's/^go +(.*)$/\1/p' go.mod) && \
wget https://dl.google.com/go/go${GO_VERSION}.${BUILDOS}-${BUILDARCH}.tar.gz && \
tar -C /usr/local -xzf go${GO_VERSION}.${BUILDOS}-${BUILDARCH}.tar.gz && \
rm go${GO_VERSION}.${BUILDOS}-${BUILDARCH}.tar.gz

ENV PATH=$PATH:/usr/local/go/bin
RUN go mod download
COPY . .
Copy link
Member

@oshoval oshoval Dec 2, 2024

Choose a reason for hiding this comment

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

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

ref k8snetworkplumbingwg/ovs-cni#336

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -o build/_output/bin/manager github.com/k8snetworkplumbingwg/kubemacpool/cmd/manager
Copy link
Member

@oshoval oshoval Dec 2, 2024

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is correct only,
Please check

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

Copy link
Member

@oshoval oshoval Dec 3, 2024

Choose a reason for hiding this comment

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

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


# Copy the controller-manager into a thin image
FROM registry.access.redhat.com/ubi9/ubi-minimal
COPY _output/bin/manager /
FROM --platform=linux/${TARGETARCH} registry.access.redhat.com/ubi9/ubi-minimal
COPY --from=builder /go/src/kubemacpool/build/_output/bin/manager /
ENTRYPOINT ["/manager"]
2 changes: 1 addition & 1 deletion config/default/manager/manager.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ spec:
capabilities:
drop:
- ALL
image: quay.io/openshift/origin-kube-rbac-proxy:4.15.0
image: quay.io/brancz/kube-rbac-proxy:v0.18.1
imagePullPolicy: IfNotPresent
name: kube-rbac-proxy
ports:
Expand Down
2 changes: 1 addition & 1 deletion config/release/kubemacpool.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ spec:
- --logtostderr
- --secure-listen-address=:8443
- --upstream=http://127.0.0.1:8080
image: quay.io/openshift/origin-kube-rbac-proxy:4.15.0
image: quay.io/brancz/kube-rbac-proxy:v0.18.1
imagePullPolicy: IfNotPresent
name: kube-rbac-proxy
ports:
Expand Down
2 changes: 1 addition & 1 deletion config/test/kubemacpool.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ spec:
- --logtostderr
- --secure-listen-address=:8443
- --upstream=http://127.0.0.1:8080
image: quay.io/openshift/origin-kube-rbac-proxy:4.15.0
image: quay.io/brancz/kube-rbac-proxy:v0.18.1
imagePullPolicy: IfNotPresent
name: kube-rbac-proxy
ports:
Expand Down
18 changes: 18 additions & 0 deletions hack/build-kubemacpool-docker.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#!/bin/bash

if [ -z "$ARCH" ] || [ -z "$PLATFORMS" ] || [ -z "$KUBEMACPOOL_IMAGE_TAGGED" ] || [ -z "$KUBEMACPOOL_IMAGE_GIT_TAGGED" ]; then
echo "Error: ARCH, PLATFORMS, KUBEMACPOOL_IMAGE_TAGGED, and KUBEMACPOOL_IMAGE_GIT_TAGGED must be set."
exit 1
fi

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

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

if [ ${#PLATFORM_LIST[@]} -eq 1 ]; then
docker build --platform "$PLATFORMS" $BUILD_ARGS
else
./hack/init-buildx.sh "$DOCKER_BUILDER"
docker buildx build --platform "$PLATFORMS" $BUILD_ARGS
docker buildx rm "$DOCKER_BUILDER" 2>/dev/null || echo "Builder ${DOCKER_BUILDER} not found or already removed, skipping."
fi
24 changes: 24 additions & 0 deletions hack/build-kubemacpool-podman.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#!/bin/bash

if [ -z "$ARCH" ] || [ -z "$PLATFORMS" ] || [ -z "$KUBEMACPOOL_IMAGE_TAGGED" ] || [ -z "$KUBEMACPOOL_IMAGE_GIT_TAGGED" ]; then
echo "Error: ARCH, PLATFORMS, KUBEMACPOOL_IMAGE_TAGGED, and KUBEMACPOOL_IMAGE_GIT_TAGGED must be set."
exit 1
fi

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

# Remove any existing manifest and image
podman manifest rm "${KUBEMACPOOL_IMAGE_TAGGED}" 2>/dev/null || true
podman manifest rm "${KUBEMACPOOL_IMAGE_GIT_TAGGED}" 2>/dev/null || true
podman rmi "${KUBEMACPOOL_IMAGE_TAGGED}" 2>/dev/null || true
podman rmi "${KUBEMACPOOL_IMAGE_GIT_TAGGED}" 2>/dev/null || true

podman manifest create "${KUBEMACPOOL_IMAGE_TAGGED}"

for platform in "${PLATFORM_LIST[@]}"; do
podman build \
--build-arg BUILD_ARCH="$ARCH" \
--platform "$platform" \
--manifest "${KUBEMACPOOL_IMAGE_TAGGED}" \
-f build/Dockerfile .
done
18 changes: 18 additions & 0 deletions hack/build-multiarch-kubemacpool.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#!/bin/bash

ARCH=$1
PLATFORMS=$2
KUBEMACPOOL_IMAGE_TAGGED=$3
KUBEMACPOOL_IMAGE_GIT_TAGGED=$4
DOCKER_BUILDER=$5
OCI_BIN=$6

if [ "$OCI_BIN" == "docker" ]; then
ARCH=$ARCH PLATFORMS=$PLATFORMS KUBEMACPOOL_IMAGE_TAGGED=$KUBEMACPOOL_IMAGE_TAGGED KUBEMACPOOL_IMAGE_GIT_TAGGED=$KUBEMACPOOL_IMAGE_GIT_TAGGED DOCKER_BUILDER=$DOCKER_BUILDER ./hack/build-kubemacpool-docker.sh
elif [ "$OCI_BIN" == "podman" ]; then
ARCH=$ARCH PLATFORMS=$PLATFORMS KUBEMACPOOL_IMAGE_TAGGED=$KUBEMACPOOL_IMAGE_TAGGED KUBEMACPOOL_IMAGE_GIT_TAGGED=$KUBEMACPOOL_IMAGE_GIT_TAGGED ./hack/build-kubemacpool-podman.sh
else
echo "Unsupported OCI_BIN value: $OCI_BIN"
exit 1
fi

56 changes: 56 additions & 0 deletions hack/init-buildx.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#!/bin/bash

check_buildx() {
export DOCKER_CLI_EXPERIMENTAL=enabled

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)
ARCH=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/')
curl -L https://github.com/docker/buildx/releases/download/"${BUILDX_VERSION}"/buildx-"${BUILDX_VERSION}".linux-"${ARCH}" --output ~/.docker/cli-plugins/docker-buildx
chmod a+x ~/.docker/cli-plugins/docker-buildx
fi
}

create_or_use_buildx_builder() {
local builder_name=$1
if [ -z "$builder_name" ]; then
echo "Error: Builder name is required."
exit 1
fi

check_buildx

current_builder="$(docker buildx inspect "${builder_name}" 2>/dev/null)" || echo "Builder '${builder_name}' not found"

if ! grep -q "^Driver: docker$" <<<"${current_builder}" && \
grep -q "linux/amd64" <<<"${current_builder}" && \
grep -q "linux/arm64" <<<"${current_builder}" && \
grep -q "linux/s390x" <<<"${current_builder}"; then
echo "The current builder already has multi-architecture support (amd64, arm64, s390x)."
echo "Skipping setup as the builder is already configured correctly."
exit 0
fi

# Check if the builder already exists by parsing the output of `docker buildx ls`
# We check if the builder_name appears in the list of active builders
existing_builder=$(docker buildx ls | grep -w "$builder_name" | awk '{print $1}')

if [ -n "$existing_builder" ]; then
echo "Builder '$builder_name' already exists."
echo "Using existing builder '$builder_name'."
docker buildx use "$builder_name"
else
echo "Creating a new Docker Buildx builder: $builder_name"
docker buildx create --driver-opt network=host --use --name "$builder_name"
echo "The new builder '$builder_name' has been created and set as active."
fi
}

if [ $# -eq 1 ]; then
create_or_use_buildx_builder "$1"
else
echo "Usage: $0 <builder_name>"
echo "Example: $0 mybuilder"
exit 1
fi
Loading