Prevent snapshots to be mounted as system indices #61517
Conversation
tlrx
added
>bug
:Distributed/Snapshot/Restore
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
| @@ -132,6 +133,11 @@ protected void masterOperation( | |||
| ) { | |||
| SearchableSnapshots.ensureValidLicense(licenseState); | |||
|
|
|||
| final String mountedIndexName = request.mountedIndexName(); | |||
| if (mountedIndexName.charAt(0) == '.') { | |||
There was a problem hiding this comment.
I think it would be best to pull in the SystemIndices class since we're in a transport action. We can bind the instance in Node and make it available for guice to inject. Then a simple modification to the the SystemIndices class could be made that adds a method to check the name (currently there is a method that takes an Index object but only looks at the name) against the defined set of system indices.
The primary reason for this is users could have data indices in 7.x that start with a . so we shouldn't break that for them restrict them from being able to use this feature on that data.
There was a problem hiding this comment.
There are also hidden indices that start with a dot (e.g. .watcher-history*), which there's no reason to prevent being backed by a searchable snapshot. Using the SystemIndices class would avoid that issue as well.
There was a problem hiding this comment.
I merged #61540 which adds the method in SystemIndices to check a string name without needing the Index object.
There was a problem hiding this comment.
Thanks everybody. I'm waiting for #60522 to be merged and I'll update this PR.
|
As a drive-by comment, I'm slightly concerned that this will trip up users that are trying to mount data stream indices manually as searchable snapshots. Data stream backing indices start with a Maybe it's not a big deal, but at the very least we should document this somewhere? |
| @@ -132,6 +136,11 @@ protected void masterOperation( | |||
| ) { | |||
| SearchableSnapshots.ensureValidLicense(licenseState); | |||
|
|
|||
| final String mountedIndexName = request.mountedIndexName(); | |||
| if (systemIndices.isSystemIndex(mountedIndexName)) { | |||
There was a problem hiding this comment.
This looks better than the previous approach 👍
| .setSettings(Settings.builder().put(IndexMetadata.SETTING_INDEX_HIDDEN, isHidden).build()) | ||
| ); | ||
|
|
||
| final int nbDocs = scaledRandomIntBetween(0, 100); |
There was a problem hiding this comment.
nit: we could use createAndPopulateIndex instead of manually creating and populating the index?
There was a problem hiding this comment.
Yes we can - I pushed 3e173cb
|
@elasticmachine run elasticsearch-ci/packaging-sample-windows |
1 similar comment
|
@elasticmachine run elasticsearch-ci/packaging-sample-windows |
|
Thanks all! |
System indices can be snapshotted and are therefore potential candidates to be mounted as searchable snapshot indices. As of today nothing prevents a snapshot to be mounted under an index name starting with . and this can lead to conflicting situations because searchable snapshot indices are read-only and Elasticsearch expects some system indices to be writable; because searchable snapshot indices will soon use an internal system index (#60522) to speed up recoveries and we should prevent the system index to be itself a searchable snapshot index (leading to some deadlock situation for recovery). This commit introduces a changes to prevent snapshots to be mounted as a system index.
System and hidden indices can be snapshotted and are therefore potential candidates to be mounted as searchable snapshot indices. As of today nothing prevents a snapshot to be mounted under an index name starting with
.and this can lead to conflicting situations because searchable snapshot indices are read-only and Elasticsearch expects some system indices to be writable; because searchable snapshot indices will soon use an internal system index (#60522) to speed up recoveries and we should prevent the system index to be itself a searchable snapshot index (leading to some deadlock situation for recovery).This pull request introduces a changes to prevent snapshots to be mounted under a system index.