Introduce transport API for cluster bootstrapping #34961

DaveCTurner · 2018-10-29T12:31:40Z

Introduces a transport API for bootstrapping a Zen2 cluster
Introduces a transport API for requesting the set of nodes that a
master-eligible node has discovered and for waiting until this comprises the
expected number of nodes.
Alters ESIntegTestCase to use these APIs when forming a cluster, rather than
injecting the initial configuration directly.

- Introduces a transport API for bootstrapping a Zen2 cluster - Introduces a transport API for requesting the set of nodes that a master-eligible node has discovered and for waiting until this comprises the expected number of nodes. - Alters ESIntegTestCase to use these APIs when forming a cluster, rather than injecting the initial configuration directly.

elasticmachine · 2018-10-29T12:31:42Z

Pinging @elastic/es-distributed

DaveCTurner

Added some comments to help with the review.

DaveCTurner · 2018-10-29T12:32:21Z

...src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/GetDiscoveredNodesAction.java

+
+public class GetDiscoveredNodesAction extends Action<GetDiscoveredNodesResponse> {
+    public static final GetDiscoveredNodesAction INSTANCE = new GetDiscoveredNodesAction();
+    public static final String NAME = "cluster:monitor/discovered_nodes";


Not sure about this name.

this is an existing category for security, I am not sure either if the API falls under monitor but it sounds ok to me. talk with the security team about this?

DaveCTurner · 2018-10-29T12:32:50Z

...r/src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/BootstrapClusterAction.java

+
+public class BootstrapClusterAction extends Action<AcknowledgedResponse> {
+    public static final BootstrapClusterAction INSTANCE = new BootstrapClusterAction();
+    public static final String NAME = "cluster:coordination/bootstrap_cluster";


Not sure about this name.

cluster should be good but coordination would introduce a new subcategory for security I think, which should be discussed probably. See https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ClusterPrivilege.java

DaveCTurner · 2018-10-29T12:34:51Z

server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java

    public void setInitialConfiguration(final VotingConfiguration votingConfiguration) {
        synchronized (mutex) {
            final ClusterState currentState = getStateForMasterService();

-            if (currentState.getLastAcceptedConfiguration().isEmpty() == false) {
+            if (isInitialConfigurationSet()) {


This is safe to extract because getStateForMasterService depends only on things protected by mutex.

DaveCTurner · 2018-10-29T12:35:42Z

server/src/main/java/org/elasticsearch/cluster/coordination/ElectionSchedulerFactory.java

-            threadPool.schedule(TimeValue.timeValueMillis(delayMillis), Names.GENERIC, runnable);
+            try {
+                threadPool.schedule(TimeValue.timeValueMillis(delayMillis), Names.GENERIC, runnable);
+            } catch (EsRejectedExecutionException e) {


This change is just for EsRejectedExecutionException-handling, which arose in test failures. No test to verify this specifically is added here.

DaveCTurner · 2018-10-29T12:35:59Z

server/src/main/java/org/elasticsearch/discovery/PeerFinder.java

+                    return "PeerFinder handling wakeup";
+                }
+            });
+        } catch (EsRejectedExecutionException e) {


The changes to this file are just for EsRejectedExecutionException-handling, which arose in test failures. No test to verify this specifically is added here.

Can you explain what kind of test failures you were seeing? I would like to avoid this wrapping here.

ok, got it now (after running into failure myself). As this pattern is repeated here many times, let's add a method to ThreadPool that schedules while ignoring shutdowns.

Ok see 1b62782

DaveCTurner · 2018-10-29T12:37:08Z

server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java

@@ -881,6 +895,18 @@ public String toString() {
        });
    }

+    public Releasable withDiscoveryListener(Consumer<Iterable<DiscoveryNode>> listener) {


These changes to Coordinator are tested by testDiscoveryOfPeersTriggersNotification.

DaveCTurner · 2018-10-29T12:37:57Z

server/src/test/java/org/elasticsearch/cluster/coordination/NodeJoinTests.java

@@ -520,16 +518,4 @@ private boolean isLocalNodeElectedMaster() {
    private boolean clusterStateHasNode(DiscoveryNode node) {
        return node.equals(MasterServiceTests.discoveryState(masterService).nodes().get(node.getId()));
    }
-
-    private static class NoOpClusterApplier implements ClusterApplier {


This class was extracted to the top level.

javanna

I had a look at the API part and it looks good, I left a couple of thoughts in there. It would be nice to add some javadocs to request and response classes. The test coverage for request, response and transport actions looks great. I know nothing about what these API do so I cannot comment on the rest ;)

javanna · 2018-10-30T16:20:00Z

...r/src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/BootstrapClusterAction.java

+
+public class BootstrapClusterAction extends Action<AcknowledgedResponse> {
+    public static final BootstrapClusterAction INSTANCE = new BootstrapClusterAction();
+    public static final String NAME = "cluster:coordination/bootstrap_cluster";


cluster should be good but coordination would introduce a new subcategory for security I think, which should be discussed probably. See https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ClusterPrivilege.java

javanna · 2018-10-30T16:21:32Z

.../src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/BootstrapClusterRequest.java

+public class BootstrapClusterRequest extends ActionRequest {
+    private BootstrapConfiguration bootstrapConfiguration;
+
+    public BootstrapClusterRequest bootstrapConfiguration(BootstrapConfiguration bootstrapConfiguration) {


nit: should we use proper setters for new requests? this was probably discussed a million times before and I may not remember what the last conclusion was, but I seem to recall that we said we would use getters and setters for new stuff. Totally unimportant though :)

javanna · 2018-10-30T16:25:26Z

...src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/GetDiscoveredNodesAction.java

+
+public class GetDiscoveredNodesAction extends Action<GetDiscoveredNodesResponse> {
+    public static final GetDiscoveredNodesAction INSTANCE = new GetDiscoveredNodesAction();
+    public static final String NAME = "cluster:monitor/discovered_nodes";


this is an existing category for security, I am not sure either if the API falls under monitor but it sounds ok to me. talk with the security team about this?

DaveCTurner · 2018-10-31T12:43:10Z

Thanks @javanna for the comments, particularly pointing out the implications of action names for security.

@andrershov would you look at the Zen2-specific parts of this PR?

ywelsch

I've left some initial thoughts.

.../src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/BootstrapClusterRequest.java

...r/src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/BootstrapConfiguration.java

...src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/GetDiscoveredNodesAction.java

...rc/main/java/org/elasticsearch/action/admin/cluster/bootstrap/GetDiscoveredNodesRequest.java

...c/main/java/org/elasticsearch/action/admin/cluster/bootstrap/GetDiscoveredNodesResponse.java

...n/java/org/elasticsearch/action/admin/cluster/bootstrap/TransportBootstrapClusterAction.java

ywelsch · 2018-11-05T11:17:54Z

...n/java/org/elasticsearch/action/admin/cluster/bootstrap/TransportBootstrapClusterAction.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class TransportBootstrapClusterAction extends TransportAction<BootstrapClusterRequest, AcknowledgedResponse> {


do we not need to register a request handler on the transport service? Do we only want to execute this action locally?

Today we only execute it locally.

I moved to using HandledTransportAction, registering the actions with the transport service, and altered the tests to make their requests via the transport service in 0a5a8a8 and 3ee0174.

...java/org/elasticsearch/action/admin/cluster/bootstrap/TransportGetDiscoveredNodesAction.java

ywelsch · 2018-11-05T11:29:17Z

server/src/main/java/org/elasticsearch/discovery/PeerFinder.java

+                    return "PeerFinder handling wakeup";
+                }
+            });
+        } catch (EsRejectedExecutionException e) {


Can you explain what kind of test failures you were seeing? I would like to avoid this wrapping here.

andrershov

I've finished the review and left no major comments. It would be nice to get @ywelsch opinion as well.

.../test/java/org/elasticsearch/action/admin/cluster/bootstrap/BootstrapConfigurationTests.java

andrershov · 2018-11-05T10:57:45Z

...n/java/org/elasticsearch/action/admin/cluster/bootstrap/TransportBootstrapClusterAction.java

+
+        if (coordinator.isInitialConfigurationSet()) {
+            logger.info("initial configuration already set, ignoring bootstrapping request");
+            listener.onResponse(new AcknowledgedResponse(false));


Basically, it means that if the first response to BootstrapClusterRequest is lost and the client sends the request for the second time, it's safe for the client, but "ack" will be set to false. I'm not sure we can call BootstrapClientRequest idempotent in this case. Probably check if initial configuration equals to requested configuration and reply with ack'ed true?

I don't think we do call it idempotent anywhere, but nonetheless idempotency is a property of the system state and not the response, so it seems ok to yield a different response (but not change the system state) on subsequent calls.

We do not remember the initial configuration anywhere so we can't check that it was the same.

@DaveCTurner I'm trying to understand how to properly write a client that makes bootstrap call. Assume the first call timeouts, it means we need to send the request once again. Now we send the request and get "ack": false in the response. Shall we retry? How do we know that cluster has been already bootstrapped?

You just retry until it returns a response. If you get a response to a request then the cluster has been bootstrapped (perhaps not by that specific request, but it doesn't matter).

However I have moved over to using a specific response type, BootstrapClusterResponse in place of AcknowledgedResponse in 759ad07.

andrershov · 2018-11-05T11:33:55Z

...a/org/elasticsearch/action/admin/cluster/bootstrap/TransportBootstrapClusterActionTests.java

+        final DiscoveryNode discoveryNode = new DiscoveryNode("local", buildNewFakeTransportAddress(), Version.CURRENT);
+        final TransportService transportService = transport.createTransportService(Settings.EMPTY, threadPool,
+            TransportService.NOOP_TRANSPORT_INTERCEPTOR, boundTransportAddress -> discoveryNode, null, emptySet());
+        final Discovery discovery = new Discovery() {


Consider using Mockito verifyZeroInteractions instead for mocks like this.

Nice, thanks.

andrershov · 2018-11-05T11:36:32Z

...n/java/org/elasticsearch/action/admin/cluster/bootstrap/TransportBootstrapClusterAction.java

+
+            final VotingConfiguration votingConfiguration = request.getBootstrapConfiguration().resolve(selfAndDiscoveredPeers);
+            logger.info("setting initial configuration to {}", votingConfiguration);
+            coordinator.setInitialConfiguration(votingConfiguration);


Also if there is a race of two requests with the same initial configuration, one of them will throw ElasticSearch exception. Idempotency?

Again, idempotency is a property of the system state and not the response, so it seems ok to yield a different response (but not change the system state) on subsequent calls.

andrershov · 2018-11-05T11:44:47Z

...java/org/elasticsearch/action/admin/cluster/bootstrap/TransportGetDiscoveredNodesAction.java

+        }
+
+        final AtomicBoolean responseSent = new AtomicBoolean();
+        final CountDownLatch countDownLatch = new CountDownLatch(1);


I like the implementation of the algorithm! Consider using CompletableFuture instead of AtomicBoolean+CountDownLatch though because it's only one concurrent primitive instead of 2.

Ah yes, thanks, I'll try that.

I used ListenableFuture in d4818d6 which solves the problems we were having with lost exceptions.

andrershov · 2018-11-05T12:00:09Z

...a/org/elasticsearch/action/admin/cluster/bootstrap/TransportBootstrapClusterActionTests.java

+
+        final TransportBootstrapClusterAction transportBootstrapClusterAction
+            = new TransportBootstrapClusterAction(Settings.EMPTY, mock(ActionFilters.class), transportService, coordinator);
+


There seems to be a lot of code duplication in the initialization part of the tests, which is not important for the test itself, can we refactor a method for intialization?

I did so in fc2f125. I'm not sure I wholly like it, because of things like the fact that we have to change the local node and rely on the transport service not picking it up until started, but I don't have a strong opinion about this.

andrershov · 2018-11-05T12:02:41Z

...org/elasticsearch/action/admin/cluster/bootstrap/TransportGetDiscoveredNodesActionTests.java

+
+        final TransportGetDiscoveredNodesAction transportGetDiscoveredNodesAction
+            = new TransportGetDiscoveredNodesAction(Settings.EMPTY, mock(ActionFilters.class), transportService, coordinator);
+


Same here can we refactor initialization part in a separate method?

I did so in fc2f125. I'm not sure I wholly like it, because of things like the fact that we have to change the local node and rely on the transport service not picking it up until started, and also we have to create a transport that responds to handshakes in all cases, but I don't have a strong opinion about this.

DaveCTurner · 2018-11-06T14:14:27Z

@andrershov @ywelsch I think I've addressed all the review points, this is ready for another look.

ywelsch

I've done a full pass and left mostly smaller comments. Looking quite good already.

...r/src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/BootstrapClusterAction.java

...src/main/java/org/elasticsearch/action/admin/cluster/bootstrap/GetDiscoveredNodesAction.java

...rc/main/java/org/elasticsearch/action/admin/cluster/bootstrap/GetDiscoveredNodesRequest.java

...c/main/java/org/elasticsearch/action/admin/cluster/bootstrap/GetDiscoveredNodesResponse.java

...n/java/org/elasticsearch/action/admin/cluster/bootstrap/TransportBootstrapClusterAction.java

...a/org/elasticsearch/action/admin/cluster/bootstrap/TransportBootstrapClusterActionTests.java

...org/elasticsearch/action/admin/cluster/bootstrap/TransportGetDiscoveredNodesActionTests.java

server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

DaveCTurner · 2018-11-07T16:12:14Z

Thanks @ywelsch, I think that's all the comments addressed.

ywelsch

LGTM

DaveCTurner added >enhancement v7.0.0 :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Oct 29, 2018

DaveCTurner commented Oct 29, 2018

View reviewed changes

DaveCTurner requested review from tlrx and removed request for tlrx October 29, 2018 12:38

DaveCTurner added 2 commits October 30, 2018 12:40

Merge branch 'zen2' into 2018-10-29-bootstrap-transport-api

c43f31f

Fix up post-merge

0cc2b20

javanna reviewed Oct 30, 2018

View reviewed changes

DaveCTurner added 10 commits October 31, 2018 11:12

Rename setters/getters

fb5c0ff

Javadocs

65e60b1

Rename setter

592c87e

Rename getter

d73638b

Rename timeout getter/setter

eb407f4

Void setters

08dd103

Javadocs for GetDiscoveredNodesRequest

3c3b35a

Javadocs for GetDiscoveredNodesResponse

53e2a79

Move action to cluster:admin

47b3718

Wrap clients for bootstraping in order to support security

5d97e23

DaveCTurner requested a review from andrershov October 31, 2018 12:43

Rename waitForNodes -> minimumNodeCount

bdbd600

DaveCTurner requested a review from ywelsch November 5, 2018 08:58

ywelsch suggested changes Nov 5, 2018

View reviewed changes

andrershov reviewed Nov 5, 2018

View reviewed changes

DaveCTurner added 3 commits November 5, 2018 14:22

Writeable not Streamable

4a73b26

Force immutability

4a5083a

Remove blank line

17e95d8

DaveCTurner requested a review from andrershov November 6, 2018 14:13

DaveCTurner added 2 commits November 6, 2018 15:51

Merge branch 'zen2' into 2018-10-29-bootstrap-transport-api

ad31d90

Merge branch 'zen2' into 2018-10-29-bootstrap-transport-api

abcecf6

ywelsch mentioned this pull request Nov 6, 2018

A new cluster coordination layer #32006

Closed

61 tasks

ywelsch reviewed Nov 6, 2018

View reviewed changes

DaveCTurner added 14 commits November 7, 2018 10:37

Use fields not getters

457cbc7

Specifically master-eligible nodes

144c1a5

Better exceptions

00a2be3

AbstractRunnable

e3637c5

Use direct executor

d3766b4

Use ActionListener.wrap

100ed18

Implement getResponseReader

2daf1b1

Javadocs for setInitialConfiguration

0f25ba2

Add TODO about adding local node to getFoundPeers

2d411a5

Per-suite threadpool

ad63cee

Parallelize bootstrap requests

8688b92

Allow default of 30 seconds for startup

276251e

Imports

2325d4b

Better messages

6d05c29

DaveCTurner requested a review from ywelsch November 7, 2018 16:11

ywelsch approved these changes Nov 7, 2018

View reviewed changes

DaveCTurner added 3 commits November 8, 2018 13:53

Merge branch 'zen2' into 2018-10-29-bootstrap-transport-api

b957940

TODO fix this

7e2b550

Compile fixes

eaba4a7

DaveCTurner merged commit 6885a7c into elastic:zen2 Nov 8, 2018

DaveCTurner deleted the 2018-10-29-bootstrap-transport-api branch November 8, 2018 16:09

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


		final TransportBootstrapClusterAction transportBootstrapClusterAction
		= new TransportBootstrapClusterAction(Settings.EMPTY, mock(ActionFilters.class), transportService, coordinator);


		final TransportGetDiscoveredNodesAction transportGetDiscoveredNodesAction
		= new TransportGetDiscoveredNodesAction(Settings.EMPTY, mock(ActionFilters.class), transportService, coordinator);

Introduce transport API for cluster bootstrapping #34961

Introduce transport API for cluster bootstrapping #34961

Conversation

DaveCTurner commented Oct 29, 2018

elasticmachine commented Oct 29, 2018

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

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

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

javanna 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 commented Oct 31, 2018

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrershov 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

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

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

Choose a reason for hiding this comment

DaveCTurner commented Nov 6, 2018

ywelsch left a comment

Choose a reason for hiding this comment

DaveCTurner commented Nov 7, 2018

ywelsch left a comment

Choose a reason for hiding this comment

DaveCTurner Nov 6, 2018 •

edited

Loading