Support failure store in snapshot and restore operations #107281

gmarouli · 2024-04-09T16:20:56Z

In this PR we update the snapshot and restore implementations to include the failure store of a data stream when a data stream is being snapshot.

When a data stream is requested to be snapshot, then it implies all backing indices and failure stores.
When a data stream is being restored, then it implies all backing indices and failure stores (assuming the feature flag is also enabled)
When individual backing or failure store indices that have been removed are being restored they need to be manually added to the data stream. This functionality worked out of the box.

gmarouli · 2024-04-16T09:02:31Z

We believe the test failure is not related to the failure store changes but it's a result of the restoration of the match_only_text mapper. We created a test to capture the minimal conditions that cause this failure: #107514

elasticsearchmachine · 2024-04-16T10:26:57Z

Pinging @elastic/es-data-management (Team:Data Management)

nielsbauman

Awesome that you got this to work with relatively few changes! I only left some comments on the functionality itself for now. I'll have another look at the tests some other time.

server/src/main/java/org/elasticsearch/action/support/IndicesOptions.java

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

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

jbaiera

Some initial comments and questions. Looking good though, frustrating thing that cluster state problem.

jbaiera · 2024-04-23T07:13:57Z

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

@@ -555,7 +566,9 @@ private static Tuple<Map<String, DataStream>, Map<String, DataStreamAlias>> getD
        List<String> requestedDataStreams = filterIndices(
            snapshotInfo.dataStreams(),
            Stream.of(requestIndices, featureStateDataStreams).flatMap(Collection::stream).toArray(String[]::new),
-            IndicesOptions.fromOptions(true, true, true, true)
+            DataStream.isFailureStoreFeatureFlagEnabled()
+                ? IndicesOptions.lenientExpandIncludeFailureStore()


Looking at the filterIndices function - it doesn't seem to actually make any use of whether to include failure stores or not. Could we leave this unchanged?

Good point, I made it more explicit in the javadoc and only kept the IndicesOptions.lenientExpand().

jbaiera · 2024-04-23T07:22:00Z

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

+        return renameIndex(index, request, DataStream.BACKING_INDEX_PREFIX);
+    }
+
+    private static String renameFailureIndex(String index, RestoreSnapshotRequest request) {


This looks identical to the above method. I personally think it scans a little better while reading, but the duplication makes me think that it's probably better to merge all of it and control the logic based on which prefix is present (or not present for non-data streams)

I think we're also losing a check here in renameIndex() if we're restoring an index that should not be renamed. Not sure from just reviewing if that check is important right now, but it existed beforehand and makes me worry about its removal.

I thought I commented this already but I guess I didn't; the if-statements in renameBackingIndex and renameFailureIndex are already covered by the renameIndex above. So, after removing those if-statements, it probably makes even more sense to merge the methods.

Merged the methods. Let me know if it matches your expectations

jbaiera · 2024-04-23T07:28:48Z

server/src/test/java/org/elasticsearch/snapshots/RestoreServiceTests.java


        RestoreSnapshotRequest request = new RestoreSnapshotRequest();

        DataStream updateDataStream = RestoreService.updateDataStream(dataStream, metadata, request);

        assertEquals(dataStreamName, updateDataStream.getName());
-        assertEquals(Collections.singletonList(updatedIndex), updateDataStream.getIndices());
+        assertEquals(List.of(updatedBackingIndex), updateDataStream.getIndices());
+        assertEquals(List.of(updatedFailureIndex), updateDataStream.getFailureIndices());


Will these tests work if the feature flag is disabled? I recall that we enable the flag just about everywhere but I can't remember if there are still cases where the tests run without it on (like for releases).

I thought the unit tests always have the feature flags enabled, isn't this the case?

...reams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamsSnapshotsIT.java

gmarouli · 2024-05-08T08:32:43Z

@elasticmachine update branch

nielsbauman

LGTM, thanks Mary!

gmarouli · 2024-05-10T05:56:34Z

@elasticmachine update branch

jbaiera

LGTM! 🚀

gmarouli · 2024-05-13T09:44:40Z

@elasticmachine update branch

gmarouli added 3 commits April 9, 2024 19:16

Create the IT test to capture the failure store functionality

6083473

Implement failure store snapshot

2d94716

Implement failure store restore from snapshot

3593fa6

gmarouli added >non-issue :Data Management/Data streams Data streams and their lifecycles labels Apr 9, 2024

elasticsearchmachine added the v8.14.0 label Apr 9, 2024

gmarouli changed the title ~~Implement snapshot and restore for failure store data streams~~ Support failure store in snapshot and restore operations Apr 9, 2024

gmarouli added 5 commits April 10, 2024 16:18

Fix tests

10448be

Merge branch 'main' into failure-store-snapshots

b507f37

Merge with main & reformat

acfaf4a

Fix feature flag getter

a9275f3

Merge branch 'main' into failure-store-snapshots

17c3a29

This was referenced Apr 16, 2024

[Do not merge] Snapshot restore & match_only_text cause cluster state size differences #107512

Closed

Add test to demo conditions that cause cluster state size mismatch #107514

Closed

Merge branch 'main' into failure-store-snapshots

20a45d3

gmarouli mentioned this pull request Apr 16, 2024

[CI] RestoreTemplateWithMatchOnlyTextMapperIT test failing #107515

Closed

gmarouli added 2 commits April 16, 2024 12:15

Fix tests

03d28c5

Fix another test in the same way

616ab40

gmarouli marked this pull request as ready for review April 16, 2024 10:26

gmarouli requested a review from a team as a code owner April 16, 2024 10:26

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Apr 16, 2024

gmarouli requested review from jbaiera and nielsbauman April 16, 2024 10:28

nielsbauman reviewed Apr 17, 2024

View reviewed changes

elasticsearchmachine added v8.15.0 and removed v8.14.0 labels Apr 17, 2024

jbaiera requested changes Apr 23, 2024

View reviewed changes

elasticmachine and others added 6 commits May 8, 2024 09:32

Merge branch 'main' into failure-store-snapshots

11921be

Remove the extra record

80282ea

Do not add failure store options

bcfbfe7

Reinstate accidentally deleted test

ea22b15

Fix format

8158a2a

refactor rename name

8c73f40

gmarouli requested review from jbaiera and nielsbauman May 8, 2024 13:59

nielsbauman approved these changes May 9, 2024

View reviewed changes

Merge branch 'main' into failure-store-snapshots

89359a5

jbaiera approved these changes May 10, 2024

View reviewed changes

elasticmachine and others added 2 commits May 13, 2024 10:44

Merge branch 'main' into failure-store-snapshots

146fa0c

Merge branch 'main' into failure-store-snapshots

1df8078

gmarouli added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 14, 2024

gmarouli added 2 commits May 15, 2024 12:20

merge with main

771e5e7

Fix tests

cd0302c

elasticsearchmachine merged commit c16f7d2 into elastic:main May 15, 2024
15 checks passed

gmarouli deleted the failure-store-snapshots branch May 15, 2024 11:22

DaveCTurner mentioned this pull request May 16, 2024

[CI] ClientYamlTestSuiteIT test {yaml=tsdb/30_snapshot/Create a snapshot and then restore it} failing #108712

Closed

Support failure store in snapshot and restore operations #107281

Support failure store in snapshot and restore operations #107281

Uh oh!

Conversation

gmarouli commented Apr 9, 2024

Uh oh!

gmarouli commented Apr 16, 2024

Uh oh!

elasticsearchmachine commented Apr 16, 2024

Uh oh!

nielsbauman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbaiera left a comment

Choose a reason for hiding this comment

Uh oh!

jbaiera Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

gmarouli May 8, 2024

Choose a reason for hiding this comment

Uh oh!

jbaiera Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

nielsbauman Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

gmarouli May 8, 2024

Choose a reason for hiding this comment

Uh oh!

jbaiera Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

gmarouli May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmarouli commented May 8, 2024

Uh oh!

nielsbauman left a comment

Choose a reason for hiding this comment

Uh oh!

gmarouli commented May 10, 2024

Uh oh!

jbaiera left a comment

Choose a reason for hiding this comment

Uh oh!

gmarouli commented May 13, 2024

Uh oh!

Uh oh!

Uh oh!

gmarouli May 8, 2024 •

edited

Loading