From 88f73872534a490b8052290a93775f6c827a45e1 Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Fri, 11 Oct 2024 15:30:36 -0700 Subject: [PATCH 01/12] Add new parameters to snapshot restore to rename the restored aliases similar to the existing parameters to rename indexes Signed-off-by: Spencer G. Jones --- CHANGELOG.md | 2 +- .../restore/RestoreSnapshotRequest.java | 69 ++++++++++++++++++ .../RestoreSnapshotRequestBuilder.java | 28 ++++++++ .../opensearch/snapshots/RestoreService.java | 28 ++++++-- .../restore/RestoreSnapshotRequestTests.java | 6 ++ .../snapshots/SnapshotRequestsTests.java | 4 ++ .../snapshots/SnapshotResiliencyTests.java | 70 +++++++++++++++++-- 7 files changed, 195 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76b65a6cd70dc..b079d903a8639 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Latency and Memory allocation improvements to Multi Term Aggregation queries ([#14993](https://github.com/opensearch-project/OpenSearch/pull/14993)) - Flat object field use IndexOrDocValuesQuery to optimize query ([#14383](https://github.com/opensearch-project/OpenSearch/issues/14383)) - Add method to return dynamic SecureTransportParameters from SecureTransportSettingsProvider interface ([#16387](https://github.com/opensearch-project/OpenSearch/pull/16387) -- [Star Tree - Search] Add support for metric aggregations with/without term query ([15289](https://github.com/opensearch-project/OpenSearch/pull/15289)) +- Add support for renaming aliases during snapshot restore ([XXXX](https://github.com/opensearch-project/OpenSearch/pull/XXXX)) ### Dependencies - Bump `com.azure:azure-identity` from 1.13.0 to 1.13.2 ([#15578](https://github.com/opensearch-project/OpenSearch/pull/15578)) diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java index f3110cc8f20a5..4275c8ba831d5 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java @@ -112,6 +112,8 @@ private static StorageType fromString(String string) { private IndicesOptions indicesOptions = IndicesOptions.strictExpandOpen(); private String renamePattern; private String renameReplacement; + private String renameAliasPattern; + private String renameAliasReplacement; private boolean waitForCompletion; private boolean includeGlobalState = false; private boolean partial = false; @@ -148,6 +150,8 @@ public RestoreSnapshotRequest(StreamInput in) throws IOException { indicesOptions = IndicesOptions.readIndicesOptions(in); renamePattern = in.readOptionalString(); renameReplacement = in.readOptionalString(); + renameAliasPattern = in.readOptionalString(); + renameAliasReplacement = in.readOptionalString(); waitForCompletion = in.readBoolean(); includeGlobalState = in.readBoolean(); partial = in.readBoolean(); @@ -175,6 +179,8 @@ public void writeTo(StreamOutput out) throws IOException { indicesOptions.writeIndicesOptions(out); out.writeOptionalString(renamePattern); out.writeOptionalString(renameReplacement); + out.writeOptionalString(renameAliasPattern); + out.writeOptionalString(renameAliasReplacement); out.writeBoolean(waitForCompletion); out.writeBoolean(includeGlobalState); out.writeBoolean(partial); @@ -361,6 +367,51 @@ public String renameReplacement() { return renameReplacement; } + /** + * Sets rename pattern that should be applied to restored indices' alias. + *

+ * Alias that match the rename pattern will be renamed according to {@link #renameAliasReplacement(String)}. The + * rename pattern is applied according to the {@link java.util.regex.Matcher#appendReplacement(StringBuffer, String)} + * The request will fail if two or more alias will be renamed into the same name. + * + * @param renameAliasPattern rename pattern + * @return this request + */ + public RestoreSnapshotRequest renameAliasPattern(String renameAliasPattern) { + this.renameAliasPattern = renameAliasPattern; + return this; + } + + /** + * Returns rename alias pattern + * + * @return rename alias pattern + */ + public String renameAliasPattern() { + return renameAliasPattern; + } + + /** + * Sets rename alias replacement + *

+ * See {@link #renameAliasPattern(String)} for more information. + * + * @param renameAliasReplacement rename replacement + */ + public RestoreSnapshotRequest renameAliasReplacement(String renameAliasReplacement) { + this.renameAliasReplacement = renameAliasReplacement; + return this; + } + + /** + * Returns rename alias replacement + * + * @return rename alias replacement + */ + public String renameAliasReplacement() { + return renameAliasReplacement; + } + /** * If this parameter is set to true the operation will wait for completion of restore process before returning. * @@ -625,6 +676,18 @@ public RestoreSnapshotRequest source(Map source) { } else { throw new IllegalArgumentException("malformed rename_replacement"); } + } else if (name.equals("rename_alias_pattern")) { + if (entry.getValue() instanceof String) { + renameAliasPattern((String) entry.getValue()); + } else { + throw new IllegalArgumentException("malformed rename_alias_pattern"); + } + } else if (name.equals("rename_alias_replacement")) { + if (entry.getValue() instanceof String) { + renameAliasReplacement((String) entry.getValue()); + } else { + throw new IllegalArgumentException("malformed rename_alias_replacement"); + } } else if (name.equals("index_settings")) { if (!(entry.getValue() instanceof Map)) { throw new IllegalArgumentException("malformed index_settings section"); @@ -685,6 +748,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (renameReplacement != null) { builder.field("rename_replacement", renameReplacement); } + if (renameAliasPattern != null) { + builder.field("rename_alias_pattern", renameAliasPattern); + } + if (renameAliasReplacement != null) { + builder.field("rename_alias_replacement", renameAliasReplacement); + } builder.field("include_global_state", includeGlobalState); builder.field("partial", partial); builder.field("include_aliases", includeAliases); diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilder.java b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilder.java index 53c9557a621b7..038d62ad7f4cb 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilder.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilder.java @@ -144,6 +144,34 @@ public RestoreSnapshotRequestBuilder setRenameReplacement(String renameReplaceme return this; } + /** + * Sets rename pattern that should be applied to restored indices' aliases. + *

+ * Aliases that match the rename pattern will be renamed according to {@link #setRenameAliasReplacement(String)}. The + * rename pattern is applied according to the {@link java.util.regex.Matcher#appendReplacement(StringBuffer, String)} + * The request will fail if two or more alias will be renamed into the same name. + * + * @param renameAliasPattern rename alias pattern + * @return this builder + */ + public RestoreSnapshotRequestBuilder setRenameAliasPattern(String renameAliasPattern) { + request.renameAliasPattern(renameAliasPattern); + return this; + } + + /** + * Sets rename replacement + *

+ * See {@link #setRenameAliasPattern(String)} for more information. + * + * @param renameAliasReplacement rename alias replacement + * @return this builder + */ + public RestoreSnapshotRequestBuilder setRenameAliasReplacement(String renameAliasReplacement) { + request.renameAliasReplacement(renameAliasReplacement); + return this; + } + /** * If this parameter is set to true the operation will wait for completion of restore process before returning. * diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index 79a70d835f773..a5326c3a7dec0 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -486,8 +486,18 @@ public ClusterState execute(ClusterState currentState) { // Remove all aliases - they shouldn't be restored indexMdBuilder.removeAllAliases(); } else { - for (final String alias : snapshotIndexMetadata.getAliases().keySet()) { - aliases.add(alias); + for (final Map.Entry alias : snapshotIndexMetadata.getAliases().entrySet()) { + String aliasName = alias.getKey(); + if (request.renameAliasPattern() != null && request.renameAliasReplacement() != null) { + indexMdBuilder.removeAlias(aliasName); + aliasName = aliasName.replaceAll( + request.renameAliasPattern(), + request.renameAliasReplacement() + ); + AliasMetadata newAlias = AliasMetadata.newAliasMetadata(alias.getValue(), aliasName); + indexMdBuilder.putAlias(newAlias); + } + aliases.add(aliasName); } } IndexMetadata updatedIndexMetadata = indexMdBuilder.build(); @@ -533,8 +543,18 @@ public ClusterState execute(ClusterState currentState) { indexMdBuilder.putAlias(alias); } } else { - for (final String alias : snapshotIndexMetadata.getAliases().keySet()) { - aliases.add(alias); + for (final Map.Entry alias : snapshotIndexMetadata.getAliases().entrySet()) { + String aliasName = alias.getKey(); + if (request.renameAliasPattern() != null && request.renameAliasReplacement() != null) { + indexMdBuilder.removeAlias(aliasName); + aliasName = aliasName.replaceAll( + request.renameAliasPattern(), + request.renameAliasReplacement() + ); + AliasMetadata newAlias = AliasMetadata.newAliasMetadata(alias.getValue(), aliasName); + indexMdBuilder.putAlias(newAlias); + } + aliases.add(aliasName); } } final Settings.Builder indexSettingsBuilder = Settings.builder() diff --git a/server/src/test/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java b/server/src/test/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java index c3de3413edd13..04cc45f3477c6 100644 --- a/server/src/test/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java +++ b/server/src/test/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java @@ -71,6 +71,12 @@ private RestoreSnapshotRequest randomState(RestoreSnapshotRequest instance) { if (randomBoolean()) { instance.renameReplacement(randomUnicodeOfLengthBetween(1, 100)); } + if (randomBoolean()) { + instance.renameAliasPattern(randomUnicodeOfLengthBetween(1, 100)); + } + if (randomBoolean()) { + instance.renameAliasReplacement(randomUnicodeOfLengthBetween(1, 100)); + } instance.partial(randomBoolean()); instance.includeAliases(randomBoolean()); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotRequestsTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotRequestsTests.java index a00c74f669eac..87ab95fef6a53 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotRequestsTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotRequestsTests.java @@ -77,6 +77,8 @@ public void testRestoreSnapshotRequestParsing() throws IOException { builder.field("allow_no_indices", indicesOptions.allowNoIndices()); builder.field("rename_pattern", "rename-from"); builder.field("rename_replacement", "rename-to"); + builder.field("rename_alias_pattern", "alias-rename-from"); + builder.field("rename_alias_replacement", "alias-rename-to"); boolean partial = randomBoolean(); builder.field("partial", partial); builder.startObject("settings").field("set1", "val1").endObject(); @@ -103,6 +105,8 @@ public void testRestoreSnapshotRequestParsing() throws IOException { assertArrayEquals(request.indices(), new String[] { "foo", "bar", "baz" }); assertEquals("rename-from", request.renamePattern()); assertEquals("rename-to", request.renameReplacement()); + assertEquals("alias-rename-from", request.renameAliasPattern()); + assertEquals("alias-rename-to", request.renameAliasReplacement()); assertEquals(partial, request.partial()); assertArrayEquals(request.ignoreIndexSettings(), new String[] { "set2", "set3" }); boolean expectedIgnoreAvailable = includeIgnoreUnavailable diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index d17e661615b0d..d21282ff0441f 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -66,6 +66,9 @@ import org.opensearch.action.admin.cluster.state.ClusterStateRequest; import org.opensearch.action.admin.cluster.state.ClusterStateResponse; import org.opensearch.action.admin.cluster.state.TransportClusterStateAction; +import org.opensearch.action.admin.indices.alias.IndicesAliasesAction; +import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest; +import org.opensearch.action.admin.indices.alias.TransportIndicesAliasesAction; import org.opensearch.action.admin.indices.create.CreateIndexAction; import org.opensearch.action.admin.indices.create.CreateIndexRequest; import org.opensearch.action.admin.indices.create.CreateIndexResponse; @@ -141,6 +144,7 @@ import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.metadata.MetadataCreateIndexService; import org.opensearch.cluster.metadata.MetadataDeleteIndexService; +import org.opensearch.cluster.metadata.MetadataIndexAliasesService; import org.opensearch.cluster.metadata.MetadataIndexUpgradeService; import org.opensearch.cluster.metadata.MetadataMappingService; import org.opensearch.cluster.node.DiscoveryNode; @@ -958,6 +962,7 @@ public void testConcurrentSnapshotRestoreAndDeleteOther() { String repoName = "repo"; String snapshotName = "snapshot"; final String index = "test"; + final String alias = "test_alias"; final int shards = randomIntBetween(1, 10); TestClusterNodes.TestClusterNode clusterManagerNode = testClusterNodes.currentClusterManager( @@ -967,9 +972,8 @@ public void testConcurrentSnapshotRestoreAndDeleteOther() { final StepListener createSnapshotResponseStepListener = new StepListener<>(); final int documentsFirstSnapshot = randomIntBetween(0, 100); - continueOrDie( - createRepoAndIndex(repoName, index, shards), + createRepoAndIndexAndAlias(repoName, index, shards, alias), createIndexResponse -> indexNDocuments( documentsFirstSnapshot, index, @@ -1009,19 +1013,27 @@ public void testConcurrentSnapshotRestoreAndDeleteOther() { .cluster() .restoreSnapshot( new RestoreSnapshotRequest(repoName, secondSnapshotName).waitForCompletion(true) + .includeAliases(true) .renamePattern("(.+)") - .renameReplacement("restored_$1"), + .renameReplacement("restored_$1") + .renameAliasPattern("(.+)") + .renameAliasReplacement("restored_alias_$1"), restoreSnapshotResponseListener ) ); }); - final StepListener searchResponseListener = new StepListener<>(); + final StepListener searchIndexResponseListener = new StepListener<>(); + final StepListener searchAliasResponseListener = new StepListener<>(); continueOrDie(restoreSnapshotResponseListener, restoreSnapshotResponse -> { assertEquals(shards, restoreSnapshotResponse.getRestoreInfo().totalShards()); client().search( new SearchRequest("restored_" + index).source(new SearchSourceBuilder().size(0).trackTotalHits(true)), - searchResponseListener + searchIndexResponseListener + ); + client().search( + new SearchRequest("restored_alias_" + alias).source(new SearchSourceBuilder().size(0).trackTotalHits(true)), + searchAliasResponseListener ); }); @@ -1029,7 +1041,11 @@ public void testConcurrentSnapshotRestoreAndDeleteOther() { assertEquals( documentsFirstSnapshot + documentsSecondSnapshot, - Objects.requireNonNull(searchResponseListener.result().getHits().getTotalHits()).value + Objects.requireNonNull(searchIndexResponseListener.result().getHits().getTotalHits()).value + ); + assertEquals( + documentsFirstSnapshot + documentsSecondSnapshot, + Objects.requireNonNull(searchAliasResponseListener.result().getHits().getTotalHits()).value ); assertThat(deleteSnapshotStepListener.result().isAcknowledged(), is(true)); assertThat(restoreSnapshotResponseListener.result().getRestoreInfo().failedShards(), is(0)); @@ -1520,6 +1536,22 @@ private StepListener createRepoAndIndex(String repoName, St return createIndexResponseStepListener; } + private StepListener createRepoAndIndexAndAlias(String repoName, String index, int shards, String alias) { + final StepListener createAliasListener = new StepListener<>(); + + continueOrDie( + createRepoAndIndex(repoName, index, shards), + acknowledgedResponse -> client().admin() + .indices() + .aliases( + new IndicesAliasesRequest().addAliasAction(IndicesAliasesRequest.AliasActions.add().index(index).alias(alias)), + createAliasListener + ) + ); + + return createAliasListener; + } + private void clearDisruptionsAndAwaitSync() { testClusterNodes.clearNetworkDisruptions(); stabilize(); @@ -2171,6 +2203,30 @@ public void onFailure(final Exception e) { indexNameExpressionResolver ) ); + final MetadataDeleteIndexService metadataDeleteIndexService = new MetadataDeleteIndexService( + settings, + clusterService, + allocationService + ); + final MetadataIndexAliasesService metadataIndexAliasesService = new MetadataIndexAliasesService( + clusterService, + indicesService, + new AliasValidator(), + metadataDeleteIndexService, + namedXContentRegistry + ); + actions.put( + IndicesAliasesAction.INSTANCE, + new TransportIndicesAliasesAction( + transportService, + clusterService, + threadPool, + metadataIndexAliasesService, + actionFilters, + indexNameExpressionResolver, + new RequestValidators<>(Collections.emptyList()) + ) + ); final MappingUpdatedAction mappingUpdatedAction = new MappingUpdatedAction(settings, clusterSettings, clusterService); mappingUpdatedAction.setClient(client); final TransportShardBulkAction transportShardBulkAction = new TransportShardBulkAction( @@ -2337,7 +2393,7 @@ public void onFailure(final Exception e) { transportService, clusterService, threadPool, - new MetadataDeleteIndexService(settings, clusterService, allocationService), + metadataDeleteIndexService, actionFilters, indexNameExpressionResolver, new DestructiveOperations(settings, clusterSettings) From 6f1632397af04c7f592504afff894cc82536c83e Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Fri, 11 Oct 2024 16:28:30 -0700 Subject: [PATCH 02/12] Fix comment. Update changelog. Signed-off-by: Spencer G. Jones --- CHANGELOG.md | 2 +- .../admin/cluster/snapshots/restore/RestoreSnapshotRequest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b079d903a8639..a8df628813839 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Latency and Memory allocation improvements to Multi Term Aggregation queries ([#14993](https://github.com/opensearch-project/OpenSearch/pull/14993)) - Flat object field use IndexOrDocValuesQuery to optimize query ([#14383](https://github.com/opensearch-project/OpenSearch/issues/14383)) - Add method to return dynamic SecureTransportParameters from SecureTransportSettingsProvider interface ([#16387](https://github.com/opensearch-project/OpenSearch/pull/16387) -- Add support for renaming aliases during snapshot restore ([XXXX](https://github.com/opensearch-project/OpenSearch/pull/XXXX)) +- Add support for renaming aliases during snapshot restore ([#16292](https://github.com/opensearch-project/OpenSearch/pull/16292)) ### Dependencies - Bump `com.azure:azure-identity` from 1.13.0 to 1.13.2 ([#15578](https://github.com/opensearch-project/OpenSearch/pull/15578)) diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java index 4275c8ba831d5..c511465e05f61 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java @@ -372,7 +372,7 @@ public String renameReplacement() { *

* Alias that match the rename pattern will be renamed according to {@link #renameAliasReplacement(String)}. The * rename pattern is applied according to the {@link java.util.regex.Matcher#appendReplacement(StringBuffer, String)} - * The request will fail if two or more alias will be renamed into the same name. + * If two or more aliases are renamed into the same name, they will be merged. * * @param renameAliasPattern rename pattern * @return this request From ca22aaacd90c5836be7ddac75808df72bb6d01fb Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Sun, 13 Oct 2024 20:09:43 -0700 Subject: [PATCH 03/12] New parameters needs to only used for new version Signed-off-by: Spencer G. Jones --- .../restore/RestoreSnapshotRequest.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java index c511465e05f61..2b660a4baf5d0 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java @@ -150,8 +150,6 @@ public RestoreSnapshotRequest(StreamInput in) throws IOException { indicesOptions = IndicesOptions.readIndicesOptions(in); renamePattern = in.readOptionalString(); renameReplacement = in.readOptionalString(); - renameAliasPattern = in.readOptionalString(); - renameAliasReplacement = in.readOptionalString(); waitForCompletion = in.readBoolean(); includeGlobalState = in.readBoolean(); partial = in.readBoolean(); @@ -168,6 +166,12 @@ public RestoreSnapshotRequest(StreamInput in) throws IOException { if (in.getVersion().onOrAfter(Version.V_2_17_0)) { sourceRemoteTranslogRepository = in.readOptionalString(); } + if (in.getVersion().onOrAfter(Version.V_3_0_0)) { + renameAliasPattern = in.readOptionalString(); + } + if (in.getVersion().onOrAfter(Version.V_3_0_0)) { + renameAliasReplacement = in.readOptionalString(); + } } @Override @@ -179,8 +183,6 @@ public void writeTo(StreamOutput out) throws IOException { indicesOptions.writeIndicesOptions(out); out.writeOptionalString(renamePattern); out.writeOptionalString(renameReplacement); - out.writeOptionalString(renameAliasPattern); - out.writeOptionalString(renameAliasReplacement); out.writeBoolean(waitForCompletion); out.writeBoolean(includeGlobalState); out.writeBoolean(partial); @@ -197,6 +199,12 @@ public void writeTo(StreamOutput out) throws IOException { if (out.getVersion().onOrAfter(Version.V_2_17_0)) { out.writeOptionalString(sourceRemoteTranslogRepository); } + if (out.getVersion().onOrAfter(Version.V_3_0_0)) { + out.writeOptionalString(renameAliasPattern); + } + if (out.getVersion().onOrAfter(Version.V_3_0_0)) { + out.writeOptionalString(renameAliasReplacement); + } } @Override From 19f2801d961f7154fbd31339e8d9ef14bde3dc82 Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Sun, 13 Oct 2024 20:10:07 -0700 Subject: [PATCH 04/12] Add missing equals and hash implemenation for new parameters Signed-off-by: Spencer G. Jones --- .../cluster/snapshots/restore/RestoreSnapshotRequest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java index 2b660a4baf5d0..981622c535ee3 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java @@ -810,6 +810,8 @@ public boolean equals(Object o) { && Objects.equals(indicesOptions, that.indicesOptions) && Objects.equals(renamePattern, that.renamePattern) && Objects.equals(renameReplacement, that.renameReplacement) + && Objects.equals(renameAliasPattern, that.renameAliasPattern) + && Objects.equals(renameAliasReplacement, that.renameAliasReplacement) && Objects.equals(indexSettings, that.indexSettings) && Arrays.equals(ignoreIndexSettings, that.ignoreIndexSettings) && Objects.equals(snapshotUuid, that.snapshotUuid) @@ -828,6 +830,8 @@ public int hashCode() { indicesOptions, renamePattern, renameReplacement, + renameAliasPattern, + renameAliasReplacement, waitForCompletion, includeGlobalState, partial, From d1311aca61fe86b67f1eac156f10cec9840fbe48 Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Mon, 14 Oct 2024 08:12:03 -0700 Subject: [PATCH 05/12] Add some tests Signed-off-by: Spencer G. Jones --- .../snapshots/RestoreServiceIntegTests.java | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java diff --git a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java new file mode 100644 index 0000000000000..7676935e437db --- /dev/null +++ b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java @@ -0,0 +1,129 @@ +/* + * 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.action.StepListener; +import org.opensearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; +import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotRequest; +import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; +import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest; +import org.opensearch.common.CheckedConsumer; +import org.opensearch.common.settings.Settings; +import org.opensearch.repositories.fs.FsRepository; +import org.opensearch.test.OpenSearchIntegTestCase; +import org.opensearch.test.OpenSearchSingleNodeTestCase; + +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Supplier; +//import static org.opensearch.node.Node.NODE_NAME_SETTING; + +public class RestoreServiceIntegTests extends OpenSearchSingleNodeTestCase { + // private DeterministicTaskQueue deterministicTaskQueue; + + // @Before + // public void createServices() { + // deterministicTaskQueue = new DeterministicTaskQueue(Settings.builder().put(NODE_NAME_SETTING.getKey(), "shared").build(), random()); + // } + + // TODO there is certainly a better way to do this, but I don't know what it is.... + public void testRestoreWithAliasAndRename() { + __testRestoreWithRename(false, true, true, true); + } + + public void testRestoreWithoutAliasAndWithRename() { + __testRestoreWithRename(false, false, true, true); + } + + public void testRestoreWithoutAliasAndRename() { + __testRestoreWithRename(false, false, false, false); + } + + public void testRestoreWithAliasAndWithoutRename() { + __testRestoreWithRename(false, true, false, false); + } + + public void __testRestoreWithRename(boolean closed, boolean includeAlias, boolean renameAlias, boolean renameIndexes) { + final String indexName = "index_1"; + final String aliasName = "alias_1"; + final String repoName = "repo_1"; + final String snapShotName = "snap_1"; + this.createIndex(indexName); + client().admin() + .indices() + .aliases( + new IndicesAliasesRequest().addAliasAction(IndicesAliasesRequest.AliasActions.add().alias(aliasName).index(indexName)) + ); + + final boolean expectFailure = !renameIndexes || (includeAlias && !renameAlias); + + Settings.Builder settings = Settings.builder().put("location", randomAlphaOfLength(10)); + OpenSearchIntegTestCase.putRepository(client().admin().cluster(), repoName, FsRepository.TYPE, settings); + + final StepListener createSnapshotResponseStepListener = new StepListener<>(); + + client().admin() + .cluster() + .prepareCreateSnapshot(repoName, snapShotName) + .setWaitForCompletion(true) + .setPartial(true) + .execute(createSnapshotResponseStepListener); + + final StepListener restoreSnapshotResponseStepListener = new StepListener<>(); + + final AtomicBoolean isFinished = new AtomicBoolean(false); + continueOrDie(createSnapshotResponseStepListener, r -> { + assert r.getSnapshotInfo().state() == SnapshotState.SUCCESS; + RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repoName, snapShotName).includeAliases(includeAlias); + if (renameAlias) { + restoreSnapshotRequest = restoreSnapshotRequest.renameAliasPattern("1").renameAliasReplacement("2"); + } + if (renameIndexes) { + restoreSnapshotRequest = restoreSnapshotRequest.renamePattern("1").renameReplacement("2"); + } + client().admin().cluster().restoreSnapshot(restoreSnapshotRequest, restoreSnapshotResponseStepListener); + }); + + restoreSnapshotResponseStepListener.whenComplete(r -> { + assertFalse("unexpected sucesssful restore", expectFailure); + isFinished.set(true); + }, e -> { + if (expectFailure) { + // expected failed - ignore + isFinished.set(true); + } else { + throw new RuntimeException(e); + } + }); + + runUntil(isFinished::get, TimeUnit.MINUTES.toMillis(1L)); + } + + private static void continueOrDie(StepListener listener, CheckedConsumer onResponse) { + listener.whenComplete(onResponse, e -> { throw new AssertionError(e); }); + } + + // TODO there is certainly a better way to do this, but I don't know what it is.... + private void runUntil(Supplier fulfilled, long timeout) { + // final long start = deterministicTaskQueue.getCurrentTimeMillis(); + // while (timeout > deterministicTaskQueue.getCurrentTimeMillis() - start) { + final long start = System.currentTimeMillis(); + while (timeout > System.currentTimeMillis() - start) { + if (fulfilled.get()) { + return; + } + try { + Thread.sleep(10); + } catch (InterruptedException e) {} + // deterministicTaskQueue.runAllRunnableTasks(); + // deterministicTaskQueue.advanceTime(); + } + fail("Condition wasn't fulfilled."); + } +} From 6d565c0de7eb321a95692d82cb2e0b170119f4e7 Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Mon, 14 Oct 2024 10:23:17 -0700 Subject: [PATCH 06/12] Add some more tests Signed-off-by: Spencer G. Jones --- .../snapshots/RestoreServiceIntegTests.java | 157 +++++++++++++++--- 1 file changed, 138 insertions(+), 19 deletions(-) diff --git a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java index 7676935e437db..46f2ef6f38380 100644 --- a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java +++ b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java @@ -13,14 +13,29 @@ import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotRequest; import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest; +import org.opensearch.action.admin.indices.close.CloseIndexRequest; +import org.opensearch.action.admin.indices.close.CloseIndexResponse; +import org.opensearch.action.admin.indices.open.OpenIndexRequest; +import org.opensearch.action.admin.indices.open.OpenIndexResponse; +import org.opensearch.action.bulk.BulkRequest; +import org.opensearch.action.bulk.BulkResponse; +import org.opensearch.action.index.IndexRequest; +import org.opensearch.action.search.SearchRequest; +import org.opensearch.action.search.SearchResponse; +import org.opensearch.action.support.WriteRequest; +import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.common.CheckedConsumer; import org.opensearch.common.settings.Settings; import org.opensearch.repositories.fs.FsRepository; +import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.test.OpenSearchSingleNodeTestCase; +import java.util.Collections; +import java.util.Objects; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; //import static org.opensearch.node.Node.NODE_NAME_SETTING; @@ -33,39 +48,106 @@ public class RestoreServiceIntegTests extends OpenSearchSingleNodeTestCase { // } // TODO there is certainly a better way to do this, but I don't know what it is.... - public void testRestoreWithAliasAndRename() { - __testRestoreWithRename(false, true, true, true); + public void testRestoreToNewWithAliasAndRename() { + __testRestoreWithRename(false, false, true, true, true); } - public void testRestoreWithoutAliasAndWithRename() { - __testRestoreWithRename(false, false, true, true); + public void testRestoreToNewWithoutAliasAndWithRename() { + __testRestoreWithRename(false, false, false, true, true); } - public void testRestoreWithoutAliasAndRename() { - __testRestoreWithRename(false, false, false, false); + public void testRestoreOverExistingOpenWithAliasAndRename() { + __testRestoreWithRename(true, false, true, true, true); } - public void testRestoreWithAliasAndWithoutRename() { - __testRestoreWithRename(false, true, false, false); + public void testRestoreOverExistingOpenWithoutAliasAndWithRename() { + __testRestoreWithRename(true, false, false, true, true); } - public void __testRestoreWithRename(boolean closed, boolean includeAlias, boolean renameAlias, boolean renameIndexes) { + public void testRestoreOverExistingClosedWithAliasAndRename() { + __testRestoreWithRename(true, true, true, true, true); + } + + public void testRestoreOverExistingClosedWithoutAliasAndWithRename() { + __testRestoreWithRename(true, true, false, true, true); + } + + public void testRestoreOverExistingOpenWithoutAliasAndRename() { + __testRestoreWithRename(true, false, false, false, false); + } + + public void testRestoreOverExistingOpenWithAliasAndWithoutRename() { + __testRestoreWithRename(true, false, true, false, false); + } + + public void testRestoreOverExistingClosedWithoutAliasAndRename() { + __testRestoreWithRename(true, true, false, false, false); + } + + public void testRestoreOverExistingClosedWithAliasAndWithoutRename() { + __testRestoreWithRename(true, true, true, false, false); + } + + public void __testRestoreWithRename( + boolean exists, + boolean closed, + boolean includeAlias, + boolean renameAliases, + boolean renameIndexes + ) { + assert exists || renameIndexes; final String indexName = "index_1"; + final String renamedIndexName = "index_2"; final String aliasName = "alias_1"; + final String renamedAliasName = "alias_2"; final String repoName = "repo_1"; final String snapShotName = "snap_1"; + final boolean expectSuccess = !exists || closed; + final int documents = randomIntBetween(1, 100); + this.createIndex(indexName); + if (exists && renameIndexes) { + this.createIndex(renamedIndexName); + } + + final StepListener createAliasResponseStepListener = new StepListener<>(); client().admin() .indices() .aliases( - new IndicesAliasesRequest().addAliasAction(IndicesAliasesRequest.AliasActions.add().alias(aliasName).index(indexName)) + new IndicesAliasesRequest().addAliasAction(IndicesAliasesRequest.AliasActions.add().alias(aliasName).index(indexName)), + createAliasResponseStepListener ); - final boolean expectFailure = !renameIndexes || (includeAlias && !renameAlias); + final AtomicBoolean isDocumentFinished = new AtomicBoolean(false); + continueOrDie(createAliasResponseStepListener, ignored -> { + final BulkRequest bulkRequest = new BulkRequest().setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE); + for (int i = 0; i < documents; ++i) { + bulkRequest.add(new IndexRequest(indexName).source(Collections.singletonMap("foo", "bar" + i))); + } + final StepListener bulkResponseStepListener = new StepListener<>(); + client().bulk(bulkRequest, bulkResponseStepListener); + continueOrDie(bulkResponseStepListener, bulkResponse -> { + assertFalse("Failures in bulk response: " + bulkResponse.buildFailureMessage(), bulkResponse.hasFailures()); + assertEquals(documents, bulkResponse.getItems().length); + isDocumentFinished.set(true); + }); + }); Settings.Builder settings = Settings.builder().put("location", randomAlphaOfLength(10)); OpenSearchIntegTestCase.putRepository(client().admin().cluster(), repoName, FsRepository.TYPE, settings); + runUntil(isDocumentFinished::get, TimeUnit.MINUTES.toMillis(1L)); + + if (closed) { + final AtomicBoolean isReady = new AtomicBoolean(false); + final StepListener closeIndexResponseStepListener = new StepListener<>(); + final String indexToClose = renameIndexes ? renamedIndexName : indexName; + client().admin().indices().close(new CloseIndexRequest(indexToClose), closeIndexResponseStepListener); + + continueOrDie(closeIndexResponseStepListener, ignored -> { isReady.set(true); }); + runUntil(isReady::get, TimeUnit.MINUTES.toMillis(1L)); + } + final StepListener createSnapshotResponseStepListener = new StepListener<>(); client().admin() @@ -80,8 +162,9 @@ public void __testRestoreWithRename(boolean closed, boolean includeAlias, boolea final AtomicBoolean isFinished = new AtomicBoolean(false); continueOrDie(createSnapshotResponseStepListener, r -> { assert r.getSnapshotInfo().state() == SnapshotState.SUCCESS; - RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repoName, snapShotName).includeAliases(includeAlias); - if (renameAlias) { + RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repoName, snapShotName).includeAliases(includeAlias) + .waitForCompletion(true); + if (renameAliases) { restoreSnapshotRequest = restoreSnapshotRequest.renameAliasPattern("1").renameAliasReplacement("2"); } if (renameIndexes) { @@ -91,18 +174,54 @@ public void __testRestoreWithRename(boolean closed, boolean includeAlias, boolea }); restoreSnapshotResponseStepListener.whenComplete(r -> { - assertFalse("unexpected sucesssful restore", expectFailure); isFinished.set(true); + assertTrue("unexpected sucesssful restore", expectSuccess); }, e -> { - if (expectFailure) { - // expected failed - ignore - isFinished.set(true); - } else { + isFinished.set(true); + if (expectSuccess) { throw new RuntimeException(e); } }); - runUntil(isFinished::get, TimeUnit.MINUTES.toMillis(1L)); + runUntil(isFinished::get, TimeUnit.SECONDS.toMillis(10L)); + + if (expectSuccess) { + // assertEquals(shards, restoreSnapshotResponse.getRestoreInfo().totalShards()); + final String indexToSearch = renameIndexes ? renamedIndexName : indexName; + final String aliasToSearch = renameAliases ? renamedAliasName : aliasName; + + if (closed) { + final AtomicBoolean isReady = new AtomicBoolean(false); + final StepListener openIndexResponseStepListener = new StepListener<>(); + client().admin().indices().open(new OpenIndexRequest(indexToSearch).waitForActiveShards(1), openIndexResponseStepListener); + continueOrDie(openIndexResponseStepListener, ignored -> { isReady.set(true); }); + runUntil(isReady::get, TimeUnit.MINUTES.toMillis(1L)); + } + + final StepListener searchIndexResponseListener = new StepListener<>(); + final StepListener searchAliasResponseListener = new StepListener<>(); + final int expectedCount = includeAlias ? 2 : 1; + final AtomicInteger isSearchDone = new AtomicInteger(0); + client().search( + new SearchRequest(indexToSearch).source(new SearchSourceBuilder().size(0).trackTotalHits(true)), + searchIndexResponseListener + ); + continueOrDie(searchIndexResponseListener, ignored -> { isSearchDone.addAndGet(1); }); + if (includeAlias) { + client().search( + new SearchRequest(aliasToSearch).source(new SearchSourceBuilder().size(0).trackTotalHits(true)), + searchAliasResponseListener + ); + continueOrDie(searchAliasResponseListener, ignored -> { isSearchDone.addAndGet(1); }); + } + + runUntil(() -> { return isSearchDone.get() >= expectedCount; }, TimeUnit.SECONDS.toMillis(10L)); + + assertEquals(documents, Objects.requireNonNull(searchIndexResponseListener.result().getHits().getTotalHits()).value); + if (includeAlias) { + assertEquals(documents, Objects.requireNonNull(searchAliasResponseListener.result().getHits().getTotalHits()).value); + } + } } private static void continueOrDie(StepListener listener, CheckedConsumer onResponse) { From 7d79f3c1ac4d6b5c1976be1e87b870f7eff4d187 Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Mon, 14 Oct 2024 11:10:21 -0700 Subject: [PATCH 07/12] Use CountDownLatch Signed-off-by: Spencer G. Jones --- .../snapshots/RestoreServiceIntegTests.java | 86 +++++++------------ 1 file changed, 31 insertions(+), 55 deletions(-) diff --git a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java index 46f2ef6f38380..6043a0887734a 100644 --- a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java +++ b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java @@ -33,10 +33,8 @@ import java.util.Collections; import java.util.Objects; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Supplier; //import static org.opensearch.node.Node.NODE_NAME_SETTING; public class RestoreServiceIntegTests extends OpenSearchSingleNodeTestCase { @@ -48,53 +46,48 @@ public class RestoreServiceIntegTests extends OpenSearchSingleNodeTestCase { // } // TODO there is certainly a better way to do this, but I don't know what it is.... - public void testRestoreToNewWithAliasAndRename() { + public void testRestoreToNewWithAliasAndRename() throws Exception { __testRestoreWithRename(false, false, true, true, true); } - public void testRestoreToNewWithoutAliasAndWithRename() { + public void testRestoreToNewWithoutAliasAndWithRename() throws Exception { __testRestoreWithRename(false, false, false, true, true); } - public void testRestoreOverExistingOpenWithAliasAndRename() { + public void testRestoreOverExistingOpenWithAliasAndRename() throws Exception { __testRestoreWithRename(true, false, true, true, true); } - public void testRestoreOverExistingOpenWithoutAliasAndWithRename() { + public void testRestoreOverExistingOpenWithoutAliasAndWithRename() throws Exception { __testRestoreWithRename(true, false, false, true, true); } - public void testRestoreOverExistingClosedWithAliasAndRename() { + public void testRestoreOverExistingClosedWithAliasAndRename() throws Exception { __testRestoreWithRename(true, true, true, true, true); } - public void testRestoreOverExistingClosedWithoutAliasAndWithRename() { + public void testRestoreOverExistingClosedWithoutAliasAndWithRename() throws Exception { __testRestoreWithRename(true, true, false, true, true); } - public void testRestoreOverExistingOpenWithoutAliasAndRename() { + public void testRestoreOverExistingOpenWithoutAliasAndRename() throws Exception { __testRestoreWithRename(true, false, false, false, false); } - public void testRestoreOverExistingOpenWithAliasAndWithoutRename() { + public void testRestoreOverExistingOpenWithAliasAndWithoutRename() throws Exception { __testRestoreWithRename(true, false, true, false, false); } - public void testRestoreOverExistingClosedWithoutAliasAndRename() { + public void testRestoreOverExistingClosedWithoutAliasAndRename() throws Exception { __testRestoreWithRename(true, true, false, false, false); } - public void testRestoreOverExistingClosedWithAliasAndWithoutRename() { + public void testRestoreOverExistingClosedWithAliasAndWithoutRename() throws Exception { __testRestoreWithRename(true, true, true, false, false); } - public void __testRestoreWithRename( - boolean exists, - boolean closed, - boolean includeAlias, - boolean renameAliases, - boolean renameIndexes - ) { + public void __testRestoreWithRename(boolean exists, boolean closed, boolean includeAlias, boolean renameAliases, boolean renameIndexes) + throws Exception { assert exists || renameIndexes; final String indexName = "index_1"; final String renamedIndexName = "index_2"; @@ -118,7 +111,7 @@ public void __testRestoreWithRename( createAliasResponseStepListener ); - final AtomicBoolean isDocumentFinished = new AtomicBoolean(false); + final CountDownLatch isDocumentFinished = new CountDownLatch(1); continueOrDie(createAliasResponseStepListener, ignored -> { final BulkRequest bulkRequest = new BulkRequest().setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE); for (int i = 0; i < documents; ++i) { @@ -129,23 +122,23 @@ public void __testRestoreWithRename( continueOrDie(bulkResponseStepListener, bulkResponse -> { assertFalse("Failures in bulk response: " + bulkResponse.buildFailureMessage(), bulkResponse.hasFailures()); assertEquals(documents, bulkResponse.getItems().length); - isDocumentFinished.set(true); + isDocumentFinished.countDown(); }); }); Settings.Builder settings = Settings.builder().put("location", randomAlphaOfLength(10)); OpenSearchIntegTestCase.putRepository(client().admin().cluster(), repoName, FsRepository.TYPE, settings); - runUntil(isDocumentFinished::get, TimeUnit.MINUTES.toMillis(1L)); + isDocumentFinished.await(1, TimeUnit.MINUTES); if (closed) { - final AtomicBoolean isReady = new AtomicBoolean(false); + final CountDownLatch isReady = new CountDownLatch(1); final StepListener closeIndexResponseStepListener = new StepListener<>(); final String indexToClose = renameIndexes ? renamedIndexName : indexName; client().admin().indices().close(new CloseIndexRequest(indexToClose), closeIndexResponseStepListener); - continueOrDie(closeIndexResponseStepListener, ignored -> { isReady.set(true); }); - runUntil(isReady::get, TimeUnit.MINUTES.toMillis(1L)); + continueOrDie(closeIndexResponseStepListener, ignored -> { isReady.countDown(); }); + isReady.await(1, TimeUnit.MINUTES); } final StepListener createSnapshotResponseStepListener = new StepListener<>(); @@ -159,7 +152,7 @@ public void __testRestoreWithRename( final StepListener restoreSnapshotResponseStepListener = new StepListener<>(); - final AtomicBoolean isFinished = new AtomicBoolean(false); + final CountDownLatch isFinished = new CountDownLatch(1); continueOrDie(createSnapshotResponseStepListener, r -> { assert r.getSnapshotInfo().state() == SnapshotState.SUCCESS; RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repoName, snapShotName).includeAliases(includeAlias) @@ -174,16 +167,16 @@ public void __testRestoreWithRename( }); restoreSnapshotResponseStepListener.whenComplete(r -> { - isFinished.set(true); + isFinished.countDown(); assertTrue("unexpected sucesssful restore", expectSuccess); }, e -> { - isFinished.set(true); + isFinished.countDown(); if (expectSuccess) { throw new RuntimeException(e); } }); - runUntil(isFinished::get, TimeUnit.SECONDS.toMillis(10L)); + isFinished.await(1, TimeUnit.MINUTES); if (expectSuccess) { // assertEquals(shards, restoreSnapshotResponse.getRestoreInfo().totalShards()); @@ -191,31 +184,32 @@ public void __testRestoreWithRename( final String aliasToSearch = renameAliases ? renamedAliasName : aliasName; if (closed) { - final AtomicBoolean isReady = new AtomicBoolean(false); + final CountDownLatch isReady = new CountDownLatch(1); final StepListener openIndexResponseStepListener = new StepListener<>(); client().admin().indices().open(new OpenIndexRequest(indexToSearch).waitForActiveShards(1), openIndexResponseStepListener); - continueOrDie(openIndexResponseStepListener, ignored -> { isReady.set(true); }); - runUntil(isReady::get, TimeUnit.MINUTES.toMillis(1L)); + continueOrDie(openIndexResponseStepListener, ignored -> { isReady.countDown(); }); + + isReady.await(1, TimeUnit.MINUTES); } final StepListener searchIndexResponseListener = new StepListener<>(); final StepListener searchAliasResponseListener = new StepListener<>(); final int expectedCount = includeAlias ? 2 : 1; - final AtomicInteger isSearchDone = new AtomicInteger(0); + final CountDownLatch isSearchDone = new CountDownLatch(expectedCount); client().search( new SearchRequest(indexToSearch).source(new SearchSourceBuilder().size(0).trackTotalHits(true)), searchIndexResponseListener ); - continueOrDie(searchIndexResponseListener, ignored -> { isSearchDone.addAndGet(1); }); + continueOrDie(searchIndexResponseListener, ignored -> { isSearchDone.countDown(); }); if (includeAlias) { client().search( new SearchRequest(aliasToSearch).source(new SearchSourceBuilder().size(0).trackTotalHits(true)), searchAliasResponseListener ); - continueOrDie(searchAliasResponseListener, ignored -> { isSearchDone.addAndGet(1); }); + continueOrDie(searchAliasResponseListener, ignored -> { isSearchDone.countDown(); }); } - runUntil(() -> { return isSearchDone.get() >= expectedCount; }, TimeUnit.SECONDS.toMillis(10L)); + isSearchDone.await(1, TimeUnit.MINUTES); assertEquals(documents, Objects.requireNonNull(searchIndexResponseListener.result().getHits().getTotalHits()).value); if (includeAlias) { @@ -227,22 +221,4 @@ public void __testRestoreWithRename( private static void continueOrDie(StepListener listener, CheckedConsumer onResponse) { listener.whenComplete(onResponse, e -> { throw new AssertionError(e); }); } - - // TODO there is certainly a better way to do this, but I don't know what it is.... - private void runUntil(Supplier fulfilled, long timeout) { - // final long start = deterministicTaskQueue.getCurrentTimeMillis(); - // while (timeout > deterministicTaskQueue.getCurrentTimeMillis() - start) { - final long start = System.currentTimeMillis(); - while (timeout > System.currentTimeMillis() - start) { - if (fulfilled.get()) { - return; - } - try { - Thread.sleep(10); - } catch (InterruptedException e) {} - // deterministicTaskQueue.runAllRunnableTasks(); - // deterministicTaskQueue.advanceTime(); - } - fail("Condition wasn't fulfilled."); - } } From 7c8d5b9522fad4a6bbdd7c1801ab2d8956123e8a Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Mon, 14 Oct 2024 12:24:06 -0700 Subject: [PATCH 08/12] Add two more tests. Refactoring and cleanup. Signed-off-by: Spencer G. Jones --- .../snapshots/RestoreServiceIntegTests.java | 109 ++++++++++-------- 1 file changed, 63 insertions(+), 46 deletions(-) diff --git a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java index 6043a0887734a..6177a3fac6cf7 100644 --- a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java +++ b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java @@ -15,6 +15,7 @@ import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest; import org.opensearch.action.admin.indices.close.CloseIndexRequest; import org.opensearch.action.admin.indices.close.CloseIndexResponse; +import org.opensearch.action.admin.indices.delete.DeleteIndexRequest; import org.opensearch.action.admin.indices.open.OpenIndexRequest; import org.opensearch.action.admin.indices.open.OpenIndexResponse; import org.opensearch.action.bulk.BulkRequest; @@ -35,16 +36,8 @@ import java.util.Objects; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; -//import static org.opensearch.node.Node.NODE_NAME_SETTING; public class RestoreServiceIntegTests extends OpenSearchSingleNodeTestCase { - // private DeterministicTaskQueue deterministicTaskQueue; - - // @Before - // public void createServices() { - // deterministicTaskQueue = new DeterministicTaskQueue(Settings.builder().put(NODE_NAME_SETTING.getKey(), "shared").build(), random()); - // } - // TODO there is certainly a better way to do this, but I don't know what it is.... public void testRestoreToNewWithAliasAndRename() throws Exception { __testRestoreWithRename(false, false, true, true, true); @@ -54,6 +47,14 @@ public void testRestoreToNewWithoutAliasAndWithRename() throws Exception { __testRestoreWithRename(false, false, false, true, true); } + public void testRestoreToNewWithAliasAndWithoutRename() throws Exception { + __testRestoreWithRename(false, false, true, false, false); + } + + public void testRestoreToNewWithoutAliasAndRename() throws Exception { + __testRestoreWithRename(false, false, false, false, false); + } + public void testRestoreOverExistingOpenWithAliasAndRename() throws Exception { __testRestoreWithRename(true, false, true, true, true); } @@ -86,9 +87,9 @@ public void testRestoreOverExistingClosedWithAliasAndWithoutRename() throws Exce __testRestoreWithRename(true, true, true, false, false); } - public void __testRestoreWithRename(boolean exists, boolean closed, boolean includeAlias, boolean renameAliases, boolean renameIndexes) + private void __testRestoreWithRename(boolean exists, boolean closed, boolean includeAlias, boolean renameAliases, boolean renameIndexes) throws Exception { - assert exists || renameIndexes; + assert exists || !closed; final String indexName = "index_1"; final String renamedIndexName = "index_2"; final String aliasName = "alias_1"; @@ -103,6 +104,16 @@ public void __testRestoreWithRename(boolean exists, boolean closed, boolean incl this.createIndex(renamedIndexName); } + final StepListener putRepositoryResponseStepListener = new StepListener<>(); + Settings.Builder settings = Settings.builder().put("location", randomAlphaOfLength(10)); + OpenSearchIntegTestCase.putRepository( + client().admin().cluster(), + repoName, + FsRepository.TYPE, + settings, + putRepositoryResponseStepListener + ); + final StepListener createAliasResponseStepListener = new StepListener<>(); client().admin() .indices() @@ -126,76 +137,82 @@ public void __testRestoreWithRename(boolean exists, boolean closed, boolean incl }); }); - Settings.Builder settings = Settings.builder().put("location", randomAlphaOfLength(10)); - OpenSearchIntegTestCase.putRepository(client().admin().cluster(), repoName, FsRepository.TYPE, settings); - isDocumentFinished.await(1, TimeUnit.MINUTES); if (closed) { - final CountDownLatch isReady = new CountDownLatch(1); + final CountDownLatch isClosed = new CountDownLatch(1); final StepListener closeIndexResponseStepListener = new StepListener<>(); final String indexToClose = renameIndexes ? renamedIndexName : indexName; client().admin().indices().close(new CloseIndexRequest(indexToClose), closeIndexResponseStepListener); - continueOrDie(closeIndexResponseStepListener, ignored -> { isReady.countDown(); }); - isReady.await(1, TimeUnit.MINUTES); + continueOrDie(closeIndexResponseStepListener, ignored -> { isClosed.countDown(); }); + isClosed.await(1, TimeUnit.MINUTES); } final StepListener createSnapshotResponseStepListener = new StepListener<>(); + continueOrDie(putRepositoryResponseStepListener, ignored -> { + client().admin() + .cluster() + .prepareCreateSnapshot(repoName, snapShotName) + .setWaitForCompletion(true) + .setPartial(true) + .execute(createSnapshotResponseStepListener); + }); - client().admin() - .cluster() - .prepareCreateSnapshot(repoName, snapShotName) - .setWaitForCompletion(true) - .setPartial(true) - .execute(createSnapshotResponseStepListener); + final CountDownLatch isRestorable = new CountDownLatch(1); - final StepListener restoreSnapshotResponseStepListener = new StepListener<>(); + if (!exists && !renameIndexes) { + final StepListener deleteIndexResponseStepListener = new StepListener<>(); + continueOrDie(createSnapshotResponseStepListener, ignored -> { + client().admin().indices().delete(new DeleteIndexRequest(indexName), deleteIndexResponseStepListener); + }); + continueOrDie(deleteIndexResponseStepListener, ignored -> isRestorable.countDown()); + } else { + continueOrDie(createSnapshotResponseStepListener, ignored -> isRestorable.countDown()); + } - final CountDownLatch isFinished = new CountDownLatch(1); - continueOrDie(createSnapshotResponseStepListener, r -> { - assert r.getSnapshotInfo().state() == SnapshotState.SUCCESS; - RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repoName, snapShotName).includeAliases(includeAlias) - .waitForCompletion(true); - if (renameAliases) { - restoreSnapshotRequest = restoreSnapshotRequest.renameAliasPattern("1").renameAliasReplacement("2"); - } - if (renameIndexes) { - restoreSnapshotRequest = restoreSnapshotRequest.renamePattern("1").renameReplacement("2"); - } - client().admin().cluster().restoreSnapshot(restoreSnapshotRequest, restoreSnapshotResponseStepListener); - }); + isRestorable.await(1, TimeUnit.MINUTES); + + final StepListener restoreSnapshotResponseStepListener = new StepListener<>(); + final CountDownLatch isRestored = new CountDownLatch(1); + RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repoName, snapShotName).includeAliases(includeAlias) + .waitForCompletion(true); + if (renameAliases) { + restoreSnapshotRequest = restoreSnapshotRequest.renameAliasPattern("1").renameAliasReplacement("2"); + } + if (renameIndexes) { + restoreSnapshotRequest = restoreSnapshotRequest.renamePattern("1").renameReplacement("2"); + } + client().admin().cluster().restoreSnapshot(restoreSnapshotRequest, restoreSnapshotResponseStepListener); - restoreSnapshotResponseStepListener.whenComplete(r -> { - isFinished.countDown(); + restoreSnapshotResponseStepListener.whenComplete(ignored -> { + isRestored.countDown(); assertTrue("unexpected sucesssful restore", expectSuccess); }, e -> { - isFinished.countDown(); + isRestored.countDown(); if (expectSuccess) { throw new RuntimeException(e); } }); - isFinished.await(1, TimeUnit.MINUTES); + isRestored.await(1, TimeUnit.MINUTES); if (expectSuccess) { - // assertEquals(shards, restoreSnapshotResponse.getRestoreInfo().totalShards()); final String indexToSearch = renameIndexes ? renamedIndexName : indexName; final String aliasToSearch = renameAliases ? renamedAliasName : aliasName; if (closed) { - final CountDownLatch isReady = new CountDownLatch(1); + final CountDownLatch isOpened = new CountDownLatch(1); final StepListener openIndexResponseStepListener = new StepListener<>(); client().admin().indices().open(new OpenIndexRequest(indexToSearch).waitForActiveShards(1), openIndexResponseStepListener); - continueOrDie(openIndexResponseStepListener, ignored -> { isReady.countDown(); }); + continueOrDie(openIndexResponseStepListener, ignored -> { isOpened.countDown(); }); - isReady.await(1, TimeUnit.MINUTES); + isOpened.await(1, TimeUnit.MINUTES); } + final CountDownLatch isSearchDone = new CountDownLatch(includeAlias ? 2 : 1); final StepListener searchIndexResponseListener = new StepListener<>(); final StepListener searchAliasResponseListener = new StepListener<>(); - final int expectedCount = includeAlias ? 2 : 1; - final CountDownLatch isSearchDone = new CountDownLatch(expectedCount); client().search( new SearchRequest(indexToSearch).source(new SearchSourceBuilder().size(0).trackTotalHits(true)), searchIndexResponseListener From c5261b06607f6db1ccfed8008f339ee17d3e5ba5 Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Thu, 17 Oct 2024 06:43:10 -0700 Subject: [PATCH 09/12] Use CURRENT version to pass backward compatibility tests. Change to V2.18 later once it is backported into that version. Signed-off-by: Spencer G. Jones --- .../snapshots/restore/RestoreSnapshotRequest.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java index 981622c535ee3..42c64e04268e3 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java @@ -166,10 +166,11 @@ public RestoreSnapshotRequest(StreamInput in) throws IOException { if (in.getVersion().onOrAfter(Version.V_2_17_0)) { sourceRemoteTranslogRepository = in.readOptionalString(); } - if (in.getVersion().onOrAfter(Version.V_3_0_0)) { + // TODO: change to V_2_18_0 once this is backported into that version + if (in.getVersion().onOrAfter(Version.CURRENT)) { renameAliasPattern = in.readOptionalString(); } - if (in.getVersion().onOrAfter(Version.V_3_0_0)) { + if (in.getVersion().onOrAfter(Version.CURRENT)) { renameAliasReplacement = in.readOptionalString(); } } @@ -199,10 +200,11 @@ public void writeTo(StreamOutput out) throws IOException { if (out.getVersion().onOrAfter(Version.V_2_17_0)) { out.writeOptionalString(sourceRemoteTranslogRepository); } - if (out.getVersion().onOrAfter(Version.V_3_0_0)) { + // TODO: change to V_2_18_0 once this is backported into that version + if (out.getVersion().onOrAfter(Version.CURRENT)) { out.writeOptionalString(renameAliasPattern); } - if (out.getVersion().onOrAfter(Version.V_3_0_0)) { + if (out.getVersion().onOrAfter(Version.CURRENT)) { out.writeOptionalString(renameAliasReplacement); } } From d611a4d0b749ccf2d9309129e1070ea08d1add45 Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Thu, 17 Oct 2024 09:58:31 -0700 Subject: [PATCH 10/12] Refactoring Signed-off-by: Spencer G. Jones --- .../opensearch/snapshots/RestoreService.java | 48 +++++++++---------- .../snapshots/RestoreServiceIntegTests.java | 29 +++++------ 2 files changed, 37 insertions(+), 40 deletions(-) diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index a5326c3a7dec0..5c0d0c29ff88b 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -111,6 +111,7 @@ import java.util.function.Function; import java.util.function.Predicate; import java.util.function.Supplier; +import java.util.regex.Pattern; import java.util.stream.Collectors; import static java.util.Collections.unmodifiableSet; @@ -486,19 +487,7 @@ public ClusterState execute(ClusterState currentState) { // Remove all aliases - they shouldn't be restored indexMdBuilder.removeAllAliases(); } else { - for (final Map.Entry alias : snapshotIndexMetadata.getAliases().entrySet()) { - String aliasName = alias.getKey(); - if (request.renameAliasPattern() != null && request.renameAliasReplacement() != null) { - indexMdBuilder.removeAlias(aliasName); - aliasName = aliasName.replaceAll( - request.renameAliasPattern(), - request.renameAliasReplacement() - ); - AliasMetadata newAlias = AliasMetadata.newAliasMetadata(alias.getValue(), aliasName); - indexMdBuilder.putAlias(newAlias); - } - aliases.add(aliasName); - } + aliases = applyAliasesWithRename(snapshotIndexMetadata, indexMdBuilder); } IndexMetadata updatedIndexMetadata = indexMdBuilder.build(); if (partial) { @@ -543,19 +532,7 @@ public ClusterState execute(ClusterState currentState) { indexMdBuilder.putAlias(alias); } } else { - for (final Map.Entry alias : snapshotIndexMetadata.getAliases().entrySet()) { - String aliasName = alias.getKey(); - if (request.renameAliasPattern() != null && request.renameAliasReplacement() != null) { - indexMdBuilder.removeAlias(aliasName); - aliasName = aliasName.replaceAll( - request.renameAliasPattern(), - request.renameAliasReplacement() - ); - AliasMetadata newAlias = AliasMetadata.newAliasMetadata(alias.getValue(), aliasName); - indexMdBuilder.putAlias(newAlias); - } - aliases.add(aliasName); - } + aliases = applyAliasesWithRename(snapshotIndexMetadata, indexMdBuilder); } final Settings.Builder indexSettingsBuilder = Settings.builder() .put(snapshotIndexMetadata.getSettings()) @@ -685,6 +662,25 @@ private void checkAliasNameConflicts(Map renamedIndices, Set applyAliasesWithRename(IndexMetadata snapshotIndexMetadata, IndexMetadata.Builder indexMdBuilder) { + Set aliases = new HashSet<>(); + Pattern renameAliasPattern = null; + if (request.renameAliasPattern() != null && request.renameAliasReplacement() != null) { + renameAliasPattern = Pattern.compile(request.renameAliasPattern()); + } + for (final Map.Entry alias : snapshotIndexMetadata.getAliases().entrySet()) { + String aliasName = alias.getKey(); + if (renameAliasPattern != null) { + indexMdBuilder.removeAlias(aliasName); + aliasName = renameAliasPattern.matcher(aliasName).replaceAll(request.renameAliasReplacement()); + AliasMetadata newAlias = AliasMetadata.newAliasMetadata(alias.getValue(), aliasName); + indexMdBuilder.putAlias(newAlias); + } + aliases.add(aliasName); + } + return aliases; + } + private String[] getIgnoreSettingsInternal() { // for non-remote store enabled domain, we will remove all the remote store // related index settings present in the snapshot. diff --git a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java index 6177a3fac6cf7..e6a8c652ff528 100644 --- a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java +++ b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java @@ -40,56 +40,57 @@ public class RestoreServiceIntegTests extends OpenSearchSingleNodeTestCase { // TODO there is certainly a better way to do this, but I don't know what it is.... public void testRestoreToNewWithAliasAndRename() throws Exception { - __testRestoreWithRename(false, false, true, true, true); + testRestoreWithRename(false, false, true, true, true); } public void testRestoreToNewWithoutAliasAndWithRename() throws Exception { - __testRestoreWithRename(false, false, false, true, true); + testRestoreWithRename(false, false, false, true, true); } public void testRestoreToNewWithAliasAndWithoutRename() throws Exception { - __testRestoreWithRename(false, false, true, false, false); + testRestoreWithRename(false, false, true, false, false); } public void testRestoreToNewWithoutAliasAndRename() throws Exception { - __testRestoreWithRename(false, false, false, false, false); + testRestoreWithRename(false, false, false, false, false); } public void testRestoreOverExistingOpenWithAliasAndRename() throws Exception { - __testRestoreWithRename(true, false, true, true, true); + testRestoreWithRename(true, false, true, true, true); } public void testRestoreOverExistingOpenWithoutAliasAndWithRename() throws Exception { - __testRestoreWithRename(true, false, false, true, true); + testRestoreWithRename(true, false, false, true, true); } public void testRestoreOverExistingClosedWithAliasAndRename() throws Exception { - __testRestoreWithRename(true, true, true, true, true); + testRestoreWithRename(true, true, true, true, true); } public void testRestoreOverExistingClosedWithoutAliasAndWithRename() throws Exception { - __testRestoreWithRename(true, true, false, true, true); + testRestoreWithRename(true, true, false, true, true); } public void testRestoreOverExistingOpenWithoutAliasAndRename() throws Exception { - __testRestoreWithRename(true, false, false, false, false); + testRestoreWithRename(true, false, false, false, false); } public void testRestoreOverExistingOpenWithAliasAndWithoutRename() throws Exception { - __testRestoreWithRename(true, false, true, false, false); + testRestoreWithRename(true, false, true, false, false); } public void testRestoreOverExistingClosedWithoutAliasAndRename() throws Exception { - __testRestoreWithRename(true, true, false, false, false); + testRestoreWithRename(true, true, false, false, false); } public void testRestoreOverExistingClosedWithAliasAndWithoutRename() throws Exception { - __testRestoreWithRename(true, true, true, false, false); + testRestoreWithRename(true, true, true, false, false); } - private void __testRestoreWithRename(boolean exists, boolean closed, boolean includeAlias, boolean renameAliases, boolean renameIndexes) + private void testRestoreWithRename(boolean exists, boolean closed, boolean includeAlias, boolean renameAliases, boolean renameIndexes) throws Exception { - assert exists || !closed; + assert exists || !closed; // index close state doesn't exist when the index doesn't exist - so only permit one value of closed to + // avoid pointless duplicate tests final String indexName = "index_1"; final String renamedIndexName = "index_2"; final String aliasName = "alias_1"; From 8bcd71897d9559fa70c4a7be7a27e0901f558a6e Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Thu, 17 Oct 2024 15:09:54 -0700 Subject: [PATCH 11/12] Overwriting aliases variable causes test failures for reasons I do not understand. Also some refactoring. Signed-off-by: Spencer G. Jones --- .../opensearch/snapshots/RestoreService.java | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index 5c0d0c29ff88b..88eff93e51b38 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -487,7 +487,7 @@ public ClusterState execute(ClusterState currentState) { // Remove all aliases - they shouldn't be restored indexMdBuilder.removeAllAliases(); } else { - aliases = applyAliasesWithRename(snapshotIndexMetadata, indexMdBuilder); + applyAliasesWithRename(snapshotIndexMetadata, indexMdBuilder, aliases); } IndexMetadata updatedIndexMetadata = indexMdBuilder.build(); if (partial) { @@ -532,7 +532,7 @@ public ClusterState execute(ClusterState currentState) { indexMdBuilder.putAlias(alias); } } else { - aliases = applyAliasesWithRename(snapshotIndexMetadata, indexMdBuilder); + applyAliasesWithRename(snapshotIndexMetadata, indexMdBuilder, aliases); } final Settings.Builder indexSettingsBuilder = Settings.builder() .put(snapshotIndexMetadata.getSettings()) @@ -662,23 +662,25 @@ private void checkAliasNameConflicts(Map renamedIndices, Set applyAliasesWithRename(IndexMetadata snapshotIndexMetadata, IndexMetadata.Builder indexMdBuilder) { - Set aliases = new HashSet<>(); - Pattern renameAliasPattern = null; - if (request.renameAliasPattern() != null && request.renameAliasReplacement() != null) { - renameAliasPattern = Pattern.compile(request.renameAliasPattern()); - } - for (final Map.Entry alias : snapshotIndexMetadata.getAliases().entrySet()) { - String aliasName = alias.getKey(); - if (renameAliasPattern != null) { - indexMdBuilder.removeAlias(aliasName); - aliasName = renameAliasPattern.matcher(aliasName).replaceAll(request.renameAliasReplacement()); - AliasMetadata newAlias = AliasMetadata.newAliasMetadata(alias.getValue(), aliasName); + private void applyAliasesWithRename( + IndexMetadata snapshotIndexMetadata, + IndexMetadata.Builder indexMdBuilder, + Set aliases + ) { + if (request.renameAliasPattern() == null || request.renameAliasReplacement() == null) { + aliases.addAll(snapshotIndexMetadata.getAliases().keySet()); + } else { + Pattern renameAliasPattern = Pattern.compile(request.renameAliasPattern()); + for (final Map.Entry alias : snapshotIndexMetadata.getAliases().entrySet()) { + String currentAliasName = alias.getKey(); + indexMdBuilder.removeAlias(currentAliasName); + String newAliasName = renameAliasPattern.matcher(currentAliasName) + .replaceAll(request.renameAliasReplacement()); + AliasMetadata newAlias = AliasMetadata.newAliasMetadata(alias.getValue(), newAliasName); indexMdBuilder.putAlias(newAlias); + aliases.add(newAliasName); } - aliases.add(aliasName); } - return aliases; } private String[] getIgnoreSettingsInternal() { From 50e07d324a1a865c845e70a9632dfb14f5cdd6d0 Mon Sep 17 00:00:00 2001 From: "Spencer G. Jones" Date: Mon, 21 Oct 2024 08:28:03 -0700 Subject: [PATCH 12/12] Convert to paramaterized tests Signed-off-by: Spencer G. Jones --- .../snapshots/RestoreServiceIntegTests.java | 189 +++++++++++------- 1 file changed, 122 insertions(+), 67 deletions(-) diff --git a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java index e6a8c652ff528..92da980d70f34 100644 --- a/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java +++ b/server/src/test/java/org/opensearch/snapshots/RestoreServiceIntegTests.java @@ -8,14 +8,21 @@ package org.opensearch.snapshots; +import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; + import org.opensearch.action.StepListener; 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.get.GetSnapshotsRequest; +import org.opensearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotRequest; import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest; import org.opensearch.action.admin.indices.close.CloseIndexRequest; import org.opensearch.action.admin.indices.close.CloseIndexResponse; import org.opensearch.action.admin.indices.delete.DeleteIndexRequest; +import org.opensearch.action.admin.indices.exists.indices.IndicesExistsRequest; +import org.opensearch.action.admin.indices.exists.indices.IndicesExistsResponse; import org.opensearch.action.admin.indices.open.OpenIndexRequest; import org.opensearch.action.admin.indices.open.OpenIndexResponse; import org.opensearch.action.bulk.BulkRequest; @@ -31,77 +38,125 @@ import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.test.OpenSearchSingleNodeTestCase; +import org.junit.After; +import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Objects; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; public class RestoreServiceIntegTests extends OpenSearchSingleNodeTestCase { - // TODO there is certainly a better way to do this, but I don't know what it is.... - public void testRestoreToNewWithAliasAndRename() throws Exception { - testRestoreWithRename(false, false, true, true, true); - } - - public void testRestoreToNewWithoutAliasAndWithRename() throws Exception { - testRestoreWithRename(false, false, false, true, true); + private final String indexName = "index_1"; + private final String renamedIndexName = "index_2"; + private final String aliasName = "alias_1"; + private final String renamedAliasName = "alias_2"; + private final String repoName = "repo_1"; + private final String snapShotName = "snap_1"; + private final int waitInSeconds = 60; + private boolean exists; + private boolean closed; + private boolean includeAlias; + private boolean renameAliases; + private boolean renameIndexes; + + public RestoreServiceIntegTests(TestCase testCase) { + this.exists = testCase.exists; + this.closed = testCase.closed; + this.includeAlias = testCase.includeAlias; + this.renameAliases = testCase.renameAliases; + this.renameIndexes = testCase.renameIndexes; } - public void testRestoreToNewWithAliasAndWithoutRename() throws Exception { - testRestoreWithRename(false, false, true, false, false); - } - - public void testRestoreToNewWithoutAliasAndRename() throws Exception { - testRestoreWithRename(false, false, false, false, false); - } - - public void testRestoreOverExistingOpenWithAliasAndRename() throws Exception { - testRestoreWithRename(true, false, true, true, true); - } - - public void testRestoreOverExistingOpenWithoutAliasAndWithRename() throws Exception { - testRestoreWithRename(true, false, false, true, true); - } + public static class TestCase { + public boolean exists; + public boolean closed; + public boolean includeAlias; + public boolean renameAliases; + public boolean renameIndexes; + + public TestCase(boolean exists, boolean closed, boolean includeAlias, boolean renameAliases, boolean renameIndexes) { + this.exists = exists; + this.closed = closed; + this.includeAlias = includeAlias; + this.renameAliases = renameAliases; + this.renameIndexes = renameIndexes; + } - public void testRestoreOverExistingClosedWithAliasAndRename() throws Exception { - testRestoreWithRename(true, true, true, true, true); + public String toString() { + return String.join( + " and ", + new String[] { + exists ? "target index exists and is" + (closed ? "closed" : "open") : "doesn't exist", + includeAlias ? "including aliases" : "not including aliases", + renameIndexes ? "renaming indexes" : "not renaming indexes", + renameAliases ? "renaming aliases" : "not renaming aliases" } + ); + } } - public void testRestoreOverExistingClosedWithoutAliasAndWithRename() throws Exception { - testRestoreWithRename(true, true, false, true, true); + @ParametersFactory + public static Collection parameters() { + return Arrays.asList( + new Object[] { new TestCase(false, false, true, true, true) }, + new Object[] { new TestCase(false, false, false, true, true) }, + new Object[] { new TestCase(false, false, true, false, false) }, + new Object[] { new TestCase(false, false, false, false, false) }, + new Object[] { new TestCase(true, false, true, true, true) }, + new Object[] { new TestCase(true, false, false, true, true) }, + new Object[] { new TestCase(true, true, true, true, true) }, + new Object[] { new TestCase(true, true, false, true, true) }, + new Object[] { new TestCase(true, false, false, false, false) }, + new Object[] { new TestCase(true, false, true, false, false) }, + new Object[] { new TestCase(true, true, false, false, false) }, + new Object[] { new TestCase(true, true, true, false, false) } + ); } - public void testRestoreOverExistingOpenWithoutAliasAndRename() throws Exception { - testRestoreWithRename(true, false, false, false, false); - } + @After + public void cleanup() throws InterruptedException { + final CountDownLatch allDeleted = new CountDownLatch(3); + for (String indexName : new String[] { indexName, renamedIndexName }) { + final StepListener existsIndexResponseStepListener = new StepListener<>(); + client().admin().indices().exists(new IndicesExistsRequest(indexName), existsIndexResponseStepListener); + continueOrDie(existsIndexResponseStepListener, resp -> { + if (resp.isExists()) { + final StepListener deleteIndexResponseStepListener = new StepListener<>(); + client().admin().indices().delete(new DeleteIndexRequest(indexName), deleteIndexResponseStepListener); + continueOrDie(deleteIndexResponseStepListener, ignored -> allDeleted.countDown()); + } else { + allDeleted.countDown(); + } + }); + } - public void testRestoreOverExistingOpenWithAliasAndWithoutRename() throws Exception { - testRestoreWithRename(true, false, true, false, false); - } + final StepListener snapStatusResponseStepListener = new StepListener<>(); + client().admin().cluster().getSnapshots(new GetSnapshotsRequest(repoName), snapStatusResponseStepListener); + continueOrDie(snapStatusResponseStepListener, resp -> { + if (resp.getSnapshots().stream().anyMatch(s -> s.snapshotId().getName().equals(snapShotName))) { + final StepListener deleteSnapResponseStepListener = new StepListener<>(); + client().admin() + .cluster() + .deleteSnapshot(new DeleteSnapshotRequest(repoName, snapShotName), deleteSnapResponseStepListener); + continueOrDie(deleteSnapResponseStepListener, ignored -> allDeleted.countDown()); + } else { + allDeleted.countDown(); + } + }); - public void testRestoreOverExistingClosedWithoutAliasAndRename() throws Exception { - testRestoreWithRename(true, true, false, false, false); + allDeleted.await(waitInSeconds, TimeUnit.SECONDS); } - public void testRestoreOverExistingClosedWithAliasAndWithoutRename() throws Exception { - testRestoreWithRename(true, true, true, false, false); - } + public void testRestoreWithRename() throws Exception { - private void testRestoreWithRename(boolean exists, boolean closed, boolean includeAlias, boolean renameAliases, boolean renameIndexes) - throws Exception { - assert exists || !closed; // index close state doesn't exist when the index doesn't exist - so only permit one value of closed to - // avoid pointless duplicate tests - final String indexName = "index_1"; - final String renamedIndexName = "index_2"; - final String aliasName = "alias_1"; - final String renamedAliasName = "alias_2"; - final String repoName = "repo_1"; - final String snapShotName = "snap_1"; - final boolean expectSuccess = !exists || closed; + assert this.exists || !this.closed; // index close state doesn't exist when the index doesn't exist - so only permit one value of + // closed to avoid pointless duplicate tests + final boolean expectSuccess = !this.exists || this.closed; final int documents = randomIntBetween(1, 100); this.createIndex(indexName); - if (exists && renameIndexes) { + if (this.exists && this.renameIndexes) { this.createIndex(renamedIndexName); } @@ -138,16 +193,16 @@ private void testRestoreWithRename(boolean exists, boolean closed, boolean inclu }); }); - isDocumentFinished.await(1, TimeUnit.MINUTES); + isDocumentFinished.await(waitInSeconds, TimeUnit.SECONDS); - if (closed) { + if (this.closed) { final CountDownLatch isClosed = new CountDownLatch(1); final StepListener closeIndexResponseStepListener = new StepListener<>(); - final String indexToClose = renameIndexes ? renamedIndexName : indexName; + final String indexToClose = this.renameIndexes ? renamedIndexName : indexName; client().admin().indices().close(new CloseIndexRequest(indexToClose), closeIndexResponseStepListener); continueOrDie(closeIndexResponseStepListener, ignored -> { isClosed.countDown(); }); - isClosed.await(1, TimeUnit.MINUTES); + isClosed.await(waitInSeconds, TimeUnit.SECONDS); } final StepListener createSnapshotResponseStepListener = new StepListener<>(); @@ -162,7 +217,7 @@ private void testRestoreWithRename(boolean exists, boolean closed, boolean inclu final CountDownLatch isRestorable = new CountDownLatch(1); - if (!exists && !renameIndexes) { + if (!this.exists && !this.renameIndexes) { final StepListener deleteIndexResponseStepListener = new StepListener<>(); continueOrDie(createSnapshotResponseStepListener, ignored -> { client().admin().indices().delete(new DeleteIndexRequest(indexName), deleteIndexResponseStepListener); @@ -172,16 +227,16 @@ private void testRestoreWithRename(boolean exists, boolean closed, boolean inclu continueOrDie(createSnapshotResponseStepListener, ignored -> isRestorable.countDown()); } - isRestorable.await(1, TimeUnit.MINUTES); + isRestorable.await(waitInSeconds, TimeUnit.SECONDS); final StepListener restoreSnapshotResponseStepListener = new StepListener<>(); final CountDownLatch isRestored = new CountDownLatch(1); - RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repoName, snapShotName).includeAliases(includeAlias) + RestoreSnapshotRequest restoreSnapshotRequest = new RestoreSnapshotRequest(repoName, snapShotName).includeAliases(this.includeAlias) .waitForCompletion(true); - if (renameAliases) { + if (this.renameAliases) { restoreSnapshotRequest = restoreSnapshotRequest.renameAliasPattern("1").renameAliasReplacement("2"); } - if (renameIndexes) { + if (this.renameIndexes) { restoreSnapshotRequest = restoreSnapshotRequest.renamePattern("1").renameReplacement("2"); } client().admin().cluster().restoreSnapshot(restoreSnapshotRequest, restoreSnapshotResponseStepListener); @@ -196,22 +251,22 @@ private void testRestoreWithRename(boolean exists, boolean closed, boolean inclu } }); - isRestored.await(1, TimeUnit.MINUTES); + isRestored.await(waitInSeconds, TimeUnit.SECONDS); if (expectSuccess) { - final String indexToSearch = renameIndexes ? renamedIndexName : indexName; - final String aliasToSearch = renameAliases ? renamedAliasName : aliasName; + final String indexToSearch = this.renameIndexes ? renamedIndexName : indexName; + final String aliasToSearch = this.renameAliases ? renamedAliasName : aliasName; - if (closed) { + if (this.closed) { final CountDownLatch isOpened = new CountDownLatch(1); final StepListener openIndexResponseStepListener = new StepListener<>(); client().admin().indices().open(new OpenIndexRequest(indexToSearch).waitForActiveShards(1), openIndexResponseStepListener); continueOrDie(openIndexResponseStepListener, ignored -> { isOpened.countDown(); }); - isOpened.await(1, TimeUnit.MINUTES); + isOpened.await(waitInSeconds, TimeUnit.SECONDS); } - final CountDownLatch isSearchDone = new CountDownLatch(includeAlias ? 2 : 1); + final CountDownLatch isSearchDone = new CountDownLatch(this.includeAlias ? 2 : 1); final StepListener searchIndexResponseListener = new StepListener<>(); final StepListener searchAliasResponseListener = new StepListener<>(); client().search( @@ -219,7 +274,7 @@ private void testRestoreWithRename(boolean exists, boolean closed, boolean inclu searchIndexResponseListener ); continueOrDie(searchIndexResponseListener, ignored -> { isSearchDone.countDown(); }); - if (includeAlias) { + if (this.includeAlias) { client().search( new SearchRequest(aliasToSearch).source(new SearchSourceBuilder().size(0).trackTotalHits(true)), searchAliasResponseListener @@ -227,10 +282,10 @@ private void testRestoreWithRename(boolean exists, boolean closed, boolean inclu continueOrDie(searchAliasResponseListener, ignored -> { isSearchDone.countDown(); }); } - isSearchDone.await(1, TimeUnit.MINUTES); + isSearchDone.await(waitInSeconds, TimeUnit.SECONDS); assertEquals(documents, Objects.requireNonNull(searchIndexResponseListener.result().getHits().getTotalHits()).value); - if (includeAlias) { + if (this.includeAlias) { assertEquals(documents, Objects.requireNonNull(searchAliasResponseListener.result().getHits().getTotalHits()).value); } }