add restore finalizer to clean up external resources

Signed-off-by: allenxu404 <qix2@vmware.com>

changelogs/unreleased/6479-allenxu404

@@ -0,0 +1 @@
+Add restore finalizer to clean up external resources

pkg/cmd/cli/uninstall/uninstall.go

@@ -34,11 +34,15 @@ import (
 	kubeerrs "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/wait"
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/client"
 	"github.com/vmware-tanzu/velero/pkg/cmd"
 	"github.com/vmware-tanzu/velero/pkg/cmd/cli"
+	"github.com/vmware-tanzu/velero/pkg/controller"
 	"github.com/vmware-tanzu/velero/pkg/install"
+	kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
 )
 
 // uninstallOptions collects all the options for uninstalling Velero from a Kubernetes cluster.
@@ -159,6 +163,10 @@ func Run(ctx context.Context, kbClient kbclient.Client, namespace string) error
 }
 
 func deleteNamespace(ctx context.Context, kbClient kbclient.Client, namespace string) error {
+	if err := removeFinalizer(ctx, kbClient, namespace); err != nil {
+		return errors.Wrap(err, "Fail to remove finalizer from restores")
+	}
+
 	ns := &corev1.Namespace{}
 	key := kbclient.ObjectKey{Name: namespace}
 	if err := kbClient.Get(ctx, key, ns); err != nil {
@@ -203,3 +211,20 @@ func deleteNamespace(ctx context.Context, kbClient kbclient.Client, namespace st
 	fmt.Printf("Velero namespace %q deleted\n", namespace)
 	return nil
 }
+
+func removeFinalizer(ctx context.Context, kbClient kbclient.Client, namespace string) error {
+	// remove restore finalizer to ensure the process of deleting the namespace won't get stuck and the restore files in object storage won't be deleted as before.
+	restoreList := &velerov1api.RestoreList{}
+	if err := kbClient.List(ctx, restoreList, &kbclient.ListOptions{Namespace: namespace}); err != nil {
+		return err
+	}
+
+	for i := range restoreList.Items {
+		original := restoreList.Items[i].DeepCopy()
+		controllerutil.RemoveFinalizer(&restoreList.Items[i], controller.ExternalResourcesFinalizer)
+		if err := kubeutil.PatchResource(original, &restoreList.Items[i], kbClient); err != nil {
+			return err
+		}
+	}
+	return nil
+}

pkg/controller/restore_controller.go

@@ -37,6 +37,11 @@ import (
 	"k8s.io/utils/clock"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+
+	"sigs.k8s.io/controller-runtime/pkg/builder"
 
 	"github.com/vmware-tanzu/velero/internal/hook"
 	api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
@@ -84,6 +89,8 @@ var nonRestorableResources = []string{
 	"backuprepositories.velero.io",
 }
 
+var ExternalResourcesFinalizer = "restores.velero.io/external-resources-finalizer"
+
 type restoreReconciler struct {
 	ctx                         context.Context
 	namespace                   string
@@ -159,6 +166,28 @@ func (r *restoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		return ctrl.Result{}, err
 	}
 
+	// deal with the finalizer
+	if !restore.DeletionTimestamp.IsZero() && len(restore.Finalizers) != 0 {
+		// check the finalizer and run clean-up
+		if controllerutil.ContainsFinalizer(restore, ExternalResourcesFinalizer) {
+			if err := r.deleteExternalResources(restore); err != nil {
+				log.Errorf("fail to delete external resources: %s", err.Error())
+				return ctrl.Result{}, err
+			}
+			// once finish clean-up, remove the finalizer from the restore so that the restore will be unlocked and deleted.
+			original := restore.DeepCopy()
+			controllerutil.RemoveFinalizer(restore, ExternalResourcesFinalizer)
+			if err := kubeutil.PatchResource(original, restore, r.kbClient); err != nil {
+				log.Errorf("fail to remove the finalizer: %s", err.Error())
+				return ctrl.Result{}, err
+			}
+			return ctrl.Result{}, nil
+		} else {
+			log.Error("DeletionTimestamp is marked but can't find the expected finalizer")
+			return ctrl.Result{}, fmt.Errorf("DeletionTimestamp is marked but can't find the expected finalizer")
+		}
+	}
+
 	// store a copy of the original restore for creating patch
 	original := restore.DeepCopy()
 
@@ -210,7 +239,13 @@ func (r *restoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		restore.Status.Phase == api.RestorePhaseCompleted {
 		restore.Status.CompletionTimestamp = &metav1.Time{Time: r.clock.Now()}
 	}
-	log.Debug("Updating restore's final status")
+	log.Debug("Updating restore's final status and finalizer")
+
+	// add the finalizer for new restore in terminal phases.
+	if restore.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(restore, ExternalResourcesFinalizer) {
+		controllerutil.AddFinalizer(restore, ExternalResourcesFinalizer)
+	}
+
 	if err = kubeutil.PatchResource(original, restore, r.kbClient); err != nil {
 		log.WithError(errors.WithStack(err)).Info("Error updating restore's final status")
 		// No need to re-enqueue here, because restore's already set to InProgress before.
@@ -222,22 +257,38 @@ func (r *restoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 
 func (r *restoreReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
-		WithEventFilter(kubeutil.NewCreateEventPredicate(func(obj client.Object) bool {
-			restore := obj.(*api.Restore)
-
-			switch restore.Status.Phase {
-			case "", api.RestorePhaseNew:
-				// only process new restores
-				return true
-			default:
-				r.logger.WithFields(logrus.Fields{
-					"restore": kubeutil.NamespaceAndName(restore),
-					"phase":   restore.Status.Phase,
-				}).Debug("Restore is not new, skipping")
+		For(&api.Restore{}, builder.WithPredicates(predicate.Funcs{
+			CreateFunc: func(ce event.CreateEvent) bool {
+				restore := ce.Object.(*api.Restore)
+				switch restore.Status.Phase {
+				case "", api.RestorePhaseNew:
+					// only process new restores
+					return true
+				default:
+					r.logger.WithFields(logrus.Fields{
+						"restore": kubeutil.NamespaceAndName(restore),
+						"phase":   restore.Status.Phase,
+					}).Debug("Restore is not new, skipping")
+					return false
+				}
+			},
+			UpdateFunc: func(ue event.UpdateEvent) bool {
+				// only process the deletion for restores attached with a finalizer
+				// note that a finalizer makes a deletion on the object become an update by setting deletion timestamp
+				restore := ue.ObjectNew.(*api.Restore)
+				if !restore.DeletionTimestamp.IsZero() && len(restore.Finalizers) != 0 && (restore.Status.Phase == api.RestorePhaseFailed ||
+					restore.Status.Phase == api.RestorePhasePartiallyFailed || restore.Status.Phase == api.RestorePhaseCompleted) {
+					return true
+				}
 				return false
-			}
+			},
+			DeleteFunc: func(de event.DeleteEvent) bool {
+				return false
+			},
+			GenericFunc: func(ge event.GenericEvent) bool {
+				return false
+			},
 		})).
-		For(&api.Restore{}).
 		Complete(r)
 }
 
@@ -600,6 +651,36 @@ func (r *restoreReconciler) updateTotalRestoreMetric() {
 	}()
 }
 
+// deleteExternalResources deletes all the external resources related to the restore
+func (r *restoreReconciler) deleteExternalResources(restore *api.Restore) error {
+	r.logger.Infof("Finalizer is deleting external resources, backup: %s", restore.Spec.BackupName)
+
+	backupInfo, err := r.fetchBackupInfo(restore.Spec.BackupName)
+	if err != nil {
+		return errors.Wrap(err, fmt.Sprintf("can't get backup info, backup: %s", restore.Spec.BackupName))
+	}
+
+	// if storage locations is read-only, skip deletion
+	if backupInfo.location.Spec.AccessMode == api.BackupStorageLocationAccessModeReadOnly {
+		return nil
+	}
+
+	// delete restore files in object storage
+	pluginManager := r.newPluginManager(r.logger)
+	defer pluginManager.CleanupClients()
+
+	backupStore, err := r.backupStoreGetter.Get(backupInfo.location, pluginManager, r.logger)
+	if err != nil {
+		return errors.Wrap(err, fmt.Sprintf("can't get backupStore, backup: %s", restore.Spec.BackupName))
+	}
+
+	if err = backupStore.DeleteRestore(restore.Name); err != nil {
+		return errors.Wrap(err, fmt.Sprintf("can't delete restore files in object storage, backup: %s", restore.Spec.BackupName))
+	}
+
+	return nil
+}
+
 func putResults(restore *api.Restore, results map[string]results.Result, backupStore persistence.BackupStore) error {
 	buf := new(bytes.Buffer)
 	gzw := gzip.NewWriter(buf)

pkg/controller/restore_controller_test.go

@@ -234,6 +234,7 @@ func TestRestoreReconcile(t *testing.T) {
 		backupStoreGetBackupContentsErr error
 		putRestoreLogErr                error
 		expectedFinalPhase              string
+		addValidFinalizer               bool
 	}{
 		{
 			name:                     "restore with both namespace in both includedNamespaces and excludedNamespaces fails validation",
@@ -391,6 +392,22 @@ func TestRestoreReconcile(t *testing.T) {
 			backupStoreGetBackupContentsErr: errors.New("Couldn't download backup"),
 			backup:                          defaultBackup().StorageLocation("default").Result(),
 		},
+		{
+			name:              "restore attached with an expected finalizer gets cleaned up successfully",
+			location:          defaultStorageLocation,
+			restore:           NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseCompleted).ObjectMeta(builder.WithFinalizers(ExternalResourcesFinalizer), builder.WithDeletionTimestamp(timestamp.Time)).Result(),
+			backup:            defaultBackup().StorageLocation("default").Result(),
+			expectedErr:       false,
+			addValidFinalizer: true,
+		},
+		{
+			name:              "restore attached with an unknown finalizer is not supported",
+			location:          defaultStorageLocation,
+			restore:           NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseCompleted).ObjectMeta(builder.WithFinalizers("restores.velero.io/unknown-finalizer"), builder.WithDeletionTimestamp(timestamp.Time)).Result(),
+			backup:            defaultBackup().StorageLocation("default").Result(),
+			expectedErr:       true,
+			addValidFinalizer: false,
+		},
 	}
 
 	formatFlag := logging.FormatText
@@ -483,6 +500,10 @@ func TestRestoreReconcile(t *testing.T) {
 				pluginManager.On("CleanupClients")
 			}
 
+			if test.addValidFinalizer {
+				backupStore.On("DeleteRestore", test.restore.Name).Return(nil)
+			}
+
 			//err = r.processQueueItem(key)
 			_, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{
 				Namespace: test.restore.Namespace,
@@ -536,7 +557,9 @@ func TestRestoreReconcile(t *testing.T) {
 				assert.Zero(t, restorer.calledWithArg)
 				return
 			}
-			assert.Equal(t, 1, len(restorer.Calls))
+			if !test.addValidFinalizer {
+				assert.Equal(t, 1, len(restorer.Calls))
+			}
 
 			// validate Patch call 2 (setting phase)