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

Provide option to cleanup temporary objects (PVCs) made by Restore Process #1158

Open
reefland opened this issue Mar 7, 2024 · 4 comments
Open
Labels
enhancement New feature or request

Comments

@reefland
Copy link

reefland commented Mar 7, 2024

Describe the feature you'd like to have.
An option added to ReplicationDestination to request volsync to clean-up temporary PVCs created during the restore process which are no longer needed when the restore has completed.

For example:

kind: ReplicationDestination
metadata:
  name: apt-cacher-ng-dst
spec:
  trigger:
    manual: restore-once

Leaves behind 2 snapshots when the restore is completed:

volsync-apt-cacher-ng-dst-cache   Bound    pvc-a2f70adb-2acf-499a-a0d1-707bb55f1e4b   1Gi        RWO            csi-ceph-block   2d18h
volsync-apt-cacher-ng-dst-dest    Bound    pvc-50b3140b-200a-4a26-b093-3cfba92a9313   50Gi       RWO            csi-ceph-block   2d18h

What is the value to the end user? (why is it a priority?)

While the docs state After the restore completes, the ReplicationDestination object can be deleted. It's not clear there can be multiple objects to cleanup. Why not allow the cleanup process to be automated by volsync instead of the user needing to remember to do this manually?

How will we know we have a good solution? (acceptance criteria)
Upon completion of a successful restore, if configured to do so, volsync will cleanup temporary objects such as PVCs no longer needed.

Additional context

My preference is this should be enabled by default unless you have a reason to keep the objects. However, I understand this could be considered a breaking change if users expect/want these objects to be left behind. I'm fine if disabled by default, just need to the option to enable it.

@reefland reefland added the enhancement New feature or request label Mar 7, 2024
@tesshuflower
Copy link
Contributor

I think the main hesitation about implementing this feature would be that we'd need to specifically document not to use the flag in most situations. Many use-cases benefit from these PVCs not being cleaned up as it means you don't need to re-download the entire data on each replication cycle (and this is crucial for things like rsync). When users are done the expectation is that they can remove their replicationdestination if syncs are no longer required, and at that point the pvcs would be cleaned up as well.

@onedr0p
Copy link
Contributor

onedr0p commented Mar 7, 2024

Oh I was unaware removing the replicationdestination would clean up the PVCs, makes sense though. However since I have that declarative in my Flux / GitOps configuration it's not great to remove it via kubectl delete... because Flux will just put it back. I have it this way so I don't need to do any comment/uncomment commit dancing when re-provisioning my cluster.

@reefland
Copy link
Author

reefland commented Mar 7, 2024

hmm. I like having the ReplicationDestination defined within gitops for bootstrapping the application PVC with backed up data. I want to leave this in-place and just have PVCs cleaned up. :)

If this is something specific to Restic restore, and doesn't make sense elsewhere, then can we only apply to ReplicationDestination .spec.restric and not .spec.rsync ?

@reefland
Copy link
Author

reefland commented Mar 9, 2024

Seems related to #1129

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

3 participants