Introduce searchable snapshots index setting for cascade deletion of snapshots #74977
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,20 +9,27 @@ | |
|
|
||
| import org.apache.lucene.search.TotalHits; | ||
| import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; | ||
| import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse; | ||
| import org.elasticsearch.cluster.metadata.IndexMetadata; | ||
| import org.elasticsearch.common.settings.Settings; | ||
| import org.elasticsearch.common.unit.ByteSizeValue; | ||
| import org.elasticsearch.repositories.fs.FsRepository; | ||
| import org.elasticsearch.snapshots.SnapshotRestoreException; | ||
| import org.hamcrest.Matcher; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
|
|
||
| import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING; | ||
| import static org.elasticsearch.index.IndexSettings.INDEX_SOFT_DELETES_SETTING; | ||
| import static org.elasticsearch.repositories.RepositoriesService.SEARCHABLE_SNAPSHOTS_DELETE_SNAPSHOT_ON_INDEX_DELETION; | ||
| import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; | ||
| import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; | ||
| import static org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotRequest.Storage; | ||
| import static org.hamcrest.Matchers.allOf; | ||
| import static org.hamcrest.Matchers.containsString; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
|
|
||
| public class SearchableSnapshotsRepositoryIntegTests extends BaseFrozenSearchableSnapshotsIntegTestCase { | ||
|
|
||
|
|
@@ -110,4 +117,153 @@ public void testRepositoryUsedBySearchableSnapshotCanBeUpdatedButNotUnregistered | |
|
|
||
| assertAcked(clusterAdmin().prepareDeleteRepository(updatedRepositoryName)); | ||
| } | ||
|
|
||
| public void testMountIndexWithDeletionOfSnapshotFailsIfNotSingleIndexSnapshot() throws Exception { | ||
|
There was a problem hiding this comment. Can we add a test that demonstrates that restoring a searchable snapshot cannot be done if its There was a problem hiding this comment. Sure, I pushed more tests in ff0f834 |
||
| final String repository = "repository-" + getTestName().toLowerCase(Locale.ROOT); | ||
| createRepository(repository, FsRepository.TYPE, randomRepositorySettings()); | ||
|
|
||
| final int nbIndices = randomIntBetween(2, 5); | ||
| for (int i = 0; i < nbIndices; i++) { | ||
| createAndPopulateIndex( | ||
| "index-" + i, | ||
| Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0).put(INDEX_SOFT_DELETES_SETTING.getKey(), true) | ||
| ); | ||
| } | ||
|
|
||
| final String snapshot = "snapshot"; | ||
| createFullSnapshot(repository, snapshot); | ||
| assertAcked(client().admin().indices().prepareDelete("index-*")); | ||
|
|
||
| final String index = "index-" + randomInt(nbIndices - 1); | ||
| final String mountedIndex = "mounted-" + index; | ||
|
|
||
| final SnapshotRestoreException exception = expectThrows( | ||
| SnapshotRestoreException.class, | ||
| () -> mountSnapshot(repository, snapshot, index, mountedIndex, deleteSnapshotIndexSettings(true), randomFrom(Storage.values())) | ||
| ); | ||
| assertThat( | ||
| exception.getMessage(), | ||
| allOf( | ||
| containsString("cannot mount snapshot [" + repository + '/'), | ||
| containsString(snapshot + "] as index [" + mountedIndex + "] with the deletion of snapshot on index removal enabled"), | ||
| containsString("[index.store.snapshot.delete_searchable_snapshot: true]; "), | ||
| containsString("snapshot contains [" + nbIndices + "] indices instead of 1.") | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| public void testMountIndexWithDeletionOfSnapshot() throws Exception { | ||
| final String repository = "repository-" + getTestName().toLowerCase(Locale.ROOT); | ||
| createRepository(repository, FsRepository.TYPE, randomRepositorySettings()); | ||
|
|
||
| final String index = "index"; | ||
| 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"; | ||
| createSnapshot(repository, snapshot, List.of(index)); | ||
| assertAcked(client().admin().indices().prepareDelete(index)); | ||
|
|
||
| String mounted = "mounted-with-setting-enabled"; | ||
| mountSnapshot(repository, snapshot, index, mounted, deleteSnapshotIndexSettings(true), randomFrom(Storage.values())); | ||
| assertIndexSetting(mounted, SEARCHABLE_SNAPSHOTS_DELETE_SNAPSHOT_ON_INDEX_DELETION, equalTo("true")); | ||
| assertHitCount(client().prepareSearch(mounted).setTrackTotalHits(true).get(), totalHits.value); | ||
|
|
||
| // the snapshot is already mounted as an index with "index.store.snapshot.delete_searchable_snapshot: true", | ||
| // any attempt to mount the snapshot again should fail | ||
| final String mountedAgain = randomValueOtherThan(mounted, () -> randomAlphaOfLength(10).toLowerCase(Locale.ROOT)); | ||
| SnapshotRestoreException exception = expectThrows( | ||
| SnapshotRestoreException.class, | ||
| () -> mountSnapshot(repository, snapshot, index, mountedAgain, deleteSnapshotIndexSettings(randomBoolean())) | ||
| ); | ||
| assertThat( | ||
| exception.getMessage(), | ||
| allOf( | ||
| containsString("cannot mount snapshot [" + repository + '/'), | ||
| containsString(':' + snapshot + "] as index [" + mountedAgain + "]; another index [" + mounted + '/'), | ||
| containsString("] uses the snapshot with the deletion of snapshot on index removal enabled "), | ||
| containsString("[index.store.snapshot.delete_searchable_snapshot: true].") | ||
| ) | ||
| ); | ||
|
|
||
| assertAcked(client().admin().indices().prepareDelete(mounted)); | ||
| mounted = "mounted-with-setting-disabled"; | ||
| mountSnapshot(repository, snapshot, index, mounted, deleteSnapshotIndexSettings(false), randomFrom(Storage.values())); | ||
| assertIndexSetting(mounted, SEARCHABLE_SNAPSHOTS_DELETE_SNAPSHOT_ON_INDEX_DELETION, equalTo("false")); | ||
| assertHitCount(client().prepareSearch(mounted).setTrackTotalHits(true).get(), totalHits.value); | ||
|
|
||
| // the snapshot is now mounted as an index with "index.store.snapshot.delete_searchable_snapshot: false", | ||
| // any attempt to mount the snapshot again with "delete_searchable_snapshot: true" should fail | ||
| exception = expectThrows( | ||
| SnapshotRestoreException.class, | ||
| () -> mountSnapshot(repository, snapshot, index, mountedAgain, deleteSnapshotIndexSettings(true)) | ||
| ); | ||
| assertThat( | ||
| exception.getMessage(), | ||
| allOf( | ||
| containsString("cannot mount snapshot [" + repository + '/'), | ||
| containsString(snapshot + "] as index [" + mountedAgain + "] with the deletion of snapshot on index removal enabled"), | ||
| containsString("[index.store.snapshot.delete_searchable_snapshot: true]; another index [" + mounted + '/'), | ||
| containsString("] uses the snapshot.") | ||
| ) | ||
| ); | ||
|
|
||
| // but we can continue to mount the snapshot, as long as it does not require the cascade deletion of the snapshot | ||
| mountSnapshot(repository, snapshot, index, mountedAgain, deleteSnapshotIndexSettings(false)); | ||
| assertIndexSetting(mountedAgain, SEARCHABLE_SNAPSHOTS_DELETE_SNAPSHOT_ON_INDEX_DELETION, equalTo("false")); | ||
| assertHitCount(client().prepareSearch(mountedAgain).setTrackTotalHits(true).get(), totalHits.value); | ||
|
|
||
| assertAcked(client().admin().indices().prepareDelete(mountedAgain)); | ||
| assertAcked(client().admin().indices().prepareDelete(mounted)); | ||
| } | ||
|
|
||
| public void testDeletionOfSnapshotSettingCannotBeUpdated() throws Exception { | ||
| final String repository = "repository-" + getTestName().toLowerCase(Locale.ROOT); | ||
| createRepository(repository, FsRepository.TYPE, randomRepositorySettings()); | ||
|
|
||
| final String index = "index"; | ||
| 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"; | ||
| createSnapshot(repository, snapshot, List.of(index)); | ||
| assertAcked(client().admin().indices().prepareDelete(index)); | ||
|
|
||
| final String mounted = "mounted-" + index; | ||
| final boolean deleteSnapshot = randomBoolean(); | ||
|
|
||
| mountSnapshot(repository, snapshot, index, mounted, deleteSnapshotIndexSettings(deleteSnapshot), randomFrom(Storage.values())); | ||
| assertIndexSetting(mounted, SEARCHABLE_SNAPSHOTS_DELETE_SNAPSHOT_ON_INDEX_DELETION, equalTo(Boolean.toString(deleteSnapshot))); | ||
| assertHitCount(client().prepareSearch(mounted).setTrackTotalHits(true).get(), totalHits.value); | ||
|
|
||
| final IllegalArgumentException exception = expectThrows( | ||
| IllegalArgumentException.class, | ||
| () -> client().admin() | ||
| .indices() | ||
| .prepareUpdateSettings(mounted) | ||
| .setSettings(deleteSnapshotIndexSettings(deleteSnapshot == false)) | ||
| .get() | ||
| ); | ||
| assertThat( | ||
| exception.getMessage(), | ||
| containsString("can not update private setting [index.store.snapshot.delete_searchable_snapshot]; ") | ||
| ); | ||
|
|
||
| assertAcked(client().admin().indices().prepareDelete(mounted)); | ||
| } | ||
|
|
||
| private static Settings deleteSnapshotIndexSettings(boolean value) { | ||
| return Settings.builder().put(SEARCHABLE_SNAPSHOTS_DELETE_SNAPSHOT_ON_INDEX_DELETION, value).build(); | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment.
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+. |
||
| } | ||
|
|
||
| private static void assertIndexSetting(String indexName, String indexSettingName, Matcher<String> matcher) { | ||
| final GetSettingsResponse getSettingsResponse = client().admin().indices().prepareGetSettings(indexName).get(); | ||
| assertThat( | ||
| "Unexpected value for setting [" + indexSettingName + "] of index [" + indexName + ']', | ||
| getSettingsResponse.getSetting(indexName, indexSettingName), | ||
| matcher | ||
| ); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
truefor all mounts as soon as there is one mount withtrue. 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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposal of cloning before frozen has an issue related to snapshots of searchable snapshots. An example:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed 76480bb to allow snapshots to be mounted multiple times but only with the same value for the new setting.