From dd403a1bc0d307331d83bcf2a94619df5fa5ff9e Mon Sep 17 00:00:00 2001 From: Tommy Shao <69884021+anntians@users.noreply.github.com> Date: Mon, 27 Jan 2025 14:45:43 -0800 Subject: [PATCH 1/2] Add UnmodifiableOnRestore property to index.knn setting (#2445) * Add UnmodifiableOnRestore property to index.knn setting Signed-off-by: AnnTian Shao * Update tests with helper methods Signed-off-by: AnnTian Shao * Update test names Signed-off-by: AnnTian Shao * Add to changeLog and fixes to tests Signed-off-by: AnnTian Shao --------- Signed-off-by: AnnTian Shao Co-authored-by: AnnTian Shao (cherry picked from commit 1d2c4c0c783428fdea7141ad069134a25b590c54) --- CHANGELOG.md | 12 ++ .../org/opensearch/knn/index/KNNSettings.java | 8 +- .../knn/index/RestoreSnapshotIT.java | 108 ++++++++++++++++++ .../org/opensearch/knn/KNNRestTestCase.java | 28 +++++ 4 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 70ec3ae51..cbcec8c86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Features ### Enhancements ### Bug Fixes +* Fixing the bug when a segment has no vector field present for disk based vector search (#2282)[https://github.com/opensearch-project/k-NN/pull/2282] +* Fixing the bug where search fails with "fields" parameter for an index with a knn_vector field (#2314)[https://github.com/opensearch-project/k-NN/pull/2314] +* Fix for NPE while merging segments after all the vector fields docs are deleted (#2365)[https://github.com/opensearch-project/k-NN/pull/2365] +* Allow validation for non knn index only after 2.17.0 (#2315)[https://github.com/opensearch-project/k-NN/pull/2315] +* Fixing the bug to prevent updating the index.knn setting after index creation(#2348)[https://github.com/opensearch-project/k-NN/pull/2348] +* Release query vector memory after execution (#2346)[https://github.com/opensearch-project/k-NN/pull/2346] +* Fix shard level rescoring disabled setting flag (#2352)[https://github.com/opensearch-project/k-NN/pull/2352] +* 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 bug where mapping accepts both dimension and model-id (#2410)[https://github.com/opensearch-project/k-NN/pull/2410] +* 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] * Fix derived source for binary and byte vectors [#2533](https://github.com/opensearch-project/k-NN/pull/2533/) * Fix the put mapping issue for already created index with flat mapper [#2542](https://github.com/opensearch-project/k-NN/pull/2542) ### Infrastructure diff --git a/src/main/java/org/opensearch/knn/index/KNNSettings.java b/src/main/java/org/opensearch/knn/index/KNNSettings.java index c33f3ea63..dffe3009e 100644 --- a/src/main/java/org/opensearch/knn/index/KNNSettings.java +++ b/src/main/java/org/opensearch/knn/index/KNNSettings.java @@ -283,7 +283,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..a2c48a221 --- /dev/null +++ b/src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java @@ -0,0 +1,108 @@ +/* + * 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.common.xcontent.json.JsonXContent; +import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.knn.KNNRestTestCase; +import org.junit.Before; +import org.junit.Test; +import lombok.SneakyThrows; +import static org.hamcrest.Matchers.containsString; + +public class RestoreSnapshotIT extends KNNRestTestCase { + + private String index = "test-index";; + private String snapshot = "snapshot-" + index; + private String repository = "repo"; + + @Before + @SneakyThrows + public void setUp() { + super.setUp(); + setupSnapshotRestore(index, snapshot, repository); + } + + @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()); + + final Response restoreResponse = client().performRequest(restoreRequest); + assertEquals(200, restoreResponse.getStatusLine().getStatusCode()); + } + + @Test + @SneakyThrows + public void testKnnSettingIsUnmodifiable_whenRestore_thenFailure() { + // 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/" + repository + "/" + 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")); + } + + @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); + 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/" + repository + "/" + 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")); + } +} diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java index c067a211d..e1b0e2700 100644 --- a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java +++ b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java @@ -19,6 +19,7 @@ import org.junit.AfterClass; import org.junit.Before; import org.opensearch.Version; +import org.opensearch.common.xcontent.support.XContentMapValues; import org.opensearch.client.Request; import org.opensearch.client.Response; import org.opensearch.common.settings.Settings; @@ -86,6 +87,8 @@ import static org.opensearch.knn.TestUtils.VECTOR_TYPE; import static org.opensearch.knn.TestUtils.computeGroundTruthValues; import static org.opensearch.knn.common.KNNConstants.CLEAR_CACHE; +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; @@ -2370,4 +2373,29 @@ protected boolean isRemoteIndexBuildSupported(final Optional bwcVersion) 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); + } } From ac33c13dc4006d3b7d05fff24fcf2a2bf2a3838d Mon Sep 17 00:00:00 2001 From: AnnTian Shao Date: Tue, 25 Feb 2025 11:43:19 -0800 Subject: [PATCH 2/2] fix changelog Signed-off-by: AnnTian Shao --- CHANGELOG.md | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbcec8c86..7ccfe4a9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,20 +22,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Features ### Enhancements ### Bug Fixes -* Fixing the bug when a segment has no vector field present for disk based vector search (#2282)[https://github.com/opensearch-project/k-NN/pull/2282] -* Fixing the bug where search fails with "fields" parameter for an index with a knn_vector field (#2314)[https://github.com/opensearch-project/k-NN/pull/2314] -* Fix for NPE while merging segments after all the vector fields docs are deleted (#2365)[https://github.com/opensearch-project/k-NN/pull/2365] -* Allow validation for non knn index only after 2.17.0 (#2315)[https://github.com/opensearch-project/k-NN/pull/2315] -* Fixing the bug to prevent updating the index.knn setting after index creation(#2348)[https://github.com/opensearch-project/k-NN/pull/2348] -* Release query vector memory after execution (#2346)[https://github.com/opensearch-project/k-NN/pull/2346] -* Fix shard level rescoring disabled setting flag (#2352)[https://github.com/opensearch-project/k-NN/pull/2352] -* 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 bug where mapping accepts both dimension and model-id (#2410)[https://github.com/opensearch-project/k-NN/pull/2410] -* 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] * Fix derived source for binary and byte vectors [#2533](https://github.com/opensearch-project/k-NN/pull/2533/) * Fix the put mapping issue for already created index with flat mapper [#2542](https://github.com/opensearch-project/k-NN/pull/2542) +* 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 ### Documentation ### Maintenance