Enable support for multiple platforms in the Linux Bridge CNI. #1927

ashokpariya0 · 2024-11-28T11:13:32Z

These updates allow the building and pushing of Linux Bridge container images for multiple platforms (e.g., amd64, s390x) using a single Dockerfile. Multi-platform build support is provided for both Docker and Podman container runtimes.

What this PR does / why we need it:

"The upstream cni-default-plugins image ([link](https://quay.io/repository/kubevirt/cni-default-plugins?tab=tags&tag=latest)) currently supports only the amd64 architecture. The proposed changes in this PR enable multi-platform support for the linux-bridge image, broadening its compatibility to include additional architectures like s390x."

Special notes for your reviewer:
Multi-platform build support is enabled for both Docker and Podman container runtimes, and the necessary changes have been made accordingly.

To use this feature, you need to:
For Podman:
No additional dependencies are required to build multi-platform images. Podman supports this feature natively.
and We set the default value of OCI_BIN to podman if it's available, so we are good here.

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

Release note:

None

kubevirt-bot · 2024-11-28T11:13:44Z

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 · 2024-12-01T09:13:55Z

hack/components/bump-linux-bridge.sh

+ARCH=$(uname -m | sed 's/x86_64/amd64/')
+PLATFORMS="linux/amd64,linux/s390x"


please add auto detection instead hardcoding both, we should be able to compile as fast as possible

we can have a flag export PLATFORMS=all that is translated to push all, which we can use on CI when publishing to quay

oshoval

Thanks, some comments

oshoval · 2024-12-01T09:16:37Z

hack/components/bump-linux-bridge.sh

@@ -39,7 +46,7 @@ RUN \
 WORKDIR ${LINUX_BRIDGE_PATH}
 RUN GOFLAGS=-mod=vendor ./build_linux.sh

-FROM registry.access.redhat.com/ubi8/ubi-minimal
+FROM registry.access.redhat.com/ubi8/ubi-minimal AS final


please drop AS final, it is not used
prone theoretically for mistakes if someone adds layers below that

oshoval · 2024-12-01T09:17:41Z

hack/components/bump-linux-bridge.sh

+    podman manifest create "${LINUX_BRIDGE_IMAGE_TAGGED}"
+
+    for platform in "${PLATFORM_LIST[@]}"; do
+        podman build --no-cache --build-arg BUILD_ARCH="$ARCH" --platform "$platform" --manifest "${LINUX_BRIDGE_IMAGE_TAGGED}" .


why no-cache ?
we do want cache locally (fast build is important)

As discussed no-cache is removed to speed up build.

oshoval · 2024-12-01T09:20:44Z

hack/components/bump-linux-bridge.sh

+modify_dockerfile_for_platform_and_architecture() {
+    local dockerfile="$1"
+    # Modify Dockerfile to set platform and architecture
+    sed -i 's|^FROM registry.access.redhat.com/ubi8/ubi-minimal AS builder$|FROM --platform=linux/${BUILD_ARCH} registry.access.redhat.com/ubi8/ubi-minimal AS builder|' "$dockerfile"
+    sed -i 's|RUN GOFLAGS=-mod=vendor ./build_linux.sh|RUN GOFLAGS=-mod=vendor GOARCH=${TARGETARCH} GOOS=${TARGETOS} ./build_linux.sh|' "$dockerfile"
+    sed -i 's/^FROM registry.access.redhat.com\/ubi8\/ubi-minimal AS final$/FROM --platform=linux\/${TARGETARCH} registry.access.redhat.com\/ubi8\/ubi-minimal AS final/' "$dockerfile"
+}


i dont understand why we sed -i instead directly building the dockerfile as needed please
after all the file is built in create_dockerfile right?
lets refactor this please

We need to use TARGETARCH and TARGETOS in the Dockerfile, but these values are only available at runtime. Since the Dockerfile is generated using a cat command, directly inserting these variables during creation would result in empty values, leading to an incorrect Dockerfile. To address this, we generate the Dockerfile first and then update it using the sed command to properly set the values at runtime.

I am a bit confused why it is different than all the other cases where we have a real Dockerfile (not catted)
with those as args, but we can take it offline, and unblock this PR, (because in any case, not sure we want a standalone Dockerfile for the bump / publish)

