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

anntians · 2025-01-06T18:45:34Z

Description

This PR adds a new Setting property UnmodifiableOnRestore to prevent customers from updating certain settings on restore snapshot. The motivation behind this change is to prevent customers from updating index.knn setting when restoring a snapshot. The PR is related to another PR (opensearch-project/k-NN#2348) which fixes a bug by making index.knn setting immutable after index creation. However, that PR doesn't cover the case for snapshots, so this PR adds Setting property UnmodifiableOnRestore so that index.knn will be immutable when restoring an index from a snapshot.

Related Issues

Resolves Issue 2334 in KNN repository: opensearch-project/k-NN#2334
#17019

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…napshot Signed-off-by: AnnTian Shao <[email protected]>

Signed-off-by: Tommy Shao <[email protected]>

github-actions · 2025-01-06T19:52:12Z

✅ Gradle check result for 4a659c0: SUCCESS

codecov · 2025-01-06T21:28:20Z

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.19%. Comparing base (6b1861a) to head (e517731).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
.../java/org/opensearch/snapshots/RestoreService.java	80.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #16957      +/-   ##
============================================
- Coverage     72.23%   72.19%   -0.04%     
+ Complexity    65335    65309      -26     
============================================
  Files          5301     5302       +1     
  Lines        303824   303921      +97     
  Branches      44033    44048      +15     
============================================
- Hits         219471   219427      -44     
- Misses        66363    66543     +180     
+ Partials      17990    17951      -39

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Tommy Shao <[email protected]>

github-actions · 2025-01-07T18:24:17Z

❌ Gradle check result for f33c9bc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

…utable on restore snapshot Signed-off-by: AnnTian Shao <[email protected]>

github-actions · 2025-01-16T18:06:55Z

❌ Gradle check result for fddd875: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Tommy Shao <[email protected]> Signed-off-by: AnnTian Shao <[email protected]>

github-actions · 2025-01-16T19:17:17Z

✅ Gradle check result for 1227bbb: SUCCESS

Signed-off-by: AnnTian Shao <[email protected]>

github-actions · 2025-01-16T20:41:36Z

✅ Gradle check result for 2afd94e: SUCCESS

jed326 · 2025-01-16T20:51:48Z

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.number_of_shards setting modified
+        Settings indexKnnSettings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, false).build();


Don't call this knn

Also, SETTING_NUMBER_OF_SHARDS is not a bool setting so this would fail anyways.

jed326 · 2025-01-16T20:53:59Z

server/src/main/java/org/opensearch/snapshots/RestoreService.java

