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

S390x support #66

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

modassarrana89
Copy link

Multi-architecture support for model-registry

Description

Updated Dockerfile
Updated Makefile to use buildx for multi-arch image
Updated install_protoc scripts for multi-architecture

Removed Goarch

Signed-off-by: root <[email protected]>
use of buildx to build & push multi-architecture image

Signed-off-by: root <[email protected]>
Install protoc for multi-architecture

Signed-off-by: root <[email protected]>
@modassarrana89
Copy link
Author

@tarilabs I have updated all the changes as you have requested in old PR ( #62 )

@modassarrana89
Copy link
Author

This is the updated PR with all the comments suggested by @tarilabs in old PR #62

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

Thank you @modassarrana89 for a refreshed PR including most of the previous comments!

I've dded commit 8bcd792 as I believe necessary for the scripts/install_protoc.sh script to correctly resolve the zip to download when used in a ARM based linux container.

The problem I see in the way this PR currently stands, is that the model-registry is not actually cross-compiled:
Screenshot 2024-04-17 at 08 34 20 (2)

make image/buildx                  
# copy existing Dockerfile and insert --platform= into Dockerfile.cross, and preserve the original Dockerfile
sed -e '1 s/\(^FROM\)/FROM --platform=\$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$\{BUILDPLATFORM\}/' Dockerfile > Dockerfile.cross
docker buildx create --name project-v3-builder
project-v3-builder
docker buildx use project-v3-builder
docker buildx build --platform=linux/arm64,linux/amd64,linux/s390x,linux/ppc64le --tag kubeflow/model-registry:main -f Dockerfile.cross .
[+] Building 239.2s (36/36) FINISHED                                                                                                                                                                                                      docker-container:project-v3-builder
 => [internal] booting buildkit                                                                                                                                                                                                                                          2.1s
 => => pulling image moby/buildkit:buildx-stable-1                                                                                                                                                                                                                       1.8s
 => => creating container buildx_buildkit_project-v3-builder0                                                                                                                                                                                                            0.3s
 => [internal] load build definition from Dockerfile.cross                                                                                                                                                                                                               0.0s
 => => transferring dockerfile: 1.47kB                                                                                                                                                                                                                                   0.0s
 => [linux/ppc64le internal] load metadata for registry.access.redhat.com/ubi8/ubi-minimal:8.8                                                                                                                                                                           3.6s
 => [linux/arm64 internal] load metadata for registry.access.redhat.com/ubi8/ubi-minimal:8.8                                                                                                                                                                             3.6s
 => [linux/amd64 internal] load metadata for registry.access.redhat.com/ubi8/ubi-minimal:8.8                                                                                                                                                                             3.8s
 => [linux/s390x internal] load metadata for registry.access.redhat.com/ubi8/ubi-minimal:8.8                                                                                                                                                                             3.8s
 => [linux/arm64 internal] load metadata for registry.access.redhat.com/ubi8/go-toolset:1.19                                                                                                                                                                             4.0s
 => [internal] load .dockerignore                                                                                                                                                                                                                                        0.0s
 => => transferring context: 52B                                                                                                                                                                                                                                         0.0s
 => [internal] load build context                                                                                                                                                                                                                                        0.2s
 => => transferring context: 6.79MB                                                                                                                                                                                                                                      0.2s
 => [linux/arm64 builder  1/19] FROM registry.access.redhat.com/ubi8/go-toolset:1.19@sha256:dd16d34f7e4fae45a620ba8fdde378d0853314e4e0f4998658fd81628b36f0d6                                                                                                            98.4s
 => => resolve registry.access.redhat.com/ubi8/go-toolset:1.19@sha256:dd16d34f7e4fae45a620ba8fdde378d0853314e4e0f4998658fd81628b36f0d6                                                                                                                                   0.0s
 => => sha256:b953f5a6908d8aa51e2200fe3aa4b40102eb758be1d56e7dd3bec2a9cb85662e 162.55MB / 162.55MB                                                                                                                                                                      87.9s
 => => sha256:b8104c5f40fdcf0549bf9ad8b9ea4e5d3fabcc446f920cb976986f255e884a42 143.61MB / 143.61MB                                                                                                                                                                      66.5s
 => => sha256:8550ec08a06cf30baf540ef95dcc7334958355ef556b38eca4fd67cd68de5af1 18.17MB / 18.17MB                                                                                                                                                                        13.8s
 => => sha256:e1bb0572465a9e03d7af5024abb36d7227b5bf133c448b54656d908982127874 76.88MB / 76.88MB                                                                                                                                                                        41.7s
 => => extracting sha256:e1bb0572465a9e03d7af5024abb36d7227b5bf133c448b54656d908982127874                                                                                                                                                                                1.0s
 => => extracting sha256:8550ec08a06cf30baf540ef95dcc7334958355ef556b38eca4fd67cd68de5af1                                                                                                                                                                                0.2s
 => => extracting sha256:b8104c5f40fdcf0549bf9ad8b9ea4e5d3fabcc446f920cb976986f255e884a42                                                                                                                                                                                1.7s
 => => extracting sha256:b953f5a6908d8aa51e2200fe3aa4b40102eb758be1d56e7dd3bec2a9cb85662e                                                                                                                                                                                1.6s
 => [linux/ppc64le stage-1 1/3] FROM registry.access.redhat.com/ubi8/ubi-minimal:8.8@sha256:b93deceb59a58588d5b16429fc47f98920f84740a1f2ed6454e33275f0701b59                                                                                                            27.2s
 => => resolve registry.access.redhat.com/ubi8/ubi-minimal:8.8@sha256:b93deceb59a58588d5b16429fc47f98920f84740a1f2ed6454e33275f0701b59                                                                                                                                   0.0s
 => => sha256:655f24e7656ea1c1a2711179ea0c6d6f7dac72ba7d2d3fd34c36d062f00f74d8 44.03MB / 44.03MB                                                                                                                                                                        26.7s
 => => extracting sha256:655f24e7656ea1c1a2711179ea0c6d6f7dac72ba7d2d3fd34c36d062f00f74d8                                                                                                                                                                                0.4s
 => [linux/arm64 stage-1 1/3] FROM registry.access.redhat.com/ubi8/ubi-minimal:8.8@sha256:b93deceb59a58588d5b16429fc47f98920f84740a1f2ed6454e33275f0701b59                                                                                                              38.1s
 => => resolve registry.access.redhat.com/ubi8/ubi-minimal:8.8@sha256:b93deceb59a58588d5b16429fc47f98920f84740a1f2ed6454e33275f0701b59                                                                                                                                   0.0s
 => => sha256:4da493486a8e6bed896d94989fd40199cacab25cef2022dc5b72dc9b0be61387 37.69MB / 37.69MB                                                                                                                                                                        37.7s
 => => extracting sha256:4da493486a8e6bed896d94989fd40199cacab25cef2022dc5b72dc9b0be61387                                                                                                                                                                                0.4s
 => [linux/amd64 stage-1 1/3] FROM registry.access.redhat.com/ubi8/ubi-minimal:8.8@sha256:b93deceb59a58588d5b16429fc47f98920f84740a1f2ed6454e33275f0701b59                                                                                                              36.9s
 => => resolve registry.access.redhat.com/ubi8/ubi-minimal:8.8@sha256:b93deceb59a58588d5b16429fc47f98920f84740a1f2ed6454e33275f0701b59                                                                                                                                   0.0s
 => => sha256:dc35b837139a95d1b9f7f7b0435a024a74ab972416bdc248f3f608c9f917a753 39.31MB / 39.31MB                                                                                                                                                                        36.5s
 => => extracting sha256:dc35b837139a95d1b9f7f7b0435a024a74ab972416bdc248f3f608c9f917a753                                                                                                                                                                                0.4s
 => [linux/s390x stage-1 1/3] FROM registry.access.redhat.com/ubi8/ubi-minimal:8.8@sha256:b93deceb59a58588d5b16429fc47f98920f84740a1f2ed6454e33275f0701b59                                                                                                               9.3s
 => => resolve registry.access.redhat.com/ubi8/ubi-minimal:8.8@sha256:b93deceb59a58588d5b16429fc47f98920f84740a1f2ed6454e33275f0701b59                                                                                                                                   0.0s
 => => sha256:8737e1657f05079caa8c726fb829e674f98ab5a3dd612e1bbd5a3b46b095e360 37.55MB / 37.55MB                                                                                                                                                                         8.9s
 => => extracting sha256:8737e1657f05079caa8c726fb829e674f98ab5a3dd612e1bbd5a3b46b095e360                                                                                                                                                                                0.4s
 => [linux/arm64 builder  2/19] WORKDIR /workspace                                                                                                                                                                                                                       0.3s
 => [linux/arm64 builder  3/19] COPY [go.mod, go.sum, ./]                                                                                                                                                                                                                0.0s
 => [linux/arm64 builder  4/19] RUN go mod download                                                                                                                                                                                                                     17.2s
 => [linux/arm64 builder  5/19] RUN yum remove -y nodejs npm                                                                                                                                                                                                             0.6s
 => [linux/arm64 builder  6/19] RUN yum module -y reset nodejs                                                                                                                                                                                                           2.7s
 => [linux/arm64 builder  7/19] RUN yum module -y enable nodejs:18                                                                                                                                                                                                       0.4s 
 => [linux/arm64 builder  8/19] RUN yum install -y nodejs npm java-11                                                                                                                                                                                                   30.8s 
 => [linux/arm64 builder  9/19] COPY [Makefile, main.go, .openapi-generator-ignore, openapitools.json, ./]                                                                                                                                                               0.1s 
 => [linux/arm64 builder 10/19] COPY .git/ .git/                                                                                                                                                                                                                         0.1s 
 => [linux/arm64 builder 11/19] COPY cmd/ cmd/                                                                                                                                                                                                                           0.0s 
 => [linux/arm64 builder 12/19] COPY api/ api/                                                                                                                                                                                                                           0.0s 
 => [linux/arm64 builder 13/19] COPY internal/ internal/                                                                                                                                                                                                                 0.0s 
 => [linux/arm64 builder 14/19] COPY scripts/ scripts/                                                                                                                                                                                                                   0.0s 
 => [linux/arm64 builder 15/19] COPY pkg/ pkg/                                                                                                                                                                                                                           0.0s
 => [linux/arm64 builder 16/19] COPY patches/ patches/                                                                                                                                                                                                                   0.0s
 => [linux/arm64 builder 17/19] COPY templates/ templates/                                                                                                                                                                                                               0.0s
 => [linux/arm64 builder 18/19] RUN make deps                                                                                                                                                                                                                           55.0s
 => [linux/arm64 builder 19/19] RUN CGO_ENABLED=1 GOOS=linux make clean model-registry                                                                                                                                                                                  27.1s 
 => [linux/s390x stage-1 2/3] COPY --from=builder /workspace/model-registry .                                                                                                                                                                                            0.0s 
 => [linux/amd64 stage-1 2/3] COPY --from=builder /workspace/model-registry .                                                                                                                                                                                            0.0s 
 => [linux/ppc64le stage-1 2/3] COPY --from=builder /workspace/model-registry .                                                                                                                                                                                          0.0s 
 => [linux/arm64 stage-1 2/3] COPY --from=builder /workspace/model-registry .                                                                                                                                                                                            0.0s 
WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load                                                                            
docker buildx rm project-v3-builder                                                                                                                                                                                                                                           
project-v3-builder removed
rm Dockerfile.cross

In other words: it looks to me model registry get compiled for the platform the build is currently on (in my screenshot, arm), and then the binary copied to linux/arm64,linux/amd64,linux/s390x,linux/ppc64le, which is unlikely to work.

Further, it looks to me cross-compilation would require something ~like this in the dockerfile:

ARG TARGETOS TARGETARCH
RUN CGO_ENABLED=1 GOOS=$TARGETOS GOARCH=$TARGETARCH make clean model-registry

but currently would fail with:

go: cannot install cross-compiled binaries when GOBIN is set

which is used as part of the Makefile.

I believe solutions could be along the lines of:

  • rework the build to avoid gobin IFF all the remainder buildx problems are solved after accounting for the above
  • use buildx for emulation rather than cross-compilation
  • other solution?

I believe a proper reproducible cross-platform build is needed, advise if you believe I might have missed on anything and how planning to solve for the mentioned issues.

@modassarrana89
Copy link
Author

Thank you @modassarrana89 for a refreshed PR including most of the previous comments!

I've dded commit 8bcd792 as I believe necessary for the scripts/install_protoc.sh script to correctly resolve the zip to download when used in a ARM based linux container.

The problem I see in the way this PR currently stands, is that the model-registry is not actually cross-compiled: Screenshot 2024-04-17 at 08 34 20 (2)

In other words: it looks to me model registry get compiled for the platform the build is currently on (in my screenshot, arm), and then the binary copied to linux/arm64,linux/amd64,linux/s390x,linux/ppc64le, which is unlikely to work.

Further, it looks to me cross-compilation would require something ~like this in the dockerfile:

ARG TARGETOS TARGETARCH
RUN CGO_ENABLED=1 GOOS=$TARGETOS GOARCH=$TARGETARCH make clean model-registry

but currently would fail with:

go: cannot install cross-compiled binaries when GOBIN is set

which is used as part of the Makefile.

I believe solutions could be along the lines of:

  • rework the build to avoid gobin IFF all the remainder buildx problems are solved after accounting for the above
  • use buildx for emulation rather than cross-compilation
  • other solution?

I believe a proper reproducible cross-platform build is needed, advise if you believe I might have missed on anything and how planning to solve for the mentioned issues.

Let me quickly test , once & get back on this

Added TargetOS and TARGETARCH
@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Apr 17, 2024
@modassarrana89
Copy link
Author

Thank you @modassarrana89 for a refreshed PR including most of the previous comments!
I've dded commit 8bcd792 as I believe necessary for the scripts/install_protoc.sh script to correctly resolve the zip to download when used in a ARM based linux container.
The problem I see in the way this PR currently stands, is that the model-registry is not actually cross-compiled: Screenshot 2024-04-17 at 08 34 20 (2)
In other words: it looks to me model registry get compiled for the platform the build is currently on (in my screenshot, arm), and then the binary copied to linux/arm64,linux/amd64,linux/s390x,linux/ppc64le, which is unlikely to work.
Further, it looks to me cross-compilation would require something ~like this in the dockerfile:

ARG TARGETOS TARGETARCH
RUN CGO_ENABLED=1 GOOS=$TARGETOS GOARCH=$TARGETARCH make clean model-registry

but currently would fail with:

go: cannot install cross-compiled binaries when GOBIN is set

which is used as part of the Makefile.
I believe solutions could be along the lines of:

  • rework the build to avoid gobin IFF all the remainder buildx problems are solved after accounting for the above
  • use buildx for emulation rather than cross-compilation
  • other solution?

I believe a proper reproducible cross-platform build is needed, advise if you believe I might have missed on anything and how planning to solve for the mentioned issues.

Let me quickly test , once & get back on this

Below Github action works fine for building & pushing multi - arch image

- name: Build and Push Image
        uses: docker/build-push-action@v3
        with:
          tags: ${{ env.IMAGE_NAME }}:${{ env.TAG }}
          platforms: linux/amd64,linux/s390x,linux/ppc64le
          push: true

If ok , we can update the github workflow for build and push to use this action & then it will automatically solves our problem.

@tarilabs
Copy link
Member

Not seeing / nor understood how the new commit would solve for:

go: cannot install cross-compiled binaries when GOBIN is set

dhirajsb added a commit to dhirajsb/model-registry-kfp that referenced this pull request May 22, 2024
* Refactor openapi definition

* Add name query parameter for model_version url, fix Artifact oneOf definition

* Added sortOrder query param

* Fix allOf composites for BaseArtifactUpdate and ModelArtifactUpdate

* Add model-serving metadata resources, openapi-generator-cli, and generated model, server, and proxy cmd

* Fix allOf types for InferenceServiceCreate and InferenceServiceUpdate
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@modassarrana89
Copy link
Author

@tarilabs Below Github action should works fine for building & pushing multi - arch image. Need to verify once . We can use this inplace of the current scripts for multi-arch image build procedure . If you guys are. fine with Github workflow change , I can quickly verify this approach & let you know the update.

- name: Build and Push Image
   uses: docker/build-push-action@v3
   with:
          tags: ${{ env.IMAGE_NAME }}:${{ env.TAG }}
          platforms: linux/amd64,linux/s390x,linux/ppc64le
          push: true

Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andreyvelich for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@tarilabs
Copy link
Member

If you guys are. fine with Github workflow change , I can quickly verify this approach & let you know the update

We are looking into removing the GOBIN and local build toolchain, so to support more general multi-arch support, per KF 1.10 roadmap: #175

We can keep this PR open, to revise this as we will rampup on those items.

@modassarrana89
Copy link
Author

If you guys are. fine with Github workflow change , I can quickly verify this approach & let you know the update

We are looking into removing the GOBIN and local build toolchain, so to support more general multi-arch support, per KF 1.10 roadmap: #175

We can keep this PR open, to revise this as we will rampup on those items.

Ok Great. But may i know whats the deadline would be

@modassarrana89
Copy link
Author

@tarilabs it would be great if you can share some update on GOBIN removal and local build toolchain.

@tarilabs
Copy link
Member

tarilabs commented Aug 29, 2024

@tarilabs it would be great if you can share some update on GOBIN removal and local build toolchain.

Sure!
We found in general gobin usage and cgo usage giving us some troubles while attempting to instruct for cross-compilation.

There is also another observation that generated source code are re-generated in the docker build phase itself, which is less ideal especially considering we have them already under VCS here in this git repo, in order to point to the actual sources for the bin/image we ship.

We have some contributors actively working on this front for which we can expect some further update, per PRs already iterated on, or being currently worked on right now.

edit: with more recent changes, we just discovered gobin looks currently not an impediment to crosscompilation, so we're now looking to keep it (hence not drop its current usage). More updates to follow.

@modassarrana89
Copy link
Author

source code are re-generated in the docker build phase itself

we have discussed previously that cross compilation is creating problem with current GOBIN setup. If GOBIN is not an issue , are you saying that cross-compilation will work

@tarilabs
Copy link
Member

tarilabs commented Sep 5, 2024

we have discussed previously that cross compilation is creating problem with current GOBIN setup. If GOBIN is not an issue , are you saying that cross-compilation will work

as mentioned in #66 (comment), gobin looks currently not an impediment to crosscompilation, so we're now looking to keep (gobin usage)

more updates to follow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants