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

Push multi-arch images #314

Merged
merged 51 commits into from
May 5, 2023
Merged

Push multi-arch images #314

merged 51 commits into from
May 5, 2023

Conversation

ddelange
Copy link
Contributor

@ddelange ddelange commented Jan 16, 2023

Motivation

Multi-platform build for modelmesh-controller and modelmesh-controller-develop images; enable minikube deployments on Mac M1 laptops (#162, #231)

Modifications

  • remove amd64 arch-specific nodeAffinity for modelmesh controller (Quick start not working for Mac M1 laptops #162, 0/1 nodes are available: 1 node(s) didn't match Pod's node affinity/selector #231)
  • build for platforms linux/amd64, linux/arm64, linux/ppc64le, linux/s390x
  • use ubi8/go-toolset:1.18 for developer image instead of ubi8/ubi-minimal:8.7
  • build on pull_request as well as on push to catch build breaks during PR review
  • push developer images to DockerHub on PR merge, use local registry for PR builds
  • update deprecated checkout action
  • fix infinite docker inside docker error when running make run fmt inside the developer container
  • update build_devimage.sh script to only pull developer image if not already present
  • update develop.sh script to use the .develop_image_name

Related PRs

Related issues

Resolves #162
Resolves #231

Signed-off-by: ddelange <[email protected]>
Signed-off-by: ddelange <[email protected]>
@ckadner
Copy link
Member

ckadner commented Jan 21, 2023

I think we need to widen the scope of this change to ...

  • move to multi-stage docker build to instead of having two Dockerfiles (see current build error)
  • use ubi8/go-toolset as base image instead of ubi8/minimal to avoid complexities of building (or even just installing) go inside the Dockerfile

@ckadner
Copy link
Member

ckadner commented Feb 16, 2023

Hi @ddelange, are you still planing to work on the remaining multi-arch PRs? We had another request to support multiple (non-amd64) platforms since. What do you think about me pushing some updates to your PRs to address some of the outstanding work -- if you don't mind me becoming a co-author on your PRs.

@ddelange
Copy link
Contributor Author

Hi @ckadner feel free to take them over! I'm pretty side-tracked at work at the moment, but I'll be around, feel free to ping me!

@ckadner ckadner linked an issue Feb 25, 2023 that may be closed by this pull request
ckadner added 14 commits March 27, 2023 18:03
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
so it's available for controller image build later

Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
So the controller image build cannot use the developer image built
in the previous step

Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
@ckadner ckadner removed their request for review March 30, 2023 20:40
ckadner added 4 commits April 3, 2023 16:54
Signed-off-by: Christian Kadner <[email protected]>
To prevent error 'failed to parse platform ""' for
standard non-multi-platform docker builds.

Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
@ckadner
Copy link
Member

ckadner commented Apr 4, 2023

The optimization to check if the developer image exists (instead of always building it), shaves about 30 mins off the overall job completion time, but the arch-specific go build for the controller image take a horrendous 50 mins:

    #37 [linux/amd64 build 7/7] RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go
    #37 DONE 349.6s

    #43 [linux/ppc64le->amd64 build 7/7] RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go
    #43 DONE 2622.6s

    #44 [linux/arm64->amd64 build 7/7] RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go
    #44 DONE 2793.7s

    #26 [linux/s390x->amd64 build 7/7] RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go
    #26 DONE 3020.3s

I need to read up on how to optimize that.

Woohoo, the multi-platform build times are down to 13.5 minutes, or 8.5 minutes if the developer image exists.

ckadner added 4 commits April 3, 2023 18:49
The --mount option requires BuildKit, refer to
https://docs.docker.com/go/buildkit/

Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
@ckadner
Copy link
Member

ckadner commented Apr 11, 2023

... the arch-specific go build for the controller image takes a horrendous 50 mins:

    #37 [linux/amd64 build 7/7] RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go
    #37 DONE 349.6s

    #43 [linux/ppc64le->amd64 build 7/7] RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go
    #43 DONE 2622.6s

    #44 [linux/arm64->amd64 build 7/7] RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go
    #44 DONE 2793.7s

    #26 [linux/s390x->amd64 build 7/7] RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go
    #26 DONE 3020.3s

...

Thanks @tjohnson31415 for pointing out the error with setting "default" ARGs for TARGETOS and TARGETARCH in the Dockerfile which had the effect of building amd64 binaries for all platforms.

Updated, now we actually build target platform specific binaries (with comparable cross-compile build times):

#22 [linux/amd64->s390x build 7/7] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg     CGO_ENABLED=0 GOOS=linux GOARCH=s390x GO111MODULE=on go build -a -o manager main.go
#22 DONE 421.5s

#23 [linux/amd64->arm64 build 7/7] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg     CGO_ENABLED=0 GOOS=linux GOARCH=arm64 GO111MODULE=on go build -a -o manager main.go
#23 DONE 423.5s

#24 [linux/amd64 build 7/7] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg     CGO_ENABLED=0 GOOS=linux GOARCH=amd64 GO111MODULE=on go build -a -o manager main.go
#24 DONE 426.9s

#21 [linux/amd64->ppc64le build 7/7] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg     CGO_ENABLED=0 GOOS=linux GOARCH=ppc64le GO111MODULE=on go build -a -o manager main.go
#21 DONE 426.9s

Copy link
Contributor

@tjohnson31415 tjohnson31415 left a comment

Choose a reason for hiding this comment

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

LGTM

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddelange, tjohnson31415

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

@ckadner ckadner merged commit ceee2f0 into kserve:main May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants