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

[RayCluster] Validate RayClusterSpec for empty containers and GCS FT #2749

Merged
merged 4 commits into from
Jan 15, 2025

Conversation

kevin85421
Copy link
Member

@kevin85421 kevin85421 commented Jan 14, 2025

Why are these changes needed?

  1. Ensure PodSpec has at least one container: OpenAPI only validates whether containers exists in the PodSpec or not. However, it doesn't validate whether the PodSpec has at least one container or not. In K8s, it is validated in the server side: https://sourcegraph.com/github.com/kubernetes/kubernetes/-/blob/pkg/apis/core/validation/validation.go?L3635. In KubeRay, we assume that the Ray container is the first container in the Pod. To avoid segfault in KubeRay, this PR checks whether all PodSpec have at least one container or not.

  2. GcsFaultToleranceOptions and the annotation should not be set at the same time.

  3. RAY_REDIS_ADDRESS should not be set if KubeRay doesn't aware of GCS FT is enabled.

  4. Update error messages to encourage users to use new API GcsFaultToleranceOptions.

Related issue number

Follow up #2726

Checks

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

Signed-off-by: kaihsun <[email protected]>
Signed-off-by: kaihsun <[email protected]>
Signed-off-by: kaihsun <[email protected]>
Signed-off-by: kaihsun <[email protected]>
@kevin85421 kevin85421 changed the title WIP [RayCluster] Validate RayClusterSpec for empty containers and GCS FT Jan 14, 2025
@kevin85421 kevin85421 marked this pull request as ready for review January 14, 2025 23:27
@kevin85421
Copy link
Member Author

cc @rueian would you mind reviewing this PR?

@kevin85421
Copy link
Member Author

also cc @CheyuWu since this is the follow up of your PR.

Copy link
Contributor

@rueian rueian left a comment

Choose a reason for hiding this comment

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

LGTM

@kevin85421 kevin85421 merged commit eba1459 into ray-project:master Jan 15, 2025
24 checks passed
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