From 0c0332e67c73a1c60c02e80e933b674db1d1d3f5 Mon Sep 17 00:00:00 2001 From: Tommy Shao <69884021+anntians@users.noreply.github.com> Date: Thu, 9 Jan 2025 15:13:46 -0800 Subject: [PATCH] Fixed a bug to prevent updating index.knn setting after index creation (#2348) * Change index.knn setting to FINAL, immutable after index creation Signed-off-by: AnnTian Shao * Add to ChangeLog the description of bug fix Signed-off-by: AnnTian Shao * Add restart upgrade test for checking immutability of knn.index setting after version upgrade Signed-off-by: Tommy Shao --------- Signed-off-by: AnnTian Shao Signed-off-by: Tommy Shao <69884021+anntians@users.noreply.github.com> Signed-off-by: Tommy Shao Co-authored-by: AnnTian Shao (cherry picked from commit a875eb866dcc16086d1410b1cceae88e8cbfc9c5) --- CHANGELOG.md | 1 + .../org/opensearch/knn/bwc/IndexingIT.java | 109 ++++++++++++++++++ .../org/opensearch/knn/index/KNNSettings.java | 3 +- .../knn/index/KNNESSettingsTestIT.java | 20 ++++ 4 files changed, 132 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ef36dc57..252ef5c1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), * 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] * 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] diff --git a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java index b212d844f..edccada44 100644 --- a/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java +++ b/qa/restart-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java @@ -5,17 +5,23 @@ package org.opensearch.knn.bwc; +import org.apache.hc.core5.http.io.entity.EntityUtils; import org.junit.Assert; +import org.opensearch.client.Response; +import org.opensearch.client.ResponseException; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.knn.KNNResult; import org.opensearch.knn.index.KNNSettings; import org.opensearch.knn.index.SpaceType; import org.opensearch.knn.index.VectorDataType; import org.opensearch.knn.index.engine.KNNEngine; +import org.opensearch.knn.index.query.KNNQueryBuilder; import java.util.List; import java.util.Map; +import static org.hamcrest.Matchers.containsString; import static org.opensearch.knn.TestUtils.KNN_ALGO_PARAM_EF_CONSTRUCTION_MIN_VALUE; import static org.opensearch.knn.TestUtils.KNN_ALGO_PARAM_M_MIN_VALUE; import static org.opensearch.knn.TestUtils.KNN_VECTOR; @@ -32,6 +38,8 @@ import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_EF_SEARCH; import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_M; import static org.opensearch.knn.common.KNNConstants.METHOD_PARAMETER_SPACE_TYPE; +import static org.opensearch.knn.common.KNNConstants.METHOD_ENCODER_PARAMETER; +import static org.opensearch.knn.common.KNNConstants.ENCODER_SQ; import static org.opensearch.knn.common.KNNConstants.NAME; import static org.opensearch.knn.common.KNNConstants.PARAMETERS; @@ -126,6 +134,107 @@ public void testKNNIndexLuceneForceMerge() throws Exception { } } + public void testKNNIndexSettingImmutableAfterUpgrade() throws Exception { + waitForClusterHealthGreen(NODES_BWC_CLUSTER); + + if (isRunningAgainstOldCluster()) { + createKnnIndex(testIndex, getKNNDefaultIndexSettings(), createKnnIndexMapping(TEST_FIELD, DIMENSIONS)); + } else { + Exception ex = expectThrows( + ResponseException.class, + () -> updateIndexSettings(testIndex, Settings.builder().put(KNNSettings.KNN_INDEX, false)) + ); + assertThat(ex.getMessage(), containsString("Can't update non dynamic settings [[index.knn]] for open indices")); + + closeIndex(testIndex); + + ex = expectThrows( + ResponseException.class, + () -> updateIndexSettings(testIndex, Settings.builder().put(KNNSettings.KNN_INDEX, false)) + ); + assertThat(ex.getMessage(), containsString(String.format("final %s setting [index.knn], not updateable", testIndex))); + } + } + + public void testKNNIndexLuceneByteVector() throws Exception { + waitForClusterHealthGreen(NODES_BWC_CLUSTER); + + if (isRunningAgainstOldCluster()) { + createKnnIndex( + testIndex, + getKNNDefaultIndexSettings(), + createKnnIndexMapping(TEST_FIELD, DIMENSIONS, METHOD_HNSW, LUCENE_NAME, SpaceType.L2.getValue(), true, VectorDataType.BYTE) + ); + addKNNByteDocs(testIndex, TEST_FIELD, DIMENSIONS, DOC_ID, 50); + // Flush to ensure that index is not re-indexed when node comes back up + flush(testIndex, true); + validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, 50, 5); + } else { + validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, 50, 5); + addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, 50, 25); + validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, 75, 5); + deleteKNNIndex(testIndex); + } + } + + public void testKNNIndexLuceneQuantization() throws Exception { + waitForClusterHealthGreen(NODES_BWC_CLUSTER); + int k = 4; + int dimension = 2; + + if (isRunningAgainstOldCluster()) { + String mapping = XContentFactory.jsonBuilder() + .startObject() + .startObject("properties") + .startObject(TEST_FIELD) + .field(VECTOR_TYPE, KNN_VECTOR) + .field(DIMENSION, dimension) + .startObject(KNN_METHOD) + .field(NAME, METHOD_HNSW) + .field(METHOD_PARAMETER_SPACE_TYPE, SpaceType.INNER_PRODUCT.getValue()) + .field(KNN_ENGINE, LUCENE_NAME) + .startObject(PARAMETERS) + .startObject(METHOD_ENCODER_PARAMETER) + .field(NAME, ENCODER_SQ) + .endObject() + .field(METHOD_PARAMETER_EF_CONSTRUCTION, 256) + .field(METHOD_PARAMETER_M, 16) + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() + .toString(); + createKnnIndex(testIndex, getKNNDefaultIndexSettings(), mapping); + + Float[] vector1 = { -10.6f, 25.48f }; + Float[] vector2 = { -10.8f, 25.48f }; + Float[] vector3 = { -11.0f, 25.48f }; + Float[] vector4 = { -11.2f, 25.48f }; + addKnnDoc(testIndex, "1", TEST_FIELD, vector1); + addKnnDoc(testIndex, "2", TEST_FIELD, vector2); + addKnnDoc(testIndex, "3", TEST_FIELD, vector3); + addKnnDoc(testIndex, "4", TEST_FIELD, vector4); + + float[] queryVector = { -10.5f, 25.48f }; + Response searchResponse = searchKNNIndex(testIndex, new KNNQueryBuilder(TEST_FIELD, queryVector, k), k); + List results = parseSearchResponse(EntityUtils.toString(searchResponse.getEntity()), TEST_FIELD); + assertEquals(k, results.size()); + for (int i = 0; i < k; i++) { + assertEquals(k - i, Integer.parseInt(results.get(i).getDocId())); + } + } else { + float[] queryVector = { -10.5f, 25.48f }; + Response searchResponse = searchKNNIndex(testIndex, new KNNQueryBuilder(TEST_FIELD, queryVector, k), k); + List results = parseSearchResponse(EntityUtils.toString(searchResponse.getEntity()), TEST_FIELD); + assertEquals(k, results.size()); + for (int i = 0; i < k; i++) { + assertEquals(k - i, Integer.parseInt(results.get(i).getDocId())); + } + deleteKNNIndex(testIndex); + } + } + // Ensure bwc works for binary force merge public void testKNNIndexBinaryForceMerge() throws Exception { int dimension = 40; diff --git a/src/main/java/org/opensearch/knn/index/KNNSettings.java b/src/main/java/org/opensearch/knn/index/KNNSettings.java index 6dc72a22b..097329d81 100644 --- a/src/main/java/org/opensearch/knn/index/KNNSettings.java +++ b/src/main/java/org/opensearch/knn/index/KNNSettings.java @@ -43,6 +43,7 @@ import static org.opensearch.common.settings.Setting.Property.Dynamic; 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.unit.MemorySizeValue.parseBytesSizeValueOrHeapRatio; import static org.opensearch.core.common.unit.ByteSizeValue.parseBytesSizeValue; import static org.opensearch.knn.common.featureflags.KNNFeatureFlags.getFeatureFlags; @@ -268,7 +269,7 @@ public class KNNSettings { /** * This setting identifies KNN index. */ - public static final Setting IS_KNN_INDEX_SETTING = Setting.boolSetting(KNN_INDEX, false, IndexScope); + public static final Setting IS_KNN_INDEX_SETTING = Setting.boolSetting(KNN_INDEX, false, IndexScope, Final); /** * 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/KNNESSettingsTestIT.java b/src/test/java/org/opensearch/knn/index/KNNESSettingsTestIT.java index 178b1d0c0..410d6e905 100644 --- a/src/test/java/org/opensearch/knn/index/KNNESSettingsTestIT.java +++ b/src/test/java/org/opensearch/knn/index/KNNESSettingsTestIT.java @@ -124,6 +124,26 @@ public void testUpdateIndexSetting() throws IOException { assertThat(ex.getMessage(), containsString("Failed to parse value [1] for setting [index.knn.algo_param.ef_search] must be >= 2")); } + public void testUpdateIndexSettingKnnFlagImmutable() throws IOException { + Settings settings = Settings.builder().put(KNNSettings.KNN_INDEX, true).build(); + createKnnIndex(INDEX_NAME, settings, createKnnIndexMapping(FIELD_NAME, 2)); + + Exception ex = expectThrows( + ResponseException.class, + () -> updateIndexSettings(INDEX_NAME, Settings.builder().put(KNNSettings.KNN_INDEX, false)) + ); + assertThat(ex.getMessage(), containsString("Can't update non dynamic settings [[index.knn]] for open indices")); + + closeIndex(INDEX_NAME); + + ex = expectThrows( + ResponseException.class, + () -> updateIndexSettings(INDEX_NAME, Settings.builder().put(KNNSettings.KNN_INDEX, false)) + ); + assertThat(ex.getMessage(), containsString(String.format("final %s setting [index.knn], not updateable", INDEX_NAME))); + + } + @SuppressWarnings("unchecked") public void testCacheRebuiltAfterUpdateIndexSettings() throws Exception { createKnnIndex(INDEX_NAME, getKNNDefaultIndexSettings(), createKnnIndexMapping(FIELD_NAME, 2));