Fix Snapshot Out of Order Finalization Repo Corruption by original-brownbear · Pull Request #75362 · elastic/elasticsearch

original-brownbear · 2021-07-15T07:49:10Z

Fix up shard generations in SnapshotsInProgress during snapshot finalization (don't do it earlier because it's a really heavy computation and we have a ton of places where it would have to run).
Adjust finalization queue to be able to work with changing snapshot entries after they've been enqueued for finalisation
Still one remaining bug left after this (see TODO about leaking generations) that I don't feel confident in fixing for 7.13.4 due to the complexity of a fix and how minor the blob leak is (+ it's cleaned up just fine during snapshot deletes)

NOTE: this could probably be dried up a lot against other shard state machine logic but I did want to isolate this change as much as I could for easy backporting as well as to minimise risk as it's not a trivial change at all. By only running the generation fixing after finalizing and with the newly added tests I feel confident in this fix though.

Closes #75336

…-of-ordersnapshot-finalization

elasticmachine · 2021-07-15T13:45:20Z

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

original-brownbear · 2021-07-15T13:54:08Z

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

     */
-    private static ClusterState stateWithoutSnapshot(ClusterState state, Snapshot snapshot) {
+    private static ClusterState stateWithoutSuccessfulSnapshot(ClusterState state, Snapshot snapshot) {
+        // TODO: updating snapshots here leaks their outdated generation files, we should add logic to clean those up and enhance


The logic in this method would definitely benefit from a state tracking object like we have for the shard status update executor. In the interest of time and keeping it simple I went with this solution for now. Ideally, I'd like to resolve this todo and make the whole logic simpler to follow in 7.15 by finally refactoring SnapshotsInProgress into a form that is more appropriate for the logic around concurrent snapshots than to add yet another round of elaborate logic to nicely work around its shortcomings.

original-brownbear · 2021-07-15T14:02:47Z

Jenkins run elasticsearch-ci/part-1 (unrelated + known)

original-brownbear · 2021-07-15T14:18:06Z

Jenkins run elasticsearch-ci/part-2 (unrelated + known)

original-brownbear · 2021-07-15T15:20:27Z

Jenkins run elasticsearch-ci/part-2 (unrelated + known)

DaveCTurner · 2021-07-16T07:33:54Z

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

        final String bestGeneration = generations.getOrDefault(indexName, Collections.emptyMap()).get(shardId);
-        assert bestGeneration == null || activeGeneration == null || activeGeneration.equals(bestGeneration);
+        if ((bestGeneration == null || activeGeneration == null || activeGeneration.equals(bestGeneration)) == false) {
+            throw new AssertionFailedException("gnarf");


Changing this assertion failure into a runtime exception is, I think, cheating ;)

sorry needed a breakpoint there :D

…-of-ordersnapshot-finalization

fcofdez · 2021-07-16T09:39:59Z

server/src/internalClusterTest/java/org/elasticsearch/snapshots/ConcurrentSnapshotsIT.java

+        );
+    }
+
+    public void testOutOfOrderCloneFinalization() throws Exception {


I think the clone - clone case is missing?

Yea sorta, that's incredibly hard to reproduce because it all runs on the same master node (can't selectively block one just one shard easily with the current infra) and I couldn't find a quick way of adding the infrastructure for that test. I can try to find time for it later today but no guarantees I will be able to.

fcofdez

LGTM, but I agree that we should simplify this logic in the near future, it's becoming quite complex to follow. 👍

DaveCTurner

I left one question and one tiny nit, LGTM otherwise.

DaveCTurner · 2021-07-16T11:49:27Z

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

