Modify DownloadRequest controller logic

1. Avoid patch DownloadRequest when it's deleted.
2. Add periodic enqueue resource for reconcile.

Signed-off-by: Xun Jiang <jxun@vmware.com>

changelogs/unreleased/6433-blackpiglet

@@ -0,0 +1 @@
+Modify DownloadRequest controller logic

pkg/cmd/server/server.go

@@ -90,6 +90,7 @@ const (
 	defaultStoreValidationFrequency   = time.Minute
 	defaultPodVolumeOperationTimeout  = 240 * time.Minute
 	defaultResourceTerminatingTimeout = 10 * time.Minute
+	defaultDownloadRequestSyncPeriod  = time.Minute
 
 	// server's client default qps and burst
 	defaultClientQPS      float32 = 20.0
@@ -890,6 +891,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string
 			s.logger,
 			backupOpsMap,
 			restoreOpsMap,
+			defaultDownloadRequestSyncPeriod,
 		)
 		if err := r.SetupWithManager(s.mgr); err != nil {
 			s.logger.Fatal(err, "unable to create controller", "controller", controller.DownloadRequest)

pkg/controller/download_request_controller.go

@@ -18,19 +18,22 @@ package controller
 
 import (
 	"context"
+	"time"
 
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	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"
 )
 
 // downloadRequestReconciler reconciles a DownloadRequest object
@@ -47,7 +50,8 @@ type downloadRequestReconciler struct {
 	// used to force update of async restore item operations before processing download request
 	restoreItemOperationsMap *itemoperationmap.RestoreItemOperationsMap
 
-	log logrus.FieldLogger
+	log       logrus.FieldLogger
+	frequency time.Duration
 }
 
 // NewDownloadRequestReconciler initializes and returns downloadRequestReconciler struct.
@@ -59,6 +63,7 @@ func NewDownloadRequestReconciler(
 	log logrus.FieldLogger,
 	backupItemOperationsMap *itemoperationmap.BackupItemOperationsMap,
 	restoreItemOperationsMap *itemoperationmap.RestoreItemOperationsMap,
+	syncPeriod time.Duration,
 ) *downloadRequestReconciler {
 	return &downloadRequestReconciler{
 		client:                   client,
@@ -68,6 +73,7 @@ func NewDownloadRequestReconciler(
 		backupItemOperationsMap:  backupItemOperationsMap,
 		restoreItemOperationsMap: restoreItemOperationsMap,
 		log:                      log,
+		frequency:                syncPeriod,
 	}
 }
 
@@ -93,15 +99,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 +108,28 @@ 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 after each reconciliation.
+			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 +142,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 +157,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 +170,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 +184,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 +203,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 +215,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{}, r.frequency, 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)
 }

pkg/controller/download_request_controller_test.go

@@ -114,6 +114,7 @@ var _ = Describe("Download Request Reconciler", func() {
 				velerotest.NewLogger(),
 				nil,
 				nil,
+				time.Minute,
 			)
 
 			if test.backupLocation != nil && test.expectGetsURL {
@@ -156,110 +157,110 @@ 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(),
 			restore:         builder.ForRestore(velerov1api.DefaultNamespace, "a-backup-20170912150214").Phase(velerov1api.RestorePhaseCompleted).Backup("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 'New' gets a url", request{
 			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214").Result(),
 			backup:          defaultBackup(),
 			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(),
 			restore:         builder.ForRestore(velerov1api.DefaultNamespace, "a-backup-20170912150214").Phase(velerov1api.RestorePhaseCompleted).Backup("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 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(),
 			restore:         builder.ForRestore(velerov1api.DefaultNamespace, "a-backup-20170912150214").Phase(velerov1api.RestorePhaseCompleted).Backup("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("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{},
 		}),
 	)
 })