SUBMARINE-462. Submarine server should support helm charts by JohnTing · Pull Request #270 · apache/submarine

JohnTing · 2020-04-27T14:37:04Z

What is this PR for?

Currently, there are no tools to install submarine on k8s in one command.

Helm chart is a perfect tool we can choose. In order to do this:

Create helm charts supporting deployments of tensorflow operator, pytorch operator, and submarine server, MySQL.
Create Dockerfiles used to build submarine components' docker images and create tools to handle the required version push to docker hub.
Setup proxy using kubectl to access the submarine server locally
Verify that sample job spec can be submitted to the submarine server successfully.
Verify that workbench can be accessed in a local browser.
After this, if the user has a k8s cluster. The below command will get the installation done.

helm install /charts

What type of PR is it?

Feature

Todos

- Create helm charts supporting deployments of tensorflow operator, pytorch operator, and submarine server, MySQL.
- Create Dockerfiles used to build submarine components' docker images and create tools to handle the required version push to docker hub.
- Setup proxy using kubectl to access the submarine server locally
- Verify that sample job spec can be submitted to the submarine server successfully.
- Verify that workbench can be accessed in a local browser.
- documentation

What is the Jira issue?

https://issues.apache.org/jira/projects/SUBMARINE/issues/SUBMARINE-462

How should this be tested?

Screenshots (if appropriate)

Questions:

Does the licenses files need update? No
Is there breaking changes for older versions? No
Does this needs documentation? No

xunliu · 2020-05-07T00:46:45Z

@jiwq @tangzhankun Please help review this PR, Thanks!

jiwq

Thanks for @JohnTing great contribution.

I suggest split the charts to three items, such as submarine/ kubeflow-tf-operator/kubeflow-pytorch-operator, more info see https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-2-separate-charts. If do it, we should modify the submarine charts to dependency on others.

Because it not must changed in this pr, so I accept refactor it in next pr.
cc: @liuxunorg @tangzhankun

jiwq · 2020-05-08T00:37:17Z

dev-support/docker-images/submarine/build.sh

 if [[ -z "${submarine_dist_exists}" ]]; then
  cd "${SUBMARINE_HOME}"
-  mvn clean package -DskipTests
+  mvn clean package -DskipTests -pl '!submarine-cloud'


Why should modifying it?

Thanks for @jiwq comment

When I build submarine-cloud with normal user, there will be a /submarine-cloud/bin/ submarine-operator and requires root permission to delete
I have to manually delete with root permissions to proceed to the next build
So I usually skip submarine-cloud when build

docs/submarine-server/setup-kubernetes.md

helm-charts/submarine/templates/pytorchjob/rbac.yaml

helm-charts/submarine/values.yaml

helm-charts/submarine/templates/pytorchjob/deployment.yaml

helm-charts/submarine/templates/submarine-database.yaml

helm-charts/submarine/templates/submarine-server.yaml

xunliu · 2020-05-12T05:54:35Z

This PR, wait for me to take a look.

xunliu

@JohnTing very cool!!! Thank you contribution this important feature.

I have 3 suggestions:

Can you attach this helm running screenshot to this PR, so that it can be more easily understood by everyone.
This PR contains helm files of tf, pytorch, and submarine. Too many contents are submitted at once. The best practice is to complete one helm for each PR.
In addition, the submarine-operator helm file is missing from the submarine helm file,
https://github.com/apache/submarine/tree/master/submarine-cloud/manifests/submarine-operator,
Submarine-operator is submarine's automated operation and maintenance tool, just like tf-operator. You can wait for this PR to merge and create a new PR separately.

xunliu · 2020-05-20T14:46:34Z

@JohnTing If you complete this PR, please notify me. Thanks!

tangzhankun · 2020-05-22T04:42:33Z

@JohnTing could you please update based on our offline discussion?
@liuxunorg , As we discussed offline, we'll first go with no-operator helm charts. Once the operator is ready. We can easily update to use operator to deploy all things. For 0.4 release, let's go with the simplified version. Does this make sense?

JohnTing · 2020-05-22T04:51:38Z

@tangzhankun, Thanks for your comment
There are now 3 helm charts
submarine (server & database)
tensorflow (tensorflow-operator)
pytouch (pytouch -operator)

The current version is no submarine-operator
Am I missing something?

Wait, I forgot to delete submarine-operator

xunliu · 2020-05-22T04:52:10Z

@JohnTing could you please update based on our offline discussion?
@liuxunorg , As we discussed offline, we'll first go with no-operator helm charts. Once the operator is ready. We can easily update to use operator to deploy all things. For 0.4 release, let's go with the simplified version. Does this make sense?

Good idea! i agree.

JohnTing · 2020-05-22T14:03:03Z

@liuxunorg
This pr is done.
There are three helm charts.
submarine (server & database)
tensorflow (tensorflow-operator)
pytouch (pytouch-operator)
Wrote the document in setup-kubernetes.md.

jiwq · 2020-05-23T15:19:43Z

tensorflow (tensorflow-operator)
pytouch (pytouch-operator)

@JohnTing I think we should rename the tensorflow to tfjob and pytorch to pytorchjob, because we need to be consistent with with CRD's singular name.

tangzhankun · 2020-05-25T08:39:16Z

@JohnTing I'm kind of confusing. Why are we making 3 helm charts? Any technical reason for that?
I was expecting a single command like "helm install submarine". Anything I missed?

JohnTing · 2020-05-25T08:40:30Z

@jiwq @liuxunorg I have updated this pr, please review it. Thanks.

JohnTing · 2020-05-25T09:14:53Z

@JohnTing I'm kind of confusing. Why are we making 3 helm charts? Any technical reason for that?
I was expecting a single command like "helm install submarine". Anything I missed?

A few days ago, jiwq suggested that I should divide charts into three parts #270 (review)

tangzhankun · 2020-05-26T06:00:56Z

@JohnTing , I've discussed with the intention of multiple helm charts with Wanqiang. Only one big concern is that it seems we'd have to release multiple helms chars if we would like "helm install stable/submarine"?

JohnTing · 2020-05-26T07:34:38Z

@JohnTing , I've discussed with the intention of multiple helm charts with Wanqiang. Only one big concern is that it seems we'd have to release multiple helms chars if we would like "helm install stable/submarine"?

No, after I update dependencies , I can merge 3 helm charts into one now.

jiwq · 2020-05-27T00:06:47Z

@JohnTing , I've discussed with the intention of multiple helm charts with Wanqiang. Only one big concern is that it seems we'd have to release multiple helms chars if we would like "helm install stable/submarine"?

No, after I update dependencies , I can merge 3 helm charts into one now.

@JohnTing Thanks for your patience and careful work. If we decide merge charts, maybe we should keep only the submarine charts under helm-charts folder. Due to charts that use file:// are not allowed in the official Helm repository, so if we want to release it to official repo, you should merge to one chart rather use the dependencies feature.

By the way, the purpose of the split proposal is for easy maintenance and scale at later, also the submarine user can select which machine learning framework can be deployed within him/her cluster. And in the initial stage, I did not consider to release the charts to official repo.

I suggest use the split proposal, but if we want to release it to official repo, I think the single chart is the best practice.

cc: @tangzhankun @liuxunorg

JohnTing · 2020-05-27T06:37:05Z

Thanks @jiwq for your suggestion.

I changed the folder structure.
Placing charts in the charts directory will also automatically deploy,
so there is no need to set the dependency.

This is the current folder structure.

jiwq

+1 LGTM If no more comments, I will commit it later. Thanks @JohnTing working for this.
cc: @liuxunorg @tangzhankun

tangzhankun · 2020-05-28T07:08:57Z

@JohnTing Thanks for the hard work! @jiwq @liuxunorg Thanks for the review. Let's first go with the one helm charts without dependencies. This PR looks good to me.
Only one question, how does this image come from? Do we already have a Dockerfile for this? If so, we need to document how to build it.
"apache/submarine:server-0.4.0-SNAPSHOT"

JohnTing · 2020-05-28T08:14:09Z

@tangzhankun
Yes, we already have several Dockerfiles to build server and database.
I have updated setup-kubernetes.md.

tangzhankun · 2020-05-28T08:53:51Z

@JohnTing , Thanks for the clarification! LGTM. +1

xunliu · 2020-05-28T11:37:47Z

Will merge if no more comments.

JohnTing force-pushed the SUBMARINE-462 branch from a4f37ea to 20b143a Compare May 1, 2020 08:36

xunliu changed the title ~~SUBMARIN-462. Submarine server should support helm charts installation~~ SUBMARINE-462. Submarine server should support helm charts installation May 5, 2020

JohnTing marked this pull request as draft May 7, 2020 18:24

jiwq reviewed May 8, 2020

View reviewed changes

JohnTing marked this pull request as ready for review May 9, 2020 13:28

jiwq suggested changes May 9, 2020

View reviewed changes

xunliu reviewed May 16, 2020

View reviewed changes

SUBMARINE-462

3072b58

JohnTing force-pushed the SUBMARINE-462 branch from 9d09526 to 3072b58 Compare May 18, 2020 17:59

git add

0b9ce02

change

7c11d1d

add License, change readme.md, delete submarine-operator

e927577

JohnTing added 2 commits May 23, 2020 23:27

rename tensorflow to tfjob & pytorch to pytorchjob

a3b045d

rename tf&pytouch name in setup-kubernetes.md

e24936c

dependencies

2b25ca1

add charts

baa4e89

JohnTing added 2 commits May 27, 2020 13:51

Remove the file: dependency and use charts folder

7b0a2b0

change appVersion

feda1fe

JohnTing added 2 commits May 27, 2020 23:45

change doc

e2ad8e7

change submarine charts apiVersion

0b8bc24

jiwq approved these changes May 27, 2020

View reviewed changes

JohnTing added 2 commits May 28, 2020 16:03

add build image doc in setup-kubernetes.md

3997dcd

add build image doc in setup-kubernetes.md

05e40ef

xunliu changed the title ~~SUBMARINE-462. Submarine server should support helm charts installation~~ SUBMARINE-462. Submarine server should support helm charts May 28, 2020

asfgit closed this in 29bc4fb May 28, 2020

Conversation

JohnTing commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

xunliu commented May 7, 2020

Uh oh!

jiwq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiwq May 8, 2020

Choose a reason for hiding this comment

Uh oh!

JohnTing May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xunliu commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xunliu left a comment

Choose a reason for hiding this comment

Uh oh!

xunliu commented May 20, 2020

Uh oh!

tangzhankun commented May 22, 2020

Uh oh!

JohnTing commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xunliu commented May 22, 2020

Uh oh!

JohnTing commented May 22, 2020

Uh oh!

jiwq commented May 23, 2020

Uh oh!

tangzhankun commented May 25, 2020

Uh oh!

JohnTing commented May 25, 2020

Uh oh!

JohnTing commented May 25, 2020

Uh oh!

tangzhankun commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohnTing commented May 26, 2020

Uh oh!

jiwq commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohnTing commented May 27, 2020

Uh oh!

jiwq left a comment

Choose a reason for hiding this comment

Uh oh!

tangzhankun commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohnTing commented May 28, 2020

Uh oh!

tangzhankun commented May 28, 2020

Uh oh!

xunliu commented May 28, 2020

Uh oh!

JohnTing commented Apr 27, 2020 •

edited

Loading

jiwq left a comment •

edited

Loading

JohnTing May 8, 2020 •

edited

Loading

xunliu commented May 12, 2020 •

edited

Loading

JohnTing commented May 22, 2020 •

edited

Loading

tangzhankun commented May 26, 2020 •

edited

Loading

jiwq commented May 27, 2020 •

edited

Loading

tangzhankun commented May 28, 2020 •

edited

Loading