-
Notifications
You must be signed in to change notification settings - Fork 364
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
feat: enrich RayCluster status with head IPs #468
Conversation
Hi, Not sure how the ApiServer is communicating with the head nodes, but after viewing this PR I thought that the headNode's service could be a safer choice instead of the headNode's IP. The Kubernetes service is resolvable automatically within the Kubernetes network domain and it should guarantee delivery to the pod since it is a resource managed by Kubernetes itself |
I am not sure that adding the IP address of the headnode as an annotation is a good practice. Especially that IP address are ephemeral and can change. Plus the IP address is already part of the pod status
Why not as Christos mentioned use the service name. Plus if a component can read the annotations is there a reason why it cannot read the status as well and get the IP address. Finally the IP is assigned at a later stage after the pod is created by the Ray Operator. So the Ray Operator might not be the best entity to read the IP and annotate the pod with it (unless we poll the API-Server to get the IP). Can you please elaborate more on why this change is needed? Thanks! |
Thanks for reviewing. I just want the API to include the head IP when getting or listing clusters so that users don’t have to do it themselves which requires knowledge of K8s. Should the API itself do all the work of getting the IP by looking for the head Pod instead of the controller? If so, is it OK that the API does this work each time it lists or gets clusters or should it save the IP somewhere? regarding pod vs service IP, my use case is GKE and clients that will connect to the head pod from outside the K8s cluster. In this case the Service IP is not routable outside the cluster. We need to connect directly to the Pod IP. Any ideas how to accommodate this use case? |
Can we make API return both Pod and Service IP? |
@davidxia for the GKE case, when you need external access to the cluster, an alternative is to use a I am not exactly sure what you mean by wanting the API to expose the IP address. Would something like |
Yes that kubectl command is what I’m doing now. But I’m hoping kuberay can abstract away more of K8s implementation details from user. In particular I wanted the API endpoints like this https://github.com/ray-project/kuberay/tree/master/apiserver to return the head IP in cluster entities. Does that make sense?
At Spotify we use a single GCP VPC and GKE clusters connected to that VPC. So all Pod IPs are routable from within the VPC including offices and VPN. We don’t use Service with load balancer because for a long time GCP didn’t support internal load balancers whose IPs are routable globally. They were either routable globally but exposed to public Internet or internal but regional. Because of how we set up our project permissions, creating Service with internal load balancers now for many RayClusters is difficult and also I’m not sure the KubeRay operator supports this. That’s why I’m hoping you’re open to adding both Service and Pod IP here. Otherwise, we can’t really use this feature even if it’s accepted. happy to provide more context! |
@DmitriGekhtman , I agree, the IP is a runtime generated value, and having it in the status field makes sense to me. |
Great I can rework this to use the status field. How does the following look? Mind if I include both Pod and service IP for reasons described above?
|
That schema looks good to me. |
f4338b4
to
ab1301b
Compare
@DmitriGekhtman I just updated to use |
can we change the PR name, from |
Thanks for the initial review. Shall I go ahead and write tests? I also noticed the compatibility test fail. Is that expected? |
like so. Missing IPs will be excluded. ``` status: head: podIP: string serviceIP: string ``` Motivation for this change is so that users can get the head IP from the RayCluster CRD directly without knowing the underlying implementation details of kuberay. In a later commit we update the API server to parse the status and return the IPs for each cluster.
Added unit tests. Ready for review. |
like so. Missing IPs will be excluded. ``` status: head: podIP: string serviceIP: string ``` Motivation for this change is so that users can get the head IP from the RayCluster CRD directly without knowing the underlying implementation details of kuberay. In a later commit we update the API server to parse the status and return the IPs for each cluster.
like so. Missing IPs will be excluded.
Motivation for this change is so that users can get the head IP from the
RayCluster CRD directly without knowing the underlying implementation
details of kuberay.
In a later commit we update the API server to parse the status
and return the IPs for each cluster.
Checks