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 support for pvcs to apiserver #1118

Merged
merged 1 commit into from
Jun 6, 2023
Merged

Conversation

psschwei
Copy link
Contributor

@psschwei psschwei commented May 26, 2023

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

  • [ X ] I've made sure the tests are passing.
  • Testing Strategy
    • [ X ] Unit tests
    • Manual tests
    • This PR is not tested :(

@psschwei
Copy link
Contributor Author

cc @Jeffwan

@blublinsky
Copy link
Contributor

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?

@psschwei
Copy link
Contributor Author

My goal for this PR was really just to implement this missing functionality to be able to use PVCs in the buildVols() function. Agree with you on the final scenario (PVC for each pod during creation), consider this the first step down the road towards it.

@kevin85421
Copy link
Member

cc @blublinsky would you mind reviewing this PR? Thanks!

@kevin85421
Copy link
Member

Hi @psschwei, would you mind resolving the conflict?

Copy link
Contributor

@blublinsky blublinsky left a 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

@psschwei
Copy link
Contributor Author

psschwei commented Jun 6, 2023

Test failures don't appear to be related to my updates

Will work on pulling together an example and some documentation

@kevin85421
Copy link
Member

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!

@kevin85421
Copy link
Member

cc @scarlet25151 @Jeffwan would you mind reviewing this PR? Thanks!

Copy link
Collaborator

@scarlet25151 scarlet25151 left a 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 <paul@paulschweigert.com>
Copy link
Member

@kevin85421 kevin85421 left a 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.

Copy link
Collaborator

@Jeffwan Jeffwan left a 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.

@Jeffwan Jeffwan merged commit 2de3fe5 into ray-project:master Jun 6, 2023
18 of 19 checks passed
@psschwei psschwei deleted the add-pvc branch June 7, 2023 00:05
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
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.

None yet

5 participants