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

Add arm64 support for kfserving #507

Merged
merged 1 commit into from
Nov 4, 2019
Merged

Add arm64 support for kfserving #507

merged 1 commit into from
Nov 4, 2019

Conversation

MrXinWang
Copy link
Contributor

@MrXinWang MrXinWang commented Nov 1, 2019

Signed-off-by: Henry Wang <henry.wang@arm.com>


This change is Reviewable

Change-Id: I8d583a60e3fc6f1714a75f8040164d4ed7a0d829
Signed-off-by: Henry Wang <henry.wang@arm.com>
Jira: ENTOS-1325
@k8s-ci-robot
Copy link
Contributor

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

@jinchihe
Copy link
Contributor

jinchihe commented Nov 1, 2019

/ok-to-test

@MrXinWang
Copy link
Contributor Author

Hi @jinchihe! I think something strange is going on with the CI? Any ideas?

@jinchihe
Copy link
Contributor

jinchihe commented Nov 1, 2019

@MrXinWang That's should be caused by CI test env problem, I'm going to take a look.

@MrXinWang
Copy link
Contributor Author

@jinchihe Thanks very much!

@rakelkar
Copy link
Contributor

rakelkar commented Nov 1, 2019

Cool what's the scenario? Do istio and knative already work on arm? Also a lot more will be required including possibily selecting different base images.. can you raise an issue with details on the use case?

/hold

@MrXinWang
Copy link
Contributor Author

MrXinWang commented Nov 1, 2019

Hi @rakelkar! I think issue#2237 and this comment have something that you may be interested in.

Basically we are trying to enable kubeflow on arm64 platform, and yes you are absolutely right that a lot more work is required. When I started to work on this project the latest version of kubeflow was 0.5.1, and istio was introduced in 0.6.0 and later. For now I am just contributing what I have achieved to upstream. We do have some discussions about the base image in kubeflow/pipeline PR#2507, maybe you can also share your opinion about it.

Edited on 2, Nov: Also by asking some colleagues, I think istio supports arm64 now.

@MrXinWang
Copy link
Contributor Author

MrXinWang commented Nov 2, 2019

/retest

@ellistarn
Copy link
Contributor

Test flake resolved in another PR. You may want to rebase.
/retest

@@ -9,6 +9,8 @@ COPY vendor/ vendor/
# Build
RUN if [ "$(uname -m)" = "ppc64le" ]; then \
CGO_ENABLED=0 GOOS=linux GOARCH=ppc64le go build -a -o manager ./cmd/manager; \
elif [ "$(uname -m)" = "aarch64" ]; then \
CGO_ENABLED=0 GOOS=linux GOARCH=arm64 go build -a -o manager ./cmd/manager; \
Copy link
Contributor

Choose a reason for hiding this comment

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

make sense to just make this an envvar that gets picked up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it definately makes sense, however referencing moby/issue#29110 I don't think docker currently support the function that set ENV as the result of the shell command (in our case uname -m). I think this is also the reason why PowerPC guys keep the coding style of branch asif ["$(uname -m)" = ARCH] stuff.

Copy link
Contributor Author

@MrXinWang MrXinWang Nov 3, 2019

Choose a reason for hiding this comment

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

if we use RUN export MACHINE_ARCH=$(uname -m) && if [$MACHINE_ARCH="ppc64le"/"aarch64"/"x86_64"]; then blablabla, I think it is a little bit redundant isn't it?

@rakelkar
Copy link
Contributor

rakelkar commented Nov 2, 2019

Cool okay - will be awesome to see it work on arm64. If you are trying to minimize initial scope you dont need the executor as it is not currently used.

/hold cancel

@MrXinWang
Copy link
Contributor Author

@jinchihe Test works perfectly now, thanks

@jinchihe
Copy link
Contributor

jinchihe commented Nov 4, 2019

/lgtm

@animeshsingh
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: animeshsingh

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

@k8s-ci-robot k8s-ci-robot merged commit a070c9f into kserve:master Nov 4, 2019
ellistarn pushed a commit to ellistarn/kfserving that referenced this pull request Jul 28, 2020
Change-Id: I8d583a60e3fc6f1714a75f8040164d4ed7a0d829
Signed-off-by: Henry Wang <henry.wang@arm.com>
Jira: ENTOS-1325
spolti pushed a commit to spolti/kserve that referenced this pull request Sep 16, 2024
…erve-agent-hermetic-poc

Red Hat Konflux update kserve-agent-hermetic-poc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants