fix(operator): run Dynamo + Grove operators HA to stop webhook-flap CI failures - #12712
fix(operator): run Dynamo + Grove operators HA to stop webhook-flap CI failures#12712nv-tusharma wants to merge 1 commit into
Conversation
…I failures Deploy jobs intermittently fail at DynamoGraphDeployment admission with `dial tcp <ip>:443: connect: connection refused` (Dynamo operator webhook, mdynamographdeployment.kb.io) or the equivalent on grove-operator:9443 (pcs.defaulting.webhooks.grove.io). Both operators run a single replica, so any pod restart/reschedule — the expected cert-resync restart (grove issue #701), a node event, or a transient not-ready — briefly zeroes the webhook Service's EndpointSlice, and the fail-closed admission webhook then rejects the request. Run both operators with 2 replicas + a PodDisruptionBudget(minAvailable=1) so a single pod restart never leaves the webhook without a serving endpoint: - Dynamo operator: replicas 1 -> 2 (values.yaml); RollingUpdate with maxUnavailable=0/maxSurge=1 instead of Recreate so updates keep a serving endpoint (deployment.yaml); new PodDisruptionBudget (minAvailable=1). - Grove operator: grove.replicaCount 1 -> 2 (upstream subchart override); new PodDisruptionBudget in the parent chart (Grove ships none). Safe with >1 replica: both operators enable leader election (Dynamo: operator-config.yaml leaderElection.enabled=true; Grove: leaderElection.enabled=true), so exactly one replica reconciles while the stateless webhook is served by all replicas. Both PDBs and the RollingUpdate switch are gated on replicas>1, so single-replica installs are unchanged (Recreate, no PDB — avoids blocking node drains). Validated with `helm template` (both PDBs render, operator replicas=2, grove replicas=2, RollingUpdate applied) and `helm lint` (0 failures). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
fix
deployment::k8s
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
WalkthroughThe Helm charts configure two replicas for the controller-manager and Grove operator. Multi-replica controller-manager deployments use rolling updates. Conditional PodDisruptionBudgets require one pod to remain available. ChangesOperator high availability
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # Run 2 replicas so a single pod restart/reschedule never leaves the admission | ||
| # webhook without a serving endpoint (the cause of intermittent | ||
| # "dial tcp ...:443: connect: connection refused" DGD-admission failures). | ||
| # Safe with >1 replica: leader election elects one active manager; the webhook | ||
| # is stateless and served by all replicas. Paired with a PodDisruptionBudget | ||
| # (poddisruptionbudget.yaml) and a RollingUpdate strategy (deployment.yaml). | ||
| replicas: 2 |
There was a problem hiding this comment.
🔴 CI operator-readiness wait never succeeds once each operator runs two pods
The number of operator pods is doubled by defaulting to two copies (replicas: 2 at deploy/helm/charts/platform/components/operator/values.yaml:101, plus replicaCount: 2 at deploy/helm/charts/platform/values.yaml:290) while the CI readiness gate still insists on seeing exactly two pods in total, so every deploy job waits the full 30 minutes and then fails.
Impact: The very CI jobs this change is meant to stabilize will time out and fail at the operator-readiness step.
Hard-coded pod count in the setup-dynamo-operator readiness loop
.github/actions/setup-dynamo-operator/action.yml:494-514 collects pods whose names start with dynamo-platform-dynamo-operator-controller-manager- or grove-operator- and breaks out of the wait only when [ "${#POD_STATUSES[@]}" -eq 2 ] and both entries are 1/1. With 2 replicas of each operator there will be 4 matching pods, so the condition never holds and the step errors with "Expected two ready 1/1 operator Pods after 1800s". The loop must be generalized (e.g. require all matching pods ready and at least the expected count, or use kubectl rollout status).
Prompt for agents
The CI readiness gate in .github/actions/setup-dynamo-operator/action.yml ("Wait for operator containers to be ready" step) hard-codes an expectation of exactly two operator pods (one Dynamo controller-manager, one grove-operator) by checking `[ "${#POD_STATUSES[@]}" -eq 2 ]` and that both are `1/1`. This PR raises both operators to 2 replicas, producing 4 matching pods, so the condition can never be satisfied and the step fails after the 1800s timeout. Update the wait logic to be replica-count agnostic — e.g. require every matching pod to be `1/1` and require at least one pod per operator prefix, or replace the awk/mapfile loop with `kubectl rollout status deployment/...` for both the Dynamo operator and grove-operator deployments.
Was this helpful? React with 👍 or 👎 to provide feedback.
| {{- $groveInstalled := and .Values.global .Values.global.grove (or .Values.global.grove.install .Values.global.grove.enabled) }} | ||
| {{- if and $groveInstalled (gt (int ($grove.replicaCount | default 1)) 1) }} |
There was a problem hiding this comment.
🟡 Availability budget is created for an externally installed Grove operator, blocking node drains
A minimum-availability rule for Grove is created ($groveInstalled check at deploy/helm/charts/platform/templates/grove-poddisruptionbudget.yaml:21-22) even when this chart does not install Grove at all, so a separately installed single-pod Grove operator in the same namespace can no longer be evicted and node drains hang.
Impact: Draining or upgrading a node stalls indefinitely in clusters where Grove was installed separately with one pod.
install vs. enabled semantics and the replica-count gate
deploy/helm/charts/platform/values.yaml:49-57 documents global.grove.install as "this chart installs the bundled Grove subchart" and global.grove.enabled as "Grove is available in the cluster (installed externally)"; Chart.yaml:47 conditions the subchart only on install. The template's or .Values.global.grove.install .Values.global.grove.enabled therefore also renders the budget for install=false, enabled=true. In that case the replica gate uses this chart's grove.replicaCount (now defaulted to 2 at deploy/helm/charts/platform/values.yaml:290), which has no relation to the external deployment's real replica count — commonly 1. A minAvailable: 1 budget matching the single external app.kubernetes.io/name: grove-operator pod makes its eviction always disallowed.
| {{- $groveInstalled := and .Values.global .Values.global.grove (or .Values.global.grove.install .Values.global.grove.enabled) }} | |
| {{- if and $groveInstalled (gt (int ($grove.replicaCount | default 1)) 1) }} | |
| {{- $groveInstalled := and .Values.global .Values.global.grove .Values.global.grove.install }} | |
| {{- if and $groveInstalled (gt (int ($grove.replicaCount | default 1)) 1) }} |
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Run 2 replicas so a single pod restart/reschedule never leaves the admission | ||
| # webhook without a serving endpoint (the cause of intermittent | ||
| # "dial tcp ...:443: connect: connection refused" DGD-admission failures). | ||
| # Safe with >1 replica: leader election elects one active manager; the webhook | ||
| # is stateless and served by all replicas. Paired with a PodDisruptionBudget | ||
| # (poddisruptionbudget.yaml) and a RollingUpdate strategy (deployment.yaml). | ||
| replicas: 2 |
There was a problem hiding this comment.
🟡 Two operator pods starting together can fight over the webhook certificate and crash on startup
Both operator copies generate and write the webhook certificate at startup without any coordination or retry (cm.client.Update at deploy/operator/internal/cert/cert.go:259), so on a fresh install the loser of the race exits immediately and restarts.
Impact: On first install with two replicas one operator pod can crash-loop briefly, and the two pods may momentarily serve certificates from different authorities, causing transient admission failures.
Unsynchronized bootstrap of the webhook TLS secret
deploy/operator/cmd/main.go:326-329 calls certMgr.SetupAndRunOnce before mgr.Start, i.e. before leader election, in every replica. setupAutoProvisioning (deploy/operator/internal/cert/cert.go:165-178) creates a placeholder secret and calls bootstrapCertSecret, which reads the secret and — because the placeholder data is empty — mints a new CA and server cert and issues client.Update on the object it just read. With two replicas starting concurrently both read the same resourceVersion; the second Update returns a 409 conflict, the error is propagated up and main.go does os.Exit(1). There is no RetryOnConflict anywhere in cert.go. The pod recovers on restart (the secret is valid by then), but the write race can also leave one pod's mounted cert signed by a CA that is no longer in the injected caBundle until kubelet re-projects the secret.
Prompt for agents
With controllerManager.replicas defaulting to 2, the webhook certificate bootstrap in deploy/operator/internal/cert/cert.go (bootstrapCertSecret / refreshCertSecret, invoked from SetupAndRunOnce before mgr.Start in deploy/operator/cmd/main.go) now runs concurrently in multiple pods with no coordination. Both pods read the same webhook TLS secret, both see empty/invalid data on a fresh install, both mint a new CA + server cert, and both attempt client.Update; the loser receives a 409 conflict which bubbles up and terminates the process. Consider retrying on conflict (client-go retry.RetryOnConflict) and re-evaluating secret validity after re-reading, so a pod that lost the race adopts the winner's certificate instead of exiting.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # -- Run 2 Grove operator replicas so a single pod restart (e.g. the expected | ||
| # cert-resync restart, grove issue #701) never leaves the PodCliqueSet webhook | ||
| # without a serving endpoint. Grove enables leader election by default, so only | ||
| # one replica reconciles; the webhook is stateless and served by all replicas. | ||
| # Paired with a PodDisruptionBudget (templates/grove-poddisruptionbudget.yaml). | ||
| replicaCount: 2 |
There was a problem hiding this comment.
🔍 Grove subchart value key replicaCount not verified against the pinned upstream chart
The parent chart adds grove.replicaCount: 2 as a subchart override, but the grove-charts dependency (oci://ghcr.io/ai-dynamo/grove v0.1.0-alpha.12-rc1, deploy/helm/charts/platform/Chart.yaml:43-47) is not vendored in this repo, so the key name and nesting could not be verified locally. If the upstream chart nests the replica count elsewhere (e.g. under controllerManager or operator), the value is silently ignored, Grove still runs one pod, and the parent-chart PDB — gated on the same value — renders minAvailable: 1 for a single pod, converting an ineffective HA change into a drain blocker. Worth confirming with a helm template against the pinned chart version.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Run 2 replicas so a single pod restart/reschedule never leaves the admission | ||
| # webhook without a serving endpoint (the cause of intermittent | ||
| # "dial tcp ...:443: connect: connection refused" DGD-admission failures). | ||
| # Safe with >1 replica: leader election elects one active manager; the webhook | ||
| # is stateless and served by all replicas. Paired with a PodDisruptionBudget | ||
| # (poddisruptionbudget.yaml) and a RollingUpdate strategy (deployment.yaml). | ||
| replicas: 2 |
There was a problem hiding this comment.
🔍 Namespace-restricted mode with 2 replicas is untested by this change
The replica default is global; namespace-restricted installs (namespaceRestriction.enabled=true) use a separate namespace-ownership Lease mechanism (namespaceRestriction.lease in values.yaml, LeaseManager/LeaseWatcher in deploy/operator/cmd/main.go:331+) rather than plain controller-runtime leader election. Two replicas in that mode both contend for the ownership Lease; the PR description only argues safety via leaderElection.enabled: true in operator-config.yaml. Worth confirming the restricted-mode lease path tolerates a second replica, or gating the replica bump to cluster-wide installs.
Was this helpful? React with 👍 or 👎 to provide feedback.
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| # Safe with >1 replica: leader election elects one active manager; the webhook | ||
| # is stateless and served by all replicas. Paired with a PodDisruptionBudget | ||
| # (poddisruptionbudget.yaml) and a RollingUpdate strategy (deployment.yaml). | ||
| replicas: 2 |
There was a problem hiding this comment.
Defaulting the operator to two replicas races the pre-leader-election webhook certificate bootstrap because both pods can update the same TLS Secret and one exits on an Update conflict. Fix: make certificate bootstrap conflict-tolerant before enabling HA by default.
🤖 AI Fix
In deploy/operator/internal/cert/cert.go, update CertManager.bootstrapCertSecret or refreshCertSecret to wrap the Secret get/validation/update path in retry.RetryOnConflict, re-fetch the Secret on each retry, and return success when another replica has already written a valid CA/server certificate.
| # available during voluntary disruptions. Only rendered when Grove is installed | ||
| # and running >1 replica (see the `grove:` block in values.yaml). | ||
| {{- $grove := .Values.grove | default dict }} | ||
| {{- $groveInstalled := and .Values.global .Values.global.grove (or .Values.global.grove.install .Values.global.grove.enabled) }} |
There was a problem hiding this comment.
The Grove PDB renders when global.grove.enabled=true even if this chart did not install Grove, so the platform chart can create or manage a disruption budget for an externally managed operator with unrelated replica settings. Fix: render this PDB only when global.grove.install=true.
🤖 AI Fix
In deploy/helm/charts/platform/templates/grove-poddisruptionbudget.yaml, change $groveInstalled to require only .Values.global.grove.install and remove .Values.global.grove.enabled from the PDB render condition.
|
|
closing stale PR |
1 participant
Problem
Deploy jobs (
deploy-test-*,DynamoCheckpoint Deploy, etc.) intermittently fail atDynamoGraphDeploymentadmission with:or the equivalent on the Grove operator:
Root cause
Both operators run a single replica with a fail-closed (
failurePolicy: Fail) admission webhook. When that one pod's webhook is momentarily unavailable — the expected cert-resync restart (Grove issue #701), a node/reschedule event, an OOM, or a transient not-ready — the webhook Service's EndpointSlice drops its only endpoint, so the API server getsconnection refusedand the fail-closed webhook rejects the admission. Prior work reduced some triggers (#12255 readiness gating, #12279 vCluster version alignment), but with one replica any brief blip is still a full outage window.Fix
Run both operators with 2 replicas + a PodDisruptionBudget (
minAvailable: 1) so a single pod restart never leaves the webhook without a serving endpoint.controllerManager.replicas1 → 2; switchRecreate→RollingUpdate(maxUnavailable: 0,maxSurge: 1) so upgrades also keep a serving endpoint; newPodDisruptionBudget.grove.replicaCount1 → 2 (upstream subchart override); newPodDisruptionBudgetin the parent chart (Grove ships none).Safe with >1 replica — both operators enable leader election (Dynamo:
operator-config.yamlleaderElection.enabled: true; Grove: chart defaultleaderElection.enabled: true), so exactly one replica reconciles while the stateless webhook is served by all replicas.Both PDBs and the
RollingUpdateswitch are gated onreplicas > 1, so single-replica installs are unchanged (stillRecreate, no PDB — avoids blocking node drains).Validation
helm template— both PDBs render, Dynamoreplicas: 2+RollingUpdate, Grovereplicas: 2; single-replica path degrades cleanly (0 PDBs,Recreate).helm lint— 0 failures.Note / tradeoff
This sets
replicas: 2as the chart default, so it fixes CI and makes the operators HA in production — at the cost of a second operator pod per install (roughly doubles each operator's resource reservations). If a smaller blast radius is preferred, the replica bump could instead be scoped to CI via--setin thesetup-dynamo-operatorstep, leaving the chart default at 1.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Reliability Improvements