Fix generic CSI changeID retrieval and honor snapshot class deletion policy for CBT retention

[kaovilai]

Note

Responses generated with Claude

Does your change fix a particular issue?

Fixes #10294

Also fixes the snapshot-retention gap discussed on #9714 (RetainSnapshot volume-policy parameter from the design doc, never implemented) — no separate issue number, since that discussion lives on #9714 itself.

Summary

Two independent fixes to the CSI snapshot exposer, landed together since they touch the same file and were both needed to get a working Ceph incremental end-to-end:

1. Generic changeID retrieval (#10294). createBackupVSC constructs the backup VolumeSnapshotContent with only ObjectMeta and Spec — it never sets Status — and returns the object straight from Create(). So vsc.Status is nil unconditionally, not merely until some async fill completes; nothing re-fetches it before getCBTInfo reads it. The generic branch (every driver except vSphere, which takes the annotation branch instead) therefore gets an empty changeID on 100% of runs. Fix: read backupVSC.Spec.Source.SnapshotHandle (set at creation from the ready source VSC's Status.SnapshotHandle) as a fallback when Status isn't populated.

2. Snapshot retention / Case-2 support (design doc lines 362-374 on #9714). The backup VSC's DeletionPolicy was hardcoded to Delete, so any driver requiring the base snapshot to persist for GetMetadataDelta (Ceph RBD — "Case 2" in the design doc) had its base snapshot deleted immediately after upload, making every incremental fall back to full. Fix: honor the source VSC's (or its VolumeSnapshotClass's) own DeletionPolicy instead of hardcoding Delete.

Minor accompanying fix: the retained= log field was (retained != nil), but RetainVSC returns non-nil on both the already-Retain and just-patched branches, so the field was always true regardless of whether a patch actually occurred. Now reflects whether a patch happened.

Measured before/after (live Ceph/ODF validation)

Before: every incremental on Ceph RBD fell back to a full transfer (100% of device size) because the base snapshot was gone by the time GetMetadataDelta ran.

After: an incremental with a 20 MiB delta moved 20,971,520 B — exactly the written delta, not the device size. Restore verified byte-for-byte against source (all checksums matched).

Live validation

Both fixes here were developed and validated live on a real Ceph/ODF cluster as part of a combined branch carrying all five fixes from this campaign together, since this changeID fix is the prerequisite that makes every other fix's incremental-path testing possible — full diff: main...kaovilai:velero:ceph-changeid

• Live on Ceph/ODF (hand-applied v1beta1 SnapshotMetadataService CRD + upstream sidecar v1.1.0): changeID round-trips correctly through a full→incremental pair; base snapshot survives when DeletionPolicy: Retain; incremental delta is exact; restore is byte-identical.
• Unit-only: the retained= log field correctness (not independently exercised live, verified by code inspection and the existing test fixture).

What was deliberately not changed

This surfaces a consequence that is intentionally not fixed here: once base snapshots are retained, nothing currently reclaims them (unbounded orphan growth). That's tracked as #9835.

Tests

TestCreateBackupVSCDeletionPolicy (table test, Delete and Retain subtests, both pass). Note for reviewers: the pre-existing assertion this replaces/extends at csi_snapshot_test.go was vacuous — its fixture policy was already Delete, so it passed against the old hardcoded value regardless of whether inheritance worked.

Please indicate you have done the following:

• Accepted the DCO. Commits without the DCO will delay acceptance.
• Created a changelog file (make new-changelog) — will run once this PR is open.
• Updated the corresponding documentation in site/content/docs/main — n/a, no new user-facing flag; behavior fix to an existing feature.

[netlify]

✅ Deploy Preview for velero canceled.

Name	Link
🔨 Latest commit	79d5c25
🔍 Latest deploy log	https://app.netlify.com/projects/velero/deploys/6a83d107cd88a00008750781

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/exposer/csi_snapshot.go	60.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[weshayutin]

@sseago @shubham-pampattiwar 0/ please review

[shubham-pampattiwar]

The changeID fallback here is the core fix for #10294, but it isn't covered by a unit test. The existing TestGetCBTInfo case "fallback to pv and vsc snapshot handle" sets Status.SnapshotHandle, which exercises the old path, not this new else if. Codecov's two uncovered lines are this branch plus the new CleanUp deletion.

Could you add a TestGetCBTInfo case with vsc.Status = nil and vsc.Spec.Source.SnapshotHandle set, asserting changeID comes from the spec handle? That locks in the exact regression #10294 was about, so a future refactor of getCBTInfo can't silently reintroduce the empty changeID.

[shubham-pampattiwar]

Small wording nit: this comment says status "is populated asynchronously and may not be set yet," which reads like a timing issue that a re-fetch would resolve. But this vsc is the in-memory object returned from Create() and never re-fetched, so its status is unconditionally nil here, not just temporarily. The description puts it well ("nil unconditionally, not merely until some async fill completes"). Matching the comment to that avoids someone later "fixing" this with a re-fetch that wouldn't help.

[shubham-pampattiwar]

The description's "Minor accompanying fix" for the retained= log field doesn't appear in this diff. RetainVSC still returns non-nil on both the already-Retain and just-patched branches (pkg/util/csi/volume_snapshot.go:138-144), and pkg/exposer/csi_snapshot.go:219 still logs (retained != nil), so the field is still always true. Looks like that fix lives in the combined ceph-changeid branch but didn't make it into this PR.

Could you either pull the one-liner in (have RetainVSC signal whether it patched, or compare the source policy) or drop the paragraph from the description so it matches what ships?