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

adding volumes and volume mounts to jupyter and worker configs #102

Merged
merged 5 commits into from
Sep 14, 2020

Conversation

lbrindze
Copy link
Contributor

@lbrindze lbrindze commented Sep 4, 2020

Adds volume and volume mount configs to dask helm template. #76

@darothen
Copy link

darothen commented Sep 4, 2020

Hey @TomAugspurger and @jacobtomlinson, we were working on some dask integration details for an internal project and wanted to share the fruits of our labor with the broader community. We didn't see a contributor's guide here, can you guys help us review and (hopefully!) merge this new feature in?

@lbrindze
Copy link
Contributor Author

lbrindze commented Sep 4, 2020

Also fwiw we (@jminsk-cc, @darothen and myself, as well as other members of the climacell team), are more than happy to help contribute in other helm related PRs since we are beginning to make this a key part of our data platform moving forward.

Copy link

@jminsk-cc jminsk-cc left a comment

Choose a reason for hiding this comment

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

More than happy to work on any other problems. Ran into the issue of not having an easy way to mount volumes onto workers. Accidentally left a review.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks Climacell team!

You may also want to be aware of #78

Just a couple of comments, could you add descriptions to the new config options and regenerate the README?

dask/values.yaml Outdated Show resolved Hide resolved
dask/values.yaml Outdated Show resolved Hide resolved
@lbrindze
Copy link
Contributor Author

lbrindze commented Sep 9, 2020

@jacobtomlinson Sounds good I will update this shortly. I just took a look at #78 and didn't see it before. Do we have a preference here in terms of passing the existing kubernetes yaml api through vs specifically overriding specific variables and passing them one at a time. As this overlaps some with #78 I want to make sure we are only going forward with 1 solution here so any recommendations on how to best proceed here?

@jacobtomlinson
Copy link
Member

@lbrindze I see no reason why we couldn't do both.

My general preference is to pass the k8s YAML API through, but having some bespoke options to simplify things for folks new to k8s is also nice to have.

@lbrindze
Copy link
Contributor Author

@jacobtomlinson updated with the readme comments so if you think both options are good and will be ok to support moving forward I think this is ready to merge (pending any other feedback of course).

@jacobtomlinson
Copy link
Member

Very happy to see this thanks @lbrindze

@jacobtomlinson jacobtomlinson merged commit 14a5adf into dask:master Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants