diff --git a/changelogs/unreleased/9628-priyansh17 b/changelogs/unreleased/9628-priyansh17 new file mode 100644 index 000000000..f65b55e16 --- /dev/null +++ b/changelogs/unreleased/9628-priyansh17 @@ -0,0 +1 @@ +Implement original VolumeSnapshotContent deletion for legacy backups \ No newline at end of file diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action.go b/internal/delete/actions/csi/volumesnapshotcontent_action.go index 7a6724df1..a8b43d456 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action.go +++ b/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, diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go index 7dbd6d7ff..ee96018fe 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go +++ b/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 }