Prepare Notebook Controller for the new Jupyter UI #2349
Conversation
The declaration of the child Deployment should be aware of the entire spec. Thus, `podTemplateSpec` and `templateSpec` should be merged by preserving inner keys. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> Signed-off-by: Ioannis Androulidakis <ioannis@arrikto.com>
The `fsGroup` field should be explicitly set to avoid permission errors when accessing the volumes of the Notebook. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> Signed-off-by: Ioannis Androulidakis <ioannis@arrikto.com>
Inside the podTemplateSpec declaration, the LabApp `allow_remote_access` and `allow_root` args should have values of type Boolean, not String. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> Signed-off-by: Ioannis Androulidakis <ioannis@arrikto.com>
Otherwise, HTTP GET requests from inside the Notebook Pod result in 403 error responses. Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> Signed-off-by: Ioannis Androulidakis <ioannis@arrikto.com>
* Remove the unused, optional `disks` and `pvcMount ksonnet parameters from the notebooks component * Add a key for volumes in the PodTemplateSpec definition and set its default value to an empty list * Update the default container image to "tensorflow-1.12.0-notebook-cpu:v0.4.0" Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> Signed-off-by: Ioannis Androulidakis <ioannis@arrikto.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: If they are not already assigned, you can assign the PR to them by writing 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 |
|
Hi @kimwnasptd. 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 |
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com> Signed-off-by: Ioannis Androulidakis <ioannis@arrikto.com>
ioandr
force-pushed
the
feature-update-notebook-controller
branch
from
fe0d986 to
36d55ff
Compare
|
/test kubeflow-presubmit |
|
/assign @kkasravi |
|
/retest |
|
@kkasravi Any comment on this? @kimwnasptd @ioandr @lluunn Has reimplemented the controller in go #2336 so I think you'll need to make the same changes there once its merged. |
|
Thanks for the heads up @jlewi. We will keep an eye on it and port these changes in a new PR once the new Notebook Controller gets merged. |
|
We thought it was good to give an early warning because these are issues that @lluunn may also bump into, when testing the new Go operator. |
|
Thanks for the heads up |
| restartPolicy: "Always", | ||
| securityContext: { |
There was a problem hiding this comment.
should 100 be a param like params.notebookGid?
|
Obsolete. |
This PR adds various fixes and updates to the
sync-notebookWebhook of the Notebook Controller that is responsible for creating child resources for each Notebook CR. The suggested changes are a prerequisite to ensure that the new, forthcoming Jupyter UI can directly manage Notebook CRs and preserve existing functionality.@jlewi @kam We would really appreciate your perspective on this to make sure we are all on the same page. Hereaupon, we briefly describe the issues we diagnosed while working with the existing Notebook Controller and its Webhook:
The child
Servicedoes not use WebSockets and, thus, terminals and Python interpreters hang inside the JupyterLab UI.HTTP GETrequests from inside the Notebook Pod result in403 Forbiddenresponses.The child
Deploymentalways overrides the Notebook CRtemplateSpecwith thepodTemplateSpecthat contains certain default values. This is not desirable when Users want to spawn a Notebook that overrides default configuration.The child
Deploymentuses singled-quoted True/False values in two container spec args related to the Jupyter Notebook, so their values were treated as String and not as Boolean. This results in the Notebook Pod going toCrashLoopBackOffstatus with the following error in its logs:Related sources:
https://github.com/jupyter/notebook/blob/master/notebook/notebookapp.py#L845
https://github.com/jupyter/notebook/blob/master/notebook/notebookapp.py#L633
NOTE:
According to this issue, root access should not be allowed in a JupyterLab environment. However, the Notebook Controller currently sets the
allow_rootargument toTrue. Which is the desired behavior?The Notebook Pod does not use the latest stable Tensorflow Docker image, as the current JH Spawner UIs do
In addition, since the new Jupyter UI supports Volumes for Notebooks, we suggest the following additions to the Notebook Controller:
podTemplateSpecand set its default value to an empty list, as fallbacksecurityContext(fsGroup) of the Notebook Pod to avoid permission errors when accessing attached volumes from inside the NotebooksdisksandpvcMountksonnet parameters from thenotebooksKF component. In the future, we might need to go over the rest of the defined parameters and cross-check which are currently needed.This change is