diff --git a/changelogs/unreleased/3734-ashish-amarnath b/changelogs/unreleased/3734-ashish-amarnath new file mode 100644 index 000000000..b48e60b71 --- /dev/null +++ b/changelogs/unreleased/3734-ashish-amarnath @@ -0,0 +1,4 @@ +✨ ⚠️ Remove CSI volumesnapshot artifact deletion + +This change requires https://github.com/vmware-tanzu/velero-plugin-for-csi/pull/86 for Velero to continue +deleting of CSI volumesnapshots when the corresponding backups are deleted. \ No newline at end of file diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index 3b85b4bb2..b70cadc65 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -24,7 +24,6 @@ import ( jsonpatch "github.com/evanphx/json-patch" snapshotterClientSet "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned" - snapshotter "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/typed/volumesnapshot/v1beta1" snapshotv1beta1listers "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1beta1" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -40,7 +39,6 @@ import ( velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" pkgbackup "github.com/vmware-tanzu/velero/pkg/backup" "github.com/vmware-tanzu/velero/pkg/discovery" - "github.com/vmware-tanzu/velero/pkg/features" velerov1client "github.com/vmware-tanzu/velero/pkg/generated/clientset/versioned/typed/velero/v1" velerov1informers "github.com/vmware-tanzu/velero/pkg/generated/informers/externalversions/velero/v1" velerov1listers "github.com/vmware-tanzu/velero/pkg/generated/listers/velero/v1" @@ -370,22 +368,6 @@ func (c *backupDeletionController) processRequest(req *velerov1api.DeleteBackupR } } - if features.IsEnabled(velerov1api.CSIFeatureFlag) { - log.Info("Removing CSI volumesnapshots") - if csiErrs := deleteCSIVolumeSnapshots(backup.Name, c.csiSnapshotLister, c.csiSnapshotClient.SnapshotV1beta1(), log); len(csiErrs) > 0 { - for _, err := range csiErrs { - errs = append(errs, err.Error()) - } - } - - log.Info("Removing CSI volumesnapshotcontents") - if csiErrs := deleteCSIVolumeSnapshotContents(backup.Name, c.csiSnapshotContentLister, c.csiSnapshotClient.SnapshotV1beta1(), log); len(csiErrs) > 0 { - for _, err := range csiErrs { - errs = append(errs, err.Error()) - } - } - } - log.Info("Removing restores") if restores, err := c.restoreLister.Restores(backup.Namespace).List(labels.Everything()); err != nil { log.WithError(errors.WithStack(err)).Error("Error listing restore API objects") @@ -514,82 +496,6 @@ func (c *backupDeletionController) deleteResticSnapshots(backup *velerov1api.Bac return errs } -func setVolumeSnapshotContentDeletionPolicy(vscName string, csiClient snapshotter.SnapshotV1beta1Interface, log *logrus.Entry) error { - log.Infof("Setting DeletionPolicy of CSI volumesnapshotcontent %s to Delete", vscName) - pb := []byte(`{"spec":{"deletionPolicy":"Delete"}}`) - _, err := csiClient.VolumeSnapshotContents().Patch(context.TODO(), vscName, types.MergePatchType, pb, metav1.PatchOptions{}) - if err != nil { - return err - } - return nil -} - -func deleteCSIVolumeSnapshots(backupName string, csiSnapshotLister snapshotv1beta1listers.VolumeSnapshotLister, - csiClient snapshotter.SnapshotV1beta1Interface, log *logrus.Entry) []error { - errs := []error{} - - selector := label.NewSelectorForBackup(backupName) - csiVolSnaps, err := csiSnapshotLister.List(selector) - if err != nil { - return []error{err} - } - - log.Infof("Deleting %d CSI volumesnapshots", len(csiVolSnaps)) - for _, csiVS := range csiVolSnaps { - log.Infof("Deleting CSI volumesnapshot %s/%s", csiVS.Namespace, csiVS.Name) - if csiVS.Status != nil && csiVS.Status.BoundVolumeSnapshotContentName != nil { - // we patch the DeletionPolicy of the volumesnapshotcontent to set it to Delete. - // This ensures that the volume snapshot in the storage provider is also deleted. - err := setVolumeSnapshotContentDeletionPolicy(*csiVS.Status.BoundVolumeSnapshotContentName, csiClient, log) - if err != nil && !apierrors.IsNotFound(err) { - log.Errorf("Skipping deletion of volumesnapshot %s/%s", csiVS.Namespace, csiVS.Name) - errs = append(errs, err) - continue - } - } - err := csiClient.VolumeSnapshots(csiVS.Namespace).Delete(context.TODO(), csiVS.Name, metav1.DeleteOptions{}) - if err != nil { - errs = append(errs, err) - } - } - return errs -} - -func deleteCSIVolumeSnapshotContents(backupName string, csiVSCLister snapshotv1beta1listers.VolumeSnapshotContentLister, - csiClient snapshotter.SnapshotV1beta1Interface, log *logrus.Entry) []error { - errs := []error{} - selector := label.NewSelectorForBackup(backupName) - csiVolSnapConts, err := csiVSCLister.List(selector) - if err != nil { - return []error{err} - } - // It is possible that by the time deleteCSIVolumeSnapshotContents is called after deleteCSIVolumeSnapshots - // that deletion of VSCs hasn't been completed, by the snapshot-controller (one of the CSI components). - // For that reason the csiVSCLister returned VSCs that are yet to be deleted. To handle this scenario, - // we swallow `IsNotFound` errors from the setVolumeSnapshotContentDeletionPolicy function and the - // csiClient.VolumeSnapshotContents().Delete(...) - log.Infof("Deleting %d CSI volumesnapshotcontents", len(csiVolSnapConts)) - for _, snapCont := range csiVolSnapConts { - err := setVolumeSnapshotContentDeletionPolicy(snapCont.Name, csiClient, log) - if err != nil && !apierrors.IsNotFound(err) { - log.Errorf("Failed to set DeletionPolicy on volumesnapshotcontent %s. Skipping deletion", snapCont.Name) - errs = append(errs, err) - continue - } - if apierrors.IsNotFound(err) { - log.Infof("volumesnapshotcontent %s not found", snapCont.Name) - continue - } - log.Infof("Deleting volumesnapshotcontent %s", snapCont.Name) - err = csiClient.VolumeSnapshotContents().Delete(context.TODO(), snapCont.Name, metav1.DeleteOptions{}) - if err != nil && !apierrors.IsNotFound(err) { - errs = append(errs, err) - } - } - - return errs -} - const deleteBackupRequestMaxAge = 24 * time.Hour func (c *backupDeletionController) deleteExpiredRequests() { diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index 544c6c7c3..388df8ae3 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -24,9 +24,6 @@ import ( "testing" "time" - snapshotv1beta1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1beta1" - snapshotFake "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake" - snapshotv1beta1informers "github.com/kubernetes-csi/external-snapshotter/client/v4/informers/externalversions" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -1154,342 +1151,3 @@ func TestBackupDeletionControllerDeleteExpiredRequests(t *testing.T) { }) } } - -func TestSetVolumeSnapshotContentDeletionPolicy(t *testing.T) { - testCases := []struct { - name string - inputVSCName string - objs []runtime.Object - expectError bool - }{ - { - name: "should update DeletionPolicy of a VSC from retain to delete", - inputVSCName: "retainVSC", - objs: []runtime.Object{ - &snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: "retainVSC", - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{ - DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain, - }, - }, - }, - expectError: false, - }, - { - name: "should be a no-op updating if DeletionPolicy of a VSC is already Delete", - inputVSCName: "deleteVSC", - objs: []runtime.Object{ - &snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deleteVSC", - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{ - DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentDelete, - }, - }, - }, - expectError: false, - }, - { - name: "should update DeletionPolicy of a VSC with no DeletionPolicy", - inputVSCName: "nothingVSC", - objs: []runtime.Object{ - &snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nothingVSC", - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{}, - }, - }, - expectError: false, - }, - { - name: "should return not found error if supplied VSC does not exist", - inputVSCName: "does-not-exist", - objs: []runtime.Object{}, - expectError: true, - }, - } - - log := velerotest.NewLogger().WithFields( - logrus.Fields{ - "unit-test": "unit-test", - }, - ) - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - fakeClient := snapshotFake.NewSimpleClientset(tc.objs...) - err := setVolumeSnapshotContentDeletionPolicy(tc.inputVSCName, fakeClient.SnapshotV1beta1(), log) - if tc.expectError { - assert.NotNil(t, err) - } else { - assert.Nil(t, err) - actual, err := fakeClient.SnapshotV1beta1().VolumeSnapshotContents().Get(context.TODO(), tc.inputVSCName, metav1.GetOptions{}) - assert.Nil(t, err) - assert.Equal(t, snapshotv1beta1api.VolumeSnapshotContentDelete, actual.Spec.DeletionPolicy) - } - }) - } -} - -func TestDeleteCSIVolumeSnapshots(t *testing.T) { - //Backup1 - ns1VS1VSCName := "ns1vs1vsc" - ns1VS1VSC := snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: ns1VS1VSCName, - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{ - DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain, - }, - } - ns1VS1 := snapshotv1beta1api.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vs1", - Namespace: "ns1", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup1", - }, - }, - Status: &snapshotv1beta1api.VolumeSnapshotStatus{ - BoundVolumeSnapshotContentName: &ns1VS1VSCName, - }, - } - - ns1VS2VSCName := "ns1vs2vsc" - ns1VS2VSC := snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: ns1VS2VSCName, - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{ - DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain, - }, - } - ns1VS2 := snapshotv1beta1api.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vs2", - Namespace: "ns1", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup1", - }, - }, - Status: &snapshotv1beta1api.VolumeSnapshotStatus{ - BoundVolumeSnapshotContentName: &ns1VS2VSCName, - }, - } - - ns2VS1VSCName := "ns2vs1vsc" - ns2VS1VSC := snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: ns2VS1VSCName, - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{ - DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain, - }, - } - ns2VS1 := snapshotv1beta1api.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vs1", - Namespace: "ns2", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup1", - }, - }, - Status: &snapshotv1beta1api.VolumeSnapshotStatus{ - BoundVolumeSnapshotContentName: &ns2VS1VSCName, - }, - } - - ns2VS2VSCName := "ns2vs2vsc" - ns2VS2VSC := snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: ns2VS2VSCName, - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{ - DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain, - }, - } - ns2VS2 := snapshotv1beta1api.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "vs2", - Namespace: "ns2", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup1", - }, - }, - Status: &snapshotv1beta1api.VolumeSnapshotStatus{ - BoundVolumeSnapshotContentName: &ns2VS2VSCName, - }, - } - - // Backup2 - ns1NilStatusVS := snapshotv1beta1api.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "ns1NilStatusVS", - Namespace: "ns2", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup2", - }, - }, - Status: nil, - } - - // Backup3 - ns1NilBoundVSCVS := snapshotv1beta1api.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "ns1NilBoundVSCVS", - Namespace: "ns2", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup3", - }, - }, - Status: &snapshotv1beta1api.VolumeSnapshotStatus{ - BoundVolumeSnapshotContentName: nil, - }, - } - - // Backup4 - notFound := "not-found" - ns1NonExistentVSCVS := snapshotv1beta1api.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "ns1NonExistentVSCVS", - Namespace: "ns2", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup3", - }, - }, - Status: &snapshotv1beta1api.VolumeSnapshotStatus{ - BoundVolumeSnapshotContentName: ¬Found, - }, - } - - testCases := []struct { - name string - backupName string - objs []runtime.Object - }{ - { - name: "should delete volumesnapshots bound to existing volumesnapshotcontent", - backupName: "backup1", - objs: []runtime.Object{&ns1VS1VSC, &ns1VS1, &ns1VS2VSC, &ns1VS2, &ns2VS1VSC, &ns2VS1, &ns2VS2VSC, &ns2VS2}, - }, - { - name: "should delete volumesnapshots with nil status", - backupName: "backup2", - objs: []runtime.Object{&ns1NilStatusVS}, - }, - { - name: "should delete volumesnapshots with nil BoundVolumeSnapshotContentName", - backupName: "backup3", - objs: []runtime.Object{&ns1NilBoundVSCVS}, - }, - { - name: "should delete volumesnapshots bound to non-existent volumesnapshotcontents", - backupName: "backup4", - objs: []runtime.Object{&ns1NonExistentVSCVS}, - }, - { - name: "should be a no-op when there are no volumesnapshots to delete", - backupName: "backup-no-vs", - objs: []runtime.Object{}, - }, - } - - log := velerotest.NewLogger().WithFields( - logrus.Fields{ - "unit-test": "unit-test", - }, - ) - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - fakeClient := snapshotFake.NewSimpleClientset(tc.objs...) - fakeSharedInformer := snapshotv1beta1informers.NewSharedInformerFactoryWithOptions(fakeClient, 0) - for _, o := range tc.objs { - fakeSharedInformer.Snapshot().V1beta1().VolumeSnapshots().Informer().GetStore().Add(o) - } - errs := deleteCSIVolumeSnapshots(tc.backupName, fakeSharedInformer.Snapshot().V1beta1().VolumeSnapshots().Lister(), fakeClient.SnapshotV1beta1(), log) - assert.Empty(t, errs) - }) - } -} - -func TestDeleteCSIVolumeSnapshotContents(t *testing.T) { - retainVSC := snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: "retainVSC", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup1", - }, - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{ - DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentRetain, - }, - } - deleteVSC := snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: "deleteVSC", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup2", - }, - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{ - DeletionPolicy: snapshotv1beta1api.VolumeSnapshotContentDelete, - }, - } - - nothingVSC := snapshotv1beta1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nothingVSC", - Labels: map[string]string{ - velerov1api.BackupNameLabel: "backup3", - }, - }, - Spec: snapshotv1beta1api.VolumeSnapshotContentSpec{}, - } - - testCases := []struct { - name string - backupName string - objs []runtime.Object - }{ - { - name: "should delete volumesnapshotcontent with DeletionPolicy Retain", - backupName: "backup1", - objs: []runtime.Object{&retainVSC}, - }, - { - name: "should delete volumesnapshotcontent with DeletionPolicy Delete", - backupName: "backup3", - objs: []runtime.Object{&deleteVSC}, - }, - { - name: "should delete volumesnapshotcontent with No DeletionPolicy", - backupName: "backup3", - objs: []runtime.Object{¬hingVSC}, - }, - { - name: "should return no error when backup has no volumesnapshotconents", - backupName: "backup-with-no-vsc", - objs: []runtime.Object{}, - }, - } - log := velerotest.NewLogger().WithFields( - logrus.Fields{ - "unit-test": "unit-test", - }, - ) - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - fakeClient := snapshotFake.NewSimpleClientset(tc.objs...) - fakeSharedInformer := snapshotv1beta1informers.NewSharedInformerFactoryWithOptions(fakeClient, 0) - for _, o := range tc.objs { - fakeSharedInformer.Snapshot().V1beta1().VolumeSnapshotContents().Informer().GetStore().Add(o) - } - - errs := deleteCSIVolumeSnapshotContents(tc.backupName, fakeSharedInformer.Snapshot().V1beta1().VolumeSnapshotContents().Lister(), fakeClient.SnapshotV1beta1(), log) - assert.Empty(t, errs) - }) - } -}