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

Add UnmodifiableOnRestore property to index.knn setting #2445

Merged
merged 4 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
9 changes: 8 additions & 1 deletion src/main/java/org/opensearch/knn/index/KNNSettings.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
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.settings.Setting.Property.UnmodifiableOnRestore;
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;
Expand Down Expand Up @@ -269,7 +270,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
96 changes: 96 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,96 @@
/*
* 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.settings.Settings;
import org.opensearch.common.xcontent.json.JsonXContent;
import org.opensearch.common.xcontent.support.XContentMapValues;
import org.opensearch.core.xcontent.XContentBuilder;
import org.opensearch.test.rest.OpenSearchRestTestCase;

import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.hasSize;

public class RestoreSnapshotIT extends OpenSearchRestTestCase {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please add a test for happy path. In both the tests I am seeing the unhappy path. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the base class KNNRestTestCase which is there for all the IT of k-NN plugin


private String index;
private String snapshot;
private String repository;

private void setupSnapshotRestore() throws Exception {
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);

index = "test-index";
snapshot = "snapshot-" + index;
repository = "repo";

// create index
Settings indexSettings = Settings.builder().put("number_of_shards", 1).put("number_of_replicas", 1).put("index.knn", true).build();
Copy link
Collaborator

@navneet1v navneet1v Jan 27, 2025

Choose a reason for hiding this comment

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

given that tests run on 1 single node, having a replica will keep the index yellow. Lets not have 1 replica.

I think lets use the index setting already present in the base classes

createIndex(index, indexSettings);

// create repo
Settings repoSettings = Settings.builder().put("compress", randomBoolean()).put("location", pathRepo).build();
registerRepository(repository, "fs", true, repoSettings);

// create snapshot
createSnapshot(repository, snapshot, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets move this to base class since we can start using this function for other tests.

}

public void testKnnSettingIsUnmodifiableOnRestore() throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion]
The way we create test names in k-NN in testABC_whenXYZ_then(Success/Failure).

Copy link
Collaborator

Choose a reason for hiding this comment

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

use @SneakyThrows rather than exception in message signature

setupSnapshotRestore();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we are not using the @setup function here?


// invalid restore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there are any existing helper methods in OpenSearchRestTestCase/upstream classes for restore snapshot that take in setting updates and throws back validation errors

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"));
}

public void testKnnSettingCannotBeIgnoredDuringRestore() throws Exception {
setupSnapshotRestore();

// 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"));
}
}
Loading