Introduce searchable snapshots index setting for cascade deletion of snapshots #74977

tlrx · 2021-07-06T13:23:15Z

Today there is no relationship between the lifecycle of a snapshot mounted as an index and the lifecycle of index itself. This lack of relationship makes it possible to delete a snapshot in a repository while the mounted index still exists, causing the shards to fail in the future.

On the other hand creating a snapshot that contains a single index to later mount it as a searchable snapshot index becomes more and more natural for users and ILM; but it comes with the risk to forget to delete the snapshot when the searchable snapshot index is deleted from the cluster, "leaking" snapshots in repositories.

We'd like to improve the situation and provide a way to automatically delete the snapshot when the mounted index is deleted. To opt in for this behaviour, a user has to enable a specific index setting when mounting the snapshot (the proposed name for the setting is index.store.snapshot.delete_searchable_snapshot). Elasticsearch then verifies that the snapshot to mount contains only 1 snapshotted index and that the snapshot is not used by another mounted index with a different value for the opt in setting, and mounts the snapshot with the new private index setting. This is the part implemented in this pull request.

In follow-up pull requests this index setting will be used when the last mounted index is deleted and removed from the cluster state in order to add the searchable snapshot id to a list of snapshots to delete in the repository metadata. Snapshots that are marked as "to delete" will not be able to be restored or cloned, and Elasticsearch will take care of deleting the snapshot as soon as possible. Then ILM will be changed to use this setting when mounting a snapshot as a cold or frozen index and delete_searchable_snapshot option in ILM will be removed. Finally, deleting a snapshot that is still used by mounted indices will be prevented.

Note to reviewers: in order to not break ILM and make this change reviewable I decided to split this into multiple PRs and to describe the path here. Let me know if you prefer a single big PR instead (that would look like this). Alternatively we can go progressively here too.

…snapshots

elasticmachine · 2021-07-06T13:23:18Z

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

original-brownbear

Thanks @tlrx I have some questions on the end goal in terms of behavior here :) I'm fine reviewing in chunks or all in one, whatever works for you. A 1k diff PR is fine too if chunking this into pieces adds overhead for you.

original-brownbear · 2021-07-07T09:04:39Z

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

