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 diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index a5639537c..247ae9003 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,21 @@ 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. + // + // 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, }, 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 != "" {