[Zen2] Add low-level bootstrap implementation #34345

DaveCTurner · 2018-10-08T07:34:06Z

Today we inject the initial configuration of the cluster (i.e. the set of
voting nodes) at startup. In reality we must support injecting the initial
configuration after startup too. This commit adds low-level support for doing
so as safely as possible.

Today we inject the initial configuration of the cluster (i.e. the set of voting nodes) at startup. In reality we must support injecting the initial configuration after startup too. This commit adds low-level support for doing so as safely as possible.

elasticmachine · 2018-10-08T07:34:07Z

Pinging @elastic/es-distributed

DaveCTurner · 2018-10-08T08:01:04Z

@elasticmachine test this please

ywelsch

I've left some minor comments and questions. Looks very good already.

ywelsch · 2018-10-08T08:49:16Z

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

+            if (currentState.getLastAcceptedConfiguration().isEmpty() == false) {
+                throw new CoordinationStateRejectedException("Cannot set initial configuration: configuration has already been set");
+            }
+            assert currentState.term() == 0 : currentState;


Is this duplicating the checks in CoordinationState.setInitialState?

Yes, except that these checks happen earlier.

ywelsch · 2018-10-08T08:51:02Z

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

+            foundPeerIds.add(getLocalNode().getId());
+            peerFinder.getFoundPeers().forEach(peer -> foundPeerIds.add(peer.getId()));
+            if (votingConfiguration.hasQuorum(foundPeerIds) == false) {
+                throw new CoordinationStateRejectedException("Cannot set initial configuration: no quorum found yet");


maybe expand the message a bit to not enough nodes discovered yet so that they would form a quorum in the initial configuration + add the foundPeers to the message (not only the ids, but the full nodes).

Ok, I pushed 5c47f55.

ywelsch · 2018-10-08T08:51:48Z

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

+                throw new CoordinationStateRejectedException("Cannot set initial configuration: no quorum found yet");
+            }
+
+            logger.debug("setting initial configuration to {}", votingConfiguration);


I think we can log this at info level. This is a rare event, and worth an entry in the logs.

Ok, I pushed dbb2b1a

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

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

ywelsch · 2018-10-08T08:58:34Z

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

