fix(operator): replace snapshot pods after GMS restart - #11286
Merged
Conversation
github-actions
Bot
added
fix
documentation
Contributor
|
🔗 Commit SHA: c7ae994 | Docs | Datadog PR Page | Give us feedback! |
Contributor
WalkthroughThe change adds a feature-gated GMS pod replacement controller. It detects native GMS sidecar restarts and deletes owned snapshot restore-target pods. Failover cascade handling excludes those pods. Pod cache projections preserve the restart data required for detection, with expanded controller and contract tests. ChangesGMS snapshot pod lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
deploy/operator/internal/dynamo/v1beta1_helpers.go (1)
10-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the message constant to avoid drift with the webhook wording.
The same literal
"checkpoint/snapshot is not supported with active/passive failover"is hard-coded again inshared_v1beta1.go(Line 218) instead of referencing this constant. SincecheckpointFailoverUnsupportedMessageis unexported, the webhook package can't reuse it, so the controller-side and admission-side error text can silently diverge if one is edited later (multiple tests already assert on the exact string in both places).♻️ Suggested fix
-const checkpointFailoverUnsupportedMessage = "checkpoint/snapshot is not supported with active/passive failover" +// CheckpointFailoverUnsupportedMessage is shared between reconcile-time +// validation and webhook admission so the two enforcement points stay +// worded identically. +const CheckpointFailoverUnsupportedMessage = "checkpoint/snapshot is not supported with active/passive failover"Then use
dynamo.CheckpointFailoverUnsupportedMessageinshared_v1beta1.go'sfield.Forbidden(...)call, anderrors.New(CheckpointFailoverUnsupportedMessage)here.Also applies to: 187-197
🤖 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 `@deploy/operator/internal/dynamo/v1beta1_helpers.go` around lines 10 - 21, The failover validation message is duplicated as a literal and can drift between controller and webhook paths; export the shared constant from the helper package and reuse it everywhere. Update the identifier currently named checkpointFailoverUnsupportedMessage in the v1beta1 helpers so the webhook-side code in shared_v1beta1.go can reference the same symbol, and replace the hard-coded string in the forbidden/error construction with that shared constant to keep both paths and their tests aligned.deploy/operator/internal/webhook/validation/shared_v1alpha1.go (1)
439-447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the failover message into a shared exported helper
This literal duplicates the same failover text already defined in
deploy/operator/internal/dynamo/v1beta1_helpers.go, and the v1beta1 webhook path hardcodes it too. SincecheckpointFailoverUnsupportedMessageis unexported, this validator can’t reuse it directly; move the text behind a shared exported const/helper so both paths stay in sync.🤖 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 `@deploy/operator/internal/webhook/validation/shared_v1alpha1.go` around lines 439 - 447, Hoist the failover error text into a shared exported constant/helper so both validators reuse the same source of truth. Update SharedSpecValidatorV1Alpha1.validateCheckpointWithFailover to reference that shared symbol instead of embedding the literal, and make the existing checkpointFailoverUnsupportedMessage in the Dynamo helpers accessible from both the v1alpha1 and v1beta1 webhook paths to keep the messages in sync.
🤖 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.
Nitpick comments:
In `@deploy/operator/internal/dynamo/v1beta1_helpers.go`:
- Around line 10-21: The failover validation message is duplicated as a literal
and can drift between controller and webhook paths; export the shared constant
from the helper package and reuse it everywhere. Update the identifier currently
named checkpointFailoverUnsupportedMessage in the v1beta1 helpers so the
webhook-side code in shared_v1beta1.go can reference the same symbol, and
replace the hard-coded string in the forbidden/error construction with that
shared constant to keep both paths and their tests aligned.
In `@deploy/operator/internal/webhook/validation/shared_v1alpha1.go`:
- Around line 439-447: Hoist the failover error text into a shared exported
constant/helper so both validators reuse the same source of truth. Update
SharedSpecValidatorV1Alpha1.validateCheckpointWithFailover to reference that
shared symbol instead of embedding the literal, and make the existing
checkpointFailoverUnsupportedMessage in the Dynamo helpers accessible from both
the v1alpha1 and v1beta1 webhook paths to keep the messages in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 09854cca-0bc5-4893-a9ea-3351cf664637
📒 Files selected for processing (24)
deploy/helm/charts/platform/components/operator/templates/deployment.yamldeploy/helm/charts/platform/components/operator/values.yamldeploy/operator/cmd/main.godeploy/operator/config/samples/nvidia.com_v1alpha1_dynamocheckpoint.yamldeploy/operator/internal/checkpoint/checkpoint_test.godeploy/operator/internal/checkpoint/gms_snapshot_gate.godeploy/operator/internal/checkpoint/resolve.godeploy/operator/internal/checkpoint/resource.godeploy/operator/internal/consts/consts.godeploy/operator/internal/controller/dynamocheckpoint_controller.godeploy/operator/internal/controller/dynamocheckpoint_controller_test.godeploy/operator/internal/controller/dynamocomponentdeployment_controller.godeploy/operator/internal/controller/dynamocomponentdeployment_controller_test.godeploy/operator/internal/controller/dynamographdeployment_controller.godeploy/operator/internal/controller/dynamographdeployment_controller_test.godeploy/operator/internal/dynamo/v1beta1_helpers.godeploy/operator/internal/webhook/validation/dynamocheckpoint_handler.godeploy/operator/internal/webhook/validation/dynamocheckpoint_handler_test.godeploy/operator/internal/webhook/validation/dynamocomponentdeployment_test.godeploy/operator/internal/webhook/validation/dynamographdeployment_test.godeploy/operator/internal/webhook/validation/shared_v1alpha1.godeploy/operator/internal/webhook/validation/shared_v1beta1.godocs/kubernetes/shadow-engine-failover.mddocs/kubernetes/snapshot.md
💤 Files with no reviewable changes (12)
- deploy/operator/internal/consts/consts.go
- deploy/operator/cmd/main.go
- deploy/operator/internal/controller/dynamocheckpoint_controller.go
- deploy/helm/charts/platform/components/operator/templates/deployment.yaml
- deploy/operator/internal/webhook/validation/dynamocheckpoint_handler_test.go
- deploy/operator/internal/checkpoint/gms_snapshot_gate.go
- deploy/helm/charts/platform/components/operator/values.yaml
- deploy/operator/config/samples/nvidia.com_v1alpha1_dynamocheckpoint.yaml
- deploy/operator/internal/controller/dynamocheckpoint_controller_test.go
- deploy/operator/internal/webhook/validation/dynamocheckpoint_handler.go
- deploy/operator/internal/checkpoint/resource.go
- deploy/operator/internal/checkpoint/resolve.go
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
galletas1712
force-pushed
the
fix/operator-enable-gms-snapshot
branch
from
July 7, 2026 22:50
d62389a to
70d708f
Compare
galletas1712
force-pushed
the
fix/operator-enable-gms-snapshot
branch
from
July 24, 2026 23:12
70d708f to
55c319f
Compare
galletas1712
force-pushed
the
fix/operator-enable-gms-snapshot
branch
from
July 24, 2026 23:34
55c319f to
6aa21f0
Compare
sttts
reviewed
Jul 27, 2026
galletas1712
force-pushed
the
fix/operator-enable-gms-snapshot
branch
from
July 27, 2026 18:12
6aa21f0 to
963608d
Compare
galletas1712
force-pushed
the
fix/operator-enable-gms-snapshot
branch
from
August 5, 2026 00:05
963608d to
9bff205
Compare
galletas1712
added
operator
galletas1712
changed the title
galletas1712
added
the
fault-tolerance
galletas1712
force-pushed
the
fix/operator-enable-gms-snapshot
branch
from
August 5, 2026 00:51
9bff205 to
f790fc9
Compare
galletas1712
force-pushed
the
fix/operator-enable-gms-snapshot
branch
from
August 5, 2026 02:06
f790fc9 to
cdfde13
Compare
Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
galletas1712
force-pushed
the
fix/operator-enable-gms-snapshot
branch
from
August 5, 2026 16:54
cdfde13 to
c922a42
Compare
Contributor
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
deploy/operator/internal/controller/gms_pod_replacement_controller.go (1)
38-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one-line story comments above the new multi-line semantic blocks.
deploy/operator/internal/controller/gms_pod_replacement_controller.go#L38-L119: add focused story comments for reconciliation, native-sidecar detection, and event filtering.deploy/operator/internal/podcache/transform.go#L110-L121: add a story comment for GMS init-container selection.deploy/operator/internal/podcache/transform.go#L204-L211: add a story comment for retaining the GMS restart count.deploy/operator/internal/controller/setup.go#L172-L191: add story comments for controller construction and registration.deploy/operator/cmd/main.go#L640-L650: add a story comment for GMSSnapshot-gated registration.As per coding guidelines, “In Go code, put a one-line story comment above every multi-line block of logically connected code.”
🤖 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 `@deploy/operator/internal/controller/gms_pod_replacement_controller.go` around lines 38 - 119, add one-line story comments above the requested multi-line logical blocks: Reconcile, hasRestartedNativeGMSServer, and gmsPodReplacementPredicate in deploy/operator/internal/controller/gms_pod_replacement_controller.go; the GMS init-container selection and restart-count retention blocks in deploy/operator/internal/podcache/transform.go; controller construction and registration in deploy/operator/internal/controller/setup.go; and GMSSnapshot-gated registration in deploy/operator/cmd/main.go. Keep comments focused on each block’s behavior and do not alter the implementation.Source: Coding guidelines
deploy/operator/internal/controller/gms_pod_replacement_controller_test.go (2)
33-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
t.Logheadings before each test step.
deploy/operator/internal/controller/gms_pod_replacement_controller_test.go#L33-L260: log each predicate, reconciliation, and error-path test step.deploy/operator/internal/podcache/transform_test.go#L153-L183: log each added projection-contract test step.deploy/operator/internal/controller/pod_cache_contract_test.go#L96-L150: log the topology, failover, replacement, snapshot, model, diagnostics, and Recreate test steps.As per coding guidelines, “use
t.Logto explain the test's story, with one heading before each block implementing a test step.”🤖 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 `@deploy/operator/internal/controller/gms_pod_replacement_controller_test.go` around lines 33 - 260, Add t.Log headings before each test-step block in deploy/operator/internal/controller/gms_pod_replacement_controller_test.go (lines 33-260), covering predicate, reconciliation, and error-path cases; deploy/operator/internal/podcache/transform_test.go (lines 153-183), covering each projection-contract case; and deploy/operator/internal/controller/pod_cache_contract_test.go (lines 96-150), covering topology, failover, replacement, snapshot, model, diagnostics, and Recreate steps. Use concise headings that explain each test’s story.Source: Coding guidelines
53-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace bespoke mutation closures with explicit table data.
The
testsmap hides each test mutation in a closure. Keep the mutation inputs in the table and apply them explicitly in the test body.As per coding guidelines, “avoid hiding bespoke test logic in closures.”
🤖 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 `@deploy/operator/internal/controller/gms_pod_replacement_controller_test.go` around lines 53 - 73, Replace the closure-valued tests map in the gmsPodReplacement test with table entries containing explicit mutation data for each case, such as label removal, owner-reference value, init-container restart policy, and restart count. In the t.Run body, apply those table fields directly to the generated pod before asserting pred.Create returns false, preserving all existing scenarios.Source: Coding guidelines
🤖 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 `@deploy/operator/internal/controller/failover_cascade_controller.go`:
- Around line 66-71: Update the cascade delete selector used by the controller’s
DeleteAllOf call to require snapshotprotocol.RestoreTargetLabel is not true,
while preserving the existing failover and Grove label requirements. Add a
regression test covering a terminal non-restore Pod sharing a group with a
restore target, and verify the cascade deletes only the eligible Pod and leaves
the restore target intact.
---
Nitpick comments:
In `@deploy/operator/internal/controller/gms_pod_replacement_controller_test.go`:
- Around line 33-260: Add t.Log headings before each test-step block in
deploy/operator/internal/controller/gms_pod_replacement_controller_test.go
(lines 33-260), covering predicate, reconciliation, and error-path cases;
deploy/operator/internal/podcache/transform_test.go (lines 153-183), covering
each projection-contract case; and
deploy/operator/internal/controller/pod_cache_contract_test.go (lines 96-150),
covering topology, failover, replacement, snapshot, model, diagnostics, and
Recreate steps. Use concise headings that explain each test’s story.
- Around line 53-73: Replace the closure-valued tests map in the
gmsPodReplacement test with table entries containing explicit mutation data for
each case, such as label removal, owner-reference value, init-container restart
policy, and restart count. In the t.Run body, apply those table fields directly
to the generated pod before asserting pred.Create returns false, preserving all
existing scenarios.
In `@deploy/operator/internal/controller/gms_pod_replacement_controller.go`:
- Around line 38-119: add one-line story comments above the requested multi-line
logical blocks: Reconcile, hasRestartedNativeGMSServer, and
gmsPodReplacementPredicate in
deploy/operator/internal/controller/gms_pod_replacement_controller.go; the GMS
init-container selection and restart-count retention blocks in
deploy/operator/internal/podcache/transform.go; controller construction and
registration in deploy/operator/internal/controller/setup.go; and
GMSSnapshot-gated registration in deploy/operator/cmd/main.go. Keep comments
focused on each block’s behavior and do not alter the implementation.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 024582b8-3a64-4a5c-a974-5f729745c58c
📒 Files selected for processing (11)
deploy/operator/cmd/main.godeploy/operator/internal/controller/failover_cascade_controller.godeploy/operator/internal/controller/failover_cascade_controller_test.godeploy/operator/internal/controller/gms_pod_replacement_controller.godeploy/operator/internal/controller/gms_pod_replacement_controller_test.godeploy/operator/internal/controller/pod_cache_contract_test.godeploy/operator/internal/controller/setup.godeploy/operator/internal/dynamo/failover.godeploy/operator/internal/dynamo/failover_test.godeploy/operator/internal/podcache/transform.godeploy/operator/internal/podcache/transform_test.go
Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
julienmancuso
approved these changes
Aug 5, 2026
Contributor
Author
|
/ok to test c7ae994 |
hhzhang16
added a commit
that referenced
this pull request
Aug 6, 2026
dyn-3691-extract-shared-target-pid-cuda-customstorage-operation-layer * 'main' of https://github.com/ai-dynamo/dynamo: (65 commits) fix(frontend): emit SGLang stream role once (#12741) docs(fern): promote v1.3.1 to current release (#12752) fix(docs): remove duplicate unscoped community-rail CSS rules (#12615) feat(operator): migrate CRD storage to v1beta1 (#11904) fix: synchronize self-benchmark capacity across DP ranks (#12021) chore(deps): bump dynamo-tokenizers to 1.8.0 (#12707) fix(frontend): preserve split UTF-8 characters (#12688) docs: align Kubernetes build selector with CLI (#12729) fix(frontend): preserve completion backend error status (#12706) fix(operator): replace snapshot pods after GMS restart (#11286) refactor(media): rename installer module, drop --packages per review fix(media): harden installer against three pre-redesign review findings fix(media): verify installs in a fresh interpreter; teach --pip-args= form test(serve): install test-time decoders at the validated bounds feat(media): explicit installer for additional media decoders docs(spica): correct kv_load_ratio support guidance (#12714) feat(operator): add experimental grove.forceScalingGroup for single-node components (#11772) fix(vllm): declare entry-stage engine_input_source in GLM-Image NIXL config (#12709) chore: bump trtllm to v1.3.0rc23 (#12532) perf: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo (#12592) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
failoverCascadeReconciler, independently registered under theGrovegategmsPodReplacementReconcilerthat UID-safely replaces an owned Snapshot restore-target Pod after its nativegms-serversidecar restartsgms-serveridentity, restart policy, and restart count required by replacement detection in the shared Pod cacheBoth controllers use
For(Pod)with separate queues and predicates. Existing feature gates remain unchanged. This is the first Pod-safety change in #12671; ordinary Snapshot + GMS enablement and Snapshot + failover restore coordination remain follow-up changes.Validation
internal/podcacheandinternal/dynamotests; controller andcmdpackage compilationgo vet ./cmd ./internal/controller ./internal/dynamo ./internal/podcachemake lintmake manifestswith no RBAC, CRD, webhook, or Helm driftgofmtandgit diff --checkThe Kubernetes envtest suite was not run locally because its
etcdtest binary is unavailable; the controller package compiles and all focused controller coverage passes.Summary by CodeRabbit
New Features
Bug Fixes
Tests