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

Enable arm64 architecture support for katib images and fix grpc health probe multiarch error. #897

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

MrXinWang
Copy link
Contributor

@MrXinWang MrXinWang commented Oct 25, 2019

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


This change is Reviewable

@k8s-ci-robot
Copy link

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.

@MrXinWang
Copy link
Contributor Author

cc/ @gaocegege Please review.

apk --update add git gcc musl-dev && \
go get github.com/grpc-ecosystem/grpc-health-probe && \
mv $GOPATH/bin/grpc-health-probe /bin/grpc_health_probe && \
chmod +x /bin/grpc_health_probe; \
Copy link
Member

Choose a reason for hiding this comment

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

we suggest that grpc_health_probe should keep consistent in all kinds of platform.
Please refer to #893

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :) shall I retest it?

@hougangliu
Copy link
Member

/ok-to-test

@gaocegege
Copy link
Member

@MrXinWang

This PR contains changes in suggestion image build. While now our test infra has some problems on pip install and cannot build the suggestion image successfully. Would you mind testing and merging it after our new version 0.7 is released? (Maybe in several days)

@MrXinWang
Copy link
Contributor Author

@MrXinWang

This PR contains changes in suggestion image build. While now our test infra has some problems on pip install and cannot build the suggestion image successfully. Would you mind testing and merging it after our new version 0.7 is released? (Maybe in several days)

@gaocegege of course I don't mind, should I re-trigger the test or one of the reviewers will kindly do this for me? or should I propose another PR after you guys releasing the new version?

Also, accroding to my understanding, the fix in testing infra would not affect the mergeablity of this patch, and since I have tested that the updated dockerfiles can be build successfully, therefore is there any possibility to merge this sooner? Thanks.

@gaocegege
Copy link
Member

gaocegege commented Oct 28, 2019

@MrXinWang

When we are ready, I will comment in this PR. And you just need to rebase upstream master at that time.

Also, according to my understanding, the fix in testing infra would not affect the mergeablity of this patch, and since I have tested that the updated dockerfiles can be build successfully, therefore is there any possibility to merge this sooner? Thanks.

We disabled the image build in CI, thus we cannot verify that it actually works. Ref 818b879#diff-7038d33999b797fcebe6cdfddd5806b7R23

Thus prefer rebasing and testing it when the CI is fixed if you do not mind. Thanks. And it should not be long. We will release today or tomorrow, I think.

@MrXinWang
Copy link
Contributor Author

@gaocegege

Ahhh ok, then take your time and I will wait. Thanks for your patience in explaining this :)

@gaocegege
Copy link
Member

@MrXinWang

Thanks for your patience, too.

@gaocegege
Copy link
Member

@MrXinWang Please rebase the master.

@MrXinWang
Copy link
Contributor Author

@gaocegege Got it, thanks!

@MrXinWang
Copy link
Contributor Author

@gaocegege still failing, is there any method to see the logs? Thanks

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/lgtm

apt-get -y update && \
apt-get -y install gfortran libopenblas-dev liblapack-dev && \
pip install cython; \
pip install cython 'numpy>=1.13.3'; \
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: The reason why I added the pre-installation of numpy is that without this, when pip install -r requirements.txt, an error would occur on my arm64 machine saying that numpy should be installed first.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Require @hougangliu 's review since I am not sure if it affects PPC.

@gaocegege
Copy link
Member

Thanks for your contribution! 🎉 👍

@MrXinWang
Copy link
Contributor Author

@gaocegege sorry wait...I think a test is still running and based on my check on the details I dont think CI machine likes this (a v1alpha3 pod was failing).....I think the "/lgtm" label is too soon now. :)

@gaocegege
Copy link
Member

No worry, we need @hougangliu 's approve thus lgtm does not work, actually.

@gaocegege
Copy link
Member

testing-argo.kubeflow.org/workflows/kubeflow-test-infra/kubeflow-katib-presubmit-e2e-v1alpha3-S897-f19a6c2-6656-7188?tab=workflow

I think it is caused by gcloud build. let's retest.

/retest

…h probe multiarch error.

Change-Id: I5ddee7e8fbe96b8e0a025e3f182b4a5192c45597
Signed-off-by: Henry Wang <henry.wang@arm.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm label Nov 4, 2019
@MrXinWang
Copy link
Contributor Author

Hi @gaocegege! Solved the test failing problem and tests seem to be perfect now. Sorry for the inconvenience caused and thank you so much for your support as well as patience.

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks for your contribution! 🎉 👍

apt-get -y update && \
apt-get -y install gfortran libopenblas-dev liblapack-dev && \
pip install cython; \
pip install cython 'numpy>=1.13.3'; \
Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Require @hougangliu 's review since I am not sure if it affects PPC.

@hougangliu
Copy link
Member

/approve
thanks @MrXinWang

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hougangliu

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

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.

4 participants