search

Lirt / velero-plugin-for-openstack

Openstack Cinder, Manila and Swift plugin for Velero backups
MIT License
27 stars 16 forks source link

Allow backups to be made incrementally #114

Closed stijzermans closed 3 months ago

stijzermans commented 4 months ago

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.

Lirt commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 3 months ago

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

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 commented 3 months ago

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

stijzermans commented 3 months ago

@Lirt made the following issue https://github.com/vmware-tanzu/velero/issues/8099.

Looking forward to your review 😄.

Lirt commented 3 months ago

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 commented 3 months ago

Sounds great @Lirt! Thank you as well.