Prevent searchable snapshots indices to be shrunk/split by tlrx · Pull Request #75227 · elastic/elasticsearch

tlrx · 2021-07-12T12:10:34Z

Today if we try to shrink or to split a searchable snapshot index using the Resize API a new index will be created but can't be assigned, and even if it was assigned it won't work as the number of shards can't be changed and must always match the number of shards from the snapshot.

This pull request adds some verification to prevent a snapshot backed indices to be resized and if an attempt is made, throw a better error message.

Note that cloning is supported since #56595 and in this pull request we make sure that it is only used to convert the searchable snapshot index back to a regular index.

Relates #74977 (comment)

elasticmachine · 2021-07-12T12:10:37Z

Pinging @elastic/es-distributed (Team:Distributed)

original-brownbear

LGTM, just a few optional nits :)

original-brownbear · 2021-07-13T08:54:40Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java

    static void validateCloneIndex(ClusterState state, String sourceIndex, String targetIndexName, Settings targetIndexSettings) {
        IndexMetadata sourceMetadata = validateResize(state, sourceIndex, targetIndexName, targetIndexSettings);
+        if ("snapshot".equals(INDEX_STORE_TYPE_SETTING.get(sourceMetadata.getSettings()))) {
+            for (Setting<?> nonCloneableSetting : List.of(INDEX_STORE_TYPE_SETTING, INDEX_RECOVERY_TYPE_SETTING)) {


NIT: maybe use Arrays.asList() or some other thing that's JDK8 compatible to make the backport cleaner? :)

original-brownbear · 2021-07-13T08:56:38Z

...st/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsResizeIntegTests.java

+            IllegalArgumentException.class,
+            () -> client().admin().indices().prepareResizeIndex("mounted-index", "cloned-index").setResizeType(ResizeType.CLONE).get()
+        );
+        assertThat(


nit: just assertEquals? (here and in other similar spots)

I do prefer the error messages returned by assertThat than assertEquals, if that's ok.

Ah sure :) didn't even know they were different :D

I'm with Tanguy on this one, I find the argument order for assertEquals(expected, actual) confusing since it's the opposite to assertThat(actual, expectation).

original-brownbear · 2021-07-13T08:57:48Z

...st/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsResizeIntegTests.java

+@ESIntegTestCase.ClusterScope(numDataNodes = 1)
+public class SearchableSnapshotsResizeIntegTests extends BaseFrozenSearchableSnapshotsIntegTestCase {
+
+    @Before


No need for this annotation I think, this is automatically called since the annotation is on the parent?

At least on my Intellij 2021.1.3 this is required for integ tests for JUnit to correctly set up the internal test cluster.

original-brownbear · 2021-07-13T08:57:53Z

...st/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsResizeIntegTests.java

+        ensureGreen("mounted-index");
+    }
+
+    @After


No need for this annotation I think, this is automatically called since the annotation is on the parent?

original-brownbear · 2021-07-13T08:58:40Z

...st/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsResizeIntegTests.java

+                .prepareResizeIndex("mounted-index", "shrunk-index")
+                .setResizeType(ResizeType.SHRINK)
+                .setSettings(
+                    Settings.builder()


NIT: I added org.elasticsearch.snapshots.AbstractSnapshotIntegTestCase#indexSettingsNoReplicas at one point for this because it's such a common kind of setting in tests :)

Forgot about this one, thanks!

original-brownbear · 2021-07-13T08:58:44Z

...st/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsResizeIntegTests.java

+                .setResizeType(ResizeType.SPLIT)
+                .setSettings(
+                    Settings.builder()
+                        .put(INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 0)


NIT: I added org.elasticsearch.snapshots.AbstractSnapshotIntegTestCase#indexSettingsNoReplicas at one point for this because it's such a common kind of setting in tests :)

DaveCTurner

I left a suggestion about avoiding repeated string literals, otherwise LGTM2.

DaveCTurner · 2021-07-13T09:20:29Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java

     */
    static List<String> validateShrinkIndex(ClusterState state, String sourceIndex, String targetIndexName, Settings targetIndexSettings) {
        IndexMetadata sourceMetadata = validateResize(state, sourceIndex, targetIndexName, targetIndexSettings);
+        if ("snapshot".equals(INDEX_STORE_TYPE_SETTING.get(sourceMetadata.getSettings()))) {


Could we move SearchableSnapshotsConstants#SNAPSHOT_DIRECTORY_FACTORY_KEY and SearchableSnapshotsConstants#isSearchableSnapshotStore to server rather than just using the literal "snapshot" throughout? Either that or add an independent constant in server and then add a static constructor to o.e.x.c.searchablesnapshots.SearchableSnapshotsConstants to assert that the constant in server is equal to SNAPSHOT_DIRECTORY_FACTORY_KEY?

Yes, I'll do something like this in a follow-up (it touches places outside of this pull request too)

DaveCTurner

A follow-up is fine with me; LGTM

tlrx · 2021-07-13T10:33:35Z

Thanks Armin and David!

Today if we try to shrink or to split a searchable snapshot index using the Resize API a new index will be created but can't be assigned, and even if it was assigned it won't work as the number of shards can't be changed and must always match the number of shards from the snapshot. This commit adds some verification to prevent a snapshot backed indices to be resized and if an attempt is made, throw a better error message. Note that cloning is supported since elastic#56595 and in this change we make sure that it is only used to convert the searchable snapshot index back to a regular index. Relates elastic#74977 (comment)

) Today if we try to shrink or to split a searchable snapshot index using the Resize API a new index will be created but can't be assigned, and even if it was assigned it won't work as the number of shards can't be changed and must always match the number of shards from the snapshot. This commit adds some verification to prevent a snapshot backed indices to be resized and if an attempt is made, throw a better error message. Note that cloning is supported since #56595 and in this change we make sure that it is only used to convert the searchable snapshot index back to a regular index. Relates #74977 (comment)

Prevent searchable snapshots indices to be shrunk/split/clone

3a4b958

tlrx added >enhancement :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.15.0 labels Jul 12, 2021

elasticmachine added the Team:Distributed Meta label for distributed team. label Jul 12, 2021

tlrx mentioned this pull request Jul 12, 2021

Introduce searchable snapshots index setting for cascade deletion of snapshots #74977

Merged

tlrx changed the title ~~Prevent searchable snapshots indices to be shrunk/split/clone~~ Prevent searchable snapshots indices to be shrunk/split Jul 12, 2021

tlrx added 5 commits July 12, 2021 15:51

handle cloning

c5b050d

Merge branch 'master' into forbid-resize-actions-for-ss

66196ca

fix checkstyle

95a2f5c

Merge branch 'master' into forbid-resize-actions-for-ss

c3a2997

index docs

022627b

tlrx requested review from DaveCTurner and original-brownbear July 13, 2021 08:36

original-brownbear approved these changes Jul 13, 2021

View reviewed changes

feedback

c0be556

DaveCTurner reviewed Jul 13, 2021

View reviewed changes

DaveCTurner approved these changes Jul 13, 2021

View reviewed changes

tlrx merged commit 4410d76 into elastic:master Jul 13, 2021

tlrx deleted the forbid-resize-actions-for-ss branch July 13, 2021 10:33

tlrx mentioned this pull request Jul 13, 2021

[7.x] Prevent searchable snapshots indices to be shrunk/split #75289

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

tlrx commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 12, 2021

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

tlrx commented Jul 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tlrx commented Jul 12, 2021 •

edited

Loading