Enable Fully Concurrent Snapshot Operations by original-brownbear · Pull Request #56911 · elastic/elasticsearch

original-brownbear · 2020-05-18T13:43:58Z

Enables fully concurrent snapshot operations:

Snapshot create- and delete operations can be started in any order
Delete operations wait for snapshot finalization to finish, are batched as much as possible to improve efficiency and once enqueued in the cluster state prevent new snapshots from starting on data nodes until executed
- We could be even more concurrent here in a follow-up by interleaving deletes and snapshots on a per-shard level. I decided not to do this for now since it seemed not worth the added complexity yet. Due to batching+deduplicating of deletes the pain of having a delete stuck behind a long -running snapshot seemed manageable (dropped client connections + resulting retries don't cause issues due to deduplication of delete jobs, batching of deletes allows enqueuing more and more deletes even if a snapshot blocks for a long time that will all be executed in essentially constant time (due to bulk snapshot deletion, deleting multiple snapshots is mostly about as fast as deleting a single one))
Snapshot creation is completely concurrent across shards, but per shard snapshots are linearized for each repository as are snapshot finalizations

See updated JavaDoc and added test cases for more details and illustration on the functionality.

Some notes:

The queuing of snapshot finalizations and deletes and the related locking/synchronization is a little awkward in this version but can be much simplified with some refactoring. The problem is that snapshot finalizations resolve their listeners on the SNAPSHOT pool while deletes resolve the listener on the master update thread. With some refactoring both of these could be moved to the master update thread, effectively removing the need for any synchronization around the SnapshotService state. I didn't do this refactoring here because it's a fairly large change and not necessary for the functionality but plan to do so in a follow-up.

This change allows for completely removing any trickery around synchronizing deletes and snapshots from SLM and 100% does away with SLM errors from collisions between deletes and snapshots.

Snapshotting a single index in parallel to a long running full backup will execute without having to wait for the long running backup as required by the ILM/SLM use case of moving indices to "snapshot tier". Finalizations are linearized but ordered according to which snapshot saw all of its shards complete first

elasticmachine · 2020-05-18T13:44:00Z

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

Implements fully concurrent snapshot operations. See documentation changes to snapshot package level JavaDoc for details.

original-brownbear · 2020-06-15T09:28:01Z

...er/src/internalClusterTest/java/org/elasticsearch/snapshots/MinThreadsSnapshotRestoreIT.java

- * in the thread pool (for example, tests that use the mock repository that
- * block on master).
- */
-public class MinThreadsSnapshotRestoreIT extends AbstractSnapshotIntegTestCase {


All scenarios covered by this test become obsolete. The actual premise of this test (checking that we don't dead-lock from blocked threads) is covered by the fact that SnapshotResiliencyTests work for the most part anyway.

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

…pshots

original-brownbear · 2020-07-09T07:05:34Z

I worked through this list again:

add a check to snapshot finalization that makes sure that the index-N blob that a finalization is based on actually exists to avoid a class of possible concurrent repo access bugs

Done in #59141
IMO this provides about as much safety around concurrent repository access (from multiple mounts or otherwise) as we will get. On consistent blob stores I think it fixes all situations. On S3 it makes it incredibly unlikely that concurrent mounts would cause trouble as well. Keep in mind, even if you do run concurrent writes for concurrent repo mounts to the same location, the repo will get marked as corrupted in one of the two mounts as soon as an index-N blob is missing unexpectedly. Since the logic around writing new index-N blobs involves as CS update before and after writing and index-N blob, these writes are spaced out a little even across multiple repositories and approximately serialized via the cluster state update thread, making it even less likely to run into trouble on S3.

add a limit to the number of concurrently snapshotting shards in place of the limit on concurrent repository operations to limit memory use. Limiting the concurrent operations could be trouble for ILM that might start running snapshots for a large number of indices expecting mechanics similar to those of moving shards. (will do in this PR)

I though about this some more. I don't think this is necessary. I think we can simply deduplicate ShardId and Index instances when deserializing SnapshotsInProgress#Entry instances to lower memory use for SnapshotsInProgress but beyond that I'm not sure how much of a concern this really is. Maybe we should just put an upper bound of 1000 concurrent operations by default (as that is pretty much the upper bound for a reasonable number of snapshots per repo that we've been communicating in the past) to limit things in case of broken SLM configurations and that's it for this?

In case a repository does not yet use uuid-shard generations in all its snapshots we want to queue up snapshot operations (instead of the more elaborate but in this case unsafe concurrency) to not create inconsistent SLM behavior across versions.

I think this is actually not necessary in hindsight :) I pushed f5a44d4 to force the use of shard generations even if it's numeric ones for queued up shard level snapshots. Meaning, even if your shard level generations are numeric queued up shard level snapshots won't do listing to find the next generation and hence are safe even on S3. This change will need to be a adapted a little for the backport to 7.x because there are some corner cases here in mixed version clusters (literally only with 7.7.0 I think) where we want to filter out numeric generations in the blob store still but in mixed version clusters with 7.8.0+ it's safe as far as I can see. Certainly, it seems to me that this is the safer approach compared to adding a different state machine for queuing operations while the repository is at an older version.

=> IMO this is ready for another review.

ywelsch

LGTM. Thanks for tackling this challenge, and good luck with the backport.

…pshots

tlrx

LGTM This is great work, thanks Armin! I carefully reviewed most parts but I must admit that I lightly reviewed the "repository loop" part.

I deeply apologize for the time it took me to review this PR. The reviewing experience was not great for me due to the amount of code changes, I think using a dedicated branch here would have made sense.

tlrx · 2020-07-10T07:19:01Z

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

+    private static String startDataNodeWithLargeSnapshotPool() {
+        return internalCluster().startDataOnlyNode(LARGE_SNAPSHOT_POOL_SETTINGS);
+    }
+    public void testSnapshotRunsAfterInProgressDelete() throws Exception {


nit: add extra line

tlrx · 2020-07-10T07:25:37Z

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

+        assertThat(secondSnapshotResponse.isDone(), is(false));
+
+        unblockNode(repoName, dataNode);
+        assertThat(firstSnapshotResponse.get().getSnapshotInfo().state(), is(SnapshotState.FAILED));


Can we check that the 1st snapshot failed because it was aborted?

tlrx · 2020-07-10T07:41:41Z

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

+        ensureStableCluster(3);
+
+        awaitNoMoreRunningOperations();
+        expectThrows(RepositoryException.class, deleteFuture::actionGet);


Is there a meaningful error message we could check here?

Not really, it's just "failed to update repository". It's all in the cause here, but that's also just a JSON parse failure.

tlrx · 2020-07-10T07:52:35Z

server/src/main/java/org/elasticsearch/cluster/SnapshotDeletionsInProgress.java

+        this(in.readList(Entry::new));
+    }
+
+    private static boolean assertConsistency(List<Entry> entries) {


nit: assertConsistency -> assertNoConcurrentDeletionsForSameRepository() ?

tlrx · 2020-07-10T08:00:31Z

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

+            try {
+                assert assertConsistentEntries(entries);
+            } catch (AssertionError e) {
+                throw e;


I'm not sure to understand why we catch and rethrow here

So I could put a debug breakpoint there :D Thanks for spotting!

tlrx · 2020-07-10T08:10:18Z

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

+    private final OngoingRepositoryOperations repositoryOperations = new OngoingRepositoryOperations();
+
+    /**
+     * Setting that specifies the maximum number of allow concurrent snapshot create and delete operations in the


allow -> allowed

tlrx · 2020-07-10T08:13:17Z

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

-                    currentState.custom(SnapshotDeletionsInProgress.TYPE, SnapshotDeletionsInProgress.EMPTY);
-                if (deletionsInProgress.hasDeletionsInProgress()) {
+                        currentState.custom(SnapshotDeletionsInProgress.TYPE, SnapshotDeletionsInProgress.EMPTY);
+                if (deletionsInProgress.hasDeletionsInProgress() && concurrentOperationsAllowed == false) {
                    throw new ConcurrentSnapshotExecutionException(repositoryName, snapshotName,
                        "cannot snapshot while a snapshot deletion is in-progress in [" + deletionsInProgress + "]");


When backporting, we could maybe indicate in the error message that concurrent snapshot/deletions are possible in version 7.9?

👍 Right, I put down a note for that when doing the back-port work.

tlrx · 2020-07-10T08:18:16Z

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

                    throw new ConcurrentSnapshotExecutionException(repositoryName, snapshotName, " a snapshot is already running");
                }
+                ensureBelowConcurrencyLimit(repositoryName, snapshotName, snapshots, deletionsInProgress);


What happen if multiple snapshot operations are started but the maxConcurrentOperations settings is updated to a value lower than the current number of concurrent ops? Would it still be possible to enque more ops?

What happen if multiple snapshot operations are started but the maxConcurrentOperations settings is updated to a value lower than the current number of concurrent ops?

Existing ops won't be affected but you can't start new ones.

Would it still be possible to enque more ops?
No then the ops have to come down to below the new limit first before we can enqueue more.

…pshots

original-brownbear · 2020-07-10T13:18:55Z

Thanks so much @ywelsch @tlrx !!!

Enables fully concurrent snapshot operations: * Snapshot create- and delete operations can be started in any order * Delete operations wait for snapshot finalization to finish, are batched as much as possible to improve efficiency and once enqueued in the cluster state prevent new snapshots from starting on data nodes until executed * We could be even more concurrent here in a follow-up by interleaving deletes and snapshots on a per-shard level. I decided not to do this for now since it seemed not worth the added complexity yet. Due to batching+deduplicating of deletes the pain of having a delete stuck behind a long -running snapshot seemed manageable (dropped client connections + resulting retries don't cause issues due to deduplication of delete jobs, batching of deletes allows enqueuing more and more deletes even if a snapshot blocks for a long time that will all be executed in essentially constant time (due to bulk snapshot deletion, deleting multiple snapshots is mostly about as fast as deleting a single one)) * Snapshot creation is completely concurrent across shards, but per shard snapshots are linearized for each repository as are snapshot finalizations See updated JavaDoc and added test cases for more details and illustration on the functionality. Some notes: The queuing of snapshot finalizations and deletes and the related locking/synchronization is a little awkward in this version but can be much simplified with some refactoring. The problem is that snapshot finalizations resolve their listeners on the `SNAPSHOT` pool while deletes resolve the listener on the master update thread. With some refactoring both of these could be moved to the master update thread, effectively removing the need for any synchronization around the `SnapshotService` state. I didn't do this refactoring here because it's a fairly large change and not necessary for the functionality but plan to do so in a follow-up. This change allows for completely removing any trickery around synchronizing deletes and snapshots from SLM and 100% does away with SLM errors from collisions between deletes and snapshots. Snapshotting a single index in parallel to a long running full backup will execute without having to wait for the long running backup as required by the ILM/SLM use case of moving indices to "snapshot tier". Finalizations are linearized but ordered according to which snapshot saw all of its shards complete first

There were two subtle bugs here from backporting #56911 to 7.x. 1. We passed `null` for the `shards` map which isn't nullable any longer when creating `SnapshotsInProgress.Entry`, fixed by just passing an empty map like the `null` handling did in the past. 2. The removal of a failed `INIT` state snapshot from the cluster state tried removing it from the finalization loop (the set of repository names that are currently finalizing). This will trip an assertion since the snapshot failed before its repository was put into the set. I made the logic ignore the set in case we remove a failed `INIT` state snapshot to restore the old logic to exactly as it was before the concurrent snapshots backport to be on the safe side here. Also, added tests that explicitly call the old code paths because as can be seen from initially missing this, the BwC tests will only run in the configuration new version master, old version nodes ever so often and having a deterministic test for the old state machine seems the safest bet here. Closes #59986

original-brownbear added >enhancement WIP :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels May 18, 2020

elasticmachine added the Team:Distributed Meta label for distributed team. label May 18, 2020

Fully Concurrent Snapshots

346f306

Implements fully concurrent snapshot operations. See documentation changes to snapshot package level JavaDoc for details.

original-brownbear commented Jun 15, 2020

View reviewed changes

original-brownbear added 5 commits June 15, 2020 11:37

cleanup todo

0fffc10

docs

26e4bce

cleanup

7076a0f

doc

7cd1c4e

reduce noise

a9b894c

original-brownbear commented Jun 15, 2020

View reviewed changes

server/src/main/java/org/elasticsearch/snapshots/SnapshotShardsService.java Show resolved Hide resolved

original-brownbear added 17 commits June 15, 2020 11:59

less noise

98516c6

fix test

37d439f

cover primary fail-over

8d10c4c

drier tests

ca110ec

fix test

75a4aad

update comment

37e7994

drier

8a5eed5

make things nicer looking

457c443

drop pointless short-circuit

66c6428

doc

72dbbcf

moar test

fc267dc

another corner case fixed

fa5176f

Merge remote-tracking branch 'elastic/master' into allow-multiple-sna…

c976fce

…pshots

Merge remote-tracking branch 'elastic/master' into allow-multiple-sna…

6cb5989

…pshots

more docs

075a0b7

bck

e27ec41

save some more repo data loading

7117707

original-brownbear added 2 commits July 9, 2020 06:38

Merge remote-tracking branch 'elastic/master' into allow-multiple-sna…

ca50bf4

…pshots

be smarter with shard generations to always allow for concurrent stuff

f5a44d4

ywelsch approved these changes Jul 9, 2020

View reviewed changes

original-brownbear added 2 commits July 9, 2020 14:29

Merge remote-tracking branch 'elastic/master' into allow-multiple-sna…

e9b0311

…pshots

CR: limit 1k

897e6d1

tlrx approved these changes Jul 10, 2020

View reviewed changes

original-brownbear added 2 commits July 10, 2020 13:53

Merge remote-tracking branch 'elastic/master' into allow-multiple-sna…

382962b

…pshots

review comments

9e1a0db

original-brownbear added the backport pending label Jul 10, 2020

original-brownbear merged commit d333dac into elastic:master Jul 10, 2020

original-brownbear deleted the allow-multiple-snapshots branch July 10, 2020 13:19

original-brownbear mentioned this pull request Jul 14, 2020

Enable Fully Concurrent Snapshot Operations (#56911) #59578

Merged

original-brownbear removed the backport pending label Jul 14, 2020

original-brownbear mentioned this pull request Jul 15, 2020

Most of SLM Snapshot Orchestration can be Removed #59655

Closed

dakrone mentioned this pull request Jul 16, 2020

SnapshotLifecycleRestIT testFullPolicySnapshot failure #50358

Closed

original-brownbear mentioned this pull request Jul 21, 2020

Fix BwC Snapshot INIT Path #60006

Merged

original-brownbear mentioned this pull request Jul 22, 2020

Fix BwC Snapshot INIT Path (#60006) #60038

Merged

original-brownbear restored the allow-multiple-snapshots branch August 6, 2020 18:33

steve-mushero mentioned this pull request Aug 30, 2020

[Docs] - Snapshots can be concurrent, but docs not fully updated #61680

Closed

joegallo mentioned this pull request Nov 20, 2020

Rework SLM to avoid concurrent snapshots by logging-and-skipping #65318

Open

DaveCTurner mentioned this pull request Mar 2, 2021

Fix Concurrent Snapshot Create+Delete + Delete Index #61770

Merged

nickofthyme mentioned this pull request May 31, 2021

Remove SLM multiple policy restriction elastic/kibana#101007

Closed

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

tlrx mentioned this pull request Sep 6, 2022

Limit the number of concurrent shard snapshots #89826

Open

Conversation

original-brownbear commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented May 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

original-brownbear commented Jul 9, 2020

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

tlrx 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!

original-brownbear commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

original-brownbear commented May 18, 2020 •

edited

Loading