TESTS: Use File Based Discovery in REST Tests #34560

original-brownbear · 2018-10-17T14:38:17Z

For 7.0 use file based discovery in REST tests
Relates Fix port assignment and discovery in tests #33675

* For 7.0 use file based discovery in REST tests * Relates elastic#33675

elasticmachine · 2018-10-17T14:38:27Z

Pinging @elastic/es-distributed

original-brownbear · 2018-10-17T14:39:12Z

@DaveCTurner I hope this is what we want here :)
Also, should I do anything about pre-7.x here? (I guess that would require using the file based discovery plugin in older versions?)

DaveCTurner

File-based discovery is available without a plugin in 6.x so I think once 6.5.0 is released we can do this (in master) without worrying about versions. We should wait.

Also, with this change I think the whole concept of a "seed" node can go away.

Also, with this change I think we can undo the changes in #27344 and go back to a more reasonable setting for minimum_master_nodes.

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

…scovery

DaveCTurner · 2018-10-17T15:50:57Z

Also, should I do anything about pre-7.x here? (I guess that would require using the file based discovery plugin in older versions?)

I'm not sure - how easy is it to install a plugin for these tests? If it's easy, I think we should try and make an equivalent change to 6.x that does indeed involve installing the discovery-file plugin before starting these nodes.

original-brownbear · 2018-10-17T16:00:17Z

@DaveCTurner I think there is logic that helps with installing plugins in this code but I would need to research this a little to see how tricky it is to actually use it.
Are you suggesting otherwise generally blocking the work here until 6.5.0 is out?

DaveCTurner · 2018-10-17T16:31:32Z

Are you suggesting otherwise generally blocking the work here until 6.5.0 is out?

Yes.

DaveCTurner · 2018-10-17T17:18:24Z

Are you suggesting otherwise generally blocking the work here until 6.5.0 is out?

Yes.

Wrong again! I thought we tested master against 6.4 as well as 6.x but apparently not. This PR can proceed, sorry.

original-brownbear · 2018-10-17T18:36:08Z

@DaveCTurner no worries :) I'm back on it then

…scovery

original-brownbear · 2018-10-17T19:05:15Z

@DaveCTurner alright, that was a confusing one (sorry for missing that 6.x also supports the file based discovery out of the box :D) => Removed the condition for the version now. Take another look when you have a sec :)

…scovery

…igured

original-brownbear · 2018-10-18T09:38:09Z

@DaveCTurner argh :D see Jenkins log ... we do run against 6.4.x-snapshot => back to waiting it out here? :)

original-brownbear · 2018-10-18T15:30:38Z

@DaveCTurner alright back to plan A as discussed: Made it only use file based discovery for 6.5+ now. Should be good for another review :)

…scovery

original-brownbear · 2018-10-19T12:23:51Z

@DaveCTurner finally green :)

DaveCTurner

Neat! Can we lose the start-one-node-then-start-the-rest pattern for 6.5.0+ clusters? I asked for a handful of minor changes too.

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

DaveCTurner · 2018-10-19T14:35:51Z

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

+
+            nodes.forEach { node ->
+                Collection<String> unicastHosts = new HashSet<>()
+                nodes.forEach { otherNode ->


Does this need to be nested? Could we calculate it once up-front and then write all the hosts files?

Wouldn't we want to not have self-references in these (I guess it doesn't matter), but in that case we actually need additional code to account for the one self-reference that will return null in that loop.
I just figured this was the shortest solution? :)

Ok, more seed node weirdness. I think you should up-front call unicastTransportUri(node, null, project.ant) in order to read the transport address of every node, and then put all of them into all of the unicast_hosts.txt files. It's normal for all the nodes to have the same list of unicast hosts, even though one of them refers to itself.

DaveCTurner · 2018-10-19T14:37:08Z

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

@@ -703,6 +724,7 @@ class ClusterFormationTasks {
                    }
                }
            }
+


You could make this PR 4% smaller by removing this line again ;)

DaveCTurner

(copied from previous review): Also, with this change I think we can undo the changes in #27344 and go back to a more reasonable setting for minimum_master_nodes.

…scovery

…t in 6.5.0+

original-brownbear · 2018-10-20T14:13:13Z

