IA-2965 update home dir for jupyter #2492

Qi77Qi · 2022-01-06T17:52:54Z

https://broadworkbench.atlassian.net/browse/IA-2756
https://broadworkbench.atlassian.net/browse/IA-2965

Main changes:

PD is now mapped to $HOME inside Jupyter container
<work>/.jupyter dir on the VM is deleted when runtimes are deleted so that when we create a new one, new jupyter process have a clean start
Jupyter container is restarted in startup script becuz we're resetting PYTHONPATH and PATH to new path that doesn't have /notebooks. Because of this we need to re-copy startup script into the new container, and re-fix welder path for dataproc.
We're updating jupyter_notebook_config.py so that it doesn't have /notebooks as dir for notebooks

Tested manually:
Create a VM with develop branch that has home/jupyter/notebooks as work dir. Deploy a new leo with this branch, and see if the following works as expected on the same VM

pausing
resuming
deleting but keep the VM, and create a new VM with the same PD

Have you read CONTRIBUTING.md lately? If not, do that first.

I, the developer opening this PR, do solemnly pinky swear that:

I've documented my API changes in Swagger

In all cases:

Get a thumbsworth of review and PO signoff if necessary
Verify all tests go green (you can re-run automation tests with a comment saying jenkins retest
Run the automation tests multiple times in parallel to weed out instability if applicable via a comment saying multi-test
Squash and merge; Delete your branch after this
Test this change deployed correctly and works on dev environment after deployment

Qi77Qi · 2022-01-08T02:54:18Z

jenkins retest

Qi77Qi · 2022-01-14T04:53:07Z

jenkins retest

codecov · 2022-01-14T15:58:42Z

Codecov Report

Merging #2492 (512e6b7) into develop (491e128) will increase coverage by 0.01%.
The diff coverage is 87.50%.

@@             Coverage Diff             @@
##           develop    #2492      +/-   ##
===========================================
+ Coverage    75.30%   75.31%   +0.01%     
===========================================
  Files          113      113              
  Lines         8705     8711       +6     
  Branches       148      144       -4     
===========================================
+ Hits          6555     6561       +6     
  Misses        2150     2150

Impacted Files	Coverage Δ
.../workbench/leonardo/util/DataprocInterpreter.scala	`59.85% <33.33%> (ø)`
...rkbench/leonardo/util/BaseRuntimeInterpreter.scala	`66.66% <100.00%> (+0.34%)`	⬆️
...te/dsde/workbench/leonardo/util/BucketHelper.scala	`74.71% <100.00%> (+1.21%)`	⬆️
.../dsde/workbench/leonardo/util/GceInterpreter.scala	`72.89% <100.00%> (ø)`
...orkbench/leonardo/util/RuntimeTemplateValues.scala	`97.84% <100.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 491e128...512e6b7. Read the comment docs.

...c/test/scala/org/broadinstitute/dsde/workbench/leonardo/notebooks/NotebookPyKernelSpec.scala

...rc/test/scala/org/broadinstitute/dsde/workbench/leonardo/notebooks/NotebookRKernelSpec.scala

http/src/main/resources/init-resources/startup.sh

jdcanas · 2022-01-21T17:04:44Z

http/src/main/resources/init-resources/jupyter-docker-compose-gce.yaml

@@ -31,6 +31,10 @@ services:
      PIP_USER: "false"
      PIP_TARGET: "${NOTEBOOKS_DIR}/packages"
      R_LIBS: "${NOTEBOOKS_DIR}/packages"
+      # The next two lines aren't great. But they're for updating PYTHONPATH, PATH in older than (inclusive) us.gcr.io/broad-dsp-gcr-public/terra-jupyter-base:1.0.2
+      # We should remove the two lines once we no longer support older images. In the meantime, we need to be careful updating Jupyter base images.


Would be nice if we could link a ticket here. Seems like one of those things we'd forget to update even if we did no longer support the older images

it's a bit unclear what the condition will be for the ticket. So for now, I'm adding a comment and added date when the comment is added

jdcanas · 2022-01-21T17:07:00Z

http/src/main/resources/init-resources/startup.sh

@@ -68,6 +67,7 @@ export START_USER_SCRIPT_URI=$(startUserScriptUri)
 export START_USER_SCRIPT_OUTPUT_URI=$(startUserScriptOutputUri)
 export WELDER_MEM_LIMIT=$(welderMemLimit)
 export MEM_LIMIT=$(memLimit)
+export INIT_BUCKET_NAME=$(initBucketName)
 export USE_GCE_STARTUP_SCRIPT=$(useGceStartupScript)
 GPU_ENABLED=$(gpuEnabled)
 export IS_RSTUDIO_RUNTIME="false" # TODO: update to commented out code once we release Rmd file syncing


Not related to this ticket, but is this TODO still valid?

RMD file syncing is released currently as far as I'm aware. cc @gpcarr

I think it's turned off for now. Once @gpcarr 's current rmd ticket is done, we should be able to turn it back on

yeah it's still valid - we're hardcoding to false until everything is in place for Rmd file syncing. I'm not sure we'd be able to turn it on when my backend changes are done though, I think we'd need to wait for the frontend to be ready as well

jdcanas · 2022-01-21T17:25:36Z

http/src/main/resources/init-resources/startup.sh

+# This condition assumes Dataproc's cert directory is different from GCE's cert directory, a better condition would be
+# a dedicated flag that distinguishes gce and dataproc. But this will do for now
+# If it's GCE, we resize the PD. Dataproc doesn't have PD
+if [ -f "$FILE" ]; then


maybe make a ticket to template the cloudService in here, would be nice not too rely on this as it could change and break this in an unexpected way;

https://broadworkbench.atlassian.net/browse/IA-3186

jdcanas · 2022-01-21T17:27:06Z

.../src/main/scala/org/broadinstitute/dsde/workbench/leonardo/util/BaseRuntimeInterpreter.scala

@@ -209,7 +213,8 @@ abstract private[util] class BaseRuntimeInterpreter[F[_]](
  }

  // Shutdown script to run after the runtime is paused
-  protected def getShutdownScript(runtimeAndRuntimeConfig: RuntimeAndRuntimeConfig): F[Map[String, String]] = {
+  protected def getShutdownScript(runtimeAndRuntimeConfig: RuntimeAndRuntimeConfig,
+                                  shouldDeleteJupyterDir: Boolean): F[Map[String, String]] = {


what exactly is the use case for this flag?

Is the intent to cleanup old /notebooks dirs that exist from before this change? depending on how important the use case is, might be worth having a test where this is true to verify the functionality (I see tests where it is true, but not sure if its verified)

The flag is to tell whether shutdown script should delete .jupyter dir during shutting down. It's only true when it's GCE VM and user wants to delete the VM.

It'll have issue if we create runtimes with different images using the same PD. Will update RuntimeCreationDiskSpec to verify that

jdcanas

Mostly looks good, a couple nits

This reverts commit a017f1d.

Qi77Qi · 2022-01-24T13:56:21Z

multi-test

Qi77Qi · 2022-01-24T18:48:20Z

multi-test

Qi77Qi · 2022-01-24T18:49:06Z

out of the 3 multi test runs, 1 passed, 1 failed at fiab-start, 1 failed with one test failure due to Sam error

...test/scala/org/broadinstitute/dsde/workbench/leonardo/runtimes/RuntimeCreationDiskSpec.scala

http/src/main/resources/init-resources/gce-init.sh

http/src/main/resources/init-resources/shutdown.sh

gpcarr

lgtm

Qi77Qi force-pushed the pdMount branch 2 times, most recently from cf7804a to e601f31 Compare January 8, 2022 01:24

Qi77Qi force-pushed the pdMount branch from 919384a to 27c9b86 Compare January 12, 2022 18:56

Qi77Qi requested review from rtitle, jdcanas and gpcarr January 14, 2022 16:08

Qi77Qi mentioned this pull request Jan 14, 2022

[IA-2965] Update jupyter home dir DataBiosphere/terra-ui#2768

Merged

Qi77Qi force-pushed the pdMount branch from 748b832 to 38e1bd7 Compare January 14, 2022 20:22

gpcarr reviewed Jan 18, 2022

View reviewed changes

...c/test/scala/org/broadinstitute/dsde/workbench/leonardo/notebooks/NotebookPyKernelSpec.scala Show resolved Hide resolved

gpcarr reviewed Jan 18, 2022

View reviewed changes

...rc/test/scala/org/broadinstitute/dsde/workbench/leonardo/notebooks/NotebookRKernelSpec.scala Show resolved Hide resolved

gpcarr reviewed Jan 18, 2022

View reviewed changes

http/src/main/resources/init-resources/startup.sh Outdated Show resolved Hide resolved

Qi77Qi force-pushed the pdMount branch from 38e1bd7 to 3d2d29f Compare January 19, 2022 14:56

jdcanas reviewed Jan 21, 2022

View reviewed changes

jdcanas approved these changes Jan 21, 2022

View reviewed changes

Qi77Qi force-pushed the pdMount branch from 3d2d29f to 195a740 Compare January 21, 2022 20:46

Qi77Qi added 9 commits January 24, 2022 08:55

update home dir

a3339da

bump 2 dependencies

f1c022b

fix test

7a9080c

fix tests

0f78c32

fix a few

45cad10

turns out removing old container is important

61e2d4f

export envs

e736fd1

fix PYTHONPATH and PATH

53dc3e1

fix startup scirpt path

7d1c5ae

Qi77Qi added 10 commits January 24, 2022 08:55

Revert "fix startup scirpt path"

2d34939

This reverts commit a017f1d.

fix start up script

85f927e

fix for RStudio

776fe6c

fix dataproc

2717e5d

add wait

d7d86d2

fix patchspec failure

367e2d6

fix dataproc startup

3553512

clean up

1514d7a

bump a few deps

114eda6

address comments

512e6b7

Qi77Qi force-pushed the pdMount branch from 195a740 to 512e6b7 Compare January 24, 2022 13:56

gpcarr reviewed Jan 24, 2022

View reviewed changes

...test/scala/org/broadinstitute/dsde/workbench/leonardo/runtimes/RuntimeCreationDiskSpec.scala Show resolved Hide resolved

gpcarr reviewed Jan 24, 2022

View reviewed changes

http/src/main/resources/init-resources/gce-init.sh Show resolved Hide resolved

gpcarr reviewed Jan 24, 2022

View reviewed changes

http/src/main/resources/init-resources/shutdown.sh Show resolved Hide resolved

gpcarr approved these changes Jan 24, 2022

View reviewed changes

Qi77Qi merged commit 1ebc44e into develop Jan 24, 2022

Qi77Qi deleted the pdMount branch January 24, 2022 20:11

Qi77Qi changed the title ~~IA-2756 update home dir for jupyter~~ IA-2965 update home dir for jupyter Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IA-2965 update home dir for jupyter #2492

IA-2965 update home dir for jupyter #2492

Qi77Qi commented Jan 6, 2022 •

edited

Qi77Qi commented Jan 8, 2022

Qi77Qi commented Jan 14, 2022

codecov bot commented Jan 14, 2022 •

edited

jdcanas Jan 21, 2022

Qi77Qi Jan 21, 2022

jdcanas Jan 21, 2022

Qi77Qi Jan 21, 2022

gpcarr Jan 24, 2022

jdcanas Jan 21, 2022

Qi77Qi Jan 21, 2022

jdcanas Jan 21, 2022 •

edited

Qi77Qi Jan 21, 2022

jdcanas left a comment

Qi77Qi commented Jan 24, 2022

Qi77Qi commented Jan 24, 2022

Qi77Qi commented Jan 24, 2022

gpcarr left a comment

IA-2965 update home dir for jupyter #2492

IA-2965 update home dir for jupyter #2492

Conversation

Qi77Qi commented Jan 6, 2022 • edited

Qi77Qi commented Jan 8, 2022

Qi77Qi commented Jan 14, 2022

codecov bot commented Jan 14, 2022 • edited

Codecov Report

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

jdcanas Jan 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdcanas left a comment

Choose a reason for hiding this comment

Qi77Qi commented Jan 24, 2022

Qi77Qi commented Jan 24, 2022

Qi77Qi commented Jan 24, 2022

gpcarr left a comment

Choose a reason for hiding this comment

Qi77Qi commented Jan 6, 2022 •

edited

codecov bot commented Jan 14, 2022 •

edited

jdcanas Jan 21, 2022 •

edited