Don't create PVCs if no default StorageClass is set #2679

kimwnasptd · 2019-03-11T18:15:28Z

If no default StorageClass is set, then the user won't be able to create New PVCs for the Notebooks using the Jupyter UI.

/assign @jlewi

/hold
wait for #2671

This change is

If no default StorageClass is provided, then we don't let the users create new Volumes. Neither Workspace nor Data Volumes. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Function for * Loading the rok secret * Decoding the parameterized ROK_SECRET_NAME with a user value The user value will currently always be 'user' Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Instead of using hand-made dicts for pvcs, use client classes for kubernetes objects. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

jlewi · 2019-03-13T14:19:54Z

@avdaredevil
/assign @prodonjs

Could you review this please?

prodonjs

Made a few comments. In general, I am looking at this for the very first time so I don't know how useful my context is. I just saw a few things that I thought looked like they might be confusing if I were going to try to come in and make subsequent changes in this module.

prodonjs · 2019-03-13T15:43:06Z

components/jupyter-web-app/default/kubeflow/jupyter/server.py

+  return
+
+
+def create_datavol_pvc(body, i):


Ideally I think there would be some kind of definition for the shape of the body dict being passed in, but since I have almost no context of looking at this I'm not going to be picky.

prodonjs · 2019-03-13T15:49:55Z

components/jupyter-web-app/default/kubeflow/jupyter/server.py

@@ -60,5 +133,5 @@ def create_notebook(body):


 def create_pvc(body):
-  ns = body['metadata']['namespace']
+  ns = body.metadata.namespace


So in this case body has actually changed from a dict to an object. Again, having no context it may be helpful to have some kind of comment or change the name of the param to reflect that it's not a deserialized JSON response as a dict but is not a typed object.

prodonjs · 2019-03-13T16:04:53Z

components/jupyter-web-app/rok/kubeflow/rokui/routes.py

    try:
-      create_pvc(pvc)
+      create_workspace_pvc(body)


The code here looks very similar to components/jupyter-web-app/default/kubeflow/jupyter/server.py. Again, I wouldn't want to request any significant changes with so little context but it does seem like it would advantageous to factor out this code into a single shared module.

* Create baseui module to reduce code duplication * Edit Makefile to run the webapps Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

* Add more comments to make the code more descriptive * Replace single quotes with double ones * Add readme for baseui module Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

kimwnasptd

Thank you for reviewing it @prodonjs, I also added you and @avdaredevil as reviewers

Reviewable status: 0 of 28 files reviewed, 3 unresolved discussions (waiting on @prodonjs and @vkoukis)

components/jupyter-web-app/rok/kubeflow/rokui/routes.py, line 52 at r1 (raw file):

Previously, prodonjs (Jason Prodonovich) wrote…

The code here looks very similar to components/jupyter-web-app/default/kubeflow/jupyter/server.py. Again, I wouldn't want to request any significant changes with so little context but it does seem like it would advantageous to factor out this code into a single shared module.

Sure! I reorganized the structure of the code and put the repetitive code in a module.

components/jupyter-web-app/default/kubeflow/jupyter/server.py, line 56 at r1 (raw file):

Previously, prodonjs (Jason Prodonovich) wrote…

Ideally I think there would be some kind of definition for the shape of the body dict being passed in, but since I have almost no context of looking at this I'm not going to be picky.

Do you mean to add a definition to the backend or have it in a README/spec, for example, to make it clear?

components/jupyter-web-app/default/kubeflow/jupyter/server.py, line 136 at r1 (raw file):

Previously, prodonjs (Jason Prodonovich) wrote…

So in this case body has actually changed from a dict to an object. Again, having no context it may be helpful to have some kind of comment or change the name of the param to reflect that it's not a deserialized JSON response as a dict but is not a typed object.

Done.

Added some relative comments in other places to provide more context to the viewer

avdaredevil

Looks good otherwise!

avdaredevil · 2019-03-14T21:33:34Z

components/jupyter-web-app/OWNERS

@@ -3,3 +3,5 @@ approvers:
  - kimwnasptd
 reviewers:
  - vkoukis
+  - prodonjs
+  - avdaredevil


Please alphabetically sort

Great! lgtm-ing

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

kimwnasptd · 2019-03-14T21:38:39Z

/hold cancel

avdaredevil · 2019-03-14T22:15:18Z

/lgtm

prodonjs · 2019-03-15T12:18:49Z

/lgtm
/approve

kimwnasptd · 2019-03-19T09:23:50Z

@lluunn could you please approve this PR?

lluunn · 2019-03-20T21:06:29Z

/approve
sorry missed this

k8s-ci-robot · 2019-03-20T21:06:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lluunn, prodonjs

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:

~~components/jupyter-web-app/OWNERS~~ [lluunn]
~~kubeflow/jupyter/OWNERS~~ [lluunn]

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