@DaveCTurner alright all handled I think:

The waiting for the seed node was easy to fix functionally in 391a8a6 (just don't add the task dependency to new version nodes). I didn't refactor the code for it a great deal because of the last point below

The precomputing the unicast hosts and setting [] for the unicast hosts in the missing spots is in 2052e0c

The situation with the minimum master nodes setting I don't think I can easily fix here. For the BwC nodes this will still spell trouble because we're still using the old way of setting the unicast hosts to the first node for those and nothing has changed for them? => I didn't make any changes to that and also figured there wasn't too much point in a larger code change to separate the paths for older than 6.5 and newer than 6.5 until we had a plan for that. Maybe add a new issue (or bullet point in the existing one :)) for that and look for a clean refactoring and fix for the pre-6.5 situation there?

original-brownbear · 2018-10-20T17:11:49Z

Jenkins test this

…scovery

…of masters lowered in 6.5+

original-brownbear · 2018-10-22T15:16:56Z

@DaveCTurner looks like GH finally refreshed :) This should be good for another review now.

…scovery

DaveCTurner

I'm happy with this (except the longer timeout, see comment). However CI is not. We looked into it together and suspected that there's something wrong with how the discovery plugin tests work. If so, and if it fixes it, I'd be ok with reinstating the discovery.zen.ping.unicast.hosts lists in those cases and then following up with a proper fix as a separate PR.

DaveCTurner · 2018-10-23T13:35:43Z

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

