Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oadp-1.3: OADP-4265 Mark InProgress backup/restore as failed upon requeuing #315

Merged
merged 5 commits into from
Jun 18, 2024
Merged

oadp-1.3: OADP-4265 Mark InProgress backup/restore as failed upon requeuing #315

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c06b138
Mark InProgress backup/restore as failed upon requeuing
kaovilai Jun 5, 2024
dfb6fb1
cleanup to minimize diff from upstream
sseago Jun 12, 2024
212ccfd
error message update
sseago Jun 12, 2024
d212b4f
requeue on finalize status update.
sseago Jun 12, 2024
a51ef63
Only run patch once for all backup finalizer return scenarios
sseago Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Mark InProgress backup/restore as failed upon requeuing 
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

remove uuid, return err to requeue instead of requeue: true

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
  • Loading branch information
@kaovilai
kaovilai committed Jun 11, 2024
commit c06b13825cd7d2df1ea3b70d0df725006127d92e
1 change: 1 addition & 0 deletions changelogs/unreleased/7863-kaovilai
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Requeue backup/restore in their respective controllers to unstuck InProgress status without requiring velero pod restart.
18 changes: 17 additions & 1 deletion pkg/controller/backup_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,20 @@ func (b *backupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
switch original.Status.Phase {
case "", velerov1api.BackupPhaseNew:
// only process new backups
case velerov1api.BackupPhaseInProgress:
// if backup is in progress, we should not process it again
// we want to mark it as failed to avoid it being stuck in progress
// if so, mark it as failed, last loop did not successfully complete the backup
log.Debug("Backup has in progress status from prior reconcile, marking it as failed")
failedCopy := original.DeepCopy()
failedCopy.Status.Phase = velerov1api.BackupPhaseFailed
failedCopy.Status.FailureReason = "Backup from previous reconcile still in progress"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to suggest an APIServer failure here.
"Backup from previous reconcile still in progress. The API Server may have been down."

if err := kubeutil.PatchResource(original, failedCopy, b.kbClient); err != nil {
// return the error so the status can be re-processed; it's currently still not completed or failed
return ctrl.Result{}, err
}
// patch to mark it as failed succeeded, do not requeue
return ctrl.Result{}, nil
default:
b.logger.WithFields(logrus.Fields{
"backup": kubeutil.NamespaceAndName(original),
Expand All @@ -249,7 +263,6 @@ func (b *backupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
request.Status.Phase = velerov1api.BackupPhaseInProgress
request.Status.StartTimestamp = &metav1.Time{Time: b.clock.Now()}
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To minimize change from upstream since this is in our fork, lets not include whitespace changes like this.

// update status
if err := kubeutil.PatchResource(original, request.Backup, b.kbClient); err != nil {
return ctrl.Result{}, errors.Wrapf(err, "error updating Backup status to %s", request.Status.Phase)
Expand Down Expand Up @@ -309,7 +322,10 @@ func (b *backupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
log.Info("Updating backup's final status")
if err := kubeutil.PatchResource(original, request.Backup, b.kbClient); err != nil {
log.WithError(err).Error("error updating backup's final status")
// return the error so the status can be re-processed; it's currently still not completed or failed
return ctrl.Result{}, err
}

return ctrl.Result{}, nil
}

Expand Down
45 changes: 28 additions & 17 deletions pkg/controller/restore_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,8 @@ func (r *restoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
// the controller.
log := r.logger.WithField("Restore", req.NamespacedName.String())

restore := &api.Restore{}
err := r.kbClient.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: req.Name}, restore)
original := &api.Restore{}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the variable name change necessary here? This makes the diff larger and increases the possibility of rebase conflicts, since we're carrying this commit in our fork.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this so restore_controller and backup_controller has the same var name pattern that's all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary I agree, just make the logic more pastable across both.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaovilai I figured that's why you did that. I reverted that part here in a later commit since we're carrying the commit in our fork right now, and it removed a lot of lines from the diff, making rebase conflicts less likely. Does that seem reasonable to you?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. That's reasonable

err := r.kbClient.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: req.Name}, original)
if err != nil {
if apierrors.IsNotFound(err) {
log.Debugf("restore[%s] not found", req.Name)
Expand All @@ -175,15 +175,15 @@ func (r *restoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
}

// deal with finalizer
if !restore.DeletionTimestamp.IsZero() {
if !original.DeletionTimestamp.IsZero() {
// check the finalizer and run clean-up
if controllerutil.ContainsFinalizer(restore, ExternalResourcesFinalizer) {
if err := r.deleteExternalResources(restore); err != nil {
if controllerutil.ContainsFinalizer(original, ExternalResourcesFinalizer) {
if err := r.deleteExternalResources(original); 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()
restore := original.DeepCopy()
controllerutil.RemoveFinalizer(restore, ExternalResourcesFinalizer)
if err := kubeutil.PatchResource(original, restore, r.kbClient); err != nil {
log.Errorf("fail to remove finalizer: %s", err.Error())
Expand All @@ -197,29 +197,40 @@ func (r *restoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
}

// add finalizer
if restore.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(restore, ExternalResourcesFinalizer) {
original := restore.DeepCopy()
if original.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(original, ExternalResourcesFinalizer) {
restore := original.DeepCopy()
controllerutil.AddFinalizer(restore, ExternalResourcesFinalizer)
if err := kubeutil.PatchResource(original, restore, r.kbClient); err != nil {
log.Errorf("fail to add finalizer: %s", err.Error())
return ctrl.Result{}, err
}
}

switch restore.Status.Phase {
switch original.Status.Phase {
case "", api.RestorePhaseNew:
// only process new restores
case api.RestorePhaseInProgress:
// if restore is in progress, we should not process it again
// we want to mark it as failed to avoid it being stuck in progress
// if so, mark it as failed, last loop did not successfully complete the restore
log.Debug("Restore has in progress status from prior reconcile, marking it as failed")
failedCopy := original.DeepCopy()
failedCopy.Status.Phase = api.RestorePhaseFailed
failedCopy.Status.FailureReason = "Restore from previous reconcile still in progress"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to suggest an APIServer failure here.
"Restore from previous reconcile still in progress. The API Server may have been down."

if err := kubeutil.PatchResource(original, failedCopy, r.kbClient); err != nil {
// return the error so the status can be re-processed; it's currently still not completed or failed
return ctrl.Result{}, err
}
// patch to mark it as failed succeeded, do not requeue
return ctrl.Result{}, nil
default:
r.logger.WithFields(logrus.Fields{
"restore": kubeutil.NamespaceAndName(restore),
"phase": restore.Status.Phase,
"restore": kubeutil.NamespaceAndName(original),
"phase": original.Status.Phase,
}).Debug("Restore is not handled")
return ctrl.Result{}, nil
}

// store a copy of the original restore for creating patch
original := restore.DeepCopy()

restore := original.DeepCopy()
// Validate the restore and fetch the backup
info, resourceModifiers := r.validateAndComplete(restore)

Expand Down Expand Up @@ -272,8 +283,8 @@ func (r *restoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct

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.
// Controller only handle New restore.
// return the error so the status can be re-processed; it's currently still not completed or failed
return ctrl.Result{}, err
}

return ctrl.Result{}, nil
Expand Down