From 457898aaadd7d7296dd7c04ba37496b2194a8d39 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Mon, 6 Jan 2025 10:02:55 -0800 Subject: [PATCH 01/13] Add index.knn setting to list of unmodifiable settings when restore snapshot Signed-off-by: AnnTian Shao --- .../remotestore/RemoteRestoreSnapshotIT.java | 17 +++++++++++++++++ .../cluster/metadata/IndexMetadata.java | 2 ++ .../opensearch/snapshots/RestoreService.java | 4 +++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index 70e283791fc3e..a029789eecf7a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -740,6 +740,23 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); + // try index restore with index.knn setting modified + Settings indexKnnSettings = Settings.builder().put(IndexMetadata.SETTING_KNN_ENABLED, false).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(indexKnnSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.knn]" + " on restore")); + // try index restore with remote store repository and translog store repository disabled exception = expectThrows( SnapshotRestoreException.class, diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index f70282986ad4e..e47cfa0a07d0f 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -271,6 +271,8 @@ static Setting buildNumberOfShardsSetting() { Property.IndexScope ); + public static final String SETTING_KNN_ENABLED = "index.knn"; + public static final Setting INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING = Setting.intSetting( "index.number_of_routing_shards", INDEX_NUMBER_OF_SHARDS_SETTING, diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index 4b5bd951f80a0..8634fb1fea816 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -120,6 +120,7 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_CREATION_DATE; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_HISTORY_UUID; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_INDEX_UUID; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_KNN_ENABLED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; @@ -171,7 +172,8 @@ public class RestoreService implements ClusterStateApplier { SETTING_HISTORY_UUID, SETTING_REMOTE_STORE_ENABLED, SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, - SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY + SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, + SETTING_KNN_ENABLED ) ); From 5f709152305c68f35cbbde38fa9c382c76ae25c3 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Mon, 6 Jan 2025 10:02:55 -0800 Subject: [PATCH 02/13] Add index.knn setting to list of unmodifiable settings when restore snapshot Signed-off-by: AnnTian Shao --- CHANGELOG.md | 1 + .../remotestore/RemoteRestoreSnapshotIT.java | 17 +++++++++++++++++ .../cluster/metadata/IndexMetadata.java | 2 ++ .../opensearch/snapshots/RestoreService.java | 4 +++- 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c7c7eb7c5e8b..a119549470091 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Tiered Caching] Fix bug in cache stats API ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560)) - Bound the size of cache in deprecation logger ([16702](https://github.com/opensearch-project/OpenSearch/issues/16702)) - Ensure consistency of system flag on IndexMetadata after diff is applied ([#16644](https://github.com/opensearch-project/OpenSearch/pull/16644)) +- Fixing the bug to prevent updating the index.knn setting during restore snapshot(#)[] ### Security diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index 70e283791fc3e..a029789eecf7a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -740,6 +740,23 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); + // try index restore with index.knn setting modified + Settings indexKnnSettings = Settings.builder().put(IndexMetadata.SETTING_KNN_ENABLED, false).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(indexKnnSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.knn]" + " on restore")); + // try index restore with remote store repository and translog store repository disabled exception = expectThrows( SnapshotRestoreException.class, diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index f70282986ad4e..e47cfa0a07d0f 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -271,6 +271,8 @@ static Setting buildNumberOfShardsSetting() { Property.IndexScope ); + public static final String SETTING_KNN_ENABLED = "index.knn"; + public static final Setting INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING = Setting.intSetting( "index.number_of_routing_shards", INDEX_NUMBER_OF_SHARDS_SETTING, diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index 4b5bd951f80a0..8634fb1fea816 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -120,6 +120,7 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_CREATION_DATE; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_HISTORY_UUID; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_INDEX_UUID; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_KNN_ENABLED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; @@ -171,7 +172,8 @@ public class RestoreService implements ClusterStateApplier { SETTING_HISTORY_UUID, SETTING_REMOTE_STORE_ENABLED, SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, - SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY + SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, + SETTING_KNN_ENABLED ) ); From f9c06e8a271673831760bf90fd8a0c7efc294191 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Wed, 15 Jan 2025 15:59:27 -0800 Subject: [PATCH 03/13] Add new Setting property UnmodifiableOnRestore to mark setting as immutable on restore snapshot Signed-off-by: AnnTian Shao --- CHANGELOG.md | 2 +- .../remotestore/RemoteRestoreSnapshotIT.java | 6 +++--- .../cluster/metadata/IndexMetadata.java | 12 +++++++++--- .../common/settings/AbstractScopedSettings.java | 8 ++++++++ .../org/opensearch/common/settings/Setting.java | 11 ++++++++++- .../org/opensearch/indices/IndicesService.java | 4 ++++ .../opensearch/snapshots/RestoreService.java | 17 ++++++++++++----- 7 files changed, 47 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e89caa1d4a08..67dc9b6596500 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Introduce framework for auxiliary transports and an experimental gRPC transport plugin ([#16534](https://github.com/opensearch-project/OpenSearch/pull/16534)) - Changes to support IP field in star tree indexing([#16641](https://github.com/opensearch-project/OpenSearch/pull/16641/)) - Support object fields in star-tree index([#16728](https://github.com/opensearch-project/OpenSearch/pull/16728/)) +- Added new Setting property UnmodifiableOnRestore to prevent updating settings on restore snapshot ([#16957](https://github.com/opensearch-project/OpenSearch/pull/16957)) ### Dependencies - Bump `com.google.cloud:google-cloud-core-http` from 2.23.0 to 2.47.0 ([#16504](https://github.com/opensearch-project/OpenSearch/pull/16504)) @@ -88,7 +89,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix _list/shards API failing when closed indices are present ([#16606](https://github.com/opensearch-project/OpenSearch/pull/16606)) - Fix remote shards balance ([#15335](https://github.com/opensearch-project/OpenSearch/pull/15335)) - Always use `constant_score` query for `match_only_text` field ([#16964](https://github.com/opensearch-project/OpenSearch/pull/16964)) -- Fixing the bug to prevent updating the index.knn setting during restore snapshot ([#16957](https://github.com/opensearch-project/OpenSearch/pull/16957)) ### Security diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index a029789eecf7a..f866859c34823 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -740,8 +740,8 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); - // try index restore with index.knn setting modified - Settings indexKnnSettings = Settings.builder().put(IndexMetadata.SETTING_KNN_ENABLED, false).build(); + // try index restore with index.number_of_shards setting modified + Settings indexKnnSettings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, false).build(); exception = expectThrows( SnapshotRestoreException.class, @@ -755,7 +755,7 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.knn]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); // try index restore with remote store repository and translog store repository disabled exception = expectThrows( diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index e47cfa0a07d0f..aefb369194548 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -230,7 +230,15 @@ static Setting buildNumberOfShardsSetting() { + "]" ); } - return Setting.intSetting(SETTING_NUMBER_OF_SHARDS, defaultNumShards, 1, maxNumShards, Property.IndexScope, Property.Final); + return Setting.intSetting( + SETTING_NUMBER_OF_SHARDS, + defaultNumShards, + 1, + maxNumShards, + Property.IndexScope, + Property.Final, + Property.UnmodifiableOnRestore + ); } public static final String INDEX_SETTING_PREFIX = "index."; @@ -271,8 +279,6 @@ static Setting buildNumberOfShardsSetting() { Property.IndexScope ); - public static final String SETTING_KNN_ENABLED = "index.knn"; - public static final Setting INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING = Setting.intSetting( "index.number_of_routing_shards", INDEX_NUMBER_OF_SHARDS_SETTING, diff --git a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java index 7655135b06d6c..8c10623e48fe4 100644 --- a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java @@ -759,6 +759,14 @@ public boolean isFinalSetting(String key) { return setting != null && setting.isFinal(); } + /** + * Returns true if the setting for the given key is unmodifiableOnRestore. Otherwise false. + */ + public boolean isUnmodifiableOnRestoreSetting(String key) { + final Setting setting = get(key); + return setting != null && setting.isUnmodifiableOnRestore(); + } + /** * Returns a settings object that contains all settings that are not * already set in the given source. The diff contains either the default value for each diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java index 081029c1c106c..f5f49e5c5c6ad 100644 --- a/server/src/main/java/org/opensearch/common/settings/Setting.java +++ b/server/src/main/java/org/opensearch/common/settings/Setting.java @@ -171,7 +171,12 @@ public enum Property { /** * Extension scope */ - ExtensionScope + ExtensionScope, + + /** + * Mark this setting as immutable on restore snapshot + */ + UnmodifiableOnRestore } private final Key key; @@ -348,6 +353,10 @@ public final boolean isFinal() { return properties.contains(Property.Final); } + public final boolean isUnmodifiableOnRestore() { + return properties.contains(Property.UnmodifiableOnRestore); + } + public final boolean isInternalIndex() { return properties.contains(Property.InternalIndex); } diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index b9bad5527e3f4..c0472fb9ebc70 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -1182,6 +1182,10 @@ public IndicesQueryCache getIndicesQueryCache() { return indicesQueryCache; } + public IndexScopedSettings getIndexScopedSettings() { + return indexScopedSettings; + } + /** * Accumulate stats from the passed Object * diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index 8634fb1fea816..11985886be397 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -77,6 +77,7 @@ import org.opensearch.common.lucene.Lucene; import org.opensearch.common.regex.Regex; import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.IndexScopedSettings; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.ArrayUtils; @@ -120,16 +121,15 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_CREATION_DATE; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_HISTORY_UUID; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_INDEX_UUID; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_KNN_ENABLED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_CREATED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_UPGRADED; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY; import static org.opensearch.common.util.IndexUtils.filterIndices; import static org.opensearch.common.util.set.Sets.newHashSet; @@ -165,15 +165,13 @@ public class RestoreService implements ClusterStateApplier { private static final Set USER_UNMODIFIABLE_SETTINGS = unmodifiableSet( newHashSet( - SETTING_NUMBER_OF_SHARDS, SETTING_VERSION_CREATED, SETTING_INDEX_UUID, SETTING_CREATION_DATE, SETTING_HISTORY_UUID, SETTING_REMOTE_STORE_ENABLED, SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, - SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, - SETTING_KNN_ENABLED + SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY ) ); @@ -184,6 +182,7 @@ public class RestoreService implements ClusterStateApplier { static { Set unremovable = new HashSet<>(USER_UNMODIFIABLE_SETTINGS.size() + 4); unremovable.addAll(USER_UNMODIFIABLE_SETTINGS); + unremovable.add(SETTING_NUMBER_OF_SHARDS); unremovable.add(SETTING_NUMBER_OF_REPLICAS); unremovable.add(SETTING_AUTO_EXPAND_REPLICAS); unremovable.add(SETTING_VERSION_UPGRADED); @@ -204,6 +203,8 @@ public class RestoreService implements ClusterStateApplier { private final ClusterSettings clusterSettings; + private final IndexScopedSettings indexScopedSettings; + private final IndicesService indicesService; private final Supplier clusterInfoSupplier; @@ -236,6 +237,7 @@ public RestoreService( this.clusterSettings = clusterService.getClusterSettings(); this.shardLimitValidator = shardLimitValidator; this.indicesService = indicesService; + this.indexScopedSettings = indicesService.getIndexScopedSettings(); this.clusterInfoSupplier = clusterInfoSupplier; this.dataToFileCacheSizeRatioSupplier = dataToFileCacheSizeRatioSupplier; @@ -874,6 +876,11 @@ private IndexMetadata updateIndexSettings( .put(normalizedChangeSettings.filter(k -> { if (USER_UNMODIFIABLE_SETTINGS.contains(k)) { throw new SnapshotRestoreException(snapshot, "cannot modify setting [" + k + "] on restore"); + } else if (indexScopedSettings.isUnmodifiableOnRestoreSetting(k)) { + throw new SnapshotRestoreException( + snapshot, + "cannot modify UnmodifiableOnRestore setting [" + k + "] on restore" + ); } else { return true; } From 2afd94e1b40a6c251120e8a99583b67be27eea1b Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Thu, 16 Jan 2025 11:41:34 -0800 Subject: [PATCH 04/13] Add tests for new Setting property UnmodifiableOnRestore Signed-off-by: AnnTian Shao --- .../RestoreShallowSnapshotV2IT.java | 17 +++++++++++++ .../common/settings/ScopedSettingsTests.java | 24 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java index ecb97e79b348e..9ae99acb15f4e 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java @@ -807,6 +807,23 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); + // try index restore with index.number_of_shards setting modified + Settings indexKnnSettings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, false).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(indexKnnSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + // try index restore with remote store repository and translog store repository disabled exception = expectThrows( SnapshotRestoreException.class, diff --git a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java index 7780481c9deff..cb5b259954453 100644 --- a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java @@ -789,6 +789,30 @@ public void testIsFinal() { assertTrue(settings.isFinalSetting("foo.group.key")); } + public void testIsUnmodifiableOnRestore() { + ClusterSettings settings = new ClusterSettings( + Settings.EMPTY, + new HashSet<>( + Arrays.asList( + Setting.intSetting("foo.int", 1, Property.UnmodifiableOnRestore, Property.NodeScope), + Setting.groupSetting("foo.group.", Property.UnmodifiableOnRestore, Property.NodeScope), + Setting.groupSetting("foo.list.", Property.UnmodifiableOnRestore, Property.NodeScope), + Setting.intSetting("foo.int.baz", 1, Property.NodeScope) + ) + ) + ); + + assertFalse(settings.isUnmodifiableOnRestoreSetting("foo.int.baz")); + assertTrue(settings.isUnmodifiableOnRestoreSetting("foo.int")); + + assertFalse(settings.isUnmodifiableOnRestoreSetting("foo.list")); + assertTrue(settings.isUnmodifiableOnRestoreSetting("foo.list.0.key")); + assertTrue(settings.isUnmodifiableOnRestoreSetting("foo.list.key")); + + assertFalse(settings.isUnmodifiableOnRestoreSetting("foo.group")); + assertTrue(settings.isUnmodifiableOnRestoreSetting("foo.group.key")); + } + public void testDiff() throws IOException { Setting fooBarBaz = Setting.intSetting("foo.bar.baz", 1, Property.NodeScope); Setting fooBar = Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope); From a24b673ce3b3c7a231e10be56acf8d39041dd425 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Fri, 17 Jan 2025 13:03:33 -0800 Subject: [PATCH 05/13] fixes and added more tests for new setting property UnmodifiableOnRestore Signed-off-by: AnnTian Shao --- .../remotestore/RemoteRestoreSnapshotIT.java | 104 ++++++++++++++++-- .../RestoreShallowSnapshotV2IT.java | 104 ++++++++++++++++-- .../cluster/metadata/IndexMetadata.java | 36 +++++- .../common/settings/IndexScopedSettings.java | 3 + .../opensearch/snapshots/RestoreService.java | 25 ++--- 5 files changed, 233 insertions(+), 39 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index f866859c34823..8ab4be1ca9bde 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -8,6 +8,7 @@ package org.opensearch.remotestore; +import org.opensearch.Version; import org.opensearch.action.DocWriteResponse; import org.opensearch.action.LatchedActionListener; import org.opensearch.action.admin.cluster.remotestore.restore.RestoreRemoteStoreRequest; @@ -494,6 +495,51 @@ public void testRemoteRestoreIndexRestoredFromSnapshot() throws IOException, Exe assertDocsPresentInIndex(client(), indexName1, numDocsInIndex1); } + public void testIndexRestoredFromSnapshotWithUpdateSetting() throws IOException, ExecutionException, InterruptedException { + internalCluster().startClusterManagerOnlyNode(); + internalCluster().startDataOnlyNodes(2); + + String indexName1 = "testindex1"; + String snapshotRepoName = "test-restore-snapshot-repo"; + String snapshotName1 = "test-restore-snapshot1"; + Path absolutePath1 = randomRepoPath().toAbsolutePath(); + logger.info("Snapshot Path [{}]", absolutePath1); + + createRepository(snapshotRepoName, "fs", getRepositorySettings(absolutePath1, true)); + + Settings indexSettings = getIndexSettings(1, 0).build(); + createIndex(indexName1, indexSettings); + + final int numDocsInIndex1 = randomIntBetween(20, 30); + indexDocuments(client(), indexName1, numDocsInIndex1); + flushAndRefresh(indexName1); + ensureGreen(indexName1); + + logger.info("--> snapshot"); + SnapshotInfo snapshotInfo1 = createSnapshot(snapshotRepoName, snapshotName1, new ArrayList<>(Arrays.asList(indexName1))); + assertThat(snapshotInfo1.successfulShards(), greaterThan(0)); + assertThat(snapshotInfo1.successfulShards(), equalTo(snapshotInfo1.totalShards())); + assertThat(snapshotInfo1.state(), equalTo(SnapshotState.SUCCESS)); + + assertAcked(client().admin().indices().delete(new DeleteIndexRequest(indexName1)).get()); + assertFalse(indexExists(indexName1)); + + // try index restore with index.number_of_replicas setting modified. index.number_of_replicas can be modified on restore + Settings numberOfReplicasSettings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1).build(); + + RestoreSnapshotResponse restoreSnapshotResponse1 = client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepoName, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(numberOfReplicasSettings) + .setIndices(indexName1) + .get(); + + assertEquals(restoreSnapshotResponse1.status(), RestStatus.ACCEPTED); + ensureGreen(indexName1); + assertDocsPresentInIndex(client(), indexName1, numDocsInIndex1); + } + protected IndexShard getIndexShard(String node, String indexName) { final Index index = resolveIndex(indexName); IndicesService indicesService = internalCluster().getInstance(IndicesService.class, node); @@ -721,10 +767,8 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); - // try index restore with remote store repository modified - Settings remoteStoreIndexSettings = Settings.builder() - .put(IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, newRemoteStoreRepo) - .build(); + // try index restore with index.number_of_shards setting modified + Settings numberOfShardsSettingsDiff = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build(); exception = expectThrows( SnapshotRestoreException.class, @@ -732,16 +776,16 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIndexSettings(remoteStoreIndexSettings) + .setIndexSettings(numberOfShardsSettingsDiff) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - // try index restore with index.number_of_shards setting modified - Settings indexKnnSettings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, false).build(); + // try index restore with index.number_of_shards setting same + Settings numberOfShardsSettingsSame = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).build(); exception = expectThrows( SnapshotRestoreException.class, @@ -749,7 +793,7 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIndexSettings(indexKnnSettings) + .setIndexSettings(numberOfShardsSettingsSame) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) @@ -757,6 +801,48 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + // try index restore with mix of modifiable and unmodifiable settings on restore + // index.version.created is unmodifiable, index.number_of_replicas is modifiable + Settings mixedSettings = Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(mixedSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.version.created]" + " on restore")); + + // try index restore with multiple UnmodifiableOnRestore settings on restore + Settings unmodifiableSettings = Settings.builder() + .put(IndexMetadata.SETTING_CREATION_DATE, -1L) + .put(IndexMetadata.SETTING_INDEX_UUID, IndexMetadata.INDEX_UUID_NA_VALUE) + .put(IndexMetadata.SETTING_HISTORY_UUID, IndexMetadata.INDEX_UUID_NA_VALUE) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(unmodifiableSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.creation_date]" + " on restore")); + // try index restore with remote store repository and translog store repository disabled exception = expectThrows( SnapshotRestoreException.class, diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java index 9ae99acb15f4e..f474a7ec0301b 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java @@ -8,6 +8,7 @@ package org.opensearch.remotestore; +import org.opensearch.Version; import org.opensearch.action.DocWriteResponse; import org.opensearch.action.admin.cluster.remotestore.restore.RestoreRemoteStoreRequest; import org.opensearch.action.admin.cluster.repositories.get.GetRepositoriesRequest; @@ -561,6 +562,51 @@ public void testRemoteRestoreIndexRestoredFromSnapshot() throws IOException, Exe assertDocsPresentInIndex(client(), indexName1, numDocsInIndex1); } + public void testIndexRestoredFromSnapshotWithUpdateSetting() throws IOException, ExecutionException, InterruptedException { + internalCluster().startClusterManagerOnlyNode(); + internalCluster().startDataOnlyNodes(2); + + String indexName1 = "testindex1"; + String snapshotRepoName = "test-restore-snapshot-repo"; + String snapshotName1 = "test-restore-snapshot1"; + Path absolutePath1 = randomRepoPath().toAbsolutePath(); + logger.info("Snapshot Path [{}]", absolutePath1); + + createRepository(snapshotRepoName, "fs", getRepositorySettings(absolutePath1, true)); + + Settings indexSettings = getIndexSettings(1, 0).build(); + createIndex(indexName1, indexSettings); + + final int numDocsInIndex1 = randomIntBetween(20, 30); + indexDocuments(client(), indexName1, numDocsInIndex1); + flushAndRefresh(indexName1); + ensureGreen(indexName1); + + logger.info("--> snapshot"); + SnapshotInfo snapshotInfo1 = createSnapshot(snapshotRepoName, snapshotName1, new ArrayList<>(Arrays.asList(indexName1))); + assertThat(snapshotInfo1.successfulShards(), greaterThan(0)); + assertThat(snapshotInfo1.successfulShards(), equalTo(snapshotInfo1.totalShards())); + assertThat(snapshotInfo1.state(), equalTo(SnapshotState.SUCCESS)); + + assertAcked(client().admin().indices().delete(new DeleteIndexRequest(indexName1)).get()); + assertFalse(indexExists(indexName1)); + + // try index restore with index.number_of_replicas setting modified. index.number_of_replicas can be modified on restore + Settings numberOfReplicasSettings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1).build(); + + RestoreSnapshotResponse restoreSnapshotResponse1 = client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepoName, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(numberOfReplicasSettings) + .setIndices(indexName1) + .get(); + + assertEquals(restoreSnapshotResponse1.status(), RestStatus.ACCEPTED); + ensureGreen(indexName1); + assertDocsPresentInIndex(client(), indexName1, numDocsInIndex1); + } + private IndexShard getIndexShard(String node, String indexName) { final Index index = resolveIndex(indexName); IndicesService indicesService = internalCluster().getInstance(IndicesService.class, node); @@ -788,10 +834,8 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); - // try index restore with remote store repository modified - Settings remoteStoreIndexSettings = Settings.builder() - .put(IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, newRemoteStoreRepo) - .build(); + // try index restore with index.number_of_shards setting modified + Settings numberOfShardsSettingsDiff = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build(); exception = expectThrows( SnapshotRestoreException.class, @@ -799,16 +843,16 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIndexSettings(remoteStoreIndexSettings) + .setIndexSettings(numberOfShardsSettingsDiff) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - // try index restore with index.number_of_shards setting modified - Settings indexKnnSettings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, false).build(); + // try index restore with index.number_of_shards setting same + Settings numberOfShardsSettingsSame = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).build(); exception = expectThrows( SnapshotRestoreException.class, @@ -816,7 +860,7 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIndexSettings(indexKnnSettings) + .setIndexSettings(numberOfShardsSettingsSame) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) @@ -824,6 +868,48 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + // try index restore with mix of modifiable and unmodifiable settings on restore + // index.version.created is unmodifiable, index.number_of_replicas is modifiable + Settings mixedSettings = Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(mixedSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.version.created]" + " on restore")); + + // try index restore with multiple UnmodifiableOnRestore settings on restore + Settings unmodifiableSettings = Settings.builder() + .put(IndexMetadata.SETTING_CREATION_DATE, -1L) + .put(IndexMetadata.SETTING_INDEX_UUID, IndexMetadata.INDEX_UUID_NA_VALUE) + .put(IndexMetadata.SETTING_HISTORY_UUID, IndexMetadata.INDEX_UUID_NA_VALUE) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(unmodifiableSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.creation_date]" + " on restore")); + // try index restore with remote store repository and translog store repository disabled exception = expectThrows( SnapshotRestoreException.class, diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index aefb369194548..f41e81177d06d 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -390,7 +390,8 @@ public Iterator> settings() { }, Property.IndexScope, Property.PrivateIndex, - Property.Dynamic + Property.Dynamic, + Property.UnmodifiableOnRestore ); /** @@ -424,7 +425,8 @@ public Iterator> settings() { }, Property.IndexScope, Property.PrivateIndex, - Property.Dynamic + Property.Dynamic, + Property.UnmodifiableOnRestore ); private static void validateRemoteStoreSettingEnabled(final Map, Object> settings, Setting setting) { @@ -475,7 +477,8 @@ public Iterator> settings() { }, Property.IndexScope, Property.PrivateIndex, - Property.Dynamic + Property.Dynamic, + Property.UnmodifiableOnRestore ); public static final String SETTING_AUTO_EXPAND_REPLICAS = "index.auto_expand_replicas"; @@ -567,13 +570,21 @@ public static APIBlock readFrom(StreamInput input) throws IOException { SETTING_VERSION_CREATED, Version.V_EMPTY, Property.IndexScope, - Property.PrivateIndex + Property.PrivateIndex, + Property.UnmodifiableOnRestore ); public static final String SETTING_VERSION_CREATED_STRING = "index.version.created_string"; public static final String SETTING_VERSION_UPGRADED = "index.version.upgraded"; public static final String SETTING_VERSION_UPGRADED_STRING = "index.version.upgraded_string"; public static final String SETTING_CREATION_DATE = "index.creation_date"; + + public static final Setting SETTING_INDEX_CREATION_DATE = Setting.longSetting( + SETTING_CREATION_DATE, + -1L, + Property.IndexScope, + Property.UnmodifiableOnRestore + ); /** * The user provided name for an index. This is the plain string provided by the user when the index was created. * It might still contain date math expressions etc. (added in 5.0) @@ -597,8 +608,25 @@ public static APIBlock readFrom(StreamInput input) throws IOException { Function.identity(), Property.IndexScope ); + public static final String INDEX_UUID_NA_VALUE = Strings.UNKNOWN_UUID_VALUE; + public static final Setting INDEX_UUID_SETTING = Setting.simpleString( + SETTING_INDEX_UUID, + INDEX_UUID_NA_VALUE, + Property.IndexScope, + Property.PrivateIndex, + Property.UnmodifiableOnRestore + ); + + public static final Setting SETTING_INDEX_HISTORY_UUID = Setting.simpleString( + SETTING_HISTORY_UUID, + INDEX_UUID_NA_VALUE, + Property.IndexScope, + Property.PrivateIndex, + Property.UnmodifiableOnRestore + ); + public static final String INDEX_ROUTING_REQUIRE_GROUP_PREFIX = "index.routing.allocation.require"; public static final String INDEX_ROUTING_INCLUDE_GROUP_PREFIX = "index.routing.allocation.include"; public static final String INDEX_ROUTING_EXCLUDE_GROUP_PREFIX = "index.routing.allocation.exclude"; diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index 8d56a942c5d6e..8f59c928d758b 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -91,6 +91,9 @@ public final class IndexScopedSettings extends AbstractScopedSettings { MergeSchedulerConfig.MAX_MERGE_COUNT_SETTING, MergeSchedulerConfig.MAX_THREAD_COUNT_SETTING, IndexMetadata.SETTING_INDEX_VERSION_CREATED, + IndexMetadata.INDEX_UUID_SETTING, + IndexMetadata.SETTING_INDEX_CREATION_DATE, + IndexMetadata.SETTING_INDEX_HISTORY_UUID, IndexMetadata.INDEX_ROUTING_EXCLUDE_GROUP_SETTING, IndexMetadata.INDEX_ROUTING_INCLUDE_GROUP_SETTING, IndexMetadata.INDEX_ROUTING_REQUIRE_GROUP_SETTING, diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index ef5b770c3dd4d..c27821274599d 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -163,7 +163,8 @@ public class RestoreService implements ClusterStateApplier { private static final Logger logger = LogManager.getLogger(RestoreService.class); - private static final Set USER_UNMODIFIABLE_SETTINGS = unmodifiableSet( + // It's OK to change some settings, but we shouldn't allow simply removing them + private static final Set USER_UNREMOVABLE_SETTINGS = unmodifiableSet( newHashSet( SETTING_VERSION_CREATED, SETTING_INDEX_UUID, @@ -171,24 +172,16 @@ public class RestoreService implements ClusterStateApplier { SETTING_HISTORY_UUID, SETTING_REMOTE_STORE_ENABLED, SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, - SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY + SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, + SETTING_NUMBER_OF_SHARDS, + SETTING_NUMBER_OF_REPLICAS, + SETTING_AUTO_EXPAND_REPLICAS, + SETTING_VERSION_UPGRADED ) ); - // It's OK to change some settings, but we shouldn't allow simply removing them - private static final Set USER_UNREMOVABLE_SETTINGS; private static final String REMOTE_STORE_INDEX_SETTINGS_REGEX = "index.remote_store.*"; - static { - Set unremovable = new HashSet<>(USER_UNMODIFIABLE_SETTINGS.size() + 4); - unremovable.addAll(USER_UNMODIFIABLE_SETTINGS); - unremovable.add(SETTING_NUMBER_OF_SHARDS); - unremovable.add(SETTING_NUMBER_OF_REPLICAS); - unremovable.add(SETTING_AUTO_EXPAND_REPLICAS); - unremovable.add(SETTING_VERSION_UPGRADED); - USER_UNREMOVABLE_SETTINGS = unmodifiableSet(unremovable); - } - private final ClusterService clusterService; private final RepositoriesService repositoriesService; @@ -874,9 +867,7 @@ private IndexMetadata updateIndexSettings( Settings.Builder settingsBuilder = Settings.builder() .put(settings.filter(settingsFilter)) .put(normalizedChangeSettings.filter(k -> { - if (USER_UNMODIFIABLE_SETTINGS.contains(k)) { - throw new SnapshotRestoreException(snapshot, "cannot modify setting [" + k + "] on restore"); - } else if (indexScopedSettings.isUnmodifiableOnRestoreSetting(k)) { + if (indexScopedSettings.isUnmodifiableOnRestoreSetting(k)) { throw new SnapshotRestoreException( snapshot, "cannot modify UnmodifiableOnRestore setting [" + k + "] on restore" From 252100cb1174316198044e93647c544aac1e4394 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Tue, 21 Jan 2025 09:51:12 -0800 Subject: [PATCH 06/13] fix test failures Signed-off-by: AnnTian Shao --- .../org/opensearch/cluster/metadata/IndexMetadata.java | 2 ++ .../java/org/opensearch/index/IndexSettingsTests.java | 6 +++--- .../java/org/opensearch/test/InternalSettingsPlugin.java | 9 --------- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index f41e81177d06d..39a470a205511 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -582,7 +582,9 @@ public static APIBlock readFrom(StreamInput input) throws IOException { public static final Setting SETTING_INDEX_CREATION_DATE = Setting.longSetting( SETTING_CREATION_DATE, -1L, + -1L, Property.IndexScope, + Property.NodeScope, Property.UnmodifiableOnRestore ); /** diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index 474ec73d5fe61..cbc9d8856a27e 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -608,7 +608,7 @@ public void testTranslogGenerationSizeThreshold() { } public void testPrivateSettingsValidation() { - final Settings settings = Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, System.currentTimeMillis()).build(); + final Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_UPGRADED, Version.V_EMPTY).build(); final IndexScopedSettings indexScopedSettings = new IndexScopedSettings(settings, IndexScopedSettings.BUILT_IN_INDEX_SETTINGS); { @@ -617,7 +617,7 @@ public void testPrivateSettingsValidation() { SettingsException.class, () -> indexScopedSettings.validate(settings, randomBoolean()) ); - assertThat(e, hasToString(containsString("unknown setting [index.creation_date]"))); + assertThat(e, hasToString(containsString("unknown setting [index.version.upgraded]"))); } { @@ -626,7 +626,7 @@ public void testPrivateSettingsValidation() { SettingsException.class, () -> indexScopedSettings.validate(settings, randomBoolean(), false, randomBoolean()) ); - assertThat(e, hasToString(containsString("unknown setting [index.creation_date]"))); + assertThat(e, hasToString(containsString("unknown setting [index.version.upgraded]"))); } // nothing should happen since we are ignoring private settings diff --git a/test/framework/src/main/java/org/opensearch/test/InternalSettingsPlugin.java b/test/framework/src/main/java/org/opensearch/test/InternalSettingsPlugin.java index 986cfd9c5b613..96919f65f88fc 100644 --- a/test/framework/src/main/java/org/opensearch/test/InternalSettingsPlugin.java +++ b/test/framework/src/main/java/org/opensearch/test/InternalSettingsPlugin.java @@ -31,7 +31,6 @@ package org.opensearch.test; -import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.unit.TimeValue; @@ -59,13 +58,6 @@ public final class InternalSettingsPlugin extends Plugin { Property.IndexScope, Property.NodeScope ); - public static final Setting INDEX_CREATION_DATE_SETTING = Setting.longSetting( - IndexMetadata.SETTING_CREATION_DATE, - -1, - -1, - Property.IndexScope, - Property.NodeScope - ); public static final Setting TRANSLOG_RETENTION_CHECK_INTERVAL_SETTING = Setting.timeSetting( "index.translog.retention.check_interval", new TimeValue(10, TimeUnit.MINUTES), @@ -78,7 +70,6 @@ public final class InternalSettingsPlugin extends Plugin { public List> getSettings() { return Arrays.asList( MERGE_ENABLED, - INDEX_CREATION_DATE_SETTING, PROVIDED_NAME_SETTING, TRANSLOG_RETENTION_CHECK_INTERVAL_SETTING, RemoteConnectionStrategy.REMOTE_MAX_PENDING_CONNECTION_LISTENERS, From e86049907d43dbfee5e25a4638954f6a88ef7d8d Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Tue, 21 Jan 2025 13:57:11 -0800 Subject: [PATCH 07/13] Revert "fix test failures" This reverts commit 252100cb1174316198044e93647c544aac1e4394. Signed-off-by: AnnTian Shao --- .../org/opensearch/cluster/metadata/IndexMetadata.java | 2 -- .../java/org/opensearch/index/IndexSettingsTests.java | 6 +++--- .../java/org/opensearch/test/InternalSettingsPlugin.java | 9 +++++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index 39a470a205511..f41e81177d06d 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -582,9 +582,7 @@ public static APIBlock readFrom(StreamInput input) throws IOException { public static final Setting SETTING_INDEX_CREATION_DATE = Setting.longSetting( SETTING_CREATION_DATE, -1L, - -1L, Property.IndexScope, - Property.NodeScope, Property.UnmodifiableOnRestore ); /** diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index cbc9d8856a27e..474ec73d5fe61 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -608,7 +608,7 @@ public void testTranslogGenerationSizeThreshold() { } public void testPrivateSettingsValidation() { - final Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_UPGRADED, Version.V_EMPTY).build(); + final Settings settings = Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, System.currentTimeMillis()).build(); final IndexScopedSettings indexScopedSettings = new IndexScopedSettings(settings, IndexScopedSettings.BUILT_IN_INDEX_SETTINGS); { @@ -617,7 +617,7 @@ public void testPrivateSettingsValidation() { SettingsException.class, () -> indexScopedSettings.validate(settings, randomBoolean()) ); - assertThat(e, hasToString(containsString("unknown setting [index.version.upgraded]"))); + assertThat(e, hasToString(containsString("unknown setting [index.creation_date]"))); } { @@ -626,7 +626,7 @@ public void testPrivateSettingsValidation() { SettingsException.class, () -> indexScopedSettings.validate(settings, randomBoolean(), false, randomBoolean()) ); - assertThat(e, hasToString(containsString("unknown setting [index.version.upgraded]"))); + assertThat(e, hasToString(containsString("unknown setting [index.creation_date]"))); } // nothing should happen since we are ignoring private settings diff --git a/test/framework/src/main/java/org/opensearch/test/InternalSettingsPlugin.java b/test/framework/src/main/java/org/opensearch/test/InternalSettingsPlugin.java index 96919f65f88fc..986cfd9c5b613 100644 --- a/test/framework/src/main/java/org/opensearch/test/InternalSettingsPlugin.java +++ b/test/framework/src/main/java/org/opensearch/test/InternalSettingsPlugin.java @@ -31,6 +31,7 @@ package org.opensearch.test; +import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.unit.TimeValue; @@ -58,6 +59,13 @@ public final class InternalSettingsPlugin extends Plugin { Property.IndexScope, Property.NodeScope ); + public static final Setting INDEX_CREATION_DATE_SETTING = Setting.longSetting( + IndexMetadata.SETTING_CREATION_DATE, + -1, + -1, + Property.IndexScope, + Property.NodeScope + ); public static final Setting TRANSLOG_RETENTION_CHECK_INTERVAL_SETTING = Setting.timeSetting( "index.translog.retention.check_interval", new TimeValue(10, TimeUnit.MINUTES), @@ -70,6 +78,7 @@ public final class InternalSettingsPlugin extends Plugin { public List> getSettings() { return Arrays.asList( MERGE_ENABLED, + INDEX_CREATION_DATE_SETTING, PROVIDED_NAME_SETTING, TRANSLOG_RETENTION_CHECK_INTERVAL_SETTING, RemoteConnectionStrategy.REMOTE_MAX_PENDING_CONNECTION_LISTENERS, From 6dc984006e82453bdd26f45879a749b18e7ac9d5 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Tue, 21 Jan 2025 15:22:02 -0800 Subject: [PATCH 08/13] fixes by adding back USER_UNMODIFIABLE_SETTINGS for settings without Setting implementation Signed-off-by: AnnTian Shao --- .../remotestore/RemoteRestoreSnapshotIT.java | 25 +++++++++++-- .../RestoreShallowSnapshotV2IT.java | 25 +++++++++++-- .../cluster/metadata/IndexMetadata.java | 22 ----------- .../common/settings/IndexScopedSettings.java | 3 -- .../opensearch/common/settings/Setting.java | 2 +- .../opensearch/snapshots/RestoreService.java | 37 +++++++++++-------- 6 files changed, 64 insertions(+), 50 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index 8ab4be1ca9bde..837212badbf5b 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -767,6 +767,23 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); + // try index restore with index.uuid setting modified + Settings uuidSetting = Settings.builder().put(IndexMetadata.SETTING_INDEX_UUID, IndexMetadata.INDEX_UUID_NA_VALUE).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(uuidSetting) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.uuid]" + " on restore")); + // try index restore with index.number_of_shards setting modified Settings numberOfShardsSettingsDiff = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build(); @@ -824,9 +841,9 @@ public void testInvalidRestoreRequestScenarios() throws Exception { // try index restore with multiple UnmodifiableOnRestore settings on restore Settings unmodifiableSettings = Settings.builder() - .put(IndexMetadata.SETTING_CREATION_DATE, -1L) - .put(IndexMetadata.SETTING_INDEX_UUID, IndexMetadata.INDEX_UUID_NA_VALUE) - .put(IndexMetadata.SETTING_HISTORY_UUID, IndexMetadata.INDEX_UUID_NA_VALUE) + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) .build(); exception = expectThrows( @@ -841,7 +858,7 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.creation_date]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); // try index restore with remote store repository and translog store repository disabled exception = expectThrows( diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java index f474a7ec0301b..65fbfdf2a09e0 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java @@ -834,6 +834,23 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); + // try index restore with index.uuid setting modified + Settings uuidSetting = Settings.builder().put(IndexMetadata.SETTING_INDEX_UUID, IndexMetadata.INDEX_UUID_NA_VALUE).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(uuidSetting) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.uuid]" + " on restore")); + // try index restore with index.number_of_shards setting modified Settings numberOfShardsSettingsDiff = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build(); @@ -891,9 +908,9 @@ public void testInvalidRestoreRequestScenarios() throws Exception { // try index restore with multiple UnmodifiableOnRestore settings on restore Settings unmodifiableSettings = Settings.builder() - .put(IndexMetadata.SETTING_CREATION_DATE, -1L) - .put(IndexMetadata.SETTING_INDEX_UUID, IndexMetadata.INDEX_UUID_NA_VALUE) - .put(IndexMetadata.SETTING_HISTORY_UUID, IndexMetadata.INDEX_UUID_NA_VALUE) + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) .build(); exception = expectThrows( @@ -908,7 +925,7 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.creation_date]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); // try index restore with remote store repository and translog store repository disabled exception = expectThrows( diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index f41e81177d06d..a9eddcce6b78c 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -579,12 +579,6 @@ public static APIBlock readFrom(StreamInput input) throws IOException { public static final String SETTING_VERSION_UPGRADED_STRING = "index.version.upgraded_string"; public static final String SETTING_CREATION_DATE = "index.creation_date"; - public static final Setting SETTING_INDEX_CREATION_DATE = Setting.longSetting( - SETTING_CREATION_DATE, - -1L, - Property.IndexScope, - Property.UnmodifiableOnRestore - ); /** * The user provided name for an index. This is the plain string provided by the user when the index was created. * It might still contain date math expressions etc. (added in 5.0) @@ -611,22 +605,6 @@ public static APIBlock readFrom(StreamInput input) throws IOException { public static final String INDEX_UUID_NA_VALUE = Strings.UNKNOWN_UUID_VALUE; - public static final Setting INDEX_UUID_SETTING = Setting.simpleString( - SETTING_INDEX_UUID, - INDEX_UUID_NA_VALUE, - Property.IndexScope, - Property.PrivateIndex, - Property.UnmodifiableOnRestore - ); - - public static final Setting SETTING_INDEX_HISTORY_UUID = Setting.simpleString( - SETTING_HISTORY_UUID, - INDEX_UUID_NA_VALUE, - Property.IndexScope, - Property.PrivateIndex, - Property.UnmodifiableOnRestore - ); - public static final String INDEX_ROUTING_REQUIRE_GROUP_PREFIX = "index.routing.allocation.require"; public static final String INDEX_ROUTING_INCLUDE_GROUP_PREFIX = "index.routing.allocation.include"; public static final String INDEX_ROUTING_EXCLUDE_GROUP_PREFIX = "index.routing.allocation.exclude"; diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index 8f59c928d758b..8d56a942c5d6e 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -91,9 +91,6 @@ public final class IndexScopedSettings extends AbstractScopedSettings { MergeSchedulerConfig.MAX_MERGE_COUNT_SETTING, MergeSchedulerConfig.MAX_THREAD_COUNT_SETTING, IndexMetadata.SETTING_INDEX_VERSION_CREATED, - IndexMetadata.INDEX_UUID_SETTING, - IndexMetadata.SETTING_INDEX_CREATION_DATE, - IndexMetadata.SETTING_INDEX_HISTORY_UUID, IndexMetadata.INDEX_ROUTING_EXCLUDE_GROUP_SETTING, IndexMetadata.INDEX_ROUTING_INCLUDE_GROUP_SETTING, IndexMetadata.INDEX_ROUTING_REQUIRE_GROUP_SETTING, diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java index f5f49e5c5c6ad..d061cae0f96a0 100644 --- a/server/src/main/java/org/opensearch/common/settings/Setting.java +++ b/server/src/main/java/org/opensearch/common/settings/Setting.java @@ -174,7 +174,7 @@ public enum Property { ExtensionScope, /** - * Mark this setting as immutable on restore snapshot + * Mark this setting as immutable on snapshot restore */ UnmodifiableOnRestore } diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index c27821274599d..8ee62ad83501f 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -163,25 +163,28 @@ public class RestoreService implements ClusterStateApplier { private static final Logger logger = LogManager.getLogger(RestoreService.class); - // It's OK to change some settings, but we shouldn't allow simply removing them - private static final Set USER_UNREMOVABLE_SETTINGS = unmodifiableSet( - newHashSet( - SETTING_VERSION_CREATED, - SETTING_INDEX_UUID, - SETTING_CREATION_DATE, - SETTING_HISTORY_UUID, - SETTING_REMOTE_STORE_ENABLED, - SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, - SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, - SETTING_NUMBER_OF_SHARDS, - SETTING_NUMBER_OF_REPLICAS, - SETTING_AUTO_EXPAND_REPLICAS, - SETTING_VERSION_UPGRADED - ) + private static final Set USER_UNMODIFIABLE_SETTINGS = unmodifiableSet( + newHashSet(SETTING_INDEX_UUID, SETTING_CREATION_DATE, SETTING_HISTORY_UUID) ); + // It's OK to change some settings, but we shouldn't allow simply removing them + private static final Set USER_UNREMOVABLE_SETTINGS; private static final String REMOTE_STORE_INDEX_SETTINGS_REGEX = "index.remote_store.*"; + static { + Set unremovable = new HashSet<>(USER_UNMODIFIABLE_SETTINGS.size() + 8); + unremovable.addAll(USER_UNMODIFIABLE_SETTINGS); + unremovable.add(SETTING_NUMBER_OF_SHARDS); + unremovable.add(SETTING_VERSION_CREATED); + unremovable.add(SETTING_REMOTE_STORE_ENABLED); + unremovable.add(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY); + unremovable.add(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY); + unremovable.add(SETTING_NUMBER_OF_REPLICAS); + unremovable.add(SETTING_AUTO_EXPAND_REPLICAS); + unremovable.add(SETTING_VERSION_UPGRADED); + USER_UNREMOVABLE_SETTINGS = unmodifiableSet(unremovable); + } + private final ClusterService clusterService; private final RepositoriesService repositoriesService; @@ -867,7 +870,9 @@ private IndexMetadata updateIndexSettings( Settings.Builder settingsBuilder = Settings.builder() .put(settings.filter(settingsFilter)) .put(normalizedChangeSettings.filter(k -> { - if (indexScopedSettings.isUnmodifiableOnRestoreSetting(k)) { + if (USER_UNMODIFIABLE_SETTINGS.contains(k)) { + throw new SnapshotRestoreException(snapshot, "cannot modify setting [" + k + "] on restore"); + } else if (indexScopedSettings.isUnmodifiableOnRestoreSetting(k)) { throw new SnapshotRestoreException( snapshot, "cannot modify UnmodifiableOnRestore setting [" + k + "] on restore" From 9ebab5a7ac9fa3a15b436168f227556bf18ace87 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Tue, 21 Jan 2025 18:41:27 -0800 Subject: [PATCH 09/13] testing CI config for registering plugin settings Signed-off-by: AnnTian Shao --- .../remotestore/RemoteRestoreSnapshotIT.java | 16 ++++++++++++++++ .../org/opensearch/snapshots/RestoreService.java | 5 ++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index 837212badbf5b..a22312eb33ce7 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -801,6 +801,22 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + Settings creationDate = Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, -1).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(creationDate) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.creation_date]" + " on restore")); + // try index restore with index.number_of_shards setting same Settings numberOfShardsSettingsSame = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).build(); diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index 8ee62ad83501f..96204e83e39ba 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -164,7 +164,10 @@ public class RestoreService implements ClusterStateApplier { private static final Logger logger = LogManager.getLogger(RestoreService.class); private static final Set USER_UNMODIFIABLE_SETTINGS = unmodifiableSet( - newHashSet(SETTING_INDEX_UUID, SETTING_CREATION_DATE, SETTING_HISTORY_UUID) + newHashSet( + SETTING_INDEX_UUID, +// SETTING_CREATION_DATE, + SETTING_HISTORY_UUID) ); // It's OK to change some settings, but we shouldn't allow simply removing them From ba81816ce295bd68017fd730d2474c8ee12ccad2 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Wed, 22 Jan 2025 08:52:25 -0800 Subject: [PATCH 10/13] Revert "testing CI config for registering plugin settings" This reverts commit 9ebab5a7ac9fa3a15b436168f227556bf18ace87. Signed-off-by: AnnTian Shao --- .../remotestore/RemoteRestoreSnapshotIT.java | 16 ---------------- .../org/opensearch/snapshots/RestoreService.java | 5 +---- 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index a22312eb33ce7..837212badbf5b 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -801,22 +801,6 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - Settings creationDate = Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, -1).build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(creationDate) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.creation_date]" + " on restore")); - // try index restore with index.number_of_shards setting same Settings numberOfShardsSettingsSame = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).build(); diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index 96204e83e39ba..8ee62ad83501f 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -164,10 +164,7 @@ public class RestoreService implements ClusterStateApplier { private static final Logger logger = LogManager.getLogger(RestoreService.class); private static final Set USER_UNMODIFIABLE_SETTINGS = unmodifiableSet( - newHashSet( - SETTING_INDEX_UUID, -// SETTING_CREATION_DATE, - SETTING_HISTORY_UUID) + newHashSet(SETTING_INDEX_UUID, SETTING_CREATION_DATE, SETTING_HISTORY_UUID) ); // It's OK to change some settings, but we shouldn't allow simply removing them From dc281ce18f0bfad9d11f89e7cba839e51f5eba3f Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Wed, 22 Jan 2025 18:48:09 -0800 Subject: [PATCH 11/13] Add UnmodifiableOnRestore only to unmodifiable settings that are already registered, will raise separate PR for settings not yet registered. Add validation check in Setting.java. Add UnmodifiableOnRestore settings cannot be removed on restore Signed-off-by: AnnTian Shao --- .../remotestore/RemoteRestoreSnapshotIT.java | 180 +++++++++++++++--- .../RestoreShallowSnapshotV2IT.java | 180 +++++++++++++++--- .../cluster/metadata/IndexMetadata.java | 9 +- .../metadata/MetadataCreateIndexService.java | 4 + .../opensearch/common/settings/Setting.java | 11 +- .../opensearch/indices/IndicesService.java | 4 - .../opensearch/snapshots/RestoreService.java | 27 +-- .../common/settings/ScopedSettingsTests.java | 8 +- .../common/settings/SettingTests.java | 26 +++ 9 files changed, 377 insertions(+), 72 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index 837212badbf5b..73de2fffb471a 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -73,7 +73,6 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.SEGMENTS; import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.TRANSLOG; import static org.opensearch.index.remote.RemoteStoreEnums.DataType.DATA; @@ -752,14 +751,14 @@ public void testInvalidRestoreRequestScenarios() throws Exception { indexDocuments(client, index, numDocsInIndex, numDocsInIndex + randomIntBetween(2, 5)); ensureGreen(index); - // try index restore with remote store disabled + // try index restore with USER_UNREMOVABLE_SETTINGS setting disabled SnapshotRestoreException exception = expectThrows( SnapshotRestoreException.class, () -> client().admin() .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIgnoreIndexSettings(SETTING_REMOTE_STORE_ENABLED) + .setIgnoreIndexSettings(IndexMetadata.SETTING_REMOTE_STORE_ENABLED) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) @@ -767,24 +766,99 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); - // try index restore with index.uuid setting modified - Settings uuidSetting = Settings.builder().put(IndexMetadata.SETTING_INDEX_UUID, IndexMetadata.INDEX_UUID_NA_VALUE).build(); + // try index restore with UnmodifiableOnRestore setting disabled + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards] on restore")); + + // try index restore with mix of removable and UnmodifiableOnRestore settings disabled + // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is removable + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_VERSION_CREATED, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.version.created] on restore")); + // try index restore with mix of removable and USER_UNREMOVABLE_SETTINGS settings disabled + // index.number_of_replicas is USER_UNREMOVABLE_SETTINGS, index.number_of_search_only_replicas is removable exception = expectThrows( SnapshotRestoreException.class, () -> client().admin() .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIndexSettings(uuidSetting) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.uuid]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas] on restore")); - // try index restore with index.number_of_shards setting modified + // try index restore with multiple UnmodifiableOnRestore settings disabled + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS, IndexMetadata.SETTING_VERSION_CREATED) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + + // try index restore with multiple USER_UNREMOVABLE_SETTINGS settings disabled + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); + + // try index restore with mix of UnmodifiableOnRestore and USER_UNREMOVABLE_SETTINGS settings disabled + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SHARDS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); + + // try index restore with UnmodifiableOnRestore setting modified Settings numberOfShardsSettingsDiff = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build(); exception = expectThrows( @@ -801,7 +875,7 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - // try index restore with index.number_of_shards setting same + // try index restore with UnmodifiableOnRestore setting same Settings numberOfShardsSettingsSame = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).build(); exception = expectThrows( @@ -818,11 +892,70 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - // try index restore with mix of modifiable and unmodifiable settings on restore - // index.version.created is unmodifiable, index.number_of_replicas is modifiable + // try index restore with USER_UNMODIFIABLE_SETTINGS setting modified + Settings remoteStoreEnabledSetting = Settings.builder().put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(remoteStoreEnabledSetting) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + // try index restore with mix of modifiable and UnmodifiableOnRestore settings modified + // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is modifiable + Settings mixedSettingsUnmodifiableOnRestore = Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) + .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(mixedSettingsUnmodifiableOnRestore) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.version.created]" + " on restore")); + + // try index restore with mix of modifiable and USER_UNMODIFIABLE_SETTINGS settings modified + // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.number_of_search_only_replicas is modifiable + Settings mixedSettingsUserUnmodifiableSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) + .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(mixedSettingsUserUnmodifiableSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + // try index restore with mix of UnmodifiableOnRestore and USER_UNMODIFIABLE_SETTINGS settings modified + // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.version.created is UnmodifiableOnRestore Settings mixedSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1) .build(); exception = expectThrows( @@ -837,13 +970,12 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.version.created]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - // try index restore with multiple UnmodifiableOnRestore settings on restore - Settings unmodifiableSettings = Settings.builder() + // try index restore with multiple UnmodifiableOnRestore settings modified + Settings unmodifiableOnRestoreSettings = Settings.builder() .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) .build(); exception = expectThrows( @@ -852,7 +984,7 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIndexSettings(unmodifiableSettings) + .setIndexSettings(unmodifiableOnRestoreSettings) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) @@ -860,23 +992,25 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - // try index restore with remote store repository and translog store repository disabled + // try index restore with multiple USER_UNMODIFIABLE_SETTINGS settings modified + Settings userUnmodifiableSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) + .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) + .build(); + exception = expectThrows( SnapshotRestoreException.class, () -> client().admin() .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIgnoreIndexSettings( - IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, - IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY - ) + .setIndexSettings(userUnmodifiableSettings) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.segment.repository]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); } public void testCreateSnapshotV2_Orphan_Timestamp_Cleanup() throws Exception { diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java index 65fbfdf2a09e0..cf719f6e6024d 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java @@ -68,7 +68,6 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; import static org.opensearch.index.query.QueryBuilders.matchAllQuery; import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.SEGMENTS; import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.TRANSLOG; @@ -819,14 +818,14 @@ public void testInvalidRestoreRequestScenarios() throws Exception { indexDocuments(client, index, numDocsInIndex, numDocsInIndex + randomIntBetween(2, 5)); ensureGreen(index); - // try index restore with remote store disabled + // try index restore with USER_UNREMOVABLE_SETTINGS setting disabled SnapshotRestoreException exception = expectThrows( SnapshotRestoreException.class, () -> client().admin() .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIgnoreIndexSettings(SETTING_REMOTE_STORE_ENABLED) + .setIgnoreIndexSettings(IndexMetadata.SETTING_REMOTE_STORE_ENABLED) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) @@ -834,24 +833,99 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); - // try index restore with index.uuid setting modified - Settings uuidSetting = Settings.builder().put(IndexMetadata.SETTING_INDEX_UUID, IndexMetadata.INDEX_UUID_NA_VALUE).build(); + // try index restore with UnmodifiableOnRestore setting disabled + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards] on restore")); + + // try index restore with mix of removable and UnmodifiableOnRestore settings disabled + // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is removable + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_VERSION_CREATED, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.version.created] on restore")); + // try index restore with mix of removable and USER_UNREMOVABLE_SETTINGS settings disabled + // index.number_of_replicas is USER_UNREMOVABLE_SETTINGS, index.number_of_search_only_replicas is removable exception = expectThrows( SnapshotRestoreException.class, () -> client().admin() .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIndexSettings(uuidSetting) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.uuid]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas] on restore")); - // try index restore with index.number_of_shards setting modified + // try index restore with multiple UnmodifiableOnRestore settings disabled + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS, IndexMetadata.SETTING_VERSION_CREATED) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + + // try index restore with multiple USER_UNREMOVABLE_SETTINGS settings disabled + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); + + // try index restore with mix of UnmodifiableOnRestore and USER_UNREMOVABLE_SETTINGS settings disabled + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SHARDS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); + + // try index restore with UnmodifiableOnRestore setting modified Settings numberOfShardsSettingsDiff = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build(); exception = expectThrows( @@ -868,7 +942,7 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - // try index restore with index.number_of_shards setting same + // try index restore with UnmodifiableOnRestore setting same Settings numberOfShardsSettingsSame = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).build(); exception = expectThrows( @@ -885,11 +959,70 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - // try index restore with mix of modifiable and unmodifiable settings on restore - // index.version.created is unmodifiable, index.number_of_replicas is modifiable + // try index restore with USER_UNMODIFIABLE_SETTINGS setting modified + Settings remoteStoreEnabledSetting = Settings.builder().put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(remoteStoreEnabledSetting) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + // try index restore with mix of modifiable and UnmodifiableOnRestore settings modified + // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is modifiable + Settings mixedSettingsUnmodifiableOnRestore = Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) + .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(mixedSettingsUnmodifiableOnRestore) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.version.created]" + " on restore")); + + // try index restore with mix of modifiable and USER_UNMODIFIABLE_SETTINGS settings modified + // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.number_of_search_only_replicas is modifiable + Settings mixedSettingsUserUnmodifiableSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) + .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(mixedSettingsUserUnmodifiableSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + // try index restore with mix of UnmodifiableOnRestore and USER_UNMODIFIABLE_SETTINGS settings modified + // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.version.created is UnmodifiableOnRestore Settings mixedSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1) .build(); exception = expectThrows( @@ -904,13 +1037,12 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.version.created]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - // try index restore with multiple UnmodifiableOnRestore settings on restore - Settings unmodifiableSettings = Settings.builder() + // try index restore with multiple UnmodifiableOnRestore settings modified + Settings unmodifiableOnRestoreSettings = Settings.builder() .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) .build(); exception = expectThrows( @@ -919,7 +1051,7 @@ public void testInvalidRestoreRequestScenarios() throws Exception { .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIndexSettings(unmodifiableSettings) + .setIndexSettings(unmodifiableOnRestoreSettings) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) @@ -927,23 +1059,25 @@ public void testInvalidRestoreRequestScenarios() throws Exception { ); assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - // try index restore with remote store repository and translog store repository disabled + // try index restore with multiple USER_UNMODIFIABLE_SETTINGS settings modified + Settings userUnmodifiableSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) + .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) + .build(); + exception = expectThrows( SnapshotRestoreException.class, () -> client().admin() .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) - .setIgnoreIndexSettings( - IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, - IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY - ) + .setIndexSettings(userUnmodifiableSettings) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(restoredIndex) .get() ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.segment.repository]" + " on restore")); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); } public void testRestoreOperationsUsingDifferentRepos() throws Exception { diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index a9eddcce6b78c..cee331788e4b7 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -390,8 +390,7 @@ public Iterator> settings() { }, Property.IndexScope, Property.PrivateIndex, - Property.Dynamic, - Property.UnmodifiableOnRestore + Property.Dynamic ); /** @@ -425,8 +424,7 @@ public Iterator> settings() { }, Property.IndexScope, Property.PrivateIndex, - Property.Dynamic, - Property.UnmodifiableOnRestore + Property.Dynamic ); private static void validateRemoteStoreSettingEnabled(final Map, Object> settings, Setting setting) { @@ -477,8 +475,7 @@ public Iterator> settings() { }, Property.IndexScope, Property.PrivateIndex, - Property.Dynamic, - Property.UnmodifiableOnRestore + Property.Dynamic ); public static final String SETTING_AUTO_EXPAND_REPLICAS = "index.auto_expand_replicas"; diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index b5b2b71f977fa..f052e9940bb9a 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -231,6 +231,10 @@ public MetadataCreateIndexService( : null; } + public IndexScopedSettings getIndexScopedSettings() { + return indexScopedSettings; + } + /** * Add a provider to be invoked to get additional index settings prior to an index being created */ diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java index d061cae0f96a0..c915718e4a433 100644 --- a/server/src/main/java/org/opensearch/common/settings/Setting.java +++ b/server/src/main/java/org/opensearch/common/settings/Setting.java @@ -213,7 +213,16 @@ private Setting( final EnumSet propertiesAsSet = EnumSet.copyOf(Arrays.asList(properties)); if (propertiesAsSet.contains(Property.Dynamic) && propertiesAsSet.contains(Property.Final)) { throw new IllegalArgumentException("final setting [" + key + "] cannot be dynamic"); - } + } else if (propertiesAsSet.contains(Property.UnmodifiableOnRestore) + && (propertiesAsSet.contains(Property.Dynamic) || !propertiesAsSet.contains(Property.IndexScope))) { + throw new IllegalArgumentException( + "unmodifiableOnRestore setting [" + + key + + "] cannot be dynamic, or unmodifiableOnRestore setting [" + + key + + "] must have indexScope" + ); + } checkPropertyRequiresIndexScope(propertiesAsSet, Property.NotCopyableOnResize); checkPropertyRequiresIndexScope(propertiesAsSet, Property.InternalIndex); checkPropertyRequiresIndexScope(propertiesAsSet, Property.PrivateIndex); diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index c0472fb9ebc70..b9bad5527e3f4 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -1182,10 +1182,6 @@ public IndicesQueryCache getIndicesQueryCache() { return indicesQueryCache; } - public IndexScopedSettings getIndexScopedSettings() { - return indexScopedSettings; - } - /** * Accumulate stats from the passed Object * diff --git a/server/src/main/java/org/opensearch/snapshots/RestoreService.java b/server/src/main/java/org/opensearch/snapshots/RestoreService.java index 8ee62ad83501f..29ced9d5f0f0c 100644 --- a/server/src/main/java/org/opensearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/opensearch/snapshots/RestoreService.java @@ -123,12 +123,10 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_INDEX_UUID; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_CREATED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_VERSION_UPGRADED; import static org.opensearch.common.util.FeatureFlags.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY; import static org.opensearch.common.util.IndexUtils.filterIndices; @@ -164,7 +162,14 @@ public class RestoreService implements ClusterStateApplier { private static final Logger logger = LogManager.getLogger(RestoreService.class); private static final Set USER_UNMODIFIABLE_SETTINGS = unmodifiableSet( - newHashSet(SETTING_INDEX_UUID, SETTING_CREATION_DATE, SETTING_HISTORY_UUID) + newHashSet( + SETTING_INDEX_UUID, + SETTING_CREATION_DATE, + SETTING_HISTORY_UUID, + SETTING_REMOTE_STORE_ENABLED, + SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, + SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY + ) ); // It's OK to change some settings, but we shouldn't allow simply removing them @@ -172,13 +177,8 @@ public class RestoreService implements ClusterStateApplier { private static final String REMOTE_STORE_INDEX_SETTINGS_REGEX = "index.remote_store.*"; static { - Set unremovable = new HashSet<>(USER_UNMODIFIABLE_SETTINGS.size() + 8); + Set unremovable = new HashSet<>(USER_UNMODIFIABLE_SETTINGS.size() + 3); unremovable.addAll(USER_UNMODIFIABLE_SETTINGS); - unremovable.add(SETTING_NUMBER_OF_SHARDS); - unremovable.add(SETTING_VERSION_CREATED); - unremovable.add(SETTING_REMOTE_STORE_ENABLED); - unremovable.add(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY); - unremovable.add(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY); unremovable.add(SETTING_NUMBER_OF_REPLICAS); unremovable.add(SETTING_AUTO_EXPAND_REPLICAS); unremovable.add(SETTING_VERSION_UPGRADED); @@ -233,7 +233,7 @@ public RestoreService( this.clusterSettings = clusterService.getClusterSettings(); this.shardLimitValidator = shardLimitValidator; this.indicesService = indicesService; - this.indexScopedSettings = indicesService.getIndexScopedSettings(); + this.indexScopedSettings = createIndexService.getIndexScopedSettings(); this.clusterInfoSupplier = clusterInfoSupplier; this.dataToFileCacheSizeRatioSupplier = dataToFileCacheSizeRatioSupplier; @@ -835,6 +835,11 @@ private IndexMetadata updateIndexSettings( snapshot, "cannot remove setting [" + ignoredSetting + "] on restore" ); + } else if (indexScopedSettings.isUnmodifiableOnRestoreSetting(ignoredSetting)) { + throw new SnapshotRestoreException( + snapshot, + "cannot remove UnmodifiableOnRestore setting [" + ignoredSetting + "] on restore" + ); } else { keyFilters.add(ignoredSetting); } @@ -853,7 +858,7 @@ private IndexMetadata updateIndexSettings( } Predicate settingsFilter = k -> { - if (USER_UNREMOVABLE_SETTINGS.contains(k) == false) { + if (USER_UNREMOVABLE_SETTINGS.contains(k) == false && !indexScopedSettings.isUnmodifiableOnRestoreSetting(k)) { for (String filterKey : keyFilters) { if (k.equals(filterKey)) { return false; diff --git a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java index cb5b259954453..55e3cfa34040b 100644 --- a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java @@ -794,10 +794,10 @@ public void testIsUnmodifiableOnRestore() { Settings.EMPTY, new HashSet<>( Arrays.asList( - Setting.intSetting("foo.int", 1, Property.UnmodifiableOnRestore, Property.NodeScope), - Setting.groupSetting("foo.group.", Property.UnmodifiableOnRestore, Property.NodeScope), - Setting.groupSetting("foo.list.", Property.UnmodifiableOnRestore, Property.NodeScope), - Setting.intSetting("foo.int.baz", 1, Property.NodeScope) + Setting.intSetting("foo.int", 1, Property.UnmodifiableOnRestore, Property.IndexScope, Property.NodeScope), + Setting.groupSetting("foo.group.", Property.UnmodifiableOnRestore, Property.IndexScope, Property.NodeScope), + Setting.groupSetting("foo.list.", Property.UnmodifiableOnRestore, Property.IndexScope, Property.NodeScope), + Setting.intSetting("foo.int.baz", 1, Property.IndexScope, Property.NodeScope) ) ) ); diff --git a/server/src/test/java/org/opensearch/common/settings/SettingTests.java b/server/src/test/java/org/opensearch/common/settings/SettingTests.java index c3c399a9d88b2..a582da2141304 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingTests.java @@ -1439,6 +1439,32 @@ public void testRejectConflictingDynamicAndFinalProperties() { assertThat(ex.getMessage(), containsString("final setting [foo.bar] cannot be dynamic")); } + public void testRejectConflictingDynamicAndUnmodifiableOnRestoreProperties() { + IllegalArgumentException ex = expectThrows( + IllegalArgumentException.class, + () -> Setting.simpleString("foo.bar", Property.UnmodifiableOnRestore, Property.Dynamic) + ); + assertThat( + ex.getMessage(), + containsString( + "unmodifiableOnRestore setting [foo.bar] cannot be dynamic, or unmodifiableOnRestore setting [foo.bar] must have indexScope" + ) + ); + } + + public void testRejectMissingIndexScopeAndUnmodifiableOnRestoreProperties() { + IllegalArgumentException ex = expectThrows( + IllegalArgumentException.class, + () -> Setting.simpleString("foo.bar", Property.UnmodifiableOnRestore) + ); + assertThat( + ex.getMessage(), + containsString( + "unmodifiableOnRestore setting [foo.bar] cannot be dynamic, or unmodifiableOnRestore setting [foo.bar] must have indexScope" + ) + ); + } + public void testRejectNonIndexScopedNotCopyableOnResizeSetting() { final IllegalArgumentException e = expectThrows( IllegalArgumentException.class, From b55cbeda50e2f2257309deba5c9ce27ffefc98d6 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Thu, 23 Jan 2025 14:11:56 -0800 Subject: [PATCH 12/13] fixes and move tests to RestoreSnapshotIT Signed-off-by: AnnTian Shao --- .../remotestore/RemoteRestoreSnapshotIT.java | 307 +------------- .../RestoreShallowSnapshotV2IT.java | 307 +------------- .../snapshots/RestoreSnapshotIT.java | 388 ++++++++++++++++++ .../opensearch/common/settings/Setting.java | 15 +- .../common/settings/SettingTests.java | 18 +- 5 files changed, 399 insertions(+), 636 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index 73de2fffb471a..8fc119d49c3d1 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -8,7 +8,6 @@ package org.opensearch.remotestore; -import org.opensearch.Version; import org.opensearch.action.DocWriteResponse; import org.opensearch.action.LatchedActionListener; import org.opensearch.action.admin.cluster.remotestore.restore.RestoreRemoteStoreRequest; @@ -51,7 +50,6 @@ import org.opensearch.repositories.blobstore.BlobStoreRepository; import org.opensearch.repositories.fs.FsRepository; import org.opensearch.snapshots.SnapshotInfo; -import org.opensearch.snapshots.SnapshotRestoreException; import org.opensearch.snapshots.SnapshotState; import org.opensearch.test.InternalTestCluster; import org.opensearch.test.OpenSearchIntegTestCase; @@ -494,7 +492,7 @@ public void testRemoteRestoreIndexRestoredFromSnapshot() throws IOException, Exe assertDocsPresentInIndex(client(), indexName1, numDocsInIndex1); } - public void testIndexRestoredFromSnapshotWithUpdateSetting() throws IOException, ExecutionException, InterruptedException { + public void testSuccessfulIndexRestoredFromSnapshotWithUpdatedSetting() throws IOException, ExecutionException, InterruptedException { internalCluster().startClusterManagerOnlyNode(); internalCluster().startDataOnlyNodes(2); @@ -710,309 +708,6 @@ public void testRestoreShallowSnapshotIndexAfterSnapshot() throws ExecutionExcep assertDocsPresentInIndex(client, restoredIndexName1, numDocsInIndex1 + 2); } - public void testInvalidRestoreRequestScenarios() throws Exception { - internalCluster().startClusterManagerOnlyNode(); - internalCluster().startDataOnlyNode(); - String index = "test-index"; - String snapshotRepo = "test-restore-snapshot-repo"; - String newRemoteStoreRepo = "test-new-rs-repo"; - String snapshotName1 = "test-restore-snapshot1"; - String snapshotName2 = "test-restore-snapshot2"; - Path absolutePath1 = randomRepoPath().toAbsolutePath(); - logger.info("Snapshot Path [{}]", absolutePath1); - String restoredIndex = index + "-restored"; - - createRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, true)); - - Client client = client(); - Settings indexSettings = getIndexSettings(1, 0).build(); - createIndex(index, indexSettings); - - final int numDocsInIndex = 5; - indexDocuments(client, index, numDocsInIndex); - ensureGreen(index); - - internalCluster().startDataOnlyNode(); - logger.info("--> snapshot"); - - SnapshotInfo snapshotInfo = createSnapshot(snapshotRepo, snapshotName1, new ArrayList<>(List.of(index))); - assertThat(snapshotInfo.state(), equalTo(SnapshotState.SUCCESS)); - assertThat(snapshotInfo.successfulShards(), greaterThan(0)); - assertThat(snapshotInfo.successfulShards(), equalTo(snapshotInfo.totalShards())); - - updateRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, false)); - SnapshotInfo snapshotInfo2 = createSnapshot(snapshotRepo, snapshotName2, new ArrayList<>(List.of(index))); - assertThat(snapshotInfo2.state(), equalTo(SnapshotState.SUCCESS)); - assertThat(snapshotInfo2.successfulShards(), greaterThan(0)); - assertThat(snapshotInfo2.successfulShards(), equalTo(snapshotInfo2.totalShards())); - - DeleteResponse deleteResponse = client().prepareDelete(index, "0").execute().actionGet(); - assertEquals(deleteResponse.getResult(), DocWriteResponse.Result.DELETED); - indexDocuments(client, index, numDocsInIndex, numDocsInIndex + randomIntBetween(2, 5)); - ensureGreen(index); - - // try index restore with USER_UNREMOVABLE_SETTINGS setting disabled - SnapshotRestoreException exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_REMOTE_STORE_ENABLED) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); - - // try index restore with UnmodifiableOnRestore setting disabled - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards] on restore")); - - // try index restore with mix of removable and UnmodifiableOnRestore settings disabled - // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is removable - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_VERSION_CREATED, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.version.created] on restore")); - - // try index restore with mix of removable and USER_UNREMOVABLE_SETTINGS settings disabled - // index.number_of_replicas is USER_UNREMOVABLE_SETTINGS, index.number_of_search_only_replicas is removable - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas] on restore")); - - // try index restore with multiple UnmodifiableOnRestore settings disabled - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS, IndexMetadata.SETTING_VERSION_CREATED) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - - // try index restore with multiple USER_UNREMOVABLE_SETTINGS settings disabled - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); - - // try index restore with mix of UnmodifiableOnRestore and USER_UNREMOVABLE_SETTINGS settings disabled - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SHARDS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); - - // try index restore with UnmodifiableOnRestore setting modified - Settings numberOfShardsSettingsDiff = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(numberOfShardsSettingsDiff) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - - // try index restore with UnmodifiableOnRestore setting same - Settings numberOfShardsSettingsSame = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(numberOfShardsSettingsSame) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - - // try index restore with USER_UNMODIFIABLE_SETTINGS setting modified - Settings remoteStoreEnabledSetting = Settings.builder().put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false).build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(remoteStoreEnabledSetting) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - - // try index restore with mix of modifiable and UnmodifiableOnRestore settings modified - // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is modifiable - Settings mixedSettingsUnmodifiableOnRestore = Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(mixedSettingsUnmodifiableOnRestore) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.version.created]" + " on restore")); - - // try index restore with mix of modifiable and USER_UNMODIFIABLE_SETTINGS settings modified - // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.number_of_search_only_replicas is modifiable - Settings mixedSettingsUserUnmodifiableSettings = Settings.builder() - .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) - .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(mixedSettingsUserUnmodifiableSettings) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - - // try index restore with mix of UnmodifiableOnRestore and USER_UNMODIFIABLE_SETTINGS settings modified - // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.version.created is UnmodifiableOnRestore - Settings mixedSettings = Settings.builder() - .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(mixedSettings) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - - // try index restore with multiple UnmodifiableOnRestore settings modified - Settings unmodifiableOnRestoreSettings = Settings.builder() - .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(unmodifiableOnRestoreSettings) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - - // try index restore with multiple USER_UNMODIFIABLE_SETTINGS settings modified - Settings userUnmodifiableSettings = Settings.builder() - .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) - .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(userUnmodifiableSettings) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - } - public void testCreateSnapshotV2_Orphan_Timestamp_Cleanup() throws Exception { internalCluster().startClusterManagerOnlyNode(pinnedTimestampSettings()); internalCluster().startDataOnlyNode(pinnedTimestampSettings()); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java index cf719f6e6024d..555860fa6a1db 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java @@ -8,7 +8,6 @@ package org.opensearch.remotestore; -import org.opensearch.Version; import org.opensearch.action.DocWriteResponse; import org.opensearch.action.admin.cluster.remotestore.restore.RestoreRemoteStoreRequest; import org.opensearch.action.admin.cluster.repositories.get.GetRepositoriesRequest; @@ -45,7 +44,6 @@ import org.opensearch.repositories.blobstore.BlobStoreRepository; import org.opensearch.snapshots.AbstractSnapshotIntegTestCase; import org.opensearch.snapshots.SnapshotInfo; -import org.opensearch.snapshots.SnapshotRestoreException; import org.opensearch.snapshots.SnapshotState; import org.opensearch.test.BackgroundIndexer; import org.opensearch.test.InternalTestCluster; @@ -561,7 +559,7 @@ public void testRemoteRestoreIndexRestoredFromSnapshot() throws IOException, Exe assertDocsPresentInIndex(client(), indexName1, numDocsInIndex1); } - public void testIndexRestoredFromSnapshotWithUpdateSetting() throws IOException, ExecutionException, InterruptedException { + public void testSuccessfulIndexRestoredFromSnapshotWithUpdatedSetting() throws IOException, ExecutionException, InterruptedException { internalCluster().startClusterManagerOnlyNode(); internalCluster().startDataOnlyNodes(2); @@ -777,309 +775,6 @@ public void testRestoreShallowSnapshotIndexAfterSnapshot() throws ExecutionExcep assertDocsPresentInIndex(client, restoredIndexName1, numDocsInIndex1 + 2); } - public void testInvalidRestoreRequestScenarios() throws Exception { - internalCluster().startClusterManagerOnlyNode(); - internalCluster().startDataOnlyNode(); - String index = "test-index"; - String snapshotRepo = "test-restore-snapshot-repo"; - String newRemoteStoreRepo = "test-new-rs-repo"; - String snapshotName1 = "test-restore-snapshot1"; - String snapshotName2 = "test-restore-snapshot2"; - Path absolutePath1 = randomRepoPath().toAbsolutePath(); - logger.info("Snapshot Path [{}]", absolutePath1); - String restoredIndex = index + "-restored"; - - createRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, true)); - - Client client = client(); - Settings indexSettings = getIndexSettings(1, 0).build(); - createIndex(index, indexSettings); - - final int numDocsInIndex = 5; - indexDocuments(client, index, numDocsInIndex); - ensureGreen(index); - - internalCluster().startDataOnlyNode(); - logger.info("--> snapshot"); - - SnapshotInfo snapshotInfo = createSnapshot(snapshotRepo, snapshotName1, new ArrayList<>(List.of(index))); - assertThat(snapshotInfo.state(), equalTo(SnapshotState.SUCCESS)); - assertThat(snapshotInfo.successfulShards(), greaterThan(0)); - assertThat(snapshotInfo.successfulShards(), equalTo(snapshotInfo.totalShards())); - - updateRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, false)); - SnapshotInfo snapshotInfo2 = createSnapshot(snapshotRepo, snapshotName2, new ArrayList<>(List.of(index))); - assertThat(snapshotInfo2.state(), equalTo(SnapshotState.SUCCESS)); - assertThat(snapshotInfo2.successfulShards(), greaterThan(0)); - assertThat(snapshotInfo2.successfulShards(), equalTo(snapshotInfo2.totalShards())); - - DeleteResponse deleteResponse = client().prepareDelete(index, "0").execute().actionGet(); - assertEquals(deleteResponse.getResult(), DocWriteResponse.Result.DELETED); - indexDocuments(client, index, numDocsInIndex, numDocsInIndex + randomIntBetween(2, 5)); - ensureGreen(index); - - // try index restore with USER_UNREMOVABLE_SETTINGS setting disabled - SnapshotRestoreException exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_REMOTE_STORE_ENABLED) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); - - // try index restore with UnmodifiableOnRestore setting disabled - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards] on restore")); - - // try index restore with mix of removable and UnmodifiableOnRestore settings disabled - // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is removable - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_VERSION_CREATED, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.version.created] on restore")); - - // try index restore with mix of removable and USER_UNREMOVABLE_SETTINGS settings disabled - // index.number_of_replicas is USER_UNREMOVABLE_SETTINGS, index.number_of_search_only_replicas is removable - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas] on restore")); - - // try index restore with multiple UnmodifiableOnRestore settings disabled - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS, IndexMetadata.SETTING_VERSION_CREATED) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - - // try index restore with multiple USER_UNREMOVABLE_SETTINGS settings disabled - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); - - // try index restore with mix of UnmodifiableOnRestore and USER_UNREMOVABLE_SETTINGS settings disabled - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SHARDS) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); - - // try index restore with UnmodifiableOnRestore setting modified - Settings numberOfShardsSettingsDiff = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(numberOfShardsSettingsDiff) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - - // try index restore with UnmodifiableOnRestore setting same - Settings numberOfShardsSettingsSame = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(numberOfShardsSettingsSame) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - - // try index restore with USER_UNMODIFIABLE_SETTINGS setting modified - Settings remoteStoreEnabledSetting = Settings.builder().put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false).build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(remoteStoreEnabledSetting) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - - // try index restore with mix of modifiable and UnmodifiableOnRestore settings modified - // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is modifiable - Settings mixedSettingsUnmodifiableOnRestore = Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(mixedSettingsUnmodifiableOnRestore) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.version.created]" + " on restore")); - - // try index restore with mix of modifiable and USER_UNMODIFIABLE_SETTINGS settings modified - // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.number_of_search_only_replicas is modifiable - Settings mixedSettingsUserUnmodifiableSettings = Settings.builder() - .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) - .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(mixedSettingsUserUnmodifiableSettings) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - - // try index restore with mix of UnmodifiableOnRestore and USER_UNMODIFIABLE_SETTINGS settings modified - // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.version.created is UnmodifiableOnRestore - Settings mixedSettings = Settings.builder() - .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(mixedSettings) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - - // try index restore with multiple UnmodifiableOnRestore settings modified - Settings unmodifiableOnRestoreSettings = Settings.builder() - .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(unmodifiableOnRestoreSettings) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); - - // try index restore with multiple USER_UNMODIFIABLE_SETTINGS settings modified - Settings userUnmodifiableSettings = Settings.builder() - .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) - .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) - .build(); - - exception = expectThrows( - SnapshotRestoreException.class, - () -> client().admin() - .cluster() - .prepareRestoreSnapshot(snapshotRepo, snapshotName1) - .setWaitForCompletion(false) - .setIndexSettings(userUnmodifiableSettings) - .setIndices(index) - .setRenamePattern(index) - .setRenameReplacement(restoredIndex) - .get() - ); - assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); - } - public void testRestoreOperationsUsingDifferentRepos() throws Exception { disableRepoConsistencyCheck("Remote store repo"); String clusterManagerNode = internalCluster().startClusterManagerOnlyNode(); diff --git a/server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java index e76587653e99a..36ab97d0b730f 100644 --- a/server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java @@ -32,6 +32,7 @@ package org.opensearch.snapshots; +import org.opensearch.Version; import org.opensearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.opensearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse; @@ -49,10 +50,12 @@ import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.common.unit.ByteSizeUnit; import org.opensearch.core.rest.RestStatus; +import org.opensearch.index.IndexSettings; import org.opensearch.indices.InvalidIndexNameException; import org.opensearch.repositories.RepositoriesService; import java.nio.file.Path; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Locale; @@ -1112,4 +1115,389 @@ public void testRestoreBalancedReplica() { } } + private String index; + private String snapshotRepo; + private String snapshotName1; + private String snapshotName2; + private Path absolutePath1; + private String restoredIndex; + private Settings indexSettings; + private SnapshotInfo snapshotInfo; + private SnapshotInfo snapshotInfo2; + + public void setupSnapshotRestore() { + index = "test-index"; + snapshotRepo = "test-restore-snapshot-repo"; + snapshotName1 = "test-restore-snapshot1"; + snapshotName2 = "test-restore-snapshot2"; + absolutePath1 = randomRepoPath().toAbsolutePath(); + + logger.info("Snapshot Path [{}]", absolutePath1); + restoredIndex = index + "-restored"; + + createRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, true)); + + indexSettings = getIndexSettings(1, 0).build(); + createIndex(index, indexSettings); + ensureGreen(index); + + logger.info("--> snapshot"); + + snapshotInfo = createSnapshot(snapshotRepo, snapshotName1, new ArrayList<>(List.of(index))); + assertThat(snapshotInfo.state(), equalTo(SnapshotState.SUCCESS)); + assertThat(snapshotInfo.successfulShards(), greaterThan(0)); + assertThat(snapshotInfo.successfulShards(), equalTo(snapshotInfo.totalShards())); + + updateRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, false)); + snapshotInfo2 = createSnapshot(snapshotRepo, snapshotName2, new ArrayList<>(List.of(index))); + assertThat(snapshotInfo2.state(), equalTo(SnapshotState.SUCCESS)); + assertThat(snapshotInfo2.successfulShards(), greaterThan(0)); + assertThat(snapshotInfo2.successfulShards(), equalTo(snapshotInfo2.totalShards())); + ensureGreen(index); + } + + public void testInvalidRestoreRequest_UserUnRemovableSettingsIgnored() throws Exception { + setupSnapshotRestore(); + + // try index restore with USER_UNREMOVABLE_SETTINGS setting ignored + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_REMOTE_STORE_ENABLED) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); + + } + + public void testInvalidRestoreRequest_UnmodifiableOnRestoreIgnored() throws Exception { + setupSnapshotRestore(); + + // try index restore with UnmodifiableOnRestore setting ignored + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards] on restore")); + + } + + public void testInvalidRestoreRequest_MixRemovableAndUnmodifiableOnRestoreIgnored() throws Exception { + setupSnapshotRestore(); + + // try index restore with mix of removable and UnmodifiableOnRestore settings ignored + // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is removable + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_VERSION_CREATED, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.version.created] on restore")); + } + + public void testInvalidRestoreRequest_MixRemovableAndUserUnRemovableSettingsIgnored() throws Exception { + setupSnapshotRestore(); + + // try index restore with mix of removable and USER_UNREMOVABLE_SETTINGS settings ignored + // index.number_of_replicas is USER_UNREMOVABLE_SETTINGS, index.number_of_search_only_replicas is removable + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas] on restore")); + + } + + public void testInvalidRestoreRequest_MixUnmodifiableOnRestoreAndUserUnRemovableSettingsIgnored() throws Exception { + setupSnapshotRestore(); + + // try index restore with mix of UnmodifiableOnRestore and USER_UNREMOVABLE_SETTINGS settings ignored + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_NUMBER_OF_SHARDS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); + + } + + public void testInvalidRestoreRequest_MultipleUnmodifiableOnRestoreIgnored() throws Exception { + setupSnapshotRestore(); + + // try index restore with multiple UnmodifiableOnRestore settings ignored + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_SHARDS, IndexMetadata.SETTING_VERSION_CREATED) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + + } + + public void testInvalidRestoreRequest_MultipleUserUnRemovableSettingsIgnored() throws Exception { + setupSnapshotRestore(); + + // try index restore with multiple USER_UNREMOVABLE_SETTINGS settings ignored + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.number_of_replicas]" + " on restore")); + + } + + public void testInvalidRestoreRequest_UnmodifiableOnRestoreModified() throws Exception { + setupSnapshotRestore(); + + // try index restore with UnmodifiableOnRestore setting modified + Settings numberOfShardsSettingsDiff = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 3).build(); + + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(numberOfShardsSettingsDiff) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + + } + + public void testInvalidRestoreRequest_UnmodifiableOnRestoreUnchanged() throws Exception { + setupSnapshotRestore(); + + // try index restore with UnmodifiableOnRestore setting unchanged + Settings numberOfShardsSettingsSame = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).build(); + + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(numberOfShardsSettingsSame) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + + } + + public void testInvalidRestoreRequest_UserUnmodifiableSettingsModified() throws Exception { + setupSnapshotRestore(); + + // try index restore with USER_UNMODIFIABLE_SETTINGS setting modified + Settings remoteStoreEnabledSetting = Settings.builder().put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false).build(); + + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(remoteStoreEnabledSetting) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + } + + public void testInvalidRestoreRequest_MixModifiableAndUnmodifiableOnRestoreModified() throws Exception { + setupSnapshotRestore(); + + // try index restore with mix of modifiable and UnmodifiableOnRestore settings modified + // index.version.created is UnmodifiableOnRestore, index.number_of_search_only_replicas is modifiable + Settings mixedSettingsUnmodifiableOnRestore = Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) + .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) + .build(); + + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(mixedSettingsUnmodifiableOnRestore) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.version.created]" + " on restore")); + + } + + public void testInvalidRestoreRequest_MixModifiableAndUserUnmodifiableSettingsModified() throws Exception { + setupSnapshotRestore(); + + // try index restore with mix of modifiable and USER_UNMODIFIABLE_SETTINGS settings modified + // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.number_of_search_only_replicas is modifiable + Settings mixedSettingsUserUnmodifiableSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) + .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) + .build(); + + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(mixedSettingsUserUnmodifiableSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + } + + public void testInvalidRestoreRequest_MixUnmodifiableOnRestoreAndUserUnmodifiableSettingsModified() throws Exception { + setupSnapshotRestore(); + + // try index restore with mix of UnmodifiableOnRestore and USER_UNMODIFIABLE_SETTINGS settings modified + // index.remote_store.enabled is USER_UNMODIFIABLE_SETTINGS, index.version.created is UnmodifiableOnRestore + Settings mixedSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) + .build(); + + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(mixedSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + } + + public void testInvalidRestoreRequest_MultipleUnmodifiableOnRestoreModified() throws Exception { + setupSnapshotRestore(); + + // try index restore with multiple UnmodifiableOnRestore settings modified + Settings unmodifiableOnRestoreSettings = Settings.builder() + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY) + .build(); + + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(unmodifiableOnRestoreSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore")); + + } + + public void testInvalidRestoreRequest_MultipleUserUnmodifiableSettingsModified() throws Exception { + setupSnapshotRestore(); + + // try index restore with multiple USER_UNMODIFIABLE_SETTINGS settings modified + Settings userUnmodifiableSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false) + .put(IndexMetadata.SETTING_NUMBER_OF_SEARCH_REPLICAS, 1) + .build(); + + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(userUnmodifiableSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + } + + protected Settings.Builder getIndexSettings(int numOfShards, int numOfReplicas) { + Settings.Builder settingsBuilder = Settings.builder() + .put(super.indexSettings()) + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numOfShards) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numOfReplicas) + .put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), "300s"); + return settingsBuilder; + } + } diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java index c915718e4a433..eb63522270e87 100644 --- a/server/src/main/java/org/opensearch/common/settings/Setting.java +++ b/server/src/main/java/org/opensearch/common/settings/Setting.java @@ -175,6 +175,7 @@ public enum Property { /** * Mark this setting as immutable on snapshot restore + * i.e. the setting will not be allowed to be removed or modified during restore */ UnmodifiableOnRestore } @@ -213,19 +214,13 @@ private Setting( final EnumSet propertiesAsSet = EnumSet.copyOf(Arrays.asList(properties)); if (propertiesAsSet.contains(Property.Dynamic) && propertiesAsSet.contains(Property.Final)) { throw new IllegalArgumentException("final setting [" + key + "] cannot be dynamic"); - } else if (propertiesAsSet.contains(Property.UnmodifiableOnRestore) - && (propertiesAsSet.contains(Property.Dynamic) || !propertiesAsSet.contains(Property.IndexScope))) { - throw new IllegalArgumentException( - "unmodifiableOnRestore setting [" - + key - + "] cannot be dynamic, or unmodifiableOnRestore setting [" - + key - + "] must have indexScope" - ); - } + } else if (propertiesAsSet.contains(Property.UnmodifiableOnRestore) && propertiesAsSet.contains(Property.Dynamic)) { + throw new IllegalArgumentException("UnmodifiableOnRestore setting [" + key + "] cannot be dynamic"); + } checkPropertyRequiresIndexScope(propertiesAsSet, Property.NotCopyableOnResize); checkPropertyRequiresIndexScope(propertiesAsSet, Property.InternalIndex); checkPropertyRequiresIndexScope(propertiesAsSet, Property.PrivateIndex); + checkPropertyRequiresIndexScope(propertiesAsSet, Property.UnmodifiableOnRestore); checkPropertyRequiresNodeScope(propertiesAsSet, Property.Consistent); this.properties = propertiesAsSet; } diff --git a/server/src/test/java/org/opensearch/common/settings/SettingTests.java b/server/src/test/java/org/opensearch/common/settings/SettingTests.java index a582da2141304..a0788b0c83e11 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingTests.java @@ -1444,25 +1444,15 @@ public void testRejectConflictingDynamicAndUnmodifiableOnRestoreProperties() { IllegalArgumentException.class, () -> Setting.simpleString("foo.bar", Property.UnmodifiableOnRestore, Property.Dynamic) ); - assertThat( - ex.getMessage(), - containsString( - "unmodifiableOnRestore setting [foo.bar] cannot be dynamic, or unmodifiableOnRestore setting [foo.bar] must have indexScope" - ) - ); + assertThat(ex.getMessage(), containsString("UnmodifiableOnRestore setting [foo.bar] cannot be dynamic")); } - public void testRejectMissingIndexScopeAndUnmodifiableOnRestoreProperties() { - IllegalArgumentException ex = expectThrows( + public void testRejectNonIndexScopedUnmodifiableOnRestoreSetting() { + final IllegalArgumentException e = expectThrows( IllegalArgumentException.class, () -> Setting.simpleString("foo.bar", Property.UnmodifiableOnRestore) ); - assertThat( - ex.getMessage(), - containsString( - "unmodifiableOnRestore setting [foo.bar] cannot be dynamic, or unmodifiableOnRestore setting [foo.bar] must have indexScope" - ) - ); + assertThat(e, hasToString(containsString("non-index-scoped setting [foo.bar] can not have property [UnmodifiableOnRestore]"))); } public void testRejectNonIndexScopedNotCopyableOnResizeSetting() { From accd0921654e0bd62cac5616978d2c0ff5de5858 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Thu, 23 Jan 2025 17:30:22 -0800 Subject: [PATCH 13/13] Add back testInvalidRestoreRequestScenarios test method Signed-off-by: AnnTian Shao --- .../remotestore/RemoteRestoreSnapshotIT.java | 184 ++++++++++++++++++ .../RestoreShallowSnapshotV2IT.java | 184 ++++++++++++++++++ 2 files changed, 368 insertions(+) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index 8fc119d49c3d1..3b96636cfe771 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -50,6 +50,7 @@ import org.opensearch.repositories.blobstore.BlobStoreRepository; import org.opensearch.repositories.fs.FsRepository; import org.opensearch.snapshots.SnapshotInfo; +import org.opensearch.snapshots.SnapshotRestoreException; import org.opensearch.snapshots.SnapshotState; import org.opensearch.test.InternalTestCluster; import org.opensearch.test.OpenSearchIntegTestCase; @@ -71,6 +72,9 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY; import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.SEGMENTS; import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.TRANSLOG; import static org.opensearch.index.remote.RemoteStoreEnums.DataType.DATA; @@ -708,6 +712,186 @@ public void testRestoreShallowSnapshotIndexAfterSnapshot() throws ExecutionExcep assertDocsPresentInIndex(client, restoredIndexName1, numDocsInIndex1 + 2); } + public void testInvalidRestoreRequestScenarios() throws Exception { + internalCluster().startClusterManagerOnlyNode(); + internalCluster().startDataOnlyNode(); + String index = "test-index"; + String snapshotRepo = "test-restore-snapshot-repo"; + String newRemoteStoreRepo = "test-new-rs-repo"; + String snapshotName1 = "test-restore-snapshot1"; + String snapshotName2 = "test-restore-snapshot2"; + Path absolutePath1 = randomRepoPath().toAbsolutePath(); + logger.info("Snapshot Path [{}]", absolutePath1); + String restoredIndex = index + "-restored"; + + createRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, true)); + + Client client = client(); + Settings indexSettings = getIndexSettings(1, 0).build(); + createIndex(index, indexSettings); + + final int numDocsInIndex = 5; + indexDocuments(client, index, numDocsInIndex); + ensureGreen(index); + + internalCluster().startDataOnlyNode(); + logger.info("--> snapshot"); + + SnapshotInfo snapshotInfo = createSnapshot(snapshotRepo, snapshotName1, new ArrayList<>(List.of(index))); + assertThat(snapshotInfo.state(), equalTo(SnapshotState.SUCCESS)); + assertThat(snapshotInfo.successfulShards(), greaterThan(0)); + assertThat(snapshotInfo.successfulShards(), equalTo(snapshotInfo.totalShards())); + + updateRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, false)); + SnapshotInfo snapshotInfo2 = createSnapshot(snapshotRepo, snapshotName2, new ArrayList<>(List.of(index))); + assertThat(snapshotInfo2.state(), equalTo(SnapshotState.SUCCESS)); + assertThat(snapshotInfo2.successfulShards(), greaterThan(0)); + assertThat(snapshotInfo2.successfulShards(), equalTo(snapshotInfo2.totalShards())); + + DeleteResponse deleteResponse = client().prepareDelete(index, "0").execute().actionGet(); + assertEquals(deleteResponse.getResult(), DocWriteResponse.Result.DELETED); + indexDocuments(client, index, numDocsInIndex, numDocsInIndex + randomIntBetween(2, 5)); + ensureGreen(index); + + // try index restore with index.remote_store.enabled ignored + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(SETTING_REMOTE_STORE_ENABLED) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); + + // try index restore with index.remote_store.segment.repository ignored + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.segment.repository] on restore")); + + // try index restore with index.remote_store.translog.repository ignored + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.translog.repository] on restore")); + + // try index restore with index.remote_store.segment.repository and index.remote_store.translog.repository ignored + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings( + IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, + IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY + ) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.segment.repository]" + " on restore")); + + // try index restore with index.remote_store.enabled modified + Settings remoteStoreIndexSettings = Settings.builder().put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(remoteStoreIndexSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + // try index restore with index.remote_store.segment.repository modified + Settings remoteStoreSegmentIndexSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, newRemoteStoreRepo) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(remoteStoreSegmentIndexSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); + + // try index restore with index.remote_store.translog.repository modified + Settings remoteStoreTranslogIndexSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, newRemoteStoreRepo) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(remoteStoreTranslogIndexSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.translog.repository]" + " on restore")); + + // try index restore with index.remote_store.translog.repository and index.remote_store.segment.repository modified + Settings multipleRemoteStoreIndexSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, newRemoteStoreRepo) + .put(IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, newRemoteStoreRepo) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(multipleRemoteStoreIndexSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); + } + public void testCreateSnapshotV2_Orphan_Timestamp_Cleanup() throws Exception { internalCluster().startClusterManagerOnlyNode(pinnedTimestampSettings()); internalCluster().startDataOnlyNode(pinnedTimestampSettings()); diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java index 555860fa6a1db..c45be1ff682a7 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java @@ -44,6 +44,7 @@ import org.opensearch.repositories.blobstore.BlobStoreRepository; import org.opensearch.snapshots.AbstractSnapshotIntegTestCase; import org.opensearch.snapshots.SnapshotInfo; +import org.opensearch.snapshots.SnapshotRestoreException; import org.opensearch.snapshots.SnapshotState; import org.opensearch.test.BackgroundIndexer; import org.opensearch.test.InternalTestCluster; @@ -66,6 +67,9 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY; import static org.opensearch.index.query.QueryBuilders.matchAllQuery; import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.SEGMENTS; import static org.opensearch.index.remote.RemoteStoreEnums.DataCategory.TRANSLOG; @@ -775,6 +779,186 @@ public void testRestoreShallowSnapshotIndexAfterSnapshot() throws ExecutionExcep assertDocsPresentInIndex(client, restoredIndexName1, numDocsInIndex1 + 2); } + public void testInvalidRestoreRequestScenarios() throws Exception { + internalCluster().startClusterManagerOnlyNode(); + internalCluster().startDataOnlyNode(); + String index = "test-index"; + String snapshotRepo = "test-restore-snapshot-repo"; + String newRemoteStoreRepo = "test-new-rs-repo"; + String snapshotName1 = "test-restore-snapshot1"; + String snapshotName2 = "test-restore-snapshot2"; + Path absolutePath1 = randomRepoPath().toAbsolutePath(); + logger.info("Snapshot Path [{}]", absolutePath1); + String restoredIndex = index + "-restored"; + + createRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, true)); + + Client client = client(); + Settings indexSettings = getIndexSettings(1, 0).build(); + createIndex(index, indexSettings); + + final int numDocsInIndex = 5; + indexDocuments(client, index, numDocsInIndex); + ensureGreen(index); + + internalCluster().startDataOnlyNode(); + logger.info("--> snapshot"); + + SnapshotInfo snapshotInfo = createSnapshot(snapshotRepo, snapshotName1, new ArrayList<>(List.of(index))); + assertThat(snapshotInfo.state(), equalTo(SnapshotState.SUCCESS)); + assertThat(snapshotInfo.successfulShards(), greaterThan(0)); + assertThat(snapshotInfo.successfulShards(), equalTo(snapshotInfo.totalShards())); + + updateRepository(snapshotRepo, "fs", getRepositorySettings(absolutePath1, false)); + SnapshotInfo snapshotInfo2 = createSnapshot(snapshotRepo, snapshotName2, new ArrayList<>(List.of(index))); + assertThat(snapshotInfo2.state(), equalTo(SnapshotState.SUCCESS)); + assertThat(snapshotInfo2.successfulShards(), greaterThan(0)); + assertThat(snapshotInfo2.successfulShards(), equalTo(snapshotInfo2.totalShards())); + + DeleteResponse deleteResponse = client().prepareDelete(index, "0").execute().actionGet(); + assertEquals(deleteResponse.getResult(), DocWriteResponse.Result.DELETED); + indexDocuments(client, index, numDocsInIndex, numDocsInIndex + randomIntBetween(2, 5)); + ensureGreen(index); + + // try index restore with index.remote_store.enabled ignored + SnapshotRestoreException exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(SETTING_REMOTE_STORE_ENABLED) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.enabled] on restore")); + + // try index restore with index.remote_store.segment.repository ignored + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.segment.repository] on restore")); + + // try index restore with index.remote_store.translog.repository ignored + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.translog.repository] on restore")); + + // try index restore with index.remote_store.segment.repository and index.remote_store.translog.repository ignored + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIgnoreIndexSettings( + IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, + IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY + ) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot remove setting [index.remote_store.segment.repository]" + " on restore")); + + // try index restore with index.remote_store.enabled modified + Settings remoteStoreIndexSettings = Settings.builder().put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false).build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(remoteStoreIndexSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.enabled]" + " on restore")); + + // try index restore with index.remote_store.segment.repository modified + Settings remoteStoreSegmentIndexSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, newRemoteStoreRepo) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(remoteStoreSegmentIndexSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); + + // try index restore with index.remote_store.translog.repository modified + Settings remoteStoreTranslogIndexSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, newRemoteStoreRepo) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(remoteStoreTranslogIndexSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.translog.repository]" + " on restore")); + + // try index restore with index.remote_store.translog.repository and index.remote_store.segment.repository modified + Settings multipleRemoteStoreIndexSettings = Settings.builder() + .put(IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, newRemoteStoreRepo) + .put(IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, newRemoteStoreRepo) + .build(); + + exception = expectThrows( + SnapshotRestoreException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepo, snapshotName1) + .setWaitForCompletion(false) + .setIndexSettings(multipleRemoteStoreIndexSettings) + .setIndices(index) + .setRenamePattern(index) + .setRenameReplacement(restoredIndex) + .get() + ); + assertTrue(exception.getMessage().contains("cannot modify setting [index.remote_store.segment.repository]" + " on restore")); + } + public void testRestoreOperationsUsingDifferentRepos() throws Exception { disableRepoConsistencyCheck("Remote store repo"); String clusterManagerNode = internalCluster().startClusterManagerOnlyNode();