-
Notifications
You must be signed in to change notification settings - Fork 627
Add default Ray node label info to Ray Pod environment #3699
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 default Ray node label info to Ray Pod environment #3699
Conversation
cc: @MengjinYan |
} | ||
|
||
// add downward API environment variables for Ray default node labels | ||
addDefaultRayNodeLabels(&pod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we guard this logic with a Ray version check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that the value of the default labels might change after the ray node started?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]> Add default Ray node label info to Ray Pod environment Signed-off-by: Ryan O'Leary <[email protected]>
Signed-off-by: Ryan O'Leary <[email protected]>
Signed-off-by: Ryan O'Leary <[email protected]>
Co-authored-by: Kai-Hsun Chen <[email protected]> Signed-off-by: Ryan O'Leary <[email protected]>
Signed-off-by: Ryan O'Leary <[email protected]>
Signed-off-by: Ryan O'Leary <[email protected]>
Signed-off-by: Ryan O'Leary <[email protected]>
daebac4
to
604c162
Compare
cc @andrewsykim, do you have the bandwidth to review this PR? I will give it a final pass after you approve the PR. |
@andrewsykim @kevin85421 Follow up on this PR. cc: @ryanaoleary |
The issue still exists? |
Hmmm.. I don't see why that issue would exist. Let's wait for @ryanaoleary to update the PR based on our latest discussion and hopefully reviewing the code will make it more obviosu if future compatibility issues will be a problem. |
Let me add more details for this one. The Ray Autoscaler must identify the Ray labels associated with a worker group based on the RayCluster CR to scale pods correctly with the associated labels. I will use an example to explain the compatibility issue I am referring to:
If KubeRay introduces changes to how Ray labels are populated, the Ray Autoscaler will require corresponding updates. Would you mind helping me understand why you don't like ask users to specify |
We are still documenting how to use rayStartParams directly to update labels. This change is only simplifying behavior for well-known default labels in Ray. It's not mutually exclusive. We discussed the autoscaling behavior as well. We don't intend to change autoscalier behavior for labels just yet and this PR will not change anything for autoscaling. But Ryan has a doc for future changes. @ryanaoleary can you share the doc with Kai-Hsun as well please? |
Hi, @ryanaoleary can you also share the doc to @rueian and me, too? |
I agree with what @kevin85421 said. |
@Future-Outlier Sorry for the delay, just shared. |
From discussion with @Future-Outlier and @rueian: Using rayStartParams is not a full-proof solution for autoscaling because labels from rayStartParams can be read from file I don't object to deferring this PR, but to be clear, asking users to set labels in |
This means that cc @edoakes @jjyao please revisit the APIs so that we can avoid adding too much complexity in Ray Autoscaler. |
This is the core reason why we need to make some form of change in the KubeRay CR and can't do this entirely transparently based on This won't solve the problem for custom/user-specified labels though. For that, we'd need to introduce a more explicit API such as a The amount of complexity being added for any of the options discussed in this thread is very minor, and the feature will be alpha in both Ray and KubeRay before stabilizing, so there is no "one way door" and I see no harm in starting with the proposed option. If we want to jump directly to a top-level |
Agreed with everything @edoakes said, the level of complexity is pretty low and it's not a one way door. If it helps address some concerns from others, we can also guard the default labeling with a feature gate so we can remove it / break it in the future, but I don't see it being likely / necessary since the feature it already opt-in using pod labels. |
@edoakes thank you for the reply!
What's "this" referring to?
Why we can't reuse
Why it requires a CRD change? Can we reuse
Have you read #3699 (comment)? The contract inconsistency between KubeRay / Ray Autoscaler was a real pain before. That's why I am very uncomfortable with the potential frequent contract changes with this PR if we incrementally auto-populate more and more labels. Some solutions which provide stable contract between KubeRay and Ray Autoscaler I can accept:
|
I'm now remembering after reading the KubeRay CR spec that the parsing logic is actually what we do for resources... my suggestion would be to:
|
This sounds good to me. For For |
The parsing might get a little messy because labels can have non-alphanumeric characters |
Yeah we already have some logic to set the For the two separate issues:
Neither of the above fully address label-based autoscaling though, since we're still requiring users to configure their KubeRay CR specs in order to determine what |
It seems like the support we should implement is as follows:
|
We can consistently use a comma It's not necessarily to use CRD to verify in OpenAPI level. KubeRay also has its own validation logic and nothing will be created if the validation fails. The validation mechanism works quite well. Anyway, I don't object to add a structured field for |
I believe we are no longer considering automatic inference of labels. It is the user's responsibility to set the labels
I proposed the environment variables approach to @MengjinYan several months ago. At that time, we overlooked the Ray Autoscaler and decided to use the Kubernetes Downward API to set environment variables with label values injected by cloud providers at runtime. However, we no longer rely on dynamic label values. We should avoid reading env vars and make
If we can ensure this, the contract between KubeRay and the Ray Autoscaler will remain stable. I am OK with #3699 (comment), but it is still better to avoid reading env vars because it is tech debt that we can still avoid at this moment. |
I'm good with lifting @ryanaoleary we're cutting the release branch for v1.5 in a week, do we want to land this for v1.5 given the new scope? |
SG
It will be helpful if this can be landed in v1.5 so that we can mention it in Ray Summit. |
Yeah I think I can still try to land this for v1.5, I'll try to put out the change quickly and then maybe we can prioritize reviews before the branch cut. If it's not able to be merged in time, we can still use Just so I'm clear on the required change, we'd be adding structured fields to
And then in either the same PR or a follow-up, Labels specified under |
That sounds right. To preserve compatibility, you may need to check if either resources or labels are already set in rayStartParams. |
should we also add |
I think to both |
I think we can start the implementation |
Implemented in #4106, I'll close this PR now since we're no longer inferring |
Add default Ray node label info to Ray Pod environment
Why are these changes needed?
This PR adds market-type information for different cloud providers to the Ray pod environment based on the provided
nodeSelector
value. 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/region
andtopology.k8s.io/zone
on a GKE v1.33 alpha cluster.Related issue number
ray-project/ray#51564
Checks