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.

Sign up for GitHub

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

ci: Enable building for s390x and arm64 #73

Merged
merged 5 commits into from
Dec 3, 2024
+34 −11
Merged

ci: Enable building for s390x and arm64 #73

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac72953
ci: Enable building for s390x
jschintag Nov 28, 2024
36780e0
ci: Update workflow dependencies to latest version
jschintag Nov 28, 2024
71af676
Dockerfile: Fix lowercase as
jschintag Nov 28, 2024
d03ce8e
install-go.sh: Support installing golang on multiple arches
jschintag Nov 28, 2024
5a625eb
ci: Enable building for aarch64
jschintag Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions .github/workflows/image-push.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,19 @@ on:
env:
image-push-owner: 'kubevirt'
jobs:
push-amd64:
name: Image push/amd64
push:
name: Image push
runs-on: ubuntu-latest
steps:
- name: Check out code into the Go module directory
uses: actions/checkout@v2
uses: actions/checkout@v4

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
uses: docker/setup-buildx-action@v3

- name: Login to Container Registry
if: github.repository_owner == 'kubevirt'
uses: docker/login-action@v1
uses: docker/login-action@v3
with:
registry: ghcr.io
username: ${{ github.repository_owner }}
Expand All @@ -38,9 +38,10 @@ jobs:

- name: Push latest container image
if: github.repository_owner == 'kubevirt'
uses: docker/build-push-action@v2
uses: docker/build-push-action@v6
with:
context: .
platforms: linux/amd64,linux/s390x
Copy link
Collaborator

@oshoval oshoval Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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 have added it as a separate commit so it is easier to revert should it not work out.

push: true
tags: ghcr.io/${{ env.REPOSITORY_LC }}:latest
file: Dockerfile
Expand All @@ -49,9 +50,10 @@ jobs:

- name: Push tagged container image
if: startsWith(github.ref, 'refs/tags/')
uses: docker/build-push-action@v2
uses: docker/build-push-action@v6
with:
context: .
platforms: linux/amd64,linux/s390x
push: true
tags: ghcr.io/${{ env.REPOSITORY_LC }}:${{ github.ref_name }}
file: Dockerfile
Expand All @@ -63,7 +65,7 @@ jobs:
run: IMAGE=ghcr.io/${{ env.REPOSITORY_LC }}:${{ github.ref_name }} hack/update-manifest.sh

- name: Release the kraken
uses: softprops/action-gh-release@v1
uses: softprops/action-gh-release@v2
if: startsWith(github.ref, 'refs/tags/')
with:
generate_release_notes: true
Expand Down
6 changes: 4 additions & 2 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Build the manager binary
FROM docker.io/library/golang:1.19 as builder
FROM --platform=$BUILDPLATFORM docker.io/library/golang:1.19 AS builder

WORKDIR /workspace
# Copy the Go Modules manifests
Expand All @@ -13,8 +13,10 @@ RUN go mod download
COPY main.go main.go
COPY pkg/ pkg/

ARG TARGETARCH
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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


# 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
jschintag marked this conversation as resolved.
Show resolved Hide resolved

FROM registry.access.redhat.com/ubi8/ubi-minimal
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Nevermind, I see.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@oshoval oshoval Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@oshoval oshoval Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

WORKDIR /
Expand Down
21 changes: 20 additions & 1 deletion hack/install-go.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,26 @@

destination=$1
version=$(grep "^go " go.mod |awk '{print $2}')
tarball=go$version.linux-amd64.tar.gz

arch="$(uname -m)"

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
Comment on lines +8 to +22
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@oshoval oshoval Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

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

Copy link
Collaborator

@oshoval oshoval Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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


tarball=go$version.linux-$arch.tar.gz
url=https://dl.google.com/go/

mkdir -p $destination
Expand Down