Delete backing snapshot when searchable snapshot index is deleted #75565

tlrx · 2021-07-21T08:38:17Z

In #74977 we introduced a new index setting index.store.snapshot.delete_searchable_snapshot that can be set when mounting a snapshot as an index to inform that the snapshot should be deleted once the searchable snapshot index is deleted.

The previous pull request adds the index setting and the verifications around it. This pull request now adds the logic to detect that a searchable snapshot index with this specific logic is being deleted and triggers the deletion of the backing snapshot.

In order to do this, when a searchable snapshot index is deleted we check if the setting index.store.snapshot.delete_searchable_snapshot is set. If the index to be deleted is the last searchable snapshot index that uses the snapshot then the snapshot informations are added to the RepositoryMetadata in the cluster state in a list of "snapshots to delete". Once a snapshot is marked as "to delete" and appears in the repository metadata the snapshot cannot be cloned, mounted or restored in the cluster.

Snapshots marked as "to delete" are deleted in the background by the SnapshotsService. On cluster state updates the SnapshotsService retrieve the list of snapshots to delete and trigger the deletion by executing an explicit snapshot delete request (I tried to sneak into the snapshot state machine to directly add the snapshots as SnapshotDeletionInProgress but this requires a consistent view of the repository that is not available at the time the snapshots are marked as to delete).

Deletions of snapshots marked as "to delete" are executed per repository with some limitations to avoid hundreds of snapshots being deleted concurrently.

This PR is split into separate commits that should be reviewable on their own.

Next steps will be to make use of this setting in ILM so that it does not require a specific "delete searchable snapshot" action but instead rely on the mechanism implemented here to clean up snapshots; and finally prevent deletion of snapshots used by mounted indices (this will need BWC support).

elasticmachine · 2021-07-21T08:38:20Z

Pinging @elastic/es-distributed (Team:Distributed)

tlrx · 2021-08-02T13:25:23Z

@elasticmachine run elasticsearch-ci/docs

tlrx · 2021-08-02T13:29:14Z

@original-brownbear @DaveCTurner I know this is a big pull request and you are busy with other things but I'd appreciate your feedback once you find the time to give one :)

DaveCTurner

Thanks @tlrx, sorry it's taken a while to get around to this. I left a few comments that I hope will help to simplify it a bit.

Do you think we should hide the snapshots-to-be-deleted from APIs like get-snapshots and get-snapshot-status?

DaveCTurner · 2021-08-03T09:24:53Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDeleteIndexService.java

