-
Notifications
You must be signed in to change notification settings - Fork 432
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 support for pvcs to apiserver #1118
Conversation
cc @Jeffwan |
Hi @psschwei I am not sure I fully understand how this is supposed to work. Are you assuming that PVC is created and then passed to the cluster creation? I do not think this will work. I was assuming a different scenario, where a PVC is created for each pod during pod creation. Do we need to discuss and finalize the requirements? |
My goal for this PR was really just to implement this missing functionality to be able to use PVCs in the |
cc @blublinsky would you mind reviewing this PR? Thanks! |
Hi @psschwei, would you mind resolving the conflict? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good. The question still remains of how to properly use it. It requires RWM volumes, because the same volume is mounted to multiple pods. The PR can benefit from additional documentation on how to use it and maybe an example
Test failures don't appear to be related to my updates Will work on pulling together an example and some documentation |
Sorry about that. The CI issue should be fixed by #1145. Would you mind rebasing with the master branch? However, I found the nightly compatibility to be quite unstable. There may have been some recent breaking changes in the Ray master branch. Thanks! |
cc @scarlet25151 @Jeffwan would you mind reviewing this PR? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the config is straight forward and LGTM. Please look into the lint issue in test file and then we may merge it.
Signed-off-by: Paul S. Schweigert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not reviewed this PR, but two reviewers approve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for late. This change looks good to me.
Signed-off-by: Paul S. Schweigert <[email protected]>
Why are these changes needed?
Users may wish to add PVC volumes to Ray pods instead of hostPath volumes as its recommended to avoid hostPath volumes when possible due to security risks. Plus, it was something already marked to be implemented.
Related issue number
#1087
Checks