feat: publish checkpoint source facts - #63
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (11)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. WalkthroughThe change adds checkpoint provenance types and CRD fields. The controller reads manifests beside artifacts, projects captured node, pod, and NVIDIA facts, and publishes them when ChangesCheckpoint source publication
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change publishes checkpoint source facts alongside Ready status without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Capture
participant Controller
participant Manifest
participant PodSnapshotContent
Capture->>Controller: artifact path
Controller->>Manifest: read checkpoint manifest
Manifest-->>Controller: compatibility facts
Controller->>PodSnapshotContent: publish Ready status and source facts
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 46.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 10 files. (1 skipped: 1 unsupported.) Full details: Breaking Api ChangesExplanation No breaking API change matches the check. The only existing Kubernetes API struct changed is Full details: Rbac Least PrivilegeExplanation No RBAC least-privilege violation was introduced. The complete PR diff from merge-base c1206a4 to HEAD changes no kubebuilder RBAC marker or Helm Role/ClusterRole grant. All current markers and Helm RBAC rules use explicit verbs and resources; an exact wildcard search found no
Comment |
9f55d79 to
820a149
Compare
c7399cb to
c6fbe01
Compare
c6f8a42 to
77b1951
Compare
77b1951 to
e773be3
Compare
235bbd8 to
50f501c
Compare
e773be3 to
007f46d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1a25a63 to
995d159
Compare
Everything that decides whether a checkpoint can be restored is read at capture time and lands in the artifact's manifest, where nobody preparing a restore target can see it. PodSnapshotContent gains an optional status.source so those facts are visible on the object the user already has, starting with the machine the capture ran on. The block is projected from the manifest and never read back: the restore compatibility gates compare the manifest, so this cannot become a second source of truth. Nothing is required, so an artifact captured before a fact was recorded still publishes. Signed-off-by: liran elisha <lirane@nvidia.com>
A restore has to offer the container the same image and at least the resources it had, so the source status now names them: the image, the digest that pins which build of it ran, and the memory and CPU limits. The digest is published beside the reference rather than folded into it, because a tag is mutable and the two answer different questions. Signed-off-by: liran elisha <lirane@nvidia.com>
The GPU model, the driver version and how many GPUs the capture could see are what a restore has to match on the device side, so they join the source status. Vendors are plain properties rather than a discriminated union: each payload stays typed, validated by the API server and visible through kubectl explain, and several vendors can be present without a selector or a CEL rule tying them together. An unknown vendor is preserved rather than pruned, so a newer agent's facts survive an older installed CRD. The GPU count is the length of the instance list rather than a field of its own, so the two cannot disagree. UUIDs are left out: they name a physical card instead of describing what a target has to offer. Signed-off-by: liran elisha <lirane@nvidia.com>
…tured from The facts are already written beside every artifact; publishing them is a matter of naming them the way the API does. The projection reads the same CompatFacts view the restore gates compare, so what a user sees cannot drift from what a restore is actually checked against. The digest is the one fact that cannot be copied straight across: runtimes wrap it differently and the artifact keeps whichever form it was handed. So compat.ImageDigest is exported and reused rather than reimplemented, and the reduction a user reads is the one the gate compares. A block with nothing recorded in it is left out rather than published blank, and a manifest with nothing to say at all projects to nothing, so an absent fact reads as unknown instead of as a value that happens to be empty. Nothing calls this yet. Signed-off-by: liran elisha <lirane@nvidia.com>
The facts ride along on the write that already announces the capture, so a content that is Ready carries what it was captured on in the same observation. Nothing polls, and no second write can be seen half-applied. The manifest is read once before the conflict-retry loop rather than per attempt, and a manifest that cannot be read costs the facts and not the capture: the artifact is already committed, so Ready is written either way and the failure is logged. Signed-off-by: liran elisha <lirane@nvidia.com>
A capture reaches Ready three ways: the dump finishing, an artifact recovered after the agent died mid-write, and a work order reconciled again once the killed source pod is gone. The facts come off the artifact rather than out of memory, so all three publish them, and a test per path is what keeps that true as the paths change. Two more sit beside them, for the paths that decide whether the facts survive rather than which write publishes them: a Ready write that only lands after a conflict still carries them, and a read that came back empty does not erase what an earlier write already published. Signed-off-by: liran elisha <lirane@nvidia.com>
The point of publishing is that nobody has to exec into an agent pod to see what a snapshot came from, so the assertion goes against the object a user would actually read. It compares the published block to the node and to nvidia-smi rather than to the manifest, so a projection that renamed a field or dropped one is caught instead of agreeing with itself. The pod block is compared whole, because the CPU limit this workload never set has to stay absent there too. Signed-off-by: liran elisha <lirane@nvidia.com>
007f46d to
a230fe1
Compare
Summary
PodSnapshotContentgains an optionalstatus.sourcepublishing what a checkpoint was captured on: the node, the container, and the devices.manifest.yamland written in the same status patch that marks the captureReady, so a content that is Ready already carries its facts.Design: #99, section 7.1. Stacked on #140, which records the facts this publishes.
What it looks like
instances[].productNameanddriverVersionare the names the NVIDIA DRA driver publishes on aResourceSlice, so the same GPU reads the same way in both places. The GPU count is the length of the instance list rather than a field of its own, so the two cannot disagree.How to read this
The 7 commits form a ladder; each introduces one idea with its own tests.
compat.ImageDigest. Nothing calls it yetDecisions worth a look
Absent, not empty. A fact the agent could not read is left out, and a block with nothing in it is omitted entirely. An artifact captured before a fact was ever recorded still publishes; a reader can tell unknown from a value that happens to be blank.
The published digest is the compared digest.
compat.ImageDigestis exported rather than reimplemented, so the reduction that decides whether two images are the same content is the one this publishes. Runtimes wrap a digest differently and the artifact keeps whichever form it was handed, so a second implementation could show a value the gate never compares.One read, not one per retry. The Ready write retries on optimistic-lock conflict, so the manifest is read once before the loop.
Losing the facts never costs the capture. By the time this runs the artifact is committed, so an unreadable manifest logs and Ready is written anyway.
No
node.agentVersionyet. The design lists it, but #140 dropped agent-version recording along with the major/minor comparison, so the field would only ever be empty. It arrives with the deferred agent-version compatibility mapping, which is also what will give it a meaning.No new RBAC. The agent already holds
podsnapshotcontents/statusupdateandpatch.PodSnapshotkeeps mirroring only Ready and Failed.Single container. Capture is still one container per content (
PodReference.ContainersisMinItems=1/MaxItems=1), so onesourceblock per content is correct. Multi-container restore does not change that; if multi-container capture arrives, this block is where it grows a key.Verification
go vet ./...andgo test ./...for the agent under Linux,go test ./...for the api module, at each rung).make verify-crdsclean, so the chart CRD copy matches the generated one; license headers, fmt, tidy and helm lint clean; govulncheck clean.Summary by CodeRabbit
New Features
Bug Fixes
Superseded by #141 after removing the internal task reference from the head branch.