+                    continue; // other index is backed by a different snapshot, skip
+                }
+                final String otherRepositoryName = repositoryNameFromIndexSettings(currentState, otherSettings);
+                if (Objects.equals(repositoryName, otherRepositoryName) == false) {


I think this might break in some odd corner cases involving registering the same repository under multiple names (maybe without repository UUIDs). But do we need to check this? By this point we know the snapshot UUID matches, that should be enough to tell us not to delete it.

I agree and I removed this check in e1995d7

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDeleteIndexService.java

DaveCTurner · 2021-08-03T09:29:58Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDeleteIndexService.java

+            boolean changed = false;
+            for (Map.Entry<String, Set<SnapshotId>> snapshotToDelete : snapshotsToDelete.entrySet()) {
+                RepositoryMetadata repository = repositories.repository(snapshotToDelete.getKey());
+                if (repository != null) {


Can this be null?

I don't think it can be null and actually we don't need to re read the repository metadata here so I pushed e1995d7

DaveCTurner · 2021-08-03T09:43:38Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

+    private final Map<String, Integer> numberOfOnGoingSnapshotsToDeletes = new HashMap<>();
+
+    private volatile int maxConcurrentSnapshotsToDeletes;
+    private volatile TimeValue snapshotsToDeleteRetryInterval;


A time-based retry feels wrong to me. We retry when concurrent operations block our delete attempt, which is ok, but the blocking operations are removed by a cluster state update so I think we should make some attempt to detect conflicting operations and retry when applying a cluster state update that appears to have no conflicts rather than by using a timer.

Detecting conflicting situations and not trigger snapshot deletions sounds like a good idea, thanks. I refactored the deletion logic in 3e8dc65 to detect conflicts and do nothing but rely on subsequent cluster state updates to allow and to trigger snapshot deletions.

Still, I went back and forth with the time base retry. I agree this should not be the main path but I think that it will be useful in some situations, like the snapshot deletions failing because of expired repository or network issue when accessing the repository. I made an infinite retry with 30s delay but we could imagine some backoff policy or just a max number of retries before giving up.

DaveCTurner · 2021-08-03T09:46:40Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

+     */
+    private final Map<String, Integer> numberOfOnGoingSnapshotsToDeletes = new HashMap<>();
+
+    private volatile int maxConcurrentSnapshotsToDeletes;


We basically coalesce all snapshot deletions on a repository into a single operation (see e.g. the reusedExistingDelete flag) so I think it would be preferable, and simpler, not to have this kind of concurrency.

I agree, thanks for reminding me this. I've refactored and removed this "concurrency".

DaveCTurner · 2021-08-03T09:50:44Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

+            try {
+                logger.debug("[{}] triggering deletion of snapshot [{}]", repository, snapshotId);
+                final PlainActionFuture<Void> future = PlainActionFuture.newFuture();
+                deleteSnapshots(new DeleteSnapshotRequest(repository, snapshotId.getName()), future);


We're relying on the name of the snapshot here, but I don't think we protect against anyone deleting the snapshot manually and then creating a new one with the same name while this potentially-delayed action is in flight, so we might end up deleting the wrong snapshot here.

Now #76079 is merged I find it easier to implement and I pushed 4df53d7.

DaveCTurner · 2021-08-03T09:51:35Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

+                logger.debug("[{}] triggering deletion of snapshot [{}]", repository, snapshotId);
+                final PlainActionFuture<Void> future = PlainActionFuture.newFuture();
+                deleteSnapshots(new DeleteSnapshotRequest(repository, snapshotId.getName()), future);
+                future.actionGet();


I think we don't need to block a thread to wait for completion here, we should be able to handle the response async.

++ I reworked this.

DaveCTurner · 2021-08-03T13:56:40Z

server/src/main/java/org/elasticsearch/cluster/metadata/RepositoryMetadata.java

+    /**
+     * List of {@link org.elasticsearch.snapshots.SnapshotId} marked as "to delete"
+     */
+    private final List<SnapshotId> snapshotsToDelete;


I just realised that if we track the pending deletes it here then it'll get lost if someone does a put or delete on this repo, maybe just to adjust its settings or rename it or something. Maybe we should use a separate Metadata.Custom instead.

I thought I took care of propagating the pending deletes in case of a put on the repository but actually you're right, there's a missing bit I think (see canUpdateInPlace(newRepositoryMetadata, existing) in RepositoriesService).

Putting the pending deletes in a separate Custom also makes sense but we have to decide how to match the repository when it's re-created (by uuid?) and how long should the pending deletes be kept in cluster state. I think it would be easier to keep the pending deletes where it is today and prevent the deletion of a repository if it still have pending deletes (like we prevent repo deletion if there are ongoing operations).

++ there's no easy answer. Blocking the removal of the repository would worry me a bit, if we introduced some awful bug that made deletes fail then we could be retrying forever without a way to fix it. I guess we'd also want to prevent marking a repo as readonly for similar reasons. Maybe we'd need a ?force parameter to the delete repo API as an escape hatch just in case.

I refactored things a bit and I added some specific checks for read-only repositories so that we don't trigger snapshot deletions on such repositories. Pending deletes are still added to the repository metadata so that they'll be processed once the repository is writeable again.

Also, I did not implement the logic to prevent deleting repository (or making it read-only) when they have pending deletes. I'd like to discuss and address this as a follow up if it is ok for you.

tlrx · 2021-09-06T15:46:51Z

@DaveCTurner Thanks for your feedback and sorry for the time it took me to apply your comments to the code. I've reimplemented the deletion logic which should be better now but maybe not yet bullet proof.

Do you think we should hide the snapshots-to-be-deleted from APIs like get-snapshots and get-snapshot-status?

I'm tempted to not hide the pending snapshots deletions in these APIs so that they reflect what exist exactly in the repository. Maybe we could add a runtime flag to the response that will indicate pending deletions.

tlrx · 2021-09-14T09:37:57Z

ping @DaveCTurner - sorry for the long standing PR but if you have the time to have another look I'd be grateful.

henningandersen

I have a number of minor comments, but the main ones are around the identity of the deletion and where to store it.
I wish we could rely on repository uuids, but I suppose we cannot due to bwc?

henningandersen · 2021-10-04T12:46:24Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

+                    .custom(RepositoriesMetadata.TYPE, RepositoriesMetadata.EMPTY)
+                    .repository(repositoryName);
+                if (repositoryMetadata != null && repositoryMetadata.snapshotsToDelete().contains(sourceSnapshotId)) {
+                    throw new ConcurrentSnapshotExecutionException(


++, I wonder if we want similar protection in TransportMountSearchableSnapshotAction?

henningandersen · 2021-10-04T12:59:31Z

server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java

@@ -217,7 +217,7 @@ public ClusterState execute(ClusterState currentState) {
                                updatedMetadata = repositoryMetadata.withSettings(newRepositoryMetadata.settings());
                            } else {
                                ensureRepositoryNotInUse(currentState, request.name());
-                                updatedMetadata = newRepositoryMetadata;
+                                updatedMetadata = newRepositoryMetadata.withSnapshotsToDelete(repositoryMetadata.snapshotsToDelete());


This could update the repo-reference to point to a completely different repo. I wonder if we should not registere the repository uuid together with the snapshot to delete - and then remove the snapshots to delete if the uuid does not match when it is assigned later?
Also relates to David's comment in RepositoryMetadata, keeping the list of snapshots to delete separate from the repo-registration sort of makes sense I think.

henningandersen · 2021-10-04T13:04:55Z

...ava/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsRepositoryIntegTests.java

+        final String suffix = getTestName().toLowerCase(Locale.ROOT);
+        final String repository = "repository-" + suffix;
+        createRepository(repository, FsRepository.TYPE, randomRepositorySettings());
+
+        final String index = "index-" + suffix;
+        createAndPopulateIndex(index, Settings.builder().put(INDEX_SOFT_DELETES_SETTING.getKey(), true));
+
+        final TotalHits totalHits = internalCluster().client().prepareSearch(index).setTrackTotalHits(true).get().getHits().getTotalHits();
+
+        final String snapshot = "snapshot-" + suffix;
+        createSnapshot(repository, snapshot, List.of(index));
+        assertAcked(client().admin().indices().prepareDelete(index));
+
+        final String mounted = mountSnapshot(repository, snapshot, index, deleteSnapshotIndexSettings(true));
+        assertHitCount(client().prepareSearch(mounted).setTrackTotalHits(true).get(), totalHits.value);
+        assertAcked(client().admin().indices().prepareDelete(mounted));
+


Looks like this first part is the same across several tests, perhaps we can refactor out a setup method?

henningandersen · 2021-10-04T13:13:55Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDeleteIndexService.java

+                    return repository.name();
+                }
+            }
+        }


I wonder if we should return null in case there is a repo-uuid on the index but no such repo was found? This works differently from RepositoriesService.indexSettingsMatchRepositoryMetadata

henningandersen · 2021-10-04T13:17:30Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDeleteIndexService.java

+        if (snapshotsToDelete.isEmpty() == false) {
+            RepositoriesMetadata repositories = currentState.metadata().custom(RepositoriesMetadata.TYPE, RepositoriesMetadata.EMPTY);
+            for (Map.Entry<String, Set<SnapshotId>> snapshotToDelete : snapshotsToDelete.entrySet()) {
+                repositories = repositories.addSnapshotsToDelete(snapshotToDelete.getKey(), snapshotToDelete.getValue());


I think we risk getting an IllegalArgumentException from RepositoriesMetadata.withUpdate when the repo has been deleted while deleting the index.

henningandersen · 2021-10-04T16:45:38Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

+     */
+    private void deleteSnapshots(
+        final Function<SnapshotId, String> mapping,
+        final DeleteSnapshotRequest request,


I would prefer to let this method have the raw parameters rather than use the request, since in the uuid case it is sort of bending the original purpose of DeleteSnapshotRequest.

henningandersen · 2021-10-04T17:53:16Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

+                                ),
+                                e
+                            );
+                        } else if (e instanceof RepositoryMissingException) {


I wonder if we want to retry when/if the repo reappears? Ties in with the discussion on where the list of snapshots to delete lives.

henningandersen · 2021-10-04T17:54:26Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

+                                snapshotId
+                            );
+                        } else {
+                            logger.warn(


I am semi-torn on this. We log at warn, but retry every 30s. If we think we can recover via retries, perhaps we should be more silent (debug) about it?

henningandersen · 2021-10-04T17:56:00Z

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

+                        logger.trace("snapshot to delete [{}] is being cloned, waiting for operation to complete", snapshotId);
+                    } else if (currentDeletions.contains(snapshotId)) {
+                        logger.trace("snapshot to delete [{}] is already queued", snapshotId);
+                    } else if (onGoingSnapshotsDeletions.add(snapshotId)) {


nit: ongoingSnapshotDeletions?

henningandersen · 2021-10-04T18:02:36Z

server/src/test/java/org/elasticsearch/cluster/metadata/MetadataDeleteIndexServiceTests.java

+            assertThat(updatedRepos.repository("repo_name").snapshotsToDelete(), hasSize(1));
+            assertThat(updatedRepos.repository("repo_name").snapshotsToDelete(), hasItem(new SnapshotId("snap_name", "snap_uuid")));


Can this be:

assertThat(updatedRepos.repository("repo_name").snapshotsToDelete(), equalTo(List.of(new SnapshotId("snap_name", "snap_uuid"))));

tlrx · 2021-10-15T07:38:22Z

Thanks @DaveCTurner and @henningandersen. I've opened #79156 to see how it looks like when snapshots pending deletion are stored as dedicated custom in cluster state. With Henning's suggestion to limit the number of pending deletions to keep around I think it makes more sense and it's easier to deal with situations where the repository is gone.

elasticsearchmachine · 2022-02-02T20:49:20Z

Hi @tlrx, I've created a changelog YAML for you.

tlrx · 2022-08-03T13:05:20Z

Closing this one, a better implementation is #79156

tlrx added 5 commits July 21, 2021 09:27

add snapshots to delete to repository metadata

fdf484c

add snapshots to delete metadata

ba6b73b

prevent snapshots to delete to be mounted/restored/cloned

a446683

trigger snapshot deletions

704c8ea

add settings

955a5fc

tlrx added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 labels Jul 21, 2021

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Jul 21, 2021

tlrx added the v7.15.0 label Jul 21, 2021

tlrx requested review from original-brownbear and DaveCTurner and removed request for original-brownbear July 21, 2021 10:01

Merge branch 'master' into delete-snap-on-searchable-index-deletion

982d6ea

tlrx removed the v7.15.0 label Aug 2, 2021

Merge branch 'master' into delete-snap-on-searchable-index-deletion

4e641ab

DaveCTurner reviewed Aug 3, 2021

View reviewed changes

tlrx mentioned this pull request Aug 4, 2021

Remove and inline methods in SnapshotsService.deleteSnapshots() #76079

Merged

tlrx added 6 commits September 3, 2021 17:15

Merge branch 'master' into delete-snap-on-searchable-index-deletion

c799368

Remove other repository name check

e1995d7

Do not re read repository name

e13a11a

Delete snapshots using UUIDs

4df53d7

rework snapshot deletion

3e8dc65

Merge branch 'master' into delete-snap-on-searchable-index-deletion

e1f17d5

tlrx requested a review from DaveCTurner September 6, 2021 15:46

tlrx added 2 commits September 7, 2021 09:06

Merge branch 'master' into delete-snap-on-searchable-index-deletion

b7bcf80

Merge branch 'master' into delete-snap-on-searchable-index-deletion

0bcaf26

tlrx added 2 commits September 14, 2021 12:26

fix random int in test

83dfa73

Merge branch 'master' into delete-snap-on-searchable-index-deletion

eb0e89e

tlrx requested a review from henningandersen September 23, 2021 08:47

henningandersen reviewed Oct 4, 2021

View reviewed changes

tlrx mentioned this pull request Oct 14, 2021

Add snapshots pending deletion in cluster state to delete snapshot once index is deleted #79156

Open

arteam added v8.1.0 and removed v8.0.0 labels Jan 12, 2022

mark-vieira added v8.2.0 and removed v8.1.0 labels Feb 2, 2022

Update docs/changelog/75565.yaml

b1f36f8

salvatore-campagna added v8.3.0 and removed v8.2.0 labels Mar 30, 2022

craigtaverner added v8.4.0 and removed v8.3.0 labels May 25, 2022

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:10

mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022

tlrx closed this Aug 3, 2022

		assertThat(updatedRepos.repository("repo_name").snapshotsToDelete(), hasSize(1));
		assertThat(updatedRepos.repository("repo_name").snapshotsToDelete(), hasItem(new SnapshotId("snap_name", "snap_uuid")));

Delete backing snapshot when searchable snapshot index is deleted #75565

Delete backing snapshot when searchable snapshot index is deleted #75565

Conversation

tlrx commented Jul 21, 2021 • edited Loading

elasticmachine commented Jul 21, 2021

tlrx commented Aug 2, 2021

tlrx commented Aug 2, 2021

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Sep 6, 2021

tlrx commented Sep 14, 2021

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Oct 15, 2021

elasticsearchmachine commented Feb 2, 2022

tlrx commented Aug 3, 2022

tlrx commented Jul 21, 2021 •

edited

Loading

tlrx Sep 6, 2021 •

edited

Loading

tlrx Aug 3, 2021 •

edited

Loading