Add support to kfctl to allow deployment on PPC64LE aka IBM Power architecture #290
Conversation
|
Hi @benswinney. 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 |
|
/lgtm /assign @Tomcli |
Makefile
Outdated
| @@ -126,14 +126,16 @@ build-kfctl: deepcopy generate fmt vet | |||
| #GOOS=windows GOARCH=amd64 ${GO} build -gcflags '-N -l' -ldflags "-X main.VERSION=$(TAG)" -o bin/windows/kfctl.exe cmd/kfctl/main.go | |||
| GOOS=darwin GOARCH=amd64 ${GO} build -gcflags '-N -l' -ldflags "-X main.VERSION=${TAG}" -o bin/darwin/kfctl cmd/kfctl/main.go | |||
| GOOS=linux GOARCH=amd64 ${GO} build -gcflags '-N -l' -ldflags "-X main.VERSION=$(TAG)" -o bin/linux/kfctl cmd/kfctl/main.go | |||
| cp bin/$(ARCH)/kfctl bin/kfctl | |||
| GOOS=linux GOARCH=ppc64le ${GO} build -gcflags '-N -l' -ldflags "-X main.VERSION=$(TAG)" -o bin/ppc64le/kfctl cmd/kfctl/main.go | |||
| #cp bin/$(ARCH)/kfctl bin/kfctl | |||
There was a problem hiding this comment.
Why are we commenting out this line? If it's no longer necessary, we should delete it.
There was a problem hiding this comment.
@benswinney please address above comment from @Tomcli . That command is supposed to copy the platform specific kfctl executable to the default kfctl/bin directory. Any reason you are removing/commenting out that line?
There was a problem hiding this comment.
The architecture for kfctl on Darwin and amd64 linux is the same (x86_64). It was possible to use the same kfctl for both.
ppc64le is not the same architecture as Darwin and amd64, so you could not use the same kfctl file. We need to build separate images for ppc64le.
If the line is left in, then the final kfctl within bin/kfctl would be ppc64le and that would fail on x86_64 machines.
Hopefully this helps explain the need to remove that line.
There was a problem hiding this comment.
I have updated the code again and pushed a new commit to remove the unneeded code :)
|
/lgtm |
|
It appears tests are broken on master and will need to be fixed before this PR can be submitted. |
|
/retest |
|
/retest |
|
[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 |
|
@benswinney: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
Can you rebase and run test? |
|
/test ? |
|
@PatrickXYS: The following commands are available to trigger jobs:
Use 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/test-infra repository. |
|
@PatrickXYS: No presubmit jobs available for kubeflow/kfctl@master 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/test-infra repository. |
|
/retest |
|
@benswinney: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
I'm continuing a project that added support to Kubeflow that allowed deployment on ppc64le.
Rather than keep seperate repositories, I feel it would be best suited to be included within the Kubeflow repository. This allows for one central place to pull Kubeflow regardless of the underlying compute architecture.
This change is