DISCOVERY: Fix RollingUpgradeTests #35375

original-brownbear · 2018-11-08T14:33:52Z

Don't manually manage min master nodes
Regarding the issue asking to use file based discovery: This already happens. We are passing the transportURIs in a hacky way via the node property and they are written to the discovery file in 6.5 and newer so I don't think we have to do anything about that here.
Remove some dead code
Closes Fix discovery in rolling upgrade tests #35178

* Don't manually manage min master nodes * Remove some dead code * Closes elastic#35178

elasticmachine · 2018-11-08T14:33:54Z

Pinging @elastic/es-distributed

DaveCTurner

Looks great. Just a few minor nits.

DaveCTurner · 2018-11-08T16:28:10Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterConfiguration.groovy

+     * A closure to call which returns a manually supplied list of unicast seed hosts.
+     */
+    @Input
+    Closure<List<String>> manualSeedUris = {


How about otherUnicastHostAddresses (they're not really URIs, and "seed" isn't quite the right word either)

DaveCTurner · 2018-11-08T16:29:43Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy

+                List<String> manualSeedUris = node.config.manualSeedUris.call()
+                if (manualSeedUris.empty == false) {
+                    unicastHosts.addAll(manualSeedUris)
+                } else {


I think the unicast hosts file should be the union of these, so no need for the else.

I think I agree, unfortunately this forces us to manually set min master nodes (just tried it out). The reason being that the nodes that we have join the cluster effectively think they're in a 1 node cluster => if we add the node itself to the unicast hosts here it blows up => I'll add the min master nodes setting back but set it to 2 maybe so it's more realistic + make it merge the lists here?

We can still drop the min master node setting for the initial oldCluster though :)

Bah of course, it's a different cluster from Gradle's point of view, so yes we need to set minimum_master_nodes on the new nodes.

I'm surprised that adding the node to its own unicast hosts list makes a difference, I think this would be broken either way:

elasticsearch/server/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

Lines 213 to 214 in 93c2c60

// no point in pinging ourselves

if (localAddresses.contains(address) == false) {

Hmm it did work when I ran it locally :D but yea, set them to 2 now on the 1 node "clusters" and it passed fine locally.

DaveCTurner · 2018-11-08T16:43:32Z

qa/rolling-upgrade/build.gradle

@@ -77,8 +76,7 @@ for (Version version : bwcVersions.wireCompatible) {
    configure(extensions.findByName("${baseName}#${name}")) {
      dependsOn lastRunner, "${baseName}#oldClusterTestCluster#node${stopNode}.stop"
      clusterName = 'rolling-upgrade'
-      unicastTransportUri = { seedNode, node, ant -> unicastSeed() }
-      minimumMasterNodes = { 3 }
+      manualSeedUris = { unicastSeed() }


Also would prefer to avoid Seed (in unicastSeed) here 🌱

DaveCTurner · 2018-11-08T16:43:40Z

x-pack/qa/rolling-upgrade-basic/build.gradle

@@ -58,8 +54,7 @@ for (Version version : bwcVersions.wireCompatible) {
        configure(extensions.findByName("${baseName}#${name}")) {
            dependsOn lastRunner, "${baseName}#oldClusterTestCluster#node${stopNode}.stop"
            clusterName = 'rolling-upgrade-basic'
-            unicastTransportUri = { seedNode, node, ant -> unicastSeed() }
-            minimumMasterNodes = { 3 }
+            manualSeedUris = { unicastSeed() }


Also would prefer to avoid Seed (in unicastSeed) here 🌱

DaveCTurner · 2018-11-08T16:43:48Z

x-pack/qa/rolling-upgrade/build.gradle

@@ -188,8 +186,7 @@ subprojects {
        dependsOn lastRunner, "${baseName}#oldClusterTestCluster#node${stopNode}.stop"
        setupCommand 'setupTestUser', 'bin/elasticsearch-users', 'useradd', 'test_user', '-p', 'x-pack-test-password', '-r', 'superuser'
        clusterName = 'rolling-upgrade'
-        unicastTransportUri = { seedNode, node, ant -> unicastSeed() }
-        minimumMasterNodes = { 3 }
+        manualSeedUris = { unicastSeed() }


Also would prefer to avoid Seed (in unicastSeed) here 🌱

original-brownbear · 2018-11-08T19:03:25Z

@DaveCTurner Thanks for the review! => All suggestions applied (see me comment on the min master nodes though).

DaveCTurner · 2018-11-08T19:31:59Z

All suggestions applied

Except for the ones about losing Seed from unicastSeed. How about getOtherUnicastHostAddresses?

original-brownbear · 2018-11-08T19:33:49Z

@DaveCTurner ah sure will rename to getOtherUnicastHostAddresses (sorry missed that one since it was named like that before) as well.

original-brownbear · 2018-11-08T19:39:23Z

@DaveCTurner renamed to getOtherUnicastHostAddresses :)

DaveCTurner

LGTM thanks @original-brownbear.

original-brownbear · 2018-11-09T06:27:16Z

Jenkins test this

original-brownbear · 2018-11-09T09:47:11Z

@DaveCTurner thanks for the review!

* The logic that splits by `","` isn't necessary anymore since elastic#35375 removed the hack that set the comma separated list for the unicast host

* The logic that splits by `","` isn't necessary anymore since #35375 removed the hack that set the comma separated list for the unicast host

* DISCOVERY: Fix RollingUpgradeTests * Don't manually manage min master nodes if not necessary * Remove some dead code * Allow for manually supplying list of seed nodes * Closes elastic#35178

* DISCOVERY: Fix RollingUpgradeTests * Don't manually manage min master nodes if not necessary * Remove some dead code * Allow for manually supplying list of seed nodes * Closes #35178

DISCOVERY: Fix RollingUpgradeTests

1d11b55

* Don't manually manage min master nodes * Remove some dead code * Closes elastic#35178

original-brownbear added >non-issue v7.0.0 :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v6.5.0 labels Nov 8, 2018

original-brownbear requested a review from DaveCTurner November 8, 2018 14:33

original-brownbear added 2 commits November 8, 2018 17:16

CR: Allow for manually supplying list of seed nodes

c033c74

typo

319c45b

DaveCTurner reviewed Nov 8, 2018

View reviewed changes

original-brownbear added 2 commits November 8, 2018 19:31

Merge remote-tracking branch 'elastic/master' into 35178-2

e2678ef

CR: Renamings + merging unicast hosts lists

358fb42

CR: Renaming

f79e049

DaveCTurner approved these changes Nov 8, 2018

View reviewed changes

Merge remote-tracking branch 'elastic/master' into 35178-2

f3155af

original-brownbear merged commit 529910a into elastic:master Nov 9, 2018

original-brownbear deleted the 35178-2 branch November 9, 2018 09:47

original-brownbear added the backport pending label Nov 9, 2018

original-brownbear mentioned this pull request Nov 13, 2018

DISCOVERY: 0s Initial State Timeout in Tests #35481

Merged

original-brownbear mentioned this pull request Nov 13, 2018

MINOR: Remove Hack in TestCluster Task #35485

Merged

original-brownbear added a commit that referenced this pull request Nov 13, 2018

MINOR: Remove Hack in TestCluster Task (#35485)

3db671e

* The logic that splits by `","` isn't necessary anymore since #35375 removed the hack that set the comma separated list for the unicast host

original-brownbear mentioned this pull request Nov 15, 2018

MINOR: Remove SNAPSHOT Suffixes in Gradle #35577

Merged

original-brownbear removed the backport pending label Nov 15, 2018

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

	// no point in pinging ourselves
	if (localAddresses.contains(address) == false) {

DISCOVERY: Fix RollingUpgradeTests #35375

DISCOVERY: Fix RollingUpgradeTests #35375

Uh oh!

Conversation

original-brownbear commented Nov 8, 2018

Uh oh!

elasticmachine commented Nov 8, 2018

Uh oh!

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

original-brownbear commented Nov 8, 2018

Uh oh!

DaveCTurner commented Nov 8, 2018

Uh oh!

original-brownbear commented Nov 8, 2018

Uh oh!

original-brownbear commented Nov 8, 2018

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Nov 9, 2018

Uh oh!

original-brownbear commented Nov 9, 2018

Uh oh!

Uh oh!