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 operator image builds #1923

Merged

Conversation

ashokpariya0
Copy link
Contributor

These changes enable building and pushing CNAO operator image 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. Updated the docker-build-operator target to support builds with docker buildx and podman, and added new targets for creating Docker Buildx builders and pushing multi-platform images.

What this PR does / why we need it:
The existing CNAO operator (link) currently supports only the amd64 architecture. These changes extend multi-platform support to include arm64 and s390x architectures, enabling broader compatibility for the CNAO operator image.

Special notes for your reviewer:
Please note that Docker and Podman handle multi-architecture image builds differently. As a result, the changes have been made accordingly. Additionally, Go cross-compilation support has been added to build binaries for the target architecture.
Added a PLATFORMS argument in the Makefile to support building images for multiple architectures (amd64, s390x, arm64).
Updated the Dockerfile to dynamically handle multi-architecture builds.
Added Go cross-compilation support in Dockerfile to build binaries for the target architecture.
Enhanced the docker-build-operator target to enable multi-platform builds using both docker buildx and podman.
Introduced a new build-multiarch-operator-image target to build and push multi-platform images using podman.
Added a create-builder target to generate Docker Buildx builders for multi-platform image support.

Release note:

None

@kubevirt-bot 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. labels Nov 8, 2024
@kubevirt-bot
Copy link
Collaborator

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.

Copy link
Collaborator

@oshoval oshoval left a comment

Choose a reason for hiding this comment

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

Nice thanks
First pass

.dockerignore Outdated
pkg
cmd
tools
#pkg
Copy link
Collaborator

Choose a reason for hiding this comment

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

leftovers?

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 change is necessary because we need to build the two binaries listed here inside the Dockerfile in order to support Go cross-compilation for building the target binaries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets remove the lines then completely ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Removed in the latest commit.

Makefile Outdated
@@ -13,6 +13,14 @@ OPERATOR_IMAGE ?= cluster-network-addons-operator
REGISTRY_IMAGE ?= cluster-network-addons-registry
export 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,linux/arm64
Copy link
Collaborator

@oshoval oshoval Nov 10, 2024

Choose a reason for hiding this comment

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

lets have just amd64 by default, so it wont take time on each regular use ?
the rest can be in an example comment

moreover if we build just one image, we dont need buildx just docker build, much easier to maintain, usually this will be the case, unless on releases and such once time come,
and then we can keep the simple one image build on make,
(nit, not important) - and for multi docker build, have layout like
https://github.com/nmstate/kubernetes-nmstate does (wrt buildx scripts and make targets)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oshoval Thanks for the feedback! I agree that simplifying the default platform to just amd64 will streamline the build process and reduce unnecessary overhead. I'll update the configuration accordingly and move the other platforms into an example comment as you suggested.

As for the use of docker build vs. buildx, that approach makes sense for most cases. I'll switch to docker build for simplicity and reserve buildx for when it's needed.

During releases, we'll set PLATFORMS ?= linux/amd64,linux/s390x,linux/arm64, use buildx, and build and push a multi-platform image for the operator. For all other cases, we'll default to using amd64 as the platform. Does that sound good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sgtm thx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in the latest commit.

Makefile Outdated
Comment on lines 20 to 22
null :=
space := $(null) #
comma := ,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just inline those please?

Makefile Outdated

# Target to build the operator image with support for multiple platforms
build-multiarch-operator-image:
# Remove any existing manifest and image
Copy link
Collaborator

Choose a reason for hiding this comment

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

please drop the comments (all over unless it is not trivial)

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
Collaborator

Choose a reason for hiding this comment

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

please remove more comments, there are still some trivial ones
we usually add as few as possible
only for non trivial stuff

FROM quay.io/centos/centos:stream9
# Define build-time arguments
ARG BUILD_ARCH=amd64 # Default value for architecture is amd64
ARG GO_VERSION="1.23.3" # Default Go version is 1.23.3
Copy link
Collaborator

@oshoval oshoval Nov 10, 2024

Choose a reason for hiding this comment

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

might worth to auto take it from go.mod
i.e hack/go-version.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! I'll look into implementing that.

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 made the changes according to the script install-go.sh. With the current setup, we are compiling and building the manager binary using the Go version specified above.

Please note that the script go-version.sh returns the Go version in major.minor format. Additionally, the command curl -L -s "https://go.dev/dl/?mode=json" does not list Go 1.21, as it's archived. Before Go 1.22, we would need to download it directly from the archive. However, the install-go.sh script currently downloads the latest Go version and proceeds to compile with it.

