Add to changeLog and fixes to tests

Signed-off-by: AnnTian Shao <[email protected]>
opensearch-project · Jan 27, 2025 · d5a4355 · d5a4355
1 parent 7e5f720
commit d5a4355
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 43 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,6 +40,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 * Fix filter rewrite logic which was resulting in getting inconsistent / incorrect results for cases where filter was getting rewritten for shards (#2359)[https://github.com/opensearch-project/k-NN/pull/2359]
 * Fixing it to retrieve space_type from index setting when both method and top level don't have the value. [#2374](https://github.com/opensearch-project/k-NN/pull/2374)
 * Fixing the bug where setting rescore as false for on_disk knn_vector query is a no-op (#2399)[https://github.com/opensearch-project/k-NN/pull/2399]
+* Fixing the bug to prevent index.knn setting from being modified or removed on restore snapshot (#2445)[https://github.com/opensearch-project/k-NN/pull/2445]
 ### Infrastructure
 * Updated C++ version in JNI from c++11 to c++17 [#2259](https://github.com/opensearch-project/k-NN/pull/2259)
 * Upgrade bytebuddy and objenesis version to match OpenSearch core and, update github ci runner for macos [#2279](https://github.com/opensearch-project/k-NN/pull/2279)

diff --git a/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java b/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java
@@ -8,57 +8,52 @@
 import org.opensearch.client.Request;
 import org.opensearch.client.Response;
 import org.opensearch.client.ResponseException;
-import org.opensearch.common.settings.Settings;
 import org.opensearch.common.xcontent.json.JsonXContent;
-import org.opensearch.common.xcontent.support.XContentMapValues;
 import org.opensearch.core.xcontent.XContentBuilder;
-import org.opensearch.test.rest.OpenSearchRestTestCase;
-
-import java.util.List;
-import java.util.Map;
-
+import org.opensearch.knn.KNNRestTestCase;
+import org.junit.Before;
+import org.junit.Test;
+import lombok.SneakyThrows;
 import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.notNullValue;
-import static org.hamcrest.Matchers.hasSize;
-
-public class RestoreSnapshotIT extends OpenSearchRestTestCase {
-
-    private String index;
-    private String snapshot;
-    private String repository;
 
-    private void setupSnapshotRestore() throws Exception {
-        Request clusterSettingsRequest = new Request("GET", "/_cluster/settings");
-        clusterSettingsRequest.addParameter("include_defaults", "true");
-        Response clusterSettingsResponse = client().performRequest(clusterSettingsRequest);
-        Map<String, Object> clusterSettings = entityAsMap(clusterSettingsResponse);
+public class RestoreSnapshotIT extends KNNRestTestCase {
 
-        @SuppressWarnings("unchecked")
-        List<String> pathRepos = (List<String>) XContentMapValues.extractValue("defaults.path.repo", clusterSettings);
-        assertThat(pathRepos, notNullValue());
-        assertThat(pathRepos, hasSize(1));
+    private String index = "test-index";;
+    private String snapshot = "snapshot-" + index;
+    private String repository = "repo";
 
-        final String pathRepo = pathRepos.get(0);
-
-        index = "test-index";
-        snapshot = "snapshot-" + index;
-        repository = "repo";
-
-        // create index
-        Settings indexSettings = Settings.builder().put("number_of_shards", 1).put("number_of_replicas", 1).put("index.knn", true).build();
-        createIndex(index, indexSettings);
+    @Before
+    @SneakyThrows
+    public void setUp() {
+        super.setUp();
+        setupSnapshotRestore(index, snapshot, repository);
+    }
 
-        // create repo
-        Settings repoSettings = Settings.builder().put("compress", randomBoolean()).put("location", pathRepo).build();
-        registerRepository(repository, "fs", true, repoSettings);
+    @Test
+    @SneakyThrows
+    public void testKnnSettingIsModifiable_whenRestore_thenSuccess() {
+        // valid restore
+        XContentBuilder restoreCommand = JsonXContent.contentBuilder().startObject();
+        restoreCommand.field("indices", index);
+        restoreCommand.field("rename_pattern", index);
+        restoreCommand.field("rename_replacement", "restored-" + index);
+        restoreCommand.startObject("index_settings");
+        {
+            restoreCommand.field("knn.model.index.number_of_shards", 1);
+        }
+        restoreCommand.endObject();
+        restoreCommand.endObject();
+        Request restoreRequest = new Request("POST", "/_snapshot/" + repository + "/" + snapshot + "/_restore");
+        restoreRequest.addParameter("wait_for_completion", "true");
+        restoreRequest.setJsonEntity(restoreCommand.toString());
 
-        // create snapshot
-        createSnapshot(repository, snapshot, true);
+        final Response restoreResponse = client().performRequest(restoreRequest);
+        assertEquals(200, restoreResponse.getStatusLine().getStatusCode());
     }
 
-    public void testKnnSettingIsUnmodifiableOnRestore() throws Exception {
-        setupSnapshotRestore();
-
+    @Test
+    @SneakyThrows
+    public void testKnnSettingIsUnmodifiable_whenRestore_thenFailure() {
         // invalid restore
         XContentBuilder restoreCommand = JsonXContent.contentBuilder().startObject();
         restoreCommand.field("indices", index);
@@ -77,9 +72,26 @@ public void testKnnSettingIsUnmodifiableOnRestore() throws Exception {
         assertThat(error.getMessage(), containsString("cannot modify UnmodifiableOnRestore setting [index.knn]" + " on restore"));
     }
 
-    public void testKnnSettingCannotBeIgnoredDuringRestore() throws Exception {
-        setupSnapshotRestore();
+    @Test
+    @SneakyThrows
+    public void testKnnSettingCanBeIgnored_whenRestore_thenSuccess() {
+        // valid restore
+        XContentBuilder restoreCommand = JsonXContent.contentBuilder().startObject();
+        restoreCommand.field("indices", index);
+        restoreCommand.field("rename_pattern", index);
+        restoreCommand.field("rename_replacement", "restored-" + index);
+        restoreCommand.field("ignore_index_settings", "knn.model.index.number_of_shards");
+        restoreCommand.endObject();
+        Request restoreRequest = new Request("POST", "/_snapshot/" + repository + "/" + snapshot + "/_restore");
+        restoreRequest.addParameter("wait_for_completion", "true");
+        restoreRequest.setJsonEntity(restoreCommand.toString());
+        final Response restoreResponse = client().performRequest(restoreRequest);
+        assertEquals(200, restoreResponse.getStatusLine().getStatusCode());
+    }
 
+    @Test
+    @SneakyThrows
+    public void testKnnSettingCannotBeIgnored_whenRestore_thenFailure() {
         // invalid restore
         XContentBuilder restoreCommand = JsonXContent.contentBuilder().startObject();
         restoreCommand.field("indices", index);

diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java
@@ -17,6 +17,7 @@
 import org.apache.http.client.utils.URIBuilder;
 import org.apache.http.util.EntityUtils;
 import org.opensearch.Version;
+import org.opensearch.common.xcontent.support.XContentMapValues;
 import org.opensearch.core.common.bytes.BytesReference;
 import org.opensearch.common.xcontent.XContentHelper;
 import org.opensearch.core.xcontent.DeprecationHandler;
@@ -73,6 +74,8 @@
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.notNullValue;
 import static org.opensearch.knn.common.KNNConstants.DIMENSION;
 import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_CODE_SIZE;
 import static org.opensearch.knn.common.KNNConstants.ENCODER_PARAMETER_PQ_M;
@@ -1980,4 +1983,29 @@ protected boolean isApproximateThresholdSupported(final Optional<String> bwcVers
     protected static String randomLowerCaseString() {
         return randomAlphaOfLengthBetween(MIN_CODE_UNITS, MAX_CODE_UNITS).toLowerCase(Locale.ROOT);
     }
+
+    @SneakyThrows
+    protected void setupSnapshotRestore(String index, String snapshot, String repository) {
+        Request clusterSettingsRequest = new Request("GET", "/_cluster/settings");
+        clusterSettingsRequest.addParameter("include_defaults", "true");
+        Response clusterSettingsResponse = client().performRequest(clusterSettingsRequest);
+        Map<String, Object> clusterSettings = entityAsMap(clusterSettingsResponse);
+
+        @SuppressWarnings("unchecked")
+        List<String> pathRepos = (List<String>) XContentMapValues.extractValue("defaults.path.repo", clusterSettings);
+        assertThat(pathRepos, notNullValue());
+        assertThat(pathRepos, hasSize(1));
+
+        final String pathRepo = pathRepos.get(0);
+
+        // create index
+        createIndex(index, getDefaultIndexSettings());
+
+        // create repo
+        Settings repoSettings = Settings.builder().put("compress", randomBoolean()).put("location", pathRepo).build();
+        registerRepository(repository, "fs", true, repoSettings);
+
+        // create snapshot
+        createSnapshot(repository, snapshot, true);
+    }
 }