Convert RunTask to use testclusers, remove ClusterFormationTasks #47572

alpar-t · 2019-10-04T12:20:36Z

This PR adds a new RunTask and a way for it to start a
testclusters cluster out of band and block on it to replace
the old RunTask that used ClusterFormationTasks.

Note that the new RunTask starts existing clusters, it doesn't have a configuration of it's own, so we can avoid duplicating the configuration in a few places.

With this we can now remove ClusterFormationTasks.

We could consider adding a Gradle convention so there's a task to run any cluster in the build.

This PR adds a new RunTask and a way for it to start a testclusters cluster out of band and block on it to replace the old RunTask that used ClusterFormationTasks. With this we can now remove ClusterFormationTasks.

elasticmachine · 2019-10-04T12:20:38Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

mark-vieira · 2019-10-04T22:01:05Z

The elasticsearch-hadoop project relies on ClusterFormationTasks directly. We need to ensure they are migrated to test clusters before we merge this or we'll break their builds.

mark-vieira

So glad to finally be removing the legacy cluster handling code.

Left some comments that I think we should address.

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/RunTask.java

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchCluster.java

mark-vieira · 2019-10-04T22:24:46Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

@@ -631,7 +633,9 @@ private void runElaticsearchBinScript(String tool, String... args) {

    private Map<String, String> getESEnvironment() {
        Map<String, String> defaultEnv = new HashMap<>();
-        defaultEnv.put("JAVA_HOME", getJavaHome().getAbsolutePath());
+        if ( getJavaHome() != null) {


nit: looks like we have an extra space here

mark-vieira · 2019-10-04T22:34:25Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

@@ -147,6 +147,9 @@
    private volatile Process esProcess;
    private Function<String, String> nameCustomization = Function.identity();
    private boolean isWorkingDirConfigured = false;
+    private String httpPort = "0";
+    private String transportPort = "0";
+    private boolean logOnConsole = false;


How about logToConsole?

mark-vieira · 2019-10-04T22:35:18Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

@@ -1273,6 +1282,16 @@ void configureHttpWait(WaitForHttpResource wait) {
        }
    }

+    void configureForRun() {


I think I'd prefer these to just be exposed as settings which the RunTask explicitly sets rather than coupling ElasticsearchNode to RunTask in this way.

mark-vieira · 2019-10-04T22:38:16Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/RunTask.java

+    public void runAndWait() {
+        cluster.freeze();
+        cluster.run(debug);
+        while (Thread.currentThread().isInterrupted() == false) {


Is this even necessary since calling run() in turn calls Process.waitFor()?

mark-vieira · 2019-10-04T22:43:04Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/RunTask.java

+    }
+
+    public void setCluster(ElasticsearchCluster cluster) {
+        TestClustersAware.prepareClusterForUse(getProject(), this, cluster);


We could avoid this nastiness if we just had this task implement TestClustersAware itself.

alpar-t

Thanks for the review @mark-vieira
I did want to use the existing life-cycle management initially,
this is now possible with the introduction of beforeStart to give the chance to the task to reconfigure the cluster right before starting it.
I'm much happier with how this came out.

mark-vieira

This looks much cleaner after refactoring. Couple of items though.

mark-vieira · 2019-10-07T21:49:22Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersAware.java

            dependsOn(distro.getExtracted())
        );
        getClusters().add(cluster);
    }
+
+    default void beforeStart() {


Perhaps I'm missing something but I can't find anywhere where this is actually called.

Good catch, it wasn't and I didn't test all use-cases so didn't notice it. It's fixed now.

mark-vieira · 2019-10-07T21:51:37Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/RunTask.java

+        int port = 8000;
+        for (ElasticsearchCluster cluster : getClusters()) {
+            for (ElasticsearchNode node : cluster.getNodes()) {
+                node.setHttpPort("9200");


Won't this fail to bind for clusters w/ > 1 node? We seem to be incrementing the debug port, should we do the same for the HTTP and transport ports?

Indeed, I added port increment.

alpar-t · 2019-10-08T05:45:32Z

run elasticsearch-ci/packaging-sample

) * Convert RunTask to use testclusers, remove ClusterFormationTasks This PR adds a new RunTask and a way for it to start a testclusters cluster out of band and block on it to replace the old RunTask that used ClusterFormationTasks. With this we can now remove ClusterFormationTasks.

…mationTasks (#47572)" This reverts the removal of the ClusterFormationTaks from commit 36d018c so they are usable for a bit longer in the hadoop build.

Convert RunTask to use testclusers, remove ClusterFormationTasks

3f1cde2

This PR adds a new RunTask and a way for it to start a testclusters cluster out of band and block on it to replace the old RunTask that used ClusterFormationTasks. With this we can now remove ClusterFormationTasks.

alpar-t added :Delivery/Build Build or test infrastructure v8.0.0 v7.5.0 labels Oct 4, 2019

alpar-t requested review from rjernst and mark-vieira October 4, 2019 12:20

mark-vieira reviewed Oct 4, 2019

View reviewed changes

alpar-t added 2 commits October 7, 2019 17:20

Merge remote-tracking branch 'origin/master' into convert-run-task

b7fc2da

PR review

3f14e61

alpar-t commented Oct 7, 2019

View reviewed changes

mark-vieira approved these changes Oct 7, 2019

View reviewed changes

alpar-t added 2 commits October 8, 2019 11:56

PR review

0f345df

Merge remote-tracking branch 'origin/master' into convert-run-task

95b8115

alpar-t merged commit 8f78554 into elastic:master Oct 8, 2019

alpar-t deleted the convert-run-task branch October 8, 2019 11:40

mark-vieira mentioned this pull request Oct 11, 2019

Remove usages of ClusterFormationTasks and replace with test clusters elastic/elasticsearch-hadoop#1365

Merged

mark-vieira mentioned this pull request Oct 15, 2019

Create test superuser when launching elasticsearch via 'run' task #48072

Merged

zhongnansu mentioned this pull request Mar 13, 2020

Elasticsearch 7.6.1 compatibility opendistro-for-elasticsearch/sql#376

Closed

ftianli-amzn mentioned this pull request Mar 16, 2020

Elasticsearch 7.6.1 compatibility opendistro-for-elasticsearch/alerting#179

Closed

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert RunTask to use testclusers, remove ClusterFormationTasks #47572

Convert RunTask to use testclusers, remove ClusterFormationTasks #47572

alpar-t commented Oct 4, 2019

elasticmachine commented Oct 4, 2019

mark-vieira commented Oct 4, 2019

mark-vieira left a comment

mark-vieira Oct 4, 2019

mark-vieira Oct 4, 2019

mark-vieira Oct 4, 2019

mark-vieira Oct 4, 2019

mark-vieira Oct 4, 2019

alpar-t left a comment

mark-vieira left a comment

mark-vieira Oct 7, 2019

alpar-t Oct 8, 2019

mark-vieira Oct 7, 2019

alpar-t Oct 8, 2019

alpar-t commented Oct 8, 2019

Convert RunTask to use testclusers, remove ClusterFormationTasks #47572

Convert RunTask to use testclusers, remove ClusterFormationTasks #47572

Conversation

alpar-t commented Oct 4, 2019

elasticmachine commented Oct 4, 2019

mark-vieira commented Oct 4, 2019

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

alpar-t left a comment

Choose a reason for hiding this comment

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

alpar-t commented Oct 8, 2019