I’ve made the same update in the Dockerfile to ensure consistency.

Comment on lines 7 to 12
RUN dnf install -y tar gzip && dnf clean all
RUN ARCH=$(uname -m | sed 's/x86_64/amd64/') && \
curl -L https://go.dev/dl/go${GO_VERSION}.linux-${ARCH}.tar.gz -o go.tar.gz && \
tar -C /usr/local -xzf go.tar.gz && \
rm go.tar.gz

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe using docker ADD can simplify some of this ?

COPY build/_output/bin/manager $OPERATOR
COPY build/_output/bin/manifest-templator $MANIFEST_TEMPLATOR

# Copy the final built binaries into the image
Copy link
Collaborator

Choose a reason for hiding this comment

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

please drop comments all over unless non trivial

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

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

# Function to check if Docker Buildx is available
Copy link
Collaborator

Choose a reason for hiding this comment

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

please drop comments unless non trivial

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
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.
FYI- It would be preferable to pass the builder name at line 30 in the script init-buildx.sh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we make it even closer to that script ?
will be easier to maintain, it still seems too close to the original version if i am not wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did that.
Just to let you know, I noticed an issue in this line of init-buildx.sh — we are not performing the inspection based on the builder name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks, if you think it worth it you can open an issue on nmstate repo
i must work on some other stuff at this very moment, so will take me bit time to review

@ashokpariya0 ashokpariya0 force-pushed the multi-platform-operator-support branch 2 times, most recently from 2fdfd41 to 993cbb4 Compare November 12, 2024 10:35
@oshoval
Copy link
Collaborator

oshoval commented Nov 13, 2024

@phoracek @RamLavi @0xFelix
please consider also reviewing if you can, to see we dont miss anything

Copy link
Collaborator

@oshoval oshoval left a comment

Choose a reason for hiding this comment

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

Thanks,
there are some comments that were not addressed / replied to
please check those as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we make it even closer to that script ?
will be easier to maintain, it still seems too close to the original version if i am not wrong

Makefile Outdated

# Target to build the operator image with support for multiple platforms
build-multiarch-operator-image:
# Remove any existing manifest and image
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove more comments, there are still some trivial ones
we usually add as few as possible
only for non trivial stuff

Makefile Outdated
Comment on lines 144 to 147
else ifeq ($(OCI_BIN),docker)
ifeq ($(words $(PLATFORM_LIST)), 1)
docker push ${TLS_SETTING} ${OPERATOR_IMAGE_TAGGED}
endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

please try to think about a makefile target structure that can be simpler wrt to multi platform and one platform
the logic is split here and on docker-build-operator, it will be better to have it concentrated if possible (i understand it might be not possible because there are stages)

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 docker buildx, we need to combine both build and push in a single command because the images are not stored locally by default, unlike with docker build. As a result, we don't need a separate docker push command when working with multi-platform builds. To simplify the logic and maintain consistency, we could consider adding the --push option to docker build as well. What do you think @oshoval

Copy link
Collaborator

Choose a reason for hiding this comment

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

might be interesting to try if you think it will simplify it please

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.

@ashokpariya0 ashokpariya0 force-pushed the multi-platform-operator-support branch from 993cbb4 to 209a203 Compare November 13, 2024 10:58
@kubevirt-bot kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 13, 2024
@ashokpariya0 ashokpariya0 force-pushed the multi-platform-operator-support branch from f3073b3 to a853967 Compare November 13, 2024 11:51
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Nov 13, 2024
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

Perhaps an unpopular opinion, but why do we need to maintain two builds for Podman and Docker? In https://github.com/kubevirt/ssp-operator we went with Podman only.

See kubevirt/ssp-operator#1091

@oshoval
Copy link
Collaborator

oshoval commented Nov 14, 2024

Perhaps an unpopular opinion, but why do we need to maintain two builds for Podman and Docker? In https://github.com/kubevirt/ssp-operator we went with Podman only.

See kubevirt/ssp-operator#1091

personally i am against dropping docker support
it is convenient it exists, and also on kubevirt we still do that for some jobs
moreoever, cnao depends on lot of other component, so it better to be robust in case something happens and is required

