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

[Feature] Make head serviceType optional #851

Merged
merged 4 commits into from
Jan 1, 2023

Conversation

kevin85421
Copy link
Member

@kevin85421 kevin85421 commented Dec 30, 2022

Why are these changes needed?

Minimal configuration for RayCluster CRs should be as simple as possible. This PR makes serviceType optional and the default is ClusterIP.

(from Kubernetes official documentation)

ClusterIP: Exposes the Service on a cluster-internal IP. Choosing this value makes the Service only reachable from within the cluster. This is the default that is used if you don't explicitly specify a type for a Service.

Related issue number

Part of #368

Checks

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

Test Helm chart

# Build docker image and install KubeRay operator

# Install RayCluster chart (path: helm-chart/ray-cluster)
helm install raycluster .

kubectl get svc
# NAME                          TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)                                         AGE
# kuberay-operator              ClusterIP   10.96.148.109   <none>        8080/TCP                                        3h57m
# kubernetes                    ClusterIP   10.96.0.1       <none>        443/TCP                                         3h59m
# raycluster-kuberay-head-svc   ClusterIP   10.96.88.220    <none>        10001/TCP,6379/TCP,8265/TCP,8080/TCP,8000/TCP   5s
  • TYPE of raycluster-kuberay-head-svc is ClusterIP.

@kevin85421 kevin85421 changed the title WIP [Feature] Make head serviceType optional Dec 30, 2022
@kevin85421 kevin85421 marked this pull request as ready for review December 30, 2022 16:04
@kevin85421
Copy link
Member Author

I currently remove all serviceType from sample YAML files. We can choose some YAML files to write comments about the field serviceType. @DmitriGekhtman do you have any suggestion about which YAML files to keep the comments on? Thank you!

@DmitriGekhtman
Copy link
Collaborator

DmitriGekhtman commented Dec 30, 2022

I currently remove all serviceType from sample YAML files. We can choose some YAML files to write comments about the field serviceType. @DmitriGekhtman do you have any suggestion about which YAML files to keep the comments on? Thank you!

raycluster.complete.yaml and raycluster.complete.large.yaml

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.

Seems all reference have been removed. Actually this is optional but not deprecated at this moment. Let's leave some example for customization if user need them

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.

/lgtm

@DmitriGekhtman DmitriGekhtman merged commit 3aebd8c into ray-project:master Jan 1, 2023
@kevin85421 kevin85421 added this to the v0.5.0 release milestone Jan 2, 2023
@rentom
Copy link

rentom commented Jan 3, 2023

Following Getting Started and the KubeRay-operator was deployed just fine, but

$ kubectl apply -f ray-cluster.autoscaler.yaml
The RayCluster "raycluster-autoscaler" is invalid: spec.headGroupSpec.serviceType: Required value

@kevin85421
Copy link
Member Author

Following Getting Started and the KubeRay-operator was deployed just fine, but

$ kubectl apply -f ray-cluster.autoscaler.yaml
The RayCluster "raycluster-autoscaler" is invalid: spec.headGroupSpec.serviceType: Required value

Oh, the current configuration is only compatible with the latest KubeRay image (i.e. nightly). The document deploys KubeRay operator with the image v0.3.0. Hence, serviceType is a required value. I will open a PR to add serviceType back to sample configurations to keep backward compatibility. Thank @rentom for pointing this out!

@DmitriGekhtman
Copy link
Collaborator

Following Getting Started and the KubeRay-operator was deployed just fine, but

$ kubectl apply -f ray-cluster.autoscaler.yaml

The RayCluster "raycluster-autoscaler" is invalid: spec.headGroupSpec.serviceType: Required value

Oh, the current configuration is only compatible with the latest KubeRay image (i.e. nightly). The document deploys KubeRay operator with the image v0.3.0. Hence, serviceType is a required value. I will open a PR to add serviceType back to sample configurations to keep backward compatibility. Thank @rentom for pointing this out!

Could you instead pin the KubeRay configs to the latest KubeRay release, rather than master?

@kevin85421
Copy link
Member Author

Could you instead pin the KubeRay configs to the latest KubeRay release, rather than master?

Open issue #857 to track this. We need to promise that the nightly and latest release KubeRay operator will work well with current sample YAML files.

@DmitriGekhtman
Copy link
Collaborator

We need to promise that the nightly and latest release KubeRay operator will work well with current sample YAML files.

Makes sense!
But also when possible we should pin commits, images, etc. in the docs so that nothing suddenly breaks.

lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
Minimal configuration for RayCluster CRs should be as simple as possible. This PR makes serviceType optional and the default is ClusterIP.
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.

5 participants