Repository navigation
Cache Node metadata and self-heal missed bindings in NodeLabelDelivery - #5501
ryanaoleary wants to merge 3 commits into
Conversation
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>
ryanaoleary
changed the title
|
@cursor review |
|
Skipping Bugbot: Unable to authenticate your request. Please make sure Bugbot is properly installed and configured for this repository. |
| // K8sControllerRuntimeCacheSelectors returns a map[client.Object]cache.ByObject that scopes the manager's | ||
| // informer cache to only watch KubeRay-managed Jobs (filtered by app.kubernetes.io/created-by=kuberay-operator) | ||
| // and Ray node Pods (filtered by ray.io/node-type in head|worker|redis-cleanup). |
There was a problem hiding this comment.
Should also mention the Node cache.
| "The worker Pod %s/%s terminated without the %s annotation, and bound Node %s could not be read: %v.", | ||
| pod.Namespace, pod.Name, utils.RayNodeLabelsAnnotationKey, pod.Spec.NodeName, err) | ||
| } | ||
| for _, ref := range worker.LabelRefs { |
There was a problem hiding this comment.
Maybe extract buildNodeLabels into a shared utility function (e.g. utils.ResolveNodeLabels) and reuse it here so we don't have to maintain the same validation logic in two places.
| // plugin applied the node label after binding or the binding webhook timed out under failurePolicy: Ignore). | ||
| // Because pods/binding only executes once per Pod lifetime, the container cannot recover via in-place restart; | ||
| // once the bound Node carries all requested and allowed labelRefs, the Pod is deleted so it can be recreated. | ||
| func (r *RayClusterReconciler) shouldDeleteWorkerPod(ctx context.Context, pod corev1.Pod, worker *rayv1.WorkerGroupSpec) (bool, string) { |
There was a problem hiding this comment.
I think we should reconsider this behavior, as it changes the existing deletion logic and may cause Pods that would normally be deleted to be retained.
This affects:
restartPolicy: Neverworkers (used by autoscaler v2 with Ray < 2.56) that areFailed, or stillRunningbecause a sidecar is alive. In the latter case, they continue holding GPU/TPU resources.- Multi-host groups, where the entire replica can be retained.
These Pods still count toward the group's replicas, so KubeRay won't replace them. The retain path also provides no event or log to indicate the issue.
Maybe only adding deletion conditions, without changing existing deletion behavior. This fully addresses the Always Pod CrashLoopBackOff issue described in the PR:
shouldDelete, reason := shouldDeletePod(pod, rayv1.WorkerNode)
if shouldDelete || !missedNodeLabelDelivery(pod, worker) {
return shouldDelete, reason
}
WDYT?| return map[client.Object]cache.ByObject{ | ||
| &batchv1.Job{}: {Label: jobSelector}, | ||
| &corev1.Pod{}: {Label: podSelector}, | ||
| &corev1.Node{}: {Transform: cache.TransformStripManagedFields()}, |
There was a problem hiding this comment.
Just curious, for large clusters, how much memory overhead could caching all Node labels and annotations introduce?
Why are these changes needed?
Following #5300, the pod binding webhook still reads each bound Node directly from the API server for worker groups with
labelRefs. In addition, worker pods that bind without theray.io/node-labelsannotation (for example, during a webhook rollout or before a node labeler finishes) fail to start and remain stuck inCrashLoopBackOffbecause Kubernetes only runspods/bindingonce per pod.This PR makes two changes:
Read node metadata from the informer cache
PartialObjectMetadatafrom the manager cache in the binding webhook and fall back to a live API read only onNotFound.NodeLabelDeliveryis enabled, stripmanagedFieldsfrom cached node objects, and addlistandwatchpermissions onnodesto the cluster role.Recreate worker pods after transient missed bindings
ray.io/node-labels, check the bound node in the metadata cache during reconciliation.Related issue number
Closes #5499
Labels
doc-updates-requiredlabel.breaking-changelabel.Checks