mirrors/velero
1
0
Fork 0
mirror of https://github.com/vmware-tanzu/velero.git synced 2026-01-05 04:55:22 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Modify DownloadRequest controller logic

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

Signed-off-by: Xun Jiang <jxun@vmware.com>
This commit is contained in:
Xun Jiang
2023-06-29 13:48:22 +08:00
parent de83980a05
commit 40b2ee1323
3 changed files with 75 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
changelogs/unreleased/6433-blackpiglet Normal file
View File

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

74
pkg/controller/download_request_controller.go
View File

@@ -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)
}

42
pkg/controller/download_request_controller_test.go
View File

@@ -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{},
}),
)
})