+                            ImmutableOpenMap.Builder<RepositoryShardId, ShardSnapshotStatus> updatedShardAssignments = null;
+                            for (ObjectObjectCursor<RepositoryShardId, ShardSnapshotStatus> finishedShardEntry : removedEntry.clones()) {
+                                final ShardSnapshotStatus shardState = finishedShardEntry.value;
+                                if (shardState.state() == ShardState.SUCCESS) {


tiny nit: we're inconsistent about this vs if (shardState.state() != ShardState.SUCCESS) { continue; } across the 4 branches

Ah I did this on purpose to not indent so deeply when there's more complicated logic in a branch ... maybe just confusing though I can change it if you want :)

DaveCTurner · 2021-07-16T11:56:25Z

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

+                                final RepositoryShardId repoShardId = finishedShardEntry.key;
+                                final IndexMetadata indexMeta = state.metadata().index(repoShardId.indexName());
+                                if (indexMeta == null) {
+                                    // The index name that finished cloning does not exist in the cluster state so it isn't relevant


I'm confused by this. If we deleted this index and then created another one with the same name then we'd be updating the entry with the wrong index UUID. I'm not sure this matters, but it certainly seems like it puts the entry in a strange state. Can we not use the shard ID from the actual entry?

Actually in this case org.elasticsearch.snapshots.SnapshotsService#maybeAddUpdatedAssignment will just not find the shard entry in the snapshot's map that still contains the old uuid because the ShardId won't be equal so that should be fine I think.

ugh ok even weirder :) Looking forward to this all getting cleaned up soon...

DaveCTurner

LGTM

original-brownbear · 2021-07-16T12:50:42Z

Thanks David + Francisco!

* Fix up shard generations in `SnapshotsInProgress` during snapshot finalization (don't do it earlier because it's a really heavy computation and we have a ton of places where it would have to run). * Adjust finalization queue to be able to work with changing snapshot entries after they've been enqueued for finalisation * Still one remaining bug left after this (see TODO about leaking generations) that I don't feel confident in fixing for `7.13.4` due to the complexity of a fix and how minor the blob leak is (+ it's cleaned up just fine during snapshot deletes) Closes #75336

* Fix up shard generations in `SnapshotsInProgress` during snapshot finalization (don't do it earlier because it's a really heavy computation and we have a ton of places where it would have to run). * Adjust finalization queue to be able to work with changing snapshot entries after they've been enqueued for finalisation * Still one remaining bug left after this (see TODO about leaking generations) that I don't feel confident in fixing for `7.13.4` due to the complexity of a fix and how minor the blob leak is (+ it's cleaned up just fine during snapshot deletes) Closes elastic#75336

original-brownbear added 5 commits July 14, 2021 10:42

reproducer

afb6347

step1

7447f4f

Merge remote-tracking branch 'elastic/master' into repro-multiple-out…

8f28772

…-of-ordersnapshot-finalization

some fixes

8a41742

fixes

38a3f1f

original-brownbear added >bug WIP :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Jul 15, 2021

elasticsearchmachine added the v8.0.0 label Jul 15, 2021

original-brownbear added 7 commits July 15, 2021 10:52

reproduce more nonsense

f798c5e

fixed

e72f8d7

fix more stuff

d807983

fixes

fd7112c

drier

e814193

cleanup

460e497

Merge remote-tracking branch 'elastic/master' into repro-multiple-out…

b841cdc

…-of-ordersnapshot-finalization

original-brownbear added v7.13.4 v7.14.0 and removed WIP labels Jul 15, 2021

original-brownbear marked this pull request as ready for review July 15, 2021 13:45

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

original-brownbear added the v7.15.0 label Jul 15, 2021

original-brownbear commented Jul 15, 2021

View reviewed changes

original-brownbear requested review from DaveCTurner, fcofdez and tlrx July 15, 2021 18:51

DaveCTurner reviewed Jul 16, 2021

View reviewed changes

Merge remote-tracking branch 'elastic/master' into repro-multiple-out…

e638b81

…-of-ordersnapshot-finalization

fcofdez reviewed Jul 16, 2021

View reviewed changes

:D

6be356c

original-brownbear requested review from DaveCTurner and fcofdez July 16, 2021 09:43

fcofdez approved these changes Jul 16, 2021

View reviewed changes

DaveCTurner reviewed Jul 16, 2021

View reviewed changes

original-brownbear requested a review from DaveCTurner July 16, 2021 12:34

DaveCTurner approved these changes Jul 16, 2021

View reviewed changes

original-brownbear merged commit 3bd2672 into elastic:master Jul 16, 2021

original-brownbear deleted the repro-multiple-out-of-ordersnapshot-finalization branch July 16, 2021 12:51

original-brownbear added the backport pending label Jul 16, 2021

This was referenced Jul 16, 2021

Fix Snapshot Out of Order Finalization Repo Corruption (#75362) #75415

Merged

Fix Snapshot Out of Order Finalization Repo Corruption (#75362) #75416

Merged

DaveCTurner mentioned this pull request Jul 16, 2021

AssertionError: Missing assignment for [[index-0][4]] during snapshot #75423

Closed

original-brownbear removed backport pending v7.13.4 labels Jul 19, 2021

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

original-brownbear restored the repro-multiple-out-of-ordersnapshot-finalization branch April 18, 2023 20:55

Conversation

original-brownbear commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Jul 15, 2021

Uh oh!

original-brownbear commented Jul 15, 2021

Uh oh!

original-brownbear commented Jul 15, 2021

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!

original-brownbear Jul 16, 2021 • 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!

fcofdez left a comment

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!

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!

original-brownbear commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

original-brownbear commented Jul 15, 2021 •

edited

Loading

original-brownbear Jul 16, 2021 •

edited

Loading