Adjust snapshot index resolution behavior to be more intuitive #79670
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR removes the logic used to ensure backwards-compatible behavior in the Snapshot and Restore APIs and updates the tests to check the new logic. This is technically a breaking change, but significantly improves the usability of snapshots in the presence of system indices. The new behavior is as follows: - When creating a snapshot, system indices are not resolved based on the `indices` field in the request. System indices are included or excluded using the `include_global_state` or `feature_states` fields, defaulting to snapshotting all system indices. - When restoring a snapshot, system indices cannot be requested using the `indices` field in the request. System indices are included or excluded using the `include_global_state` or `feature_states` fields, defaulting to restoring no system indices.
gwbrown
added
>enhancement
>breaking
:Distributed Coordination/Snapshot/Restore
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
gwbrown
commented
Oct 22, 2021
| mockLogAppender.stop(); | ||
| Loggers.removeAppender(LogManager.getLogger("org.elasticsearch.deprecation.snapshots.RestoreService"), mockLogAppender); | ||
| IndexNotFoundException ex = expectThrows( | ||
| IndexNotFoundException.class, |
There was a problem hiding this comment.
I don't love that this results in an IndexNotFoundException vs something more informative - I'd like to improve this and welcome suggestions on what to do instead. Unfortunately, I think anything reasonable here is going to mean more involved changes to RestoreService.
williamrandolph
suggested changes
Nov 1, 2021
| @@ -237,7 +227,6 @@ public void testRestoreByFeature() { | |||
| // restore indices by feature, with only the regular index named explicitly | |||
There was a problem hiding this comment.
nitpick: this comment is now out of date
| @@ -484,7 +456,6 @@ public void testRestoreSystemIndicesAsGlobalStateWithEmptyListOfFeatureStates() | |||
|
|
|||
| // restore regular index, with global state and an empty list of feature states | |||
There was a problem hiding this comment.
nitpick: comment is out of date, since we're not specifying the single index anymore
| .setFeatureStates(new String[] { randomFrom("none", "NONE") }) | ||
| .get(); | ||
| assertThat(restoreResponse.getRestoreInfo().successfulShards(), equalTo(restoreResponse.getRestoreInfo().successfulShards())); | ||
| // GWB> What do we check here? |
There was a problem hiding this comment.
What if we delete the system index before restoring, then verify that the snapshot didn't recreate it?
There was a problem hiding this comment.
This test case was now a duplicate of another! So I deleted it.
| @@ -753,7 +710,6 @@ public void testNoneFeatureStateOnRestore() { | |||
|
|
|||
| // Restore the snapshot specifying the regular index and "none" for feature states | |||
There was a problem hiding this comment.
nitpick: update comment
| // Strip system indices out of the list of "available" indices - these should only come from feature states. | ||
| List<String> availableNonSystemIndices = new ArrayList<>(snapshotInfo.indices()); | ||
| { | ||
| Set<String> systemIndicesInSnapshot = snapshotInfo.featureStates() |
There was a problem hiding this comment.
This code might make sense as a method on the SnapshotInfo object, but I wouldn't insist on it.
This was referenced Nov 4, 2021
original-brownbear
approved these changes
Nov 30, 2021
gwbrown
added
the
auto-backport
💚 Backport successful
|
gwbrown
added a commit
to gwbrown/elasticsearch
that referenced
this pull request
Nov 30, 2021
…ic#79670) This PR removes the logic used to ensure backwards-compatible behavior in the Snapshot and Restore APIs and updates the tests to check the new logic. This is technically a breaking change, but significantly improves the usability of snapshots in the presence of system indices. The new behavior is as follows: - When creating a snapshot, system indices are not resolved based on the `indices` field in the request. System indices are included or excluded using the `include_global_state` or `feature_states` fields, defaulting to snapshotting all system indices. - When restoring a snapshot, system indices cannot be requested using the `indices` field in the request. System indices are included or excluded using the `include_global_state` or `feature_states` fields, defaulting to restoring no system indices.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Nov 30, 2021
… (#81180) This PR removes the logic used to ensure backwards-compatible behavior in the Snapshot and Restore APIs and updates the tests to check the new logic. This is technically a breaking change, but significantly improves the usability of snapshots in the presence of system indices. The new behavior is as follows: - When creating a snapshot, system indices are not resolved based on the `indices` field in the request. System indices are included or excluded using the `include_global_state` or `feature_states` fields, defaulting to snapshotting all system indices. - When restoring a snapshot, system indices cannot be requested using the `indices` field in the request. System indices are included or excluded using the `include_global_state` or `feature_states` fields, defaulting to restoring no system indices.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Dec 4, 2021
This commit changes SnapshotsService to throw an error when a system index is explicitly requested, rather than silently dropping it, and adds a test to verify that behavior. Note that *only* system indices requested by full name return an error, requesting e.g. `"indices": ".geo*"` (which would normally resolve to `.geoip_databases`) does not include its system index and does not error, but requesting `"indices": ".geoip_databases"` *will* return an error. Follow-up to #79670 --- This should have been part of the original change - I thought we had already done this, but @jrodewig pointed out to me that it silently dropped system indices instead.
gwbrown
added a commit
to gwbrown/elasticsearch
that referenced
this pull request
Dec 4, 2021
…1235) This commit changes SnapshotsService to throw an error when a system index is explicitly requested, rather than silently dropping it, and adds a test to verify that behavior. Note that *only* system indices requested by full name return an error, requesting e.g. `"indices": ".geo*"` (which would normally resolve to `.geoip_databases`) does not include its system index and does not error, but requesting `"indices": ".geoip_databases"` *will* return an error. Follow-up to elastic#79670 --- This should have been part of the original change - I thought we had already done this, but @jrodewig pointed out to me that it silently dropped system indices instead.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Dec 4, 2021
…81346) This commit changes SnapshotsService to throw an error when a system index is explicitly requested, rather than silently dropping it, and adds a test to verify that behavior. Note that *only* system indices requested by full name return an error, requesting e.g. `"indices": ".geo*"` (which would normally resolve to `.geoip_databases`) does not include its system index and does not error, but requesting `"indices": ".geoip_databases"` *will* return an error. Follow-up to #79670 --- This should have been part of the original change - I thought we had already done this, but @jrodewig pointed out to me that it silently dropped system indices instead.
jrodewig
added a commit
that referenced
this pull request
Dec 8, 2021
jrodewig
added a commit
to elastic/kibana
that referenced
this pull request
Dec 8, 2021
With elastic/elasticsearch#79670, you can only include system indices in a snapshot using feature states. This updates some references to taking a snapshots of the `.kibana*` system indices. Relates to elastic/elasticsearch#81226
elasticsearchmachine
pushed a commit
that referenced
this pull request
Dec 8, 2021
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 8, 2021
With elastic/elasticsearch#79670, you can only include system indices in a snapshot using feature states. This updates some references to taking a snapshots of the `.kibana*` system indices. Relates to elastic/elasticsearch#81226
kibanamachine
added a commit
to elastic/kibana
that referenced
this pull request
Dec 8, 2021
With elastic/elasticsearch#79670, you can only include system indices in a snapshot using feature states. This updates some references to taking a snapshots of the `.kibana*` system indices. Relates to elastic/elasticsearch#81226 Co-authored-by: James Rodewig <james.rodewig@elastic.co>
TinLe
pushed a commit
to TinLe/kibana
that referenced
this pull request
Dec 22, 2021
With elastic/elasticsearch#79670, you can only include system indices in a snapshot using feature states. This updates some references to taking a snapshots of the `.kibana*` system indices. Relates to elastic/elasticsearch#81226
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the logic used to ensure backwards-compatible behavior
in the Snapshot and Restore APIs and updates the tests to check the new
logic. This is technically a breaking change, but significantly improves
the usability of snapshots in the presence of system indices.
The new behavior is as follows:
indicesfield in the request. System indices are included or excludedusing the
include_global_stateorfeature_statesfields, defaultingto snapshotting all system indices.
the
indicesfield in the request. System indices are included orexcluded using the
include_global_stateorfeature_statesfields,defaulting to restoring no system indices.
Fixes #78320
It's worth noting that this is a breaking change, but we deprecated the behavior that won't work anymore a while ago, and I don't think anyone likes the current behavior - it's a mess because of shenanigans we had to pull to maintain BWC.