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.

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

fix(backup): only delete backups' resources when synchronization #3193

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

mantissahz
Copy link
Contributor

Which issue(s) this PR fixes:

Issue # longhorn/longhorn#9530

What this PR does / why we need it:

Only delete backups' resources (BackupVolume, BackupBackingImage, SystemBackup, and Backup) when synchronization.

Add the label DeleteCustomResourceOnly, and delete remote backup data when deleting the corresponding backup resource in the cluster and the label does not exist.

Special notes for your reviewer:

Additional documentation or context

datastore/longhorn.go Outdated Show resolved Hide resolved
@mantissahz mantissahz requested a review from a team October 8, 2024 03:53
Copy link
Contributor

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

Do we need to add a validation/mutation webhook?
If the owner of a Backup/BackupVolume contains LabelLonghornDeleteResourceOnly, the webhook should add this label for the Backup/BackupVolume as well.

controller/backup_volume_controller.go Outdated Show resolved Hide resolved
controller/backup_target_controller.go Outdated Show resolved Hide resolved
controller/backup_target_controller.go Outdated Show resolved Hide resolved
controller/backup_target_controller.go Outdated Show resolved Hide resolved
controller/backup_volume_controller.go Outdated Show resolved Hide resolved
controller/backup_volume_controller.go Outdated Show resolved Hide resolved
datastore/longhorn.go Outdated Show resolved Hide resolved
datastore/longhorn.go Outdated Show resolved Hide resolved
datastore/longhorn.go Outdated Show resolved Hide resolved
controller/backup_target_controller.go Outdated Show resolved Hide resolved
controller/backup_target_controller.go Outdated Show resolved Hide resolved
controller/backup_target_controller.go Outdated Show resolved Hide resolved
controller/backup_target_controller.go Outdated Show resolved Hide resolved
controller/backup_target_controller.go Outdated Show resolved Hide resolved
controller/backup_volume_controller.go Outdated Show resolved Hide resolved
controller/backup_volume_controller.go Outdated Show resolved Hide resolved
@mantissahz mantissahz requested a review from a team October 8, 2024 04:09
@mantissahz mantissahz force-pushed the issue9530 branch 2 times, most recently from 38a629e to bfb0e95 Compare October 8, 2024 06:21
@mantissahz
Copy link
Contributor Author

Do we need to add a validation/mutation webhook?
If the owner of a Backup/BackupVolume contains LabelLonghornDeleteResourceOnly, the webhook should add this label for the Backup/BackupVolume as well

Modified the BackupVolume mutation to check if the backup volume contains the label delete-custom-resource-only and add the label to backups of the backup volume.

derekbit
derekbit previously approved these changes Oct 8, 2024
Copy link
Member

@derekbit derekbit left a comment

Choose a reason for hiding this comment

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

LGTM

@derekbit
Copy link
Member

derekbit commented Oct 8, 2024

@PhanLe1010 Could you review the PR if you have time? Thank you.

derekbit
derekbit previously approved these changes Oct 9, 2024
Copy link
Member

@derekbit derekbit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ChanYiLin ChanYiLin left a comment

Choose a reason for hiding this comment

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

Just one nit .

controller/backup_backing_image_controller.go Outdated Show resolved Hide resolved
Only delete backups' resources (BackupVolume, BackupBackingImage,
SystemBackup, and Backup) when synchronization.

Add the label `DeleteCustomResourceOnly`, and delete remote backup
data when deleting the corresponding backup resource in the cluster
and the label does not exist.

ref: longhorn/longhorn 9530

Signed-off-by: James Lu <james.lu@suse.com>
Copy link
Contributor

@ChanYiLin ChanYiLin left a comment

Choose a reason for hiding this comment

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

approved

@derekbit derekbit merged commit 8fadbfb into longhorn:master Oct 9, 2024
8 checks passed
@derekbit
Copy link
Member

derekbit commented Oct 9, 2024

@mergify backport v1.7.x v1.6.x

Copy link

mergify bot commented Oct 9, 2024

backport v1.7.x v1.6.x

✅ Backports have been created

@WebberHuang1118
Copy link
Contributor

WebberHuang1118 commented Oct 9, 2024

I just checked with the image jamesluhz/lh-manager:v1008issue9530webber01 provided by @mantissahz, the backup data will not be deleted on the remote target, but the backups will not be recovered until the next backupstore poll interval. From my code reading, it seems to be expected by design?

@mantissahz
Copy link
Contributor Author

mantissahz commented Oct 9, 2024

the backup data will not be deleted on the remote target, but the backups will not be recovered until the next backupstore poll interval. From my code reading, it seems to be expected by design?

@WebberHuang1118,
Yes, it is expected because we do not know whether the deleting with an empty response from backupstore is correct or not. We only can get the backup information back when there is correct backup information from the backupstore at synchronization (for every polling interval).

@mantissahz mantissahz deleted the issue9530 branch October 9, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants