Repository navigation
refactor!: say checkpoint and snapshot, not capture - #794
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Rename timeout configuration keys agent/internal/types/config.go, agent/internal/types/config_test.go, agent/internal/controller/*_test.go, charts/snapshot/values.yaml, charts/snapshot/templates/configmap.yaml, charts/snapshot/tests/config_test.yaml, CHANGELOG.md, docs/development/naming.md, docs/operations/troubleshooting.md, docs/reference/api.md |
Checkpoint and restore timeout fields and YAML keys now use TimeoutSeconds and timeoutSeconds. Helm emits the new keys and deprecated keys from the new values for this release, and rejects values supplied under the old Helm keys. Defaults, conversions, bounds checks, and timeout test expectations remain unchanged. |
Update API and resource descriptions api/v1alpha1/*, api/v1alpha1/crds/*, docs/reference/api.md, docs/reference/compatibility.md, docs/reference/restore-pod-contract.md |
API comments, CRD descriptions, and reference text use updated snapshot and checkpoint terminology. Field declarations and validation rules remain unchanged. |
Update operator messages and guidance operator/cmd/snapshotctl/*, operator/internal/controller/*, operator/internal/maintenance/*, charts/snapshot/README.md, charts/snapshot/values.yaml, docs/*, README.md, CHANGELOG.md |
Operator messages and user guidance use checkpoint or snapshot terminology in place of capture-related wording. The documented conditions, outcomes, and workflows remain unchanged. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Merge Risk: 
db71f
-
Helm-managed upgrades preserve startup compatibility, but externally managed installations still lack required migration instructions, and remaining documentation inconsistencies could mislead storage and troubleshooting operations.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title uses the valid Conventional Commits prefix refactor!, stays under 72 characters, and accurately summarizes the terminology and configuration-key refactor.Linked Issues check No active directly linked issue remains. Issue #688is closed and supplies historical context only. Therefore, no linked-issue coding requirement applies. The reviewed changes still implement the docu…Out of Scope Changes check The changes stay within the current PR scope. They update checkpoint and snapshot terminology in user-facing documentation, CRD descriptions, chart text, API comments, and messages. They rename the tw… Breaking Api Changes PASS. The PR changes only comments and generated CRD descriptions/messages under api/**. The exported Go field names, Go types, and JSON tags are unchanged; no API fields were added or removed. The `s… Rbac Least Privilege No wildcard RBAC grant is present or introduced. The kubebuilder markers use explicit verbs and resources, and the Helm RBAC templates use explicit lists only. The PR changes no RBAC manifest or RBAC … Full details: Linked Issues check
-
Explanation
No active directly linked issue remains. Issue
#688is closed and supplies historical context only. Therefore, no linked-issue coding requirement applies. The reviewed changes still implement the documented timeout-key rename and update related tests and Helm rendering.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within the current PR scope. They update checkpoint and snapshot terminology in user-facing documentation, CRD descriptions, chart text, API comments, and messages. They rename the two agent timeout keys and add Helm tests and rollout compatibility handling. The excluded condition strings, labels, volume name, agent step name, manifest paths, and detailed architecture terminology are not reported as changed. No unrelated change is demonstrated.
Full details: Breaking Api Changes
-
Explanation
PASS. The PR changes only comments and generated CRD descriptions/messages under api/**. The exported Go field names, Go types, and JSON tags are unchanged; no API fields were added or removed. The
self == oldSelfimmutability XValidation markers onPodSnapshotandPodSnapshotContentare unchanged. No required-field compatibility issue is introduced.
Full details: Rbac Least Privilege
-
Explanation
No wildcard RBAC grant is present or introduced. The kubebuilder markers use explicit verbs and resources, and the Helm RBAC templates use explicit lists only. The PR changes no RBAC manifest or RBAC marker.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 11 functions across 16 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
ozbarshalom
left a comment
ozbarshalom
left a comment
There was a problem hiding this comment.
Checked the sweep against the naming guide. Old wording is still left in places this PR covers. Fix here or track:
charts/snapshot/README.md:16, 28, 57, 105, 198andcharts/snapshot/values.yaml:111, 166, 170, 231, 233: "checkpoint PVC", "checkpoint image", "checkpoint directory". Line 16 is in the same paragraph as a line this PR changed to "snapshot store".README.md:117, 179anddocs/operations/install.md:20: "checkpoint volume". The README links to a page now headed "The snapshot volume".docs/operations/storage.md:158, 300, 301,docs/development/benchmarks-guide.md:24anddocs/operations/troubleshooting.md:277.docs/operations/troubleshooting.md:470: the heading "capturing rootfs diff". The description listscheckpointas a noun in the agent step names as a known deviation, but not thiscapturingheading.- Follow-up: the agent step names and the
capture/manifest directories in the framework guides still use the old terms. Is there an issue to link?
|
@ozbarshalom thanks — all of those were real misses. Fixed in 438c247, and I swept the rest of the same patterns rather than only the lines you cited, which turned up a few more:
On the two follow-ups:
An issue to link — #744 stays open and covers step 2 (the API rename). It doesn't currently enumerate the agent step names or the One thing your review led me to that wasn't in it: the timeout rename had a rollout hole. The ConfigMap updates in one shot while the DaemonSet rolls node by node, and the agent reads config only at startup and |
The naming guide (docs/development/naming.md) settled the phase names: a snapshot is the saved state of a pod, checkpoint is the verb, and capture is the mechanism inside a checkpoint — internal only, for code, comments and the detailed architecture pages. Sweep the user-facing pages to match: capture becomes checkpoint, checkpoint used as a noun for the artifact becomes snapshot, and checkpoint store/volume/artifact become snapshot store/volume/snapshot. The same wording goes through the CRD field descriptions that `kubectl explain` renders, the chart README, and the condition and failure messages. No condition type, reason or label changed, so nothing matching on those strings has to move. Step 2 of #744, the API rename, stays open. BREAKING CHANGE: the two agent timeout values lost their redundant prefixes (#688). config.checkpoint.checkpointTimeoutSeconds is now config.checkpoint.timeoutSeconds, and config.restore.restoreTimeoutSeconds is now config.restore.timeoutSeconds. Defaults are unchanged, and a chart upgrade moves both because the ConfigMap and the DaemonSet roll together. Refs #744 Closes #688 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com>
ozbarshalom spotted old wording this PR had left behind: "checkpoint PVC", "checkpoint volume", "checkpoint image", "checkpoint directory" and "checkpoint files" across the chart README and values, the root README, install, storage, benchmarks-guide, troubleshooting, compatibility and the architecture overview. Swept them, and the rest of the same patterns rather than only the lines cited. Two accuracy fixes from the bot review: - PodSnapshot.spec.source.podRef said the operator prepares the pod with a "snapshot volume mount". It does not — EnsureControlVolume injects only the snapshot-control emptyDir, and the node agent is what mounts the store. - "Workload pods never mount the snapshot store" is not true of a non-target artifact writer, which supplies the mount itself. The timeout rename also had a rollout hole. The ConfigMap updates in one shot while the DaemonSet rolls node by node, and the agent reads its config only at startup, failing fatally when the restore timeout is absent. A v0.2.0 pod that restarted before the roll reached it would crash-loop. Render both spellings for this release, and refuse to render when a value is still set under an old key, because Helm drops an unknown value silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com>
438c247 to
bbf880b
Compare
The new timeout-key assertions were written against `helm template` output, where config.yaml is indented a further four spaces under `data:`. The value helm-unittest matches is the string itself, which indents two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com>
Nothing else in the file marks a break, so whoever writes the release notes has to find it. Also say plainly that an install which never set either value needs no action. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Dan Bar <118804663+danbar2@users.noreply.github.com>
main renamed config.checkpoint.checkpointTimeoutSeconds and config.restore.restoreTimeoutSeconds to config.checkpoint.timeoutSeconds and config.restore.timeoutSeconds (#794). Use the new names in the API reference and the lifecycle hooks design. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: shayasoolin <shay.asoolin@run.ai>
main renamed config.checkpoint.checkpointTimeoutSeconds and config.restore.restoreTimeoutSeconds to config.checkpoint.timeoutSeconds and config.restore.timeoutSeconds (#794). Use the new names in the API reference and the lifecycle hooks design. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: shayasoolin <shay.asoolin@run.ai>
main renamed config.checkpoint.checkpointTimeoutSeconds and config.restore.restoreTimeoutSeconds to config.checkpoint.timeoutSeconds and config.restore.timeoutSeconds (#794). Use the new names in the API reference and the lifecycle hooks design. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: shayasoolin <shay.asoolin@run.ai>
What
The naming guide settled the phase names: a snapshot is the saved state of a pod, checkpoint is the verb, and capture is the mechanism inside a checkpoint — internal only, for code, comments and the detailed architecture pages.
This is step 3 of #744 (align the docs). Step 2, the API rename, is deliberately not in here — see below.
capture→ checkpoint,checkpointused as a noun for the artifact → snapshot, andcheckpoint store/volume/artifact→ snapshot store/volume/snapshot.kubectl explainrenders, the chart README, and the condition and failure messages.Breaking change
config.checkpoint.checkpointTimeoutSecondsconfig.checkpoint.timeoutSecondsconfig.restore.restoreTimeoutSecondsconfig.restore.timeoutSecondsDefaults are unchanged, and a normal
helm upgrademoves both because the ConfigMap and the DaemonSet roll together. An installation that manages the agent ConfigMap itself must rename the keys: left under the old names the checkpoint timeout silently falls back to3600, and the agent refuses to start at all without a restore timeout (RestoreSpec.TimeoutSecondsis a plainintwhoseValidate()rejects<= 0).What is NOT in here
No condition type, reason or label changed, so nothing matching on those strings has to move. Step 2 of #744 —
Captured→Checkpointed, the fourCapture*reasons, thenvidia.com/snapshot-capture-eligiblelabel,operator.testFailCaptureSourceAnnotation— is still open and wants its own PR, since it breaks every external consumer that matches on those strings (Dynamo, scripts, dashboards, alerts).Further deviations spotted while sweeping, all left alone as out of scope:
capturing rootfs diff(StepCaptureOverlay) is the one remaining user-visible string that sayscapture. It is a one-line change inagent/internal/executor/step.goplus its troubleshooting heading, but it is a message rename, so it belongs with step 2.checkpointas a noun —resolving checkpoint artifact,reading checkpoint manifest,checking the staged checkpoint. They are enforced verbatim againstagent/internal/executor/step.gobyverify-troubleshooting-steps, so renaming them moves the troubleshooting headings too.capture/manifest directories underdocs/guides/<framework>/are paths users type. Renaming them ripples into the e2e framework resolver and the benchmark runner.v1alpha1.CheckpointVolumeName = "checkpoint-storage"is a Kubernetes volume name that appears in live pod specs.docs/reference/architecture/multi-gpu.mdkeeps bothcapture(the mechanism) andcheckpointas a noun in its own headings and anchors. It is a detailed architecture page, which the guide exempts.The detailed architecture pages keep
capturefor the mechanism, as the guide allows, and CUDA's own "graph capture" is untouched.Test plan
make verify-crds verify-toc verify-troubleshooting-steps helm-lint— passgolangci-lint— 0 issues acrossapi,agent(cross-compiledlinux/amd64) andoperatorgo test ./...— pass inapiandoperator; agent packages pass in thelinux/amd64containerhelm templaterenderstimeoutSeconds: 3600/7200under the new keysmain)Refs #744
Closes #688
🤖 Generated with Claude Code
Summary by CodeRabbit
timeoutSeconds; defaults and timeout behavior are unchanged. Chart-generated configuration includes both current and legacy keys, while Helm values using the old setting names are rejected. Update Helm values to use the new names.