@@ -131,7 +137,7 @@ class ClusterConfiguration {
     * condition is executed.
     */
    @Input
-    int nodeStartupWaitSeconds = 30
+    int nodeStartupWaitSeconds = 120


I'd rather not do this - I think there's something wrong with the discovery plugin tests.

original-brownbear · 2018-10-23T14:25:03Z

@DaveCTurner it seems the normal discovery mechanism interfering (or rather it not doing so) wasn't the issue here.
We're making all the nodes return null for their transport endpoint in

elasticsearch/plugins/discovery-ec2/qa/amazon-ec2/build.gradle

Line 59 in ca6808e

unicastTransportUri = { seedNode, node, ant -> return null }

=> still at it.
Looking into the changed dependency order now.

…scovery

DaveCTurner

LGTM

* For `6.5+` use file based discovery in REST tests * Relates #33675

* For `6.5+` use file based discovery in REST tests * Relates elastic#33675

* For `6.5+` use file based discovery in REST tests * Relates #33675

TESTS: Use File Based Discovery in REST Tests

5d9d619

* For 7.0 use file based discovery in REST tests * Relates elastic#33675

original-brownbear added >test Issues or PRs that are addressing/adding tests v7.0.0 :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Oct 17, 2018

original-brownbear requested a review from DaveCTurner October 17, 2018 14:38

DaveCTurner requested changes Oct 17, 2018

View reviewed changes

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy Outdated Show resolved Hide resolved

DaveCTurner mentioned this pull request Oct 17, 2018

Fix port assignment and discovery in tests #33675

Closed

6 tasks

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

48decc7

…scovery

DaveCTurner mentioned this pull request Oct 17, 2018

REST test against clusters with dedicated master nodes #34563

Open

original-brownbear added 2 commits October 17, 2018 20:39

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

50a8033

…scovery

Use file based discovery in 6.x as well

cd50d89

original-brownbear added 2 commits October 18, 2018 07:32

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

a3128ac

…scovery

Don't use file based discovery when other discovery mechanism is conf…

ca832d2

…igured

original-brownbear added 2 commits October 18, 2018 12:17

Merge branch 'master' into rest-tests-file-discovery

dcf5625

only use file based discovery in 6.5+

8ba68ce

original-brownbear added 2 commits October 19, 2018 10:56

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

3e0251c

…scovery

Fix unicast hosts returned as csv erring out and order of ready checks

48d2c87

DaveCTurner requested changes Oct 19, 2018

View reviewed changes

original-brownbear added 3 commits October 20, 2018 15:32

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

a3b9e55

…scovery

CR: only compute unicast hosts txt once

2052e0c

CR: Don't depend on the seed node starting up for other nodes to star…

391a8a6

…t in 6.5.0+

original-brownbear added 2 commits October 22, 2018 13:03

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

fca6c93

…scovery

CR: Different code paths for 6.5+ and older versions, minimum number …

33f044a

…of masters lowered in 6.5+

original-brownbear added 3 commits October 23, 2018 05:20

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

7d32ba4

…scovery

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

61bc8ad

…scovery

increase timeout

f641b82

DaveCTurner reviewed Oct 23, 2018

View reviewed changes

original-brownbear added 6 commits October 23, 2018 17:45

TEST: Make other nodes depend on first node

97420c5

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

5c332d0

…scovery

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

ed3b328

…scovery

Merge remote-tracking branch 'elastic/master' into rest-tests-file-di…

9a9bf8d

…scovery

back to correct task order

ac02a81

try reverting master node count change

622231d

DaveCTurner approved these changes Oct 24, 2018

View reviewed changes

original-brownbear merged commit 0a85997 into elastic:master Oct 24, 2018

original-brownbear deleted the rest-tests-file-discovery branch October 24, 2018 15:18

kcm pushed a commit that referenced this pull request Oct 30, 2018

TESTS: Use File Based Discovery in REST Tests (#34560)

b3d2cfc

* For `6.5+` use file based discovery in REST tests * Relates #33675

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Nov 14, 2018

TESTS: Use File Based Discovery in REST Tests (elastic#34560)

15fc185

* For `6.5+` use file based discovery in REST tests * Relates elastic#33675

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

TESTS: Use File Based Discovery in REST Tests (#34560)

21acf31

* For `6.5+` use file based discovery in REST tests * Relates #33675

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

original-brownbear restored the rest-tests-file-discovery branch January 6, 2021 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TESTS: Use File Based Discovery in REST Tests #34560

TESTS: Use File Based Discovery in REST Tests #34560

original-brownbear commented Oct 17, 2018

elasticmachine commented Oct 17, 2018

original-brownbear commented Oct 17, 2018

DaveCTurner left a comment

DaveCTurner commented Oct 17, 2018

original-brownbear commented Oct 17, 2018

DaveCTurner commented Oct 17, 2018

DaveCTurner commented Oct 17, 2018

original-brownbear commented Oct 17, 2018

original-brownbear commented Oct 17, 2018

original-brownbear commented Oct 18, 2018

original-brownbear commented Oct 18, 2018

original-brownbear commented Oct 19, 2018

DaveCTurner left a comment

DaveCTurner Oct 19, 2018

original-brownbear Oct 19, 2018

DaveCTurner Oct 19, 2018

DaveCTurner Oct 19, 2018

DaveCTurner left a comment

original-brownbear commented Oct 20, 2018 •

edited

Loading

original-brownbear commented Oct 20, 2018

original-brownbear commented Oct 22, 2018

DaveCTurner left a comment

DaveCTurner Oct 23, 2018

original-brownbear commented Oct 23, 2018

DaveCTurner left a comment

@@ @@ -703,6 +724,7 @@ class ClusterFormationTasks { @@
                                   }
                               }
                           }

TESTS: Use File Based Discovery in REST Tests #34560

TESTS: Use File Based Discovery in REST Tests #34560

Conversation

original-brownbear commented Oct 17, 2018

elasticmachine commented Oct 17, 2018

original-brownbear commented Oct 17, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 17, 2018

original-brownbear commented Oct 17, 2018

DaveCTurner commented Oct 17, 2018

DaveCTurner commented Oct 17, 2018

original-brownbear commented Oct 17, 2018

original-brownbear commented Oct 17, 2018

original-brownbear commented Oct 18, 2018

original-brownbear commented Oct 18, 2018

original-brownbear commented Oct 19, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner Oct 19, 2018

Choose a reason for hiding this comment

original-brownbear Oct 19, 2018

Choose a reason for hiding this comment

DaveCTurner Oct 19, 2018

Choose a reason for hiding this comment

DaveCTurner Oct 19, 2018

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

original-brownbear commented Oct 20, 2018 • edited Loading

original-brownbear commented Oct 20, 2018

original-brownbear commented Oct 22, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner Oct 23, 2018

Choose a reason for hiding this comment

original-brownbear commented Oct 23, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

original-brownbear commented Oct 20, 2018 •

edited

Loading