Fixed this, Removed sed commands and used backslash to avoid substitution while creating file for few specific lines with cat command.

oshoval · 2024-12-01T09:21:36Z

hack/components/bump-linux-bridge.sh

+    if [ "${OCI_BIN}" == "podman" ]; then
+        if [ ! -z ${PUSH_IMAGES} ]; then
+            podman manifest push "${LINUX_BRIDGE_IMAGE_TAGGED}"
+        fi


please move it after build_podman_image

oshoval

Just a small comment, thanks

oshoval · 2024-12-08T07:37:26Z

hack/components/bump-linux-bridge.sh

+# 2. Or specify specific platforms: export PLATFORMS=linux/amd64,linux/arm64
+PLATFORM_LIST="linux/amd64,linux/s390x,linux/arm64"
+ARCH=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/')
+[ -z "$PLATFORMS" ] && PLATFORMS="linux/${ARCH}"


please use default value bash syntax, as in line 30

oshoval · 2024-12-10T08:38:35Z

Looks great thanks

/test all
/approve

@0xFelix
can you please take a look ?

kubevirt-bot · 2024-12-10T08:38:43Z

[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

Can you run shellcheck on this file and fix the issues introduced by this PR?

oshoval · 2024-12-10T11:41:56Z

Can you run shellcheck on this file and fix the issues introduced by this PR?

Note that some the warnings are considered annoyance imho, so it is better to have judgement per warning please
(i even saw some delicate ones that can create bugs, but on more sophisticated scripts like we have on kubevirtci sriov)

ashokpariya0 · 2024-12-10T11:56:54Z

Can you run shellcheck on this file and fix the issues introduced by this PR?

This PR does not introduce any new warnings; I have already verified it using shellcheck

0xFelix · 2024-12-10T13:44:56Z

In /home/felix/Downloads/bump-linux-bridge.sh line 95:
        if [ ! -z "${PUSH_IMAGES}" ]; then
             ^-- SC2236 (style): Use -n instead of ! -z.


In /home/felix/Downloads/bump-linux-bridge.sh line 103:
    cd ${LINUX_BRIDGE_PATH}
       ^------------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
    cd "${LINUX_BRIDGE_PATH}"

These are relevant.

oshoval · 2024-12-10T13:50:14Z

The 2nd isnt regression of this PR,
as long as we don't afraid from word splitting / globbing in our values i prefer to not to enforce it please.

I remember something vaguely about -n when not using quotes, if the value is empty, it can have undesired output, but value shouldn't be empty here.
If needed I can find it.

More over, the 1st isn't regression of this PR, so imo we don't need to enforce fixing it on this PR
(and it is syntactic equivalent)

oshoval · 2024-12-10T14:26:06Z

Once this PR is merged, also need to recall / find out please how to trigger its release

ashokpariya0 · 2024-12-10T14:46:01Z

In /home/felix/Downloads/bump-linux-bridge.sh line 95:
        if [ ! -z "${PUSH_IMAGES}" ]; then
             ^-- SC2236 (style): Use -n instead of ! -z.


In /home/felix/Downloads/bump-linux-bridge.sh line 103:
    cd ${LINUX_BRIDGE_PATH}
       ^------------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
    cd "${LINUX_BRIDGE_PATH}"

These are relevant.

The 2nd isnt regression of this PR, as long as we don't afraid from word splitting / globbing in our values i prefer to not to enforce it please.

I remember something vaguely about -n when not using quotes, if the value is empty, it can have undesired output, but value shouldn't be empty here. If needed I can find it.

More over, the 1st isn't regression of this PR, so imo we don't need to enforce fixing it on this PR (and it is syntactic equivalent)

@oshoval I also encountered some issues recently when trying to achieve a 100% result in ShellCheck without any warnings.
Since none of the warnings above were introduced by this PR, can we proceed with it? I can submit a follow-up PR if needed. @0xFelix

oshoval · 2024-12-10T14:49:00Z

I think we can proceed please,
btw a follow-up on this isn't crucial at the moment.
Thanks

0xFelix · 2024-12-10T14:49:11Z

I'm not a fan of leaving warnings in place when you change the code anyway. But it's up to you to decide.

ashokpariya0 · 2024-12-10T15:05:55Z

imo we can proceed please, and even we don't need a follow-up about it from my side atm thanks

okay.

oshoval · 2024-12-15T13:57:57Z

Thanks

seems https://github.com/kubevirt/cluster-network-addons-operator/actions/runs/12339360372/job/34435568203?pr=1927 is failing, would you have time to look please ?
it seems it is just because the packages it tries to uninstall arent there,
simple fix it to || true with that
better is to check what https://github.com/ovn-kubernetes/ovn-kubernetes are doing on their lane, because they were the ref

oshoval · 2024-12-15T14:08:12Z

/test all

ashokpariya0 · 2024-12-15T14:08:15Z

https://github.com/ovn-kubernetes/ovn-kubernetes

Thanks

seems https://github.com/kubevirt/cluster-network-addons-operator/actions/runs/12339360372/job/34435568203?pr=1927 is failing, would you have time to look please ? it seems it is just because the packages it tries to uninstall arent there, simple fix it to || true with that better is to check what https://github.com/ovn-kubernetes/ovn-kubernetes are doing on their lane, because they were the ref

I don't see any change w.r.t to free disk space https://github.com/ovn-kubernetes/ovn-kubernetes/blob/eee91422cc619cde68dcb4e3be30ccf33e188867/.github/workflows/test.yml#L295

oshoval · 2024-12-15T14:09:17Z

I don't see any change w.r.t to free disk space https://github.com/ovn-kubernetes/ovn-kubernetes/blob/eee91422cc619cde68dcb4e3be30ccf33e188867/.github/workflows/test.yml#L295

i believe git actions default container was changed and it will fail there soon as well if we use the same

oshoval · 2024-12-15T14:11:06Z

lets try to remove please the ones that NA, and see if it pass ? (well it might be flaky if there wont be enough space, but this is a start)

oshoval · 2024-12-15T14:19:08Z

unit test fail on kmp image mismatch (might be related to the last PR that was merged)
https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1927/pull-cluster-network-addons-operator-unit-test/1868297019211124736

maybe @RamLavi knows something about it ?

diff --git a/test/releases/99.0.0.go b/test/releases/99.0.0.go
index b3c8ca32..2784c3fa 100644
--- a/test/releases/99.0.0.go
+++ b/test/releases/99.0.0.go
@@ -42,7 +42,7 @@ func init() {
 				ParentName: "kubemacpool-mac-controller-manager",
 				ParentKind: "Deployment",
 				Name:       "manager",
-				Image:      "quay.io/kubevirt/kubemacpool@sha256:eebb65b8a12cbfc20a429bbba399eb5a5c2279f8613c36965957ee7c36cfcbd6",
+				Image:      "quay.io/kubevirt/kubemacpool@sha256:15c7be1e7aad2836237e9a73443192bfa58c044fcc5488b175222f3ae997064f",
 			},
 			{
 				ParentName: "kubemacpool-mac-controller-manager",
@@ -54,7 +54,7 @@ func init() {
 				ParentName: "kubemacpool-cert-manager",
 				ParentKind: "Deployment",
 				Name:       "manager",
-				Image:      "quay.io/kubevirt/kubemacpool@sha256:eebb65b8a12cbfc20a429bbba399eb5a5c2279f8613c36965957ee7c36cfcbd6",
+				Image:      "quay.io/kubevirt/kubemacpool@sha256:15c7be1e7aad2836237e9a73443192bfa58c044fcc5488b175222f3ae997064f",
 			},
 			{
 				ParentName: "ovs-cni-amd64",
+ exit 1
make: *** [Makefile:101: check] Error 1

oshoval · 2024-12-15T14:25:39Z

opened ovn-kubernetes/ovn-kubernetes#4914 and the one that is mentioned there
ipam-ext one (which has the same code as here) also fails for the same
lets see if OVN as well

oshoval · 2024-12-15T14:46:32Z

still fails after rebase, unit test should be fixed first, and worth to check if main branch state is broken
after the KMP bump

oshoval · 2024-12-15T14:47:18Z

opened for that #1944

oshoval · 2024-12-15T15:12:49Z

storage problem also hit OVN
ovn-kubernetes/ovn-kubernetes#4914

ashokpariya0 · 2024-12-15T15:13:11Z

opened ovn-kubernetes/ovn-kubernetes#4914 and the one that is mentioned there ipam-ext one (which has the same code as here) also fails for the same lets see if OVN as well

The issue also occurs with OVN, as it seems that ubuntu-latest now points to Ubuntu 24.04. This is causing disk cleanup problems across the board. We can address this by either removing only the installed packages or switching to ubuntu-22.04.

Additionally, the following link still doesn't reflect the correct version:

https://github.com/actions/runner-images

Please also review the issue here: actions/runner-images#10636.

oshoval · 2024-12-15T15:15:07Z

Thank you
best to sync please with OVN people with your findings on ovn-kubernetes/ovn-kubernetes#4914 and choose the right direction
I will post that PR on slack once you post your findings there (or can just link your message, what ever you prefer)

ashokpariya0 · 2024-12-15T15:18:49Z

Thank you best to sync please with OVN people with your findings on ovn-kubernetes/ovn-kubernetes#4914 and choose the right direction I will post that PR on slack once you post your findings there (or can just link your message, what ever you prefer)

Sure

jommeke22f · 2024-12-15T15:32:55Z

Deze updates maken het mogelijk om Linux Zeecontainers te bouwen en te pushen voor meerdere platformen (bijv. amd64, s390x) met behulp van één Docker file. Ondersteuning voor multiplatform builds wordt geboden voor zowel Docker- als Persconferenties.

Wat deze PR doet / waarom we het nodig hebben :

"De upstream calculations image ([ link ]( https://quay.io/repository/kubevirt/cni-default-plugins?tab=tags&tag=latest )) ondersteunt momenteel alleen de architecture. De voorgestelde wijzigingen in deze PR maken cultuurverandering mogelijk voor de linux-bridge image, waardoor de compatibiliteit wordt uitgebreid met extra architecturen zoals s390x."

Speciale opmerkingen voor uw reviewer : Ondersteuning voor multi-platform builds is ingeschakeld voor zowel Docker- als Postmodernisme en de nodige wijzigingen zijn dienovereenkomstig doorgevoerd.

Om deze functie te gebruiken, moet u het volgende doen: Voor Polman: Er zijn geen extra afhankelijkheden vereist om multi-platform images te bouwen. Podia ondersteunt deze functie standaard. en We stellen de standaardwaarde van OCI_BIN in op podium als deze beschikbaar is, dus we zitten hier goed.

Voor Docker: Heb toegang tot docker build. Meer informatie: [ Docker Documentatie ]( https://docs.docker.com/build/building/multi-platform/ ) Zorg ervoor dat Builds is ingeschakeld. Meer informatie: [ Docker Kwaliteitsverbetering ]( https://docs.docker.com/develop/develop-images/build_enhancements/ )

Release-opmerking :
None

ashokpariya0 · 2024-12-16T05:11:31Z

Before merging this PR, we need to first merge the following:

The PR that fixes the unit test issue with the Kubemacpool image: #1946
The PR that provides a workaround for the cleanup issue with the latest Ubuntu version: #1945

oshoval · 2024-12-16T09:38:39Z

can you please rebase to take ipam fix ?
we will also run the rest

ashokpariya0 · 2024-12-16T09:41:33Z

can you please rebase to take ipam fix ? we will also run the rest

I am on it.

These updates allow the building and pushing of Linux Bridge container images for multiple platforms (e.g., amd64, s390x) using a single Dockerfile. Multi-platform build support is provided for both Docker and Podman container runtimes. Signed-off-by: Ashok Pariya <[email protected]>

sonarqubecloud · 2024-12-16T09:46:30Z

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

ashokpariya0 · 2024-12-16T09:46:35Z

can you please rebase to take ipam fix ? we will also run the rest

Done.

ashokpariya0 · 2024-12-16T10:18:45Z

@oshoval IPAM controller e2e tests passed on this pr, could you please trigger all other tests?

oshoval · 2024-12-16T10:30:56Z

Thanks

/test all

oshoval · 2024-12-16T10:31:53Z

/lgtm

since was removed just due to rebase

oshoval · 2024-12-16T10:32:14Z

/hold cancel

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

kubevirt-bot added the size/M label Nov 28, 2024

kubevirt-bot requested review from qinqon and RamLavi November 28, 2024 11:13

ashokpariya0 mentioned this pull request Nov 28, 2024

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

Open

oshoval reviewed Dec 1, 2024

View reviewed changes

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 Dec 5, 2024

ashokpariya0 force-pushed the enable-multi-platform-linux-bridge branch from b070bc8 to 6a4f404 Compare December 5, 2024 09:35

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 Dec 5, 2024

oshoval reviewed Dec 8, 2024

View reviewed changes

ashokpariya0 force-pushed the enable-multi-platform-linux-bridge branch 2 times, most recently from e6d21e2 to 3454f57 Compare December 8, 2024 17:38

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

0xFelix reviewed Dec 10, 2024

View reviewed changes

oshoval mentioned this pull request Dec 15, 2024

WIP check main branch state ovn-kubernetes/ovn-kubernetes#4914

Closed

5 tasks

ashokpariya0 force-pushed the enable-multi-platform-linux-bridge branch from e1c4a33 to c7b00fe Compare December 16, 2024 09:45

kubevirt-bot assigned oshoval Dec 16, 2024

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

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2024

kubevirt-bot merged commit d78b8c0 into kubevirt:main Dec 16, 2024
15 checks passed

		ARCH=$(uname -m \| sed 's/x86_64/amd64/')
		PLATFORMS="linux/amd64,linux/s390x"

Enable support for multiple platforms in the Linux Bridge CNI. #1927

Enable support for multiple platforms in the Linux Bridge CNI. #1927

Conversation

ashokpariya0 commented Nov 28, 2024

kubevirt-bot commented Nov 28, 2024

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

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 left a 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 10, 2024

kubevirt-bot commented Dec 10, 2024

0xFelix left a comment

Choose a reason for hiding this comment

oshoval commented Dec 10, 2024

ashokpariya0 commented Dec 10, 2024

0xFelix commented Dec 10, 2024

oshoval commented Dec 10, 2024 • edited Loading

oshoval commented Dec 10, 2024

ashokpariya0 commented Dec 10, 2024

oshoval commented Dec 10, 2024 • edited Loading

0xFelix commented Dec 10, 2024 • edited Loading

ashokpariya0 commented Dec 10, 2024

oshoval commented Dec 15, 2024 • edited Loading

oshoval commented Dec 15, 2024

ashokpariya0 commented Dec 15, 2024

oshoval commented Dec 15, 2024 • edited Loading

oshoval commented Dec 15, 2024 • edited Loading

oshoval commented Dec 15, 2024 • edited Loading

oshoval commented Dec 15, 2024 • edited Loading

oshoval commented Dec 15, 2024 • edited Loading

oshoval commented Dec 15, 2024

oshoval commented Dec 15, 2024

ashokpariya0 commented Dec 15, 2024

oshoval commented Dec 15, 2024 • edited Loading

ashokpariya0 commented Dec 15, 2024

jommeke22f commented Dec 15, 2024

ashokpariya0 commented Dec 16, 2024

oshoval commented Dec 16, 2024 • edited Loading

ashokpariya0 commented Dec 16, 2024

sonarqubecloud bot commented Dec 16, 2024

Quality Gate passed

ashokpariya0 commented Dec 16, 2024

ashokpariya0 commented Dec 16, 2024

oshoval commented Dec 16, 2024

oshoval commented Dec 16, 2024

oshoval commented Dec 16, 2024

oshoval Dec 1, 2024 •

edited

Loading

oshoval commented Dec 10, 2024 •

edited

Loading

oshoval commented Dec 10, 2024 •

edited

Loading

0xFelix commented Dec 10, 2024 •

edited

Loading

oshoval commented Dec 15, 2024 •

edited

Loading

oshoval commented Dec 15, 2024 •

edited

Loading

oshoval commented Dec 15, 2024 •

edited

Loading

oshoval commented Dec 15, 2024 •

edited

Loading

oshoval commented Dec 15, 2024 •

edited

Loading

oshoval commented Dec 15, 2024 •

edited

Loading

oshoval commented Dec 15, 2024 •

edited

Loading

oshoval commented Dec 16, 2024 •

edited

Loading