ci: Enable building for s390x and arm64 #73

jschintag · 2024-11-29T08:04:00Z

What this PR does / why we need it:
This PR enables building kubesecondarydns for s390x in addition to amd64.
Additionally arm64 is enabled as well, though i can't test it to verify it works.
Additionally it bumps the workflow action versions to their latest to get rid of multiple deprecation warnings.
Finally it fixes a small warning about the case of "AS" in the Dockerfile.

Special notes for your reviewer:
PRs to enable unit-tests and build test for s390x will follow. E2E needs the provider, which for s390x still has some problems to be worked out.

Dockerfile

oshoval · 2024-12-01T13:34:08Z

Once all the comments are ref please let me know and i will review
Thank you all for your contribution

Change the Dockerfile to support fast multi-arch builds using cross-compilation. Do not hardcode the target arch as amd64. Signed-off-by: Jan Schintag <[email protected]>

Signed-off-by: Jan Schintag <[email protected]>

The ci outputs a warning since both FROM and AS keywords should be all uppercase. Signed-off-by: Jan Schintag <[email protected]>

Add support for installing golang on arm64 and s390x. Signed-off-by: Jan Schintag <[email protected]>

jschintag · 2024-12-02T09:25:01Z

@oshoval I have addressed the comments and added the changes from #74 to this PR, as requested in #74 (comment)

oshoval · 2024-12-02T14:15:40Z

.github/workflows/image-push.yaml

@@ -41,6 +41,7 @@ jobs:
        uses: docker/build-push-action@v2
        with:
          context: .
+          platforms: linux/amd64,linux/s390x


Do we want to add arm please ? (as you added on install go script)
(assuming git actions support arm cross compile)

if you prefer it can wait for follow-ups, unless it is straight forward
(according other PRs i saw, it seems straight forward and supported)

Sure i can add it. I just haven't tested it, so if it works or not i can't say.

I have added it as a separate commit so it is easier to revert should it not work out.

Signed-off-by: Jan Schintag <[email protected]>

oshoval · 2024-12-03T09:38:25Z

Dockerfile

@@ -13,8 +13,10 @@ RUN go mod download
 COPY main.go main.go
 COPY pkg/ pkg/

+ARG TARGETARCH


dont we need something like this ?
https://github.com/kubevirt/bridge-marker/pull/77/files#diff-5aecd73bbcb9b7410938673269f0d4e676dd18509277b0ff2f2787c9d3e22604R3-R11

Not really, we only need TARGETARCH and ARG already sets it as an enviroment variable.

Thanks

@ashokpariya0 does it mean maybe that the logic on bridge-marker or whatever projects that done the same as it can be simplified maybe?

@oshoval No, it's important to handle default cases. The combination of ARG and ENV is beneficial because it allows you to manage both build-time and runtime configurations more flexibly and efficiently.

ashokpariya0 · 2024-12-03T10:04:51Z

hack/install-go.sh

+case $arch in
+    x86_64 | amd64)
+        arch="amd64"
+        ;;
+    aarch64 | arm64)
+        arch="arm64"
+        ;;
+    s390x)
+        arch="s390x"
+        ;;
+    *)
+        echo "ERROR: invalid arch=${arch}, only support x86_64, aarch64 and s390x"
+        exit 1
+        ;;
+esac


Can we modify it like below? Instead of restricting the architecture to only support x86_64, aarch64, and s390x, it would be helpful to allow building for other architectures, such as ppc64le, using the make command.
arch=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/')

lets see please if we do need it imo, because there wasnt an item to support ppc and i prefer to not do just parts unless needed

on the other hand, the suggestion is indeed more compact and robust, so it will be fine to adapt this
but the rest shouldn't be adapted yet

thanks

EDIT - for more context
kubevirt/bridge-marker#77 (comment)

I actually prefer the more verbose version, since it is more easy to read later on and to see what is intended to work.

My proposal would be i keep the switch-case, but change the default to a warning instead of an error.

i do understand it is more verbose, but it is good to be in the same format on the repos we maintain
anyhow i wont insist, we can change later according needs,
(i also tend to like compactness personally)

we do need to error imo if one supplied unsupported version in case we keep it this way

Thanks

oshoval · 2024-12-03T10:46:23Z

/lgtm

Thanks,
Please add arm support to PR desc

oshoval · 2024-12-03T10:49:31Z

/lgtm cancel
/approve

we need another reviewer

kubevirt-bot · 2024-12-03T10:49:46Z

