Skip to content

Convert testclusters to use distro download plugin #44253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 15, 2019
Merged

Convert testclusters to use distro download plugin #44253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
47407aa
Convert testclusters to use distro download plugin
rjernst Jul 12, 2019
16584eb
Merge branch 'master' into distro_tests13
rjernst Jul 12, 2019
6d897bd
address feedback, move default distro and version into node
rjernst Jul 13, 2019
3b2ff7d
use setters for setting defaults
rjernst Jul 13, 2019
95d0b1a
solidify naming of test distro enum
rjernst Jul 14, 2019
c28dbbf
fix third party audit tests
rjernst Jul 14, 2019
870e109
Merge branch 'master' into distro_tests13
rjernst Jul 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions buildSrc/src/main/groovy/org/elasticsearch/gradle/doc/DocsTestPlugin.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,9 @@ package org.elasticsearch.gradle.doc
import org.elasticsearch.gradle.OS
import org.elasticsearch.gradle.Version
import org.elasticsearch.gradle.VersionProperties
import org.elasticsearch.gradle.test.ClusterFormationTasks
import org.elasticsearch.gradle.test.RestTestPlugin
import org.gradle.api.Project
import org.gradle.api.Task

/**
* Sets up tests for documentation.
*/
Expand All @@ -38,7 +36,7 @@ public class DocsTestPlugin extends RestTestPlugin {
super.apply(project)
String distribution = System.getProperty('tests.distribution', 'default')
// The distribution can be configured with -Dtests.distribution on the command line
project.testClusters.integTest.distribution = distribution.toUpperCase()
project.testClusters.integTest.testDistribution = distribution.toUpperCase()
project.testClusters.integTest.nameCustomization = { it.replace("integTest", "node") }
// Docs are published separately so no need to assemble
project.tasks.assemble.enabled = false
Expand Down
6 changes: 1 addition & 5 deletions buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,14 @@
*/
package org.elasticsearch.gradle.test

import org.elasticsearch.gradle.VersionProperties

import org.elasticsearch.gradle.testclusters.ElasticsearchCluster
import org.elasticsearch.gradle.testclusters.TestClustersPlugin
import org.gradle.api.DefaultTask
import org.gradle.api.Task
import org.gradle.api.execution.TaskExecutionAdapter
import org.gradle.api.logging.Logger
import org.gradle.api.logging.Logging
import org.gradle.api.specs.Specs
import org.gradle.api.tasks.Copy
import org.gradle.api.tasks.Input
import org.gradle.api.tasks.TaskState
Expand All @@ -37,7 +36,6 @@ import org.gradle.plugins.ide.idea.IdeaPlugin
import java.nio.charset.StandardCharsets
import java.nio.file.Files
import java.util.stream.Stream

/**
* A wrapper task around setting up a cluster and running rest tests.
*/
Expand Down Expand Up @@ -69,8 +67,6 @@ class RestIntegTestTask extends DefaultTask {
} else {
project.testClusters {
"$name" {
distribution = 'INTEG_TEST'
version = VersionProperties.elasticsearch
javaHome = project.file(project.ext.runtimeJavaHome)
}
}
Expand Down
80 changes: 0 additions & 80 deletions buildSrc/src/main/java/org/elasticsearch/gradle/Distribution.java
View file
Open in desktop

This file was deleted.

8 changes: 7 additions & 1 deletion buildSrc/src/main/java/org/elasticsearch/gradle/DistributionDownloadPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
*/
public class DistributionDownloadPlugin implements Plugin<Project> {

private static final String CONTAINER_NAME = "elasticsearch_distributions";
private static final String FAKE_IVY_GROUP = "elasticsearch-distribution";
private static final String DOWNLOAD_REPO_NAME = "elasticsearch-downloads";

Expand All @@ -67,7 +68,7 @@ public void apply(Project project) {
Configuration extractedConfiguration = project.getConfigurations().create("es_distro_extracted_" + name);
return new ElasticsearchDistribution(name, project.getObjects(), fileConfiguration, extractedConfiguration);
});
project.getExtensions().add("elasticsearch_distributions", distributionsContainer);
project.getExtensions().add(CONTAINER_NAME, distributionsContainer);

setupDownloadServiceRepo(project);

Expand All @@ -78,6 +79,11 @@ public void apply(Project project) {
project.afterEvaluate(this::setupDistributions);
}

@SuppressWarnings("unchecked")
public static NamedDomainObjectContainer<ElasticsearchDistribution> getContainer(Project project) {
return (NamedDomainObjectContainer<ElasticsearchDistribution>) project.getExtensions().getByName(CONTAINER_NAME);
}

// pkg private for tests
void setupDistributions(Project project) {
for (ElasticsearchDistribution distribution : distributionsContainer) {
Expand Down
7 changes: 7 additions & 0 deletions buildSrc/src/main/java/org/elasticsearch/gradle/ElasticsearchDistribution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@
package org.elasticsearch.gradle;

import org.gradle.api.Buildable;
import org.gradle.api.Project;
import org.gradle.api.artifacts.Configuration;
import org.gradle.api.file.FileTree;
import org.gradle.api.model.ObjectFactory;
import org.gradle.api.provider.Property;
import org.gradle.api.tasks.TaskDependency;

import java.io.File;
import java.util.Iterator;
import java.util.Locale;
import java.util.concurrent.Callable;

public class ElasticsearchDistribution implements Buildable {

Expand Down Expand Up @@ -90,6 +93,10 @@ public TaskDependency getBuildDependencies() {
return configuration.getBuildDependencies();
}

public FileTree getFileTree(Project project) {
return project.fileTree((Callable<File>) configuration::getSingleFile);
}

@Override
public String toString() {
return configuration.getSingleFile().toString();
Expand Down
4 changes: 2 additions & 2 deletions buildSrc/src/main/java/org/elasticsearch/gradle/test/RestTestRunnerTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import java.util.ArrayList;
import java.util.Collection;

import static org.elasticsearch.gradle.Distribution.INTEG_TEST;
import static org.elasticsearch.gradle.testclusters.TestDistribution.INTEG_TEST;

/**
* Customized version of Gradle {@link Test} task which tracks a collection of {@link ElasticsearchCluster} as a task input. We must do this
Expand All @@ -23,7 +23,7 @@ public class RestTestRunnerTask extends Test {
public RestTestRunnerTask() {
super();
this.getOutputs().doNotCacheIf("Build cache is only enabled for tests against clusters using the 'integ-test' distribution",
task -> clusters.stream().flatMap(c -> c.getNodes().stream()).anyMatch(n -> n.getDistribution() != INTEG_TEST));
task -> clusters.stream().flatMap(c -> c.getNodes().stream()).anyMatch(n -> n.getTestDistribution() != INTEG_TEST));
}

@Nested
Expand Down
31 changes: 12 additions & 19 deletions buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchCluster.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,9 @@
*/
package org.elasticsearch.gradle.testclusters;

import org.elasticsearch.gradle.Distribution;
import org.elasticsearch.gradle.ElasticsearchDistribution;
import org.elasticsearch.gradle.FileSupplier;
import org.elasticsearch.gradle.PropertyNormalization;
import org.elasticsearch.gradle.Version;
import org.elasticsearch.gradle.http.WaitForHttpResource;
import org.gradle.api.Named;
import org.gradle.api.NamedDomainObjectContainer;
Expand All @@ -43,7 +42,6 @@
import java.util.Objects;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.BiConsumer;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.function.Supplier;
Expand All @@ -60,22 +58,23 @@ public class ElasticsearchCluster implements TestClusterConfiguration, Named {
private final String clusterName;
private final NamedDomainObjectContainer<ElasticsearchNode> nodes;
private final File workingDirBase;
private final File artifactsExtractDir;
private final Function<Integer, ElasticsearchDistribution> distributionFactory;
Copy link
Contributor

@alpar-t alpar-t Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment here previously which seems to have vanished.

It seems that we are generating a distribution per node, defeating the purpose of syncWithLinks.
This optimization is important both in terms of disk space and time the build takes and I would prefer we don't merge this without it.

It might make more sense to have this optimization in the distribution downloaded plugin trough and have test cluusters use the extracted distro dirs directly.

Copy link
Member Author

@rjernst rjernst Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The distribution download plugin already optimizes by only doing a single extraction for a given set of distribution properties. While we create a unique ElasticsearchDistribution object per node here, the underlying extracted dir will just exist once, in the root project.

Copy link
Contributor

@alpar-t alpar-t Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, got it, we only use the index because we need unique names for the domain object container.
I wonder if we should be concerned about name clashes here ? In theory other uses of the distribution download plugin could use the same name as testclusters even trough unlikely.

Copy link
Contributor

@mark-vieira mark-vieira Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clashes are possible but will result in an obvious error. I don't think we need to be too concerned about it.

private final LinkedHashMap<String, Predicate<TestClusterConfiguration>> waitConditions = new LinkedHashMap<>();
private final Project project;

public ElasticsearchCluster(String path, String clusterName, Project project, File artifactsExtractDir, File workingDirBase) {
public ElasticsearchCluster(String path, String clusterName, Project project,
Function<Integer, ElasticsearchDistribution> distributionFactory, File workingDirBase) {
this.path = path;
this.clusterName = clusterName;
this.project = project;
this.distributionFactory = distributionFactory;
this.workingDirBase = workingDirBase;
this.artifactsExtractDir = artifactsExtractDir;
this.nodes = project.container(ElasticsearchNode.class);
this.nodes.add(
new ElasticsearchNode(
path, clusterName + "-0",
project, artifactsExtractDir, workingDirBase
)
project, workingDirBase, distributionFactory.apply(0)
)
);
// configure the cluster name eagerly so nodes know about it
this.nodes.all((node) -> node.defaultConfig.put("cluster.name", safeName(clusterName)));
Expand All @@ -98,8 +97,8 @@ public void setNumberOfNodes(int numberOfNodes) {

for (int i = nodes.size() ; i < numberOfNodes; i++) {
this.nodes.add(new ElasticsearchNode(
path, clusterName + "-" + i, project, artifactsExtractDir, workingDirBase
));
path, clusterName + "-" + i, project, workingDirBase, distributionFactory.apply(i)
));
}
}

Expand All @@ -121,8 +120,8 @@ public void setVersion(String version) {
}

@Override
public void setDistribution(Distribution distribution) {
nodes.all(each -> each.setDistribution(distribution));
public void setTestDistribution(TestDistribution distribution) {
nodes.all(each -> each.setTestDistribution(distribution));
}

@Override
Expand Down Expand Up @@ -248,7 +247,7 @@ public void start() {
for (ElasticsearchNode node : nodes) {
if (nodeNames != null) {
// Can only configure master nodes if we have node names defined
if (Version.fromString(node.getVersion()).getMajor() >= 7) {
if (node.getVersion().getMajor() >= 7) {
node.defaultConfig.put("cluster.initial_master_nodes", "[" + nodeNames + "]");
node.defaultConfig.put("discovery.seed_providers", "file");
node.defaultConfig.put("discovery.seed_hosts", "[]");
Expand Down Expand Up @@ -338,12 +337,6 @@ public boolean isProcessAlive() {
return nodes.stream().noneMatch(node -> node.isProcessAlive() == false);
}

void eachVersionedDistribution(BiConsumer<String, Distribution> consumer) {
nodes.forEach(each -> {
consumer.accept(each.getVersion(), each.getDistribution());
});
}

public ElasticsearchNode singleNode() {
if (nodes.size() != 1) {
throw new IllegalStateException(
Expand Down
Loading