Skip to content

fix(operator): run Dynamo + Grove operators HA to stop webhook-flap CI failures #12712

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.

Sign up for GitHub

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

Closed
nv-tusharma wants to merge 1 commit into from
Closed

fix(operator): run Dynamo + Grove operators HA to stop webhook-flap CI failures #12712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions deploy/helm/charts/platform/components/operator/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,18 @@ metadata:
spec:
replicas: {{ .Values.controllerManager.replicas }}
strategy:
{{- if gt (int .Values.controllerManager.replicas) 1 }}
# With >1 replica, roll one pod at a time and never drop below the desired
# count so the admission webhook always has a serving endpoint during updates.
# Leader election (see operator-config.yaml) keeps only one active manager;
# the webhook is stateless, so multiple serving replicas do not conflict.
type: RollingUpdate
rollingUpdate:
maxUnavailable: 0
maxSurge: 1
{{- else }}
type: Recreate
{{- end }}
selector:
matchLabels:
control-plane: controller-manager
Expand Down
34 changes: 34 additions & 0 deletions deploy/helm/charts/platform/components/operator/templates/poddisruptionbudget.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# Keep at least one operator pod (and therefore one serving admission-webhook
# endpoint) available during voluntary disruptions such as node drains and
# upgrades. Only rendered when running >1 replica; with a single replica a
# minAvailable:1 budget would block node drains entirely.
{{- if gt (int .Values.controllerManager.replicas) 1 }}
apiVersion: policy/v1
kind: PodDisruptionBudget
metadata:
name: {{ include "dynamo-operator.fullname" . }}-controller-manager
labels:
app.kubernetes.io/component: manager
{{- include "dynamo-operator.labels" . | nindent 4 }}
spec:
minAvailable: 1
selector:
matchLabels:
control-plane: controller-manager
{{- include "dynamo-operator.selectorLabels" . | nindent 6 }}
{{- end }}
8 changes: 7 additions & 1 deletion deploy/helm/charts/platform/components/operator/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,13 @@ controllerManager:
requests:
cpu: 512m
memory: 1Gi
replicas: 1
# 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
Comment on lines +95 to +101

@devin-ai-integration devin-ai-integration Bot Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +95 to +101

@devin-ai-integration devin-ai-integration Bot Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +95 to +101

@devin-ai-integration devin-ai-integration Bot Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@dynamo-review-agent dynamo-review-agent Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

serviceAccount:
annotations: {}

Expand Down
36 changes: 36 additions & 0 deletions deploy/helm/charts/platform/templates/grove-poddisruptionbudget.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# The upstream Grove chart ships no PodDisruptionBudget, so define one here to
# keep at least one grove-operator pod (and its PodCliqueSet admission webhook)
# 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) }}

@dynamo-review-agent dynamo-review-agent Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{- if and $groveInstalled (gt (int ($grove.replicaCount | default 1)) 1) }}
Comment on lines +21 to +22

@devin-ai-integration devin-ai-integration Bot Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Suggested change
{{- $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) }}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

apiVersion: policy/v1
kind: PodDisruptionBudget
metadata:
name: grove-operator
namespace: {{ .Release.Namespace }}
labels:
app.kubernetes.io/name: grove-operator
app.kubernetes.io/part-of: dynamo-platform
spec:
minAvailable: 1
selector:
matchLabels:
app.kubernetes.io/name: grove-operator
{{- end }}
6 changes: 6 additions & 0 deletions deploy/helm/charts/platform/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,12 @@ dynamo-operator:
# Grove component - distributed inference orchestration
# Installation is controlled by global.grove.install above.
grove:
# -- 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
Comment on lines +285 to +290

@devin-ai-integration devin-ai-integration Bot Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

# -- Node tolerations for Grove pods
tolerations: []
# -- Affinity for Grove pods
Expand Down
Loading