Merge pull request #9628 from priyansh17/9625-fixOrphanesVscCleanup

Implement original VolumeSnapshotContent deletion for legacy backups - Fixes #9625

changelogs/unreleased/9628-priyansh17

@@ -0,0 +1 @@
+Implement original VolumeSnapshotContent deletion for legacy backups

internal/delete/actions/csi/volumesnapshotcontent_action.go

@@ -71,7 +71,7 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute(
 	// So skip deleting VolumeSnapshotContent not have the backup name
 	// in its labels.
 	if !kubeutil.HasBackupLabel(&snapCont.ObjectMeta, input.Backup.Name) {
-		p.log.Info(
+		p.log.Infof(
 			"VolumeSnapshotContent %s was not taken by backup %s, skipping deletion",
 			snapCont.Name,
 			input.Backup.Name,
@@ -81,6 +81,17 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute(
 
 	p.log.Infof("Deleting VolumeSnapshotContent %s", snapCont.Name)
 
+	// Try to delete the original VSC from the cluster first.
+	// This handles legacy (pre-1.15) backups where the original VSC
+	// with DeletionPolicy=Retain still exists in the cluster.
+	originalVSCName := snapCont.Name
+	if cleaned := p.tryDeleteOriginalVSC(context.TODO(), originalVSCName); cleaned {
+		p.log.Infof("Successfully deleted original VolumeSnapshotContent %s from cluster, skipping temp VSC creation", originalVSCName)
+		return nil
+	}
+
+	// create a temp VSC to trigger cloud snapshot deletion
+	// (for backups where the original VSC no longer exists in cluster)
 	uuid, err := uuid.NewRandom()
 	if err != nil {
 		p.log.WithError(err).Errorf("Fail to generate the UUID to create VSC %s", snapCont.Name)
@@ -114,6 +125,7 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute(
 	if err := p.crClient.Create(context.TODO(), &snapCont); err != nil {
 		return errors.Wrapf(err, "fail to create VolumeSnapshotContent %s", snapCont.Name)
 	}
+	p.log.Infof("Created temp VolumeSnapshotContent %s with DeletionPolicy=Delete to trigger cloud snapshot cleanup", snapCont.Name)
 
 	// Read resource timeout from backup annotation, if not set, use default value.
 	timeout, err := time.ParseDuration(
@@ -138,23 +150,68 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute(
 		},
 	); err != nil {
 		// Clean up the VSC we created since it can't become ready
+		p.log.WithError(err).Warnf("Temp VolumeSnapshotContent %s did not become ready, cleaning up", snapCont.Name)
 		if deleteErr := p.crClient.Delete(context.TODO(), &snapCont); deleteErr != nil && !apierrors.IsNotFound(deleteErr) {
-			p.log.WithError(deleteErr).Errorf("Failed to clean up VolumeSnapshotContent %s", snapCont.Name)
+			p.log.WithError(deleteErr).Errorf("Failed to clean up temp VolumeSnapshotContent %s", snapCont.Name)
 		}
 		return errors.Wrapf(err, "fail to wait VolumeSnapshotContent %s becomes ready.", snapCont.Name)
 	}
 
+	p.log.Infof("Temp VolumeSnapshotContent %s is ready, deleting to trigger cloud snapshot removal", snapCont.Name)
 	if err := p.crClient.Delete(
 		context.TODO(),
 		&snapCont,
 	); err != nil && !apierrors.IsNotFound(err) {
-		p.log.Infof("VolumeSnapshotContent %s not found", snapCont.Name)
+		p.log.WithError(err).Errorf("Failed to delete temp VolumeSnapshotContent %s", snapCont.Name)
 		return err
 	}
 
+	p.log.Infof("Successfully triggered deletion of VolumeSnapshotContent %s and its cloud snapshot", snapCont.Name)
 	return nil
 }
 
+// tryDeleteOriginalVSC attempts to find and delete the original VSC from
+// the cluster (legacy pre-1.15 backups). It patches the DeletionPolicy to
+// Delete so the CSI driver also removes the cloud snapshot, then deletes
+// the VSC object itself.
+// Returns true if the original VSC was found and deletion was initiated.
+func (p *volumeSnapshotContentDeleteItemAction) tryDeleteOriginalVSC(
+	ctx context.Context,
+	vscName string,
+) bool {
+	existing := new(snapshotv1api.VolumeSnapshotContent)
+	if err := p.crClient.Get(ctx, crclient.ObjectKey{Name: vscName}, existing); err != nil {
+		if apierrors.IsNotFound(err) {
+			p.log.Debugf("Original VolumeSnapshotContent %s not found in cluster, will use temp VSC flow", vscName)
+		} else {
+			p.log.WithError(err).Warnf("Error looking up original VolumeSnapshotContent %s, will use temp VSC flow", vscName)
+		}
+		return false
+	}
+
+	p.log.Debugf("Found original VolumeSnapshotContent %s in cluster (legacy backup), cleaning up directly", vscName)
+
+	// Patch DeletionPolicy to Delete so the CSI driver removes the cloud snapshot
+	if existing.Spec.DeletionPolicy != snapshotv1api.VolumeSnapshotContentDelete {
+		original := existing.DeepCopy()
+		existing.Spec.DeletionPolicy = snapshotv1api.VolumeSnapshotContentDelete
+		if err := p.crClient.Patch(ctx, existing, crclient.MergeFrom(original)); err != nil {
+			p.log.WithError(err).Warnf("Failed to patch DeletionPolicy on original VSC %s, will use temp VSC flow", vscName)
+			return false
+		}
+		p.log.Debugf("Patched DeletionPolicy to Delete on original VolumeSnapshotContent %s", vscName)
+	}
+
+	// Delete the original VSC — the CSI driver will clean up the cloud snapshot
+	if err := p.crClient.Delete(ctx, existing); err != nil && !apierrors.IsNotFound(err) {
+		p.log.WithError(err).Warnf("Failed to delete original VolumeSnapshotContent %s, will use temp VSC flow", vscName)
+		return false
+	}
+
+	p.log.Infof("Deleted original VolumeSnapshotContent %s with DeletionPolicy=Delete, CSI driver will remove cloud snapshot", vscName)
+	return true
+}
+
 var checkVSCReadiness = func(
 	ctx context.Context,
 	vsc *snapshotv1api.VolumeSnapshotContent,

internal/delete/actions/csi/volumesnapshotcontent_action_test.go

@@ -25,6 +25,8 @@ import (
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
+	corev1api "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -37,14 +39,44 @@ import (
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
 )
 
+// fakeClientWithErrors wraps a real client and injects errors for specific operations.
+type fakeClientWithErrors struct {
+	crclient.Client
+	getError    error
+	patchError  error
+	deleteError error
+}
+
+func (c *fakeClientWithErrors) Get(ctx context.Context, key crclient.ObjectKey, obj crclient.Object, opts ...crclient.GetOption) error {
+	if c.getError != nil {
+		return c.getError
+	}
+	return c.Client.Get(ctx, key, obj, opts...)
+}
+
+func (c *fakeClientWithErrors) Patch(ctx context.Context, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error {
+	if c.patchError != nil {
+		return c.patchError
+	}
+	return c.Client.Patch(ctx, obj, patch, opts...)
+}
+
+func (c *fakeClientWithErrors) Delete(ctx context.Context, obj crclient.Object, opts ...crclient.DeleteOption) error {
+	if c.deleteError != nil {
+		return c.deleteError
+	}
+	return c.Client.Delete(ctx, obj, opts...)
+}
+
 func TestVSCExecute(t *testing.T) {
 	snapshotHandleStr := "test"
 	tests := []struct {
-		name     string
-		item     runtime.Unstructured
-		vsc      *snapshotv1api.VolumeSnapshotContent
-		backup   *velerov1api.Backup
-		function func(
+		name           string
+		item           runtime.Unstructured
+		vsc            *snapshotv1api.VolumeSnapshotContent
+		backup         *velerov1api.Backup
+		preExistingVSC *snapshotv1api.VolumeSnapshotContent
+		function       func(
 			ctx context.Context,
 			vsc *snapshotv1api.VolumeSnapshotContent,
 			client crclient.Client,
@@ -94,6 +126,21 @@ func TestVSCExecute(t *testing.T) {
 				return false, errors.Errorf("test error case")
 			},
 		},
+		{
+			name:      "Original VSC exists in cluster, cleaned up directly",
+			vsc:       builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(),
+			backup:    builder.ForBackup("velero", "backup").Result(),
+			expectErr: false,
+			preExistingVSC: &snapshotv1api.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{Name: "bar"},
+				Spec: snapshotv1api.VolumeSnapshotContentSpec{
+					DeletionPolicy:    snapshotv1api.VolumeSnapshotContentRetain,
+					Driver:            "disk.csi.azure.com",
+					Source:            snapshotv1api.VolumeSnapshotContentSource{SnapshotHandle: stringPtr("snap-123")},
+					VolumeSnapshotRef: corev1api.ObjectReference{Name: "vs-1", Namespace: "default"},
+				},
+			},
+		},
 		{
 			name:      "Error case with CSI error, dangling VSC should be cleaned up",
 			vsc:       builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(),
@@ -115,6 +162,10 @@ func TestVSCExecute(t *testing.T) {
 			logger := logrus.StandardLogger()
 			checkVSCReadiness = test.function
 
+			if test.preExistingVSC != nil {
+				require.NoError(t, crClient.Create(t.Context(), test.preExistingVSC))
+			}
+
 			p := volumeSnapshotContentDeleteItemAction{log: logger, crClient: crClient}
 
 			if test.vsc != nil {
@@ -239,6 +290,149 @@ func TestCheckVSCReadiness(t *testing.T) {
 	}
 }
 
+func TestTryDeleteOriginalVSC(t *testing.T) {
+	tests := []struct {
+		name      string
+		vscName   string
+		existing  *snapshotv1api.VolumeSnapshotContent
+		createIt  bool
+		expectRet bool
+	}{
+		{
+			name:      "VSC not found in cluster, returns false",
+			vscName:   "not-found",
+			expectRet: false,
+		},
+		{
+			name:    "VSC found with Retain policy, patches and deletes",
+			vscName: "legacy-vsc",
+			existing: &snapshotv1api.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{Name: "legacy-vsc"},
+				Spec: snapshotv1api.VolumeSnapshotContentSpec{
+					DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain,
+					Driver:         "disk.csi.azure.com",
+					Source: snapshotv1api.VolumeSnapshotContentSource{
+						SnapshotHandle: stringPtr("snap-123"),
+					},
+					VolumeSnapshotRef: corev1api.ObjectReference{
+						Name:      "vs-1",
+						Namespace: "default",
+					},
+				},
+			},
+			createIt:  true,
+			expectRet: true,
+		},
+		{
+			name:    "VSC found with Delete policy already, just deletes",
+			vscName: "already-delete-vsc",
+			existing: &snapshotv1api.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{Name: "already-delete-vsc"},
+				Spec: snapshotv1api.VolumeSnapshotContentSpec{
+					DeletionPolicy: snapshotv1api.VolumeSnapshotContentDelete,
+					Driver:         "disk.csi.azure.com",
+					Source: snapshotv1api.VolumeSnapshotContentSource{
+						SnapshotHandle: stringPtr("snap-456"),
+					},
+					VolumeSnapshotRef: corev1api.ObjectReference{
+						Name:      "vs-2",
+						Namespace: "default",
+					},
+				},
+			},
+			createIt:  true,
+			expectRet: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			crClient := velerotest.NewFakeControllerRuntimeClient(t)
+			logger := logrus.StandardLogger()
+			p := &volumeSnapshotContentDeleteItemAction{
+				log:      logger,
+				crClient: crClient,
+			}
+
+			if test.createIt && test.existing != nil {
+				require.NoError(t, crClient.Create(t.Context(), test.existing))
+			}
+
+			result := p.tryDeleteOriginalVSC(t.Context(), test.vscName)
+			require.Equal(t, test.expectRet, result)
+
+			// If cleanup succeeded, verify the VSC is gone
+			if test.expectRet {
+				err := crClient.Get(t.Context(), crclient.ObjectKey{Name: test.vscName},
+					&snapshotv1api.VolumeSnapshotContent{})
+				require.True(t, apierrors.IsNotFound(err),
+					"VSC should have been deleted from cluster")
+			}
+		})
+	}
+
+	// Error injection tests for tryDeleteOriginalVSC
+	t.Run("Get returns non-NotFound error, returns false", func(t *testing.T) {
+		errClient := &fakeClientWithErrors{
+			Client:   velerotest.NewFakeControllerRuntimeClient(t),
+			getError: fmt.Errorf("connection refused"),
+		}
+		p := &volumeSnapshotContentDeleteItemAction{
+			log:      logrus.StandardLogger(),
+			crClient: errClient,
+		}
+		require.False(t, p.tryDeleteOriginalVSC(t.Context(), "some-vsc"))
+	})
+
+	t.Run("Patch fails, returns false", func(t *testing.T) {
+		realClient := velerotest.NewFakeControllerRuntimeClient(t)
+		vsc := &snapshotv1api.VolumeSnapshotContent{
+			ObjectMeta: metav1.ObjectMeta{Name: "patch-fail-vsc"},
+			Spec: snapshotv1api.VolumeSnapshotContentSpec{
+				DeletionPolicy:    snapshotv1api.VolumeSnapshotContentRetain,
+				Driver:            "disk.csi.azure.com",
+				Source:            snapshotv1api.VolumeSnapshotContentSource{SnapshotHandle: stringPtr("snap-789")},
+				VolumeSnapshotRef: corev1api.ObjectReference{Name: "vs-3", Namespace: "default"},
+			},
+		}
+		require.NoError(t, realClient.Create(t.Context(), vsc))
+
+		errClient := &fakeClientWithErrors{
+			Client:     realClient,
+			patchError: fmt.Errorf("patch forbidden"),
+		}
+		p := &volumeSnapshotContentDeleteItemAction{
+			log:      logrus.StandardLogger(),
+			crClient: errClient,
+		}
+		require.False(t, p.tryDeleteOriginalVSC(t.Context(), "patch-fail-vsc"))
+	})
+
+	t.Run("Delete fails, returns false", func(t *testing.T) {
+		realClient := velerotest.NewFakeControllerRuntimeClient(t)
+		vsc := &snapshotv1api.VolumeSnapshotContent{
+			ObjectMeta: metav1.ObjectMeta{Name: "delete-fail-vsc"},
+			Spec: snapshotv1api.VolumeSnapshotContentSpec{
+				DeletionPolicy:    snapshotv1api.VolumeSnapshotContentDelete,
+				Driver:            "disk.csi.azure.com",
+				Source:            snapshotv1api.VolumeSnapshotContentSource{SnapshotHandle: stringPtr("snap-999")},
+				VolumeSnapshotRef: corev1api.ObjectReference{Name: "vs-4", Namespace: "default"},
+			},
+		}
+		require.NoError(t, realClient.Create(t.Context(), vsc))
+
+		errClient := &fakeClientWithErrors{
+			Client:      realClient,
+			deleteError: fmt.Errorf("delete forbidden"),
+		}
+		p := &volumeSnapshotContentDeleteItemAction{
+			log:      logrus.StandardLogger(),
+			crClient: errClient,
+		}
+		require.False(t, p.tryDeleteOriginalVSC(t.Context(), "delete-fail-vsc"))
+	})
+}
+
 func boolPtr(b bool) *bool {
 	return &b
 }