Cluster Explain API uses the allocation process to explain shard allocation decisions #22182

abeyad · 2016-12-14T23:40:05Z

This PR completes the refactoring of the cluster allocation explain API and improves it in the following two high-level ways:

The explain API now uses the same allocators that the AllocationService uses to make shard allocation decisions. Prior to this PR, the explain API would run the deciders against each node for the shard in question, but this was not executed on the same code path as the allocators, and many of the scenarios in shard allocation were not captured due to not executing through the same code paths as the allocators.
The APIs have changed, both on the Java and JSON level, to accurately capture the decisions made by the system. The APIs also now report on shard moving and rebalancing decisions, whereas the previous API did not report decisions for moving shards which cannot remain on their current node or rebalancing shards to form a more balanced cluster.

Note: this change affects plugin developers who may have a custom implementation of the ShardsAllocator interface. The method weighShards has been removed and no longer has any utility. In order to support the new explain API, however, a custom implementation of ShardsAllocator must now implement ShardAllocationDecision decideShardAllocation(ShardRouting shard, RoutingAllocation allocation) which provides a decision and explanation for allocating a single shard. For implementations that do not support explaining a single shard allocation via the cluster allocation explain API, this method can simply return an UnsupportedOperationException.

Relates #21691 #21662 #21103 #20634 #20347

abeyad · 2016-12-16T20:55:25Z

retest this please

ywelsch

I've left some comments. Will give it a more thorough look tomorrow.

ywelsch · 2016-12-18T13:13:57Z

...ain/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplanation.java

nit: no need to specify full path, ShardRoutingState is imported

ywelsch · 2016-12-18T13:14:52Z

...ain/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplanation.java

shard has unassignedInfo if it is in state "initializing".

ywelsch · 2016-12-18T13:23:15Z

...ain/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplanation.java

current_state might be enough.

ywelsch · 2016-12-18T13:25:46Z

...ain/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplanation.java

why not change AllocationStatus.value() to print "not_permitted"?

I didn't want to change how the AllocationStatus is represented in the ClusterState

maybe convert it to an AllocationDecision and use the toString() provided by that one?

I think that will be fine, since every possible value of AllocationStatus is covered by AllocationDecision, and this rendering for UnassignedInfo is specific to the explain API

ywelsch · 2016-12-18T13:34:51Z

...g/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java

why catch this and swallow exception? I think it's ok if the API does not work if that bit is not implemented.

ywelsch · 2016-12-18T14:11:53Z

.../main/java/org/elasticsearch/cluster/routing/allocation/decider/EnableAllocationDecider.java

nicer to have:

StringBuilder buf = new StringBuilder(); if (usedIndexSetting) { buf.append("index setting ["); buf.append(INDEX_ROUTING_ALLOCATION_ENABLE_SETTING.getKey()); } else { buf.append("cluster setting ["); buf.append(CLUSTER_ROUTING_ALLOCATION_ENABLE_SETTING.getKey()); }

also, other deciders have same issue with messages (e.g. filter allocation decider). Let's have a separate PR to fix these messages to make clear that they refer to index / cluster settings.

ywelsch · 2016-12-18T14:21:03Z

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

cleaner to just use

AsyncShardFetch<TransportNodesListGatewayStartedShards.NodeGatewayStartedShards> fetch = asyncFetchStarted.computeIfAbsent(shard.shardId(), shardId -> new InternalAsyncFetch<>(logger, "shard_started", shard.shardId(), startedAction));

ywelsch · 2016-12-18T14:21:18Z

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

see comment above

ywelsch · 2016-12-18T14:25:25Z

...ain/java/org/elasticsearch/rest/action/admin/cluster/RestClusterAllocationExplainAction.java

what does Clint think about this? I also wonder if we should only enable this when ?human is not explicitly set on the request

@clintongormley what do you think about setting the human readable flag for the REST API here? it only affects the time fields (e.g. remaining_delay vs remaining_delay_in_millis for the delayed allocation scenario) and byte fields for shard stores. Should the user just be required to put ?human to get the more user friendly outputs similar to the other APIs?

What I was advocating for was to enable the human flag by default, but if someone sets human = false to respect that as well...

Understood, yes that makes sense. @clintongormley should the human flag be enabled by default for the explain API? For example, the GET indices API takes the human flag but the default is false in that case. WDYT?

Everywhere that we support the human flag, it defaults to false. Even though this API will primarily consumed by humans, I think I'd prefer to keep the consistency of defaulting to false. remaining_delay_in_millis is still quite readable for a human, and rerunning the request with ?human is easy enough.

