Zen2: Add Cluster State Applier #34257

ywelsch · 2018-10-03T08:23:05Z

Adds the cluster state applier to Coordinator, and adds tests for cluster state acking.

elasticmachine · 2018-10-03T08:23:07Z

Pinging @elastic/es-distributed

ywelsch · 2018-10-03T14:04:16Z

Build failed when checking out 6.x. I'll merge latest zen2 branch into this. All the testing etc. looks good o.w.

DaveCTurner

We discussed this in another channel and decided it'd be good to remove currentPublication and come up with a better way to make the assertions that it helps us make.

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

DaveCTurner · 2018-10-03T15:45:08Z

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

@@ -433,7 +485,8 @@ public void invariant() {

                final Set<DiscoveryNode> knownFollowers = followersChecker.getKnownFollowers();
                final Set<DiscoveryNode> lastPublishedNodes = new HashSet<>();
-                if (becomingMaster == false || publicationInProgress()) {
+                if (becomingMaster == false ||
+                    (publicationInProgress() && getCurrentTerm() == currentPublication.get().publishedState().term())) {


Hmm this is a surprising addition. How can we have a publication in progress whose term doesn't match our current term? That seems bad. I also had issues with this assertion in the vicinity of becoming a master, perhaps we should rethink this?

I've rewritten the assertions around currentPublication (see b77e5ab). I think we still need currentPublication for the term bumping though.

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

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

DaveCTurner

I left a few more requests, and there's an assertion that doesn't work correctly.

Also to get around the lack of term bumping I added this (plus imports and making things package-visible):

diff --git a/server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java b/server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java
index b714f9a9530..42d321ea303 100644
--- a/server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java
+++ b/server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java
@@ -597,6 +597,23 @@ public class CoordinatorTests extends ESTestCase {

             runUntil(stabilisationEndTime);

+            // TODO remove when term-bumping is enabled
+            final long maxTerm = clusterNodes.stream().map(n -> n.coordinator.getCurrentTerm()).max(Long::compare).orElse(0L);
+            final long maxLeaderTerm = clusterNodes.stream().filter(n -> n.coordinator.getMode() == Mode.LEADER)
+                .map(n -> n.coordinator.getCurrentTerm()).max(Long::compare).orElse(0L);
+
+            if (maxLeaderTerm < maxTerm) {
+                logger.info("--> forcing a term bump, maxTerm={}, maxLeaderTerm={}", maxTerm, maxLeaderTerm);
+                final ClusterNode leader = getAnyLeader();
+                synchronized (leader.coordinator.mutex) {
+                    leader.coordinator.ensureTermAtLeast(leader.localNode, maxTerm + 1);
+                }
+                leader.coordinator.startElection();
+                final long termBumpEndTime = stabilisationEndTime + DEFAULT_ELECTION_DELAY;
+                logger.info("--> re-stabilising after term bump until [{}ms]", termBumpEndTime);
+                runUntil(termBumpEndTime);
+            }
+
             assertUniqueLeaderAndExpectedModes();
         }

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

DaveCTurner · 2018-10-04T08:23:46Z

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

+        private final ListenableFuture<Void> localNodeAckEvent;
+        private final AckListener ackListener;
+        private final ActionListener<Void> publishListener;
+        private boolean completed;


I think this has the same semantics as isCompleted in the base Publication class. Could we just reuse that?

DaveCTurner · 2018-10-04T08:24:28Z

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

+            }, EsExecutors.newDirectExecutorService());
+        }
+
+        boolean isActive() {


Perhaps isIncomplete to align with the "completed" nomenclature?

DaveCTurner · 2018-10-04T08:42:56Z

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

+
+                if (becomingMaster && activePublicationInProgress() == false) {
+                    // cluster state update task to become master is submitted to MasterService, but publication has not started yet
+                    assert followersChecker.getKnownFollowers().isEmpty() : followersChecker.getKnownFollowers();


This assertion fails with ./gradlew :server:test -Dtests.class=org.elasticsearch.cluster.coordination.CoordinatorTests -Dtests.method=testFollowerDisconnectionDetectedQuickly -Dtests.seed=396E71ACBA99321E:9198D7E2D2FC9784 -Dtests.jvm.argline=-Dhppc.bitmixer=DETERMINISTIC - a publication that times out before being accepted locally results in becomingMaster (the last-accepted term is wrong) and activePublicationInProgress() == false, but by this point the followersChecker is active.

This is essentially why I had to do this in #34241: 34eb71b

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

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

DaveCTurner · 2018-10-04T09:41:15Z

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

-                    // TODO instead assert that knownFollowers is updated appropriately at the end of each publication
+                    assert lastPublishedNodes.remove(getLocalNode()); // followersChecker excludes local node
+                    assert lastPublishedNodes.equals(followersChecker.getKnownFollowers()) :
+                        lastPublishedNodes + " != " + followersChecker.getKnownFollowers();


This assertion fails:

:server:test -Dtests.class=org.elasticsearch.cluster.coordination.CoordinatorTests -Dtests.method=testUnresponsiveLeaderDetectedEventually -Dtests.seed=B39FA830E397231C:E88D9536D29A794 -Dtests.jvm.argline=-Dhppc.bitmixer=DETERMINISTIC

(I have some local modifications so may not reproduce exactly)

The reason is the same as in https://github.com/elastic/elasticsearch/pull/34257/files#r222583570: a publication is committed but times out before being accepted locally. Here it's a commit that's adding a node after the cluster formed, so the known followers are updated at the start of the publication but the last-accepted state doesn't reflect this.

After some back and forth, I think I've finally fixed this in 84a69c1

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

DaveCTurner

LGTM, and to a few hundred iterations of the tests too. I left a few optional nits.

DaveCTurner · 2018-10-04T14:56:35Z

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

@@ -385,18 +420,28 @@ DiscoveryNode getLocalNode() {
    }

    // package-visible for testing
-    boolean publicationInProgress() {
+    boolean activePublicationInProgress() {


Think this is just publicationInProgress() again now.

DaveCTurner · 2018-10-04T15:00:38Z

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

@@ -461,6 +516,7 @@ public void invariant() {
                assert leaderChecker.currentNodeIsMaster() == false;
                assert leaderCheckScheduler != null;
                assert followersChecker.getKnownFollowers().isEmpty();
+                assert currentPublication.map(Publication::isCommitted).orElse(true);


Nice. I think this is a strong enough link between currentPublication and mode.

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

ywelsch · 2018-10-04T16:00:30Z

@elasticmachine retest this please

From experience with elastic#34257, here are a few things that help with analysing logs from test runs. Also we prevent trying to stabilise a cluster with raised delay variability, because lowering the delay variability requires time to allow all the extra-varied-scheduled tasks to work their way out of the system.

From experience with #34257, here are a few things that help with analysing logs from test runs. Also we prevent trying to stabilise a cluster with raised delay variability, because lowering the delay variability requires time to allow all the extra-varied-scheduled tasks to work their way out of the system.

Hook in Cluster State Applier

54632b6

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

ywelsch requested a review from DaveCTurner October 3, 2018 08:23

ywelsch mentioned this pull request Oct 3, 2018

[Zen2] Add safety phase to CoordinatorTests #34241

Merged

ywelsch added 2 commits October 3, 2018 12:07

unused import

4813b3e

fix assertion

f8d1057

Merge branch 'zen2' into applier-acking

2225704

DaveCTurner reviewed Oct 3, 2018

View reviewed changes

server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java Outdated Show resolved Hide resolved

ywelsch added 8 commits October 3, 2018 22:12

add comment

15a4da8

move assertion

42451ba

move anonymous to inner class

8c846dc

remove extra assert

8fecbd6

clarify assertions around currentPublication

b77e5ab

remove superfluous parameter to stabilize

a7c286c

Merge branch 'zen2' into applier-acking

7274f7b

add safety phase to acking tests

00938c2

ywelsch requested a review from DaveCTurner October 4, 2018 07:17

ywelsch mentioned this pull request Oct 4, 2018

A new cluster coordination layer #32006

Closed

61 tasks

DaveCTurner reviewed Oct 4, 2018

View reviewed changes

ywelsch added 4 commits October 4, 2018 11:42

Separate logger for PublicationTransportHandler

cd22d4a

deduplicate completed field

c59ef80

NoOpClusterApplier

a6739a9

connect -> heal

ca833e0

DaveCTurner reviewed Oct 4, 2018

View reviewed changes

server/src/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java Show resolved Hide resolved

ywelsch added 5 commits October 4, 2018 14:09

revisit currentPublication check

627d68b

remove lastCommittedState

ff009b6

add term-bump hack

cc10b84

Adapt DEFAULT_CLUSTER_STATE_UPDATE_DELAY

e2a4f41

refine activePublication check

84a69c1

ywelsch requested a review from DaveCTurner October 4, 2018 13:22

DaveCTurner approved these changes Oct 4, 2018

View reviewed changes

misc review feedback

a773288

ywelsch merged commit b32abcb into elastic:zen2 Oct 4, 2018

DaveCTurner mentioned this pull request Oct 4, 2018

[Zen2] Minor housekeeping of tests #34315

Merged

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

Zen2: Add Cluster State Applier #34257

Zen2: Add Cluster State Applier #34257

ywelsch commented Oct 3, 2018

elasticmachine commented Oct 3, 2018

ywelsch commented Oct 3, 2018

DaveCTurner left a comment

DaveCTurner Oct 3, 2018

ywelsch Oct 4, 2018

DaveCTurner left a comment

DaveCTurner Oct 4, 2018

DaveCTurner Oct 4, 2018

DaveCTurner Oct 4, 2018

DaveCTurner Oct 4, 2018

ywelsch Oct 4, 2018

DaveCTurner left a comment

DaveCTurner Oct 4, 2018

ywelsch Oct 4, 2018

DaveCTurner Oct 4, 2018

ywelsch commented Oct 4, 2018

Zen2: Add Cluster State Applier #34257

Zen2: Add Cluster State Applier #34257

Conversation

ywelsch commented Oct 3, 2018

elasticmachine commented Oct 3, 2018

ywelsch commented Oct 3, 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

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

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

ywelsch commented Oct 4, 2018