Add UnmodifiableOnRestore property to index.knn setting #2445

anntians · 2025-01-25T23:36:01Z

Description

This PR builds on another PR. In the other PR we added a new Setting property UnmodifiableOnRestore to prevent settings from being modified on restore. So in this PR we're adding that new UnmodifiableOnRestore property to the index.knn setting to prevent it from being modified or removed on restore snapshot.

Related Issues

Resolves Issue #2334
Also resolves Issue 17019 in Core Opensearch repository opensearch-project/OpenSearch#17019

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

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

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

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

kotwanikunal

Please add a changelog entry.

anntians · 2025-01-27T17:02:33Z

@kotwanikunal This PR is a followup/part of a larger change to make index.knn setting immutable after index creation, and that change is already documented in the ChangeLog in this PR https://github.com/opensearch-project/k-NN/pull/2348/files. Could you help add the skip-changelog label to this PR? Thanks

anntians · 2025-01-27T17:25:28Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+    public void testUnmodifiableOnRestoreSettingModifiedOnRestore() throws Exception {
+        setupSnapshotRestore();
+
+        // invalid restore


I don't think there are any existing helper methods in OpenSearchRestTestCase/upstream classes for restore snapshot that take in setting updates and throws back validation errors

sohami

Overall looks good to me, just a minor comment to update the test name.

sohami · 2025-01-27T18:28:47Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+        createSnapshot(repository, snapshot, true);
+    }
+
+    public void testUnmodifiableOnRestoreSettingModifiedOnRestore() throws Exception {


nit: May be we should rename this test as testKnnSettingIsUnmodifiableOnRestore

sohami · 2025-01-27T18:29:25Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+        assertThat(error.getMessage(), containsString("cannot modify UnmodifiableOnRestore setting [index.knn]" + " on restore"));
+    }
+
+    public void testUnmodifiableOnRestoreSettingIgnoredOnRestore() throws Exception {


testKnnSettingCannotBeIgnoredDuringRestore

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

sohami

Lgtm. Thanks!

navneet1v · 2025-01-27T18:49:50Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+        createSnapshot(repository, snapshot, true);
+    }
+
+    public void testKnnSettingIsUnmodifiableOnRestore() throws Exception {


[suggestion]
The way we create test names in k-NN in testABC_whenXYZ_then(Success/Failure).

navneet1v · 2025-01-27T18:50:44Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+        repository = "repo";
+
+        // create index
+        Settings indexSettings = Settings.builder().put("number_of_shards", 1).put("number_of_replicas", 1).put("index.knn", true).build();


given that tests run on 1 single node, having a replica will keep the index yellow. Lets not have 1 replica.

I think lets use the index setting already present in the base classes

navneet1v · 2025-01-27T18:52:09Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+        // create repo
+        Settings repoSettings = Settings.builder().put("compress", randomBoolean()).put("location", pathRepo).build();
+        registerRepository(repository, "fs", true, repoSettings);
+
+        // create snapshot
+        createSnapshot(repository, snapshot, true);


Lets move this to base class since we can start using this function for other tests.

navneet1v · 2025-01-27T18:52:32Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+        createSnapshot(repository, snapshot, true);
+    }
+
+    public void testKnnSettingIsUnmodifiableOnRestore() throws Exception {


use @SneakyThrows rather than exception in message signature

navneet1v · 2025-01-27T18:54:59Z

@anntians Please don't add the label skip-changelog and add correct entry in the changelog.

navneet1v · 2025-01-27T18:52:53Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+    }
+
+    public void testKnnSettingIsUnmodifiableOnRestore() throws Exception {
+        setupSnapshotRestore();


why we are not using the @setup function here?

navneet1v · 2025-01-27T18:54:02Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.hasSize;
+
+public class RestoreSnapshotIT extends OpenSearchRestTestCase {


can you please add a test for happy path. In both the tests I am seeing the unhappy path. :)

navneet1v · 2025-01-27T19:15:27Z

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java

+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.hasSize;
+
+public class RestoreSnapshotIT extends OpenSearchRestTestCase {


Please use the base class KNNRestTestCase which is there for all the IT of k-NN plugin

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

opensearch-trigger-bot · 2025-01-27T22:51:36Z

The backport to main failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-main main
# Navigate to the new working tree
cd .worktrees/backport-main
# Create a new branch
git switch --create backport/backport-2445-to-main
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1d2c4c0c783428fdea7141ad069134a25b590c54
# Push it to GitHub
git push --set-upstream origin backport/backport-2445-to-main
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-main

Then, create a pull request where the base branch is main and the compare/head branch is backport/backport-2445-to-main.

…roject#2445) * Add UnmodifiableOnRestore property to index.knn setting Signed-off-by: AnnTian Shao <[email protected]> * Update tests with helper methods Signed-off-by: AnnTian Shao <[email protected]> * Update test names Signed-off-by: AnnTian Shao <[email protected]> * Add to changeLog and fixes to tests Signed-off-by: AnnTian Shao <[email protected]> --------- Signed-off-by: AnnTian Shao <[email protected]> Co-authored-by: AnnTian Shao <[email protected]> (cherry picked from commit 1d2c4c0)

Add UnmodifiableOnRestore property to index.knn setting

8f75f4f

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

anntians requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan, luyuncheng, shatejas and 0ctopus13prime as code owners January 25, 2025 23:36

sohami reviewed Jan 27, 2025

View reviewed changes

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java Outdated Show resolved Hide resolved

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java Outdated Show resolved Hide resolved

src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java Outdated Show resolved Hide resolved

Update tests with helper methods

f05d0f1

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

kotwanikunal reviewed Jan 27, 2025

View reviewed changes

kotwanikunal added the skip-changelog label Jan 27, 2025

anntians commented Jan 27, 2025

View reviewed changes

sohami reviewed Jan 27, 2025

View reviewed changes

Update test names

7e5f720

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

kotwanikunal approved these changes Jan 27, 2025

View reviewed changes

sohami approved these changes Jan 27, 2025

View reviewed changes

navneet1v reviewed Jan 27, 2025

View reviewed changes

navneet1v added Features Introduces a new unit of functionality that satisfies a requirement and removed skip-changelog labels Jan 27, 2025

navneet1v reviewed Jan 27, 2025

View reviewed changes

anntians force-pushed the unmodifiableOnRestore branch 3 times, most recently from 88b4beb to d5a4355 Compare January 27, 2025 20:43

Add to changeLog and fixes to tests

b722a00

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

anntians force-pushed the unmodifiableOnRestore branch from d5a4355 to b722a00 Compare January 27, 2025 21:26

navneet1v approved these changes Jan 27, 2025

View reviewed changes

VijayanB approved these changes Jan 27, 2025

View reviewed changes

VijayanB merged commit 1d2c4c0 into opensearch-project:2.x Jan 27, 2025
99 checks passed

navneet1v added the backport main label Jan 27, 2025

This was referenced Jan 28, 2025

[AUTOCUT] Integration Test Failed for k-NN-2.19.0 #2458

Closed

Add path.repo for registering snapshot repository in integ tests #2461

Merged

[Backport Main] Add path.repo for registering snapshot repository in integ tests #2463

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UnmodifiableOnRestore property to index.knn setting #2445

Add UnmodifiableOnRestore property to index.knn setting #2445

anntians commented Jan 25, 2025

kotwanikunal left a comment

anntians commented Jan 27, 2025

anntians Jan 27, 2025

sohami left a comment

sohami Jan 27, 2025

sohami Jan 27, 2025

sohami left a comment

navneet1v Jan 27, 2025

navneet1v Jan 27, 2025 •

edited

Loading

navneet1v Jan 27, 2025

navneet1v Jan 27, 2025

navneet1v commented Jan 27, 2025

navneet1v Jan 27, 2025

navneet1v Jan 27, 2025

navneet1v Jan 27, 2025

opensearch-trigger-bot bot commented Jan 27, 2025

Add UnmodifiableOnRestore property to index.knn setting #2445

Add UnmodifiableOnRestore property to index.knn setting #2445

Conversation

anntians commented Jan 25, 2025

Description

Related Issues

Check List

kotwanikunal left a comment

Choose a reason for hiding this comment

anntians commented Jan 27, 2025

Choose a reason for hiding this comment

sohami left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohami left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navneet1v Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navneet1v commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jan 27, 2025

navneet1v Jan 27, 2025 •

edited

Loading