Skip to content
This repository has been archived by the owner on Dec 2, 2021. It is now read-only.

update Dockerfile to support power #96

Merged
merged 1 commit into from
Aug 8, 2019
Merged

Conversation

dreamryx
Copy link
Contributor

@dreamryx dreamryx commented Jul 16, 2019

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:



This change is Reviewable

@k8s-ci-robot
Copy link

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

@neuromage
Copy link
Contributor

Hi @dreamryx. Thanks for your PR. Is this adding support for building under PowerPC? I don't think it's a good idea for our Dockerfile to support other architectures as we have no way to test or maintain this. One thing we could consider is maintaining a fork of the Dockerfile for PowerPC somewhere else perhaps? And we could add a link to it in the README. What do you think?

@dreamryx
Copy link
Contributor Author

Hi @neuromage . Thanks for your advice. Since we want to support Kubeflow on power, ibm may provide power machines for community. You mean to create a new dockerfile - eg. "Dockerfile.ppc64le", right? I OK with it. Which place do you think is proper to put it in? Thanks!

@neuromage
Copy link
Contributor

@dreamryx how is it done currently for other kubeflow applications? Ideally anything we check in here can be tested and maintained by the metadata dev team. Hence I can think of two possibilities here:

  • We add a README here with instructions on how to modify the Dockerfile to make it work with PowerPC.
  • We add a link in the existing README pointing to another repository hosting the powerpc version.

We can also combine the above and do both if that's preferable. What do you think?

@dreamryx
Copy link
Contributor Author

@neuromage Thanks. For other kubeflow application, I do the similar work as here. For example, tf-operator#981 and katib#673. The specific docker image for power can be build from the dockerfile. If you concern it, I can move the modification to a README file instead of changing the dockerfile directly.

@neuromage
Copy link
Contributor

Thanks for pointing me to those precedents. My concern is that since this is a developing project which will see lots of changes, maintaining that powerpc stanza will be difficult for us, especially since we can't test those changes. How about checking in a Dockerfile for powerpc within a separate powerpc folder. In that folder, you can add a README with some instructions, and also point out that it may not be up to date with the main Dockerfile so users are aware. Does this sound reasonable?

@zhenghuiwang any thoughts?

@zhenghuiwang
Copy link
Contributor

@dreamryx I have the same concern of maintaining a complicated script to build various docker images for various environments.

Can I suggest you maintain your own base image for Power PC? You can add some instructions on README to let Power PC users know they should replace the base image in current Dockerfile.

@dreamryx
Copy link
Contributor Author

@neuromage @zhenghuiwang Thanks for your advices. I'm OK with creating a dir named powerpc to hold a Dockerfile and README or maintain a image in my private docker hub. Since we want to support kubeflow on power, so how about adding a dir to hold Dockerfile.ppc64le and a build option in makefile?

@zhenghuiwang
Copy link
Contributor

zhenghuiwang commented Jul 19, 2019

@neuromage @zhenghuiwang Thanks for your advices. I'm OK with creating a dir named powerpc to hold a Dockerfile and README or maintain a image in my private docker hub. Since we want to support kubeflow on power, so how about adding a dir to hold Dockerfile.ppc64le and a build option in makefile?

@dreamryx I'm genuinely wondering if there is a better way to handle this.

The dockerfile has two steps:

  1. Setup building environments (Go, Bazel, and Java in your case)
  2. Build and move the binary; add license files.

I think these two steps can be better parameterized by base image + docker ARG:

  1. Create a dockerfiles directory, which has two files: dockerfile.linux-base and dockerfile.ppc64le-base. Each of them sets up the environment, such as EXTRA_BAZEL_ARGS in your case.
  2. Parameterize dockerfile by adding
ARG baseimage = gcr.io/public-image/metadata/base
ARG binarypath=bazel-bin/server/linux_amd64_stripped/server
  1. Add your make target to pass different ARGs.

So that we can share the improvements of steps 2, such as adding more license files for new dependencies. We also reduce the building cost by not installing bazel every time.

WDYT?

@dreamryx
Copy link
Contributor Author

@zhenghuiwang Thanks. That sounds great! But there is one problem, where can I store the base image for power? Is it appropriate for me to save the base image for power in my own docker hub? Hope the base image and the target image could support multi-arc.

@zhenghuiwang
Copy link
Contributor

@zhenghuiwang Thanks. That sounds great! But there is one problem, where can I store the base image for power? Is it appropriate for me to save the base image for power in my own docker hub? Hope the base image and the target image could support multi-arc.

You can store your ppc base image in your docker hub.

@dreamryx
Copy link
Contributor Author

@zhenghuiwang OK. Got it.

@dreamryx
Copy link
Contributor Author

@zhenghuiwang Please review it again. Thanks. The doc for how to modify dockerfile to support power is under review. We want to choose one to modify all related components. The doc is kubeflow_multiarch.

@zhenghuiwang
Copy link
Contributor

/retest

@zhenghuiwang
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhenghuiwang

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants