new deploy API following Open API format #2564

kunmingg · 2019-02-27T00:49:06Z

New deployment API supports 2 types of entries:

config entry
each entry maps to a set of config files stored in source repo or Github which can fully define a deployment entry.
deployment entry
each entry maps to a kubeflow instance on GCP which include resources like GKE, Endpoint, Storage, Service Account, IAM bindings. Deployment entry is created following definition in the corresponding config entry.

Goals of new API include simplify reuse configs and support configs customization.

This change is

bootstrap/api/swagger.yaml

jlewi · 2019-02-27T13:46:51Z

bootstrap/api/swagger.yaml

+definitions:
+  CreateDeployment:
+    type: "object"
+    properties:


is this the body of the request?

bootstrap/api/swagger.yaml

jlewi · 2019-02-27T13:52:08Z

bootstrap/api/swagger.yaml

+        description: "Pet object that needs to be added to the store"
+        required: true
+        schema:
+          $ref: "#/definitions/CreateDeployment"


Following K8s and GCP style can the body of the Create request just be the spec for the deployment?
i.e. a Create and Get request use the same object just with different fields.

Perhaps instead of Create we should just have an Update method which would create it if it doesn't exist?

jlewi · 2019-02-27T13:54:33Z

bootstrap/api/swagger.yaml

+          schema:
+            $ref: "#/definitions/InvalidInput"
+definitions:
+  CreateDeployment:


Should we follow the K8s resource model?
So maybe the Deployment should have fields
metadata
name
namespace
spec:
cluster
zone
....

jlewi · 2019-02-27T18:05:27Z

bootstrap/api/swagger.yaml

+        type: "array"
+        items:
+          type: "string"
+      parameters:


Following K8s style should we add a status section and use conditions to report status?

jlewi · 2019-02-27T18:10:53Z

bootstrap/api/swagger.yaml

+          $ref: "#/definitions/CreateDeployment"
+      responses:
+        200:
+          description: "successful operation"


One of the goals of the deployment process is to split it into separate phases generate and apply so that people can manually edit the manifests after generate in order to customize them.

Any thought how we could handle that?

One option would be to create custom verbs e.g generate & apply. But that goes against the K8s idea of having a common resource model with common verbs.

A second option would be to have a field in the spec that specifies the desiredState for the deployment. Allowed values could be something like "configured" "deployed"

The flow could then be something like

Call create with desiredState: configured

This creates the manifests and pushes them to source repo

User modifies the manifests

User calls Update setting the desiredState to deployed

jlewi · 2019-03-05T18:54:31Z

bootstrap/api/swagger.yaml

+          description: "Invalid input"
+          schema:
+            $ref: "#/definitions/ErrorMessage"
+  /projects/{project}/configs/{config}/version/{version}/file:


What is the version resource for?

Is it just fetching the config?

If users want to fetch the config why don't they talk to underlying storage (e.g source repo directly)? Is this just a helper API?

This helper API provide read / write of single config file in underlying storage:

as of today for source repo there are not such API available.

existing edit UI is confusing because it aimed to solve more complicated day-to-day dev use case.

If suggest user change config directly with underlying storage, user experience becomes:
Create config entry in UI -> setup git and git clone -> edit config -> git push -> apply in UI

With this API we can have experience:
Create config entry in UI -> edit config file in UI -> apply in UI
Then we provide whole experience within UI, would be much easier for usage like editing GCP options

jlewi · 2019-03-05T18:56:16Z

bootstrap/api/swagger.yaml

+    put:
+      tags:
+        - "config"
+      description: "Update kubeflow config entry file"


Why do we need a put operation on config?

If people want to update the config couldn't they do this directly by modifying the file; e.g. in the source repo?

jlewi · 2019-03-05T18:59:52Z

bootstrap/api/swagger.yaml

+    properties:
+      metadata:
+        $ref: "#/definitions/DeploymentMetadata"
+      extraComponents:


Should KfConfig have a spec and status and follow the K8s resource model?

Status could provide information about things like version; e.g. if the underlying source has changed this could be reported via status.

Also it allows us to think of the resource as a controller where spec describes desired state and status provides actual state.

jlewi · 2019-03-05T19:01:45Z

bootstrap/api/swagger.yaml

+        type: "array"
+        items:
+          $ref: "#/definitions/DeploymentMetadata"
+  KfConfigFile:


Per comment above what's this for?

jlewi · 2019-03-05T19:03:44Z

bootstrap/api/swagger.yaml

+          description: "Deployment name"
+          required: true
+          schema:
+            $ref: "#/definitions/KfDeployment"


How does kfDeployment relate to the Config?

Is KfDeployment just a pointer to the config?

Relation between kfDeployment and KfConfig is like
relation between notebook instance and notebook image:
User can spawn up kfDeployment from existing KfConfig entries.

…al APIs

jlewi

Reviewable status: 0 of 1 files reviewed, 15 unresolved discussions (waiting on @abhi-g, @ellis-bigelow, and @kunmingg)

