Skip to content

Conversation

@anntians
Copy link
Contributor

Description

This PR adds a new Setting property UnmodifiableOnRestore to prevent customers from updating certain settings on restore snapshot. The motivation behind this change is to prevent customers from updating index.knn setting when restoring a snapshot. The PR is related to another PR (opensearch-project/k-NN#2348) which fixes a bug by making index.knn setting immutable after index creation. However, that PR doesn't cover the case for snapshots, so this PR adds Setting property UnmodifiableOnRestore so that index.knn will be immutable when restoring an index from a snapshot.

Related Issues

Resolves Issue 2334 in KNN repository: opensearch-project/k-NN#2334
#17019

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…g settings on restore snapshot (opensearch-project#16957)

* Add index.knn setting to list of unmodifiable settings when restore snapshot

Signed-off-by: AnnTian Shao <[email protected]>

* Add index.knn setting to list of unmodifiable settings when restore snapshot

Signed-off-by: AnnTian Shao <[email protected]>

* Add new Setting property UnmodifiableOnRestore to mark setting as immutable on restore snapshot

Signed-off-by: AnnTian Shao <[email protected]>

* Add tests for new Setting property UnmodifiableOnRestore

Signed-off-by: AnnTian Shao <[email protected]>

* fixes and added more tests for new setting property UnmodifiableOnRestore

Signed-off-by: AnnTian Shao <[email protected]>

* fix test failures

Signed-off-by: AnnTian Shao <[email protected]>

* Revert "fix test failures"

This reverts commit 252100c.

Signed-off-by: AnnTian Shao <[email protected]>

* fixes by adding back USER_UNMODIFIABLE_SETTINGS for settings without Setting implementation

Signed-off-by: AnnTian Shao <[email protected]>

* testing CI config for registering plugin settings

Signed-off-by: AnnTian Shao <[email protected]>

* Revert "testing CI config for registering plugin settings"

This reverts commit 9ebab5a.

Signed-off-by: AnnTian Shao <[email protected]>

* Add UnmodifiableOnRestore only to unmodifiable settings that are already registered, will raise separate PR for settings not yet registered. Add validation check in Setting.java. Add UnmodifiableOnRestore settings cannot be removed on restore

Signed-off-by: AnnTian Shao <[email protected]>

* fixes and move tests to RestoreSnapshotIT

Signed-off-by: AnnTian Shao <[email protected]>

* Add back testInvalidRestoreRequestScenarios test method

Signed-off-by: AnnTian Shao <[email protected]>

---------

Signed-off-by: AnnTian Shao <[email protected]>
Signed-off-by: Tommy Shao <[email protected]>
Co-authored-by: AnnTian Shao <[email protected]>
(cherry picked from commit 822f80c)
@anntians anntians changed the title Added new Setting property UnmodifiableOnRestore to prevent updating settings on restore snapshot [Backport 2.x] Added new Setting property UnmodifiableOnRestore to prevent updating settings on restore snapshot Jan 24, 2025
@github-actions
Copy link
Contributor

❌ Gradle check result for 8f1fd2a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@sohami
Copy link
Contributor

sohami commented Jan 24, 2025

❌ Gradle check result for 8f1fd2a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Flaky Test: #16088

@github-actions
Copy link
Contributor

❕ Gradle check result for 8f1fd2a: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@codecov
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 71.91%. Comparing base (a56a08e) to head (8f1fd2a).
Report is 3 commits behind head on 2.x.

Files with missing lines Patch % Lines
.../java/org/opensearch/snapshots/RestoreService.java 42.85% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                2.x   #17116      +/-   ##
============================================
+ Coverage     71.90%   71.91%   +0.01%     
+ Complexity    65776    65751      -25     
============================================
  Files          5325     5325              
  Lines        306417   306448      +31     
  Branches      44681    44693      +12     
============================================
+ Hits         220335   220393      +58     
+ Misses        67715    67644      -71     
- Partials      18367    18411      +44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sohami
Copy link
Contributor

sohami commented Jan 24, 2025

❕ Gradle check result for 8f1fd2a: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Known Issue: #14289

@sohami sohami merged commit 99e0c57 into opensearch-project:2.x Jan 24, 2025
46 checks passed
@anntians anntians deleted the backport/backport-16957-to-2.x branch January 24, 2025 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants