Add default Ray node label info to Ray Pod environment #3699
Conversation
|
cc: @MengjinYan |
| @@ -493,6 +493,9 @@ func BuildPod(ctx context.Context, podTemplateSpec corev1.PodTemplateSpec, rayNo | |||
| initLivenessAndReadinessProbe(&pod.Spec.Containers[utils.RayContainerIndex], rayNodeType, creatorCRDType) | |||
| } | |||
|
|
|||
| // add downward API environment variables for Ray default node labels | |||
| addDefaultRayNodeLabels(&pod) | |||
There was a problem hiding this comment.
Should we guard this logic with a Ray version check?
There was a problem hiding this comment.
This code doesn't rely on any API change from Ray, it just sets some env vars and the actual node labels get set in Ray core using those vars, but I can add a version guard here (I guess for whatever version ray-project/ray#53360 is included in) if we don't want it setting any unused vars for users on older versions of Ray.
There was a problem hiding this comment.
From offline discussion with @MengjinYan we were leaning towards not including a version guard, since users are not required to specify the Ray version they're using in the CR spec
There was a problem hiding this comment.
LTGM @kevin85421 Can take a look from Kuberay's perspective?
| pod.Spec.Containers[utils.RayContainerIndex].Env, | ||
| // used to set the ray.io/market-type node label | ||
| corev1.EnvVar{ | ||
| Name: "RAY_NODE_MARKET_TYPE", |
There was a problem hiding this comment.
Is the plan for Ray Core to check these env vars? Can you link the PR that includes this change?
There was a problem hiding this comment.
Yeah that's correct - this is the related PR: ray-project/ray#53360
@kevin85421 Bumping this to see if we can include it in 1.4 |
|
@ryanaoleary I chatted with @MengjinYan, and my understanding is that this doesn’t need to be included in v1.4.0. Could you sync with @MengjinYan and let me know if I’m mistaken? Thanks! |
Synced offline with @MengjinYan and yeah there's no urgency to include this in v1.4.0, we can wait to include it in the next release. My thought was just that it'd be useful to have this functionality in the soonest stable release for testing, but I can just use the nightly image. |
This makes sense. I’ll review it for now. We’ll make a best-effort attempt to include this PR, but there’s no guarantee. |
|
|
||
| // getRayMarketTypeFromNodeSelector is a helper function to determine the ray.io/market-type label | ||
| // based on user-provided Kubernetes nodeSelector values. | ||
| func getRayMarketTypeFromNodeSelector(pod *corev1.Pod) string { |
There was a problem hiding this comment.
I guess some schedulers or webhooks may update the node selector after the Pod is created. We should take it into consideration.
There was a problem hiding this comment.
Do you mean that the value of the default labels might change after the ray node started?
There was a problem hiding this comment.
might change after the ray node started?
No, I mean it may be changed after KubeRay constructs the Pod spec but before the Pod is created or scheduled.
There was a problem hiding this comment.
Are there any webhooks that modify the value of cloud.google.com/gke-spot on a Pod? I'm having difficulty finding a reference. I think we should default to adding Ray node labels based on what the user specifies in their Pod spec.
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com> Add default Ray node label info to Ray Pod environment Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
Co-authored-by: Kai-Hsun Chen <kaihsun@apache.org> Signed-off-by: Ryan O'Leary <113500783+ryanaoleary@users.noreply.github.com>
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
daebac4 to
604c162
Compare
Add default Ray node label info to Ray Pod env 8000 ironment
Why are these changes needed?
This PR adds market-type information for different cloud providers to the Ray pod environment based on the provided
nodeSelectorvalue. This PR also adds environment variables to pass region and zone information using downward API (kubernetes/kubernetes#127092). These environment variables will be used in Ray core to set default Ray node labels.I'll add a comment below with my manual test results with propagating
topology.k8s.io/regionandtopology.k8s.io/zoneon a GKE v1.33 alpha cluster.Related issue number
ray-project/ray#51564
Checks