-
Notifications
You must be signed in to change notification settings - Fork 51
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 operator image builds #1923
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,9 +12,6 @@ | |
_kubevirtci | ||
|
||
# No need for source code is already compiled | ||
pkg | ||
cmd | ||
tools | ||
vendor | ||
docs | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,24 @@ | ||
FROM quay.io/centos/centos:stream9 | ||
ARG BUILD_ARCH=amd64 | ||
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') && \ | ||
curl -L "https://go.dev/dl/${GO_VERSION}.linux-${ARCH}.tar.gz" -o go.tar.gz && \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont think we want unpinned go version? unless we are fine with that or if this was the case already, lets see other maintainers idea There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was already the case; this PR follows the same approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oshoval For this as well, I will create a separate issue, resolve it, and provide a follow-up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, if this was already the case, no rush to fix it, its ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please consider in the follow-up to use ADD if it helps |
||
tar -C /usr/local -xzf go.tar.gz && \ | ||
rm go.tar.gz | ||
ENV PATH=$PATH:/usr/local/go/bin | ||
WORKDIR /go/src/cluster-network-addons-operator | ||
COPY . . | ||
|
||
ARG TARGETOS | ||
ARG TARGETARCH | ||
|
||
Comment on lines
+14
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where are these set? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TARGETOS and TARGETARCH are set by docker/podman based on the platform you're building for based on (--platform ) flag. |
||
RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -o build/_output/bin/manager ./cmd/... && \ | ||
CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -o build/_output/bin/manifest-templator ./tools/manifest-templator/... | ||
|
||
FROM --platform=linux/${TARGETARCH} quay.io/centos/centos:stream9 AS final | ||
|
||
ENV ENTRYPOINT=/entrypoint \ | ||
OPERATOR=/cluster-network-addons-operator \ | ||
MANIFEST_TEMPLATOR=/manifest-templator \ | ||
|
@@ -8,13 +28,16 @@ ENV ENTRYPOINT=/entrypoint \ | |
RUN \ | ||
yum -y update && \ | ||
yum clean all | ||
|
||
COPY build/operator/bin/user_setup /user_setup | ||
COPY build/operator/bin/csv-generator /usr/bin/csv-generator | ||
COPY templates/cluster-network-addons/VERSION/cluster-network-addons-operator.VERSION.clusterserviceversion.yaml.in cluster-network-addons-operator.VERSION.clusterserviceversion.yaml.in | ||
RUN /user_setup | ||
COPY data /data | ||
COPY build/_output/bin/manager $OPERATOR | ||
COPY build/_output/bin/manifest-templator $MANIFEST_TEMPLATOR | ||
|
||
COPY --from=builder /go/src/cluster-network-addons-operator/build/_output/bin/manager $OPERATOR | ||
COPY --from=builder /go/src/cluster-network-addons-operator/build/_output/bin/manifest-templator $MANIFEST_TEMPLATOR | ||
COPY build/operator/bin/entrypoint $ENTRYPOINT | ||
|
||
ENTRYPOINT $ENTRYPOINT | ||
USER $USER_UID |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
#!/bin/bash | ||
|
||
if [ -z "$ARCH" ] || [ -z "$PLATFORMS" ] || [ -z "$OPERATOR_IMAGE_TAGGED" ]; then | ||
echo "Error: ARCH, PLATFORMS and OPERATOR_IMAGE_TAGGED must be set." | ||
exit 1 | ||
fi | ||
|
||
IFS=',' read -r -a PLATFORM_LIST <<< "$PLATFORMS" | ||
|
||
BUILD_ARGS="--build-arg BUILD_ARCH=$ARCH -f build/operator/Dockerfile -t $OPERATOR_IMAGE_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 | ||
fi |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
#!/bin/bash | ||
|
||
if [ -z "$ARCH" ] || [ -z "$PLATFORMS" ] || [ -z "$OPERATOR_IMAGE_TAGGED" ]; then | ||
echo "Error: ARCH, PLATFORMS, and OPERATOR_IMAGE_TAGGED must be set." | ||
exit 1 | ||
fi | ||
|
||
IFS=',' read -r -a PLATFORM_LIST <<< "$PLATFORMS" | ||
|
||
# Remove any existing manifest and image | ||
podman manifest rm "${OPERATOR_IMAGE_TAGGED}" || true | ||
podman rmi "${OPERATOR_IMAGE_TAGGED}" || true | ||
Comment on lines
+11
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please consider adding redir of stderr to /dev/null so it will clean up the logs from marking error on CI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in PR: #1928 |
||
|
||
podman manifest create "${OPERATOR_IMAGE_TAGGED}" | ||
|
||
for platform in "${PLATFORM_LIST[@]}"; do | ||
podman build \ | ||
--no-cache \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please drop the no-cache on a follow-up There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks |
||
--build-arg BUILD_ARCH="$ARCH" \ | ||
--platform "$platform" \ | ||
--manifest "${OPERATOR_IMAGE_TAGGED}" \ | ||
-f build/operator/Dockerfile \ | ||
. | ||
done |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
#!/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) | ||
curl -L https://github.com/docker/buildx/releases/download/"${BUILDX_VERSION}"/buildx-"${BUILDX_VERSION}".linux-amd64 --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}")" | ||
|
||
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 |
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.
This is called docker-push-operator but uses podman?
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.
@ashokpariya0 Can you comment on this?
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.
Yes, We may need to modify the section at Makefile lines 113-126(https://github.com/kubevirt/cluster-network-addons-operator/blob/main/Makefile#L113-L126), but this can be addressed in a separate PR, as renaming might impact some build processes or jobs. The existing code already supports both Docker and Podman, as indicated in Makefile line 14.
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.
With your change this target will only push in case
OCI_BIN
is equal topodman
, IIUC?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.
Yes exactly, with docker buildx, push is combined with image build.
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.
fine by me to address naming on follow-up
thx
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.
sorry that i change my mind, can you please estimate how deep the change will be doing it in this PR on separate commits ?
once we have the estimation we can decide
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.
@oshoval I've already looked into this and found that
docker-*
is being called from various scripts in the automation, hack folder, etc. Additionally, we need to update the README and other related files. Including all of this in a single commit would add unnecessary complexity. It would be better to handle these changes separately.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 thanks
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.
@oshoval I will create an separate issue for this, resolve it, and provide a follow-up.