Skip to content

Commit

Permalink
Patch dbr's status when error happens
Browse files Browse the repository at this point in the history 
This commit makes sure the dbr's status is "Processed" when an error
happens before the actual deletion is started

fixes #7812

Signed-off-by: Daniel Jiang <[email protected]>
  • Loading branch information
@reasonerjt
reasonerjt committed Aug 6, 2024
1 parent f7f9ed3 commit 5c88c89
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 35 deletions.
1 change: 1 addition & 0 deletions changelogs/unreleased/8086-reasonerjt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Patch dbr's status when error happens
58 changes: 27 additions & 31 deletions pkg/controller/backup_deletion_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque

// Make sure we have the backup name
if dbr.Spec.BackupName == "" {
_, err := r.patchDeleteBackupRequest(ctx, dbr, func(res *velerov1api.DeleteBackupRequest) {
res.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed
res.Status.Errors = []string{"spec.backupName is required"}
})
err := r.patchDeleteBackupRequestWithError(ctx, dbr, errors.New("spec.backupName is required"))
return ctrl.Result{}, err
}

Expand All @@ -163,10 +160,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque

// Don't allow deleting an in-progress backup
if r.backupTracker.Contains(dbr.Namespace, dbr.Spec.BackupName) {
_, err := r.patchDeleteBackupRequest(ctx, dbr, func(r *velerov1api.DeleteBackupRequest) {
r.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed
r.Status.Errors = []string{"backup is still in progress"}
})
err := r.patchDeleteBackupRequestWithError(ctx, dbr, errors.New("backup is still in progress"))
return ctrl.Result{}, err
}

Expand All @@ -177,10 +171,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque
Name: dbr.Spec.BackupName,
}, backup); apierrors.IsNotFound(err) {
// Couldn't find backup - update status to Processed and record the not-found error
_, err = r.patchDeleteBackupRequest(ctx, dbr, func(r *velerov1api.DeleteBackupRequest) {
r.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed
r.Status.Errors = []string{"backup not found"}
})
err = r.patchDeleteBackupRequestWithError(ctx, dbr, errors.New("backup not found"))
return ctrl.Result{}, err
} else if err != nil {
return ctrl.Result{}, errors.Wrap(err, "error getting backup")
Expand All @@ -193,20 +184,14 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque
Name: backup.Spec.StorageLocation,
}, location); err != nil {
if apierrors.IsNotFound(err) {
_, err := r.patchDeleteBackupRequest(ctx, dbr, func(r *velerov1api.DeleteBackupRequest) {
r.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed
r.Status.Errors = append(r.Status.Errors, fmt.Sprintf("backup storage location %s not found", backup.Spec.StorageLocation))
})
err := r.patchDeleteBackupRequestWithError(ctx, dbr, fmt.Errorf("backup storage location %s not found", backup.Spec.StorageLocation))
return ctrl.Result{}, err
}
return ctrl.Result{}, errors.Wrap(err, "error getting backup storage location")
}

if location.Spec.AccessMode == velerov1api.BackupStorageLocationAccessModeReadOnly {
_, err := r.patchDeleteBackupRequest(ctx, dbr, func(r *velerov1api.DeleteBackupRequest) {
r.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed
r.Status.Errors = append(r.Status.Errors, fmt.Sprintf("cannot delete backup because backup storage location %s is currently in read-only mode", location.Name))
})
err := r.patchDeleteBackupRequestWithError(ctx, dbr, fmt.Errorf("cannot delete backup because backup storage location %s is currently in read-only mode", location.Name))
return ctrl.Result{}, err
}

Expand Down Expand Up @@ -236,8 +221,9 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque
b.Status.Phase = velerov1api.BackupPhaseDeleting
})
if err != nil {
log.WithError(errors.WithStack(err)).Error("Error setting backup phase to deleting")
return ctrl.Result{}, err
log.WithError(err).Error("Error setting backup phase to deleting")
err2 := r.patchDeleteBackupRequestWithError(ctx, dbr, errors.Wrap(err, "error setting backup phase to deleting"))
return ctrl.Result{}, err2

Check warning on line 226 in pkg/controller/backup_deletion_controller.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/backup_deletion_controller.go#L224-L226 

Added lines #L224 - L226 were not covered by tests
}

backupScheduleName := backup.GetLabels()[velerov1api.ScheduleNameLabel]
Expand All @@ -248,17 +234,17 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque

backupStore, err := r.backupStoreGetter.Get(location, pluginManager, log)
if err != nil {
_, patchErr := r.patchDeleteBackupRequest(ctx, dbr, func(r *velerov1api.DeleteBackupRequest) {
r.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed
r.Status.Errors = append(r.Status.Errors, fmt.Sprintf("cannot delete backup because backup storage location %s is currently unavailable, error: %s", location.Name, err.Error()))
})
return ctrl.Result{}, patchErr
log.WithError(err).Error("Error getting the backup store")
err2 := r.patchDeleteBackupRequestWithError(ctx, dbr, errors.Wrap(err, "error getting the backup store"))
return ctrl.Result{}, err2
}

actions, err := pluginManager.GetDeleteItemActions()
log.Debugf("%d actions before invoking actions", len(actions))
if err != nil {
return ctrl.Result{}, errors.Wrap(err, "error getting delete item actions")
log.WithError(err).Error("Error getting delete item actions")
err2 := r.patchDeleteBackupRequestWithError(ctx, dbr, errors.New("error getting delete item actions"))
return ctrl.Result{}, err2

Check warning on line 247 in pkg/controller/backup_deletion_controller.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/backup_deletion_controller.go#L245-L247 

Added lines #L245 - L247 were not covered by tests
}
// don't defer CleanupClients here, since it was already called above.

Expand All @@ -270,7 +256,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque
log.WithError(err).Errorf("Unable to download tarball for backup %s, skipping associated DeleteItemAction plugins", backup.Name)
} else {
defer closeAndRemoveFile(backupFile, r.logger)
ctx := &delete.Context{
deleteCtx := &delete.Context{

Check warning on line 259 in pkg/controller/backup_deletion_controller.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/backup_deletion_controller.go#L259 

Added line #L259 was not covered by tests
Backup: backup,
BackupReader: backupFile,
Actions: actions,
Expand All @@ -281,9 +267,11 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque

// Optimization: wrap in a gofunc? Would be useful for large backups with lots of objects.
// but what do we do with the error returned? We can't just swallow it as that may lead to dangling resources.
err = delete.InvokeDeleteActions(ctx)
err = delete.InvokeDeleteActions(deleteCtx)

Check warning on line 270 in pkg/controller/backup_deletion_controller.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/backup_deletion_controller.go#L270 

Added line #L270 was not covered by tests
if err != nil {
return ctrl.Result{}, errors.Wrap(err, "error invoking delete item actions")
log.WithError(err).Error("Error invoking delete item actions")
err2 := r.patchDeleteBackupRequestWithError(ctx, dbr, errors.New("error invoking delete item actions"))
return ctrl.Result{}, err2

Check warning on line 274 in pkg/controller/backup_deletion_controller.go

View check run for this annotation

Codecov / codecov/patch

pkg/controller/backup_deletion_controller.go#L272-L274 

Added lines #L272 - L274 were not covered by tests
}
}
}
Expand Down Expand Up @@ -593,6 +581,14 @@ func (r *backupDeletionReconciler) patchDeleteBackupRequest(ctx context.Context,
return req, nil
}

func (r *backupDeletionReconciler) patchDeleteBackupRequestWithError(ctx context.Context, req *velerov1api.DeleteBackupRequest, err error) error {
_, err = r.patchDeleteBackupRequest(ctx, req, func(r *velerov1api.DeleteBackupRequest) {
r.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed
r.Status.Errors = []string{err.Error()}
})
return err
}

func (r *backupDeletionReconciler) patchBackup(ctx context.Context, backup *velerov1api.Backup, mutate func(*velerov1api.Backup)) (*velerov1api.Backup, error) {
//TODO: The patchHelper can't be used here because the `backup/xxx/status` does not exist, until the backup resource is refactored

Expand Down
8 changes: 4 additions & 4 deletions pkg/controller/backup_deletion_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,16 +122,16 @@ func TestBackupDeletionControllerReconcile(t *testing.T) {
},
},
}
td := setupBackupDeletionControllerTest(t, defaultTestDbr(), location, backup)
dbr := defaultTestDbr()
td := setupBackupDeletionControllerTest(t, dbr, location, backup)
td.controller.backupStoreGetter = &fakeErrorBackupStoreGetter{}
_, err := td.controller.Reconcile(ctx, td.req)
require.NoError(t, err)
res := &velerov1api.DeleteBackupRequest{}
err = td.fakeClient.Get(ctx, td.req.NamespacedName, res)
require.NoError(t, err)
td.fakeClient.Get(ctx, td.req.NamespacedName, res)
assert.Equal(t, "Processed", string(res.Status.Phase))
assert.Len(t, res.Status.Errors, 1)
assert.True(t, strings.HasPrefix(res.Status.Errors[0], fmt.Sprintf("cannot delete backup because backup storage location %s is currently unavailable", location.Name)))
assert.True(t, strings.HasPrefix(res.Status.Errors[0], "error getting the backup store"))
})

t.Run("missing spec.backupName", func(t *testing.T) {
Expand Down

0 comments on commit 5c88c89

Please sign in to comment.