Skip to content

Commit

Permalink
Do not create unused testCluster (elastic#77581)
Browse files Browse the repository at this point in the history
* Do not create unused testCluster

This avoids creating test clusters that are not required during the build.
We use lazy configuration here on testClusters and only instantiate them as theyre

* Do not fail on run task (debug)

* Create more test cluster lazy

* Make more test cluster lazy

* Avoid creating unused testcluster

* Fix PluginBuildPlugin

* Fix disabling geo db download

* Fix cluster setup in repository-multi-version

* Polishing

* Fix issue with irretic groovy ogic

* Fix bwc tests

* Fix more bwcTests

* Fix more bwc tests

* Fix more bwc tests

* Fix more bwc tests

* Fix typo

* Minor polishing

* Fix rolling upgrade tests

* Fix cluster config in sql qa mixedcluster project

* Fix more bwc tests

* Clean up before review

* Document test cluster usage

* Api polising after Review

provide useCluster(Provider) method to TestClusterAware

Ideally we take this a step further and realize those test clusters only on use.
But out of scope of this PR.

* Allow gradle provider as value for nonSystemProperties

* Some simplification on test configuration

* Fix typo in rest test config

* Fix more typos

* Fix another typo

* Fix more typos
  • Loading branch information
breskeby authored Sep 23, 2021
1 parent 9de62d5 commit 6ef13ab
Show file tree
Hide file tree
Showing 111 changed files with 551 additions and 567 deletions.
17 changes: 17 additions & 0 deletions BUILDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,23 @@ The major difference between these two syntaxes is, that the configuration block

By actually doing less in the gradle configuration time as only creating tasks that are requested as part of the build and by only running the configurations for those requested tasks, using the task avoidance api contributes a major part in keeping our build fast.

#### Registering test clusters

When using the elasticsearch test cluster plugin we want to use (similar to the task avoidance API) a Gradle API to create domain objects lazy or only if required by the build.
Therefore we register test cluster by using the following syntax:

def someClusterProvider = testClusters.register('someCluster') { ... }

This registers a potential testCluster named `somecluster` and provides a provider instance, but doesn't create it yet nor configures it. This makes the gradle configuration phase more efficient by
doing less.

To wire this registered cluster into a `TestClusterAware` task (e.g. `RestIntegTest`) you can resolve the actual cluster from the provider instance:

tasks.register('someClusterTest', RestIntegTestTask) {
useCluster someClusterProvider
nonInputProperties.systemProperty 'tests.leader_host', "${-> someClusterProvider.get().getAllHttpSocketURI().get(0)}"
}

#### Adding additional integration tests

Additional integration tests for a certain elasticsearch modules that are specific to certain cluster configuration can be declared in a separate so called `qa` subproject of your module.
Expand Down
6 changes: 2 additions & 4 deletions build-tools-internal/src/main/groovy/elasticsearch.run.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ import org.elasticsearch.gradle.testclusters.RunTask
// precompiled script plugins (see https://github.com/gradle/gradle/issues/17004)
// apply plugin: 'elasticsearch.internal-testclusters'

testClusters {
runTask {
testClusters.register("runTask") {
testDistribution = providers.systemProperty('run.distribution').orElse('default').forUseAtConfigurationTime().get()
if (providers.systemProperty('run.distribution').orElse('default').forUseAtConfigurationTime().get() == 'default') {
String licenseType = providers.systemProperty("run.license_type").orElse("basic").forUseAtConfigurationTime().get()
Expand All @@ -30,11 +29,10 @@ testClusters {
keystore 'bootstrap.password', 'password'
user username: 'elastic-admin', password: 'elastic-password', role: 'superuser'
}
}
}

tasks.register("run", RunTask) {
useCluster testClusters.runTask;
useCluster testClusters.named("runTask")
description = 'Runs elasticsearch in the foreground'
group = 'Verification'

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public void apply(final Project project) {
// Auto add dependent modules to the test cluster
if (project1.findProject(":modules:" + pluginName) != null) {
NamedDomainObjectContainer<ElasticsearchCluster> testClusters = testClusters(project, "testClusters");
testClusters.all(elasticsearchCluster -> elasticsearchCluster.module(":modules:" + pluginName));
testClusters.configureEach(elasticsearchCluster -> elasticsearchCluster.module(":modules:" + pluginName));
}
});
final var extension1 = project1.getExtensions().getByType(PluginPropertiesExtension.class);
Expand Down Expand Up @@ -120,17 +120,17 @@ public void apply(final Project project) {

// allow running ES with this plugin in the foreground of a build
var testClusters = testClusters(project, TestClustersPlugin.EXTENSION_NAME);
final var runCluster = testClusters.create("runTask", cluster -> {
var runCluster = testClusters.register("runtTask", c -> {
if (GradleUtils.isModuleProject(project.getPath())) {
cluster.module(bundleTask.flatMap((Transformer<Provider<RegularFile>, Zip>) zip -> zip.getArchiveFile()));
c.module(bundleTask.flatMap((Transformer<Provider<RegularFile>, Zip>) zip -> zip.getArchiveFile()));
} else {
cluster.plugin(bundleTask.flatMap((Transformer<Provider<RegularFile>, Zip>) zip -> zip.getArchiveFile()));
c.plugin(bundleTask.flatMap((Transformer<Provider<RegularFile>, Zip>) zip -> zip.getArchiveFile()));
}
});

project.getTasks().register("run", RunTask.class, runTask -> {
runTask.useCluster(runCluster);
runTask.dependsOn(project.getTasks().named(BUNDLE_PLUGIN_TASK_NAME));
project.getTasks().register("run", RunTask.class, r -> {
r.useCluster(runCluster.get());
r.dependsOn(project.getTasks().named(BUNDLE_PLUGIN_TASK_NAME));
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,16 @@ public void apply(Project project) {
NamedDomainObjectContainer<ElasticsearchCluster> testClusters = (NamedDomainObjectContainer<ElasticsearchCluster>) project
.getExtensions()
.getByName(TestClustersPlugin.EXTENSION_NAME);
var cluster = testClusters.maybeCreate(JAVA_REST_TEST);
var clusterProvider = testClusters.register(JAVA_REST_TEST);

// Register test task
TaskProvider<StandaloneRestIntegTestTask> javaRestTestTask = project.getTasks()
.register(JAVA_REST_TEST, StandaloneRestIntegTestTask.class, task -> {
task.useCluster(cluster);
task.useCluster(clusterProvider);
task.setTestClassesDirs(testSourceSet.getOutput().getClassesDirs());
task.setClasspath(testSourceSet.getRuntimeClasspath());

var cluster = clusterProvider.get();
var nonInputProperties = new SystemPropertyCommandLineArgumentProvider();
nonInputProperties.systemProperty("tests.rest.cluster", () -> String.join(",", cluster.getAllHttpSocketURI()));
nonInputProperties.systemProperty("tests.cluster", () -> String.join(",", cluster.getAllTransportPortURI()));
Expand All @@ -67,7 +68,7 @@ public void apply(Project project) {
// Register plugin bundle with test cluster
project.getPlugins().withType(PluginBuildPlugin.class, p -> {
TaskProvider<Zip> bundle = project.getTasks().withType(Zip.class).named(BUNDLE_PLUGIN_TASK_NAME);
cluster.plugin(bundle.flatMap(Zip::getArchiveFile));
clusterProvider.configure( c-> c.plugin(bundle.flatMap(Zip::getArchiveFile)));
javaRestTestTask.configure(t -> t.dependsOn(bundle));
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/
package org.elasticsearch.gradle.test;

import org.gradle.api.provider.Provider;
import org.gradle.api.tasks.Input;
import org.gradle.process.CommandLineArgumentProvider;

Expand All @@ -18,6 +19,10 @@
public class SystemPropertyCommandLineArgumentProvider implements CommandLineArgumentProvider {
private final Map<String, Object> systemProperties = new LinkedHashMap<>();

public void systemProperty(String key, Provider<Object> value) {
systemProperties.put(key, (Supplier<String>) () -> String.valueOf(value.get()));
}

public void systemProperty(String key, Supplier<String> value) {
systemProperties.put(key, value);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.gradle.transform.UnzipTransform;
import org.gradle.api.Action;
import org.gradle.api.NamedDomainObjectContainer;
import org.gradle.api.NamedDomainObjectProvider;
import org.gradle.api.Plugin;
import org.gradle.api.Project;
import org.gradle.api.Task;
Expand Down Expand Up @@ -81,11 +82,11 @@ public void apply(Project project) {
testSourceSet.getOutput().dir(copyRestTestSpecs.map(Task::getOutputs));
Configuration yamlRestTestImplementation = configurations.getByName(testSourceSet.getImplementationConfigurationName());
setupDefaultDependencies(project.getDependencies(), restTestSpecs, yamlRestTestImplementation);
var cluster = testClusters.maybeCreate(YAML_REST_TEST);
var cluster = testClusters.register(YAML_REST_TEST);
TaskProvider<StandaloneRestIntegTestTask> yamlRestTestTask = setupTestTask(project, testSourceSet, cluster);
project.getPlugins().withType(PluginBuildPlugin.class, p -> {
TaskProvider<Zip> bundle = project.getTasks().withType(Zip.class).named(BUNDLE_PLUGIN_TASK_NAME);
cluster.plugin(bundle.flatMap(Zip::getArchiveFile));
cluster.configure(c -> c.plugin(bundle.flatMap(Zip::getArchiveFile)));
yamlRestTestTask.configure(t -> t.dependsOn(bundle));
});

Expand All @@ -109,15 +110,16 @@ private static void setupDefaultDependencies(
}

private TaskProvider<StandaloneRestIntegTestTask> setupTestTask(
Project project,
SourceSet testSourceSet,
ElasticsearchCluster cluster
Project project,
SourceSet testSourceSet,
NamedDomainObjectProvider<ElasticsearchCluster> clusterProvider
) {
return project.getTasks().register(YAML_REST_TEST, StandaloneRestIntegTestTask.class, task -> {
task.useCluster(cluster);
task.useCluster(clusterProvider.get());
task.setTestClassesDirs(testSourceSet.getOutput().getClassesDirs());
task.setClasspath(testSourceSet.getRuntimeClasspath());

var cluster = clusterProvider.get();
var nonInputProperties = new SystemPropertyCommandLineArgumentProvider();
nonInputProperties.systemProperty("tests.rest.cluster", () -> String.join(",", cluster.getAllHttpSocketURI()));
nonInputProperties.systemProperty("tests.cluster", () -> String.join(",", cluster.getAllTransportPortURI()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.gradle.api.Action;
import org.gradle.api.Task;
import org.gradle.api.artifacts.Configuration;
import org.gradle.api.provider.Provider;
import org.gradle.api.tasks.Nested;

import java.util.Collection;
Expand All @@ -31,5 +32,9 @@ default void useCluster(ElasticsearchCluster cluster) {
getClusters().add(cluster);
}

default void useCluster(Provider<ElasticsearchCluster> cluster) {
useCluster(cluster.get());
}

default void beforeStart() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ private NamedDomainObjectContainer<ElasticsearchCluster> createTestClustersConta
);
});
project.getExtensions().add(EXTENSION_NAME, container);
container.all(cluster -> cluster.systemProperty("ingest.geoip.downloader.enabled.default", "false"));
container.configureEach(cluster -> cluster.systemProperty("ingest.geoip.downloader.enabled.default", "false"));
return container;
}

Expand Down
2 changes: 1 addition & 1 deletion client/rest-high-level/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ tasks.named("check").configure {
dependsOn(asyncIntegTest)
}

testClusters.all {
testClusters.configureEach {
testDistribution = 'DEFAULT'
systemProperty 'es.scripting.update.ctx_in_params', 'false'
setting 'reindex.remote.whitelist', '[ "[::1]:*", "127.0.0.1:*" ]'
Expand Down
3 changes: 1 addition & 2 deletions distribution/archives/integ-test-zip/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ tasks.named("integTest").configure {
* when running against an external cluster.
*/
if (project.providers.systemProperty("tests.rest.cluster").forUseAtConfigurationTime().isPresent()) {
nonInputProperties.systemProperty 'tests.logfile',
"${-> testClusters.integTest.singleNode().getServerLog()}"
nonInputProperties.systemProperty 'tests.logfile', testClusters.named('integTest').map(c -> c.singleNode().serverLog)
} else {
systemProperty 'tests.logfile', '--external--'
}
Expand Down
2 changes: 1 addition & 1 deletion modules/ingest-common/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ restResources {
}
}

testClusters.all {
testClusters.configureEach {
// Needed in order to test ingest pipeline templating:
// (this is because the integTest node is not using default distribution, but only the minimal number of required modules)
module ':modules:lang-mustache'
Expand Down
4 changes: 2 additions & 2 deletions modules/ingest-geoip/qa/file-based-update/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
apply plugin: 'elasticsearch.standalone-rest-test'
apply plugin: 'elasticsearch.rest-test'

testClusters.all {
testClusters.configureEach {
testDistribution = 'DEFAULT'
setting 'resource.reload.interval.high', '100ms'
setting 'xpack.security.enabled', 'true'
Expand All @@ -18,7 +18,7 @@ testClusters.all {

tasks.named("integTest").configure {
systemProperty 'tests.security.manager', 'false' // Allows the test the add databases to config directory.
nonInputProperties.systemProperty 'tests.config.dir', "${-> testClusters.integTest.singleNode().getConfigDir()}"
nonInputProperties.systemProperty 'tests.config.dir', testClusters.named("integTest").map(c -> c.singleNode().getConfigDir())
}

tasks.named("forbiddenPatterns").configure {
Expand Down
2 changes: 1 addition & 1 deletion modules/ingest-user-agent/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ restResources {
}
}

testClusters.all {
testClusters.configureEach {
extraConfigFile 'ingest-user-agent/test-regexes.yml', file('src/test/test-regexes.yml')
}

Expand Down
2 changes: 1 addition & 1 deletion modules/kibana/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ dependencies {
api project(path: ':modules:reindex')
}

testClusters.all {
testClusters.configureEach {
module ':modules:reindex'
}

22 changes: 9 additions & 13 deletions modules/lang-painless/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ esplugin {
classname 'org.elasticsearch.painless.PainlessPlugin'
}

testClusters.all {
testClusters.configureEach {
module ':modules:mapper-extras'
systemProperty 'es.scripting.update.ctx_in_params', 'false'
// TODO: remove this once cname is prepended to transport.publish_address by default in 8.0
Expand Down Expand Up @@ -109,41 +109,37 @@ dependencies {
}
}

testClusters {
generateContextCluster {
testDistribution = 'DEFAULT'
}
def generateContextCluster = testClusters.register("generateContextCluster") {
testDistribution = 'DEFAULT'
}

tasks.register("generateContextDoc", DefaultTestClustersTask) {
dependsOn sourceSets.doc.runtimeClasspath
useCluster testClusters.generateContextCluster
useCluster generateContextCluster
doFirst {
project.javaexec {
mainClass = 'org.elasticsearch.painless.ContextDocGenerator'
classpath = sourceSets.doc.runtimeClasspath
systemProperty "cluster.uri", "${-> testClusters.generateContextCluster.singleNode().getAllHttpSocketURI().get(0)}"
systemProperty "cluster.uri", "${-> generateContextCluster.get().singleNode().getAllHttpSocketURI().get(0)}"
}.assertNormalExitValue()
}
}
/**********************************************
* Context JSON Generation *
**********************************************/
testClusters {
generateContextApiSpecCluster {
def generateContextApiSpecCluster = testClusters.register("generateContextApiSpecCluster") {
testDistribution = 'DEFAULT'
}
}

tasks.register("generateContextApiSpec", DefaultTestClustersTask) {
dependsOn sourceSets.doc.runtimeClasspath
useCluster testClusters.generateContextApiSpecCluster
useCluster generateContextApiSpecCluster
doFirst {
project.javaexec {
mainClass = 'org.elasticsearch.painless.ContextApiSpecGenerator'
classpath = sourceSets.doc.runtimeClasspath
systemProperty "cluster.uri", "${-> testClusters.generateContextApiSpecCluster.singleNode().getAllHttpSocketURI().get(0)}"
systemProperty "jdksrc", providers.systemProperty("jdksrc").forUseAtConfigurationTime().getOrNull()
systemProperty "cluster.uri", "${-> generateContextApiSpecCluster.get().singleNode().getAllHttpSocketURI().get(0)}"
systemProperty "jdksrc", providers.systemProperty("jdksrc").getOrNull()
systemProperty "packageSources", providers.systemProperty("packageSources").forUseAtConfigurationTime().getOrNull()
}.assertNormalExitValue()
}
Expand Down
2 changes: 1 addition & 1 deletion modules/rank-eval/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ restResources {
}
}

testClusters.all {
testClusters.configureEach {
// Modules who's integration is explicitly tested in integration tests
module ':modules:lang-mustache'
}
4 changes: 2 additions & 2 deletions modules/reindex/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ esplugin {
classname 'org.elasticsearch.reindex.ReindexPlugin'
}

testClusters.all {
testClusters.configureEach {
// Modules who's integration is explicitly tested in integration tests
module ':modules:parent-join'
module ':modules:lang-painless'
Expand Down Expand Up @@ -151,7 +151,7 @@ if (Os.isFamily(Os.FAMILY_WINDOWS)) {
/* Use a closure on the string to delay evaluation until right before we
* run the integration tests so that we can be sure that the file is
* ready. */
nonInputProperties.systemProperty "es${version}.port", "${-> fixture.get().addressAndPort}"
nonInputProperties.systemProperty "es${version}.port", fixture.map(f->f.addressAndPort)
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion modules/repository-url/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def fixtureAddress = { fixtureName ->

File repositoryDir = fixture.fsRepositoryDir as File

testClusters.all {
testClusters.configureEach {
// repositoryDir is used by a FS repository to create snapshots
setting 'path.repo', "${repositoryDir.absolutePath}", PropertyNormalization.IGNORE_VALUE
// repositoryDir is used by two URL repositories to restore snapshots
Expand Down
2 changes: 1 addition & 1 deletion plugins/examples/custom-settings/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ esplugin {
noticeFile rootProject.file('NOTICE.txt')
}

testClusters.all {
testClusters.configureEach {
// Adds a setting in the Elasticsearch keystore before running the integration tests
keystore 'custom.secured', 'password'
}
Expand Down
2 changes: 1 addition & 1 deletion plugins/examples/custom-suggester/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ esplugin {
noticeFile rootProject.file('NOTICE.txt')
}

testClusters.all {
testClusters.configureEach {
numberOfNodes = 2
}
2 changes: 1 addition & 1 deletion plugins/examples/painless-whitelist/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ dependencies {
compileOnly "org.elasticsearch.plugin:elasticsearch-scripting-painless-spi:${elasticsearchVersion}"
}

testClusters.all {
testClusters.configureEach {
testDistribution = 'DEFAULT'
setting 'xpack.security.enabled', 'false'
}
2 changes: 1 addition & 1 deletion qa/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import org.elasticsearch.gradle.testclusters.TestClustersPlugin

subprojects { Project subproj ->
plugins.withType(TestClustersPlugin).whenPluginAdded {
testClusters.all {
testClusters.configureEach {
testDistribution = 'DEFAULT'
}
}
Expand Down
Loading

0 comments on commit 6ef13ab

Please sign in to comment.