Auto-allocate searchable snapshots #52527

DaveCTurner · 2020-02-19T17:20:30Z

This commit allows plugins to supply their own allocator for existing shards,
generalizing the default GatewayAllocator. It uses this to implement a very
simple auto-allocation process for searchable snapshots which respects the
allocation deciders but which can otherwise be assigned to any node.

Relates #50999

This commit allows plugins to supply their own allocator for existing shards, generalizing the default `GatewayAllocator`. It uses this to implement a very simple auto-allocation process for searchable snapshots which respects the allocation deciders but which can otherwise be assigned to any node.

elasticmachine · 2020-02-19T17:20:33Z

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

DaveCTurner

I'm opening this for initial comments before I get too deep into adding/modifying tests to match the changes here; it should pass the existing test suite at least.

server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationService.java

server/src/main/java/org/elasticsearch/index/store/Store.java

DaveCTurner · 2020-02-19T18:21:28Z

@elasticmachine please run elasticsearch-ci/2

DaveCTurner · 2020-02-19T18:24:03Z

I forgot to mention that of course we expect to have a more sophisticated mechanism in future with async fetching of information from the individual nodes before making a decision. This PR is only focussed on the plumbing needed to expose this kind of allocation to plugins.

DaveCTurner · 2020-02-19T19:30:56Z

@elasticmachine please run elasticsearch-ci/2

…-allocator-for-existing-shards

henningandersen

Thanks Dave, I read through it mainly out of interest. I added a few comments anyway, feel free to dismiss me quickly if it makes no sense 😃

server/src/main/java/org/elasticsearch/gateway/PrimaryShardAllocator.java

...s/src/main/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotAllocator.java

ywelsch

I've left some thoughts similar to what Henning said

server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationService.java

server/src/main/java/org/elasticsearch/gateway/PrimaryShardAllocator.java

server/src/main/java/org/elasticsearch/index/store/Store.java

Move the iteration through the shards from the individual allocators up to the AllocationService, allowing us to allocate them in proper priority order and also allowing us to assign a unique allocator to each shard according to an index setting.

DaveCTurner · 2020-02-21T14:05:32Z

I've taken this in the direction suggested by both @ywelsch and @henningandersen, added some tests, and removed the WIP label. This is ready for a proper review now.

…-allocator-for-existing-shards

ywelsch

I've left some smaller comments, overall looking good. Should we also have some basic tests for SearchableSnapshotAllocator or do you want to leave that to a follow-up?

server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationService.java

ywelsch · 2020-02-23T16:31:36Z

server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationService.java