+        cluster.getAnyNode().applyInitialConfiguration();
+        cluster.stabilise(defaultMillis(
+            // the first election should succeed
+            ELECTION_INITIAL_TIMEOUT_SETTING) // TODO this wait is unnecessary, we could trigger the election immediately


why does the first election succeed?

In this case only one node has a nonempty configuration, so it's the only one that can pass through prevoting and win the election, so there's no election collisions. I expanded the comment in dcbacd8.

ywelsch · 2018-10-08T09:02:34Z

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

+                assertThat("setting initial configuration may fail with blackholed nodes", blackholedNodes, empty());
+                runFor(defaultMillis(DISCOVERY_FIND_PEERS_INTERVAL_SETTING) * 2, "discovery prior to setting initial configuration");
+                final ClusterNode bootstrapNode = getAnyNode();
+                bootstrapNode.applyInitialConfiguration();


a bootstrap API will use networking an have this thing arrive in different times on the node. Should we already simulate a short network delay here? (i.e. schedule as a task)

I would prefer not to, so we can use this to assert a tighter bound on the speed of bootstrapping. In most runs this doesn't happen (bootstrapping has already occurred at a random time in runRandomly()).

DaveCTurner

Thanks, I addressed your comments.

DaveCTurner · 2018-10-08T09:12:19Z

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

+            if (currentState.getLastAcceptedConfiguration().isEmpty() == false) {
+                throw new CoordinationStateRejectedException("Cannot set initial configuration: configuration has already been set");
+            }
+            assert currentState.term() == 0 : currentState;


Yes, except that these checks happen earlier.

DaveCTurner · 2018-10-08T09:15:03Z

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

+                assertThat("setting initial configuration may fail with blackholed nodes", blackholedNodes, empty());
+                runFor(defaultMillis(DISCOVERY_FIND_PEERS_INTERVAL_SETTING) * 2, "discovery prior to setting initial configuration");
+                final ClusterNode bootstrapNode = getAnyNode();
+                bootstrapNode.applyInitialConfiguration();


I would prefer not to, so we can use this to assert a tighter bound on the speed of bootstrapping. In most runs this doesn't happen (bootstrapping has already occurred at a random time in runRandomly()).

DaveCTurner · 2018-10-08T09:16:23Z

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

+        cluster.getAnyNode().applyInitialConfiguration();
+        cluster.stabilise(defaultMillis(
+            // the first election should succeed
+            ELECTION_INITIAL_TIMEOUT_SETTING) // TODO this wait is unnecessary, we could trigger the election immediately


In this case only one node has a nonempty configuration, so it's the only one that can pass through prevoting and win the election, so there's no election collisions. I expanded the comment in dcbacd8.

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

DaveCTurner · 2018-10-08T09:31:48Z

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

+            foundPeerIds.add(getLocalNode().getId());
+            peerFinder.getFoundPeers().forEach(peer -> foundPeerIds.add(peer.getId()));
+            if (votingConfiguration.hasQuorum(foundPeerIds) == false) {
+                throw new CoordinationStateRejectedException("Cannot set initial configuration: no quorum found yet");


Ok, I pushed 5c47f55.

DaveCTurner · 2018-10-08T09:31:56Z

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

+                throw new CoordinationStateRejectedException("Cannot set initial configuration: no quorum found yet");
+            }
+
+            logger.debug("setting initial configuration to {}", votingConfiguration);


Ok, I pushed dbb2b1a

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

ywelsch

LGTM

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

DaveCTurner · 2018-10-08T12:20:31Z

Build failed due to maven flakiness? @elasticmachine test this please.

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

DaveCTurner requested a review from ywelsch October 8, 2018 07:34

ywelsch mentioned this pull request Oct 8, 2018

A new cluster coordination layer #32006

Closed

61 tasks

ywelsch suggested changes Oct 8, 2018

View reviewed changes

DaveCTurner added 4 commits October 8, 2018 10:27

Better message in the case where a quorum has not been discovered

5c47f55

Review feedback

dbb2b1a

Set initial configuration at the start of stabilisation

ef712a5

Describe why the first election should succeed

dcbacd8

DaveCTurner commented Oct 8, 2018

View reviewed changes

ywelsch approved these changes Oct 8, 2018

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java Show resolved Hide resolved

DaveCTurner merged commit 52a3a19 into elastic:zen2 Oct 8, 2018

DaveCTurner deleted the 2018-10-07-low-level-bootstrapping branch October 8, 2018 14:56

jimczi 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

[Zen2] Add low-level bootstrap implementation #34345

[Zen2] Add low-level bootstrap implementation #34345

DaveCTurner commented Oct 8, 2018

elasticmachine commented Oct 8, 2018

DaveCTurner commented Oct 8, 2018

ywelsch left a comment

ywelsch Oct 8, 2018

DaveCTurner Oct 8, 2018

ywelsch Oct 8, 2018

DaveCTurner Oct 8, 2018

ywelsch Oct 8, 2018

DaveCTurner Oct 8, 2018

ywelsch Oct 8, 2018

DaveCTurner Oct 8, 2018

ywelsch Oct 8, 2018

DaveCTurner Oct 8, 2018

DaveCTurner left a comment

DaveCTurner Oct 8, 2018

DaveCTurner Oct 8, 2018

DaveCTurner Oct 8, 2018

DaveCTurner Oct 8, 2018

DaveCTurner Oct 8, 2018

ywelsch left a comment

DaveCTurner commented Oct 8, 2018

[Zen2] Add low-level bootstrap implementation #34345

[Zen2] Add low-level bootstrap implementation #34345

Conversation

DaveCTurner commented Oct 8, 2018

elasticmachine commented Oct 8, 2018

DaveCTurner commented Oct 8, 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

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

ywelsch left a comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 8, 2018