diff --git a/changelogs/unreleased/6479-allenxu404 b/changelogs/unreleased/6479-allenxu404 new file mode 100644 index 0000000000..559fe2046a --- /dev/null +++ b/changelogs/unreleased/6479-allenxu404 @@ -0,0 +1 @@ +Add restore finalizer to clean up external resources \ No newline at end of file diff --git a/pkg/cmd/cli/uninstall/uninstall.go b/pkg/cmd/cli/uninstall/uninstall.go index 884ba0312a..6eb8226bbf 100644 --- a/pkg/cmd/cli/uninstall/uninstall.go +++ b/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 +} diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index c63de4fa5f..9bdceda00a 100644 --- a/pkg/controller/restore_controller.go +++ b/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) diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index 3e8c21677c..dac365eec4 100644 --- a/pkg/controller/restore_controller_test.go +++ b/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)