-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added s390x and ppc64le support #10766
base: master
Are you sure you want to change the base?
Conversation
Hi @R3hankhan123. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/retest
/retest |
New changes are detected. LGTM label has been removed. |
@R3hankhan123 We have an known issue with the current e2e tests so for now we'll need to run them locally to ensure the PR does not introduce any breaking changes. |
@R3hankhan123 the e2e test was migrated to GitHub Actions and is working now. Can you please rebase? /reopen |
@hbelmiro: Reopened this PR. In response to this:
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. |
/reopen |
@dilipgb: You can't reopen an issue/PR unless you authored it or you are a collaborator. In response to this:
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. |
/reopen |
@R3hankhan123: Failed to re-open PR: state cannot be changed. There are no new commits on the R3hankhan123:s390x branch. In response to this:
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. |
Signed-off-by: Rehan Khan <[email protected]>
/reopen |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
@hbelmiro looks like the workflows still arent being triggered automatically |
Signed-off-by: Rehan Khan <[email protected]>
/ok-to-test |
@hbelmiro looks like all the tests have passed and just needs the lgtm label |
@@ -1,11 +1,17 @@ | |||
FROM gcr.io/google.com/cloudsdktool/google-cloud-cli:alpine | |||
FROM alpine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hbelmiro gcr.io/google.com/cloudsdktool/google-cloud-cli:alpine image is not supporting multiple architecture. It only amd/arm architectures.
Since gcloud component installed is only kubectl (https://github.com/kubeflow/pipelines/blob/master/backend/src/cache/deployer/Dockerfile#L8) we are taking plain vanila alpine and installing all the needed packages including kubectl from binary distributions as you see in below code snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Rehan Khan <[email protected]>
/ok-to-test |
@hbelmiro can we list this as one of the topic to discuss in coming community meeting? |
@R3hankhan123 Sure. |
@@ -23,7 +23,7 @@ RUN sh /third_party/download_source.sh </third_party/minio/repo-MPL.txt | |||
|
|||
|
|||
# Minio image | |||
FROM minio/minio:RELEASE.2019-08-14T20-37-41Z | |||
FROM minio/minio:RELEASE.2020-12-18T03-27-42Z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this required for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HumairAK the image used here is published few years back. We enabled the s390x and ppc64le builds later after this image being build. Hence we are requesting to update this. You can see from tag, the image used is released in 2019 and we are having image supported from 2020.
@@ -66,25 +67,25 @@ image_all: image_apiserver image_persistence_agent image_cache image_swf image_v | |||
|
|||
.PHONY: image_apiserver | |||
image_apiserver: | |||
cd $(MOD_ROOT) && ${CONTAINER_ENGINE} build -t ${IMG_TAG_APISERVER} -f backend/Dockerfile . | |||
cd $(MOD_ROOT) && ${CONTAINER_ENGINE} buildx build --platform $(PLATFORMS) -t ${IMG_TAG_APISERVER} -f backend/Dockerfile . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I think this should require a KFP community call topic & consideration
this looks to me like it's essentially increasing the support level of KFP to now support all of these architectures, and we should understand what the overhead of this is
cc @chensun / @zijianjoy do you have any thoughts here?
/hold due to: #10766 (comment) |
Hi @HumairAK , can you add multi arch discussion in the doc for the upcoming community meet |
@@ -4,6 +4,7 @@ CSV_PATH=backend/third_party_licenses | |||
|
|||
# Container Build Params | |||
CONTAINER_ENGINE ?= docker | |||
PLATFORMS ?= linux/amd64,linux/s390x,linux/ppc64le |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PLATFORMS ?= linux/amd64,linux/s390x,linux/ppc64le | |
PLATFORMS ?= linux/amd64 |
Can we remove s390x and ppc64le from this PR? So this PR is focused on just enabling the ability to build other architectures with minimal effort. We can revisit the "build new architectures by default" topic in future. @HumairAK @gregsheremeta thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anishasthana we can have another rule to build multiarch build and add build platform there for cross platform builds and we can use current rule for native build (This is something other communities like contour uses for multiarch builds). code may looks something like this.
# Platforms to build the multi-arch image for.
IMAGE_PLATFORMS ?= linux/amd64,linux/s390x,linux/ppc64le
multiarch-build:
@mkdir -p $(shell pwd)/image
docker buildx build $(IMAGE_RESULT_FLAG) \
--platform $(IMAGE_PLATFORMS) \
--build-arg "BUILD_GOPRIVATE=$(BUILD_GOPRIVATE)" \
--build-arg "BUILD_GOPROXY=$(BUILD_GOPROXY)" \
--build-arg "BUILD_GOSUMDB=$(BUILD_GOSUMDB)" \
--build-arg "BUILD_BASE_IMAGE=$(BUILD_BASE_IMAGE)" \
--build-arg "BUILD_VERSION=$(BUILD_VERSION)" \
--build-arg "BUILD_BRANCH=$(BUILD_BRANCH)" \
--build-arg "BUILD_SHA=$(BUILD_SHA)" \
--build-arg "BUILD_CGO_ENABLED=$(BUILD_CGO_ENABLED)" \
--build-arg "BUILD_EXTRA_GO_LDFLAGS=$(BUILD_EXTRA_GO_LDFLAGS)" \
--build-arg "BUILD_GOEXPERIMENT=$(BUILD_GOEXPERIMENT)" \
$(DOCKER_BUILD_LABELS) \
$(IMAGE_TAGS) \
$(shell pwd)
build:
docker build \
--build-arg "BUILD_GOPRIVATE=$(BUILD_GOPRIVATE)" \
--build-arg "BUILD_GOPROXY=$(BUILD_GOPROXY)" \
--build-arg "BUILD_GOSUMDB=$(BUILD_GOSUMDB)" \
--build-arg "BUILD_BASE_IMAGE=$(BUILD_BASE_IMAGE)" \
--build-arg "BUILD_VERSION=$(BUILD_VERSION)" \
--build-arg "BUILD_BRANCH=$(BUILD_BRANCH)" \
--build-arg "BUILD_SHA=$(BUILD_SHA)" \
--build-arg "BUILD_CGO_ENABLED=$(BUILD_CGO_ENABLED)" \
--build-arg "BUILD_EXTRA_GO_LDFLAGS=$(BUILD_EXTRA_GO_LDFLAGS)" \
--build-arg "BUILD_GOEXPERIMENT=$(BUILD_GOEXPERIMENT)" \
$(DOCKER_BUILD_LABELS) \
$(shell pwd) \
--tag $(IMAGE):$(VERSION)
So primary architecture which is x86/native platform here don't have build the multiarch image for every time.
Description of your changes:
Added multiarch support for s390x and ppc64le by adding Architectural parameters wherever possible in dockerfiles and bash scripts
Checklist: