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

Allow backups to be made incrementally #114

Merged
merged 8 commits into from
Aug 12, 2024

Conversation

stijzermans
Copy link
Contributor

This implements the incremental backup functionality as described in the OpenStack docs

Added some unit tests for block storage backup creation and the different incremental backup scenario's.

@stijzermans stijzermans force-pushed the enh/allow-incremental-backups branch from 9712505 to 32fa9ac Compare August 2, 2024 12:20
@Lirt
Copy link
Owner

Lirt commented Aug 4, 2024

Hello @stijzermans,

Thank you for your contribution. Let me know when the PR is finished and I will check it. From my first glance the code looks good.

@stijzermans
Copy link
Contributor Author

@Lirt from my perspective should be good to go! Only thing to be aware of, which is that deletion of backups which have a dependant backup is not possible, so always need to delete the latest first instead of the last, but not something that we (imo) can or should manage from Velero perspective.

@Lirt
Copy link
Owner

Lirt commented Aug 5, 2024

Only thing to be aware of, which is that deletion of backups which have a dependant backup is not possible, so always need to delete the latest first instead of the last, but not something that we (imo) can or should manage from Velero perspective.

Yes, that is understandable.

I think it will have one special consequence. When you create a backup from schedule and configure TTL, Velero will try to remove the backup from oldest, eventually coming to newest. I think enabling cleanup (TTL) for incremental backup overall doesn't make a sense.

Also I think there is no way to know whether Velero is asking the plugin to delete backup because of TTL (if you see such, please let me know). So this should be something that we need to document -> incremental backups must not have TTL configured!.

Have you also tried to restore from incremental backup? I guess there you will get the restored data based on which backup you asked for restore. So to include newest data, you need to restore from latest, but if you need older version of data, to use older incremental backup as source. That most likely shouldn't require code change.

Then if user plans to create new backup (maybe because he/she has 1000 of incremental backups and would like to start fresh), he/she can create new backup and let the schedule create incrementals. Then after some time the user can remove the old backup completely (by deleting in reverse order with velero CLI and some shell script for example).

Let me know what do you think about those points.

@stijzermans
Copy link
Contributor Author

Regarding restoration functionality: Did some extra testing for restoring and this works completely as expected. Given three backups

  • Backup A (full): file A,B,C
  • Backup B (incremental): file +D, +E
  • Backup C (incremental): file +F

Restoring backup B restores a volume with A,B,C,D,E present, and does not include F.

Regarding backup deletion: Will add a comment that some manual scripting / CLI usage is required to perform a cleanup.

Regarding TTL: I for now add a suggestion to add a very large value for this TTL. Will add an issue to Velero to allow an infinite / never expiration behaviour, cause there are scenarios that expiration should never be enabled. To my knowledge and research there's indeed no data available in the DeleteSnapshot method which indicates what the reason for deletion is...

Do you agree with the given solution and is the explanation given in the docs clear?

@Lirt
Copy link
Owner

Lirt commented Aug 8, 2024

Yes! That all sounds very good. Thank you for caring about the details.

@stijzermans
Copy link
Contributor Author

@Lirt made the following issue vmware-tanzu/velero#8099.

Looking forward to your review 😄.

@Lirt
Copy link
Owner

Lirt commented Aug 12, 2024

Everything looks good to me now. Thank you for a good work and also handling the management tasks.

Is it OK to merge and release this feature in v0.8.0?

@stijzermans
Copy link
Contributor Author

Sounds great @Lirt! Thank you as well.

@Lirt Lirt merged commit 6701b13 into Lirt:master Aug 12, 2024
3 checks passed
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.

2 participants