Fixed a bug to prevent updating index.knn setting after index creation (

opensearch-project#2348) * Change index.knn setting to FINAL, immutable after index creation Signed-off-by: AnnTian Shao <[email protected]> * Add to ChangeLog the description of bug fix Signed-off-by: AnnTian Shao <[email protected]> * Add restart upgrade test for checking immutability of knn.index setting after version upgrade Signed-off-by: Tommy Shao <[email protected]> --------- Signed-off-by: AnnTian Shao <[email protected]> Signed-off-by: Tommy Shao <[email protected]> Signed-off-by: Tommy Shao <[email protected]> Co-authored-by: AnnTian Shao <[email protected]> (cherry picked from commit a875eb8)
anntians · Jan 10, 2025 · a003c56 · a003c56
1 parent cc72442
commit a003c56
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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<Boolean> IS_KNN_INDEX_SETTING = Setting.boolSetting(KNN_INDEX, false, IndexScope);
+    public static final Setting<Boolean> 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
@@ -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));