From 8f75f4fa7b58600262a3a14b7387af870af6702c Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Sat, 25 Jan 2025 15:21:37 -0800 Subject: [PATCH 1/4] Add UnmodifiableOnRestore property to index.knn setting Signed-off-by: AnnTian Shao --- .../org/opensearch/knn/index/KNNSettings.java | 9 +- .../knn/index/RestoreSnapshotIT.java | 122 ++++++++++++++++++ 2 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java diff --git a/src/main/java/org/opensearch/knn/index/KNNSettings.java b/src/main/java/org/opensearch/knn/index/KNNSettings.java index 097329d81..abce6cc76 100644 --- a/src/main/java/org/opensearch/knn/index/KNNSettings.java +++ b/src/main/java/org/opensearch/knn/index/KNNSettings.java @@ -44,6 +44,7 @@ import static org.opensearch.common.settings.Setting.Property.IndexScope; import static org.opensearch.common.settings.Setting.Property.NodeScope; import static org.opensearch.common.settings.Setting.Property.Final; +import static org.opensearch.common.settings.Setting.Property.UnmodifiableOnRestore; import static org.opensearch.common.unit.MemorySizeValue.parseBytesSizeValueOrHeapRatio; import static org.opensearch.core.common.unit.ByteSizeValue.parseBytesSizeValue; import static org.opensearch.knn.common.featureflags.KNNFeatureFlags.getFeatureFlags; @@ -269,7 +270,13 @@ public class KNNSettings { /** * This setting identifies KNN index. */ - public static final Setting IS_KNN_INDEX_SETTING = Setting.boolSetting(KNN_INDEX, false, IndexScope, Final); + public static final Setting IS_KNN_INDEX_SETTING = Setting.boolSetting( + KNN_INDEX, + false, + IndexScope, + Final, + UnmodifiableOnRestore + ); /** * index_thread_quantity - the parameter specifies how many threads the nms library should use to create the graph. diff --git a/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java b/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java new file mode 100644 index 000000000..36761f136 --- /dev/null +++ b/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java @@ -0,0 +1,122 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.knn.index; + +import org.opensearch.client.Request; +import org.opensearch.client.Response; +import org.opensearch.client.ResponseException; +import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.xcontent.json.JsonXContent; +import org.opensearch.common.xcontent.support.XContentMapValues; +import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.index.IndexSettings; +import org.opensearch.test.rest.OpenSearchRestTestCase; + +import java.util.List; +import java.util.Map; + +import static org.hamcrest.Matchers.*; +import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; + +public class RestoreSnapshotIT extends OpenSearchRestTestCase { + + private String index; + private String snapshot; + + public void setupSnapshotRestore() throws Exception { + Request clusterSettingsRequest = new Request("GET", "/_cluster/settings"); + clusterSettingsRequest.addParameter("include_defaults", "true"); + Response clusterSettingsResponse = client().performRequest(clusterSettingsRequest); + Map clusterSettings = entityAsMap(clusterSettingsResponse); + + @SuppressWarnings("unchecked") + List pathRepos = (List) XContentMapValues.extractValue("defaults.path.repo", clusterSettings); + assertThat(pathRepos, notNullValue()); + assertThat(pathRepos, hasSize(1)); + + final String pathRepo = pathRepos.get(0); + + index = "test-index"; + snapshot = "snapshot-" + index; + + // create index + XContentBuilder settings = jsonBuilder(); + settings.startObject(); + { + settings.startObject("settings"); + settings.field(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1); + settings.field(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1); + settings.field(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true); + settings.endObject(); + } + settings.endObject(); + + Request createIndex = new Request("PUT", "/" + index); + createIndex.setJsonEntity(settings.toString()); + createIndex.setOptions(allowTypesRemovalWarnings()); + client().performRequest(createIndex); + + // create repo + XContentBuilder repoConfig = JsonXContent.contentBuilder().startObject(); + { + repoConfig.field("type", "fs"); + repoConfig.startObject("settings"); + { + repoConfig.field("compress", randomBoolean()); + repoConfig.field("location", pathRepo); + } + repoConfig.endObject(); + } + repoConfig.endObject(); + Request createRepoRequest = new Request("PUT", "/_snapshot/repo"); + createRepoRequest.setJsonEntity(repoConfig.toString()); + client().performRequest(createRepoRequest); + + // create snapshot + Request createSnapshot = new Request("PUT", "/_snapshot/repo/" + snapshot); + createSnapshot.addParameter("wait_for_completion", "true"); + createSnapshot.setJsonEntity("{\"indices\": \"" + index + "\"}"); + client().performRequest(createSnapshot); + } + + public void testUnmodifiableOnRestoreSettingModifiedOnRestore() throws Exception { + setupSnapshotRestore(); + + // invalid 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("index.knn", false); + } + restoreCommand.endObject(); + restoreCommand.endObject(); + Request restoreRequest = new Request("POST", "/_snapshot/repo/" + snapshot + "/_restore"); + restoreRequest.addParameter("wait_for_completion", "true"); + restoreRequest.setJsonEntity(restoreCommand.toString()); + final ResponseException error = expectThrows(ResponseException.class, () -> client().performRequest(restoreRequest)); + assertThat(error.getMessage(), containsString("cannot modify UnmodifiableOnRestore setting [index.knn]" + " on restore")); + } + + public void testUnmodifiableOnRestoreSettingIgnoredOnRestore() throws Exception { + setupSnapshotRestore(); + + // invalid 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", "index.knn"); + restoreCommand.endObject(); + Request restoreRequest = new Request("POST", "/_snapshot/repo/" + snapshot + "/_restore"); + restoreRequest.addParameter("wait_for_completion", "true"); + restoreRequest.setJsonEntity(restoreCommand.toString()); + final ResponseException error = expectThrows(ResponseException.class, () -> client().performRequest(restoreRequest)); + assertThat(error.getMessage(), containsString("cannot remove UnmodifiableOnRestore setting [index.knn] on restore")); + } +} From f05d0f161049e815fa8bffd8b37ccc7d5cde39d8 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Mon, 27 Jan 2025 08:32:52 -0800 Subject: [PATCH 2/4] Update tests with helper methods Signed-off-by: AnnTian Shao --- .../knn/index/RestoreSnapshotIT.java | 54 +++++-------------- 1 file changed, 14 insertions(+), 40 deletions(-) diff --git a/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java b/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java index 36761f136..c9936f2db 100644 --- a/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java +++ b/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java @@ -8,25 +8,26 @@ import org.opensearch.client.Request; import org.opensearch.client.Response; import org.opensearch.client.ResponseException; -import org.opensearch.cluster.metadata.IndexMetadata; +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.index.IndexSettings; import org.opensearch.test.rest.OpenSearchRestTestCase; import java.util.List; import java.util.Map; -import static org.hamcrest.Matchers.*; -import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; +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; - public void setupSnapshotRestore() throws Exception { + private void setupSnapshotRestore() throws Exception { Request clusterSettingsRequest = new Request("GET", "/_cluster/settings"); clusterSettingsRequest.addParameter("include_defaults", "true"); Response clusterSettingsResponse = client().performRequest(clusterSettingsRequest); @@ -41,45 +42,18 @@ public void setupSnapshotRestore() throws Exception { index = "test-index"; snapshot = "snapshot-" + index; + repository = "repo"; // create index - XContentBuilder settings = jsonBuilder(); - settings.startObject(); - { - settings.startObject("settings"); - settings.field(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1); - settings.field(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1); - settings.field(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true); - settings.endObject(); - } - settings.endObject(); - - Request createIndex = new Request("PUT", "/" + index); - createIndex.setJsonEntity(settings.toString()); - createIndex.setOptions(allowTypesRemovalWarnings()); - client().performRequest(createIndex); + Settings indexSettings = Settings.builder().put("number_of_shards", 1).put("number_of_replicas", 1).put("index.knn", true).build(); + createIndex(index, indexSettings); // create repo - XContentBuilder repoConfig = JsonXContent.contentBuilder().startObject(); - { - repoConfig.field("type", "fs"); - repoConfig.startObject("settings"); - { - repoConfig.field("compress", randomBoolean()); - repoConfig.field("location", pathRepo); - } - repoConfig.endObject(); - } - repoConfig.endObject(); - Request createRepoRequest = new Request("PUT", "/_snapshot/repo"); - createRepoRequest.setJsonEntity(repoConfig.toString()); - client().performRequest(createRepoRequest); + Settings repoSettings = Settings.builder().put("compress", randomBoolean()).put("location", pathRepo).build(); + registerRepository(repository, "fs", true, repoSettings); // create snapshot - Request createSnapshot = new Request("PUT", "/_snapshot/repo/" + snapshot); - createSnapshot.addParameter("wait_for_completion", "true"); - createSnapshot.setJsonEntity("{\"indices\": \"" + index + "\"}"); - client().performRequest(createSnapshot); + createSnapshot(repository, snapshot, true); } public void testUnmodifiableOnRestoreSettingModifiedOnRestore() throws Exception { @@ -96,7 +70,7 @@ public void testUnmodifiableOnRestoreSettingModifiedOnRestore() throws Exception } restoreCommand.endObject(); restoreCommand.endObject(); - Request restoreRequest = new Request("POST", "/_snapshot/repo/" + snapshot + "/_restore"); + Request restoreRequest = new Request("POST", "/_snapshot/" + repository + "/" + snapshot + "/_restore"); restoreRequest.addParameter("wait_for_completion", "true"); restoreRequest.setJsonEntity(restoreCommand.toString()); final ResponseException error = expectThrows(ResponseException.class, () -> client().performRequest(restoreRequest)); @@ -113,7 +87,7 @@ public void testUnmodifiableOnRestoreSettingIgnoredOnRestore() throws Exception restoreCommand.field("rename_replacement", "restored-" + index); restoreCommand.field("ignore_index_settings", "index.knn"); restoreCommand.endObject(); - Request restoreRequest = new Request("POST", "/_snapshot/repo/" + snapshot + "/_restore"); + Request restoreRequest = new Request("POST", "/_snapshot/" + repository + "/" + snapshot + "/_restore"); restoreRequest.addParameter("wait_for_completion", "true"); restoreRequest.setJsonEntity(restoreCommand.toString()); final ResponseException error = expectThrows(ResponseException.class, () -> client().performRequest(restoreRequest)); From 7e5f72012326fd690828ac0680ac07a2acc355b0 Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Mon, 27 Jan 2025 10:38:10 -0800 Subject: [PATCH 3/4] Update test names Signed-off-by: AnnTian Shao --- src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java b/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java index c9936f2db..150ec2540 100644 --- a/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java +++ b/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java @@ -56,7 +56,7 @@ private void setupSnapshotRestore() throws Exception { createSnapshot(repository, snapshot, true); } - public void testUnmodifiableOnRestoreSettingModifiedOnRestore() throws Exception { + public void testKnnSettingIsUnmodifiableOnRestore() throws Exception { setupSnapshotRestore(); // invalid restore @@ -77,7 +77,7 @@ public void testUnmodifiableOnRestoreSettingModifiedOnRestore() throws Exception assertThat(error.getMessage(), containsString("cannot modify UnmodifiableOnRestore setting [index.knn]" + " on restore")); } - public void testUnmodifiableOnRestoreSettingIgnoredOnRestore() throws Exception { + public void testKnnSettingCannotBeIgnoredDuringRestore() throws Exception { setupSnapshotRestore(); // invalid restore From b722a00b2261926f8847533cc1e8c38fff8f365e Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Mon, 27 Jan 2025 11:56:22 -0800 Subject: [PATCH 4/4] Add to changeLog and fixes to tests Signed-off-by: AnnTian Shao --- CHANGELOG.md | 1 + .../knn/index/RestoreSnapshotIT.java | 98 +++++++++++-------- .../org/opensearch/knn/KNNRestTestCase.java | 28 ++++++ 3 files changed, 84 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f1929a5a..c6403bafb 100644 --- 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 index 150ec2540..a2c48a221 100644 --- 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 clusterSettings = entityAsMap(clusterSettingsResponse); +public class RestoreSnapshotIT extends KNNRestTestCase { - @SuppressWarnings("unchecked") - List pathRepos = (List) 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 index 6722dc235..572ff4c5e 100644 --- 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 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 clusterSettings = entityAsMap(clusterSettingsResponse); + + @SuppressWarnings("unchecked") + List pathRepos = (List) 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); + } }