Add check when trying to reroute a shard to a non-data discovery node #28886

autophagy · 2018-03-02T20:27:36Z

While trying to reroute a shard to or from a non-data node (a node with node.data=false), I encountered a null pointer exception. Though an exception is to be expected, the NPE was occurring because allocation.routingNodes() would not contain any non-data nodes, so when you attempt to do allocation.routingNodes.node(non-data-node) it would not find it, and thus error. This occured regardless of whether I was rerouting to or from a non-data node.

This PR adds a check (as well as a test for these use cases) to return a legible, useful exception if the discovery node you are rerouting to or from is not a data node.

elasticmachine · 2018-03-02T20:27:37Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-03-02T20:27:37Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

autophagy · 2018-03-02T20:35:11Z

Sorry - I have signed the CLA now. 🙂

bleskes · 2018-03-02T22:33:09Z

@elasticmachine - please test this

bleskes

Thank you @autophagy for the PR. I left some very minor asks. Looks good otherwise.

bleskes · 2018-03-02T22:41:19Z

...rc/main/java/org/elasticsearch/cluster/routing/allocation/command/MoveAllocationCommand.java

@@ -122,6 +127,9 @@ public RerouteExplanation execute(RoutingAllocation allocation, boolean explain)
            }

            RoutingNode toRoutingNode = allocation.routingNodes().node(toDiscoNode.getId());
+            if (toRoutingNode == null && !toDiscoNode.isDataNode()) {


can we move this validation up to the start of the method, next to the validation of the fromNode? then all of this is together and we can fail early.

bleskes · 2018-03-02T22:42:19Z

...src/test/java/org/elasticsearch/cluster/routing/allocation/NonDataNodeShardRoutingTests.java

+
+import static java.util.Collections.emptyMap;
+
+public class NonDataNodeShardRoutingTests extends ESAllocationTestCase {


I think we can move these to AllocationCommandsTests - no need to have a dedicated class for this.

bleskes · 2018-03-02T22:43:50Z

...src/test/java/org/elasticsearch/cluster/routing/allocation/NonDataNodeShardRoutingTests.java

+                .add(new DiscoveryNode("node1", "node1", "node1", "test1", "test1", buildNewFakeTransportAddress(), emptyMap(),
+                    MASTER_DATA_ROLES, Version.CURRENT))
+                .add(new DiscoveryNode("node2", "node2", "node2", "test2", "test2", buildNewFakeTransportAddress(), emptyMap(),
+                    Collections.unmodifiableSet(EnumSet.of(DiscoveryNode.Role.MASTER)), Version.CURRENT))).build();


can we randomize and sometime choose a non-data/non-master nodes? i.e. give the node a random combination of roles, making sure it never gets the data role.

bleskes · 2018-03-02T22:44:02Z

...src/test/java/org/elasticsearch/cluster/routing/allocation/NonDataNodeShardRoutingTests.java

+                .add(new DiscoveryNode("node1", "node1", "node1", "test1", "test1", buildNewFakeTransportAddress(), emptyMap(),
+                    MASTER_DATA_ROLES, Version.CURRENT))
+                .add(new DiscoveryNode("node2", "node2", "node2", "test2", "test2", buildNewFakeTransportAddress(), emptyMap(),
+                    Collections.unmodifiableSet(EnumSet.of(DiscoveryNode.Role.MASTER)), Version.CURRENT))).build();


same comment on randomization of roles.

…ry node

autophagy · 2018-03-03T20:21:33Z

@bleskes Thank you for the comments! I think that fixup should address them.

DaveCTurner · 2018-03-09T12:48:25Z

...rc/main/java/org/elasticsearch/cluster/routing/allocation/command/MoveAllocationCommand.java

-        for (ShardRouting shardRouting : allocation.routingNodes().node(fromDiscoNode.getId())) {
+        RoutingNode fromRoutingNode = allocation.routingNodes().node(fromDiscoNode.getId());
+        if (fromRoutingNode == null && !fromDiscoNode.isDataNode()) {
+            throw new IllegalArgumentException("[move_allocation] can't move " + shardId + ", from " + fromDiscoNode + ", to " + toDiscoNode + ", since its not allowed, reason: " +  fromDiscoNode + " is not a data node.");


Nit: I think it'd be better for the message to read:

[move_allocation] can't move abc123 from node1 to node2: node1 is not a data node

(NB less punctuation, and no need to say since its not allowed)

DaveCTurner · 2018-03-09T12:49:39Z

server/src/test/java/org/elasticsearch/cluster/routing/allocation/AllocationCommandsTests.java

+        RoutingAllocation routingAllocation = new RoutingAllocation(new AllocationDeciders(Settings.EMPTY, Collections.emptyList()),
+            new RoutingNodes(clusterState, false), clusterState, ClusterInfo.EMPTY, System.nanoTime());
+        logger.info("--> executing move allocation command to non-data node");
+        expectThrows(IllegalArgumentException.class, () -> command.execute(routingAllocation, false));


Please could you assert that the content of the message is correct? (expectThrows returns the exception thrown, so you just need to assign it to a variable and check its message.)

DaveCTurner · 2018-03-09T12:50:18Z

...rc/main/java/org/elasticsearch/cluster/routing/allocation/command/MoveAllocationCommand.java

+        }
+        RoutingNode toRoutingNode = allocation.routingNodes().node(toDiscoNode.getId());
+        if (toRoutingNode == null && !toDiscoNode.isDataNode()) {
+            throw new IllegalArgumentException("[move_allocation] can't move " + shardId + ", from " + fromDiscoNode + ", to " + toDiscoNode + ", since its not allowed, reason: " +  toDiscoNode + " is not a data node.");


As previous comment - this message could be leaner.

DaveCTurner · 2018-03-09T12:50:45Z

server/src/test/java/org/elasticsearch/cluster/routing/allocation/AllocationCommandsTests.java

+        RoutingAllocation routingAllocation = new RoutingAllocation(new AllocationDeciders(Settings.EMPTY, Collections.emptyList()),
+            new RoutingNodes(clusterState, false), clusterState, ClusterInfo.EMPTY, System.nanoTime());
+        logger.info("--> executing move allocation command from non-data node");
+        expectThrows(IllegalArgumentException.class, () -> command.execute(routingAllocation, false));


As previous comment: please could you assert that the content of the message is correct?

DaveCTurner · 2018-03-09T13:08:37Z

Also you seem to have signed the CLA with a different email address from the one attached to these commits (Autophagy <mika@crate.io>). Could you sign again with this email address please?

…discovery node

autophagy · 2018-03-09T14:39:22Z

@DaveCTurner Thanks for the suggestion to check the message - I've refactored the test to use the same initial set up as testMoveShardCommand as the test was testing the wrong exception (but the right class).

I've also signed the CLA with the crate.io address. 🙂

DaveCTurner

Couple more minor things that I missed on the first review, sorry.

DaveCTurner · 2018-03-09T15:38:33Z

server/src/test/java/org/elasticsearch/cluster/routing/allocation/AllocationCommandsTests.java

+            new RoutingNodes(clusterState, false), clusterState, ClusterInfo.EMPTY, System.nanoTime());
+        logger.info("--> executing move allocation command from non-data node");
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> command.execute(routingAllocation, false));
+        assertEquals("[move_allocation] can't move 0 from node2 to node1: node2 is not a data node.", e.getMessage());


Excellent, thanks. I'm glad you did that, because it shows that this message isn't what I was expecting. The shard number on its own is likely to cause confusion, and also it's useful to have the full DiscoveryNode.toString() output rather than just using .getName().

Something like this'd be great:

DaveCTurner@c53a980

DaveCTurner · 2018-03-09T15:39:18Z

...rc/main/java/org/elasticsearch/cluster/routing/allocation/command/MoveAllocationCommand.java

-        for (ShardRouting shardRouting : allocation.routingNodes().node(fromDiscoNode.getId())) {
+        RoutingNode fromRoutingNode = allocation.routingNodes().node(fromDiscoNode.getId());
+        if (fromRoutingNode == null && !fromDiscoNode.isDataNode()) {
+            throw new IllegalArgumentException("[move_allocation] can't move " + shardId + " from " + fromDiscoNode.getName() + " to " + toDiscoNode.getName() + ": " +  fromDiscoNode.getName() + " is not a data node.");


Could you wrap these lines to <120 characters? We're trying to cut down on overlong lines.

Sure thing!

…n-data discovery node

DaveCTurner

Great, LGTM.

DaveCTurner · 2018-03-09T16:50:24Z

@elasticmachine please test this.

DaveCTurner · 2018-03-09T16:50:55Z

@bleskes are you happy with the changes made?

elasticmachine · 2018-03-09T22:43:24Z

Pinging @elastic/es-distributed

bleskes

LGTM @autophagy . I'll pull this in.

…#28886) While trying to reroute a shard to or from a non-data node (a node with ``node.data=false``), I encountered a null pointer exception. Though an exception is to be expected, the NPE was occurring because ``allocation.routingNodes()`` would not contain any non-data nodes, so when you attempt to do ``allocation.routingNodes.node(non-data-node)`` it would not find it, and thus error. This occurred regardless of whether I was rerouting to or from a non-data node. This PR adds a check (as well as a test for these use cases) to return a legible, useful exception if the discovery node you are rerouting to or from is not a data node.

Add check when trying to reroute a shard to a non-data discovery node

2eef691

bleskes suggested changes Mar 2, 2018

View reviewed changes

fixup! Add check when trying to reroute a shard to a non-data discove…

99d0c6d

…ry node

DaveCTurner reviewed Mar 9, 2018

View reviewed changes

fixup! fixup! Add check when trying to reroute a shard to a non-data …

0226fb7

…discovery node

DaveCTurner reviewed Mar 9, 2018

View reviewed changes

fixup! fixup! fixup! Add check when trying to reroute a shard to a no…

9ae73e5

…n-data discovery node

DaveCTurner approved these changes Mar 9, 2018

View reviewed changes

DaveCTurner added >bug v7.0.0 v6.3.0 :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) labels Mar 9, 2018

bleskes approved these changes Mar 12, 2018

View reviewed changes

bleskes merged commit a7b53fd into elastic:master Mar 12, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check when trying to reroute a shard to a non-data discovery node #28886

Add check when trying to reroute a shard to a non-data discovery node #28886

autophagy commented Mar 2, 2018

elasticmachine commented Mar 2, 2018

elasticmachine commented Mar 2, 2018

autophagy commented Mar 2, 2018

bleskes commented Mar 2, 2018

bleskes left a comment

bleskes Mar 2, 2018

bleskes Mar 2, 2018

bleskes Mar 2, 2018

bleskes Mar 2, 2018

autophagy commented Mar 3, 2018

DaveCTurner Mar 9, 2018

DaveCTurner Mar 9, 2018

DaveCTurner Mar 9, 2018

DaveCTurner Mar 9, 2018

DaveCTurner commented Mar 9, 2018

autophagy commented Mar 9, 2018 •

edited

DaveCTurner left a comment

DaveCTurner Mar 9, 2018

DaveCTurner Mar 9, 2018

autophagy Mar 9, 2018

DaveCTurner left a comment

DaveCTurner commented Mar 9, 2018

DaveCTurner commented Mar 9, 2018

elasticmachine commented Mar 9, 2018

bleskes left a comment


		import static java.util.Collections.emptyMap;

		public class NonDataNodeShardRoutingTests extends ESAllocationTestCase {

Add check when trying to reroute a shard to a non-data discovery node #28886

Add check when trying to reroute a shard to a non-data discovery node #28886

Conversation

autophagy commented Mar 2, 2018

elasticmachine commented Mar 2, 2018

elasticmachine commented Mar 2, 2018

autophagy commented Mar 2, 2018

bleskes commented Mar 2, 2018

bleskes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autophagy commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Mar 9, 2018

autophagy commented Mar 9, 2018 • edited

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner commented Mar 9, 2018

DaveCTurner commented Mar 9, 2018

elasticmachine commented Mar 9, 2018

bleskes left a comment

Choose a reason for hiding this comment

autophagy commented Mar 9, 2018 •

edited