From eff2522eac42f07c31ed909538b5748b48cd8946 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Sun, 16 Aug 2026 10:48:18 -0400 Subject: [PATCH 1/3] Fix generic CSI changeID retrieval and honor snapshot class deletion policy Two defects in the CSI snapshot exposer, both of which make every CBT incremental silently degrade to a full backup on non-vSphere drivers. 1. getCBTInfo read the changeID from the freshly-created backup VSC's status, which is populated asynchronously and is therefore usually empty at that point. The handle is already present in the spec, so fall back to vsc.Spec.Source.SnapshotHandle. vSphere is unaffected -- it takes the VSphereCNSChangeIDAnno branch and never reads the VSC handle -- so this affects every other CSI driver. 2. createBackupVSC hardcoded DeletionPolicy: Delete, so the physical snapshot was removed when the backup completed. Case-2 storage such as Ceph RBD requires the base snapshot to survive for the next GetMetadataDelta call, and the block-data-mover design specifies a RetainSnapshot volume-policy parameter that was never implemented. Inherit the source snapshot class's deletion policy instead, and clean up the backup VSC object in CleanUp. Adds TestCreateBackupVSCDeletionPolicy covering both Delete and Retain. Co-Authored-By: Claude Fable 5 Signed-off-by: Tiger Kaovilai (cherry picked from commit f4867d0489788ce0c7a63ffe08da4ad059db88b4) Signed-off-by: Tiger Kaovilai --- pkg/exposer/csi_snapshot.go | 16 ++++++++- pkg/exposer/csi_snapshot_test.go | 59 ++++++++++++++++++++++++++++++++ pkg/util/csi/cbt.go | 5 +++ 3 files changed, 79 insertions(+), 1 deletion(-) diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index a5639537c..535268a3d 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -496,6 +496,7 @@ func (e *csiSnapshotExposer) CleanUp(ctx context.Context, ownerObject corev1api. backupPodName := ownerObject.Name backupPVCName := ownerObject.Name backupVSName := ownerObject.Name + backupVSCName := ownerObject.Name kube.DeletePodIfAny(ctx, e.kubeClient.CoreV1(), backupPodName, ownerObject.Namespace, e.log) kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVCName, ownerObject.Namespace, cleanUpTimeout, e.log) @@ -507,6 +508,13 @@ func (e *csiSnapshotExposer) CleanUp(ctx context.Context, ownerObject corev1api. csi.DeleteVolumeSnapshotIfAny(ctx, e.csiSnapshotClient, backupVSName, ownerObject.Namespace, e.log) csi.DeleteVolumeSnapshotIfAny(ctx, e.csiSnapshotClient, vsName, sourceNamespace, e.log) + + // The backup VSC is created by Velero as an internal handle to the source + // snapshot. Deleting the backup VS above only cascades to it when its + // deletion policy is Delete, so remove it explicitly to avoid leaking the + // object under a Retain policy. Deleting a Retain VSC drops only the API + // object and leaves the underlying snapshot intact. + csi.DeleteVolumeSnapshotContentIfAny(ctx, e.csiSnapshotClient, backupVSCName, e.log) } func getVolumeModeByAccessMode(accessMode string, dataMover string) (corev1api.PersistentVolumeMode, error) { @@ -571,7 +579,13 @@ func (e *csiSnapshotExposer) createBackupVSC(ctx context.Context, ownerObject co Source: snapshotv1api.VolumeSnapshotContentSource{ SnapshotHandle: snapshotVSC.Status.SnapshotHandle, }, - DeletionPolicy: snapshotv1api.VolumeSnapshotContentDelete, + // The backup VSC is statically provisioned against the same + // snapshot handle as the source VSC, so both objects refer to one + // physical snapshot. Inherit the source's deletion policy instead + // of forcing Delete, otherwise a user who configured Retain on the + // VolumeSnapshotClass still loses the snapshot when the backup VSC + // is cleaned up. + DeletionPolicy: snapshotVSC.Spec.DeletionPolicy, Driver: snapshotVSC.Spec.Driver, VolumeSnapshotClassName: snapshotVSC.Spec.VolumeSnapshotClassName, }, diff --git a/pkg/exposer/csi_snapshot_test.go b/pkg/exposer/csi_snapshot_test.go index 688c439a9..8502b47b8 100644 --- a/pkg/exposer/csi_snapshot_test.go +++ b/pkg/exposer/csi_snapshot_test.go @@ -2537,3 +2537,62 @@ func TestCleanUp_SecretsAndConfigMaps(t *testing.T) { _, err = fakeKubeClient.CoreV1().Secrets("velero").Get(t.Context(), "other-secret", metav1.GetOptions{}) assert.NoError(t, err, "unrelated secret should not be deleted") } + +func TestCreateBackupVSCDeletionPolicy(t *testing.T) { + tests := []struct { + name string + sourcePolicy snapshotv1api.DeletionPolicy + expectedPolicy snapshotv1api.DeletionPolicy + }{ + { + name: "Delete policy is inherited", + sourcePolicy: snapshotv1api.VolumeSnapshotContentDelete, + expectedPolicy: snapshotv1api.VolumeSnapshotContentDelete, + }, + { + // The backup VSC points at the same snapshot handle as the source + // VSC, so forcing Delete here would destroy a snapshot the user + // asked to keep. + name: "Retain policy is inherited", + sourcePolicy: snapshotv1api.VolumeSnapshotContentRetain, + expectedPolicy: snapshotv1api.VolumeSnapshotContentRetain, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + handle := "fake-snapshot-handle" + className := "fake-snapshot-class" + + sourceVSC := &snapshotv1api.VolumeSnapshotContent{ + ObjectMeta: metav1.ObjectMeta{Name: "source-vsc"}, + Spec: snapshotv1api.VolumeSnapshotContentSpec{ + DeletionPolicy: test.sourcePolicy, + Driver: "fake-driver", + VolumeSnapshotClassName: &className, + }, + Status: &snapshotv1api.VolumeSnapshotContentStatus{ + SnapshotHandle: &handle, + }, + } + + exposer := csiSnapshotExposer{ + csiSnapshotClient: snapshotFake.NewSimpleClientset().SnapshotV1(), + log: velerotest.NewLogger(), + } + + ownerObject := corev1api.ObjectReference{ + Name: "fake-du", + Namespace: "velero", + } + vs := &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{Name: "fake-du", Namespace: "velero"}, + } + + backupVSC, err := exposer.createBackupVSC(t.Context(), ownerObject, sourceVSC, vs) + require.NoError(t, err) + assert.Equal(t, test.expectedPolicy, backupVSC.Spec.DeletionPolicy) + assert.Equal(t, handle, *backupVSC.Spec.Source.SnapshotHandle) + }) + } +} diff --git a/pkg/util/csi/cbt.go b/pkg/util/csi/cbt.go index 00342996d..ed872023f 100644 --- a/pkg/util/csi/cbt.go +++ b/pkg/util/csi/cbt.go @@ -64,6 +64,11 @@ func GetCBTInfo(ctx context.Context, kubeClient kubernetes.Interface, log logrus if vsc.Status != nil && vsc.Status.SnapshotHandle != nil { cbtInfo.ChangeID = *vsc.Status.SnapshotHandle + } else if vsc.Spec.Source.SnapshotHandle != nil { + // The backup VSC is statically provisioned from the source VSC's + // snapshot handle; its status is populated asynchronously and may + // not be set yet, but the handle is already in the spec. + cbtInfo.ChangeID = *vsc.Spec.Source.SnapshotHandle } if pv.Spec.CSI != nil && pv.Spec.CSI.VolumeHandle != "" { From e2769706463856bd53fc9e00352fbbb25d05131e Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 17 Aug 2026 23:04:06 -0400 Subject: [PATCH 2/3] Note why Case 2 storages require inherited Retain, not just permit it For a Case 2 driver (design/block-data-mover/block-data-mover.md), such as Ceph RBD, rbd snap diff needs the base and target snapshots in the same clone chain. Delete destroys the base as soon as the backup completes, so the next incremental's delta query fails and degrades to an allocated-blocks backup (or a full whole-device transfer without that fix). Inheriting Retain there isn't an optional nicety, it's what makes incrementals possible at all. Signed-off-by: Tiger Kaovilai --- pkg/exposer/csi_snapshot.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 535268a3d..247ae9003 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -585,6 +585,14 @@ func (e *csiSnapshotExposer) createBackupVSC(ctx context.Context, ownerObject co // of forcing Delete, otherwise a user who configured Retain on the // VolumeSnapshotClass still loses the snapshot when the backup VSC // is cleaned up. + // + // For Case 2 storages per the design (design/block-data-mover/block-data-mover.md, + // e.g. Ceph RBD), inheriting Retain is not just an option but a requirement for + // incrementals to work at all: rbd snap diff needs the base and target snapshots + // in the same clone chain, so Delete destroys the base as soon as this backup + // completes. The next incremental's delta query then fails and degrades to an + // allocated-blocks backup (see the CBT tier ladder) or, without that fix, a full + // whole-device transfer. DeletionPolicy: snapshotVSC.Spec.DeletionPolicy, Driver: snapshotVSC.Spec.Driver, VolumeSnapshotClassName: snapshotVSC.Spec.VolumeSnapshotClassName, From 22a2feb4e83851e9b39d5db50fa3728a3445c910 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 17 Aug 2026 23:26:59 -0400 Subject: [PATCH 3/3] Add changelog for #10307 Signed-off-by: Tiger Kaovilai --- changelogs/unreleased/10307-kaovilai | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10307-kaovilai diff --git a/changelogs/unreleased/10307-kaovilai b/changelogs/unreleased/10307-kaovilai new file mode 100644 index 000000000..cdb6f54a1 --- /dev/null +++ b/changelogs/unreleased/10307-kaovilai @@ -0,0 +1 @@ +Fix generic CSI changeID retrieval and honor snapshot class deletion policy for CBT retention