+    private ExistingShardsAllocator getAllocatorForShard(ShardRouting shardRouting, RoutingAllocation routingAllocation) {
+        assert assertInitialized();
+        final String allocatorName = ExistingShardsAllocator.EXISTING_SHARDS_ALLOCATOR_SETTING.get(
+            routingAllocation.metaData().getIndexSafe(shardRouting.index()).getSettings());


as these lookups can be expensive in case there are many unassigned shards, I wonder if we should pass the retrieved IndexMetaData to the ExistingShardsAllocator.

It involves passing the IndexMetaData around to a lot of places. I mean we could do that if you're sure this is sufficiently expensive, but it seems surprising to me that this would be a concern.

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

server/src/main/java/org/elasticsearch/plugins/ClusterPlugin.java

henningandersen

I left a number of smaller comments and a question on how the balancer and searchable snapshot allocation will interact.

server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationService.java

server/src/main/java/org/elasticsearch/cluster/routing/allocation/ExistingShardsAllocator.java

server/src/main/java/org/elasticsearch/gateway/GatewayAllocator.java

server/src/main/java/org/elasticsearch/cluster/routing/allocation/ExistingShardsAllocator.java

server/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationService.java

server/src/main/java/org/elasticsearch/gateway/GatewayAllocator.java

...s/src/main/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotAllocator.java

…-allocator-for-existing-shards

DaveCTurner · 2020-03-05T13:47:12Z

Hey folks, any more comments on this PR?

ywelsch

LGTM

henningandersen

LGTM.

henningandersen · 2020-03-06T07:02:17Z

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

+                : clusterPlugin.getExistingShardsAllocators().entrySet()) {
+                final String allocatorName = existingShardsAllocatorEntry.getKey();
+                if (existingShardsAllocators.put(allocatorName, existingShardsAllocatorEntry.getValue()) != null) {
+                    throw new IllegalArgumentException("ExistingShardsAllocator [" + allocatorName + "] already defined");


Can we include clusterPlugin.getClass().getName() into the message?

henningandersen · 2020-03-06T07:12:29Z

server/src/main/java/org/elasticsearch/cluster/routing/allocation/ExistingShardsAllocator.java

+     * Called before starting a round of allocation, allowing the allocator to invalidate some caches if appropriate.
+     */
+    void beforeAllocation(RoutingAllocation allocation);
+


nit: remove extra empty line.

henningandersen · 2020-03-06T07:21:21Z

server/src/main/java/org/elasticsearch/gateway/BaseGatewayShardAllocator.java

-        while (unassignedIterator.hasNext()) {
-            final ShardRouting shard = unassignedIterator.next();
-            final AllocateUnassignedDecision allocateUnassignedDecision = makeAllocationDecision(shard, allocation, logger);
+    public void allocateUnassigned(RoutingAllocation allocation, ShardRouting shardRouting,


nit: swap argument order to make it the same as in ExistingShardsAllocator:

Suggested change

public void allocateUnassigned(RoutingAllocation allocation, ShardRouting shardRouting,

public void allocateUnassigned(ShardRouting shardRouting, RoutingAllocation allocation,

…-allocator-for-existing-shards

In elastic#50239 we introduced a temporary workaround in Store#tryOpenIndex to allow searchable snapshots shards to be allocated anywhere. elastic#52527 reverts this workaround but did not revert the associated test changes. This commit reverts the associated test changes.

In #50239 we introduced a temporary workaround in Store#tryOpenIndex to allow searchable snapshots shards to be allocated anywhere. #52527 reverts this workaround but did not revert the associated test changes. This commit reverts the associated test changes.

DaveCTurner added >enhancement WIP :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) labels Feb 19, 2020

DaveCTurner requested a review from ywelsch February 19, 2020 17:20

DaveCTurner requested a review from tlrx February 19, 2020 17:20

DaveCTurner commented Feb 19, 2020

View reviewed changes

Rename

44df860

DaveCTurner added 2 commits February 19, 2020 20:11

Merge branch 'feature/searchable-snapshots' into 2020-02-19-pluggable…

1015ac8

…-allocator-for-existing-shards

Merge branch 'feature/searchable-snapshots' into 2020-02-19-pluggable…

438ec61

…-allocator-for-existing-shards

henningandersen reviewed Feb 20, 2020

View reviewed changes

ywelsch reviewed Feb 20, 2020

View reviewed changes

DaveCTurner added 8 commits February 20, 2020 18:10

Didn't know I edited that

9d1da96

Revert sorting by primary/replica since we have to iterate twice anyway

ab39f0a

Revert some unnecessary changes

66ca063

Add tests for duplicate checks of existing shards allocators

02e6158

We always take a decision right now

5154f7f

Add test of allocation service behaviour

e61a342

Add test for unknown allocator too

aad3fc0

DaveCTurner requested review from henningandersen and ywelsch February 21, 2020 13:57

DaveCTurner removed the WIP label Feb 21, 2020

Merge branch 'feature/searchable-snapshots' into 2020-02-19-pluggable…

09c49b8

…-allocator-for-existing-shards

DaveCTurner mentioned this pull request Feb 21, 2020

Lazy snapshot restores #50999

Closed

19 tasks

ywelsch reviewed Feb 24, 2020

View reviewed changes

henningandersen reviewed Feb 26, 2020

View reviewed changes

DaveCTurner added 9 commits March 2, 2020 12:29

Merge branch 'feature/searchable-snapshots' into 2020-02-19-pluggable…

f1228d6

…-allocator-for-existing-shards

Reformat

7682438

Consistent use of loops

c9b84ab

Improve comment

6cd0917

Use null rather than empty for unset

e6e5b7e

Extract interface

c7c1701

Consistent argument order

a72d9ba

More specific method name

3301a2b

Capture plugins at init time

4b8888c

DaveCTurner requested review from henningandersen and ywelsch March 2, 2020 14:30

ywelsch approved these changes Mar 5, 2020

View reviewed changes

henningandersen approved these changes Mar 6, 2020

View reviewed changes

DaveCTurner added 6 commits March 11, 2020 10:03

Merge branch 'feature/searchable-snapshots' into 2020-02-19-pluggable…

ac303eb

…-allocator-for-existing-shards

Include plugin class name in error

01c9b33

Whitespace

1857794

Reorder params

41fa9c6

Merge branch 'feature/searchable-snapshots' into 2020-02-19-pluggable…

4c9d3ee

…-allocator-for-existing-shards

Merge branch 'feature/searchable-snapshots' into 2020-02-19-pluggable…

05b690c

…-allocator-for-existing-shards

DaveCTurner merged commit 215e94d into elastic:feature/searchable-snapshots Mar 11, 2020

DaveCTurner deleted the 2020-02-19-pluggable-allocator-for-existing-shards branch March 11, 2020 15:04

DaveCTurner mentioned this pull request Mar 16, 2020

Revert workarounds for tryOpenIndex change #53615

Merged

tlrx mentioned this pull request Apr 6, 2020

Merge feature/searchable-snapshots branch into master #54803

Merged

	public void allocateUnassigned(RoutingAllocation allocation, ShardRouting shardRouting,
	public void allocateUnassigned(ShardRouting shardRouting, RoutingAllocation allocation,

Auto-allocate searchable snapshots #52527

Auto-allocate searchable snapshots #52527

Uh oh!

Conversation

DaveCTurner commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 19, 2020

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner commented Feb 19, 2020

Uh oh!

DaveCTurner commented Feb 19, 2020

Uh oh!

DaveCTurner commented Feb 19, 2020

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner commented Feb 21, 2020

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ywelsch Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner commented Mar 5, 2020

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

henningandersen Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

henningandersen Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaveCTurner commented Feb 19, 2020 •

edited

Loading