Fix #285 and make few tweaks to notebook Dockerfile

[inc0]

Besides adding chown /home/$USER also move non-install layers lower in
Dockerfile to make better use of docker cache during edits
Later we will probably switch to root user (issue #300), so it will make this whole section redundant.
Aslo touches #101

Fixes #285

---

This change is 

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

Assign the PR to them by writing /assign in a comment when ready.

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:

• OWNERS

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

[inc0]

note @jlewi - we'll need to re-push images to hub after it merges

[jlewi]

Why are you no longer changing the ownership of the CONDA_DIR and JULIA_PKG_DIR.

Can you explain what the new code does differently? It looks like the only difference is that instead of doing chown with the UID we use the username now.

Do you know why that makes a difference?

[inc0]

Difference is that, instead of changing only CONDA_DIR and JULIA_PKG_DIR (both of which lives in /home/$NB_USER) we chown whole /home/$NB_USER - most importantly /home/$NB_USER/work

Because we change user here in the script (line 48) and not in kubernetes manifest, PV isn't connected with correct user ownership, it assumes root (if you look at our notebook pod manifesto, it explicitly runs container as UID 0). I'm doing chown in startup script instead.

[jlewi]

Do you know why run the container as UID 0? Should we not be doing that?

The original code was

for d in "$CONDA_DIR" "$JULIA_PKGDIR" "/home/$NB_USER"; do

So it chown'd the whole /home/$NB_USER dir.

It looks like the difference is that you are now doing this always as opposed to

if [ "$NB_UID" != $(id -u $NB_USER) ]

What I'm inferring from this is that NB_UID = id NB_USER therefore in the current startup script we are not executing the chown.

However, for some reason the permsions on /home/${NB_USER} are not set correctly to allow that user to use that container.

If UID is 0, doesn't that mean we are running as root?

[inc0]

Ah you're right, I'll restore CONDA_DIR etc, issue was "if" above.

I think we shouldn't run it as root, maybe ensure perms for these dirs in Dockerfile. I'll open separate issue for that one tho as someone did that on purpose:)

Yeah we run pod as root, but start.sh does su $NB_USER <> so we run jupyter itself as NB_USER. Not a typical way to deal with it tbh.

[jlewi]

/assign

[jlewi]

/assign jlewi

[inc0]

If we run as root ( #300 ) this issue wouldn't happen too

[jlewi]

Looks good but can you update the PR description and include links to relevant issues and the slack link as well.

[inc0]

Done, I linked #101 which has slack link in discussion

[jlewi]

/assign @willb

Adding Will because apparently running as root causes problems for open shift.

[jlewi]

@inc0 clued me in

• The pod is already running with runAsUser:0
  Here's a gist for a pod showing it to be true

• We can't figure out where this is being set; it doesn't appear to be in our config and we didn't see it in KubeSpawner

• Nothing in this PR is changing how we run as root so it shouldn't have any impact on OpenShift (its possible things are already broken on OpenShift)

• All this PR does is chown the /home/$NB_USER but this is only done if the user is root (its guarded by an if) so it shouldn't have any impact in the case where we don't run as root.

• If we set the uid/gid of the pod correctly then we think K8s will automatically set the permissions on the PV.

• So this PR fixes a bug with PVC not working and shouldn't have any other impact. So I'm merging it.