fix(operator): bind workloads to verified checkpoints - #12873
fix(operator): bind workloads to verified checkpoints#12873galletas1712 wants to merge 1 commit into
Conversation
github-actions
Bot
added
fix
deployment::k8s
|
b35cb65 to
a57a93a
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| actual := &nvidiacomv1alpha1.DynamoCheckpoint{} | ||
| if err := r.reader.Get(ctx, types.NamespacedName{ | ||
| Namespace: namespace, | ||
| Name: info.CheckpointName, | ||
| }, actual); err != nil { | ||
| return fmt.Errorf("get checkpoint %q: %w", info.CheckpointName, err) | ||
| } |
There was a problem hiding this comment.
🟡 First deployment of a Grove workload can fail because the just-created checkpoint is looked up before it is visible
The automatic checkpoint is re-read from the controller's local cache (r.reader.Get at deploy/operator/internal/controller/dgd_grove_workload_renderer.go:148-153) in the same pass that just created it, so the very first deployment attempt usually fails with a "not found" error instead of proceeding.
Impact: New checkpoint-enabled Grove deployments report a spurious failure and only converge on a later retry, delaying rollout and emitting misleading errors.
Read-your-own-write through the informer cache
In one DGD reconcile pass dgd_shared_resources_reconciler.go:91 runs dgdCheckpointsReconciler.Reconcile, which calls checkpoint.CreateOrGetAutoCheckpoint and creates the DynamoCheckpoint (deploy/operator/internal/controller/dgd_checkpoints_reconciler.go:169-194), then computes info.AutoBinding from the object returned by the create call. Immediately afterwards, groveProgram.Reconcile (deploy/operator/internal/controller/dgd_grove_program.go:104-116) invokes groveWorkloadRenderer.Render, which calls verifyAutomaticCheckpointBindings and Gets the same checkpoint through r.reader — the manager's cached client (newGroveWorkloadRenderer(kubeClient, ...) at deploy/operator/internal/controller/dgd_grove_workloads_reconciler.go:57-63).
The informer cache has almost certainly not observed the create yet, so Get returns NotFound and the renderer returns get checkpoint %q: not found, aborting the whole Grove reconcile before the PodCliqueSet is created or updated. deploy/operator/internal/controller/AGENTS.md states: "After a successful write, either continue with the object returned by the client or wait for its watch event" and "do not turn expected informer lag into a terminal failure".
A fix would be to verify against the checkpoint object already returned by CreateOrGetAutoCheckpoint (which is what produced AutoBinding), or to treat a cache miss / mismatch for a checkpoint created in this pass as pending and requeue rather than as an error.
Prompt for agents
In deploy/operator/internal/controller/dgd_grove_workload_renderer.go, verifyAutomaticCheckpointBindings re-reads each automatic DynamoCheckpoint through the manager's cached reader. In the same reconcile pass, dgdCheckpointsReconciler.Reconcile has just created that checkpoint (see deploy/operator/internal/controller/dgd_checkpoints_reconciler.go where AutoBinding is computed from the object returned by CreateOrGetAutoCheckpoint), so the informer cache typically has not observed it yet and the Get fails with NotFound, which aborts the entire Grove render and PodCliqueSet reconciliation. Per deploy/operator/internal/controller/AGENTS.md, the reconciler should continue with the object returned by the write, or treat informer lag as pending rather than a terminal failure. Consider threading the freshly created/adopted checkpoint object (or its verified binding) from the checkpoints reconciler into the Grove renderer instead of re-reading, or classifying a NotFound/stale result for a checkpoint created in this pass as pending and requeuing.
Was this helpful? React with 👍 or 👎 to provide feedback.
| func TestAutomaticCheckpointBinding(t *testing.T) { | ||
| owner := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-dgd", Namespace: testNamespace, UID: types.UID("dgd-uid"), | ||
| }} | ||
| ckpt := mustExpectedAutoCheckpoint( | ||
| t, | ||
| testScheme(), | ||
| testIdentity(), | ||
| corev1.PodTemplateSpec{Spec: corev1.PodSpec{ | ||
| Containers: []corev1.Container{{ | ||
| Name: consts.MainContainerName, | ||
| Image: "worker:expected", | ||
| }}, | ||
| }}, | ||
| consts.MainContainerName, | ||
| nvidiacomv1alpha1.CheckpointDeletionPolicyDelete, | ||
| nil, | ||
| owner, | ||
| ) | ||
| ckpt.UID = types.UID("checkpoint-uid") | ||
| ckpt.Generation = 7 | ||
| ckpt.Status.CheckpointID = testHash | ||
|
|
||
| binding, err := AutomaticCheckpointBinding(ckpt) | ||
| require.NoError(t, err) | ||
| assert.Regexp(t, `^v1/checkpoint-uid/7/[0-9a-f]{64}$`, binding) | ||
| recomputed, err := AutomaticCheckpointBinding(ckpt.DeepCopy()) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, binding, recomputed) |
There was a problem hiding this comment.
🟡 New checkpoint binding test does not narrate its steps as required by the repository test style
The new binding test contains no t.Log step headings (TestAutomaticCheckpointBinding at deploy/operator/internal/checkpoint/checkpoint_test.go:125-243), violating the mandatory Go test style for this module.
Impact: Test output does not tell the scenario's story, which the repository requires for reviewability.
Rule reference
deploy/operator/AGENTS.md, "Go Test Style": "Use t.Log to tell the test's story, with one heading before each block that implements a test step." The test has distinct steps (build the expected automatic checkpoint, compute and re-compute the binding, run the mutation table) with no headings. Other new tests added in this PR (for example TestValidateGroveCheckpointBindings) do follow the rule.
Was this helpful? React with 👍 or 👎 to provide feedback.
| func TestDCDRendererRejectsMissingOrStaleAutomaticCheckpointBinding(t *testing.T) { | ||
| ckpt := &v1alpha1.DynamoCheckpoint{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "checkpoint-worker", | ||
| Namespace: "default", | ||
| UID: types.UID("checkpoint-uid"), | ||
| Generation: 3, | ||
| Labels: map[string]string{ | ||
| snapshotprotocol.CheckpointIDLabel: "checkpoint-id", | ||
| }, | ||
| Annotations: map[string]string{ | ||
| commonconsts.CheckpointAutoAnnotation: commonconsts.KubeLabelValueTrue, | ||
| }, | ||
| }, | ||
| } | ||
| unmarked := ckpt.DeepCopy() | ||
| unmarked.Name = "unmarked-checkpoint" | ||
| delete(unmarked.Annotations, commonconsts.CheckpointAutoAnnotation) | ||
| staleGeneration := ckpt.DeepCopy() | ||
| staleGeneration.Generation-- | ||
| staleGenerationBinding, err := checkpoint.AutomaticCheckpointBinding(staleGeneration) | ||
| require.NoError(t, err) | ||
| replaced := ckpt.DeepCopy() | ||
| replaced.UID = types.UID("old-uid") | ||
| replacedBinding, err := checkpoint.AutomaticCheckpointBinding(replaced) | ||
| require.NoError(t, err) | ||
| matchingBinding, err := checkpoint.AutomaticCheckpointBinding(ckpt) | ||
| require.NoError(t, err) | ||
| reader := fake.NewClientBuilder(). | ||
| WithScheme(scheme.Scheme). | ||
| WithObjects(ckpt, unmarked). | ||
| Build() |
There was a problem hiding this comment.
🟡 New renderer binding test does not narrate its steps as required by the repository test style
The new renderer rejection test contains no t.Log step headings (TestDCDRendererRejectsMissingOrStaleAutomaticCheckpointBinding at deploy/operator/internal/controller/dynamocomponentdeployment_controller_test.go:77-170), violating the mandatory Go test style for this module.
Impact: Test output does not tell the scenario's story, which the repository requires for reviewability.
Rule reference
deploy/operator/AGENTS.md, "Go Test Style": "Use t.Log to tell the test's story, with one heading before each block that implements a test step." This test builds several checkpoint fixtures, runs a rejection table, and then asserts the unmarked-checkpoint case, all without headings.
Was this helpful? React with 👍 or 👎 to provide feedback.
| currentMetadata, currentTemplate := checkpointBindingCopies(current) | ||
| if currentMetadata == "" || currentTemplate == "" { | ||
| return fmt.Errorf( | ||
| "automatic checkpoint binding copies are required on existing DynamoComponentDeployment %s/%s; recreate the workload", | ||
| current.Namespace, | ||
| current.Name, | ||
| ) | ||
| } | ||
| if currentMetadata != currentTemplate { | ||
| return fmt.Errorf( | ||
| "automatic checkpoint binding copies disagree on DynamoComponentDeployment %s/%s", | ||
| current.Namespace, | ||
| current.Name, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔍 Existing DCDs and PodCliqueSets without binding copies become permanently unreconcilable after upgrade
validateCheckpointBinding fails closed whenever the desired object carries a binding but the live DynamoComponentDeployment does not have both durable copies (metadata annotation and pod-template annotation), returning ...copies are required on existing DynamoComponentDeployment ...; recreate the workload. validateGroveCheckpointBindings (deploy/operator/internal/controller/dgd_grove_workload_renderer.go:249-256) does the same for existing PodCliqueSet cliques. Every checkpoint-enabled workload created by an operator version prior to this PR lacks those copies, so the first reconcile after upgrade returns an error for that DGD and never converges until an operator manually deletes the DCD/PCS — which for Grove means deleting all running pods. The behavior is clearly deliberate (both tables assert it), but the upgrade path deserves an explicit migration note or a one-time adoption branch that stamps the copies when the live object's referenced checkpoint still verifies.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Resolve and verify the exact automatic checkpoint object. | ||
| ckpt := &nvidiacomv1alpha1.DynamoCheckpoint{} | ||
| if err := h.client.Get(ctx, types.NamespacedName{ | ||
| Namespace: podNamespace, | ||
| Name: checkpointName, | ||
| }, ckpt); err != nil { | ||
| return admission.Denied("bound automatic checkpoint is unavailable") | ||
| } | ||
| if err := verifyBoundAutomaticCheckpoint(ckpt, binding); err != nil { | ||
| logger.Error(err, "already-shaped restore target rejected because its binding is invalid", | ||
| "namespace", podNamespace, "pod", pod.Name, "checkpoint", checkpointName) | ||
| return admission.Denied("automatic checkpoint binding is missing or stale") | ||
| } | ||
| return admission.Allowed("bound pod is already checkpoint-shaped") |
There was a problem hiding this comment.
🔍 Bound restore pods are denied at admission when their checkpoint disappears
The already-shaped branch now denies pod creation whenever the pod carries a binding and the referenced DynamoCheckpoint cannot be read or no longer carries the automatic marker. Previously any already-shaped pod was admitted unchanged. Consequently, if the automatic checkpoint is deleted (or transiently missing from the webhook client's cache) while a bound workload is still running with a stamped pod template, every replacement pod for that workload is rejected, so the workload cannot recover replicas until the controller re-renders the pod template. Worth confirming that the DGD/DCD controllers always strip the binding annotations promptly when the checkpoint goes away, and that the webhook's client read is reliable (a cache miss here turns into a hard denial, not a retry).
Was this helpful? React with 👍 or 👎 to provide feedback.
| autoBinding := "" | ||
| automatic := ckpt.Annotations[consts.CheckpointAutoAnnotation] == consts.KubeLabelValueTrue | ||
| if automatic && ckpt.UID != "" && ckpt.Generation > 0 { | ||
| autoBinding, err = AutomaticCheckpointBinding(ckpt) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Binding computation now fails resolution for any automatic checkpoint whose ID label diverges
checkpointInfoFromObject now calls AutomaticCheckpointBinding for every checkpoint carrying the automatic marker, and automaticCheckpointBindingFor (deploy/operator/internal/checkpoint/resource.go:399-403) hard-errors when labels[CheckpointIDLabel] differs from the canonical CheckpointID(ckpt) (which prefers status.checkpointID, then status.identityHash, then the label). Any legacy automatic checkpoint whose status.identityHash was written from the identity hash rather than the DGD-scoped checkpoint ID will therefore make ResolveCheckpointForService return an error, failing the whole DGD/DCD reconcile rather than degrading to "no binding". The current controller keeps label and status in sync (internal/controller/dynamocheckpoint_controller.go:132-160), so this only affects objects written by older versions; still, an error here is unrecoverable without manual edits.
Was this helpful? React with 👍 or 👎 to provide feedback.
a57a93a to
d11873b
Compare
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| current *nvidiacomv1beta1.DynamoComponentDeployment, | ||
| desired *nvidiacomv1beta1.DynamoComponentDeployment, | ||
| ) error { | ||
| const annotation = consts.CheckpointBindingAnnotation |
There was a problem hiding this comment.
validateCheckpointBinding declares annotation but never uses it, so the controller package will not compile. Fix: remove the unused local constant.
🤖 AI Fix
In deploy/operator/internal/controller/dgd_component_workloads_reconciler.go, inside validateCheckpointBinding, delete the line const annotation = consts.CheckpointBindingAnnotation.
| ArtifactVersion string `json:"artifactVersion"` | ||
| DeletionPolicy string `json:"deletionPolicy"` | ||
| Controller *metav1.OwnerReference `json:"controller,omitempty"` | ||
| CaptureSpec nvidiacomv1alpha1.DynamoCheckpointSpec `json:"captureSpec"` |
There was a problem hiding this comment.
Including deletion policy and controller owner references in the binding digest makes a lifecycle-only deletionPolicy change produce a new binding that the DCD/Grove validators reject as immutable. Fix: keep mutable lifecycle metadata out of the workload binding digest.
🤖 AI Fix
In deploy/operator/internal/checkpoint/resource.go, change the provenance used by AutomaticCheckpointBinding/VerifyAutomaticCheckpointBinding so it omits CheckpointDeletionPolicyAnnotation and metav1.GetControllerOf(ckpt), while leaving VerifyExpectedAutoCheckpoint's lifecycle validation unchanged.
d11873b to
0263444
Compare
0263444 to
4d703a2
Compare
6acae2b to
e8b2de8
Compare
e8b2de8 to
6dfe3fb
Compare
6dfe3fb to
f89cf09
Compare
Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
f89cf09 to
b941527
Compare
Final exact-source AKS validation (72295be)
|
1 participant
Summary
This is the seventh layer of the eight-PR stack for DEP #12671. It durably binds generated workloads to the exact automatic checkpoint verified by #12872.
A checkpoint can be deleted, replaced under the same name, or changed after a workload is rendered. Name-only lookup would let that workload restore from a different artifact.
The binding format is:
UID detects same-name replacement, generation detects specification changes, and the digest detects metadata-only provenance changes such as artifact version, checkpoint ID, deletion policy, or owner changes.
flowchart LR C["Verified DynamoCheckpoint"] --> B["Versioned binding"] B --> D["DynamoComponentDeployment"] B --> G["Grove engine clique"] D --> P["Restore-target Pod"] G --> P P --> V["Admission re-reads and verifies checkpoint"]DCD metadata and Pod-template copies must both exist and agree. Grove stores a checkpoint map at the PodCliqueSet and a singular binding only on the relevant engine clique; GMS and unrelated cliques are not stamped. Bound, already-shaped restore Pods are revalidated before admission returns, while checkpoint source Pods and ordinary unbound Pods retain existing behavior.
This binding depends on #12902 preserving explicit checkpoint identity before #12872 verifies and hashes its provenance; otherwise the durable digest could bind false normalized TP1 metadata for a real TP2 capture.
Missing, malformed, legacy, divergent, replaced, or stale bindings fail closed.
Validation
go vet, DCO, signature, and diff-hygiene checks passedFinal eight-branch chain validation
go test ./internal/checkpoint ./internal/controller ./internal/webhook/validation -count=1.make lint, invokinggolangci-lint v1.64.8.make lint,go test ./internal/criu -count=1,go test -race ./internal/criu -count=1,go test ./... -count=1, andgo vet ./...fromdeploy/snapshot.Live AKS TP2 identity validation
At exact eight-branch composite
d209f77f91911a4267dea203490a0e410b9e1b1e:sha256:fba2f709673f7949b3da463a577e18522a11bd35ca08634c1ff06329529077aa(amd64 manifestsha256:dc2d076270079a7e4f65e04eacda6c7227a4870e1c1e75fc4c8d38de6b552b5b); CRDs were untouched.checkpoint-5d2f7c0d202210efee4455fa50f3727d(UID89acc792-c13d-4302-9dab-bb4e8e143eac) became Ready. Its live identity was exactly modelQwen/Qwen3-0.6B, backendvllm, TP=2, and PP=1; customextrametadata was preserved, while spoofed reserveddgdUID,component, andcheckpointIDvalues were overwritten correctly.device-0anddevice-1each contained 24 allocations totaling 610,271,232 bytes.v1/89acc792-c13d-4302-9dab-bb4e8e143eac/1/942c43260c8204ad6867ebea038ea31cc37b390d406fbf1ee1f6d9cd700b1c73both reachedRestoreSucceeded, ranks 0/1, two GPUs/loaders, and inference HTTP 200; restore totals were approximately 21.53s and 20.95s.Scope: This proves ordinary TP2 Snapshot capture/restore plus corrected identity, provenance, and immutable binding. TP2 automatic failover remains unsupported and is not claimed; Snapshot-backed automatic failover remains limited to TP=PP=DP=1.
Full CI is not claimed.
Stack
GitHub Stack: #12903