@@ -164,7 +165,6 @@ public class RestoreService implements ClusterStateApplier {

    private static final Set<String> USER_UNMODIFIABLE_SETTINGS = unmodifiableSet(
        newHashSet(
-            SETTING_NUMBER_OF_SHARDS,


Any reason not to do this to all of the settings in this list?

No particular reason, will add UnmodifiableOnRestore to those settings as well.

Okay I think I see the confusion -- it looks like some of the existing index settings like INDEX_UUID_SETTING are not actually implemented as Setting so we can't use the property. Let's create a separate issue to track updating those settings that aren't settings and not take that up in the scope of this PR.

jed326 · 2025-01-16T20:58:12Z

server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java

+                .setRenameReplacement(restoredIndex)
+                .get()
+        );
+        assertTrue(exception.getMessage().contains("cannot modify UnmodifiableOnRestore setting [index.number_of_shards]" + " on restore"));


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

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

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

Signed-off-by: Tommy Shao <[email protected]>

github-actions · 2025-01-17T19:12:54Z

✅ Gradle check result for c29a0db: SUCCESS

…tore Signed-off-by: AnnTian Shao <[email protected]>

github-actions · 2025-01-17T21:20:58Z

❌ Gradle check result for a24b673: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

jed326 · 2025-01-17T22:02:24Z

❌ Gradle check result for a24b673: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

FYI @anntians these test failures seem to be due to test changes made in this PR

anntians · 2025-01-17T22:14:00Z

Yes, I'm currently looking into the test failures

Signed-off-by: AnnTian Shao <[email protected]>

github-actions · 2025-01-21T18:50:25Z

❌ Gradle check result for 252100c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

jed326 · 2025-01-21T20:50:50Z

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();


Let's not change existing tests like this

jed326 · 2025-01-21T20:51:42Z

server/src/main/java/org/opensearch/snapshots/RestoreService.java

@@ -164,7 +165,6 @@ public class RestoreService implements ClusterStateApplier {

    private static final Set<String> USER_UNMODIFIABLE_SETTINGS = unmodifiableSet(
        newHashSet(
-            SETTING_NUMBER_OF_SHARDS,


Okay I think I see the confusion -- it looks like some of the existing index settings like INDEX_UUID_SETTING are not actually implemented as Setting so we can't use the property. Let's create a separate issue to track updating those settings that aren't settings and not take that up in the scope of this PR.

sohami · 2025-01-16T21:07:27Z

server/src/main/java/org/opensearch/common/settings/Setting.java

+        ExtensionScope,
+
+        /**
+         * Mark this setting as immutable on restore snapshot


nit: ... snapshot restore

sohami · 2025-01-21T20:06:57Z

server/src/main/java/org/opensearch/indices/IndicesService.java

@@ -1182,6 +1182,10 @@ public IndicesQueryCache getIndicesQueryCache() {
        return indicesQueryCache;
    }

+    public IndexScopedSettings getIndexScopedSettings() {


Can we add this API in MetadataCreateIndexService instead ? The IndicesService class is marked with @PublicAPi tag, so exposing anything from this class will be hard to hide it in future whereas MetadataCreateIndexService is internal.

sohami · 2025-01-21T20:57:13Z

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,


I think we should avoid registering these internal settings to keep current behavior. I believe if user tries to update these internal setting, it will throw errors on the lines of unknown setting or something.

These new settings added to IndexScopedSettings previously were not implemented as a Setting as noted in @jed326's #16957 (comment) as well. So the reason they're added to IndexScopedSettings is for the check

indexScopedSettings.isUnmodifiableOnRestoreSetting(k)

to work correctly, or else those settings will not be recognized because they're not registered as part of IndexScopedSettings.

I'm going to open a separate issue to update these settings with Setting implementation and add the UnmodifiableOnRestore setting.

However, there is a known issue with the SETTING_CREATION_DATE setting. The setting does have a Setting implementation, but it's in InternalSettingsPlugin.java. SETTING_CREATION_DATE setting also has several existing tests testCreationDateGivenFails and testPrivateSettingsValidation for enforcing its private/internal behavior. This means that if we add INDEX_CREATION_DATE_SETTING to IndexScopedSettings then both those tests will fail.

The way for the check

indexScopedSettings.isUnmodifiableOnRestoreSetting(k)

to work is if SETTING_CREATION_DATE setting is registered in IndexScopedSettings, which would require us to change/update the tests mentioned above. Unless there is another way to check a Setting's property without having to register that Setting to IndexScopedSettings?

However, there is a known issue with the SETTING_CREATION_DATE setting. The setting does have a Setting implementation, but it's in InternalSettingsPlugin.java.

Why do we need to add this to IndexScopedSettings? InternalSettingsPlugin is just another plugin so it should be the same as any other index setting that is provided by a plugin.

Or put the opposite way, if this doesn't work for InternalSettingsPlugin then wouldn't it also not work for the knn (or any other) plugin?

sohami · 2025-01-21T21:44:18Z

server/src/main/java/org/opensearch/common/settings/Setting.java

+        /**
+         * Mark this setting as immutable on restore snapshot
+         */
+        UnmodifiableOnRestore


I was thinking can we use this property to enforce that settings defined with this property cannot be changed at all i.e. cannot be modified with a different value or cannot be also removed from the index. Currently we seem to handle only the modified part in RestoreService.

I do think @sohami raises a good point here that for example, having a setting with only UnmodifiableOnRestore but without Final probably doesn't make any sense because with only the former the setting could still be modified post restore.

On the other hand, I also think it's best to not have overlapping functionalities across the properties because that can also cause confusion.

This reverts commit 252100c. Signed-off-by: AnnTian Shao <[email protected]>

…Setting implementation Signed-off-by: AnnTian Shao <[email protected]>

github-actions · 2025-01-22T00:21:35Z

❕ Gradle check result for 6dc9840: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Signed-off-by: Tommy Shao <[email protected]>

github-actions · 2025-01-22T02:28:57Z

❕ Gradle check result for e517731: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Signed-off-by: AnnTian Shao <[email protected]>

github-actions · 2025-01-22T02:52:35Z

❌ Gradle check result for 9ebab5a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

AnnTian Shao added 3 commits January 6, 2025 10:02

Add index.knn setting to list of unmodifiable settings when restore s…

457898a

…napshot Signed-off-by: AnnTian Shao <[email protected]>

Add index.knn setting to list of unmodifiable settings when restore s…

5f70915

…napshot Signed-off-by: AnnTian Shao <[email protected]>

Merge branch 'main' of github.com:anntians/OpenSearch

736696c

Merge branch 'main' into main

4a659c0

Signed-off-by: Tommy Shao <[email protected]>

Merge branch 'main' into main

f33c9bc

Signed-off-by: Tommy Shao <[email protected]>

Add new Setting property UnmodifiableOnRestore to mark setting as imm…

f9c06e8

…utable on restore snapshot Signed-off-by: AnnTian Shao <[email protected]>

anntians force-pushed the main branch from 550a131 to f9c06e8 Compare January 16, 2025 17:42

Merge branch 'main' into main

1227bbb

Signed-off-by: Tommy Shao <[email protected]> Signed-off-by: AnnTian Shao <[email protected]>

anntians force-pushed the main branch from fddd875 to 1227bbb Compare January 16, 2025 18:17

Add tests for new Setting property UnmodifiableOnRestore

2afd94e

Signed-off-by: AnnTian Shao <[email protected]>

jed326 reviewed Jan 16, 2025

View reviewed changes

Merge branch 'main' into main

c29a0db

Signed-off-by: Tommy Shao <[email protected]>

fixes and added more tests for new setting property UnmodifiableOnRes…

a24b673

…tore Signed-off-by: AnnTian Shao <[email protected]>

fix test failures

252100c

Signed-off-by: AnnTian Shao <[email protected]>

anntians requested a review from cwperks as a code owner January 21, 2025 17:51

opensearch-ci-bot mentioned this pull request Jan 21, 2025

[AUTOCUT] Gradle Check Flaky Test Report for DeleteByQueryBasicTests #15820

Open

jed326 reviewed Jan 21, 2025

View reviewed changes

sohami reviewed Jan 21, 2025

View reviewed changes

AnnTian Shao added 2 commits January 21, 2025 15:20

Revert "fix test failures"

e860499

This reverts commit 252100c. Signed-off-by: AnnTian Shao <[email protected]>

fixes by adding back USER_UNMODIFIABLE_SETTINGS for settings without …

6dc9840

…Setting implementation Signed-off-by: AnnTian Shao <[email protected]>

opensearch-ci-bot mentioned this pull request Jan 22, 2025

[AUTOCUT] Gradle Check Flaky Test Report for DedicatedClusterSnapshotRestoreIT #15806

Open

Merge branch 'main' into main

e517731

Signed-off-by: Tommy Shao <[email protected]>

testing CI config for registering plugin settings

9ebab5a

Signed-off-by: AnnTian Shao <[email protected]>

opensearch-ci-bot mentioned this pull request Jan 22, 2025

[AUTOCUT] Gradle Check Flaky Test Report for MinimumClusterManagerNodesIT #14289

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

anntians commented Jan 6, 2025 •

edited

Loading

github-actions bot commented Jan 6, 2025

codecov bot commented Jan 6, 2025 •

edited

Loading

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 16, 2025

github-actions bot commented Jan 16, 2025

github-actions bot commented Jan 16, 2025

jed326 Jan 16, 2025

jed326 Jan 16, 2025

jed326 Jan 16, 2025

anntians Jan 16, 2025

jed326 Jan 21, 2025

jed326 Jan 16, 2025

anntians Jan 17, 2025

jed326 Jan 17, 2025

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

jed326 commented Jan 17, 2025

anntians commented Jan 17, 2025

github-actions bot commented Jan 21, 2025

jed326 Jan 21, 2025

jed326 Jan 21, 2025

sohami Jan 16, 2025

sohami Jan 21, 2025

sohami Jan 21, 2025

anntians Jan 21, 2025

jed326 Jan 21, 2025

sohami Jan 21, 2025

jed326 Jan 22, 2025

github-actions bot commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

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

Are you sure you want to change the base?

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

Conversation

anntians commented Jan 6, 2025 • edited Loading

Description

Related Issues

Check List

github-actions bot commented Jan 6, 2025

codecov bot commented Jan 6, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 16, 2025

github-actions bot commented Jan 16, 2025

github-actions bot commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

jed326 commented Jan 17, 2025

anntians commented Jan 17, 2025

github-actions bot commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

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

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

anntians commented Jan 6, 2025 •

edited

Loading

codecov bot commented Jan 6, 2025 •

edited

Loading