+                        snapshotInfo.snapshotId().getName(),
+                        String.format(
+                            Locale.ROOT,
+                            "cannot mount snapshot [%s/%s:%s] as index [%s] with the deletion of snapshot on index removal enabled "


I find this quite confusing. If we only have a single index in the snapshot and already have it mounted, wouldn't it make sense to forbid mounting it yet again regardless of settings? It seems like this situation is just wasting resources and you could just as well add an alias if you wanted to use that same snapshot via a different name?
Also, looking at this from a different perspective where we don't change anything in the above, wouldn't it make sense to simply force the setting to true for all mounts as soon as there is one mount with true. Then on index delete we can run the deletion only if there's no more mounts of the snapshot around (so only deleting the last mount deletes the snapshot) instead?

I find this quite confusing. If we only have a single index in the snapshot and already have it mounted, wouldn't it make sense to forbid mounting it yet again regardless of settings? It seems like this situation is just wasting resources and you could just as well add an alias if you wanted to use that same snapshot via a different name?

I didn't want to break the current behaviour but I guess that's something we can do if needed. Allowing to mount the same snapshot make it easier to transition an index from fully mounted to partially mounted. If we were to forbid the same single index snapshot to be mounted multiple times then it requires to clone the snapshot to transition from fully to partially, which is what I'd like ILM to do but I don't have a strong opinion for non-ILM use cases.

Then on index delete we can run the deletion only if there's no more mounts of the snapshot around (so only deleting the last mount deletes the snapshot) instead?

I had this idea as it would avoid ILM to clone the snapshot when moving to frozen, but I finally found it easier to reason about when only 1 index is linked to 1 snapshot. Again, no strong opinion so I'd like to hear what others think.

The proposal of cloning before frozen has an issue related to snapshots of searchable snapshots. An example:

Index I1 is mounted on cold tier using snapshot S1.

ILM clones S1 into S2 and partially mounts I2 using S2

ILM does alias swap and delete index I1, which also deletes S1.

Assume that shortly after, a disaster happens and there is a need to restore the cluster from snapshot. The last snapshot was taken before the steps above were taken. That snapshot will still contain index I1 mounted using snapshot S1, which no longer exists. Manual intervention will be necessary to fix this. It is much worse than at the end of the ILM lifecycle, since the data is supposed to still exist in the transition cold to frozen, whereas after the delete phase, the expectation is that the data is gone.

If we instead allow multiple mounts of the same single index snapshot, with the requirement that they must all have the flag set (or not), the transition from cold to frozen will not have this issue with snapshots of searchable snapshots. I am therefore in favor of going that route.

Thanks @henningandersen for finding the right argument 👍 I am going to revisit the code here to allow multiple mounts; that will also simplify ILM changes.

I pushed 76480bb to allow snapshots to be mounted multiple times but only with the same value for the new setting.

…on-index-deletion

tlrx · 2021-07-08T15:02:27Z

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

henningandersen

LGTM.

henningandersen · 2021-07-08T15:18:26Z

...snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshots.java

+        false,
+        Setting.Property.IndexScope,
+        Setting.Property.PrivateIndex,
+        Setting.Property.NotCopyableOnResize


Can we shrink/split/clone searchable snapshots and what is the result (a regular index or a searchable snapshot)? Not really a comment to this PR, just a question 🙂

I expect shrink & split actions to fail because if the number of shards is changed it won't match the number of shards in the snapshot. I'll take a look at this separately, we should prevent this I think. Clone should work but I'll verify this too.

I opened #75227 to prevent resize actions on searchable snapshot indices.

Cloning is supported since #56595

henningandersen · 2021-07-08T15:19:07Z

...snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshots.java

+    public static final Setting<Boolean> DELETE_SEARCHABLE_SNAPSHOT_ON_INDEX_DELETION = Setting.boolSetting(
+        SEARCHABLE_SNAPSHOTS_DELETE_SNAPSHOT_ON_INDEX_DELETION,
+        false,
+        Setting.Property.IndexScope,


Should we add "Final" too, in case the searchable snapshot is closed? Mostly a nit because it is private anyway.

I added Final too but I also implemented a check that prevents to change the value of this setting when restoring a searchable snapshots index (as Final does not prevent changes at restore time)

henningandersen · 2021-07-08T15:22:02Z

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

+    }
+
+    private static Settings deleteSnapshotIndexSettings(boolean value) {
+        return Settings.builder().put(SEARCHABLE_SNAPSHOTS_DELETE_SNAPSHOT_ON_INDEX_DELETION, value).build();


Is having to use this setting in the API an interim state, with a follow-up PR adding a direct flag in the API?

Otherwise, I think the setting should not be private if the intention is that external users should set this flag using the mount call.

Is having to use this setting in the API an interim state, with a follow-up PR adding a direct flag in the API?

I planned to use the setting in the Mount API and not introducing a flag in the API.

I still think it can be private - in a follow up I'd like to always set this setting on indices mounted in 8.0+.

henningandersen · 2021-07-08T15:24:24Z

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

@@ -110,4 +117,130 @@ public void testRepositoryUsedBySearchableSnapshotCanBeUpdatedButNotUnregistered

        assertAcked(clusterAdmin().prepareDeleteRepository(updatedRepositoryName));
    }
+
+    public void testMountIndexWithDeletionOfSnapshotFailsIfNotSingleIndexSnapshot() throws Exception {


Can we add a test that demonstrates that restoring a searchable snapshot cannot be done if its delete_searchable_snapshot flag is in conflict with existing searchable snapshots?

Sure, I pushed more tests in ff0f834

…on-index-deletion

tlrx · 2021-07-12T12:11:20Z

@original-brownbear would you mind having a look at this change when you have time? Thanks a lot

…on-index-deletion

Today if we try to shrink or to split a searchable snapshot index using the Resize API a new index will be created but can't be assigned, and even if it was assigned it won't work as the number of shards can't be changed and must always match the number of shards from the snapshot. This commit adds some verification to prevent a snapshot backed indices to be resized and if an attempt is made, throw a better error message. Note that cloning is supported since #56595 and in this change we make sure that it is only used to convert the searchable snapshot index back to a regular index. Relates #74977 (comment)

Today if we try to shrink or to split a searchable snapshot index using the Resize API a new index will be created but can't be assigned, and even if it was assigned it won't work as the number of shards can't be changed and must always match the number of shards from the snapshot. This commit adds some verification to prevent a snapshot backed indices to be resized and if an attempt is made, throw a better error message. Note that cloning is supported since elastic#56595 and in this change we make sure that it is only used to convert the searchable snapshot index back to a regular index. Relates elastic#74977 (comment)

original-brownbear

LGTM, thanks Tanguy + sorry again for the delay here!

) Today if we try to shrink or to split a searchable snapshot index using the Resize API a new index will be created but can't be assigned, and even if it was assigned it won't work as the number of shards can't be changed and must always match the number of shards from the snapshot. This commit adds some verification to prevent a snapshot backed indices to be resized and if an attempt is made, throw a better error message. Note that cloning is supported since #56595 and in this change we make sure that it is only used to convert the searchable snapshot index back to a regular index. Relates #74977 (comment)

…on-index-deletion

tlrx · 2021-07-15T10:21:06Z

Thanks Henning and Armin!

…snapshots (elastic#74977) Today there is no relationship between the lifecycle of a snapshot mounted as an index and the lifecycle of index itself. This lack of relationship makes it possible to delete a snapshot in a repository while the mounted index still exists, causing the shards to fail in the future. On the other hand creating a snapshot that contains a single index to later mount it as a searchable snapshot index becomes more and more natural for users and ILM; but it comes with the risk to forget to delete the snapshot when the searchable snapshot index is deleted from the cluster, "leaking" snapshots in repositories. We'd like to improve the situation and provide a way to automatically delete the snapshot when the mounted index is deleted. To opt in for this behaviour, a user has to enable a specific index setting when mounting the snapshot (the proposed name for the setting is index.store.snapshot.delete_searchable_snapshot). Elasticsearch then verifies that the snapshot to mount contains only 1 snapshotted index and that the snapshot is not used by another mounted index with a different value for the opt in setting, and mounts the snapshot with the new private index setting. This is the part implemented in this commit. In follow-up pull requests this index setting will be used when the last mounted index is deleted and removed from the cluster state in order to add the searchable snapshot id to a list of snapshots to delete in the repository metadata. Snapshots that are marked as "to delete" will not be able to be restored or cloned, and Elasticsearch will take care of deleting the snapshot as soon as possible. Then ILM will be changed to use this setting when mounting a snapshot as a cold or frozen index and delete_searchable_snapshot option in ILM will be removed. Finally, deleting a snapshot that is still used by mounted indices will be prevented.

Introduce searchable snapshots index setting for cascade deletion of …

6621511

…snapshots

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

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

tlrx requested review from original-brownbear and henningandersen July 6, 2021 14:04

original-brownbear reviewed Jul 7, 2021

View reviewed changes

tlrx added 2 commits July 8, 2021 14:56

Merge branch 'master' into add-setting-to-delete-searchable-snapshot-…

dcd6bd8

…on-index-deletion

review

76480bb

original-brownbear self-requested a review July 8, 2021 14:07

henningandersen approved these changes Jul 8, 2021

View reviewed changes

tlrx added 6 commits July 9, 2021 10:27

Merge branch 'master' into add-setting-to-delete-searchable-snapshot-…

58832ba

…on-index-deletion

also use null value setting

80d638c

make final

6d73227

feedback

ff0f834

Merge branch 'master' into add-setting-to-delete-searchable-snapshot-…

6e9336a

…on-index-deletion

Merge branch 'master' into add-setting-to-delete-searchable-snapshot-…

1e62dfc

…on-index-deletion

tlrx mentioned this pull request Jul 12, 2021

Prevent searchable snapshots indices to be shrunk/split #75227

Merged

Merge branch 'master' into add-setting-to-delete-searchable-snapshot-…

c5168e1

…on-index-deletion

tlrx mentioned this pull request Jul 13, 2021

[7.x] Prevent searchable snapshots indices to be shrunk/split #75289

Merged

original-brownbear approved these changes Jul 13, 2021

View reviewed changes

tlrx added 2 commits July 15, 2021 11:16

Merge branch 'master' into add-setting-to-delete-searchable-snapshot-…

22d9c4e

…on-index-deletion

merge master

8f1d1d4

tlrx merged commit 935a388 into elastic:master Jul 15, 2021

tlrx deleted the add-setting-to-delete-searchable-snapshot-on-index-deletion branch July 15, 2021 10:22

tlrx mentioned this pull request Jul 21, 2021

Delete backing snapshot when searchable snapshot index is deleted #75565

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

tlrx mentioned this pull request Oct 14, 2021

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

Open

joegallo mentioned this pull request Mar 31, 2022

Prevent deletion of snapshots that are used by snapshot backed indices #73821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce searchable snapshots index setting for cascade deletion of snapshots #74977

Introduce searchable snapshots index setting for cascade deletion of snapshots #74977

tlrx commented Jul 6, 2021 •

edited

Loading

elasticmachine commented Jul 6, 2021

original-brownbear left a comment

original-brownbear Jul 7, 2021

tlrx Jul 7, 2021

henningandersen Jul 8, 2021

tlrx Jul 8, 2021

tlrx Jul 8, 2021

tlrx commented Jul 8, 2021

henningandersen left a comment

henningandersen Jul 8, 2021

tlrx Jul 12, 2021

tlrx Jul 12, 2021

tlrx Jul 12, 2021

henningandersen Jul 8, 2021

tlrx Jul 12, 2021

henningandersen Jul 8, 2021

tlrx Jul 12, 2021

henningandersen Jul 8, 2021

tlrx Jul 12, 2021

tlrx commented Jul 12, 2021

original-brownbear left a comment

tlrx commented Jul 15, 2021

Introduce searchable snapshots index setting for cascade deletion of snapshots #74977

Introduce searchable snapshots index setting for cascade deletion of snapshots #74977

Conversation

tlrx commented Jul 6, 2021 • edited Loading

elasticmachine commented Jul 6, 2021

original-brownbear 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

tlrx commented Jul 8, 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 Jul 12, 2021

original-brownbear left a comment

Choose a reason for hiding this comment

tlrx commented Jul 15, 2021

tlrx commented Jul 6, 2021 •

edited

Loading