Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added new Setting property UnmodifiableOnRestore to prevent updating settings on restore snapshot #16957

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
+349 −25
Open

Added new Setting property UnmodifiableOnRestore to prevent updating settings on restore snapshot #16957

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
457898a
Add index.knn setting to list of unmodifiable settings when restore s…
Jan 6, 2025
5f70915
Add index.knn setting to list of unmodifiable settings when restore s…
Jan 6, 2025
736696c
Merge branch 'main' of github.com:anntians/OpenSearch
Jan 6, 2025
4a659c0
Merge branch 'main' into main
anntians Jan 6, 2025
f33c9bc
Merge branch 'main' into main
anntians Jan 7, 2025
f9c06e8
Add new Setting property UnmodifiableOnRestore to mark setting as imm…
Jan 15, 2025
1227bbb
Merge branch 'main' into main
anntians Jan 16, 2025
2afd94e
Add tests for new Setting property UnmodifiableOnRestore
Jan 16, 2025
c29a0db
Merge branch 'main' into main
anntians Jan 17, 2025
a24b673
fixes and added more tests for new setting property UnmodifiableOnRes…
Jan 17, 2025
252100c
fix test failures
Jan 21, 2025
e860499
Revert "fix test failures"
Jan 21, 2025
6dc9840
fixes by adding back USER_UNMODIFIABLE_SETTINGS for settings without …
Jan 21, 2025
e517731
Merge branch 'main' into main
anntians Jan 22, 2025
9ebab5a
testing CI config for registering plugin settings
Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Support searching from doc_value using termQueryCaseInsensitive/termQuery in flat_object/keyword field([#16974](https://github.com/opensearch-project/OpenSearch/pull/16974/))
- Added a new `time` field to replace the deprecated `getTime` field in `GetStats`. ([#17009](https://github.com/opensearch-project/OpenSearch/pull/17009))
- Improve performance of the bitmap filtering([#16936](https://github.com/opensearch-project/OpenSearch/pull/16936/))
- 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))
Expand Down
130 changes: 125 additions & 5 deletions server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -721,9 +767,83 @@ 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)
// 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();

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 index.number_of_shards 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 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_NUMBER_OF_SHARDS, 1)
.put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY)
.put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false)
.build();

exception = expectThrows(
Expand All @@ -732,13 +852,13 @@ public void testInvalidRestoreRequestScenarios() throws Exception {
.cluster()
.prepareRestoreSnapshot(snapshotRepo, snapshotName1)
.setWaitForCompletion(false)
.setIndexSettings(remoteStoreIndexSettings)
.setIndexSettings(unmodifiableSettings)
.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"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some more test scenarios here? Off the top of my head:

  • Setting has unmodifiable on restore, but the setting is passed in to the request with the same value
  • Setting does not have unmodifiable on restore, we are able to modify the setting on restore
  • Some settings have unmodifiable on restore, still able to modify other settings

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing.

For the test cases below:

Setting has unmodifiable on restore, but the setting is passed in to the request with the same value

The existing behavior is that restore api will still throw a cannot modify error even if the setting has the same value. As long as there is a setting tagged as unmodifiable, restore will throw. I think it's more clean this way to not allow any unmodifiable settings in the request, to avoid unnecessary confusion for customers. Instead of including the same value as part of the request, just remove that setting from the request. I assume we would want to maintain that existing behavior?

Some settings have unmodifiable on restore, still able to modify other settings

For this test case, the existing behavior is that even if one of the settings in the restore request is unmodifiable, then restore will throw. I think it makes sense or else customers might think all the setting updates executed when some didn't. I assume we would want to maintain this behavior?

Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, as a general rule of thumb we want to maintain the existing behavior (and that's why we're adding tests)


// try index restore with remote store repository and translog store repository disabled
exception = expectThrows(
Expand Down
130 changes: 125 additions & 5 deletions ...r/src/internalClusterTest/java/org/opensearch/remotestore/RestoreShallowSnapshotV2IT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -788,9 +834,83 @@ 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)
// 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();

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 index.number_of_shards 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 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_NUMBER_OF_SHARDS, 1)
.put(IndexMetadata.SETTING_VERSION_CREATED, Version.V_EMPTY)
.put(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false)
.build();

exception = expectThrows(
Expand All @@ -799,13 +919,13 @@ public void testInvalidRestoreRequestScenarios() throws Exception {
.cluster()
.prepareRestoreSnapshot(snapshotRepo, snapshotName1)
.setWaitForCompletion(false)
.setIndexSettings(remoteStoreIndexSettings)
.setIndexSettings(unmodifiableSettings)
.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 remote store repository and translog store repository disabled
exception = expectThrows(
Expand Down
Loading
Loading