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 a snapshot to be deleted if a restore for a different snapshot is in progress #41463

Closed
bpiper opened this issue Apr 23, 2019 · 3 comments · Fixed by #51608
Closed

Allow a snapshot to be deleted if a restore for a different snapshot is in progress #41463

bpiper opened this issue Apr 23, 2019 · 3 comments · Fixed by #51608
Assignees
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement high hanging fruit

Comments

@bpiper
Copy link

bpiper commented Apr 23, 2019

Currently, according to SnapshotsService.deleteSnapshot, a snapshot deletion request is rejected if there is any restore in progress. The reason given is commented thusly:

// don't allow snapshot deletions while a restore is taking place,
// otherwise we could end up deleting a snapshot that is being restored
// and the files the restore depends on would all be gone

This makes perfect sense, however it doesn't necessarily explain why I can't delete snapshot A while snapshot B is being restored (which currently is forbidden). Would it not be feasible to just check if the snapshot we want to delete is among those snapshots with a restore in progress and allow the deletion if it is not, or is there more to it than the comment suggests?

In my use case, I have a 1-to-1 relationship between indices and snapshots, and being able to restore multiple indices in parallel now is great, but as I also need to be able to delete snapshots after I've restored them (for housekeeping purposes), or indeed just delete them along with the index if I no longer need it, not being able to do so while potentially numerous other indices are being restored (something that is driven by users, not me) is a bit of a drag.

@astefan astefan added the :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label Apr 24, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@DaveCTurner
Copy link
Contributor

@bpiper to clarify the comment a little, there is a many-to-many relationship between snapshots and the actual objects in a repository. Each snapshot contains many objects (maybe spread across many indices) and each object may belong to more than one snapshot. This means that it's not so simple to determine whether a restore conflicts with a concurrent deletion or not.

Relates the conversation surrounding #27353 (comment), particularly the observation that there remain significant obstacles to doing this correctly.

@bpiper
Copy link
Author

bpiper commented Apr 25, 2019

Thanks for clarifying @DaveCTurner. I had suspected that might be the case, and I appreciate this is probably stretching the snapshot/restore system a bit beyond its original intent. I think I can live with snapshot deletion failures for now, though if this fruit ever moves down a few branches, it would definitely be great to have a bit more flexibility.

@original-brownbear original-brownbear self-assigned this Nov 25, 2019
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jan 29, 2020
There is no reason not to allow deletes in parallel to restores
if they're dealing with different snapshots.
A delete will not remove any files related to the snapshot that
is being restored if it is different from the deleted snapshot
because those files will still be referenced by the restoring
snapshot.
Loading RepositoryData concurrently to modifying it is concurrency
safe nowadays as well since the repo generation is tracked in the
cluster state.

Closes elastic#41463
original-brownbear added a commit that referenced this issue Jan 30, 2020
There is no reason not to allow deletes in parallel to restores
if they're dealing with different snapshots.
A delete will not remove any files related to the snapshot that
is being restored if it is different from the deleted snapshot
because those files will still be referenced by the restoring
snapshot.
Loading RepositoryData concurrently to modifying it is concurrency
safe nowadays as well since the repo generation is tracked in the
cluster state.

Closes #41463
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jan 30, 2020
There is no reason not to allow deletes in parallel to restores
if they're dealing with different snapshots.
A delete will not remove any files related to the snapshot that
is being restored if it is different from the deleted snapshot
because those files will still be referenced by the restoring
snapshot.
Loading RepositoryData concurrently to modifying it is concurrency
safe nowadays as well since the repo generation is tracked in the
cluster state.

Closes elastic#41463
original-brownbear added a commit that referenced this issue Jan 30, 2020
There is no reason not to allow deletes in parallel to restores
if they're dealing with different snapshots.
A delete will not remove any files related to the snapshot that
is being restored if it is different from the deleted snapshot
because those files will still be referenced by the restoring
snapshot.
Loading RepositoryData concurrently to modifying it is concurrency
safe nowadays as well since the repo generation is tracked in the
cluster state.

Closes #41463
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement high hanging fruit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants