From 40b2ee1323a4155e5c37161cad04391c5059e5ad Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Thu, 29 Jun 2023 13:48:22 +0800 Subject: [PATCH] Modify DownloadRequest controller logic 1. Avoid patch DownloadRequest when it's deleted. 2. Add periodic enqueue resource for reconcile. Signed-off-by: Xun Jiang --- changelogs/unreleased/6433-blackpiglet | 1 + pkg/controller/download_request_controller.go | 74 +++++++++++++------ .../download_request_controller_test.go | 42 +++++------ 3 files changed, 75 insertions(+), 42 deletions(-) create mode 100644 changelogs/unreleased/6433-blackpiglet diff --git a/changelogs/unreleased/6433-blackpiglet b/changelogs/unreleased/6433-blackpiglet new file mode 100644 index 0000000000..a804fc890f --- /dev/null +++ b/changelogs/unreleased/6433-blackpiglet @@ -0,0 +1 @@ +Modify DownloadRequest controller logic \ No newline at end of file diff --git a/pkg/controller/download_request_controller.go b/pkg/controller/download_request_controller.go index 479c6d4079..1f3de3955e 100644 --- a/pkg/controller/download_request_controller.go +++ b/pkg/controller/download_request_controller.go @@ -18,6 +18,7 @@ package controller import ( "context" + "time" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -25,12 +26,18 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clocks "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" kbclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/itemoperationmap" "github.com/vmware-tanzu/velero/pkg/persistence" "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" + "github.com/vmware-tanzu/velero/pkg/util/kube" +) + +const ( + defaultDownloadRequestSyncPeriod = time.Minute ) // downloadRequestReconciler reconciles a DownloadRequest object @@ -93,15 +100,6 @@ func (r *downloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, errors.WithStack(err) } - original := downloadRequest.DeepCopy() - defer func() { - // Always attempt to Patch the downloadRequest object and status after each reconciliation. - if err := r.client.Patch(ctx, downloadRequest, kbclient.MergeFrom(original)); err != nil { - log.WithError(err).Error("Error updating download request") - return - } - }() - if downloadRequest.Status != (velerov1api.DownloadRequestStatus{}) && downloadRequest.Status.Expiration != nil { if downloadRequest.Status.Expiration.Time.Before(r.clock.Now()) { // Delete any request that is expired, regardless of the phase: it is not @@ -111,19 +109,25 @@ func (r *downloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Requ log.WithError(err).Error("Error deleting an expired download request") return ctrl.Result{}, errors.WithStack(err) } - return ctrl.Result{Requeue: false}, nil + return ctrl.Result{}, nil } else if downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseProcessed { - // Requeue the request if is not yet expired and has already been processed before, - // since it might still be in use by the logs streaming and shouldn't - // be deleted until after its expiration. - log.Debug("DownloadRequest has not yet expired - requeueing") - return ctrl.Result{Requeue: true}, nil + log.Debug("DownloadRequest has not yet expired.") + return ctrl.Result{}, nil } } // Process a brand new request. - backupName := downloadRequest.Spec.Target.Name if downloadRequest.Status.Phase == "" || downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseNew { + backupName := downloadRequest.Spec.Target.Name + original := downloadRequest.DeepCopy() + defer func() { + // Always attempt to Patch the downloadRequest object and status for new DownloadRequest. + if err := r.client.Patch(ctx, downloadRequest, kbclient.MergeFrom(original)); err != nil { + log.WithError(err).Error("Error updating download request") + return + } + }() + // Update the expiration. downloadRequest.Status.Expiration = &metav1.Time{Time: r.clock.Now().Add(persistence.DownloadURLTTL)} @@ -136,6 +140,11 @@ func (r *downloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Requ Namespace: downloadRequest.Namespace, Name: downloadRequest.Spec.Target.Name, }, restore); err != nil { + if apierrors.IsNotFound(err) { + log.WithError(err).Error("fail to get restore for DownloadRequest") + return ctrl.Result{}, nil + } + log.Warnf("Fail to get restore for DownloadRequest %s. Retry later.", err.Error()) return ctrl.Result{}, errors.WithStack(err) } backupName = restore.Spec.BackupName @@ -146,6 +155,11 @@ func (r *downloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Requ Namespace: downloadRequest.Namespace, Name: backupName, }, backup); err != nil { + if apierrors.IsNotFound(err) { + log.WithError(err).Error("fail to get backup for DownloadRequest") + return ctrl.Result{}, nil + } + log.Warnf("fail to get backup for DownloadRequest %s. Retry later.", err.Error()) return ctrl.Result{}, errors.WithStack(err) } @@ -154,6 +168,11 @@ func (r *downloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Requ Namespace: backup.Namespace, Name: backup.Spec.StorageLocation, }, location); err != nil { + if apierrors.IsNotFound(err) { + log.Errorf("BSL for DownloadRequest cannot be found") + return ctrl.Result{}, nil + } + log.Warnf("Fail to get BSL for DownloadRequest: %s", err.Error()) return ctrl.Result{}, errors.WithStack(err) } @@ -163,7 +182,9 @@ func (r *downloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Requ backupStore, err := r.backupStoreGetter.Get(location, pluginManager, log) if err != nil { log.WithError(err).Error("Error getting a backup store") - return ctrl.Result{}, errors.WithStack(err) + // Fail to get backup store is due to BSL setting issue or credential issue. + // It cannot be recovered. No need to retry. + return ctrl.Result{}, nil } // If this is a request for backup item operations, force upload of in-memory operations that @@ -180,8 +201,10 @@ func (r *downloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Requ // ignore errors here. If we can't upload anything here, process the download as usual _ = r.restoreItemOperationsMap.UpdateForRestore(backupStore, downloadRequest.Spec.Target.Name) } + if downloadRequest.Status.DownloadURL, err = backupStore.GetDownloadURL(downloadRequest.Spec.Target); err != nil { - return ctrl.Result{Requeue: true}, errors.WithStack(err) + log.Warnf("fail to get Backup metadata file's download URL %s, retry later: %s", downloadRequest.Spec.Target, err) + return ctrl.Result{}, errors.WithStack(err) } downloadRequest.Status.Phase = velerov1api.DownloadRequestPhaseProcessed @@ -190,13 +213,22 @@ func (r *downloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Requ downloadRequest.Status.Expiration = &metav1.Time{Time: r.clock.Now().Add(persistence.DownloadURLTTL)} } - // Requeue is mostly to handle deleting any expired requests that were not - // deleted as part of the normal client flow for whatever reason. - return ctrl.Result{Requeue: true}, nil + return ctrl.Result{}, nil } func (r *downloadRequestReconciler) SetupWithManager(mgr ctrl.Manager) error { + downloadRequestSource := kube.NewPeriodicalEnqueueSource(r.log, mgr.GetClient(), + &velerov1api.DownloadRequestList{}, defaultDownloadRequestSyncPeriod, kube.PeriodicalEnqueueSourceOption{}) + downloadRequestPredicates := kube.NewGenericEventPredicate(func(object kbclient.Object) bool { + downloadRequest := object.(*velerov1api.DownloadRequest) + if downloadRequest.Status != (velerov1api.DownloadRequestStatus{}) && downloadRequest.Status.Expiration != nil { + return downloadRequest.Status.Expiration.Time.Before(r.clock.Now()) + } + return true + }) + return ctrl.NewControllerManagedBy(mgr). For(&velerov1api.DownloadRequest{}). + Watches(downloadRequestSource, nil, builder.WithPredicates(downloadRequestPredicates)). Complete(r) } diff --git a/pkg/controller/download_request_controller_test.go b/pkg/controller/download_request_controller_test.go index ad53b6972d..1847719913 100644 --- a/pkg/controller/download_request_controller_test.go +++ b/pkg/controller/download_request_controller_test.go @@ -156,55 +156,55 @@ var _ = Describe("Download Request Reconciler", func() { } }, - Entry("backup contents request for nonexistent backup returns an error", request{ + Entry("backup contents request for nonexistent backup returns nil", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupContents, "a1-backup").Result(), backup: builder.ForBackup(velerov1api.DefaultNamespace, "non-matching-backup").StorageLocation("a-location").Result(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), - expectedReconcileErr: "backups.velero.io \"a1-backup\" not found", - expectedRequeue: ctrl.Result{Requeue: false}, + expectedReconcileErr: "", + expectedRequeue: ctrl.Result{}, }), - Entry("restore log request for nonexistent restore returns an error", request{ + Entry("restore log request for nonexistent restore returns nil", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214").Result(), restore: builder.ForRestore(velerov1api.DefaultNamespace, "non-matching-restore").Phase(velerov1api.RestorePhaseCompleted).Backup("a-backup").Result(), backup: defaultBackup(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), - expectedReconcileErr: "restores.velero.io \"a-backup-20170912150214\" not found", - expectedRequeue: ctrl.Result{Requeue: false}, + expectedReconcileErr: "", + expectedRequeue: ctrl.Result{}, }), - Entry("backup contents request for backup with nonexistent location returns an error", request{ + Entry("backup contents request for backup with nonexistent location returns nil", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupContents, "a-backup").Result(), backup: defaultBackup(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "non-matching-location").Provider("a-provider").Bucket("a-bucket").Result(), - expectedReconcileErr: "backupstoragelocations.velero.io \"a-location\" not found", - expectedRequeue: ctrl.Result{Requeue: false}, + expectedReconcileErr: "", + expectedRequeue: ctrl.Result{}, }), Entry("backup contents request with phase '' gets a url", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupContents, "a-backup").Result(), backup: defaultBackup(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), expectGetsURL: true, - expectedRequeue: ctrl.Result{Requeue: true}, + expectedRequeue: ctrl.Result{}, }), Entry("backup contents request with phase 'New' gets a url", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindBackupContents, "a-backup").Result(), backup: defaultBackup(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), expectGetsURL: true, - expectedRequeue: ctrl.Result{Requeue: true}, + expectedRequeue: ctrl.Result{}, }), Entry("backup log request with phase '' gets a url", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupLog, "a-backup").Result(), backup: defaultBackup(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), expectGetsURL: true, - expectedRequeue: ctrl.Result{Requeue: true}, + expectedRequeue: ctrl.Result{}, }), Entry("backup log request with phase 'New' gets a url", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindBackupLog, "a-backup").Result(), backup: defaultBackup(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), expectGetsURL: true, - expectedRequeue: ctrl.Result{Requeue: true}, + expectedRequeue: ctrl.Result{}, }), Entry("restore log request with phase '' gets a url", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214").Result(), @@ -212,7 +212,7 @@ var _ = Describe("Download Request Reconciler", func() { backup: defaultBackup(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), expectGetsURL: true, - expectedRequeue: ctrl.Result{Requeue: true}, + expectedRequeue: ctrl.Result{}, }), Entry("restore log request with phase 'New' gets a url", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214").Result(), @@ -220,7 +220,7 @@ var _ = Describe("Download Request Reconciler", func() { restore: builder.ForRestore(velerov1api.DefaultNamespace, "a-backup-20170912150214").Phase(velerov1api.RestorePhaseCompleted).Backup("a-backup").Result(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), expectGetsURL: true, - expectedRequeue: ctrl.Result{Requeue: true}, + expectedRequeue: ctrl.Result{}, }), Entry("restore results request with phase '' gets a url", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindRestoreResults, "a-backup-20170912150214").Result(), @@ -228,7 +228,7 @@ var _ = Describe("Download Request Reconciler", func() { backup: defaultBackup(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), expectGetsURL: true, - expectedRequeue: ctrl.Result{Requeue: true}, + expectedRequeue: ctrl.Result{}, }), Entry("restore results request with phase 'New' gets a url", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindRestoreResults, "a-backup-20170912150214").Result(), @@ -236,30 +236,30 @@ var _ = Describe("Download Request Reconciler", func() { backup: defaultBackup(), backupLocation: builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(), expectGetsURL: true, - expectedRequeue: ctrl.Result{Requeue: true}, + expectedRequeue: ctrl.Result{}, }), Entry("request with phase 'Processed' and not expired is not deleted", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseProcessed).Target(velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214").Result(), backup: defaultBackup(), - expectedRequeue: ctrl.Result{Requeue: true}, + expectedRequeue: ctrl.Result{}, }), Entry("request with phase 'Processed' and expired is deleted", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseProcessed).Target(velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214").Result(), backup: defaultBackup(), expired: true, - expectedRequeue: ctrl.Result{Requeue: false}, + expectedRequeue: ctrl.Result{}, }), Entry("request with phase '' and expired is deleted", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214").Result(), backup: defaultBackup(), expired: true, - expectedRequeue: ctrl.Result{Requeue: false}, + expectedRequeue: ctrl.Result{}, }), Entry("request with phase 'New' and expired is deleted", request{ downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214").Result(), backup: defaultBackup(), expired: true, - expectedRequeue: ctrl.Result{Requeue: false}, + expectedRequeue: ctrl.Result{}, }), ) })