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

Jump to bottom

Added s390x and ppc64le support #10766

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added s390x and ppc64le support #10766

wants to merge 3 commits into from
+72 −21

Conversation

R3hankhan123
Copy link

Description of your changes:
Added multiarch support for s390x and ppc64le by adding Architectural parameters wherever possible in dockerfiles and bash scripts
Checklist:

@google-oss-prow Google OSS Prow
Copy link

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@hbelmiro
Copy link
Contributor

/ok-to-test

hbelmiro
hbelmiro reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@hbelmiro hbelmiro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/retest

@google-oss-prow google-oss-prow bot added the lgtm label Apr 30, 2024
@hbelmiro
Copy link
Contributor

/retest

@google-oss-prow google-oss-prow bot removed the lgtm label May 1, 2024
@google-oss-prow Google OSS Prow
Copy link

New changes are detected. LGTM label has been removed.

@R3hankhan123
Copy link
Author

R3hankhan123 commented May 2, 2024 •
edited
Loading

@hbelmiro @chensun are the test failing because Kubernetes 1.25 is no longer supported?

@R3hankhan123 R3hankhan123 requested a review from hbelmiro May 2, 2024 05:41
@rimolive
Copy link
Member

rimolive commented May 4, 2024

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

@github-actions github-actions bot added the Stale label Jul 4, 2024
@github-actions github-actions bot closed this Jul 25, 2024
@hbelmiro
Copy link
Contributor

@R3hankhan123 the e2e test was migrated to GitHub Actions and is working now. Can you please rebase?

/reopen

@google-oss-prow google-oss-prow bot reopened this Jul 25, 2024
@google-oss-prow Google OSS Prow
Copy link

@hbelmiro: Reopened this PR.

In response to this:

@R3hankhan123 the e2e test was migrated to GitHub Actions and is working now. Can you please rebase?

/reopen

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.

@google-oss-prow google-oss-prow bot added size/XS and removed size/M labels Jul 26, 2024
@dilipgb
Copy link

dilipgb commented Jul 26, 2024

/reopen

@google-oss-prow Google OSS Prow
Copy link

@dilipgb: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

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.

@R3hankhan123
Copy link
Author

/reopen

@google-oss-prow Google OSS Prow
Copy link

@R3hankhan123: Failed to re-open PR: state cannot be changed. There are no new commits on the R3hankhan123:s390x branch.

In response to this:

/reopen

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.

@google-oss-prow google-oss-prow bot added size/XXL and removed size/M labels Sep 20, 2024
@R3hankhan123
Added s390x and ppc64le support after rebasing
a6b6951 
Signed-off-by: Rehan Khan <Rehan.Khan7@ibm.com>
@R3hankhan123
Copy link
Author

/reopen

@R3hankhan123 R3hankhan123 reopened this Sep 20, 2024
@google-oss-prow Google OSS Prow
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 chensun 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

@google-oss-prow google-oss-prow bot added size/M and removed size/XS labels Sep 20, 2024
@google-oss-prow Google OSS Prow
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 chensun 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

@R3hankhan123
Copy link
Author

@hbelmiro looks like the workflows still arent being triggered automatically
Screenshot 2024-09-20 at 5 38 45 PM

@R3hankhan123
Fixed argo installation in Dockerfile
5d47c2e 
Signed-off-by: Rehan Khan <Rehan.Khan7@ibm.com>
@hbelmiro
Copy link
Contributor

/ok-to-test
/rerun-all

@R3hankhan123
Copy link
Author

@hbelmiro looks like all the tests have passed and just needs the lgtm label

hbelmiro
hbelmiro reviewed Sep 23, 2024
View reviewed changes
backend/Dockerfile.launcher Outdated Show resolved Hide resolved
backend/Makefile Outdated Show resolved Hide resolved
backend/api/Dockerfile Outdated Show resolved Hide resolved
backend/src/cache/deployer/Dockerfile
@@ -1,11 +1,17 @@
FROM gcr.io/google.com/cloudsdktool/google-cloud-cli:alpine
FROM alpine
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this.
@rimolive @chensun thoughts?

Copy link

@dilipgb dilipgb Sep 24, 2024 •
edited
Loading

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dilipgb I see. But I'm not sure how able we are to change images without a broader involvement of the community. Like asking in the community call, for example.

cc @HumairAK

backend/src/cache/deployer/deploy-cache-service.sh Outdated Show resolved Hide resolved
backend/src/cache/deployer/deploy-cache-service.sh Outdated Show resolved Hide resolved
third_party/minio/Dockerfile Outdated Show resolved Hide resolved
@R3hankhan123
Made the requested changes
b573e98 
Signed-off-by: Rehan Khan <Rehan.Khan7@ibm.com>
@hbelmiro
Copy link
Contributor

/ok-to-test
/rerun-all

@hbelmiro hbelmiro mentioned this pull request Sep 27, 2024
Merged
2 tasks
@dilipgb
Copy link

dilipgb commented Oct 3, 2024

@hbelmiro can we list this as one of the topic to discuss in coming community meeting?

@hbelmiro
Copy link
Contributor

hbelmiro commented Oct 3, 2024

@hbelmiro can we list this as one of the topic to discuss in coming community meeting?

@R3hankhan123 Sure.

HumairAK
HumairAK reviewed Oct 9, 2024
View reviewed changes
third_party/minio/Dockerfile
@@ -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
Copy link
Collaborator

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?

Copy link

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.

HumairAK
HumairAK reviewed Oct 9, 2024
View reviewed changes
backend/Makefile
@@ -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 .
Copy link
Collaborator

@HumairAK HumairAK Oct 9, 2024 •
edited
Loading

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?

@HumairAK
Copy link
Collaborator

HumairAK commented Oct 9, 2024

/hold due to: #10766 (comment)

@R3hankhan123
Copy link
Author

Hi @HumairAK , can you add multi arch discussion in the doc for the upcoming community meet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HumairAK HumairAK HumairAK left review comments

@hbelmiro hbelmiro hbelmiro left review comments

@dilipgb dilipgb dilipgb left review comments

@chensun chensun Awaiting requested review from chensun

@zijianjoy zijianjoy Awaiting requested review from zijianjoy

Assignees

@hbelmiro hbelmiro

Labels
do-not-merge/hold ok-to-test size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@R3hankhan123 @hbelmiro @rimolive @dilipgb @HumairAK