feat(backup): delete backup in the backupstore asynchronously #3038

ChanYiLin · 2024-08-01T13:54:49Z

follow LEP to implement: longhorn/longhorn#9152

Make backup deletion asynchronous
Add Deleting state to Backup
- Add deleting in-memory map to allow go routine to pass the command failure to the controller.
- Add backoff to delay deletion command execution.
- When the Backup is being deleted it should follow the following diagram
Disallow backup creation when there is any backup being deleted.

# Normal Case
Completed => Deleting 
=> finalizer removed (CR isgone)

# Command failure Case
Completed => Deleting 
=> Error (found the error message in the map) 
=> Deleting (Retry the command) => finalizer removed (CR isgone)

# Controller crashes
Completed => Deleting 
=> Error (failed to find the record in the map)
=> Deleting (Retry the command) => finalizer removed (CR isgone)

ChanYiLin · 2024-08-01T14:06:40Z

Test Plan

Normal Case

Create a Volume
Write small data and then create a BackupA
Write large data (~2G) and then create a BackupB
Write small data and then create a Snapshot
Delete the BackupB(large data), at the same time, create a BackupC from the Snapshot(you can click from UI)
BackupC will be in Pending state with message (waiting for backupB to be deleted)
After BackupB is deleted, BackupC should be in progress.

Error Case (use nfs)

Create a Volume
Write some data and then create a BackupA
Write some data and then create a sSnapshot
Exec into the backupstore pod and make the backup.cfg immutable
```
$ chattr +i backups/backup_backup-5640dfd33a054f98.cfg`
```
Delete the BackupA, at the same time, create a BackupB from the Snapshot(you can click from UI)
BackupA will be in Deleting and Error state repeatedly to retry the deletion. When in Error state, it shows error message related to permission
BackupB will be InProgress when BackupA is in Deleting state. BackupB should be complete after awhile.
Remove the immutable, after a while, the BackupA should be in Deleting again and should be deleted successfully.

Controller Crashes Case (use nfs)

Create a Volume
Write some data and then create a BackupA
Exec into the backupstore pod and make the backup.cfg immutable, example
```
$ chattr +i backups/backup_backup-5640dfd33a054f98.cfg`
```
Delete the BackupA
BackupA will be in Deleting and Error state repeatedly to retry the deletion. When in Error state, it shows error message related to permission
When the BackupA is in Deleting state, delete the longhorn manager pod directly. (you can find the one doing the deleting with Backup.Status.OwnerID)
After the longhorn manager pod is recreated, the BackupA should turn into Error state with message No deletion in progress record, retry the deletion command
Then after a while the BackupA should be in Deleting again and should be deleted successfully after remove the immutable.

ChanYiLin · 2024-08-02T09:57:22Z

There is a regression
https://ci.longhorn.io/job/private/job/longhorn-tests-regression/7389/console
test_backup_status_for_unavailable_replicas
I know the root cause of it, fixing...

ChanYiLin · 2024-08-05T10:48:55Z

There is a regression https://ci.longhorn.io/job/private/job/longhorn-tests-regression/7389/console test_backup_status_for_unavailable_replicas I know the root cause of it, fixing...

Fixed.
https://ci.longhorn.io/job/private/job/longhorn-tests-regression/7392/

ChanYiLin · 2024-08-09T07:53:54Z

change the backoff time to
min: 1Min -> max: 24hour(1 time/day)

derekbit · 2024-09-02T01:38:49Z

@ChanYiLin Is it ready for review?

ChanYiLin · 2024-09-02T04:00:21Z

Yes, it is ready for review.

ChanYiLin · 2024-09-20T11:05:17Z

Hi @derekbit
This is ready for review, thanks

controller/backup_controller.go

derekbit · 2024-09-23T14:35:12Z

controller/backup_controller.go

+			if !backupDeleted {
 				return nil
 			}


The PR introduces deletingBackoff for handling deletion backoff. Since handleErr() can also achieve backoff after returning an error if the backup has not been deleted yet. Can we remove deletingBackoff and leverage handleErr() instead?

I prefer not to do that, it is not an error after all.
Besides, inside the handleErr we even print the logs Failed to sync Longhorn backup
I think that might confuse the users.

We actually use this backoff a lot in our controllers

engine-controller: https://github.com/longhorn/longhorn-manager/blob/master/controller/engine_controller.go#L93

backing-image-data-source: https://github.com/longhorn/longhorn-manager/blob/master/controller/backing_image_data_source_controller.go#L57

Another point is that, we can control the backOff period with this backoff
But with using k8s workqueue, we can't set different backOff time for this case from other error cases.

ref: longhorn/longhorn 8746 Signed-off-by: Jack Lin <jack.lin@suse.com>

ChanYiLin · 2024-09-24T07:19:08Z

Hi @derekbit
I have updated the PR, please take a look, thanks!
Let me know if you have any concern for the last comment.

derekbit

In general, LGTM

derekbit · 2024-09-25T13:58:20Z

controller/backup_controller.go

+		deletingMapLock:       &sync.Mutex{},
+		inProgressDeletingMap: map[string]*DeletingStatus{},
+
+		deletingBackoff: flowcontrol.NewBackOff(time.Minute*1, time.Hour*24),


Make time.Minute*1, time.Hour*24 as a constant

derekbit · 2024-09-26T03:09:44Z

controller/backup_controller.go

+	// We should consider the backup exists even the backupInfo is nil when it is in progress.
+	backupInfo, err := backupTargetClient.BackupGet(backupURL, backupTargetClient.Credential)
+	if err != nil && !types.ErrorIsNotFound(err) && !types.ErrorIsInProgress(err) {
+		log.WithError(err).Debugf("failed to check backup %v in the backupstore", backup.Name)


Suggested change

log.WithError(err).Debugf("failed to check backup %v in the backupstore", backup.Name)

log.WithError(err).Debugf("Failed to check backup %v in the backupstore", backup.Name)

derekbit · 2024-09-26T03:12:16Z

controller/backup_controller.go

+			return false
+		}
+		if _, err := bc.ds.UpdateBackupStatus(backup); err != nil {
+			log.WithError(err).Debugf("Backup %v update status error", backup.Name)


Suggested change

log.WithError(err).Debugf("Backup %v update status error", backup.Name)

log.WithError(err).Errorf("Backup %v update status error", backup.Name)

ChanYiLin requested review from derekbit and c3y1huang August 1, 2024 13:54

ChanYiLin self-assigned this Aug 1, 2024

ChanYiLin force-pushed the LH8746_make_backup_deletion_async_and_add_deleting_state branch 2 times, most recently from c809fd8 to 707f44c Compare August 1, 2024 14:04

ChanYiLin force-pushed the LH8746_make_backup_deletion_async_and_add_deleting_state branch from 707f44c to 4e74975 Compare August 1, 2024 14:12

ChanYiLin marked this pull request as draft August 2, 2024 09:53

ChanYiLin force-pushed the LH8746_make_backup_deletion_async_and_add_deleting_state branch from 4e74975 to 5e21ded Compare August 5, 2024 10:00

ChanYiLin marked this pull request as ready for review August 5, 2024 10:49

ChanYiLin force-pushed the LH8746_make_backup_deletion_async_and_add_deleting_state branch from 5e21ded to cf4419c Compare August 9, 2024 07:52

innobead requested a review from shuo-wu August 11, 2024 16:14

derekbit reviewed Sep 23, 2024

View reviewed changes

derekbit requested a review from PhanLe1010 September 23, 2024 14:59

feat(backup): delete backup in the backupstore asynchronously

25dcfe8

ref: longhorn/longhorn 8746 Signed-off-by: Jack Lin <jack.lin@suse.com>

ChanYiLin force-pushed the LH8746_make_backup_deletion_async_and_add_deleting_state branch from cf4419c to 25dcfe8 Compare September 24, 2024 07:08

ChanYiLin requested a review from derekbit September 25, 2024 06:00

derekbit reviewed Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backup): delete backup in the backupstore asynchronously #3038

feat(backup): delete backup in the backupstore asynchronously #3038

ChanYiLin commented Aug 1, 2024

ChanYiLin commented Aug 1, 2024 •

edited

Loading

ChanYiLin commented Aug 2, 2024

ChanYiLin commented Aug 5, 2024

ChanYiLin commented Aug 9, 2024

derekbit commented Sep 2, 2024

ChanYiLin commented Sep 2, 2024

ChanYiLin commented Sep 20, 2024

derekbit Sep 23, 2024

ChanYiLin Sep 24, 2024

ChanYiLin Sep 24, 2024 •

edited

Loading

ChanYiLin commented Sep 24, 2024

derekbit left a comment

derekbit Sep 25, 2024

derekbit Sep 26, 2024

derekbit Sep 26, 2024

	log.WithError(err).Debugf("failed to check backup %v in the backupstore", backup.Name)
	log.WithError(err).Debugf("Failed to check backup %v in the backupstore", backup.Name)

	log.WithError(err).Debugf("Backup %v update status error", backup.Name)
	log.WithError(err).Errorf("Backup %v update status error", backup.Name)

feat(backup): delete backup in the backupstore asynchronously #3038

Are you sure you want to change the base?

feat(backup): delete backup in the backupstore asynchronously #3038

Conversation

ChanYiLin commented Aug 1, 2024

ChanYiLin commented Aug 1, 2024 • edited Loading

Test Plan

Normal Case

Error Case (use nfs)

Controller Crashes Case (use nfs)

ChanYiLin commented Aug 2, 2024

ChanYiLin commented Aug 5, 2024

ChanYiLin commented Aug 9, 2024

derekbit commented Sep 2, 2024

ChanYiLin commented Sep 2, 2024

ChanYiLin commented Sep 20, 2024

derekbit Sep 23, 2024

Choose a reason for hiding this comment

ChanYiLin Sep 24, 2024

Choose a reason for hiding this comment

ChanYiLin Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

ChanYiLin commented Sep 24, 2024

derekbit left a comment

Choose a reason for hiding this comment

derekbit Sep 25, 2024

Choose a reason for hiding this comment

derekbit Sep 26, 2024

Choose a reason for hiding this comment

derekbit Sep 26, 2024

Choose a reason for hiding this comment

ChanYiLin commented Aug 1, 2024 •

edited

Loading

ChanYiLin Sep 24, 2024 •

edited

Loading