From a003c56afe8cd21e590056bb3bca835ecd4e34fe 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 | 25 +++++++++++++++++++ .../org/opensearch/knn/index/KNNSettings.java | 3 ++- .../knn/index/KNNESSettingsTestIT.java | 20 +++++++++++++++ 4 files changed, 48 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..5bbf165e6 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 @@ -6,6 +6,8 @@ package org.opensearch.knn.bwc; 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.index.KNNSettings; @@ -16,6 +18,7 @@ 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; @@ -126,6 +129,28 @@ 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))); + } + } + // 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));