Block New Index Ceation using NMSLIB From OS Version 3.0.0 (#2573)

* Block New Index Ceation using NMSLIB FROM OS Version 3.0.0 Signed-off-by: Vikasht34 <[email protected]> * Fix Spotless Signed-off-by: Vikasht34 <[email protected]> * Fix BWC Tests Signed-off-by: Vikasht34 <[email protected]> --------- Signed-off-by: Vikasht34 <[email protected]>
opensearch-project · Mar 5, 2025 · cc6b4f4 · cc6b4f4
1 parent f6577ea
commit cc6b4f4
Show file tree

Hide file tree

Showing 18 changed files with 300 additions and 243 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Refactoring
 * Small Refactor Post Lucene 10.0.1 upgrade [#2541](https://github.com/opensearch-project/k-NN/pull/2541)
 * Refactor codec to leverage backwards_codecs [#2546](https://github.com/opensearch-project/k-NN/pull/2546)
+* Blocking Index Creation using NMSLIB [#2573] (https://github.com/opensearch-project/k-NN/pull/2573)
 * Improve Streaming Compatibility Issue for MethodComponetContext and Remove OpenDistro URL [#2575] (https://github.com/opensearch-project/k-NN/pull/2575)
 * 3.0.0 Breaking Changes For KNN [#2564] (https://github.com/opensearch-project/k-NN/pull/2564)
 

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
@@ -418,4 +418,37 @@ public void validateKNNIndexingOnUpgrade(int numOfDocs) throws Exception {
         validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, QUERY_COUNT, K);
         deleteKNNIndex(testIndex);
     }
+
+    /**
+     * Test to verify that NMSLIB index creation is blocked in OpenSearch 3.0.0 and later,
+     * while ensuring backward compatibility (BWC) for existing indexes created in OpenSearch 2.19.
+     *
+     * @throws Exception if any unexpected error occurs during the test execution.
+     */
+    public void testBlockNMSLIBIndexCreationPost3_0_0() throws Exception {
+        waitForClusterHealthGreen(NODES_BWC_CLUSTER);
+
+        if (isRunningAgainstOldCluster()) {
+            createKnnIndex(
+                testIndex,
+                getKNNDefaultIndexSettings(),
+                createKnnIndexMapping(TEST_FIELD, DIMENSIONS, METHOD_HNSW, KNNEngine.NMSLIB.getName())
+            );
+            addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, DOC_ID, NUM_DOCS);
+            // Flush to ensure the index persists after upgrade
+            flush(testIndex, true);
+        } else {
+            validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, NUM_DOCS, K);
+            expectThrows(
+                ResponseException.class,
+                () -> createKnnIndex(
+                    testIndex + "_new",
+                    getKNNDefaultIndexSettings(),
+                    createKnnIndexMapping(TEST_FIELD, DIMENSIONS, METHOD_HNSW, KNNEngine.NMSLIB.getName())
+                )
+            );
+            deleteKNNIndex(testIndex);
+        }
+    }
+
 }
diff --git a/qa/rolling-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java b/qa/rolling-upgrade/src/test/java/org/opensearch/knn/bwc/IndexingIT.java
@@ -5,6 +5,8 @@
 
 package org.opensearch.knn.bwc;
 
+import org.opensearch.client.ResponseException;
+
 import static org.opensearch.knn.TestUtils.NODES_BWC_CLUSTER;
 
 public class IndexingIT extends AbstractRollingUpgradeTestCase {
@@ -17,6 +19,7 @@ public class IndexingIT extends AbstractRollingUpgradeTestCase {
 
     private static final String FAISS_NAME = "faiss";
     private static final String LUCENE_NAME = "lucene";
+    private static final String NMSLIB_NAME = "nmslib";
 
     public void testKNNDefaultIndexSettings() throws Exception {
         waitForClusterHealthGreen(NODES_BWC_CLUSTER);
@@ -185,4 +188,68 @@ public void validateKNNIndexingOnUpgrade(int totalDocsCount, int docId) throws E
         totalDocsCount = totalDocsCount + NUM_DOCS;
         validateKNNSearch(testIndex, TEST_FIELD, DIMENSIONS, totalDocsCount, K);
     }
+
+    /**
+     * Test to verify that NMSLIB index creation is blocked in OpenSearch 3.0.0 and later,
+     * while ensuring backward compatibility (BWC) for existing indexes created in OpenSearch 2.19,
+     * within a rolling upgrade scenario.
+     *
+     * <p><b>Test Flow:</b></p>
+     * <ol>
+     *     <li> <b>OLD CLUSTER (OpenSearch 2.19):</b>
+     *         <ul>
+     *             <li>Create an index using the NMSLIB engine.</li>
+     *             <li>Add sample documents.</li>
+     *             <li>Flush the index to ensure it persists after upgrade.</li>
+     *         </ul>
+     *     </li>
+     *     <li> <b>MIXED CLUSTER (Some nodes upgraded to OpenSearch 3.0.0):</b>
+     *         <ul>
+     *             <li>Validate that the previously created NMSLIB index is still searchable.</li>
+     *             <li>Ensure documents can still be added.</li>
+     *         </ul>
+     *     </li>
+     *     <li> <b>UPGRADED CLUSTER (All nodes upgraded to OpenSearch 3.0.0):</b>
+     *         <ul>
+     *             <li>Ensure the old NMSLIB index is still usable.</li>
+     *             <li>Attempt to create a new index using NMSLIB → <b>Should Fail</b> with a proper error message.</li>
+     *             <li>Ensure the error message matches expected behavior.</li>
+     *             <li>Cleanup: Delete the original index.</li>
+     *         </ul>
+     *     </li>
+     * </ol>
+     *
+     * @throws Exception if any unexpected error occurs during the test execution.
+     */
+    public void testBlockNMSLIBIndexCreationPost3_0_0_RollingUpgrade() throws Exception {
+        waitForClusterHealthGreen(NODES_BWC_CLUSTER);
+
+        switch (getClusterType()) {
+            case OLD:
+                createKnnIndex(testIndex, getKNNDefaultIndexSettings(), createKnnIndexMapping(TEST_FIELD, DIMENSIONS, ALGO, NMSLIB_NAME));
+                addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, 0, NUM_DOCS);
+                // Flush to ensure the index persists after upgrade
+                flush(testIndex, true);
+                break;
+
+            case MIXED:
+                addKNNDocs(testIndex, TEST_FIELD, DIMENSIONS, NUM_DOCS, NUM_DOCS);
+                break;
+
+            case UPGRADED:
+                Exception ex = expectThrows(
+                    ResponseException.class,
+                    () -> createKnnIndex(
+                        testIndex + "_new",
+                        getKNNDefaultIndexSettings(),
+                        createKnnIndexMapping(TEST_FIELD, DIMENSIONS, ALGO, NMSLIB_NAME)
+                    )
+                );
+
+                // Step 6: Cleanup - Delete the original index
+                deleteKNNIndex(testIndex);
+                break;
+        }
+    }
+
 }
diff --git a/src/main/java/org/opensearch/knn/index/engine/KNNEngine.java b/src/main/java/org/opensearch/knn/index/engine/KNNEngine.java
@@ -6,6 +6,7 @@
 package org.opensearch.knn.index.engine;
 
 import com.google.common.collect.ImmutableSet;
+import org.opensearch.Version;
 import org.opensearch.common.ValidationException;
 import org.opensearch.knn.index.SpaceType;
 import org.opensearch.knn.index.engine.faiss.Faiss;
@@ -26,11 +27,12 @@
  * passed to the respective k-NN library's JNI layer.
  */
 public enum KNNEngine implements KNNLibrary {
-    NMSLIB(NMSLIB_NAME, Nmslib.INSTANCE),
+    NMSLIB(NMSLIB_NAME, Nmslib.INSTANCE, Version.V_3_0_0),
     FAISS(FAISS_NAME, Faiss.INSTANCE),
     LUCENE(LUCENE_NAME, Lucene.INSTANCE);
 
     public static final KNNEngine DEFAULT = FAISS;
+    private final Version restrictedFromVersion; // Nullable field
 
     private static final Set<KNNEngine> CUSTOM_SEGMENT_FILE_ENGINES = ImmutableSet.of(KNNEngine.NMSLIB, KNNEngine.FAISS);
     private static final Set<KNNEngine> ENGINES_SUPPORTING_FILTERS = ImmutableSet.of(KNNEngine.LUCENE, KNNEngine.FAISS);
@@ -54,6 +56,16 @@ public enum KNNEngine implements KNNLibrary {
     KNNEngine(String name, KNNLibrary knnLibrary) {
         this.name = name;
         this.knnLibrary = knnLibrary;
+        this.restrictedFromVersion = null;
+    }
+
+    /**
+     * Constructor for deprecated engines.
+     */
+    KNNEngine(String name, KNNLibrary knnLibrary, Version restrictedVersion) {
+        this.name = name;
+        this.knnLibrary = knnLibrary;
+        this.restrictedFromVersion = restrictedVersion;
     }
 
     private final String name;
@@ -81,6 +93,17 @@ public static KNNEngine getEngine(String name) {
         throw new IllegalArgumentException(String.format("Invalid engine type: %s", name));
     }
 
+    /**
+     * Checks if the KNN engine is deprecated for a given OpenSearch version.
+     *
+     * @param indexVersionCreated The OpenSearch version in which the index is being created.
+     * @return {@code true} if the engine is deprecated in the specified version or later, {@code false} otherwise.
+     */
+    @Override
+    public boolean isRestricted(Version indexVersionCreated) {
+        return restrictedFromVersion != null && indexVersionCreated.onOrAfter(restrictedFromVersion);
+    }
+
     /**
      * Get the engine from the path.
      *
@@ -130,6 +153,15 @@ public String getName() {
         return name;
     }
 
+    /**
+     * Get the Deprecated Version
+     *
+     * @return Deprecated Version
+     */
+    public Version getRestrictedFromVersion() {
+        return restrictedFromVersion;
+    }
+
     @Override
     public String getVersion() {
         return knnLibrary.getVersion();

diff --git a/src/main/java/org/opensearch/knn/index/engine/KNNLibrary.java b/src/main/java/org/opensearch/knn/index/engine/KNNLibrary.java
@@ -5,6 +5,7 @@
 
 package org.opensearch.knn.index.engine;
 
+import org.opensearch.Version;
 import org.opensearch.common.ValidationException;
 import org.opensearch.knn.index.SpaceType;
 import org.opensearch.knn.index.remote.RemoteIndexParameters;
@@ -150,7 +151,18 @@ default boolean supportsRemoteIndexBuild() {
         return false;
     }
 
+    /**
+     * Checks if the library is deprecated for a given OpenSearch version.
+     *
+     * @param indexVersionCreated the OpenSearch version where the index is created
+     * @return true if deprecated, false otherwise
+     */
+    default boolean isRestricted(Version indexVersionCreated) {
+        return false; // By default, libraries are not deprecated
+    }
+
     default RemoteIndexParameters createRemoteIndexingParameters(Map<String, Object> indexInfoParameters) {
         throw new UnsupportedOperationException("Remote build service does not support this engine");
+
     }
 }
diff --git a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java
@@ -424,6 +424,8 @@ public Mapper.Builder<?> parse(String name, Map<String, Object> node, ParserCont
                 // Resolve method component. For the legacy case where space type can be configured at index level,
                 // it first tries to use the given one then tries to get it from index setting when the space type is UNDEFINED.
                 resolveKNNMethodComponents(builder, parserContext, resolvedSpaceType);
+                // Validate if the KNN engine is allowed for index creation
+                validateBlockedKNNEngine(builder.knnMethodContext.get(), parserContext.indexVersionCreated());
                 validateFromKNNMethod(builder);
             }
 
@@ -456,6 +458,29 @@ private void validateModeAndCompressionForDataType(KNNVectorFieldMapper.Builder
             }
         }
 
+        /**
+         * Validates whether the provided KNN engine is allowed for index creation.
+         * If the engine is NMSLIB and the OpenSearch version is 3.0.0 or later,
+         * it throws an IllegalArgumentException to prevent new index creation.
+         *
+         * @param knnMethodContext The KNN method configuration that contains the engine type.
+         * @param indexVersionCreated The OpenSearch version when the index is being created.
+         * @throws IllegalArgumentException if the engine is NMSLIB and version is 3.0.0 or later.
+         */
+        private void validateBlockedKNNEngine(KNNMethodContext knnMethodContext, Version indexVersionCreated) {
+            if (knnMethodContext == null) return;
+            KNNEngine engine = knnMethodContext.getKnnEngine();
+            if (engine.isRestricted(indexVersionCreated)) {
+                throw new IllegalArgumentException(
+                    engine.getName()
+                        + " engine is deprecated in OpenSearch "
+                        + " and cannot be used for new index creation in OpenSearch from  "
+                        + engine.getRestrictedFromVersion()
+                        + "."
+                );
+            }
+        }
+
         private void validateFromFlat(KNNVectorFieldMapper.Builder builder) {
             if (builder.modelId.get() != null || builder.knnMethodContext.get() != null) {
                 throw new IllegalArgumentException("Cannot set modelId or method parameters when index.knn setting is false");

diff --git a/src/test/java/org/opensearch/knn/index/KNNIndexShardTests.java b/src/test/java/org/opensearch/knn/index/KNNIndexShardTests.java
@@ -17,7 +17,6 @@
 import org.opensearch.common.settings.Settings;
 import org.opensearch.knn.KNNSingleNodeTestCase;
 import org.opensearch.index.IndexService;
-import org.opensearch.index.engine.Engine;
 import org.opensearch.index.shard.IndexShard;
 import org.opensearch.knn.index.engine.KNNEngine;
 import org.opensearch.knn.index.memory.NativeMemoryCacheManager;
@@ -109,27 +108,10 @@ public void testWarmup_shardNotPresentInCache() throws InterruptedException, Exe
 
     public void testGetAllEngineFileContexts() throws IOException, ExecutionException, InterruptedException {
         IndexService indexService = createKNNIndex(testIndexName);
-        createKnnIndexMapping(testIndexName, testFieldName, dimensions, KNNEngine.NMSLIB);
-        updateIndexSetting(testIndexName, Settings.builder().put(KNNSettings.INDEX_KNN_ADVANCED_APPROXIMATE_THRESHOLD, 0).build());
-
-        IndexShard indexShard = indexService.iterator().next();
-        KNNIndexShard knnIndexShard = new KNNIndexShard(indexShard);
-
-        Engine.Searcher searcher = indexShard.acquireSearcher("test-hnsw-paths-1");
-        List<KNNIndexShard.EngineFileContext> engineFileContexts = knnIndexShard.getAllEngineFileContexts(searcher.getIndexReader());
-        assertEquals(0, engineFileContexts.size());
-        searcher.close();
-
-        addKnnDoc(testIndexName, "1", testFieldName, new Float[] { 2.5F, 3.5F });
-
-        searcher = indexShard.acquireSearcher("test-hnsw-paths-2");
-        engineFileContexts = knnIndexShard.getAllEngineFileContexts(searcher.getIndexReader());
-        assertEquals(1, engineFileContexts.size());
-        List<String> paths = engineFileContexts.stream()
-            .map(KNNIndexShard.EngineFileContext::getVectorFileName)
-            .collect(Collectors.toList());
-        assertTrue(paths.get(0).contains("hnsw") || paths.get(0).contains("hnswc"));
-        searcher.close();
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> createKnnIndexMapping(testIndexName, testFieldName, dimensions, KNNEngine.NMSLIB)
+        );
     }
 
     @SneakyThrows

diff --git a/src/test/java/org/opensearch/knn/index/KNNMapperSearcherIT.java b/src/test/java/org/opensearch/knn/index/KNNMapperSearcherIT.java
@@ -354,7 +354,7 @@ public void testStoredFields_whenByteDataType_thenSucceed() {
      */
     @SneakyThrows
     public void testStoredFields_whenFloatDataType_thenSucceed() {
-        List<KNNEngine> enginesToTest = List.of(KNNEngine.NMSLIB, KNNEngine.FAISS, KNNEngine.LUCENE);
+        List<KNNEngine> enginesToTest = List.of(KNNEngine.FAISS, KNNEngine.LUCENE);
         float[] testVector = new float[] { -100.0f, 100.0f, 0f, 1f };
         String expectedResponse = String.format("\"fields\":{\"%s\":[[-100.0,100.0,0.0,1.0]]}}", FIELD_NAME);
         for (KNNEngine knnEngine : enginesToTest) {
@@ -379,7 +379,7 @@ public void testStoredFields_whenFloatDataType_thenSucceed() {
 
     @SneakyThrows
     public void testPutMappings_whenIndexAlreadyCreated_thenSuccess() {
-        List<KNNEngine> enginesToTest = List.of(KNNEngine.NMSLIB, KNNEngine.FAISS, KNNEngine.LUCENE);
+        List<KNNEngine> enginesToTest = List.of(KNNEngine.FAISS, KNNEngine.LUCENE);
         float[] testVector = new float[] { -100.0f, 100.0f, 0f, 1f };
         for (KNNEngine knnEngine : enginesToTest) {
             String indexName = INDEX_NAME + "_" + knnEngine.getName();

diff --git a/src/test/java/org/opensearch/knn/index/LuceneEngineIT.java b/src/test/java/org/opensearch/knn/index/LuceneEngineIT.java
@@ -47,7 +47,6 @@
 import static org.opensearch.knn.common.KNNConstants.MINIMUM_CONFIDENCE_INTERVAL;
 import static org.opensearch.knn.common.KNNConstants.MIN_SCORE;
 import static org.opensearch.knn.common.KNNConstants.NAME;
-import static org.opensearch.knn.common.KNNConstants.NMSLIB_NAME;
 import static org.opensearch.knn.common.KNNConstants.PARAMETERS;
 import static org.opensearch.knn.common.KNNConstants.VECTOR_DATA_TYPE_FIELD;
 
@@ -164,7 +163,7 @@ public void testQuery_multipleEngines() throws Exception {
             .startObject(KNNConstants.KNN_METHOD)
             .field(KNNConstants.NAME, METHOD_HNSW)
             .field(KNNConstants.METHOD_PARAMETER_SPACE_TYPE, nmslibSpaceType.getValue())
-            .field(KNNConstants.KNN_ENGINE, KNNEngine.NMSLIB.getName())
+            .field(KNNConstants.KNN_ENGINE, KNNEngine.FAISS.getName())
             .startObject(KNNConstants.PARAMETERS)
             .field(KNNConstants.METHOD_PARAMETER_M, M)
             .field(KNNConstants.METHOD_PARAMETER_EF_CONSTRUCTION, EF_CONSTRUCTION)
@@ -399,45 +398,6 @@ public void testQueryWithFilter_whenNonExistingFieldUsedInFilter_thenSuccessful(
         assertEquals(1, resultsQuery2.size());
     }
 
-    public void testQuery_filterWithNonLuceneEngine() throws Exception {
-        XContentBuilder builder = XContentFactory.jsonBuilder()
-            .startObject()
-            .startObject(PROPERTIES_FIELD_NAME)
-            .startObject(FIELD_NAME)
-            .field(TYPE_FIELD_NAME, KNN_VECTOR_TYPE)
-            .field(DIMENSION_FIELD_NAME, DIMENSION)
-            .startObject(KNNConstants.KNN_METHOD)
-            .field(KNNConstants.NAME, METHOD_HNSW)
-            .field(KNNConstants.METHOD_PARAMETER_SPACE_TYPE, SpaceType.L2.getValue())
-            .field(KNNConstants.KNN_ENGINE, NMSLIB_NAME)
-            .endObject()
-            .endObject()
-            .endObject()
-            .endObject();
-
-        String mapping = builder.toString();
-        createKnnIndex(INDEX_NAME, mapping);
-
-        addKnnDocWithAttributes(
-            DOC_ID,
-            new float[] { 6.0f, 7.9f, 3.1f },
-            ImmutableMap.of(COLOR_FIELD_NAME, "red", TASTE_FIELD_NAME, "sweet")
-        );
-        addKnnDocWithAttributes(DOC_ID_2, new float[] { 3.2f, 2.1f, 4.8f }, ImmutableMap.of(COLOR_FIELD_NAME, "green"));
-        addKnnDocWithAttributes(DOC_ID_3, new float[] { 4.1f, 5.0f, 7.1f }, ImmutableMap.of(COLOR_FIELD_NAME, "red"));
-
-        final float[] searchVector = { 6.0f, 6.0f, 5.6f };
-        int k = 5;
-        expectThrows(
-            ResponseException.class,
-            () -> searchKNNIndex(
-                INDEX_NAME,
-                new KNNQueryBuilder(FIELD_NAME, searchVector, k, QueryBuilders.termQuery(COLOR_FIELD_NAME, "red")),
-                k
-            )
-        );
-    }
-
     public void testIndexReopening() throws Exception {
         createKnnIndexMappingWithLuceneEngine(DIMENSION, SpaceType.L2, VectorDataType.FLOAT);