While it is a nice question, not sure if the PR is the right place for it (i.e it shouldn't block it)

Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

@oshoval Not to block the PR, but IMO it adds a lot of complexity which I'm not sure is required. (especially looking at hack/build-multiarch-operator.sh)

Makefile Outdated
Comment on lines 25 to 27
NULL :=
SPACE := $(NULL) #
COMMA := ,
Copy link
Member

Choose a reason for hiding this comment

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

Are these vars really required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was needed, but removed it latest commit and moved to shell script.


docker-build-registry:
$(OCI_BIN) build -f build/registry/Dockerfile -t $(IMAGE_REGISTRY)/$(REGISTRY_IMAGE):$(IMAGE_TAG) .

docker-push: docker-push-operator docker-push-registry

docker-push-operator:
$(OCI_BIN) push ${TLS_SETTING} $(IMAGE_REGISTRY)/$(OPERATOR_IMAGE):$(IMAGE_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.

This is called docker-push-operator but uses podman?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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 to podman, IIUC?

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok thanks

Copy link
Contributor Author

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.

Makefile Outdated
Comment on lines 253 to 256
--build-arg BUILD_ARCH=$(ARCH) \
--platform $(PLATFORMS) \
-f build/operator/Dockerfile \
-t ${OPERATOR_IMAGE_TAGGED} --push .
Copy link
Member

Choose a reason for hiding this comment

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

Can you reuse/deduplicate the args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest commit.

# If there is no buildx let's install it
if ! docker buildx > /dev/null 2>&1; then
mkdir -p ~/.docker/cli-plugins
curl -L https://github.com/docker/buildx/releases/download/v0.6.3/buildx-v0.6.3.linux-amd64 --output ~/.docker/cli-plugins/docker-buildx
Copy link
Member

Choose a reason for hiding this comment

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

Hardcoded buildx version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, corrected it.

Copy link
Member

Choose a reason for hiding this comment

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

Did you run shellcheck against this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@oshoval
Copy link
Collaborator

oshoval commented Nov 17, 2024

@oshoval Not to block the PR, but IMO it adds a lot of complexity which I'm not sure is required. (especially looking at hack/build-multiarch-operator.sh)

It is fine to block while we discuss how to make it better please,
We can indeed request to have a separate script for it instead having it internal as it is now, wdyt?
Unless you have other thing in mind (we do need to support multi platform on podman / docker)

@ashokpariya0 ashokpariya0 force-pushed the multi-platform-operator-support branch from a853967 to 22f160c Compare November 17, 2024 11:37
@ashokpariya0
Copy link
Contributor Author

@oshoval @0xFelix The review comments have been addressed. Please let me know if there's anything else I need to do.

@ashokpariya0 ashokpariya0 force-pushed the multi-platform-operator-support branch from 22f160c to 820b930 Compare November 18, 2024 07:53

ARG TARGETOS
ARG TARGETARCH
ARG TARGETPLATFORM
Copy link
Member

Choose a reason for hiding this comment

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

Is this arg used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

I may be blind, but where?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we use at

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/...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TARGETPLATFORM can be removed.

./hack/init-buildx.sh "$DOCKER_BUILDER"
docker buildx build --platform "$PLATFORMS" $BUILD_ARGS
fi

Copy link
Member

Choose a reason for hiding this comment

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

Empty lines at EOF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Comment on lines +14 to +15
ARG TARGETOS
ARG TARGETARCH
Copy link
Member

Choose a reason for hiding this comment

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

Where are these set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

@ashokpariya0 ashokpariya0 force-pushed the multi-platform-operator-support branch from 820b930 to 106c2b6 Compare November 18, 2024 16:33
@ashokpariya0
Copy link
Contributor Author

@oshoval @0xFelix I’ve addressed all the review comments.
Could you please take another look and proceed with the next steps for the PR?
Let me know if there’s anything else you’d like me to update.

@ashokpariya0 ashokpariya0 force-pushed the multi-platform-operator-support branch from 106c2b6 to 4aa7e93 Compare November 19, 2024 10:25
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024
Copy link
Collaborator

@oshoval oshoval left a comment

Choose a reason for hiding this comment

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

quick pass


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') && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

i dont think we want unpinned go version?
stuff can break suddenly

unless we are fine with that or if this was the case already, lets see other maintainers 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 was already the case; this PR follows the same approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please consider in the follow-up to use ADD if it helps

exit 1
fi

# Split the comma-separated platforms into an array
Copy link
Collaborator

Choose a reason for hiding this comment

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

please drop comments
from all files, unless non trivial

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.


docker-build-registry:
$(OCI_BIN) build -f build/registry/Dockerfile -t $(IMAGE_REGISTRY)/$(REGISTRY_IMAGE):$(IMAGE_TAG) .

docker-push: docker-push-operator docker-push-registry

docker-push-operator:
$(OCI_BIN) push ${TLS_SETTING} $(IMAGE_REGISTRY)/$(OPERATOR_IMAGE):$(IMAGE_TAG)
ifeq ($(OCI_BIN),podman)
Copy link
Collaborator

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

@oshoval
Copy link
Collaborator

oshoval commented Nov 20, 2024

please see pull-cluster-network-addons-operator-unit-test

@ashokpariya0
Copy link
Contributor Author

please see pull-cluster-network-addons-operator-unit-test

Sure.

These changes enable building and pushing 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.
Updated the docker-build-operator target to support builds with docker buildx
and podman, and added new targets for creating Docker Buildx builders and
pushing multi-platform images.

Signed-off-by: Ashok Pariya <[email protected]>
@ashokpariya0 ashokpariya0 force-pushed the multi-platform-operator-support branch from 4aa7e93 to 46b23df Compare November 20, 2024 12:17
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud

@oshoval
Copy link
Collaborator

oshoval commented Nov 20, 2024

Thanks

@qinqon
any thing you want to add please ?

@ashokpariya0
Copy link
Contributor Author

Could we please have a release once this PR is merged? There were commits made after the last release (v0.96.0) on October 8th. We would like to perform end-to-end testing on this release.
Also, as discussed earlier, please set PLATFORMS=linux/amd64,linux/s390x before pushing the upstream image, as by default it will only publish amd64-compatible images.

@oshoval
Copy link
Collaborator

oshoval commented Nov 20, 2024

yes thanks but at this very moment i cant work on that
once one of us have more time

you are welcome if you want to check project-infra and suggest a PR

@ashokpariya0
Copy link
Contributor Author

ashokpariya0 commented Nov 20, 2024

yes thanks but at this very moment i cant work on that once one of us have more time

you are welcome if you want to check project-infra and suggest a PR

Okay, thanks! For now, merging the PR once it passes the tests would be helpful.

@oshoval
Copy link
Collaborator

oshoval commented Nov 21, 2024

@RamLavi
if you want to also take a look, before we approve soon

@ashokpariya0
Copy link
Contributor Author

@RamLavi Would like to add anything ??
@oshoval Could you please approve the changes if no further actions are required, so that it can trigger the other tests?

@oshoval
Copy link
Collaborator

oshoval commented Nov 24, 2024

Very nice thanks
/approve

@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oshoval

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2024
@ashokpariya0
Copy link
Contributor Author

@oshoval Thanks for approving the PR! How can we trigger all the tests for it?

@oshoval
Copy link
Collaborator

oshoval commented Nov 24, 2024

/test all

thought it auto triggers once lgtm approve
EDIT - ah lgtm is missing

@ashokpariya0
Copy link
Contributor Author

Thanks @oshoval all tests have passed. There's one pending test job (tide) that requires the LGTM label.

@oshoval
Copy link
Collaborator

oshoval commented Nov 25, 2024

Hi @0xFelix
Can you please review / lgtm once you are good with it ?
Thanks

Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2024
@kubevirt-bot kubevirt-bot merged commit 302ad3e into kubevirt:main Nov 25, 2024
10 of 15 checks passed
@oshoval
Copy link
Collaborator

oshoval commented Dec 1, 2024

Hi
Please check this error before we advance on any PR (EDIT - maybe this is just the rm when it doesnt exists, worth to forward stderr to /dev/null)

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1923/pull-cluster-network-addons-operator-unit-test/1860989134017925120#1:build-log.txt%3A10836

also i dont understand how come some lanes seems they didn't run
I will create a vanilla main PR to see CNAO state

@oshoval
Copy link
Collaborator

oshoval commented Dec 1, 2024

Created #1929
we can first see what going on there and act accordingly - EDIT seems it pass so we are good

Might need some time to upgrade the non laptop machine i am using, sorry about that, but otherwise
hard to maintain CNAO. - EDIT machines are back to business
Meanwhile with laptop it does work.


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

Choose a reason for hiding this comment

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

please drop the no-cache on a follow-up

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
Collaborator

Choose a reason for hiding this comment

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

Thanks

Comment on lines +11 to +12
podman manifest rm "${OPERATOR_IMAGE_TAGGED}" || true
podman rmi "${OPERATOR_IMAGE_TAGGED}" || true
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in PR: #1928

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants