Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport Main] Add UnmodifiableOnRestore property to index.knn setting #2476

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the changelog ?

* 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
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/org/opensearch/knn/index/KNNSettings.java
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,13 @@ public class KNNSettings {
/**
* This setting identifies KNN index.
*/
public static final Setting<Boolean> IS_KNN_INDEX_SETTING = Setting.boolSetting(KNN_INDEX, false, IndexScope, Final);
public static final Setting<Boolean> 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.
Expand Down
108 changes: 108 additions & 0 deletions src/test/java/org/opensearch/knn/index/RestoreSnapshotIT.java
Original file line number Diff line number Diff line change
@@ -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"));
}
}
28 changes: 28 additions & 0 deletions src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -2370,4 +2373,29 @@ protected boolean isRemoteIndexBuildSupported(final Optional<String> 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<String, Object> clusterSettings = entityAsMap(clusterSettingsResponse);

@SuppressWarnings("unchecked")
List<String> pathRepos = (List<String>) 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);
}
}
Loading