From 5d30a569ab9bd4e23064c2d9ba0f7d540c50f1d7 Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Fri, 4 Nov 2022 11:39:48 +0530 Subject: [PATCH 01/14] Prevent deletion of snapshots that are backing searchable snapshot indexes Signed-off-by: Vishal Sarda --- .../snapshots/SnapshotDeletionException.java | 23 +++++++++++++ .../snapshots/SnapshotsService.java | 33 ++++++++++++++++++- 2 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java b/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java new file mode 100644 index 0000000000000..e0c54a7cf5fd3 --- /dev/null +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java @@ -0,0 +1,23 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.snapshots; + +import org.opensearch.rest.RestStatus; + +public class SnapshotDeletionException extends SnapshotException { + + public SnapshotDeletionException(final String repositoryName, final String snapshotName, final String msg) { + super(repositoryName, snapshotName, msg); + } + + @Override + public RestStatus status() { + return RestStatus.FORBIDDEN; + } +} diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java index a4cd2fbe97bd5..ca8df731d1df4 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java @@ -38,6 +38,7 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; import org.opensearch.ExceptionsHelper; +import org.opensearch.OpenSearchStatusException; import org.opensearch.Version; import org.opensearch.action.ActionListener; import org.opensearch.action.ActionRunnable; @@ -89,7 +90,10 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.concurrent.OpenSearchRejectedExecutionException; +import org.opensearch.http.HttpException; import org.opensearch.index.Index; +import org.opensearch.index.IndexModule; import org.opensearch.index.shard.ShardId; import org.opensearch.repositories.IndexId; import org.opensearch.repositories.RepositoriesService; @@ -1753,10 +1757,37 @@ public ClusterState execute(ClusterState currentState) throws Exception { snapshotNames, repoName ); - deleteFromRepoTask = createDeleteStateUpdate(snapshotIds, repoName, repositoryData, Priority.NORMAL, listener); + final List snapshotIdNotBackingIndex = filterSnapshotsBackingAnyIndex(currentState, snapshotIds); + deleteFromRepoTask = createDeleteStateUpdate(snapshotIdNotBackingIndex, repoName, repositoryData, Priority.NORMAL, listener); return deleteFromRepoTask.execute(currentState); } + private List filterSnapshotsBackingAnyIndex(ClusterState currentState, List snapshotIds){ + final Set snapshotsToBeDeleted = new HashSet<>(); + final Set snapshotsToBeNotDeleted = new HashSet<>(); + ImmutableOpenMap indicesMap = currentState.getMetadata().getIndices(); + for(SnapshotId snapshotId: snapshotIds) { + Boolean indexBackedBySnapshotFound = false; + for (Iterator it = indicesMap.valuesIt(); it.hasNext(); ) { + IndexMetadata indexMetadata = it.next(); + String storeType = indexMetadata.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()); + if(storeType != null && storeType.equals(IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) && indexMetadata.getSettings().get("index.searchable_snapshot.snapshot_id.uuid").equals(snapshotId.getUUID())){ + indexBackedBySnapshotFound = true; + break; + } + } + if(indexBackedBySnapshotFound == false) { + snapshotsToBeDeleted.add(snapshotId); + } else { + snapshotsToBeNotDeleted.add(snapshotId.getName()); + } + } + if(snapshotIds.size() != snapshotsToBeDeleted.size()) { + throw new SnapshotDeletionException(repoName, snapshotsToBeNotDeleted.toString(), "These remote snapshots are backing some indices and hence can't be deleted! No snapshots were deleted."); + } + return List.copyOf(snapshotsToBeDeleted); + } + @Override public void onFailure(String source, Exception e) { listener.onFailure(e); From bf329796fa773aa76d7d537331760f59daea0ed0 Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Fri, 4 Nov 2022 11:55:57 +0530 Subject: [PATCH 02/14] Adding changelog and applying gradle precommit Signed-off-by: Vishal Sarda --- CHANGELOG.md | 1 + .../snapshots/SnapshotsService.java | 31 ++++++++++++------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05ec47bce635d..f9095e656d28e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Data node side change([#4204](https://github.com/opensearch-project/OpenSearch/pull/4204)) - on-boarding of tasks([#4542](https://github.com/opensearch-project/OpenSearch/pull/4542)) - Integs ([4588](https://github.com/opensearch-project/OpenSearch/pull/4588)) +- Prevent deletion of snapshots that are backing searchable snapshot indexes ([#5069](https://github.com/opensearch-project/OpenSearch/pull/5069)) ### Dependencies - Bumps `log4j-core` from 2.18.0 to 2.19.0 diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java index ad271d52e9ff0..ccc64af2bf7ea 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java @@ -38,7 +38,6 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; import org.opensearch.ExceptionsHelper; -import org.opensearch.OpenSearchStatusException; import org.opensearch.Version; import org.opensearch.action.ActionListener; import org.opensearch.action.ActionRunnable; @@ -92,8 +91,6 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.concurrent.OpenSearchRejectedExecutionException; -import org.opensearch.http.HttpException; import org.opensearch.index.Index; import org.opensearch.index.IndexModule; import org.opensearch.index.shard.ShardId; @@ -1774,32 +1771,44 @@ public ClusterState execute(ClusterState currentState) throws Exception { repoName ); final List snapshotIdNotBackingIndex = filterSnapshotsBackingAnyIndex(currentState, snapshotIds); - deleteFromRepoTask = createDeleteStateUpdate(snapshotIdNotBackingIndex, repoName, repositoryData, Priority.NORMAL, listener); + deleteFromRepoTask = createDeleteStateUpdate( + snapshotIdNotBackingIndex, + repoName, + repositoryData, + Priority.NORMAL, + listener + ); return deleteFromRepoTask.execute(currentState); } - private List filterSnapshotsBackingAnyIndex(ClusterState currentState, List snapshotIds){ + private List filterSnapshotsBackingAnyIndex(ClusterState currentState, List snapshotIds) { final Set snapshotsToBeDeleted = new HashSet<>(); final Set snapshotsToBeNotDeleted = new HashSet<>(); ImmutableOpenMap indicesMap = currentState.getMetadata().getIndices(); - for(SnapshotId snapshotId: snapshotIds) { + for (SnapshotId snapshotId : snapshotIds) { Boolean indexBackedBySnapshotFound = false; - for (Iterator it = indicesMap.valuesIt(); it.hasNext(); ) { + for (Iterator it = indicesMap.valuesIt(); it.hasNext();) { IndexMetadata indexMetadata = it.next(); String storeType = indexMetadata.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()); - if(storeType != null && storeType.equals(IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) && indexMetadata.getSettings().get("index.searchable_snapshot.snapshot_id.uuid").equals(snapshotId.getUUID())){ + if (storeType != null + && storeType.equals(IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + && indexMetadata.getSettings().get("index.searchable_snapshot.snapshot_id.uuid").equals(snapshotId.getUUID())) { indexBackedBySnapshotFound = true; break; } } - if(indexBackedBySnapshotFound == false) { + if (indexBackedBySnapshotFound == false) { snapshotsToBeDeleted.add(snapshotId); } else { snapshotsToBeNotDeleted.add(snapshotId.getName()); } } - if(snapshotIds.size() != snapshotsToBeDeleted.size()) { - throw new SnapshotDeletionException(repoName, snapshotsToBeNotDeleted.toString(), "These remote snapshots are backing some indices and hence can't be deleted! No snapshots were deleted."); + if (snapshotIds.size() != snapshotsToBeDeleted.size()) { + throw new SnapshotDeletionException( + repoName, + snapshotsToBeNotDeleted.toString(), + "These remote snapshots are backing some indices and hence can't be deleted! No snapshots were deleted." + ); } return List.copyOf(snapshotsToBeDeleted); } From 3b43e06771db211196229ead9b046c4ee1a16e74 Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Fri, 4 Nov 2022 13:48:47 +0530 Subject: [PATCH 03/14] Adding javadoc Signed-off-by: Vishal Sarda --- .../org/opensearch/snapshots/SnapshotDeletionException.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java b/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java index e0c54a7cf5fd3..d16ada3ef2c24 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java @@ -10,6 +10,11 @@ import org.opensearch.rest.RestStatus; +/** + * Thrown if requested snapshot/s can't be deleted + * + * @opensearch.internal + */ public class SnapshotDeletionException extends SnapshotException { public SnapshotDeletionException(final String repositoryName, final String snapshotName, final String msg) { From eabebd87efe85309ddf60beb2c0ae258c33f75ef Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Sat, 5 Nov 2022 00:39:14 +0530 Subject: [PATCH 04/14] Fixing gradle check error Signed-off-by: Vishal Sarda --- .../src/main/java/org/opensearch/OpenSearchException.java | 6 ++++++ .../opensearch/snapshots/SnapshotDeletionException.java | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/server/src/main/java/org/opensearch/OpenSearchException.java b/server/src/main/java/org/opensearch/OpenSearchException.java index e30b5a91b54d0..6e15600b86a62 100644 --- a/server/src/main/java/org/opensearch/OpenSearchException.java +++ b/server/src/main/java/org/opensearch/OpenSearchException.java @@ -1611,6 +1611,12 @@ private enum OpenSearchExceptionHandle { ClusterManagerThrottlingException::new, 165, Version.V_2_4_0 + ), + SNAPSHOT_DELETION_EXCEPTION( + org.opensearch.snapshots.SnapshotDeletionException.class, + org.opensearch.snapshots.SnapshotDeletionException::new, + 166, + UNKNOWN_VERSION_ADDED ); final Class exceptionClass; diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java b/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java index d16ada3ef2c24..58eea6d7da856 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java @@ -8,8 +8,11 @@ package org.opensearch.snapshots; +import org.opensearch.common.io.stream.StreamInput; import org.opensearch.rest.RestStatus; +import java.io.IOException; + /** * Thrown if requested snapshot/s can't be deleted * @@ -21,6 +24,10 @@ public SnapshotDeletionException(final String repositoryName, final String snaps super(repositoryName, snapshotName, msg); } + public SnapshotDeletionException(StreamInput in) throws IOException { + super(in); + } + @Override public RestStatus status() { return RestStatus.FORBIDDEN; From 0228972fe9b40daf1341285414af3b79dd493934 Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Sat, 5 Nov 2022 01:32:52 +0530 Subject: [PATCH 05/14] Fixing tests Signed-off-by: Vishal Sarda --- .../java/org/opensearch/ExceptionSerializationTests.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/opensearch/ExceptionSerializationTests.java b/server/src/test/java/org/opensearch/ExceptionSerializationTests.java index 616ad0a57bf93..9f6af15b81149 100644 --- a/server/src/test/java/org/opensearch/ExceptionSerializationTests.java +++ b/server/src/test/java/org/opensearch/ExceptionSerializationTests.java @@ -102,10 +102,7 @@ import org.opensearch.search.SearchShardTarget; import org.opensearch.search.aggregations.MultiBucketConsumerService; import org.opensearch.search.internal.ShardSearchContextId; -import org.opensearch.snapshots.Snapshot; -import org.opensearch.snapshots.SnapshotException; -import org.opensearch.snapshots.SnapshotId; -import org.opensearch.snapshots.SnapshotInProgressException; +import org.opensearch.snapshots.*; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; import org.opensearch.transport.ActionNotFoundTransportException; @@ -862,6 +859,7 @@ public void testIds() { ids.put(163, DecommissioningFailedException.class); ids.put(164, NodeDecommissionedException.class); ids.put(165, ClusterManagerThrottlingException.class); + ids.put(166, SnapshotDeletionException.class); Map, Integer> reverse = new HashMap<>(); for (Map.Entry> entry : ids.entrySet()) { From dbbe321dc7b728e390f38e330eaa27728829a0cd Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Thu, 10 Nov 2022 01:06:22 +0530 Subject: [PATCH 06/14] WIP Signed-off-by: Vishal Sarda --- .../java/org/opensearch/snapshots/SnapshotsService.java | 6 +++--- .../java/org/opensearch/ExceptionSerializationTests.java | 6 +++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java index ccc64af2bf7ea..206623c335fb9 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java @@ -93,6 +93,7 @@ import org.opensearch.common.unit.TimeValue; import org.opensearch.index.Index; import org.opensearch.index.IndexModule; +import org.opensearch.index.IndexSettings; import org.opensearch.index.shard.ShardId; import org.opensearch.repositories.IndexId; import org.opensearch.repositories.RepositoriesService; @@ -1790,9 +1791,8 @@ private List filterSnapshotsBackingAnyIndex(ClusterState currentStat for (Iterator it = indicesMap.valuesIt(); it.hasNext();) { IndexMetadata indexMetadata = it.next(); String storeType = indexMetadata.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()); - if (storeType != null - && storeType.equals(IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) - && indexMetadata.getSettings().get("index.searchable_snapshot.snapshot_id.uuid").equals(snapshotId.getUUID())) { + if (IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey().equals(storeType) + && indexMetadata.getSettings().get(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey()).equals(snapshotId.getUUID())) { indexBackedBySnapshotFound = true; break; } diff --git a/server/src/test/java/org/opensearch/ExceptionSerializationTests.java b/server/src/test/java/org/opensearch/ExceptionSerializationTests.java index 9f6af15b81149..af579047e40d6 100644 --- a/server/src/test/java/org/opensearch/ExceptionSerializationTests.java +++ b/server/src/test/java/org/opensearch/ExceptionSerializationTests.java @@ -102,7 +102,11 @@ import org.opensearch.search.SearchShardTarget; import org.opensearch.search.aggregations.MultiBucketConsumerService; import org.opensearch.search.internal.ShardSearchContextId; -import org.opensearch.snapshots.*; +import org.opensearch.snapshots.Snapshot; +import org.opensearch.snapshots.SnapshotException; +import org.opensearch.snapshots.SnapshotId; +import org.opensearch.snapshots.SnapshotInProgressException; +import org.opensearch.snapshots.SnapshotDeletionException; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; import org.opensearch.transport.ActionNotFoundTransportException; From fb6d33defebcea65c76adc7571460c32e6c699df Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Fri, 18 Nov 2022 16:03:32 -0800 Subject: [PATCH 07/14] Adding unit and integration tests, moving validation function to utils Signed-off-by: Vishal Sarda --- .../snapshots/SearchableSnapshotIT.java | 120 ++++++++++++++---- .../opensearch/snapshots/SnapshotUtils.java | 40 ++++++ .../snapshots/SnapshotsService.java | 44 +------ .../snapshots/SnapshotUtilsTests.java | 52 ++++++++ 4 files changed, 189 insertions(+), 67 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java index 17c32bb407bc3..1aef077866203 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java @@ -7,11 +7,14 @@ import com.carrotsearch.randomizedtesting.generators.RandomPicks; import org.hamcrest.MatcherAssert; import org.junit.BeforeClass; +import org.opensearch.action.StepListener; import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; import org.opensearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; +import org.opensearch.action.admin.cluster.snapshots.delete.DeleteSnapshotRequest; import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotRequest; import org.opensearch.action.admin.indices.settings.put.UpdateSettingsRequestBuilder; import org.opensearch.action.index.IndexRequestBuilder; +import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.client.Client; import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.block.ClusterBlockException; @@ -27,6 +30,7 @@ import org.opensearch.index.Index; import org.opensearch.index.IndexNotFoundException; import org.opensearch.monitor.fs.FsInfo; +import org.opensearch.repositories.fs.FsRepository; import java.nio.file.Files; import java.nio.file.Path; @@ -74,23 +78,29 @@ private Settings.Builder chunkedRepositorySettings() { * Ensures availability of sufficient data nodes and search capable nodes. */ public void testCreateSearchableSnapshot() throws Exception { + final String snapshotName = "test-snap"; + final String repoName = "test-repo"; + final String indexName1 = "test-idx-1"; + final String restoredIndexName1 = indexName1 + "-copy"; + final String indexName2 = "test-idx-2"; + final String restoredIndexName2 = indexName2 + "-copy"; final int numReplicasIndex1 = randomIntBetween(1, 4); final int numReplicasIndex2 = randomIntBetween(0, 2); final Client client = client(); internalCluster().ensureAtLeastNumDataNodes(Math.max(numReplicasIndex1, numReplicasIndex2) + 1); - createIndexWithDocsAndEnsureGreen(numReplicasIndex1, 100, "test-idx-1"); - createIndexWithDocsAndEnsureGreen(numReplicasIndex2, 100, "test-idx-2"); + createIndexWithDocsAndEnsureGreen(numReplicasIndex1, 100, indexName1); + createIndexWithDocsAndEnsureGreen(numReplicasIndex2, 100, indexName2); - takeSnapshot(client, "test-idx-1", "test-idx-2"); - deleteIndicesAndEnsureGreen(client, "test-idx-1", "test-idx-2"); + takeSnapshot(client, snapshotName, repoName, false, indexName1, indexName2); + deleteIndicesAndEnsureGreen(client, indexName1, indexName2); internalCluster().ensureAtLeastNumSearchNodes(Math.max(numReplicasIndex1, numReplicasIndex2) + 1); - restoreSnapshotAndEnsureGreen(client); + restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); - assertDocCount("test-idx-1-copy", 100L); - assertDocCount("test-idx-2-copy", 100L); - assertIndexDirectoryDoesNotExist("test-idx-1-copy", "test-idx-2-copy"); + assertDocCount(restoredIndexName1, 100L); + assertDocCount(restoredIndexName2, 100L); + assertIndexDirectoryDoesNotExist(restoredIndexName1, restoredIndexName2); } /** @@ -101,16 +111,18 @@ public void testCreateSearchableSnapshotWithChunks() throws Exception { final int numReplicasIndex = randomIntBetween(1, 4); final String indexName = "test-idx"; final String restoredIndexName = indexName + "-copy"; + final String repoName = "test-repo"; + final String snapshotName = "test-snap"; final Client client = client(); Settings.Builder repositorySettings = chunkedRepositorySettings(); internalCluster().ensureAtLeastNumSearchAndDataNodes(numReplicasIndex + 1); createIndexWithDocsAndEnsureGreen(numReplicasIndex, 1000, indexName); - takeSnapshot(client, repositorySettings, indexName); + takeSnapshot(client, repositorySettings, snapshotName, repoName, false, indexName); deleteIndicesAndEnsureGreen(client, indexName); - restoreSnapshotAndEnsureGreen(client); + restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); assertDocCount(restoredIndexName, 1000L); } @@ -124,13 +136,15 @@ public void testSearchableSnapshotAllocationForLocalAndRemoteShardsOnSameNode() final int numReplicasIndex = randomIntBetween(1, 4); final String indexName = "test-idx"; final String restoredIndexName = indexName + "-copy"; + final String repoName = "test-repo"; + final String snapshotName = "test-snap"; final Client client = client(); internalCluster().ensureAtLeastNumSearchAndDataNodes(numReplicasIndex + 1); createIndexWithDocsAndEnsureGreen(numReplicasIndex, 100, indexName); - takeSnapshot(client, indexName); + takeSnapshot(client, snapshotName, repoName, false, indexName); - restoreSnapshotAndEnsureGreen(client); + restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); assertDocCount(restoredIndexName, 100L); assertDocCount(indexName, 100L); @@ -145,16 +159,18 @@ public void testSearchableSnapshotAllocationForFailoverAndRecovery() throws Exce final int numReplicasIndex = 1; final String indexName = "test-idx"; final String restoredIndexName = indexName + "-copy"; + final String repoName = "test-repo"; + final String snapshotName = "test-snap"; final Client client = client(); internalCluster().ensureAtLeastNumDataNodes(numReplicasIndex + 1); createIndexWithDocsAndEnsureGreen(numReplicasIndex, 100, indexName); - takeSnapshot(client, indexName); + takeSnapshot(client, snapshotName, repoName, false, indexName); deleteIndicesAndEnsureGreen(client, indexName); internalCluster().ensureAtLeastNumSearchNodes(numReplicasIndex + 1); - restoreSnapshotAndEnsureGreen(client); + restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); assertDocCount(restoredIndexName, 100L); logger.info("--> stop a random search node"); @@ -183,14 +199,16 @@ public void testSearchableSnapshotAllocationForFailoverAndRecovery() throws Exce public void testSearchableSnapshotIndexIsReadOnly() throws Exception { final String indexName = "test-index"; final String restoredIndexName = indexName + "-copy"; + final String repoName = "test-repo"; + final String snapshotName = "test-snap"; final Client client = client(); createIndexWithDocsAndEnsureGreen(0, 100, indexName); - takeSnapshot(client, indexName); + takeSnapshot(client, snapshotName, repoName, false, indexName); deleteIndicesAndEnsureGreen(client, indexName); internalCluster().ensureAtLeastNumSearchNodes(1); - restoreSnapshotAndEnsureGreen(client); + restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); assertIndexingBlocked(restoredIndexName); assertIndexSettingChangeBlocked(restoredIndexName); @@ -202,6 +220,47 @@ public void testSearchableSnapshotIndexIsReadOnly() throws Exception { ); } + public void testDeleteSearchableSnapshotBackingIndexThrowsException() throws Exception { + disableRepoConsistencyCheck("reason"); + final String indexName = "test-index"; + final Client client = client(); + final String repoName = "test-repo"; + final String snapshotName = "test-snap"; + final StepListener deleteSnapshotStepListener = new StepListener<>(); + createRepository(repoName, FsRepository.TYPE); + createIndexWithDocsAndEnsureGreen(0, 100, indexName); + takeSnapshot(client, snapshotName, repoName, false, indexName); + internalCluster().ensureAtLeastNumSearchNodes(1); + restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); + assertThrows( + SnapshotDeletionException.class, + () -> client().admin().cluster().deleteSnapshot(new DeleteSnapshotRequest(repoName, snapshotName)).actionGet() + ); + } + + public void testDeleteSearchableSnapshotBackingIndex() throws Exception { + disableRepoConsistencyCheck("reason"); + final String indexName1 = "test-index1"; + final String indexName2 = "test-index2"; + final Client client = client(); + final String repoName = "test-repo"; + final String snapshotName1 = "test-snapshot1"; + final String snapshotName2 = "test-snap"; + final StepListener deleteSnapshotStepListener = new StepListener<>(); + createRepository(repoName, FsRepository.TYPE); + createIndexWithDocsAndEnsureGreen(0, 100, indexName1); + createIndexWithDocsAndEnsureGreen(0, 100, indexName2); + takeSnapshot(client, snapshotName1, repoName, false, indexName1); + takeSnapshot(client, snapshotName2, repoName, true, indexName2); + internalCluster().ensureAtLeastNumSearchNodes(1); + restoreSnapshotAndEnsureGreen(client, snapshotName2, repoName); + try { + client().admin().cluster().deleteSnapshot(new DeleteSnapshotRequest(repoName, snapshotName1)).actionGet(); + } catch (Exception e) { + fail("Should not have thrown any exception"); + } + } + private void createIndexWithDocsAndEnsureGreen(int numReplicasIndex, int numOfDocs, String indexName) throws InterruptedException { createIndex( indexName, @@ -216,21 +275,30 @@ private void createIndexWithDocsAndEnsureGreen(int numReplicasIndex, int numOfDo ensureGreen(); } - private void takeSnapshot(Client client, String... indices) { - takeSnapshot(client, null, indices); + private void takeSnapshot(Client client, String snapshotName, String repoName, Boolean doesRepoExist, String... indices) { + takeSnapshot(client, null, snapshotName, repoName, doesRepoExist, indices); } - private void takeSnapshot(Client client, Settings.Builder repositorySettings, String... indices) { + private void takeSnapshot( + Client client, + Settings.Builder repositorySettings, + String snapshotName, + String repoName, + Boolean doesRepoExist, + String... indices + ) { logger.info("--> Create a repository"); - if (repositorySettings == null) { - createRepository("test-repo", "fs"); - } else { - createRepository("test-repo", "fs", repositorySettings); + if (!doesRepoExist) { + if (repositorySettings == null) { + createRepository(repoName, FsRepository.TYPE); + } else { + createRepository(repoName, FsRepository.TYPE, repositorySettings); + } } logger.info("--> Take a snapshot"); final CreateSnapshotResponse createSnapshotResponse = client.admin() .cluster() - .prepareCreateSnapshot("test-repo", "test-snap") + .prepareCreateSnapshot(repoName, snapshotName) .setWaitForCompletion(true) .setIndices(indices) .get(); @@ -247,11 +315,11 @@ private void deleteIndicesAndEnsureGreen(Client client, String... indices) { ensureGreen(); } - private void restoreSnapshotAndEnsureGreen(Client client) { + private void restoreSnapshotAndEnsureGreen(Client client, String snapshotName, String repoName) { logger.info("--> restore indices as 'remote_snapshot'"); client.admin() .cluster() - .prepareRestoreSnapshot("test-repo", "test-snap") + .prepareRestoreSnapshot(repoName, snapshotName) .setRenamePattern("(.+)") .setRenameReplacement("$1-copy") .setStorageType(RestoreSnapshotRequest.StorageType.REMOTE_SNAPSHOT) diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java b/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java index 073e4f7723077..e1ac87c0e3c20 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java @@ -32,9 +32,14 @@ package org.opensearch.snapshots; import org.opensearch.action.support.IndicesOptions; +import org.opensearch.cluster.ClusterState; +import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; +import org.opensearch.common.collect.ImmutableOpenMap; import org.opensearch.common.regex.Regex; +import org.opensearch.index.IndexModule; import org.opensearch.index.IndexNotFoundException; +import org.opensearch.index.IndexSettings; import java.util.ArrayList; import java.util.Arrays; @@ -42,6 +47,9 @@ import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.HashMap; +import java.util.Map; +import java.util.Iterator; /** * Snapshot utilities @@ -135,4 +143,36 @@ public static List filterIndices(List availableIndices, String[] } return Collections.unmodifiableList(new ArrayList<>(result)); } + + /** + * Validates if there are any remote snapshots backing an index + * @param currentState current cluster state + * @param snapshotIds list of snapshot Ids to be verified + * @param repoName repo name for which the verification is being done + */ + public static void validateSnapshotsBackingAnyIndex(ClusterState currentState, List snapshotIds, String repoName) { + final Map uuidToSnapshotId = new HashMap<>(); + final Set snapshotsToBeNotDeleted = new HashSet<>(); + ImmutableOpenMap indicesMap = currentState.getMetadata().getIndices(); + snapshotIds.forEach(snapshotId -> uuidToSnapshotId.put(snapshotId.getUUID(), snapshotId)); + + for (Iterator it = indicesMap.valuesIt(); it.hasNext();) { + IndexMetadata indexMetadata = it.next(); + String storeType = indexMetadata.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()); + if (IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey().equals(storeType)) { + String snapshotId = indexMetadata.getSettings().get(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey()); + if (uuidToSnapshotId.get(snapshotId) != null) { + snapshotsToBeNotDeleted.add(uuidToSnapshotId.get(snapshotId).getName()); + } + } + } + + if (!snapshotsToBeNotDeleted.isEmpty()) { + throw new SnapshotDeletionException( + repoName, + snapshotsToBeNotDeleted.toString(), + "These remote snapshots are backing some indices and hence can't be deleted! No snapshots were deleted." + ); + } + } } diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java index 206623c335fb9..2ddc532bb449b 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java @@ -92,8 +92,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; import org.opensearch.index.Index; -import org.opensearch.index.IndexModule; -import org.opensearch.index.IndexSettings; import org.opensearch.index.shard.ShardId; import org.opensearch.repositories.IndexId; import org.opensearch.repositories.RepositoriesService; @@ -134,6 +132,7 @@ import static java.util.Collections.emptySet; import static java.util.Collections.unmodifiableList; import static org.opensearch.cluster.SnapshotsInProgress.completed; +import static org.opensearch.snapshots.SnapshotUtils.validateSnapshotsBackingAnyIndex; /** * Service responsible for creating snapshots. This service runs all the steps executed on the cluster-manager node during snapshot creation and @@ -1771,48 +1770,11 @@ public ClusterState execute(ClusterState currentState) throws Exception { snapshotNames, repoName ); - final List snapshotIdNotBackingIndex = filterSnapshotsBackingAnyIndex(currentState, snapshotIds); - deleteFromRepoTask = createDeleteStateUpdate( - snapshotIdNotBackingIndex, - repoName, - repositoryData, - Priority.NORMAL, - listener - ); + validateSnapshotsBackingAnyIndex(currentState, snapshotIds, repoName); + deleteFromRepoTask = createDeleteStateUpdate(snapshotIds, repoName, repositoryData, Priority.NORMAL, listener); return deleteFromRepoTask.execute(currentState); } - private List filterSnapshotsBackingAnyIndex(ClusterState currentState, List snapshotIds) { - final Set snapshotsToBeDeleted = new HashSet<>(); - final Set snapshotsToBeNotDeleted = new HashSet<>(); - ImmutableOpenMap indicesMap = currentState.getMetadata().getIndices(); - for (SnapshotId snapshotId : snapshotIds) { - Boolean indexBackedBySnapshotFound = false; - for (Iterator it = indicesMap.valuesIt(); it.hasNext();) { - IndexMetadata indexMetadata = it.next(); - String storeType = indexMetadata.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()); - if (IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey().equals(storeType) - && indexMetadata.getSettings().get(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey()).equals(snapshotId.getUUID())) { - indexBackedBySnapshotFound = true; - break; - } - } - if (indexBackedBySnapshotFound == false) { - snapshotsToBeDeleted.add(snapshotId); - } else { - snapshotsToBeNotDeleted.add(snapshotId.getName()); - } - } - if (snapshotIds.size() != snapshotsToBeDeleted.size()) { - throw new SnapshotDeletionException( - repoName, - snapshotsToBeNotDeleted.toString(), - "These remote snapshots are backing some indices and hence can't be deleted! No snapshots were deleted." - ); - } - return List.copyOf(snapshotsToBeDeleted); - } - @Override public ClusterManagerTaskThrottler.ThrottlingKey getClusterManagerThrottlingKey() { return deleteSnapshotTaskKey; diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java index 489294fd53bd4..7a793a3e7e509 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java @@ -31,13 +31,21 @@ package org.opensearch.snapshots; +import org.opensearch.Version; import org.opensearch.action.support.IndicesOptions; +import org.opensearch.cluster.ClusterState; +import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.cluster.metadata.Metadata; +import org.opensearch.common.settings.Settings; +import org.opensearch.index.IndexModule; +import org.opensearch.index.IndexSettings; import org.opensearch.test.OpenSearchTestCase; import java.util.Arrays; import java.util.List; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_CREATED; public class SnapshotUtilsTests extends OpenSearchTestCase { public void testIndexNameFiltering() { @@ -85,4 +93,48 @@ private void assertIndexNameFiltering(String[] indices, String[] filter, Indices List actual = SnapshotUtils.filterIndices(indicesList, filter, indicesOptions); assertThat(actual, containsInAnyOrder(expected)); } + + public void testValidateSnapshotsBackingAnyIndex() { + final String repoName = "test-repo"; + final SnapshotId snapshotId1 = new SnapshotId("testSnapshot1", "uuid1"); + final SnapshotId snapshotId2 = new SnapshotId("testSnapshot2", "uuid2"); + ClusterState clusterState = getClusterState(snapshotId1, repoName); + try { + SnapshotUtils.validateSnapshotsBackingAnyIndex(clusterState, List.of(snapshotId2), repoName); + } catch (Exception e) { + fail("Should not have thrown any exception"); + } + } + + public void testValidateSnapshotsBackingAnyIndexThrowsException() { + final String repoName = "test-repo"; + final SnapshotId snapshotId1 = new SnapshotId("testSnapshot1", "uuid1"); + ClusterState clusterState = getClusterState(snapshotId1, repoName); + expectThrows( + SnapshotDeletionException.class, + () -> SnapshotUtils.validateSnapshotsBackingAnyIndex(clusterState, List.of(snapshotId1), repoName) + ); + } + + private static ClusterState getClusterState(SnapshotId snapshotId, String repoName) { + final String index = "test-index"; + Snapshot snapshot = new Snapshot(repoName, snapshotId); + final Metadata.Builder metaBuilder = Metadata.builder(Metadata.EMPTY_METADATA); + metaBuilder.put( + IndexMetadata.builder(index) + .settings( + Settings.builder() + .put(SETTING_VERSION_CREATED, Version.CURRENT.id) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_REPOSITORY.getKey(), snapshot.getRepository()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey(), snapshot.getSnapshotId().getUUID()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.getKey(), snapshot.getSnapshotId().getName()) + ) + .numberOfShards(1) + .numberOfReplicas(1) + .build(), + false + ); + return ClusterState.builder(ClusterState.EMPTY_STATE).metadata(metaBuilder).build(); + } } From 6b601ee620845910450b8c7c4d47e4b33b58ad32 Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Fri, 18 Nov 2022 16:07:16 -0800 Subject: [PATCH 08/14] removing unwanted characters Signed-off-by: Vishal Sarda --- CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d3424a175a6f..b9d33ad393302 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Allow mmap to use new JDK-19 preview APIs in Apache Lucene 9.4+ ([#5151](https://github.com/opensearch-project/OpenSearch/pull/5151)) - Add feature flag for extensions ([#5211](https://github.com/opensearch-project/OpenSearch/pull/5211)) ->>>>>>> main ### Dependencies - Bumps `log4j-core` from 2.18.0 to 2.19.0 - Bumps `reactor-netty-http` from 1.0.18 to 1.0.23 @@ -99,4 +98,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Security [Unreleased 3.0]: https://github.com/opensearch-project/OpenSearch/compare/2.4...HEAD -[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.4...2.x \ No newline at end of file +[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.4...2.x From 8434fbd54b5931e6e625cc7db9821f0319851f1a Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Fri, 18 Nov 2022 16:11:06 -0800 Subject: [PATCH 09/14] Remove stale entries from changelog Signed-off-by: Vishal Sarda --- CHANGELOG.md | 8 -------- 1 file changed, 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9d33ad393302..90031a016137c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,15 +17,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Apply reproducible builds configuration for OpenSearch plugins through gradle plugin ([#4746](https://github.com/opensearch-project/OpenSearch/pull/4746)) - Add project health badges to the README.md ([#4843](https://github.com/opensearch-project/OpenSearch/pull/4843)) - [Test] Add IAE test for deprecated edgeNGram analyzer name ([#5040](https://github.com/opensearch-project/OpenSearch/pull/5040)) -- Add Cluster manager task throttling framework. Cluster manager node will throttle task submission based on throttling thresholds. - This throttling will be at task type level. Data nodes will perform retries on these throttling exception with exponential delay. (PR: [#4986](https://github.com/opensearch-project/OpenSearch/pull/4986)) ( Issue : [#479](https://github.com/opensearch-project/OpenSearch/issues/479)) - - Throttling Exception / New Backoff policy([#3527](https://github.com/opensearch-project/OpenSearch/pull/3527)) - - Cluster Manager node side change([#3882](https://github.com/opensearch-project/OpenSearch/pull/3882)) - - Data node side change([#4204](https://github.com/opensearch-project/OpenSearch/pull/4204)) - - on-boarding of tasks([#4542](https://github.com/opensearch-project/OpenSearch/pull/4542)) - - Integs ([4588](https://github.com/opensearch-project/OpenSearch/pull/4588)) - Prevent deletion of snapshots that are backing searchable snapshot indexes ([#5069](https://github.com/opensearch-project/OpenSearch/pull/5069)) -- Integration tests for searchable snapshots ([#5048](https://github.com/opensearch-project/OpenSearch/pull/5048)) - Allow mmap to use new JDK-19 preview APIs in Apache Lucene 9.4+ ([#5151](https://github.com/opensearch-project/OpenSearch/pull/5151)) - Add feature flag for extensions ([#5211](https://github.com/opensearch-project/OpenSearch/pull/5211)) From 6481b1191ee626eec2cffd6bac58d185aa894e75 Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Mon, 21 Nov 2022 15:27:28 -0800 Subject: [PATCH 10/14] Changing HTTP status from 401 to 409 Signed-off-by: Vishal Sarda --- .../org/opensearch/snapshots/SnapshotDeletionException.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java b/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java index 58eea6d7da856..7653aeffe5591 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java @@ -30,6 +30,6 @@ public SnapshotDeletionException(StreamInput in) throws IOException { @Override public RestStatus status() { - return RestStatus.FORBIDDEN; + return RestStatus.CONFLICT; } } From 8341095cb9522de7e7705edf854e45c1dc924ffc Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Tue, 22 Nov 2022 15:06:02 -0800 Subject: [PATCH 11/14] PR review implementation Signed-off-by: Vishal Sarda --- .../snapshots/SearchableSnapshotIT.java | 62 ++++++++----------- .../org/opensearch/OpenSearchException.java | 5 +- ...va => SnapshotInUseDeletionException.java} | 6 +- .../opensearch/snapshots/SnapshotUtils.java | 14 ++--- .../snapshots/SnapshotsService.java | 2 +- .../ExceptionSerializationTests.java | 4 +- .../snapshots/SnapshotUtilsTests.java | 52 +++++++--------- 7 files changed, 65 insertions(+), 80 deletions(-) rename server/src/main/java/org/opensearch/snapshots/{SnapshotDeletionException.java => SnapshotInUseDeletionException.java} (69%) diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java index 1aef077866203..59ead7e98f03e 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java @@ -7,14 +7,12 @@ import com.carrotsearch.randomizedtesting.generators.RandomPicks; import org.hamcrest.MatcherAssert; import org.junit.BeforeClass; -import org.opensearch.action.StepListener; import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; import org.opensearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.opensearch.action.admin.cluster.snapshots.delete.DeleteSnapshotRequest; import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotRequest; import org.opensearch.action.admin.indices.settings.put.UpdateSettingsRequestBuilder; import org.opensearch.action.index.IndexRequestBuilder; -import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.client.Client; import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.block.ClusterBlockException; @@ -92,7 +90,8 @@ public void testCreateSearchableSnapshot() throws Exception { createIndexWithDocsAndEnsureGreen(numReplicasIndex1, 100, indexName1); createIndexWithDocsAndEnsureGreen(numReplicasIndex2, 100, indexName2); - takeSnapshot(client, snapshotName, repoName, false, indexName1, indexName2); + createRepositoryWithSettings(null, repoName); + takeSnapshot(client, snapshotName, repoName, indexName1, indexName2); deleteIndicesAndEnsureGreen(client, indexName1, indexName2); internalCluster().ensureAtLeastNumSearchNodes(Math.max(numReplicasIndex1, numReplicasIndex2) + 1); @@ -119,7 +118,8 @@ public void testCreateSearchableSnapshotWithChunks() throws Exception { internalCluster().ensureAtLeastNumSearchAndDataNodes(numReplicasIndex + 1); createIndexWithDocsAndEnsureGreen(numReplicasIndex, 1000, indexName); - takeSnapshot(client, repositorySettings, snapshotName, repoName, false, indexName); + createRepositoryWithSettings(repositorySettings, repoName); + takeSnapshot(client, snapshotName, repoName, indexName); deleteIndicesAndEnsureGreen(client, indexName); restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); @@ -142,7 +142,8 @@ public void testSearchableSnapshotAllocationForLocalAndRemoteShardsOnSameNode() internalCluster().ensureAtLeastNumSearchAndDataNodes(numReplicasIndex + 1); createIndexWithDocsAndEnsureGreen(numReplicasIndex, 100, indexName); - takeSnapshot(client, snapshotName, repoName, false, indexName); + createRepositoryWithSettings(null, repoName); + takeSnapshot(client, snapshotName, repoName, indexName); restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); @@ -166,7 +167,8 @@ public void testSearchableSnapshotAllocationForFailoverAndRecovery() throws Exce internalCluster().ensureAtLeastNumDataNodes(numReplicasIndex + 1); createIndexWithDocsAndEnsureGreen(numReplicasIndex, 100, indexName); - takeSnapshot(client, snapshotName, repoName, false, indexName); + createRepositoryWithSettings(null, repoName); + takeSnapshot(client, snapshotName, repoName, indexName); deleteIndicesAndEnsureGreen(client, indexName); internalCluster().ensureAtLeastNumSearchNodes(numReplicasIndex + 1); @@ -204,7 +206,8 @@ public void testSearchableSnapshotIndexIsReadOnly() throws Exception { final Client client = client(); createIndexWithDocsAndEnsureGreen(0, 100, indexName); - takeSnapshot(client, snapshotName, repoName, false, indexName); + createRepositoryWithSettings(null, repoName); + takeSnapshot(client, snapshotName, repoName, indexName); deleteIndicesAndEnsureGreen(client, indexName); internalCluster().ensureAtLeastNumSearchNodes(1); @@ -221,44 +224,36 @@ public void testSearchableSnapshotIndexIsReadOnly() throws Exception { } public void testDeleteSearchableSnapshotBackingIndexThrowsException() throws Exception { - disableRepoConsistencyCheck("reason"); final String indexName = "test-index"; final Client client = client(); final String repoName = "test-repo"; final String snapshotName = "test-snap"; - final StepListener deleteSnapshotStepListener = new StepListener<>(); - createRepository(repoName, FsRepository.TYPE); + createRepositoryWithSettings(null, repoName); createIndexWithDocsAndEnsureGreen(0, 100, indexName); - takeSnapshot(client, snapshotName, repoName, false, indexName); + takeSnapshot(client, snapshotName, repoName, indexName); internalCluster().ensureAtLeastNumSearchNodes(1); restoreSnapshotAndEnsureGreen(client, snapshotName, repoName); assertThrows( - SnapshotDeletionException.class, + SnapshotInUseDeletionException.class, () -> client().admin().cluster().deleteSnapshot(new DeleteSnapshotRequest(repoName, snapshotName)).actionGet() ); } public void testDeleteSearchableSnapshotBackingIndex() throws Exception { - disableRepoConsistencyCheck("reason"); final String indexName1 = "test-index1"; final String indexName2 = "test-index2"; final Client client = client(); final String repoName = "test-repo"; final String snapshotName1 = "test-snapshot1"; final String snapshotName2 = "test-snap"; - final StepListener deleteSnapshotStepListener = new StepListener<>(); - createRepository(repoName, FsRepository.TYPE); + createRepositoryWithSettings(null, repoName); createIndexWithDocsAndEnsureGreen(0, 100, indexName1); createIndexWithDocsAndEnsureGreen(0, 100, indexName2); - takeSnapshot(client, snapshotName1, repoName, false, indexName1); - takeSnapshot(client, snapshotName2, repoName, true, indexName2); + takeSnapshot(client, snapshotName1, repoName, indexName1); + takeSnapshot(client, snapshotName2, repoName, indexName2); internalCluster().ensureAtLeastNumSearchNodes(1); restoreSnapshotAndEnsureGreen(client, snapshotName2, repoName); - try { - client().admin().cluster().deleteSnapshot(new DeleteSnapshotRequest(repoName, snapshotName1)).actionGet(); - } catch (Exception e) { - fail("Should not have thrown any exception"); - } + client().admin().cluster().deleteSnapshot(new DeleteSnapshotRequest(repoName, snapshotName1)).actionGet(); } private void createIndexWithDocsAndEnsureGreen(int numReplicasIndex, int numOfDocs, String indexName) throws InterruptedException { @@ -275,26 +270,12 @@ private void createIndexWithDocsAndEnsureGreen(int numReplicasIndex, int numOfDo ensureGreen(); } - private void takeSnapshot(Client client, String snapshotName, String repoName, Boolean doesRepoExist, String... indices) { - takeSnapshot(client, null, snapshotName, repoName, doesRepoExist, indices); - } - private void takeSnapshot( Client client, - Settings.Builder repositorySettings, String snapshotName, String repoName, - Boolean doesRepoExist, String... indices ) { - logger.info("--> Create a repository"); - if (!doesRepoExist) { - if (repositorySettings == null) { - createRepository(repoName, FsRepository.TYPE); - } else { - createRepository(repoName, FsRepository.TYPE, repositorySettings); - } - } logger.info("--> Take a snapshot"); final CreateSnapshotResponse createSnapshotResponse = client.admin() .cluster() @@ -310,6 +291,15 @@ private void takeSnapshot( ); } + private void createRepositoryWithSettings(Settings.Builder repositorySettings, String repoName) { + logger.info("--> Create a repository"); + if (repositorySettings == null) { + createRepository(repoName, FsRepository.TYPE); + } else { + createRepository(repoName, FsRepository.TYPE, repositorySettings); + } + } + private void deleteIndicesAndEnsureGreen(Client client, String... indices) { assertTrue(client.admin().indices().prepareDelete(indices).get().isAcknowledged()); ensureGreen(); diff --git a/server/src/main/java/org/opensearch/OpenSearchException.java b/server/src/main/java/org/opensearch/OpenSearchException.java index d6e24163c44a1..f865acb88d413 100644 --- a/server/src/main/java/org/opensearch/OpenSearchException.java +++ b/server/src/main/java/org/opensearch/OpenSearchException.java @@ -51,6 +51,7 @@ import org.opensearch.index.shard.ShardId; import org.opensearch.rest.RestStatus; import org.opensearch.search.aggregations.MultiBucketConsumerService; +import org.opensearch.snapshots.SnapshotInUseDeletionException; import org.opensearch.transport.TcpTransport; import java.io.IOException; @@ -1613,8 +1614,8 @@ private enum OpenSearchExceptionHandle { Version.V_2_4_0 ), SNAPSHOT_DELETION_EXCEPTION( - org.opensearch.snapshots.SnapshotDeletionException.class, - org.opensearch.snapshots.SnapshotDeletionException::new, + SnapshotInUseDeletionException.class, + SnapshotInUseDeletionException::new, 166, UNKNOWN_VERSION_ADDED ); diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java b/server/src/main/java/org/opensearch/snapshots/SnapshotInUseDeletionException.java similarity index 69% rename from server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java rename to server/src/main/java/org/opensearch/snapshots/SnapshotInUseDeletionException.java index 7653aeffe5591..e93bf5ab0cd91 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotDeletionException.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotInUseDeletionException.java @@ -18,13 +18,13 @@ * * @opensearch.internal */ -public class SnapshotDeletionException extends SnapshotException { +public class SnapshotInUseDeletionException extends SnapshotException { - public SnapshotDeletionException(final String repositoryName, final String snapshotName, final String msg) { + public SnapshotInUseDeletionException(final String repositoryName, final String snapshotName, final String msg) { super(repositoryName, snapshotName, msg); } - public SnapshotDeletionException(StreamInput in) throws IOException { + public SnapshotInUseDeletionException(StreamInput in) throws IOException { super(in); } diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java b/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java index e1ac87c0e3c20..faf981738878f 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java @@ -31,8 +31,8 @@ package org.opensearch.snapshots; +import com.carrotsearch.hppc.cursors.ObjectCursor; import org.opensearch.action.support.IndicesOptions; -import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.common.collect.ImmutableOpenMap; @@ -49,7 +49,6 @@ import java.util.Set; import java.util.HashMap; import java.util.Map; -import java.util.Iterator; /** * Snapshot utilities @@ -146,18 +145,17 @@ public static List filterIndices(List availableIndices, String[] /** * Validates if there are any remote snapshots backing an index - * @param currentState current cluster state + * @param metadata index metadata from cluster state * @param snapshotIds list of snapshot Ids to be verified * @param repoName repo name for which the verification is being done */ - public static void validateSnapshotsBackingAnyIndex(ClusterState currentState, List snapshotIds, String repoName) { + public static void validateSnapshotsBackingAnyIndex(ImmutableOpenMap metadata, List snapshotIds, String repoName) { final Map uuidToSnapshotId = new HashMap<>(); final Set snapshotsToBeNotDeleted = new HashSet<>(); - ImmutableOpenMap indicesMap = currentState.getMetadata().getIndices(); snapshotIds.forEach(snapshotId -> uuidToSnapshotId.put(snapshotId.getUUID(), snapshotId)); - for (Iterator it = indicesMap.valuesIt(); it.hasNext();) { - IndexMetadata indexMetadata = it.next(); + for (ObjectCursor cursor : metadata.values()) { + IndexMetadata indexMetadata = cursor.value; String storeType = indexMetadata.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey()); if (IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey().equals(storeType)) { String snapshotId = indexMetadata.getSettings().get(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey()); @@ -168,7 +166,7 @@ public static void validateSnapshotsBackingAnyIndex(ClusterState currentState, L } if (!snapshotsToBeNotDeleted.isEmpty()) { - throw new SnapshotDeletionException( + throw new SnapshotInUseDeletionException( repoName, snapshotsToBeNotDeleted.toString(), "These remote snapshots are backing some indices and hence can't be deleted! No snapshots were deleted." diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java index 2ddc532bb449b..645775c3ec09c 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotsService.java @@ -1770,7 +1770,7 @@ public ClusterState execute(ClusterState currentState) throws Exception { snapshotNames, repoName ); - validateSnapshotsBackingAnyIndex(currentState, snapshotIds, repoName); + validateSnapshotsBackingAnyIndex(currentState.getMetadata().getIndices(), snapshotIds, repoName); deleteFromRepoTask = createDeleteStateUpdate(snapshotIds, repoName, repositoryData, Priority.NORMAL, listener); return deleteFromRepoTask.execute(currentState); } diff --git a/server/src/test/java/org/opensearch/ExceptionSerializationTests.java b/server/src/test/java/org/opensearch/ExceptionSerializationTests.java index af579047e40d6..559963b0e0b68 100644 --- a/server/src/test/java/org/opensearch/ExceptionSerializationTests.java +++ b/server/src/test/java/org/opensearch/ExceptionSerializationTests.java @@ -106,7 +106,7 @@ import org.opensearch.snapshots.SnapshotException; import org.opensearch.snapshots.SnapshotId; import org.opensearch.snapshots.SnapshotInProgressException; -import org.opensearch.snapshots.SnapshotDeletionException; +import org.opensearch.snapshots.SnapshotInUseDeletionException; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; import org.opensearch.transport.ActionNotFoundTransportException; @@ -863,7 +863,7 @@ public void testIds() { ids.put(163, DecommissioningFailedException.class); ids.put(164, NodeDecommissionedException.class); ids.put(165, ClusterManagerThrottlingException.class); - ids.put(166, SnapshotDeletionException.class); + ids.put(166, SnapshotInUseDeletionException.class); Map, Integer> reverse = new HashMap<>(); for (Map.Entry> entry : ids.entrySet()) { diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java index 7a793a3e7e509..15c1d152e7e64 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java @@ -33,10 +33,11 @@ import org.opensearch.Version; import org.opensearch.action.support.IndicesOptions; -import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.Metadata; +import org.opensearch.common.collect.ImmutableOpenMap; import org.opensearch.common.settings.Settings; +import org.opensearch.index.Index; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexSettings; import org.opensearch.test.OpenSearchTestCase; @@ -98,43 +99,38 @@ public void testValidateSnapshotsBackingAnyIndex() { final String repoName = "test-repo"; final SnapshotId snapshotId1 = new SnapshotId("testSnapshot1", "uuid1"); final SnapshotId snapshotId2 = new SnapshotId("testSnapshot2", "uuid2"); - ClusterState clusterState = getClusterState(snapshotId1, repoName); - try { - SnapshotUtils.validateSnapshotsBackingAnyIndex(clusterState, List.of(snapshotId2), repoName); - } catch (Exception e) { - fail("Should not have thrown any exception"); - } + SnapshotUtils.validateSnapshotsBackingAnyIndex(getIndexMetadata(snapshotId1, repoName), List.of(snapshotId2), repoName); } public void testValidateSnapshotsBackingAnyIndexThrowsException() { final String repoName = "test-repo"; final SnapshotId snapshotId1 = new SnapshotId("testSnapshot1", "uuid1"); - ClusterState clusterState = getClusterState(snapshotId1, repoName); expectThrows( - SnapshotDeletionException.class, - () -> SnapshotUtils.validateSnapshotsBackingAnyIndex(clusterState, List.of(snapshotId1), repoName) + SnapshotInUseDeletionException.class, + () -> SnapshotUtils.validateSnapshotsBackingAnyIndex(getIndexMetadata(snapshotId1, repoName), List.of(snapshotId1), repoName) ); } - private static ClusterState getClusterState(SnapshotId snapshotId, String repoName) { + private static ImmutableOpenMap getIndexMetadata(SnapshotId snapshotId, String repoName) { final String index = "test-index"; Snapshot snapshot = new Snapshot(repoName, snapshotId); - final Metadata.Builder metaBuilder = Metadata.builder(Metadata.EMPTY_METADATA); - metaBuilder.put( - IndexMetadata.builder(index) - .settings( - Settings.builder() - .put(SETTING_VERSION_CREATED, Version.CURRENT.id) - .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) - .put(IndexSettings.SEARCHABLE_SNAPSHOT_REPOSITORY.getKey(), snapshot.getRepository()) - .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey(), snapshot.getSnapshotId().getUUID()) - .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.getKey(), snapshot.getSnapshotId().getName()) - ) - .numberOfShards(1) - .numberOfReplicas(1) - .build(), - false - ); - return ClusterState.builder(ClusterState.EMPTY_STATE).metadata(metaBuilder).build(); + final Metadata.Builder builder = Metadata.builder(); + builder.put(createIndexMetadata(new Index(index, "uuid"), snapshot), true); + return builder.build().getIndices(); + } + + private static IndexMetadata createIndexMetadata(final Index index, Snapshot snapshot) { + final Settings settings = Settings.builder() + .put(SETTING_VERSION_CREATED, Version.CURRENT.id) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.REMOTE_SNAPSHOT.getSettingsKey()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_REPOSITORY.getKey(), snapshot.getRepository()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey(), snapshot.getSnapshotId().getUUID()) + .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.getKey(), snapshot.getSnapshotId().getName()) + .build(); + return IndexMetadata.builder(index.getName()) + .settings(settings) + .numberOfShards(1) + .numberOfReplicas(0) + .build(); } } From eb73220835c182bc8494902075f33e247c56512a Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Tue, 22 Nov 2022 15:11:49 -0800 Subject: [PATCH 12/14] PR review implementation Signed-off-by: Vishal Sarda --- server/src/main/java/org/opensearch/OpenSearchException.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/OpenSearchException.java b/server/src/main/java/org/opensearch/OpenSearchException.java index f865acb88d413..aef098403ec2b 100644 --- a/server/src/main/java/org/opensearch/OpenSearchException.java +++ b/server/src/main/java/org/opensearch/OpenSearchException.java @@ -1613,7 +1613,7 @@ private enum OpenSearchExceptionHandle { 165, Version.V_2_4_0 ), - SNAPSHOT_DELETION_EXCEPTION( + SNAPSHOT_IN_USE_DELETION_EXCEPTION( SnapshotInUseDeletionException.class, SnapshotInUseDeletionException::new, 166, From e5ec3aac9b6ae506c6b35b4d5179eb104edd265c Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Tue, 22 Nov 2022 15:56:55 -0800 Subject: [PATCH 13/14] Gradle spotless Signed-off-by: Vishal Sarda --- .../org/opensearch/snapshots/SearchableSnapshotIT.java | 7 +------ .../main/java/org/opensearch/snapshots/SnapshotUtils.java | 6 +++++- .../java/org/opensearch/snapshots/SnapshotUtilsTests.java | 6 +----- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java index 59ead7e98f03e..53b70aa915a37 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/SearchableSnapshotIT.java @@ -270,12 +270,7 @@ private void createIndexWithDocsAndEnsureGreen(int numReplicasIndex, int numOfDo ensureGreen(); } - private void takeSnapshot( - Client client, - String snapshotName, - String repoName, - String... indices - ) { + private void takeSnapshot(Client client, String snapshotName, String repoName, String... indices) { logger.info("--> Take a snapshot"); final CreateSnapshotResponse createSnapshotResponse = client.admin() .cluster() diff --git a/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java b/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java index faf981738878f..3ef3523961df8 100644 --- a/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java +++ b/server/src/main/java/org/opensearch/snapshots/SnapshotUtils.java @@ -149,7 +149,11 @@ public static List filterIndices(List availableIndices, String[] * @param snapshotIds list of snapshot Ids to be verified * @param repoName repo name for which the verification is being done */ - public static void validateSnapshotsBackingAnyIndex(ImmutableOpenMap metadata, List snapshotIds, String repoName) { + public static void validateSnapshotsBackingAnyIndex( + ImmutableOpenMap metadata, + List snapshotIds, + String repoName + ) { final Map uuidToSnapshotId = new HashMap<>(); final Set snapshotsToBeNotDeleted = new HashSet<>(); snapshotIds.forEach(snapshotId -> uuidToSnapshotId.put(snapshotId.getUUID(), snapshotId)); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java index 15c1d152e7e64..8dae5026a18bc 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotUtilsTests.java @@ -127,10 +127,6 @@ private static IndexMetadata createIndexMetadata(final Index index, Snapshot sna .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_UUID.getKey(), snapshot.getSnapshotId().getUUID()) .put(IndexSettings.SEARCHABLE_SNAPSHOT_ID_NAME.getKey(), snapshot.getSnapshotId().getName()) .build(); - return IndexMetadata.builder(index.getName()) - .settings(settings) - .numberOfShards(1) - .numberOfReplicas(0) - .build(); + return IndexMetadata.builder(index.getName()).settings(settings).numberOfShards(1).numberOfReplicas(0).build(); } } From 575ff53978499b450f94650822b0f82712ba4ad2 Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Tue, 22 Nov 2022 18:22:28 -0800 Subject: [PATCH 14/14] Updating CHANGELOG Signed-off-by: Vishal Sarda --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90031a016137c..b756307a727d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Apply reproducible builds configuration for OpenSearch plugins through gradle plugin ([#4746](https://github.com/opensearch-project/OpenSearch/pull/4746)) - Add project health badges to the README.md ([#4843](https://github.com/opensearch-project/OpenSearch/pull/4843)) - [Test] Add IAE test for deprecated edgeNGram analyzer name ([#5040](https://github.com/opensearch-project/OpenSearch/pull/5040)) -- Prevent deletion of snapshots that are backing searchable snapshot indexes ([#5069](https://github.com/opensearch-project/OpenSearch/pull/5069)) - Allow mmap to use new JDK-19 preview APIs in Apache Lucene 9.4+ ([#5151](https://github.com/opensearch-project/OpenSearch/pull/5151)) - Add feature flag for extensions ([#5211](https://github.com/opensearch-project/OpenSearch/pull/5211)) @@ -76,6 +75,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased 2.x] ### Added +- Prevent deletion of snapshots that are backing searchable snapshot indexes ([#5069](https://github.com/opensearch-project/OpenSearch/pull/5069)) + ### Dependencies - Bumps `bcpg-fips` from 1.0.5.1 to 1.0.7.1 - Bumps `azure-storage-blob` from 12.16.1 to 12.20.0 ([#4995](https://github.com/opensearch-project/OpenSearch/pull/4995))