From 2e611789f8206d75bd4ee12947d623a7a83074ef Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 10 Feb 2021 12:13:25 +0000 Subject: [PATCH 1/4] Reject remounting snapshot of a searchable snapshot Today you can mount a snapshot of a searchable snapshot index, but the shard fails to allocate since the underlying snapshot is devoid of content. Doing this is a mistake, you probably meant to restore the index instead, so this commit rejects it earlier with a more helpful message. Closes #68792 --- .../org/elasticsearch/index/store/Store.java | 2 +- .../SearchableSnapshotsIntegTests.java | 27 +++++++++++++++++++ ...ransportMountSearchableSnapshotAction.java | 16 +++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/store/Store.java b/server/src/main/java/org/elasticsearch/index/store/Store.java index c43dd162bebc1..66757375a636b 100644 --- a/server/src/main/java/org/elasticsearch/index/store/Store.java +++ b/server/src/main/java/org/elasticsearch/index/store/Store.java @@ -1360,7 +1360,7 @@ public void markStoreCorrupted(IOException exception) throws IOException { BytesRef ref = bytes.toBytesRef(); output.writeBytes(ref.bytes, ref.offset, ref.length); CodecUtil.writeFooter(output); - } catch (IOException ex) { + } catch (IOException | ImmutableDirectoryException ex) { logger.warn("Can't mark store as corrupted", ex); } directory().sync(Collections.singleton(corruptionMarkerName)); diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java index e81ad34dd6e5b..df4ef432adaa1 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java @@ -9,6 +9,7 @@ import com.carrotsearch.hppc.cursors.ObjectCursor; import org.apache.lucene.index.IndexFileNames; import org.apache.lucene.search.TotalHits; +import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; import org.elasticsearch.action.admin.cluster.snapshots.status.SnapshotIndexShardStatus; @@ -96,7 +97,9 @@ import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshots.getDataTiersPreference; import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshotsConstants.SNAPSHOT_DIRECTORY_FACTORY_KEY; import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshotsConstants.SNAPSHOT_RECOVERY_STATE_FACTORY_KEY; +import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.greaterThanOrEqualTo; @@ -1288,6 +1291,30 @@ public void testSnapshotOfSearchableSnapshotIncludesNoDataButCanBeRestored() thr logger.info("--> finished restoring snapshot-2"); assertTotalHits(restoredIndexName, originalAllHits, originalBarHits); + + final IllegalArgumentException remountException = expectThrows(IllegalArgumentException.class, () -> { + try { + mountSnapshot( + restoreRepositoryName, + snapshotTwo.getName(), + restoredIndexName, + randomAlphaOfLength(10).toLowerCase(Locale.ROOT), + Settings.EMPTY); + } catch (Exception e) { + final Throwable cause = ExceptionsHelper.unwrap(e, IllegalArgumentException.class); + throw cause == null ? e : cause; + } + }); + assertThat(remountException.getMessage(), allOf( + containsString("is a searchable snapshot backed by index"), + containsString(repositoryName), + containsString(snapshotOne.getName()), + containsString(indexName), + containsString(restoreRepositoryName), + containsString(snapshotTwo.getName()), + containsString(restoredIndexName), + containsString("cannot be mounted; did you mean to restore it instead?") + )); } private void assertTotalHits(String indexName, TotalHits originalAllHits, TotalHits originalBarHits) throws Exception { diff --git a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java index 3fa05b4b89c07..133e721a01022 100644 --- a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java +++ b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java @@ -43,6 +43,7 @@ import org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshotsConstants; import java.util.Arrays; +import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -179,6 +180,21 @@ protected void masterOperation( final String[] ignoreIndexSettings = Arrays.copyOf(request.ignoreIndexSettings(), request.ignoreIndexSettings().length + 1); ignoreIndexSettings[ignoreIndexSettings.length - 1] = IndexMetadata.SETTING_DATA_PATH; + final IndexMetadata indexMetadata = repository.getSnapshotIndexMetaData(repoData, snapshotId, indexId); + if (INDEX_STORE_TYPE_SETTING.get(indexMetadata.getSettings()).equals(SearchableSnapshotsConstants.SNAPSHOT_DIRECTORY_FACTORY_KEY)) { + throw new IllegalArgumentException(String.format(Locale.ROOT, + "index [%s] in snapshot [%s/%s:%s] is a searchable snapshot backed by index [%s] in snapshot [%s/%s:%s] " + + "and cannot be mounted; did you mean to restore it instead?", + indexName, + repoName, + repository.getMetadata().uuid(), + snapName, + SearchableSnapshots.SNAPSHOT_INDEX_NAME_SETTING.get(indexMetadata.getSettings()), + SearchableSnapshots.SNAPSHOT_REPOSITORY_NAME_SETTING.get(indexMetadata.getSettings()), + SearchableSnapshots.SNAPSHOT_REPOSITORY_UUID_SETTING.get(indexMetadata.getSettings()), + SearchableSnapshots.SNAPSHOT_SNAPSHOT_NAME_SETTING.get(indexMetadata.getSettings()))); + } + client.admin() .cluster() .restoreSnapshot( From 9fe96a315759e0745a08ee975481ae0bf3c45c3a Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 10 Feb 2021 12:17:52 +0000 Subject: [PATCH 2/4] Use utility method --- .../action/TransportMountSearchableSnapshotAction.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java index 133e721a01022..9bbe0a4e5d8bc 100644 --- a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java +++ b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java @@ -51,6 +51,7 @@ import static org.elasticsearch.index.IndexModule.INDEX_RECOVERY_TYPE_SETTING; import static org.elasticsearch.index.IndexModule.INDEX_STORE_TYPE_SETTING; import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshots.getDataTiersPreference; +import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshotsConstants.isSearchableSnapshotStore; /** * Action that mounts a snapshot as a searchable snapshot, by converting the mount request into a restore request with specific settings @@ -181,7 +182,7 @@ protected void masterOperation( ignoreIndexSettings[ignoreIndexSettings.length - 1] = IndexMetadata.SETTING_DATA_PATH; final IndexMetadata indexMetadata = repository.getSnapshotIndexMetaData(repoData, snapshotId, indexId); - if (INDEX_STORE_TYPE_SETTING.get(indexMetadata.getSettings()).equals(SearchableSnapshotsConstants.SNAPSHOT_DIRECTORY_FACTORY_KEY)) { + if (isSearchableSnapshotStore(indexMetadata.getSettings())) { throw new IllegalArgumentException(String.format(Locale.ROOT, "index [%s] in snapshot [%s/%s:%s] is a searchable snapshot backed by index [%s] in snapshot [%s/%s:%s] " + "and cannot be mounted; did you mean to restore it instead?", From 10c37464d285f81c556c3ad6ff24d95681ac013a Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 10 Feb 2021 12:20:02 +0000 Subject: [PATCH 3/4] Moar grammer --- .../searchablesnapshots/SearchableSnapshotsIntegTests.java | 2 +- .../action/TransportMountSearchableSnapshotAction.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java index df4ef432adaa1..6a8b7b2503d2e 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java @@ -1306,7 +1306,7 @@ public void testSnapshotOfSearchableSnapshotIncludesNoDataButCanBeRestored() thr } }); assertThat(remountException.getMessage(), allOf( - containsString("is a searchable snapshot backed by index"), + containsString("is a snapshot of a searchable snapshot index backed by index"), containsString(repositoryName), containsString(snapshotOne.getName()), containsString(indexName), diff --git a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java index 9bbe0a4e5d8bc..94e56a00067a7 100644 --- a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java +++ b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java @@ -184,8 +184,8 @@ protected void masterOperation( final IndexMetadata indexMetadata = repository.getSnapshotIndexMetaData(repoData, snapshotId, indexId); if (isSearchableSnapshotStore(indexMetadata.getSettings())) { throw new IllegalArgumentException(String.format(Locale.ROOT, - "index [%s] in snapshot [%s/%s:%s] is a searchable snapshot backed by index [%s] in snapshot [%s/%s:%s] " + - "and cannot be mounted; did you mean to restore it instead?", + "index [%s] in snapshot [%s/%s:%s] is a snapshot of a searchable snapshot index " + + "backed by index [%s] in snapshot [%s/%s:%s] and cannot be mounted; did you mean to restore it instead?", indexName, repoName, repository.getMetadata().uuid(), From 62ded49d22b4d901ed6b6ceaed57d75024241e48 Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 10 Feb 2021 12:20:23 +0000 Subject: [PATCH 4/4] Spotless --- .../SearchableSnapshotsIntegTests.java | 18 +++++++++++------- ...TransportMountSearchableSnapshotAction.java | 12 ++++++++---- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java index 6a8b7b2503d2e..9af516afdaf6f 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java @@ -1295,17 +1295,20 @@ public void testSnapshotOfSearchableSnapshotIncludesNoDataButCanBeRestored() thr final IllegalArgumentException remountException = expectThrows(IllegalArgumentException.class, () -> { try { mountSnapshot( - restoreRepositoryName, - snapshotTwo.getName(), - restoredIndexName, - randomAlphaOfLength(10).toLowerCase(Locale.ROOT), - Settings.EMPTY); + restoreRepositoryName, + snapshotTwo.getName(), + restoredIndexName, + randomAlphaOfLength(10).toLowerCase(Locale.ROOT), + Settings.EMPTY + ); } catch (Exception e) { final Throwable cause = ExceptionsHelper.unwrap(e, IllegalArgumentException.class); throw cause == null ? e : cause; } }); - assertThat(remountException.getMessage(), allOf( + assertThat( + remountException.getMessage(), + allOf( containsString("is a snapshot of a searchable snapshot index backed by index"), containsString(repositoryName), containsString(snapshotOne.getName()), @@ -1314,7 +1317,8 @@ public void testSnapshotOfSearchableSnapshotIncludesNoDataButCanBeRestored() thr containsString(snapshotTwo.getName()), containsString(restoredIndexName), containsString("cannot be mounted; did you mean to restore it instead?") - )); + ) + ); } private void assertTotalHits(String indexName, TotalHits originalAllHits, TotalHits originalBarHits) throws Exception { diff --git a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java index 94e56a00067a7..7f31b53466125 100644 --- a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java +++ b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/TransportMountSearchableSnapshotAction.java @@ -183,9 +183,11 @@ protected void masterOperation( final IndexMetadata indexMetadata = repository.getSnapshotIndexMetaData(repoData, snapshotId, indexId); if (isSearchableSnapshotStore(indexMetadata.getSettings())) { - throw new IllegalArgumentException(String.format(Locale.ROOT, - "index [%s] in snapshot [%s/%s:%s] is a snapshot of a searchable snapshot index " + - "backed by index [%s] in snapshot [%s/%s:%s] and cannot be mounted; did you mean to restore it instead?", + throw new IllegalArgumentException( + String.format( + Locale.ROOT, + "index [%s] in snapshot [%s/%s:%s] is a snapshot of a searchable snapshot index " + + "backed by index [%s] in snapshot [%s/%s:%s] and cannot be mounted; did you mean to restore it instead?", indexName, repoName, repository.getMetadata().uuid(), @@ -193,7 +195,9 @@ protected void masterOperation( SearchableSnapshots.SNAPSHOT_INDEX_NAME_SETTING.get(indexMetadata.getSettings()), SearchableSnapshots.SNAPSHOT_REPOSITORY_NAME_SETTING.get(indexMetadata.getSettings()), SearchableSnapshots.SNAPSHOT_REPOSITORY_UUID_SETTING.get(indexMetadata.getSettings()), - SearchableSnapshots.SNAPSHOT_SNAPSHOT_NAME_SETTING.get(indexMetadata.getSettings()))); + SearchableSnapshots.SNAPSHOT_SNAPSHOT_NAME_SETTING.get(indexMetadata.getSettings()) + ) + ); } client.admin()