fix: prevent authentication controller deadlock under OOM/webhook pressure#184
Conversation
…ssure When the kuberay operator experiences OOM kills or its internal webhook becomes unresponsive, the authentication controller's single worker can block indefinitely on API calls, preventing all RayClusters from having their AuthenticationReady condition set — which blocks head pod creation. This change adds three resilience improvements: 1. Context timeout (60s) on each reconcile — breaks indefinite blocks caused by unresponsive webhooks or slow API calls 2. Increase MaxConcurrentReconciles from 1 to 3 — prevents head-of-line blocking when one cluster's reconciliation is slow 3. Safety requeue (5min) — ensures clusters self-heal if watch events are missed due to predicate filtering or operator restarts Additionally fixes: - Re-fetches RayCluster after enforceEnableIngressFalse to avoid stale resourceVersion causing 409 Conflict errors - Registers ReferenceGrant and HTTPRoute as owned resources for proper watch-based reconciliation Verified: operator recovers from OOM (32Mi → 512Mi) and successfully reconciles all pending RayClusters with these fixes applied. Ref: RHOAIENG-61483 Signed-off-by: Laura Fitzgerald <lfitzger@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughReconcile context is wrapped with a 60s timeout and successful reconciles now return RequeueAfter: SafetyRequeueDelay (5 minutes). After enforcing enableIngress=false on OpenShift the controller re-fetches the RayCluster to obtain the updated resourceVersion. SetupWithManager sets MaxConcurrentReconciles to 3 and limits owner-watch wiring to in-namespace resources. The OIDC ConfigMap gains the KubernetesCreatedBy label. main.go adds corev1 import and registers ConfigMap in cacheSelectors. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security & Code Quality Findings
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ray-operator/controllers/ray/authentication_controller.go`:
- Around line 1109-1110: The controller currently uses
Owns(&gatewayv1beta1.ReferenceGrant{}) and Owns(&gatewayv1beta1.HTTPRoute{})
which won't enqueue reconciles for these objects because ReferenceGrant avoids
OwnerReferences and HTTPRoute is often created in a different namespace; replace
these Owns() entries with Watches() using a custom EnqueueRequestsFromMapFunc
that looks up the owning RayCluster by labels (e.g. "ray.io/cluster-namespace"
and "ray.io/cluster-name") and enqueues the corresponding reconcile.Request;
implement the mapping function referenced from the Watches call and ensure the
Watch is registered for gatewayv1beta1.ReferenceGrant and
gatewayv1beta1.HTTPRoute instead of Owns to get watch-based triggers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a0fd4ec3-c266-47d5-b2d2-4c27aaae955a
📒 Files selected for processing (1)
ray-operator/controllers/ray/authentication_controller.go
| Owns(&gatewayv1beta1.ReferenceGrant{}). | ||
| Owns(&gatewayv1beta1.HTTPRoute{}). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify owner reference handling in ensureReferenceGrant and ensureHttpRoute
echo "=== ReferenceGrant owner reference handling ==="
rg -n -A5 'SetControllerReference.*referenceGrant' ray-operator/controllers/ray/authentication_controller.go || echo "No SetControllerReference for ReferenceGrant found"
echo ""
echo "=== HTTPRoute owner reference handling ==="
rg -n -B2 -A5 'SetControllerReference.*httpRoute' ray-operator/controllers/ray/authentication_controller.go
echo ""
echo "=== Comment about shared grant ==="
rg -n 'Do NOT set controller reference' ray-operator/controllers/ray/authentication_controller.goRepository: opendatahub-io/kuberay
Length of output: 639
🏁 Script executed:
sed -n '1100,1115p' ray-operator/controllers/ray/authentication_controller.goRepository: opendatahub-io/kuberay
Length of output: 699
Owns() watches are ineffective for resources without OwnerReferences.
Owns(&gatewayv1beta1.ReferenceGrant{}) and Owns(&gatewayv1beta1.HTTPRoute{}) only trigger reconciliation when these resources have an OwnerReference pointing to the parent RayCluster. However:
ReferenceGrantexplicitly avoids setting owner reference (line 637: shared across clusters)HTTPRouteonly sets owner reference whenplatformNamespace == cluster.Namespace(lines 438-443), which is typically not the case for cross-namespace routes
These watches will not fire for the majority of cases. The controller will rely only on SafetyRequeueDelay for eventual consistency. If watch-based triggers are required for these resources, use Watches() with a custom EnqueueRequestsFromMapFunc that maps via labels (ray.io/cluster-namespace, ray.io/cluster-name) instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ray-operator/controllers/ray/authentication_controller.go` around lines 1109
- 1110, The controller currently uses Owns(&gatewayv1beta1.ReferenceGrant{}) and
Owns(&gatewayv1beta1.HTTPRoute{}) which won't enqueue reconciles for these
objects because ReferenceGrant avoids OwnerReferences and HTTPRoute is often
created in a different namespace; replace these Owns() entries with Watches()
using a custom EnqueueRequestsFromMapFunc that looks up the owning RayCluster by
labels (e.g. "ray.io/cluster-namespace" and "ray.io/cluster-name") and enqueues
the corresponding reconcile.Request; implement the mapping function referenced
from the Watches call and ensure the Watch is registered for
gatewayv1beta1.ReferenceGrant and gatewayv1beta1.HTTPRoute instead of Owns to
get watch-based triggers.
There was a problem hiding this comment.
@laurafitzgerald changes look good for me. got this comment from cursor though (not sure if we want to address it here or not):
The Owns(&gatewayv1beta1.HTTPRoute{}) call is a no-op as a watch trigger because HTTPRoute is intentionally created in platformNamespace rather than cluster.Namespace,
making cross-namespace owner references impossible (as the code itself acknowledges with // NOTE: Cannot use SetControllerReference for cross-namespace ownership).
Similarly, Owns(&gatewayv1beta1.ReferenceGrant{}) won't enqueue reconciles because the ReferenceGrant intentionally has no owner reference set — cleanup is managed via reference counting instead.
The SafetyRequeueDelay of 5 minutes effectively compensates for this gap, so it's low priority, but the comment "ensures proper watch-based reconciliation" is misleading.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
/run-kuberay-e2e |
|
@laurafitzgerald: The following test has Succeeded: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:odh-pr-test-kuberay-hbzv6 |
HTTPRoute is created in platformNamespace (not the RayCluster namespace), making cross-namespace owner references impossible. ReferenceGrant uses reference-counting cleanup without owner references. In both cases, Owns() never triggers reconciliation because no ownerRef is set. SafetyRequeueDelay (5m) already compensates by periodically re-reconciling to detect drift in these resources. Update comments to reflect this. Part of RHOAIENG-61483. Co-authored-by: Cursor <cursoragent@cursor.com>
The operator's informer cache watches all ConfigMaps cluster-wide when no watchNamespace is set. On clusters with many namespaces and thousands of ConfigMaps, the List response is large enough to trigger HTTP/2 stream errors (INTERNAL_ERROR), which poison the shared connection pool and can cause unrelated API calls (e.g. Update for auth finalizer) to hang indefinitely. Fix by applying the same label-selector filter already used for Jobs: only cache ConfigMaps with app.kubernetes.io/created-by=kuberay-operator. The auth controller now adds this label when creating OIDC ConfigMaps. Part of RHOAIENG-61483. Co-authored-by: Cursor <cursoragent@cursor.com>
…troller The auth controller should not be updating RayCluster spec fields — that r.Update() call triggers the mutating webhook and can block indefinitely on a poisoned HTTP/2 connection, deadlocking the entire auth controller. The RayCluster controller is the natural owner of spec enforcement: - It already handles ingress reconciliation (reconcileIngress) - It already does spec-level Updates (finalizers, status) - It has configurable MaxConcurrentReconciles, so one blocked Update doesn't block all clusters The enforcement runs early in rayClusterReconcile, before reconcileIngress, and returns early with a requeue to let the next reconcile see the updated spec. The webhook continues to handle new CREATE/UPDATE operations. Part of RHOAIENG-61483. Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Summary
The authentication controller can permanently hang on clusters experiencing HTTP/2 stream errors from the API server. A single blocked API call (Update for finalizer, or CreateOrUpdate for HTTPRoute/ReferenceGrant) deadlocks the controller's only worker goroutine, preventing all RayClusters from progressing past the
AuthenticationReadygate.Root cause: On multi-tenant clusters, the operator watches all ConfigMaps cluster-wide. Large List responses break HTTP/2 streams (
INTERNAL_ERROR), poisoning the shared connection pool. The auth controller's API calls then block indefinitely on dead connections. WithMaxConcurrentReconciles: 1and no context timeout, a single stuck call permanently blocks the entire controller — even across pod restarts.Changes
Context timeout (60s) on every reconcile — Ensures blocked API calls are cancelled rather than hanging indefinitely.
Increased MaxConcurrentReconciles to 3 — One stuck worker no longer blocks all clusters.
Safety requeue (5m) on success — Periodically re-reconciles to detect drift in cross-namespace resources (HTTPRoute, ReferenceGrant) that lack owner references.
Removed no-op Owns() for HTTPRoute and ReferenceGrant — These never triggered reconciliation because cross-namespace owner references are impossible (HTTPRoute) and ReferenceGrant uses reference-counting cleanup without owner refs. Updated comments to accurately describe the SafetyRequeueDelay's purpose.
Scoped ConfigMap informer to only kuberay-managed ConfigMaps — Adds
app.kubernetes.io/created-by=kuberay-operatorlabel selector to the manager cache (same pattern as Jobs). Eliminates cluster-wide ConfigMap List that triggers HTTP/2 stream errors.Moved
enforceEnableIngressFalsefrom auth controller to RayCluster controller — The auth controller should not Update RayCluster spec fields; that triggers the mutating webhook and is the primary deadlock vector. The RayCluster controller is the natural owner of spec enforcement — it already handles ingress reconciliation and has configurable concurrency.Evidence from customer must-gather
restartCount: 0, clean start — auth controller logged "Handling Integrated OAuth" at16:00:09then never logged again (1h48m silence)"Failed to watch *v1.ConfigMap: stream error: INTERNAL_ERROR"Test plan
go test ./controllers/ray/...)app.kubernetes.io/created-by=kuberay-operatorlabelenableIngressenforcement works from the RayCluster controller on upgrade (pre-existing cluster withenableIngress: trueor nil)AuthenticationReady=Trueand head pod comes up