Build: Rework integ test setup and shutdown to ensure stop runs when …

…desired (#23304)

Gradle's finalizedBy on tasks only ensures one task runs after another,
but not immediately after. This is problematic for our integration tests
since it allows multiple project's integ test clusters to be
simultaneously. While this has not been a problem thus far (gradle 2.13
happened to keep the finalizedBy tasks close enough that no clusters
were running in parallel), with gradle 3.3 the task graph generation has
changed, and numerous clusters may be running simultaneously, causing
memory pressure, and thus generally slower tests, or even failure if the
system has a limited amount of memory (eg in a vagrant host).

This commit reworks how integ tests are configured. It adds an
`integTestCluster` extension to gradle which is equivalent to the current
`integTest.cluster` and moves the rest test runner task to
`integTestRunner`.  The `integTest` task is then just a dummy task,
which depends on the cluster runner task, as well as the cluster stop
task. This means running `integTest` in one project will both run the
rest tests, and shut down the cluster, before running `integTest` in
another project.

benchmarks/build.gradle

@@ -37,10 +37,7 @@ apply plugin: 'application'
 archivesBaseName = 'elasticsearch-benchmarks'
 mainClassName = 'org.openjdk.jmh.Main'
 
-// never try to invoke tests on the benchmark project - there aren't any
-check.dependsOn.remove(test)
-// explicitly override the test task too in case somebody invokes 'gradle test' so it won't trip
-task test(type: Test, overwrite: true)
+test.enabled = false
 
 dependencies {
     compile("org.elasticsearch:elasticsearch:${version}") {
@@ -59,7 +56,6 @@ compileJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-try,-u
 // enable the JMH's BenchmarkProcessor to generate the final benchmark classes
 // needs to be added separately otherwise Gradle will quote it and javac will fail
 compileJava.options.compilerArgs.addAll(["-processor", "org.openjdk.jmh.generators.BenchmarkProcessor"])
-compileTestJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-try,-unchecked"
 
 forbiddenApis {
     // classes generated by JMH can use all sorts of forbidden APIs but we have no influence at all and cannot exclude these classes

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

@@ -51,16 +51,12 @@ class ClusterFormationTasks {
      *
      * Returns a list of NodeInfo objects for each node in the cluster.
      */
-    static List<NodeInfo> setup(Project project, Task task, ClusterConfiguration config) {
-        if (task.getEnabled() == false) {
-            // no need to add cluster formation tasks if the task won't run!
-            return
-        }
+    static List<NodeInfo> setup(Project project, String prefix, Task runner, ClusterConfiguration config) {
         File sharedDir = new File(project.buildDir, "cluster/shared")
         // first we remove everything in the shared cluster directory to ensure there are no leftovers in repos or anything
         // in theory this should not be necessary but repositories are only deleted in the cluster-state and not on-disk
         // such that snapshots survive failures / test runs and there is no simple way today to fix that.
-        Task cleanup = project.tasks.create(name: "${task.name}#prepareCluster.cleanShared", type: Delete, dependsOn: task.dependsOn.collect()) {
+        Task cleanup = project.tasks.create(name: "${prefix}#prepareCluster.cleanShared", type: Delete, dependsOn: runner.dependsOn.collect()) {
             delete sharedDir
             doLast {
                 sharedDir.mkdirs()
@@ -75,7 +71,7 @@ class ClusterFormationTasks {
             throw new GradleException("bwcVersion must not be null if numBwcNodes is > 0")
         }
         // this is our current version distribution configuration we use for all kinds of REST tests etc.
-        String distroConfigName = "${task.name}_elasticsearchDistro"
+        String distroConfigName = "${prefix}_elasticsearchDistro"
         Configuration currentDistro = project.configurations.create(distroConfigName)
         configureDistributionDependency(project, config.distribution, currentDistro, VersionProperties.elasticsearch)
         if (config.bwcVersion != null && config.numBwcNodes > 0) {
@@ -89,7 +85,7 @@ class ClusterFormationTasks {
             }
             configureDistributionDependency(project, config.distribution, project.configurations.elasticsearchBwcDistro, config.bwcVersion)
             for (Map.Entry<String, Project> entry : config.plugins.entrySet()) {
-                configureBwcPluginDependency("${task.name}_elasticsearchBwcPlugins", project, entry.getValue(),
+                configureBwcPluginDependency("${prefix}_elasticsearchBwcPlugins", project, entry.getValue(),
                         project.configurations.elasticsearchBwcPlugins, config.bwcVersion)
             }
             project.configurations.elasticsearchBwcDistro.resolutionStrategy.cacheChangingModulesFor(0, TimeUnit.SECONDS)
@@ -104,13 +100,13 @@ class ClusterFormationTasks {
                 elasticsearchVersion = config.bwcVersion
                 distro = project.configurations.elasticsearchBwcDistro
             }
-            NodeInfo node = new NodeInfo(config, i, project, task, elasticsearchVersion, sharedDir)
+            NodeInfo node = new NodeInfo(config, i, project, prefix, elasticsearchVersion, sharedDir)
             nodes.add(node)
-            startTasks.add(configureNode(project, task, cleanup, node, distro, nodes.get(0)))
+            startTasks.add(configureNode(project, prefix, runner, cleanup, node, distro, nodes.get(0)))
         }
 
-        Task wait = configureWaitTask("${task.name}#wait", project, nodes, startTasks)
-        task.dependsOn(wait)
+        Task wait = configureWaitTask("${prefix}#wait", project, nodes, startTasks)
+        runner.dependsOn(wait)
 
         return nodes
     }
@@ -150,58 +146,58 @@ class ClusterFormationTasks {
      *
      * @return a task which starts the node.
      */
-    static Task configureNode(Project project, Task task, Object dependsOn, NodeInfo node, Configuration configuration, NodeInfo seedNode) {
+    static Task configureNode(Project project, String prefix, Task runner, Object dependsOn, NodeInfo node, Configuration configuration, NodeInfo seedNode) {
 
         // tasks are chained so their execution order is maintained
-        Task setup = project.tasks.create(name: taskName(task, node, 'clean'), type: Delete, dependsOn: dependsOn) {
+        Task setup = project.tasks.create(name: taskName(prefix, node, 'clean'), type: Delete, dependsOn: dependsOn) {
             delete node.homeDir
             delete node.cwd
             doLast {
                 node.cwd.mkdirs()
             }
         }
-        setup = configureCheckPreviousTask(taskName(task, node, 'checkPrevious'), project, setup, node)
-        setup = configureStopTask(taskName(task, node, 'stopPrevious'), project, setup, node)
-        setup = configureExtractTask(taskName(task, node, 'extract'), project, setup, node, configuration)
-        setup = configureWriteConfigTask(taskName(task, node, 'configure'), project, setup, node, seedNode)
+        setup = configureCheckPreviousTask(taskName(prefix, node, 'checkPrevious'), project, setup, node)
+        setup = configureStopTask(taskName(prefix, node, 'stopPrevious'), project, setup, node)
+        setup = configureExtractTask(taskName(prefix, node, 'extract'), project, setup, node, configuration)
+        setup = configureWriteConfigTask(taskName(prefix, node, 'configure'), project, setup, node, seedNode)
         if (node.config.plugins.isEmpty() == false) {
             if (node.nodeVersion == VersionProperties.elasticsearch) {
-                setup = configureCopyPluginsTask(taskName(task, node, 'copyPlugins'), project, setup, node)
+                setup = configureCopyPluginsTask(taskName(prefix, node, 'copyPlugins'), project, setup, node)
             } else {
-                setup = configureCopyBwcPluginsTask(taskName(task, node, 'copyBwcPlugins'), project, setup, node)
+                setup = configureCopyBwcPluginsTask(taskName(prefix, node, 'copyBwcPlugins'), project, setup, node)
             }
         }
 
         // install modules
         for (Project module : node.config.modules) {
             String actionName = pluginTaskName('install', module.name, 'Module')
-            setup = configureInstallModuleTask(taskName(task, node, actionName), project, setup, node, module)
+            setup = configureInstallModuleTask(taskName(prefix, node, actionName), project, setup, node, module)
         }
 
         // install plugins
         for (Map.Entry<String, Project> plugin : node.config.plugins.entrySet()) {
             String actionName = pluginTaskName('install', plugin.getKey(), 'Plugin')
-            setup = configureInstallPluginTask(taskName(task, node, actionName), project, setup, node, plugin.getValue())
+            setup = configureInstallPluginTask(taskName(prefix, node, actionName), project, setup, node, plugin.getValue())
         }
 
         // sets up any extra config files that need to be copied over to the ES instance;
         // its run after plugins have been installed, as the extra config files may belong to plugins
-        setup = configureExtraConfigFilesTask(taskName(task, node, 'extraConfig'), project, setup, node)
+        setup = configureExtraConfigFilesTask(taskName(prefix, node, 'extraConfig'), project, setup, node)
 
         // extra setup commands
         for (Map.Entry<String, Object[]> command : node.config.setupCommands.entrySet()) {
             // the first argument is the actual script name, relative to home
             Object[] args = command.getValue().clone()
             args[0] = new File(node.homeDir, args[0].toString())
-            setup = configureExecTask(taskName(task, node, command.getKey()), project, setup, node, args)
+            setup = configureExecTask(taskName(prefix, node, command.getKey()), project, setup, node, args)
         }
 
-        Task start = configureStartTask(taskName(task, node, 'start'), project, setup, node)
+        Task start = configureStartTask(taskName(prefix, node, 'start'), project, setup, node)
 
         if (node.config.daemonize) {
-            Task stop = configureStopTask(taskName(task, node, 'stop'), project, [], node)
+            Task stop = configureStopTask(taskName(prefix, node, 'stop'), project, [], node)
             // if we are running in the background, make sure to stop the server when the task completes
-            task.finalizedBy(stop)
+            runner.finalizedBy(stop)
             start.finalizedBy(stop)
         }
         return start
@@ -648,11 +644,11 @@ class ClusterFormationTasks {
     }
 
     /** Returns a unique task name for this task and node configuration */
-    static String taskName(Task parentTask, NodeInfo node, String action) {
+    static String taskName(String prefix, NodeInfo node, String action) {
         if (node.config.numNodes > 1) {
-            return "${parentTask.name}#node${node.nodeNum}.${action}"
+            return "${prefix}#node${node.nodeNum}.${action}"
         } else {
-            return "${parentTask.name}#${action}"
+            return "${prefix}#${action}"
         }
     }
 

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

@@ -21,7 +21,6 @@ package org.elasticsearch.gradle.test
 import org.apache.tools.ant.taskdefs.condition.Os
 import org.gradle.api.InvalidUserDataException
 import org.gradle.api.Project
-import org.gradle.api.Task
 
 /**
  * A container for the files and configuration associated with a single node in a test cluster.
@@ -96,17 +95,17 @@ class NodeInfo {
     /** the version of elasticsearch that this node runs */
     String nodeVersion
 
-    /** Creates a node to run as part of a cluster for the given task */
-    NodeInfo(ClusterConfiguration config, int nodeNum, Project project, Task task, String nodeVersion, File sharedDir) {
+    /** Holds node configuration for part of a test cluster. */
+    NodeInfo(ClusterConfiguration config, int nodeNum, Project project, String prefix, String nodeVersion, File sharedDir) {
         this.config = config
         this.nodeNum = nodeNum
         this.sharedDir = sharedDir
         if (config.clusterName != null) {
             clusterName = config.clusterName
         } else {
-            clusterName = "${task.path.replace(':', '_').substring(1)}"
+            clusterName = project.path.replace(':', '_').substring(1) + '_' + prefix
         }
-        baseDir = new File(project.buildDir, "cluster/${task.name} node${nodeNum}")
+        baseDir = new File(project.buildDir, "cluster/${prefix} node${nodeNum}")
         pidFile = new File(baseDir, 'es.pid')
         this.nodeVersion = nodeVersion
         homeDir = homeDir(baseDir, config.distribution, nodeVersion)

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

@@ -20,19 +20,23 @@ package org.elasticsearch.gradle.test
 
 import com.carrotsearch.gradle.junit4.RandomizedTestingTask
 import org.elasticsearch.gradle.BuildPlugin
+import org.gradle.api.DefaultTask
 import org.gradle.api.Task
 import org.gradle.api.internal.tasks.options.Option
 import org.gradle.api.plugins.JavaBasePlugin
 import org.gradle.api.tasks.Input
 import org.gradle.util.ConfigureUtil
 
 /**
- * Runs integration tests, but first starts an ES cluster,
- * and passes the ES cluster info as parameters to the tests.
+ * A wrapper task around setting up a cluster and running rest tests.
  */
-public class RestIntegTestTask extends RandomizedTestingTask {
+public class RestIntegTestTask extends DefaultTask {
 
-    ClusterConfiguration clusterConfig
+    protected ClusterConfiguration clusterConfig
+
+    protected RandomizedTestingTask runner
+
+    protected Task clusterInit
 
     /** Info about nodes in the integ test cluster. Note this is *not* available until runtime. */
     List<NodeInfo> nodes
@@ -44,35 +48,44 @@ public class RestIntegTestTask extends RandomizedTestingTask {
     public RestIntegTestTask() {
         description = 'Runs rest tests against an elasticsearch cluster.'
         group = JavaBasePlugin.VERIFICATION_GROUP
-        dependsOn(project.testClasses)
-        classpath = project.sourceSets.test.runtimeClasspath
-        testClassesDir = project.sourceSets.test.output.classesDir
-        clusterConfig = new ClusterConfiguration(project)
+        runner = project.tasks.create("${name}Runner", RandomizedTestingTask.class)
+        super.dependsOn(runner)
+        clusterInit = project.tasks.create(name: "${name}Cluster#init", dependsOn: project.testClasses)
+        runner.dependsOn(clusterInit)
+        runner.classpath = project.sourceSets.test.runtimeClasspath
+        runner.testClassesDir = project.sourceSets.test.output.classesDir
+        clusterConfig = project.extensions.create("${name}Cluster", ClusterConfiguration.class, project)
 
         // start with the common test configuration
-        configure(BuildPlugin.commonTestConfig(project))
+        runner.configure(BuildPlugin.commonTestConfig(project))
         // override/add more for rest tests
-        parallelism = '1'
-        include('**/*IT.class')
-        systemProperty('tests.rest.load_packaged', 'false')
+        runner.parallelism = '1'
+        runner.include('**/*IT.class')
+        runner.systemProperty('tests.rest.load_packaged', 'false')
         // we pass all nodes to the rest cluster to allow the clients to round-robin between them
         // this is more realistic than just talking to a single node
-        systemProperty('tests.rest.cluster', "${-> nodes.collect{it.httpUri()}.join(",")}")
-        systemProperty('tests.config.dir', "${-> nodes[0].confDir}")
+        runner.systemProperty('tests.rest.cluster', "${-> nodes.collect{it.httpUri()}.join(",")}")
+        runner.systemProperty('tests.config.dir', "${-> nodes[0].confDir}")
         // TODO: our "client" qa tests currently use the rest-test plugin. instead they should have their own plugin
         // that sets up the test cluster and passes this transport uri instead of http uri. Until then, we pass
         // both as separate sysprops
-        systemProperty('tests.cluster', "${-> nodes[0].transportUri()}")
+        runner.systemProperty('tests.cluster', "${-> nodes[0].transportUri()}")
 
         // copy the rest spec/tests into the test resources
         RestSpecHack.configureDependencies(project)
         project.afterEvaluate {
-            dependsOn(RestSpecHack.configureTask(project, includePackaged))
+            runner.dependsOn(RestSpecHack.configureTask(project, includePackaged))
         }
         // this must run after all projects have been configured, so we know any project
         // references can be accessed as a fully configured
         project.gradle.projectsEvaluated {
-            nodes = ClusterFormationTasks.setup(project, this, clusterConfig)
+            if (enabled == false) {
+                runner.enabled = false
+                clusterInit.enabled = false
+                return // no need to add cluster formation tasks if the task won't run!
+            }
+            nodes = ClusterFormationTasks.setup(project, "${name}Cluster", runner, clusterConfig)
+            super.dependsOn(runner.finalizedBy)
         }
     }
 
@@ -84,37 +97,33 @@ public class RestIntegTestTask extends RandomizedTestingTask {
         clusterConfig.debug = enabled;
     }
 
-    @Input
-    public void cluster(Closure closure) {
-        ConfigureUtil.configure(closure, clusterConfig)
-    }
-
-    public ClusterConfiguration getCluster() {
-        return clusterConfig
-    }
-
     public List<NodeInfo> getNodes() {
         return nodes
     }
 
     @Override
     public Task dependsOn(Object... dependencies) {
-        super.dependsOn(dependencies)
+        runner.dependsOn(dependencies)
         for (Object dependency : dependencies) {
             if (dependency instanceof Fixture) {
-                finalizedBy(((Fixture)dependency).stopTask)
+                runner.finalizedBy(((Fixture)dependency).stopTask)
             }
         }
         return this
     }
 
     @Override
     public void setDependsOn(Iterable<?> dependencies) {
-        super.setDependsOn(dependencies)
+        runner.setDependsOn(dependencies)
         for (Object dependency : dependencies) {
             if (dependency instanceof Fixture) {
-                finalizedBy(((Fixture)dependency).stopTask)
+                runner.finalizedBy(((Fixture)dependency).stopTask)
             }
         }
     }
+
+    @Override
+    public Task mustRunAfter(Object... tasks) {
+        clusterInit.mustRunAfter(tasks)
+    }
 }

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

@@ -43,7 +43,7 @@ public class RestTestPlugin implements Plugin<Project> {
         }
 
         RestIntegTestTask integTest = project.tasks.create('integTest', RestIntegTestTask.class)
-        integTest.cluster.distribution = 'zip' // rest tests should run with the real zip
+        integTest.clusterConfig.distribution = 'zip' // rest tests should run with the real zip
         integTest.mustRunAfter(project.precommit)
         project.check.dependsOn(integTest)
     }

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

@@ -18,7 +18,7 @@ public class RunTask extends DefaultTask {
         clusterConfig.daemonize = false
         clusterConfig.distribution = 'zip'
         project.afterEvaluate {
-            ClusterFormationTasks.setup(project, this, clusterConfig)
+            ClusterFormationTasks.setup(project, name, this, clusterConfig)
         }
     }
 

distribution/build.gradle

@@ -81,20 +81,16 @@ project.rootProject.subprojects.findAll { it.path.startsWith(':modules:') }.each
     }
   }
   // We would like to make sure integ tests for the distribution run after
-  // integ tests for the modules included in the distribution. However, gradle
-  // has a bug where depending on a task with a finalizer can sometimes not make
-  // the finalizer task follow the original task immediately. To work around this,
-  // we make the mustRunAfter the finalizer task itself.
-  // See https://discuss.gradle.org/t/cross-project-task-dependencies-ordering-screws-up-finalizers/13190
+  // integ tests for the modules included in the distribution.
   project.configure(distributions.findAll { it.name != 'integ-test-zip' }) { Project distribution ->
     distribution.afterEvaluate({
       // some integTest tasks will have multiple finalizers
-      distribution.integTest.mustRunAfter module.tasks.find { t -> t.name.matches(".*integTest\$") }.getFinalizedBy()
+      distribution.integTest.mustRunAfter module.tasks.find { t -> t.name.matches(".*integTest\$") }
     })
   }
   // also want to make sure the module's integration tests run after the integ-test-zip (ie rest tests)
   module.afterEvaluate({
-    module.integTest.mustRunAfter(':distribution:integ-test-zip:integTest#stop')
+    module.integTest.mustRunAfter(':distribution:integ-test-zip:integTest')
   })
   restTestExpansions['expected.modules.count'] += 1
 }
@@ -129,14 +125,13 @@ configure(distributions) {
   project.integTest {
     dependsOn project.assemble
     includePackaged project.name == 'integ-test-zip'
-    cluster {
-      distribution = project.name
-    }
     if (project.name != 'integ-test-zip') {
-      // see note above with module mustRunAfter about why integTest#stop is used here
-      mustRunAfter ':distribution:integ-test-zip:integTest#stop'
+      mustRunAfter ':distribution:integ-test-zip:integTest'
     }
   }
+  project.integTestCluster {
+    distribution = project.name
+  }
 
   processTestResources {
     inputs.properties(project(':distribution').restTestExpansions)