ywelsch · 2016-12-18T14:27:02Z

.../test/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

I think this comes for free if you use auto-managed min-master-nodes.

abeyad · 2016-12-19T05:53:21Z

@ywelsch I pushed 02dc012353ab8325ac9036fd3e2b728cf1339842 to address your initial comments.

ywelsch

I'm not sure how we should handle initializing / relocating shards. Giving the same output as for started shards is highly confusing (e.g. SameShardAllocator). There are also no tests for these 2 scenarios.
The API should support selecting a specific shard if there are multiple replica shards that are allocated. One possibility would be to pass the node id of the node the selected shard is allocated to. This will help answer the question "Why is that replica shard allocated to that node".
The docs also require updates with the changed output format.

ywelsch · 2016-12-19T17:43:37Z

...g/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java

I wonder if we should have an extra flag to set "explain mode".

I thought about that, but felt that there really is no concrete distinction between "debug" mode and "explain" mode in this case. Would changing the naming of the enum values help?

With the change regarding relocated/initializing shard I think it's fine now.

ywelsch · 2016-12-19T17:49:26Z

core/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/Decision.java

should this throw an UnsupportedOperationException?

Sure, it would help to catch incorrect usages, as getExplanation() should never be called on Multi

abeyad · 2016-12-19T19:54:48Z

@ywelsch

I'm not sure how we should handle initializing / relocating shards. Giving the same output as for started shards is highly confusing (e.g. SameShardAllocator). There are also no tests for these 2 scenarios.

I am not sure myself. Thinking about it some more, I prefer if we don't try to explain initializing / relocating shards. It doesn't make sense in context - an initializing shard just got assigned, so there is nothing to report about in terms of "why are my shards unassigned" and whichever node it got assigned to is either (1) the most optimal node from a cluster balance perspective or (2) it had a copy of the data. In any case, nothing to really explain. In the case of relocating shards, the same thing - we are relocating due to a better balance or being required to move, so there is nothing new to explain that the allocators didn't already take into account by starting the relocation process. I think if we returned a ShardAllocationDecision that had a method isDecisionTaken() that returned false for the Java API, it would be sufficient, and for the JSON output, we could just have a single string output "cannot explain initializing | relocating shards". The other alternative is to throw an exception. What do you think? @clintongormley do you have any thoughts on this point?

The API should support selecting a specific shard if there are multiple replica shards that are allocated. One possibility would be to pass the node id of the node the selected shard is allocated to. This will help answer the question "Why is that replica shard allocated to that node".

This is a good idea. I'll add this.

The docs also require updates with the changed output format.

Yes, I was going to do this in a follow up PR.

abeyad · 2016-12-19T21:46:40Z

I pushed 56573f99d6518996d98eefa6b7a5a56288e218d1 to add the option to specify a node id for an assigned replica shard to explain.

clintongormley · 2016-12-20T08:49:01Z

I think if we returned a ShardAllocationDecision that had a method isDecisionTaken() that returned false for the Java API, it would be sufficient, and for the JSON output, we could just have a single string output "cannot explain initializing | relocating shards". The other alternative is to throw an exception. What do you think? @clintongormley do you have any thoughts on this point?

Sounds reasonable to me

abeyad · 2016-12-20T15:10:31Z

@ywelsch I pushed 7c960c4f81ae2819834d1d3367c5217e881aa395 to not try to explain initializing or relocating shards.

ywelsch · 2016-12-20T15:18:40Z

...ain/java/org/elasticsearch/rest/action/admin/cluster/RestClusterAllocationExplainAction.java

why default to false? This API is to be consumed primarily by humans

Per Clint's comment here #22182 (comment)

ywelsch · 2016-12-20T15:20:34Z

...ain/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplanation.java

this variable is only used in one place. Just put it there.

this was a mistake, i meant to use it in the line below as well, so I changed the line below to use it (its used twice now)

ywelsch · 2016-12-20T15:21:41Z

...ain/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplanation.java

maybe swap the branches, no need to have an == false condition here

ywelsch · 2016-12-20T15:24:32Z

core/src/main/java/org/elasticsearch/cluster/routing/allocation/ShardAllocationDecision.java

just make NOT_TAKEN public static.

ywelsch · 2016-12-20T15:28:22Z

.../java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainRequest.java

current_node might suffice (see reroute API)

ywelsch · 2016-12-20T15:30:15Z

.../java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainRequest.java

ywelsch · 2016-12-20T15:31:33Z

.../java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainRequest.java

return null as well?

ywelsch · 2016-12-20T15:34:27Z

...g/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java

if replicaNode is specified, we have to check that a shard on that node can be found. Also I think we should use DiscoveryNodes.resolveNode so that the user can specify the node by name or id or ...

This code already fails if a replica shard on the specified node cannot be found. foundShard will be null so we will throw an exception below.

ywelsch · 2016-12-20T16:25:17Z

...g/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java

can you also unit test?

ywelsch · 2016-12-20T16:27:55Z

...g/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java

With the change regarding relocated/initializing shard I think it's fine now.

ywelsch

Left a few more comments.

abeyad · 2016-12-20T20:49:41Z

@ywelsch I pushed 9a5134af5c92120ed362efc4c156c060d9a9f2e6 and e0d6df724b6d489129bbc4d0e382cf16e8114b98

ywelsch

Left some minor comments but looks good o.w.

ywelsch · 2016-12-21T17:39:20Z

...g/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java

IllegalStateException?

ywelsch · 2016-12-21T17:53:43Z

...g/elasticsearch/action/admin/cluster/allocation/TransportClusterAllocationExplainAction.java

why not just let it bubble up?

ywelsch · 2016-12-21T17:54:21Z

.../java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainRequest.java

not true anymore

ywelsch · 2016-12-21T17:56:35Z

.../java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainRequest.java

should this also include this.currentNode == null?

ywelsch · 2016-12-21T17:57:47Z

...rg/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainRequestBuilder.java

not true anymore

ywelsch · 2016-12-21T17:58:45Z

...ain/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplanation.java

I prefer relocationTargetNode to relocatingTargetNode.

ywelsch · 2016-12-21T18:04:05Z

...ain/java/org/elasticsearch/rest/action/admin/cluster/RestClusterAllocationExplainAction.java

ywelsch · 2016-12-21T18:05:14Z

...a/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainActionTests.java

testInitializingOrRelocatingShardExplanation

ywelsch · 2016-12-21T18:06:21Z

rest-api-spec/src/main/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yaml

current_state?

abeyad · 2016-12-21T20:01:07Z

@ywelsch thanks for your review! I pushed another commit to address the last round of feedback.

ywelsch

LGTM. Thanks @abeyad

abeyad · 2016-12-22T03:19:58Z

retest this please

abeyad · 2016-12-22T05:00:01Z

retest this please

abeyad · 2016-12-22T16:10:42Z

retest this please

abeyad · 2016-12-22T18:42:35Z

retest this please

This commit changes the REST and transport layers to use the allocation deciders refactoring and new formats for presenting the allocation explanations. The API is changed from both the Java API layer as well as the REST layer. This is a breaking change from 5.0.x and 5.1.x explain API.

the coordinating node and master node differ in version numbers.

ywelsch · 2016-12-29T16:12:00Z

rest-api-spec/src/main/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yaml

 "cluster shard allocation explanation test":
+  - skip:
+      version: " - 5.2.0"
+      reason: allocation explain api format is different in versions <= 5.2.0


did you mean 5.1.99? The explain format is different in versions < 5.2.0?

ywelsch · 2016-12-29T16:29:11Z