bootstrap/api/swagger.yaml, line 77 at r2 (raw file):

Previously, kunmingg (Kunming Qu) wrote…

This helper API provide read / write of single config file in underlying storage:

as of today for source repo there are not such API available.

existing edit UI is confusing because it aimed to solve more complicated day-to-day dev use case.

If suggest user change config directly with underlying storage, user experience becomes:
Create config entry in UI -> setup git and git clone -> edit config -> git push -> apply in UI

With this API we can have experience:
Create config entry in UI -> edit config file in UI -> apply in UI
Then we provide whole experience within UI, would be much easier for usage like editing GCP options

Is version referring to a git version?

Should the API not expose the version? The user just fetches and/or uploads a file (or tarball) containing the file contains.

The implementation is responsible for committing this to a source repo if that is the desired state.

bootstrap/api/swagger.yaml, line 184 at r2 (raw file):

Previously, kunmingg (Kunming Qu) wrote…

Relation between kfDeployment and KfConfig is like
relation between notebook instance and notebook image:
User can spawn up kfDeployment from existing KfConfig entries.

Does KfConfig refer to a KfApp?

Specifically does it refer to the set of files that would be in the KfApp dir?

bootstrap/api/swagger.yaml, line 269 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Should KfConfig have a spec and status and follow the K8s resource model?

Status could provide information about things like version; e.g. if the underlying source has changed this could be reported via status.

Also it allows us to think of the resource as a controller where spec describes desired state and status provides actual state.

Thoughts?

bootstrap/api/swagger.yaml, line 12 at r3 (raw file):

    name: "Apache 2.0"
    url: "http://www.apache.org/licenses/LICENSE-2.0.html"
basePath: "/kfctl/v1"

How difficult would be it to generate documentation for the API?

I find it a lot hard to evaluate the API based on the YAMl file. Generated docs might help.

bootstrap/api/swagger.yaml, line 26 at r3 (raw file):

      tags:
        - "configEntry"
      summary: "Create a new kubeflow config entry on source repo"

What is a ConfigEntry? Is a ConfigEntry the entire contents of ${KFAPP}?

bootstrap/api/swagger.yaml, line 124 at r3 (raw file):

    get:
      tags:
        - "configFile"

Why get/set individual files?

Why not upload a complete bundle corresponding to the KFAPP?

If we need a different API just to support some use cases in the UI (e.g. editing certain files) maybe we should do that as part of adding that functionality to the web app and mark it as an internal API.

jlewi · 2019-03-07T07:15:17Z

@kunmingg Thanks. Could you take a look at some of my previous comments such as adding a spec and status.

If your blocked on getting started on implementation I think we can go ahead and merge this.

We will likely need to make some revisions in the future anyway after circulating it more broadly.

I think it would help to actually generate the API documentation (we could do that in a follow on PR) but that is probably a lot more readable then the swagger spec.

/lgtm
/approve
/hold

kunmingg

Reviewable status: 0 of 1 files reviewed, 15 unresolved discussions (waiting on @abhi-g, @ellis-bigelow, @jlewi, and @kunmingg)

bootstrap/api/swagger.yaml, line 76 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Following K8s style should we add a status section and use conditions to report status?

Done.

bootstrap/api/swagger.yaml, line 77 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Is version referring to a git version?

Should the API not expose the version? The user just fetches and/or uploads a file (or tarball) containing the file contains.

The implementation is responsible for committing this to a source repo if that is the desired state.

version is git version, currently we provide single file read / write, as whole config files set might be pretty large and increase flaky to download/upload + git operations.

bootstrap/api/swagger.yaml, line 184 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Does KfConfig refer to a KfApp?

Specifically does it refer to the set of files that would be in the KfApp dir?

One KfConfig refers to a set of config files (DM + ksonnet) on source repo

bootstrap/api/swagger.yaml, line 269 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Thoughts?

Will update

bootstrap/api/swagger.yaml, line 26 at r3 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

What is a ConfigEntry? Is a ConfigEntry the entire contents of ${KFAPP}?

One config entry is 1 KfConfig

bootstrap/api/swagger.yaml, line 124 at r3 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Why get/set individual files?

Why not upload a complete bundle corresponding to the KFAPP?

If we need a different API just to support some use cases in the UI (e.g. editing certain files) maybe we should do that as part of adding that functionality to the web app and mark it as an internal API.

Sure, can do it.

kunmingg · 2019-03-07T23:28:27Z

@jlewi
To view API as docs:
https://rebilly.github.io/ReDoc/?url=https://raw.githubusercontent.com/kubeflow/kubeflow/0f8a3babb6e0f8425dd5dff9432a68889911366a/bootstrap/api/swagger.yaml

jlewi

Reviewable status: 0 of 1 files reviewed, 15 unresolved discussions (waiting on @abhi-g, @ellis-bigelow, @jlewi, and @kunmingg)

a discussion (no related file):
The docs page really helps can you add link to that either in the .yaml file or in a README.

