Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mount of pvc #270

Merged
merged 2 commits into from Feb 23, 2018
Merged

Add mount of pvc #270

merged 2 commits into from Feb 23, 2018

Conversation

inc0
Copy link

@inc0 inc0 commented Feb 20, 2018

While we were adding pvc for every jupyter instance, we didn't mount it
anywhere.
Let's mount it to work dir, as I assume this is dir where users will
likely put their notebooks. This will ensure that work will be retained
even if pod dies.

Fix #19
Fix #22


This change is Reviewable

While we were adding pvc for every jupyter instance, we didn't mount it
anywhere.
Let's mount it to work dir, as I assume this is dir where users will
likely put their notebooks. This will ensure that work will be retained
even if pod dies.
]
c.KubeSpawner.volume_mounts = [
{
'mountPath': '/home/jovyan/work',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should everything in /home be on the PVC?

Copy link
Author

Choose a reason for hiding this comment

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

well we have tf examples there too, which I think is a nice touch, and that would disappear.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about about putting it under, /mnt ? I'd like to make it more obvious which directories correspond to PD and which don't.

Copy link
Author

Choose a reason for hiding this comment

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

jupyter tree lands in /home/jovyan, so if we put it in /mnt it won't be obvious how to use it by users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Please address other comments.

@@ -91,3 +91,18 @@ def extra_resource_limits(self):
# How much disk space do we want?
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to uncomment the line above that sets singleuser_fs_gid
#22 (comment)

Did you test if this works?

Copy link
Author

Choose a reason for hiding this comment

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

yup, just tested more thoroughly:

  1. created notebook in work dir
  2. deleted jupyter pod
  3. recreated pod
  4. notebook was there

Copy link
Author

Choose a reason for hiding this comment

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

AD. singleuser_fs_gid - just checked - we run container as root. That makes it work without gid gimmicks but it's different issue on it's own:)

@jlewi
Copy link
Contributor

jlewi commented Feb 21, 2018

/ok-to-test

@jlewi
Copy link
Contributor

jlewi commented Feb 22, 2018

@inc0 please open an issue to add an E2E test to verify that pods actually mount the PVC.

It looks like there was a regression when we switched from YAML to ksonnet. #22 had added this feature to our manifests. So it would be good to have an E2E test to prevent further regressions.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@jlewi jlewi merged commit 3cb128f into kubeflow:master Feb 23, 2018
yanniszark pushed a commit to arrikto/kubeflow that referenced this pull request Nov 1, 2019
yanniszark pushed a commit to arrikto/kubeflow that referenced this pull request Feb 15, 2021
…es to vizier-core (kubeflow#270)

* Ensure vizier-core never been stuck too long waiting for DB conn

Signed-off-by: Koichiro Den <den@valinux.co.jp>

* Add standard Health gRPC service

Signed-off-by: Koichiro Den <den@valinux.co.jp>

* Change db.New to return error instead of exit(1) with log.Fatal

Signed-off-by: Koichiro Den <den@valinux.co.jp>

* Add SelectOne() to VizierDBInterface

Signed-off-by: Koichiro Den <den@valinux.co.jp>

* Rename import for later convenience

Signed-off-by: Koichiro Den <den@valinux.co.jp>

* Implement and register Health Server for Katib manager

Signed-off-by: Koichiro Den <den@valinux.co.jp>

* Add readiness/liveness probes to vizier-core

Signed-off-by: Koichiro Den <den@valinux.co.jp>

* Update test codebase

Fixes: 61ac5607353 ("Add SelectOne() to VizierDBInterface")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
elenzio9 pushed a commit to arrikto/kubeflow that referenced this pull request Oct 31, 2022
* Add Bobgy to kubeflow ci-team

* Update ci-viewer.members.txt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants