[Bug] Reconcile the head service selector when head Pod labels change (#2564)

[stantheman0128]

Why are these changes needed?

The head service selector is not constant. BuildServiceForHeadPod keys it off HeadServiceLabels, and two of those keys, app.kubernetes.io/name and app.kubernetes.io/created-by, can be overridden on the head Pod. reconcileHeadService only built the service when it was missing and returned early whenever one already existed, so the selector kept the value the cluster was created with.

Rename the label on a running RayCluster and the head Pod that comes back after an eviction or a node drain carries the new value while the service still points at the old one. The service then selects nothing and the head is unreachable. Deleting the service does not recover it either: since #2166 the operator finds the head service through ray.io/cluster, ray.io/node-type and ray.io/identifier, which the rename does not touch, so it finds the stale object, decides there is nothing to do, and returns. The reporter of #2564 was left deleting the whole RayCluster.

#2166 taught the lookup to survive the rename, but nothing was added to repair the service it finds. This PR adds that:

• reconcileHeadService now builds the desired service on every pass and updates spec.selector when it differs.
• The two app.kubernetes.io values in the selector are read from the head Pod that is running, not from the head Pod template.
• Labels are merged key by key, so labels another tool put on the service (Helm, Argo CD, a mesh injector) are kept.
• The operator writes nothing else: ClusterIP, ports, service type, node ports and third-party annotations stay as they are.
• A service that already matches produces no API write.

Reading the selector from the live Pod rather than the template is the part worth explaining, because the obvious implementation is wrong. KubeRay never relabels a running Pod, and shouldRecreatePodsForUpgrade returns false unless upgradeStrategy.type is Recreate, which is not the default. A selector taken straight from an edited template would therefore move off the Pod that is currently serving traffic and cut the head off immediately, which is worse than the bug being fixed. Sourcing from the live Pod gives both halves: the service keeps selecting the Pod that is up, and once that Pod restarts carrying the new template label the next reconcile follows it, which is the case #2564 reports.

Pods with a deletion timestamp are skipped, since their replacements come back with the template labels. When several head Pods are alive, the oldest one decides, because List order is not specified and without a tiebreak the selector could flip between reconciles and rewrite the service every time. The head Pod lookup itself uses ray.io/cluster and ray.io/node-type, both refused by labelPod, so a renamed app.kubernetes.io label cannot hide the Pod from that query.

The selector and the labels are handled differently on purpose. The selector is replaced whole, because it is the operator's alone and BuildServiceForHeadPod already ignores any selector a user puts in HeadGroupSpec.HeadService. A key added to the live selector by hand is therefore removed on the next reconcile, which is intended: an extra selector key the operator did not write narrows the match and can drop the head Pod out of the service, the same failure this PR is fixing. Labels get the opposite treatment, since other tools legitimately write labels the operator never wrote. TestReconcileHeadServiceReplacesHandEditedSelector pins both halves.

The same path also picks up app.kubernetes.io/created-by. Both app.kubernetes.io labels in the selector can be overridden on the head Pod, but only app.kubernetes.io/name was ever read back, so overriding created-by produced a head service that never matched its Pod at all. The three ray.io labels are deliberately not sourced this way: labelPod refuses two of them, and ray.io/identifier is how the operator finds this service again.

Level of fix

Fixed in RayClusterReconciler.reconcileHeadService because that is the only place in the operator where a service selector is derived from user supplied labels. I weighed two alternatives and dropped both.

The first was rejecting the override in validation. #1040 and #2166 made overriding app.kubernetes.io/name a supported thing to do, so rejecting it is a breaking change, and the webhook is optional, so the operator still could not rely on it. It also leaves already broken services broken.

The second was dropping the two app.kubernetes.io keys from the selector so it carries only the three ray.io identity labels, the same set the lookup uses. That makes the drift structurally impossible and would cover the RayJob and RayService head services too, but it rewrites the selector of every existing head service in a fleet and takes away a customization users are allowed to make today. It also still needs the update path in this PR to heal services that already exist, so this change is a prerequisite either way. I can follow up with that shape if you prefer it.

Six service reconcilers in the operator never reconcile an existing service: reconcileHeadService, reconcileServeService, reconcileHeadlessService, reconcilePerClusterServeService, plus the RayJob and RayService reconcileServices, which update only when the RayCluster switches. Only the RayCluster head service derives its selector from user input, so only that one is changed here. The new code follows ingressNeedsUpdate and reconcileIngressKubernetes in the same file.

Related issue number

Closes #2564

Labels

• If this PR has user-facing changes that require documentation updates at release time, I have added the doc-updates-required label.
• If this PR contains breaking changes, I have added the breaking-change label.

Neither box applies as far as I can tell. The user-facing behavior is a head service that stops going stale, which matches what the docs already promise, and nothing that worked before stops working. Happy to add either label if you read it differently.

Checks

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

Evidence

Nine new unit cases in raycluster_controller_unit_test.go. Seven of them fail against master:

$ go test ./controllers/ray/ -run TestReconcileHeadService -count=1 -v
--- PASS: TestReconcileHeadService
--- FAIL: TestReconcileHeadServiceSelectorDrift
--- PASS: TestReconcileHeadServiceKeepsSelectingRunningHeadPod
--- FAIL: TestReconcileHeadServicePrefersOldestHeadPod
--- FAIL: TestReconcileHeadServiceFollowsRestartedHeadPod
--- FAIL: TestReconcileHeadServiceOverriddenCreatedByLabel
--- PASS: TestReconcileHeadServiceIgnoresProtectedLabelOverrides
--- FAIL: TestReconcileHeadServiceSelectorDriftWithCustomHeadService
--- FAIL: TestReconcileHeadServiceReplacesHandEditedSelector
--- FAIL: TestReconcileHeadServiceLeavesUnmanagedSpecAlone
FAIL github.com/ray-project/kuberay/ray-operator/controllers/ray  1.328s

Two of those failures, one for each half of the change:

raycluster_controller_unit_test.go:1219: selector should use the head Pod value of app.kubernetes.io/created-by
  expected: "my-platform"
  actual  : "kuberay-operator"

raycluster_controller_unit_test.go:1274: the operator owns the whole selector, so an extra key is removed
  map[... "hand-edited":"yes" ...] should not contain "hand-edited"

The two that pass on master do so on purpose. IgnoresProtectedLabelOverrides pins that ray.io/cluster, ray.io/node-type and ray.io/identifier are never sourced from the Pod. KeepsSelectingRunningHeadPod passes on master because master never touches an existing selector at all, so there is nothing to move off the Pod; it is a guard on this PR's own update path, and it fails against the first draft of this change, which read the selector from the template.

All ten pass with the fix:

$ go test ./controllers/ray/ -run TestReconcileHeadService -count=1 -v
--- PASS: TestReconcileHeadService
--- PASS: TestReconcileHeadServiceSelectorDrift
--- PASS: TestReconcileHeadServiceKeepsSelectingRunningHeadPod
--- PASS: TestReconcileHeadServicePrefersOldestHeadPod
--- PASS: TestReconcileHeadServiceFollowsRestartedHeadPod
--- PASS: TestReconcileHeadServiceOverriddenCreatedByLabel
--- PASS: TestReconcileHeadServiceIgnoresProtectedLabelOverrides
--- PASS: TestReconcileHeadServiceSelectorDriftWithCustomHeadService
--- PASS: TestReconcileHeadServiceReplacesHandEditedSelector
--- PASS: TestReconcileHeadServiceLeavesUnmanagedSpecAlone
ok   github.com/ray-project/kuberay/ray-operator/controllers/ray  2.484s

Three new envtest specs join the RayCluster with overridden app.kubernetes.io labels block that #2166 created: the service keeps selecting the head Pod that is still running, the service follows the head Pod after it restarts with the renamed label, and the selector prefers the oldest live head Pod.

$ go test ./controllers/ray/ -run TestAPIs -count=1 -args -ginkgo.focus="app.kubernetes.io"
Ran 9 of 205 Specs in 14.425 seconds
SUCCESS! -- 9 Passed | 0 Failed | 0 Pending | 196 Skipped

Whole package, envtest 1.34.1:

$ go test ./controllers/ray/ -count=1
ok   github.com/ray-project/kuberay/ray-operator/controllers/ray   428.081s

Removing any one part of the fix turns the matching case red, checked by reverting each piece in turn.

What was not tested

• No live Kubernetes cluster. Everything above is envtest and the fake client, both on Windows. The fake client does not enforce field immutability, so ClusterIP surviving the update is pinned by the test but not proven against a real API server. Only spec.selector and metadata.labels are ever written, so both headless and ClusterIP services take the same path.
• spec.headServiceAnnotations is still not reconciled onto a live service. That is unchanged behavior, not a regression, and out of scope here.
• No test for an update conflict. r.Update errors propagate up and the cluster is requeued, which is the usual controller-runtime shape, and the reconcile is idempotent so a retry converges.
• The RayJob and RayService head services have a related defect that this PR does not touch: they build the selector through BuildServiceForHeadPod(ctx, rayCluster, nil, nil), so an overridden head Pod label leaves them wrong from creation rather than after a rename. It spans two more controllers and belongs in its own issue.
• ray.io/identifier is also overridable on Pods through labelPod, which breaks the selector in the same way. It is not sourced from the Pod here because the operator uses that label to find the head service, so the fix for it is to stop labelPod accepting it, which is a separate behavior change.
• pkg/webhooks/v1 cannot finish cleanly on this machine. All 7 specs pass and only the envtest teardown fails, with unable to signal for process ... kube-apiserver to stop: not supported by windows. Nothing in this PR touches that package.

AI disclosure

This change was written with AI assistance (Claude). A human reviewed the diff, ran every command shown above, and is responsible for the result.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 581fb88. Configure here.}

[stantheman0128]

Bugbot is right, and my e2e case is what hid it. This is fixed now.

The two keys I was syncing really are in the selector. BuildServiceForHeadPod builds spec.selector from every key in HeadServiceLabels and lets the caller override the values (common/service.go:46-48), so app.kubernetes.io/name and app.kubernetes.io/created-by end up there. On the Pod side labelPod refuses only ray.io/node-type, ray.io/group and ray.io/cluster (common/pod.go:1171), so those two keys follow the template. Nothing relabels a running Pod, since the only update path on an existing Pod writes annotations (raycluster_controller.go:1490), and shouldRecreatePodsForUpgrade returns false unless the upgrade strategy is Recreate (raycluster_controller.go:1465), which is not the default. So the first version of this PR pointed the Service at a value no live Pod carried, and the head stayed unreachable until something restarted it.

My e2e case deleted the head Pod right after editing the template, so it only ever looked at the state after the restart. That is why it stayed green.

The selector values now come from the head Pod that is running, not from the template. While the old Pod is up, the desired selector equals what the Service already has, so nothing is written and connectivity holds. When the Pod restarts it comes back with the template labels and the Service follows it, which is the case #2564 reports. Terminating Pods are skipped, and creation still falls back to the template because there is no Pod to read yet. When more than one head Pod is alive, the oldest one wins, so the selector cannot flip between reconciles.

Tests: one unit case asserts the selector keeps matching the running Pod after a template rename, one pins the tiebreak, and one asserts the selector follows a restarted Pod. I kept the old e2e case and added one beside it that never touches the Pod. The two that target this bug fail on the previous commit. The full controllers/ray suite is green at 205 specs.

One behavior change worth calling out: the selector is no longer purely CR derived, so anyone able to patch labels on the head Pod can move those two keys, and the operator will follow rather than correct it. The blast radius is bounded, because ray.io/cluster, ray.io/node-type and ray.io/identifier still come from the CR, so the selector can only ever match head Pods of this cluster.

There is a larger option I did not take, since it feels like a maintainer call: narrow the selector to ray.io/cluster and ray.io/node-type, which labelPod never lets a user override, and then it would never need reconciling at all. That rewrites the selector on every existing head Service during an operator upgrade. Happy to switch if you prefer that shape.

[stantheman0128]

On the red test-rayservice-incremental-upgrade-e2e step, that is the known Locust flake rather than anything from this PR. The job artifact gotest.log shows TestRayServiceIncrementalUpgradeWithLocust failing on timeout waiting for Locust to reach the steady state with RPS >= 400.00, with aggregate throughput at 364.30 and the rest of the package passing. That matches the cases #5098 is reproducing.