From fb6d33defebcea65c76adc7571460c32e6c699df Mon Sep 17 00:00:00 2001 From: Vishal Sarda Date: Fri, 18 Nov 2022 16:03:32 -0800 Subject: [PATCH] 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(); + } }