bootstrap/api/swagger.yaml, line 53 at r1 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

is this the body of the request?

Why are email, zone part of the metadata as opposed to being in the deployment spec?

And shouldn't the deploymentSpec have a link to the config it is using?

jlewi · 2019-03-08T23:44:49Z

Discussed with Kunming. We'll submit as is and iterate in follow on PRs

Some open questions

I'm not sure email should be a query parameter.
- I think it might be better to make it part of the KfConfig; e.g. have a YAML file somewhere with users/groups to be granted permission
- For purposes of auditing the email query arg isn't good because it can't be audited.
- Audit logs will automatically be generated for all actions based on the access token used.
Will move namespace into the KfConfig app.

jlewi · 2019-03-08T23:44:54Z

/lgtm
/approve

k8s-ci-robot · 2019-03-08T23:44:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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:

~~bootstrap/OWNERS~~ [jlewi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kunmingg · 2019-03-09T00:21:21Z

/hold cancel

* new deploy API following Open API format * start * split deployment APIs based on 2 types of objects: config and deployment * remove host * modify definitions following comments; add description for experimental APIs * add delete methods; unify response code * change following comments * resolve review feedback; move email to query

new deploy API following Open API format

32761b0

kunmingg assigned jlewi Feb 27, 2019

googlebot added the cla: yes label Feb 27, 2019

k8s-ci-robot added the size/L label Feb 27, 2019

k8s-ci-robot requested review from abhi-g and ellistarn February 27, 2019 00:49

jlewi reviewed Feb 27, 2019

View reviewed changes

bootstrap/api/swagger.yaml Outdated Show resolved Hide resolved

jlewi reviewed Feb 27, 2019

View reviewed changes

bootstrap/api/swagger.yaml

definitions:

CreateDeployment:

type: "object"

properties:

Copy link

Contributor

jlewi Feb 27, 2019

Choose a reason for hiding this comment

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

is this the body of the request?

jlewi reviewed Feb 27, 2019

View reviewed changes

bootstrap/api/swagger.yaml Outdated Show resolved Hide resolved

jlewi reviewed Feb 27, 2019

View reviewed changes

bootstrap/api/swagger.yaml Outdated Show resolved Hide resolved

jlewi reviewed Feb 27, 2019

View reviewed changes

kunmingg added 3 commits February 28, 2019 22:04

start

28108d8

split deployment APIs based on 2 types of objects: config and deployment

9f64eb1

remove host

912ecc4

jlewi reviewed Mar 5, 2019

View reviewed changes

kunmingg added 2 commits March 5, 2019 18:41

modify definitions following comments; add description for experiment…

1108d89

…al APIs

add delete methods; unify response code

3edffe1

jlewi suggested changes Mar 7, 2019

View reviewed changes

k8s-ci-robot added lgtm do-not-merge/hold approved labels Mar 7, 2019

kunmingg commented Mar 7, 2019

View reviewed changes

change following comments

0f8a3ba

k8s-ci-robot removed the lgtm label Mar 7, 2019

jlewi suggested changes Mar 8, 2019

View reviewed changes

resolve review feedback; move email to query

1301486

k8s-ci-robot added the lgtm label Mar 8, 2019

k8s-ci-robot removed the do-not-merge/hold label Mar 9, 2019

k8s-ci-robot merged commit 6fa88b7 into kubeflow:master Mar 9, 2019

pdmack mentioned this pull request Mar 11, 2019

Enabling EFS on AWS for Tensorboard #2569

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new deploy API following Open API format #2564

new deploy API following Open API format #2564

kunmingg commented Feb 27, 2019 •

edited

jlewi Feb 27, 2019

jlewi Feb 27, 2019

jlewi Feb 27, 2019

jlewi Feb 27, 2019

jlewi Feb 27, 2019

jlewi Feb 27, 2019

jlewi Mar 5, 2019

kunmingg Mar 5, 2019

jlewi Mar 5, 2019

jlewi Mar 5, 2019

jlewi Mar 5, 2019

jlewi Mar 5, 2019

kunmingg Mar 5, 2019

jlewi left a comment

jlewi commented Mar 7, 2019

kunmingg left a comment

kunmingg commented Mar 7, 2019

jlewi left a comment

jlewi commented Mar 8, 2019

jlewi commented Mar 8, 2019

k8s-ci-robot commented Mar 8, 2019

kunmingg commented Mar 9, 2019

new deploy API following Open API format #2564

new deploy API following Open API format #2564

Conversation

kunmingg commented Feb 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlewi left a comment

Choose a reason for hiding this comment

jlewi commented Mar 7, 2019

kunmingg left a comment

Choose a reason for hiding this comment

kunmingg commented Mar 7, 2019

jlewi left a comment

Choose a reason for hiding this comment

jlewi commented Mar 8, 2019

jlewi commented Mar 8, 2019

k8s-ci-robot commented Mar 8, 2019

kunmingg commented Mar 9, 2019

kunmingg commented Feb 27, 2019 •

edited