test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java

            try {
                orig = serialize(version, streamable);
-            } catch (IllegalArgumentException e) {
+            } catch (IllegalArgumentException | IllegalStateException e) {


Use IllegalArgumentException for checkVersion functionality as well. It fits better than IllegalStateException I think (the node does not need to look at current state to reject serializing the message but just at the arguments).

ywelsch · 2016-12-29T16:34:16Z

.../java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainRequest.java


    @Override
    public void writeTo(StreamOutput out) throws IOException {
+        checkVersion(out.getVersion());


I wonder if we can have a REST test that checks this exception. This could be done with a skip section of the form 5.2.0 - so that the test is only active in BWC tests where the minimum es version is < 5.2.0.

The issue with doing this is that such a test will sometimes pass (having triggered the IllegalStateException) and sometimes it won't. In particular, if the REST request is routed directly to the master node, there will be no exception. The exception will only occur if the REST request is routed to the non-master node which is on a different version from master (and not backward compatible w.r.t. the explain API), in which case the exception will be triggered when routed to the master node.

I did manually test these scenarios. We can discuss options tomorrow.

abeyad · 2017-01-01T23:45:04Z

@ywelsch I pushed 03656a9 to address your feedback.

abeyad · 2017-01-02T14:58:18Z

retest this please

…cation decisions (#22182) This PR completes the refactoring of the cluster allocation explain API and improves it in the following two high-level ways: 1. The explain API now uses the same allocators that the AllocationService uses to make shard allocation decisions. Prior to this PR, the explain API would run the deciders against each node for the shard in question, but this was not executed on the same code path as the allocators, and many of the scenarios in shard allocation were not captured due to not executing through the same code paths as the allocators. 2. The APIs have changed, both on the Java and JSON level, to accurately capture the decisions made by the system. The APIs also now report on shard moving and rebalancing decisions, whereas the previous API did not report decisions for moving shards which cannot remain on their current node or rebalancing shards to form a more balanced cluster. Note: this change affects plugin developers who may have a custom implementation of the ShardsAllocator interface. The method weighShards has been removed and no longer has any utility. In order to support the new explain API, however, a custom implementation of ShardsAllocator must now implement ShardAllocationDecision decideShardAllocation(ShardRouting shard, RoutingAllocation allocation) which provides a decision and explanation for allocating a single shard. For implementations that do not support explaining a single shard allocation via the cluster allocation explain API, this method can simply return an UnsupportedOperationException.

abeyad · 2017-01-02T18:45:37Z

5.x commit: 7c879ef

Removes ensureStableCluster() calls in ClusterAllocationExplainIT testing after startNode*() calls. The stabilization checks were originally necessary when the tests were added in #22182. However, #49248 later enhanced validateClusterFormed(), which startNode*() implicitly calls, to perform similar checks to ensureStableCluster()

abeyad added :Allocation >enhancement v5.2.0 v6.0.0-alpha1 labels Dec 14, 2016

abeyad requested a review from ywelsch December 14, 2016 23:40

abeyad added the :Cluster label Dec 14, 2016

clintongormley added the release highlight label Dec 16, 2016

ywelsch suggested changes Dec 18, 2016

View reviewed changes

abeyad added the >breaking-java label Dec 19, 2016

ywelsch reviewed Dec 19, 2016

View reviewed changes

ywelsch reviewed Dec 20, 2016

View reviewed changes

ywelsch approved these changes Dec 21, 2016

View reviewed changes

abeyad force-pushed the explain_api_refactor_rest branch from 81c16f5 to 02213ed Compare December 21, 2016 19:59

ywelsch approved these changes Dec 21, 2016

View reviewed changes

abeyad force-pushed the explain_api_refactor_rest branch 2 times, most recently from 9648ff4 to 0089cc1 Compare December 22, 2016 15:13

Ali Beyad added 9 commits December 23, 2016 09:05

more feedback

eecd385

fix

4917f12

fix merge conflict with master

12e8247

fix test

b0eb2c0

fix test

c997b72

more test fixes

5156c84

skip section for backwards compat rest tests

683d275

fix test

c26d7f4

Better error reporting with mixed cluster explain API execution when

4e7bcff

the coordinating node and master node differ in version numbers.

abeyad force-pushed the explain_api_refactor_rest branch from aa5aae2 to 4e7bcff Compare December 23, 2016 17:38

ywelsch reviewed Dec 29, 2016

View reviewed changes

address review

03656a9

abeyad merged commit 20ab4be into elastic:master Jan 2, 2017

abeyad deleted the explain_api_refactor_rest branch January 2, 2017 18:28

This was referenced Jan 3, 2017

Cluster allocation explain backwards tests fail #22414

Closed

ClusterAllocationExplainIT.testUnassignedPrimaryWithExistingIndex fails #22417

Closed

lcawl added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. and removed :Allocation labels Feb 13, 2018

clintongormley added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. and removed :Cluster labels Feb 13, 2018

This was referenced Aug 17, 2023

Add node "roles" to allocation explain response #98550

Merged

[TEST] Remove redundant cluster checks #98694

Merged

Cluster Explain API uses the allocation process to explain shard allocation decisions #22182

Cluster Explain API uses the allocation process to explain shard allocation decisions #22182

Uh oh!

Conversation

abeyad commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abeyad commented Dec 16, 2016

Uh oh!

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

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!

abeyad commented Dec 19, 2016

Uh oh!

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

abeyad commented Dec 19, 2016

Uh oh!

abeyad commented Dec 19, 2016

Uh oh!

clintongormley commented Dec 20, 2016

Uh oh!

abeyad commented Dec 20, 2016

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

abeyad commented Dec 14, 2016 •

edited

Loading