* Check for default StorageClass If no default StorageClass is provided, then we don't let the users create new Volumes. Neither Workspace nor Data Volumes. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Use client for creating pvcs Instead of using hand-made dicts for pvcs, use client classes for kubernetes objects. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Fix sharp corners Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Add helper functions Function for * Loading the rok secret * Decoding the parameterized ROK_SECRET_NAME with a user value The user value will currently always be 'user' Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Handle the token from the html Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Refactor code. Cleanup post_notebook() Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Add permissions to list storage classes Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Add logging Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Reorganize code * Create baseui module to reduce code duplication * Edit Makefile to run the webapps Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Update the Dockerfile Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Add prodonjs and avdaredevil as Reviewers Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Add comments * Add more comments to make the code more descriptive * Replace single quotes with double ones * Add readme for baseui module Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> * Sort OWNERS Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

kimwnasptd added 8 commits March 11, 2019 19:17

Check for default StorageClass

6d0d838

If no default StorageClass is provided, then we don't let the users create new Volumes. Neither Workspace nor Data Volumes. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Add helper functions

6916851

Function for * Loading the rok secret * Decoding the parameterized ROK_SECRET_NAME with a user value The user value will currently always be 'user' Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Handle the token from the html

5bdd961

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Fix sharp corners

2461d63

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Use client for creating pvcs

a375798

Instead of using hand-made dicts for pvcs, use client classes for kubernetes objects. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Refactor code. Cleanup post_notebook()

050e37d

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Add permissions to list storage classes

9a244f7

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Add logging

254c49b

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

k8s-ci-robot added the do-not-merge/hold label Mar 11, 2019

k8s-ci-robot assigned jlewi Mar 11, 2019

googlebot added the cla: yes label Mar 11, 2019

k8s-ci-robot added the size/L label Mar 11, 2019

k8s-ci-robot requested review from ellistarn and vkoukis March 11, 2019 18:15

jlewi assigned avdaredevil and unassigned jlewi Mar 13, 2019

jlewi removed the request for review from ellistarn March 13, 2019 14:19

k8s-ci-robot assigned prodonjs Mar 13, 2019

prodonjs reviewed Mar 13, 2019

View reviewed changes

kimwnasptd added 4 commits March 14, 2019 20:37

Reorganize code

38aeb64

* Create baseui module to reduce code duplication * Edit Makefile to run the webapps Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Update the Dockerfile

ddd6f23

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Add prodonjs and avdaredevil as Reviewers

6ebb370

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

Add comments

5c4ae93

* Add more comments to make the code more descriptive * Replace single quotes with double ones * Add readme for baseui module Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

k8s-ci-robot added size/XL and removed size/L labels Mar 14, 2019

kimwnasptd commented Mar 14, 2019

View reviewed changes

avdaredevil suggested changes Mar 14, 2019

View reviewed changes

Sort OWNERS

80846f3

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>

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

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

k8s-ci-robot added the approved label Mar 20, 2019

k8s-ci-robot merged commit b066e07 into kubeflow:master Mar 20, 2019

kimwnasptd deleted the feature-webapp-storageclass branch May 19, 2021 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't create PVCs if no default StorageClass is set #2679

Don't create PVCs if no default StorageClass is set #2679

kimwnasptd commented Mar 11, 2019 •

edited by jlewi

Loading

jlewi commented Mar 13, 2019

prodonjs left a comment

prodonjs Mar 13, 2019

prodonjs Mar 13, 2019

prodonjs Mar 13, 2019

kimwnasptd left a comment

avdaredevil left a comment

avdaredevil Mar 14, 2019

kimwnasptd Mar 14, 2019

avdaredevil Mar 14, 2019

kimwnasptd commented Mar 14, 2019

avdaredevil commented Mar 14, 2019

prodonjs commented Mar 15, 2019

kimwnasptd commented Mar 19, 2019

lluunn commented Mar 20, 2019

k8s-ci-robot commented Mar 20, 2019

Don't create PVCs if no default StorageClass is set #2679

Don't create PVCs if no default StorageClass is set #2679

Conversation

kimwnasptd commented Mar 11, 2019 • edited by jlewi Loading

jlewi commented Mar 13, 2019

prodonjs left a comment

Choose a reason for hiding this comment

prodonjs Mar 13, 2019

Choose a reason for hiding this comment

prodonjs Mar 13, 2019

Choose a reason for hiding this comment

prodonjs Mar 13, 2019

Choose a reason for hiding this comment

kimwnasptd left a comment

Choose a reason for hiding this comment

avdaredevil left a comment

Choose a reason for hiding this comment

avdaredevil Mar 14, 2019

Choose a reason for hiding this comment

kimwnasptd Mar 14, 2019

Choose a reason for hiding this comment

avdaredevil Mar 14, 2019

Choose a reason for hiding this comment

kimwnasptd commented Mar 14, 2019

avdaredevil commented Mar 14, 2019

prodonjs commented Mar 15, 2019

kimwnasptd commented Mar 19, 2019

lluunn commented Mar 20, 2019

k8s-ci-robot commented Mar 20, 2019

kimwnasptd commented Mar 11, 2019 •

edited by jlewi

Loading