[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

oshoval · 2024-12-03T11:23:41Z

@0xFelix
can you please take a look ?

jschintag · 2024-12-03T12:14:14Z

Thanks, Please add arm support to PR desc

done

0xFelix

Did you verify this by running it against a test registry?

0xFelix · 2024-12-03T12:39:22Z

Dockerfile

 # Build
-RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o manager main.go
+RUN CGO_ENABLED=0 GOOS=linux GOARCH="${TARGETARCH}" go build -a -o manager main.go

 FROM registry.access.redhat.com/ubi8/ubi-minimal


Which image is pulled here? Do the resulting image arch and the BUILDPLATFORM match?

I have tested the whole workflow on my own fork against ghcr.io.
The BUILDPLATFORM means that the first stage is run natively, e.g. on amd64, while the TARGETARCH may be s390x.
The 2nd stage is not affected, so if you target s390x, you compile the s390x binary on amd64 via cross-compile but still get a s390x image in the 2nd stage.

If you set --platform in the first stage, it will be used for the second stage as well. Is that correct?

https://docs.docker.com/build/building/multi-platform/#cross-compilation

Nevermind, I see.

I believe we need to update the following:
FROM registry.access.redhat.com/ubi8/ubi-minimal
to:
FROM --platform=linux/${TARGETARCH} registry.access.redhat.com/ubi8/ubi-minimal
This is necessary because FROM registry.access.redhat.com/ubi8/ubi-minimal works fine with docker buildx, but it fails with podman when building for multiple platforms.

i am not familiar about it by heart tbh
maybe lets wait please from ideas from the people involved

curious, iiuc it works without it on all the required combinations ?
native and s390x (cross / native) are most important, on podman (because this what CNV suggests)
to see it builds and runs on those combinations

I have tested native with docker/podman by running make build on s390x. Let me test it on x86 as well just to be sure, though there should not be a difference. Cross as i said seemed to not work for me with OCI: podman, host: s390x, target: amd64

Native amd64 -> amd64 works as well with docker/podman.

host: s390x, target: amd64

we can fix this one later specifically according needs, because atm native host/target amd64 works which is what important in this aspect

host: amd64, target: s390x - does work ? (podman / docker, higher priority for podman)

maybe the PRs of @ashokpariya0 might help in this, if it works there

Thanks

The combination of host: s390x and target: amd64 is not supported, so there is no need to test this configuration.
As @oshoval pointed out we need to test following:

Host: amd64
Target: amd64, arm64, and s390x.

Also For the final stage of the build, better to use the following:
FROM --platform=linux/${TARGETARCH} registry.access.redhat.com/ubi8/ubi-minimal
This approach is necessary when building multi-platform images using Podman. You will need to create a Podman manifest and then add the individual platform-specific builds one by one to the manifest. Also adding (linux/${TARGETARCH}) works well with docker buildx.

0xFelix

/lgtm

oshoval · 2024-12-05T14:17:20Z

once we are good to create a release please let me know
thanks

jschintag · 2024-12-06T08:40:39Z

@oshoval I have created #78 to address the issue with podman cross-build on amd64. Once that is merged, from my side that should be everything we need for a release of kubesecondarydns.

@ashokpariya0 WDYT?

ashokpariya0 · 2024-12-06T08:59:51Z

@oshoval I have created #78 to address the issue with podman cross-build on amd64. Once that is merged, from my side that should be everything we need for a release of kubesecondarydns.

@ashokpariya0 WDYT?

@jschintag yes sounds good.
@oshoval Looks good to me! Once #78 is merged, we should be all set for the release of kubesecondarydns.

oshoval · 2024-12-08T07:00:15Z

Created v0.0.16
https://github.com/kubevirt/kubesecondarydns/releases/tag/v0.0.16
Please check it works as expected if possible
Thanks all

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Nov 29, 2024

kubevirt-bot requested review from AlonaKaplan and oshoval November 29, 2024 08:04

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

ashokpariya0 reviewed Nov 29, 2024

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

ashokpariya0 reviewed Nov 29, 2024

View reviewed changes

Dockerfile Show resolved Hide resolved

This was referenced Dec 1, 2024

install-go.sh: Support installing golang on multiple arches #74

Closed

Enable multi-architecture support, specifically for s390x, in CNAO and the associated CNIs. kubevirt/cluster-network-addons-operator#1916

Open

jschintag added 4 commits December 2, 2024 10:21

ci: Enable building for s390x

ac72953

Change the Dockerfile to support fast multi-arch builds using cross-compilation. Do not hardcode the target arch as amd64. Signed-off-by: Jan Schintag <[email protected]>

ci: Update workflow dependencies to latest version

36780e0

Signed-off-by: Jan Schintag <[email protected]>

Dockerfile: Fix lowercase as

71af676

The ci outputs a warning since both FROM and AS keywords should be all uppercase. Signed-off-by: Jan Schintag <[email protected]>

install-go.sh: Support installing golang on multiple arches

d03ce8e

Add support for installing golang on arm64 and s390x. Signed-off-by: Jan Schintag <[email protected]>

jschintag force-pushed the s390x-build branch from eed4043 to d03ce8e Compare December 2, 2024 09:22

kubevirt-bot added size/M and removed size/S labels Dec 2, 2024

oshoval reviewed Dec 2, 2024

View reviewed changes

ci: Enable building for aarch64

5a625eb

Signed-off-by: Jan Schintag <[email protected]>

oshoval reviewed Dec 3, 2024

View reviewed changes

ashokpariya0 reviewed Dec 3, 2024

View reviewed changes

kubevirt-bot assigned oshoval Dec 3, 2024

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

jschintag changed the title ~~ci: Enable building for s390x~~ ci: Enable building for s390x and arm64 Dec 3, 2024

0xFelix reviewed Dec 3, 2024

View reviewed changes

0xFelix approved these changes Dec 3, 2024

View reviewed changes

kubevirt-bot assigned 0xFelix Dec 3, 2024

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

kubevirt-bot merged commit c2d9ba6 into kubevirt:main Dec 3, 2024
8 checks passed

jschintag deleted the s390x-build branch December 3, 2024 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Enable building for s390x and arm64 #73

ci: Enable building for s390x and arm64 #73

jschintag commented Nov 29, 2024 •

edited

Loading

oshoval commented Dec 1, 2024

jschintag commented Dec 2, 2024

oshoval Dec 2, 2024 •

edited

Loading

jschintag Dec 2, 2024

jschintag Dec 2, 2024

oshoval Dec 3, 2024

jschintag Dec 3, 2024

oshoval Dec 3, 2024

ashokpariya0 Dec 3, 2024

ashokpariya0 Dec 3, 2024

oshoval Dec 3, 2024 •

edited

Loading

jschintag Dec 3, 2024

oshoval Dec 3, 2024 •

edited

Loading

oshoval commented Dec 3, 2024

oshoval commented Dec 3, 2024

kubevirt-bot commented Dec 3, 2024

oshoval commented Dec 3, 2024

jschintag commented Dec 3, 2024

0xFelix left a comment

0xFelix Dec 3, 2024

jschintag Dec 3, 2024

0xFelix Dec 3, 2024

0xFelix Dec 3, 2024

ashokpariya0 Dec 4, 2024

oshoval Dec 5, 2024 •

edited

Loading

jschintag Dec 5, 2024

jschintag Dec 5, 2024

oshoval Dec 5, 2024 •

edited

Loading

ashokpariya0 Dec 5, 2024

0xFelix left a comment

oshoval commented Dec 5, 2024

jschintag commented Dec 6, 2024

ashokpariya0 commented Dec 6, 2024

oshoval commented Dec 8, 2024 •

edited

Loading

ci: Enable building for s390x and arm64 #73

ci: Enable building for s390x and arm64 #73

Conversation

jschintag commented Nov 29, 2024 • edited Loading

oshoval commented Dec 1, 2024

jschintag commented Dec 2, 2024

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

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

oshoval Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

oshoval commented Dec 3, 2024

oshoval commented Dec 3, 2024

kubevirt-bot commented Dec 3, 2024

oshoval commented Dec 3, 2024

jschintag commented Dec 3, 2024

0xFelix 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 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xFelix left a comment

Choose a reason for hiding this comment

oshoval commented Dec 5, 2024

jschintag commented Dec 6, 2024

ashokpariya0 commented Dec 6, 2024

oshoval commented Dec 8, 2024 • edited Loading

jschintag commented Nov 29, 2024 •

edited

Loading

oshoval Dec 2, 2024 •

edited

Loading

oshoval Dec 3, 2024 •

edited

Loading

oshoval Dec 3, 2024 •

edited

Loading

oshoval Dec 5, 2024 •

edited

Loading

oshoval Dec 5, 2024 •

edited

Loading

oshoval commented Dec 8, 2024 •

edited

Loading