Enable dynamic host architecture detection. #1928

ashokpariya0 · 2024-11-29T04:13:41Z

This update sets the platform dynamically based on the host architecture, enhancing flexibility for building multi-platform images across different architectures.

What this PR does / why we need it:
Instead of setting the platform value to amd64, these changes configure it dynamically based on the host architecture.
Enhance multi-architecture build support and optimize error handling
Automatically include all supported platforms when PLATFORMS=all is exported.
Removed --no-cache flag to speed up builds by utilizing cached layers.
Improved error handling for expected failures to provide cleaner logs.
Implemented architecture auto-detection for Docker buildx installation to support various platforms.

Special notes for your reviewer:
gist link: https://gist.github.com/ashokpariya0/19b6f235a3fe149d16d82d92c84ce542

Release note:

None

This update sets the platform dynamically based on the host architecture, enhancing flexibility for building multi-platform images across different architectures. Signed-off-by: Ashok Pariya <[email protected]>

kubevirt-bot · 2024-11-29T04:13:49Z

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

Thanks
I assume it works for all the platforms mentioned on the comment

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

Makefile

@@ -13,13 +13,13 @@ 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
+ARCH := $(shell uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/')
+PLATFORMS ?= linux/${ARCH}


wdyt about adding an option to set export PLATFORMS=all
which will be translated internally to linux/amd64,linux/arm64,linux/s390x ?
it is less explicit but easier to maintain

@oshoval That's a great idea! Adding an export PLATFORMS=all option would simplify the configuration and make it easier to maintain, especially as we add more platforms in the future. I'll include this change in a follow-up PR.
All tests have passed, and there is one pending tide test awaiting the approval label.

Updated PLATFORMS to automatically include all supported platforms when export PLATFORMS=all is used.

oshoval · 2024-12-01T11:34:38Z

cc @RamLavi @qinqon
can you take a look please ?

oshoval · 2024-12-02T15:14:13Z

hack/build-operator-podman.sh

+podman manifest rm "${OPERATOR_IMAGE_TAGGED}" 2>/dev/null || echo "Manifest ${OPERATOR_IMAGE_TAGGED} not found, skipping removal."
+podman rmi "${OPERATOR_IMAGE_TAGGED}" 2>/dev/null || echo "Image ${OPERATOR_IMAGE_TAGGED} not found, skipping removal."


btw i would say 2>/dev/null || true is enough please
(same for the rest of course)

oshoval

Thanks

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

Makefile

 # Set the platforms for building a multi-platform supported image.
 # Example:
 # PLATFORMS ?= linux/amd64,linux/arm64,linux/s390x
 # Alternatively, you can export the PLATFORMS variable like this:
 # export PLATFORMS=linux/arm64,linux/s390x,linux/amd64
-ARCH := $(shell uname -m | sed 's/x86_64/amd64/')
+# # or export PLATFORMS=all to automatically include all supported platforms.


Please add this feature to PR desc

nit: there are twice #

oshoval · 2024-12-02T15:24:10Z

hack/build-operator-podman.sh

+podman manifest rm "${OPERATOR_IMAGE_TAGGED}" 2>/dev/null
+podman rmi "${OPERATOR_IMAGE_TAGGED}" 2>/dev/null


you do need || true please, isnt it ?

in anyways it will suppress the error but let me add || true.

if it suppress the error in case image / manifest not found, no need to add, my mistake then
thanks

oshoval · 2024-12-02T15:25:03Z

hack/init-buildx.sh

@@ -20,7 +21,7 @@ create_or_use_buildx_builder() {

  check_buildx

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


lets just ... 2>/dev/null)" || true please
simpler, more quiet

hmm but wont it mess current_builder ?
maybe here we are not allowed to fail ?

hmm but wont it mess current_builder ? maybe here we are not allowed to fail ?

Yes correct, this is fine.

sorry i dont understand
it assigns a value to current_builder, and we can't just || echo here in case it fails, because then current_builder will have empty value and the script continue with unexpected empty value isnt it ?

Yes, you're right; it will have an empty value. However, we handle errors properly later in the script. I don't see any issues with either the failure or success case, so everything looks good.

please correct me if i am wrong,
this is an expression that must success, hence we should fail fast (i.e remove the || echo ... , and the stderr redirection) ?

No, by default, docker buildx inspect <builder-name> will print an error to the console if the builder is not found. Since we want to suppress this error, it's better to use || with an echo statement for debugging purposes.

oshoval · 2024-12-02T15:29:47Z

hack/build-operator-docker.sh

@@ -14,4 +14,5 @@ if [ ${#PLATFORM_LIST[@]} -eq 1 ]; then
 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."


lets just use 2>/dev/null || true please

we still have the old one it seems :)

I think it's better to keep this message, as this is not an expected failure. It will help us with debugging in case an issue arises.

Updated PLATFORMS to automatically include all supported platforms when export PLATFORMS=all is used. Removed --no-cache to improve build time by leveraging cached layers during the build process. Enhanced error handling for expected failures. Added auto-detection of architecture for Docker buildx installation to support multiple platforms. Signed-off-by: Ashok Pariya [email protected]

sonarqubecloud · 2024-12-02T15:32:15Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

oshoval · 2024-12-02T15:52:28Z

hack/build-operator-podman.sh

+podman manifest rm "${OPERATOR_IMAGE_TAGGED}" 2>/dev/null || true
+podman rmi "${OPERATOR_IMAGE_TAGGED}" 2>/dev/null || true


if podman suppress the error in case the images dont exists, please drop the || true (i dont know if this is the case by heart, but you raised it is, best to double check if you want please)

lets keep the code correct and as minimal as possible please
sorry for the back and forth

I think it's better to keep || true, Below is the reason:
Without || true: The command suppresses the error output but will still fail if there's an issue (e.g., manifest not found).
With || true: The command suppresses the error and ensures the script continues even if there's an issue.

ashokpariya0 · 2024-12-03T09:12:35Z

@RamLavi @qinqon @phoracek
Could you please take a look at the PR?

oshoval

Thanks

oshoval · 2024-12-03T12:57:05Z

@0xFelix
if you can review this one please, thanks

switching labels

/lgtm cancel
/approve

kubevirt-bot · 2024-12-03T12:57:11Z

[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:

~~OWNERS~~ [oshoval]

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

0xFelix

/lgtm

Enable dynamic host architecture detection.

a943400

This update sets the platform dynamically based on the host architecture, enhancing flexibility for building multi-platform images across different architectures. Signed-off-by: Ashok Pariya <[email protected]>

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 29, 2024

kubevirt-bot requested review from phoracek and qinqon November 29, 2024 04:13

kubevirt-bot added the size/XS label Nov 29, 2024

ashokpariya0 mentioned this pull request Nov 29, 2024

Enable multi-architecture support, specifically for s390x, in CNAO and the associated CNIs. #1916

Open

oshoval approved these changes Dec 1, 2024

View reviewed changes

kubevirt-bot assigned oshoval Dec 1, 2024

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

oshoval reviewed Dec 1, 2024

View reviewed changes

oshoval mentioned this pull request Dec 1, 2024

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

Merged

kubevirt-bot added size/S and removed lgtm Indicates that a PR is ready to be merged. size/XS labels Dec 2, 2024

ashokpariya0 mentioned this pull request Dec 2, 2024

Enable multi-platform support for operator image builds #1923

Merged

oshoval reviewed Dec 2, 2024

View reviewed changes

ashokpariya0 force-pushed the auto-set-platform branch from f3ff372 to 81732b6 Compare December 2, 2024 15:21

oshoval reviewed Dec 2, 2024

View reviewed changes

ashokpariya0 force-pushed the auto-set-platform branch from 81732b6 to b05457d Compare December 2, 2024 15:31

oshoval reviewed Dec 2, 2024

View reviewed changes

oshoval approved these changes Dec 3, 2024

View reviewed changes

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

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

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

0xFelix approved these changes Dec 4, 2024

View reviewed changes

kubevirt-bot assigned 0xFelix Dec 4, 2024

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

kubevirt-bot merged commit 9f14528 into kubevirt:main Dec 4, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable dynamic host architecture detection. #1928

Enable dynamic host architecture detection. #1928

ashokpariya0 commented Nov 29, 2024 •

edited

Loading

kubevirt-bot commented Nov 29, 2024

oshoval left a comment

oshoval Dec 1, 2024

ashokpariya0 Dec 1, 2024

ashokpariya0 Dec 2, 2024

oshoval commented Dec 1, 2024

oshoval Dec 2, 2024 •

edited

Loading

ashokpariya0 Dec 2, 2024

oshoval left a comment

oshoval Dec 2, 2024

ashokpariya0 Dec 2, 2024

oshoval Dec 2, 2024

ashokpariya0 Dec 2, 2024

oshoval Dec 2, 2024

oshoval Dec 2, 2024 •

edited

Loading

ashokpariya0 Dec 2, 2024

ashokpariya0 Dec 2, 2024

oshoval Dec 2, 2024

ashokpariya0 Dec 2, 2024

oshoval Dec 3, 2024 •

edited

Loading

ashokpariya0 Dec 3, 2024

oshoval Dec 2, 2024

ashokpariya0 Dec 2, 2024

oshoval Dec 2, 2024

ashokpariya0 Dec 2, 2024

sonarqubecloud bot commented Dec 2, 2024

oshoval Dec 2, 2024 •

edited

Loading

ashokpariya0 Dec 2, 2024

ashokpariya0 commented Dec 3, 2024

oshoval left a comment

oshoval commented Dec 3, 2024

kubevirt-bot commented Dec 3, 2024

0xFelix left a comment

		podman manifest rm "${OPERATOR_IMAGE_TAGGED}" 2>/dev/null \|\| echo "Manifest ${OPERATOR_IMAGE_TAGGED} not found, skipping removal."
		podman rmi "${OPERATOR_IMAGE_TAGGED}" 2>/dev/null \|\| echo "Image ${OPERATOR_IMAGE_TAGGED} not found, skipping removal."

Enable dynamic host architecture detection. #1928

Enable dynamic host architecture detection. #1928

Conversation

ashokpariya0 commented Nov 29, 2024 • edited Loading

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

oshoval commented Dec 1, 2024

oshoval Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

sonarqubecloud bot commented Dec 2, 2024

Quality Gate passed

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

oshoval left a comment

Choose a reason for hiding this comment

oshoval commented Dec 3, 2024

kubevirt-bot commented Dec 3, 2024

0xFelix left a comment

Choose a reason for hiding this comment

ashokpariya0 commented Nov 29, 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