Skip to content

Commit 65e760f

Browse files
breskebybreskeby
authored
Change finalization of elasticsearch distribution (#64403)
- Avoid to rely on afterEvaluate lifecycle hook in DistributionDownloadPlugin - Allows us to define test cluster only in the task creation which can be later than afterevaluate which allows us to move tasks that rely on test clusters using task avoidance api. - This should result in test clusters only being defined when actually used in the build - This unblocks further progress on #56610
1 parent acfb253 commit 65e760f

File tree

9 files changed

+91
-40
lines changed

9 files changed

+91
-40
lines changed

‎buildSrc/src/integTest/groovy/org/elasticsearch/gradle/DistributionDownloadPluginFuncTest.groovy

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ class DistributionDownloadPluginFuncTest extends AbstractGradleFuncTest {
150150
private static String setupDistroTask() {
151151
return """
152152
tasks.register("setupDistro", Sync) {
153-
from(elasticsearch_distributions.test_distro.extracted)
153+
from(elasticsearch_distributions.test_distro)
154154
into("build/distro")
155155
}
156156
"""

‎buildSrc/src/integTest/groovy/org/elasticsearch/gradle/TestClustersPluginFuncTest.groovy

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,31 @@ class TestClustersPluginFuncTest extends AbstractGradleFuncTest {
7575
assertNoCustomDistro('myCluster')
7676
}
7777

78+
def "can declare test cluster in lazy evaluated task configuration block"() {
79+
given:
80+
buildFile << """
81+
tasks.register('myTask', SomeClusterAwareTask) {
82+
testClusters {
83+
myCluster {
84+
testDistribution = 'default'
85+
}
86+
}
87+
useCluster testClusters.myCluster
88+
}
89+
"""
90+
91+
when:
92+
def result = withMockedDistributionDownload(gradleRunner("myTask", '-i')) {
93+
build()
94+
}
95+
96+
then:
97+
result.output.contains("elasticsearch-keystore script executed!")
98+
assertEsStdoutContains("myCluster", "Starting Elasticsearch process")
99+
assertEsStdoutContains("myCluster", "Stopping node")
100+
assertNoCustomDistro('myCluster')
101+
}
102+
78103
def "custom distro folder created for tweaked cluster distribution"() {
79104
given:
80105
buildFile << """

‎buildSrc/src/integTest/groovy/org/elasticsearch/gradle/internal/InternalDistributionDownloadPluginFuncTest.groovy

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ class InternalDistributionDownloadPluginFuncTest extends AbstractGradleFuncTest
6363
}
6464
}
6565
tasks.register("setupDistro", Sync) {
66-
from(elasticsearch_distributions.test_distro.extracted)
66+
from(elasticsearch_distributions.test_distro)
6767
into("build/distro")
6868
}
6969
"""
@@ -93,7 +93,7 @@ class InternalDistributionDownloadPluginFuncTest extends AbstractGradleFuncTest
9393
}
9494
}
9595
tasks.register("setupDistro", Sync) {
96-
from(elasticsearch_distributions.test_distro.extracted)
96+
from(elasticsearch_distributions.test_distro)
9797
into("build/distro")
9898
}
9999
"""
@@ -124,7 +124,7 @@ class InternalDistributionDownloadPluginFuncTest extends AbstractGradleFuncTest
124124
}
125125
}
126126
tasks.register("createExtractedTestDistro") {
127-
dependsOn elasticsearch_distributions.test_distro.extracted
127+
dependsOn elasticsearch_distributions.test_distro
128128
}
129129
"""
130130
when:

‎buildSrc/src/main/java/org/elasticsearch/gradle/DistributionDownloadPlugin.java

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -81,15 +81,21 @@ public void apply(Project project) {
8181
setupResolutionsContainer(project);
8282
setupDistributionContainer(project, dockerSupport);
8383
setupDownloadServiceRepo(project);
84-
project.afterEvaluate(this::setupDistributions);
8584
}
8685

8786
private void setupDistributionContainer(Project project, Provider<DockerSupportService> dockerSupport) {
8887
distributionsContainer = project.container(ElasticsearchDistribution.class, name -> {
8988
Configuration fileConfiguration = project.getConfigurations().create("es_distro_file_" + name);
9089
Configuration extractedConfiguration = project.getConfigurations().create(DISTRO_EXTRACTED_CONFIG_PREFIX + name);
9190
extractedConfiguration.getAttributes().attribute(ArtifactAttributes.ARTIFACT_FORMAT, ArtifactTypeDefinition.DIRECTORY_TYPE);
92-
return new ElasticsearchDistribution(name, project.getObjects(), dockerSupport, fileConfiguration, extractedConfiguration);
91+
return new ElasticsearchDistribution(
92+
name,
93+
project.getObjects(),
94+
dockerSupport,
95+
fileConfiguration,
96+
extractedConfiguration,
97+
(dist) -> finalizeDistributionDependencies(project, dist)
98+
);
9399
});
94100
project.getExtensions().add(CONTAINER_NAME, distributionsContainer);
95101
}
@@ -113,20 +119,16 @@ public static NamedDomainObjectContainer<DistributionResolution> getRegistration
113119
return (NamedDomainObjectContainer<DistributionResolution>) project.getExtensions().getByName(RESOLUTION_CONTAINER_NAME);
114120
}
115121

116-
// pkg private for tests
117-
void setupDistributions(Project project) {
118-
for (ElasticsearchDistribution distribution : distributionsContainer) {
119-
distribution.finalizeValues();
120-
DependencyHandler dependencies = project.getDependencies();
121-
// for the distribution as a file, just depend on the artifact directly
122-
DistributionDependency distributionDependency = resolveDependencyNotation(project, distribution);
123-
dependencies.add(distribution.configuration.getName(), distributionDependency.getDefaultNotation());
124-
// no extraction allowed for rpm, deb or docker
125-
if (distribution.getType().shouldExtract()) {
126-
// The extracted configuration depends on the artifact directly but has
127-
// an artifact transform registered to resolve it as an unpacked folder.
128-
dependencies.add(distribution.getExtracted().getName(), distributionDependency.getExtractedNotation());
129-
}
122+
private void finalizeDistributionDependencies(Project project, ElasticsearchDistribution distribution) {
123+
DependencyHandler dependencies = project.getDependencies();
124+
// for the distribution as a file, just depend on the artifact directly
125+
DistributionDependency distributionDependency = resolveDependencyNotation(project, distribution);
126+
dependencies.add(distribution.configuration.getName(), distributionDependency.getDefaultNotation());
127+
// no extraction needed for rpm, deb or docker
128+
if (distribution.getType().shouldExtract()) {
129+
// The extracted configuration depends on the artifact directly but has
130+
// an artifact transform registered to resolve it as an unpacked folder.
131+
dependencies.add(distribution.getExtracted().getName(), distributionDependency.getExtractedNotation());
130132
}
131133
}
132134

‎buildSrc/src/main/java/org/elasticsearch/gradle/ElasticsearchDistribution.java

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package org.elasticsearch.gradle;
2121

2222
import org.elasticsearch.gradle.docker.DockerSupportService;
23+
import org.gradle.api.Action;
2324
import org.gradle.api.Buildable;
2425
import org.gradle.api.artifacts.Configuration;
2526
import org.gradle.api.model.ObjectFactory;
@@ -103,13 +104,16 @@ public String toString() {
103104
private final Property<Boolean> bundledJdk;
104105
private final Property<Boolean> failIfUnavailable;
105106
private final Configuration extracted;
107+
private Action<ElasticsearchDistribution> distributionFinalizer;
108+
private boolean froozen = false;
106109

107110
ElasticsearchDistribution(
108111
String name,
109112
ObjectFactory objectFactory,
110113
Provider<DockerSupportService> dockerSupport,
111114
Configuration fileConfiguration,
112-
Configuration extractedConfiguration
115+
Configuration extractedConfiguration,
116+
Action<ElasticsearchDistribution> distributionFinalizer
113117
) {
114118
this.name = name;
115119
this.dockerSupport = dockerSupport;
@@ -123,6 +127,7 @@ public String toString() {
123127
this.bundledJdk = objectFactory.property(Boolean.class);
124128
this.failIfUnavailable = objectFactory.property(Boolean.class).convention(true);
125129
this.extracted = extractedConfiguration;
130+
this.distributionFinalizer = distributionFinalizer;
126131
}
127132

128133
public String getName() {
@@ -196,7 +201,22 @@ public String toString() {
196201
return getName() + "_" + getType() + "_" + getVersion();
197202
}
198203

204+
/**
205+
* if not executed before, this
206+
* freezes the distribution configuration and
207+
* runs distribution finalizer logic.
208+
*/
209+
public ElasticsearchDistribution maybeFreeze() {
210+
if (!froozen) {
211+
finalizeValues();
212+
distributionFinalizer.execute(this);
213+
froozen = true;
214+
}
215+
return this;
216+
}
217+
199218
public String getFilepath() {
219+
maybeFreeze();
200220
return configuration.getSingleFile().toString();
201221
}
202222

@@ -217,22 +237,26 @@ public Configuration getExtracted() {
217237

218238
@Override
219239
public TaskDependency getBuildDependencies() {
220-
// For non-required Docker distributions, skip building the distribution is Docker is unavailable
221-
if (isDocker() && getFailIfUnavailable() == false && dockerSupport.get().getDockerAvailability().isAvailable == false) {
240+
if (skippingDockerDistributionBuild()) {
222241
return task -> Collections.emptySet();
242+
} else {
243+
maybeFreeze();
244+
return getType().shouldExtract() ? extracted.getBuildDependencies() : configuration.getBuildDependencies();
223245
}
246+
}
224247

225-
return configuration.getBuildDependencies();
248+
private boolean skippingDockerDistributionBuild() {
249+
return isDocker() && getFailIfUnavailable() == false && dockerSupport.get().getDockerAvailability().isAvailable == false;
226250
}
227251

228252
@Override
229253
public Iterator<File> iterator() {
230-
return configuration.iterator();
254+
maybeFreeze();
255+
return getType().shouldExtract() ? extracted.iterator() : configuration.iterator();
231256
}
232257

233258
// internal, make this distribution's configuration unmodifiable
234259
void finalizeValues() {
235-
236260
if (getType() == Type.INTEG_TEST_ZIP) {
237261
if (platform.getOrNull() != null) {
238262
throw new IllegalArgumentException(
@@ -294,4 +318,9 @@ void finalizeValues() {
294318
flavor.finalizeValue();
295319
bundledJdk.finalizeValue();
296320
}
321+
322+
public Configuration getArchive() {
323+
maybeFreeze();
324+
return configuration;
325+
}
297326
}

‎buildSrc/src/main/java/org/elasticsearch/gradle/test/DistroTestPlugin.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,8 @@ public void apply(Project project) {
113113
for (ElasticsearchDistribution distribution : testDistributions) {
114114
String taskname = destructiveDistroTestTaskName(distribution);
115115
TaskProvider<?> depsTask = project.getTasks().register(taskname + "#deps");
116-
depsTask.configure(t -> t.dependsOn(distribution, examplePlugin, quotaAwareFsPlugin));
116+
// explicitly depend on the archive not on the implicit extracted distribution
117+
depsTask.configure(t -> t.dependsOn(distribution.getArchive(), examplePlugin, quotaAwareFsPlugin));
117118
depsTasks.put(taskname, depsTask);
118119
TaskProvider<Test> destructiveTask = configureTestTask(project, taskname, distribution, t -> {
119120
t.onlyIf(t2 -> distribution.isDocker() == false || dockerSupport.get().getDockerAvailability().isAvailable);

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ public void setVersions(List<String> versions) {
231231
}
232232

233233
private void doSetVersion(String version) {
234-
String distroName = "testclusters" + path.replace(":", "-") + "-" + this.name + "-" + version + "-";
234+
String distroName = "testclusters" + path.replace(":", "-") + "-" + this.name + "-" + version;
235235
NamedDomainObjectContainer<ElasticsearchDistribution> container = DistributionDownloadPlugin.getContainer(project);
236236
if (container.findByName(distroName) == null) {
237237
container.create(distroName);
@@ -425,6 +425,7 @@ public Path getConfigDir() {
425425
public void freeze() {
426426
requireNonNull(testDistribution, "null testDistribution passed when configuring test cluster `" + this + "`");
427427
LOGGER.info("Locking configuration of `{}`", this);
428+
distributions.stream().forEach(d -> d.maybeFreeze());
428429
configurationFrozen.set(true);
429430
}
430431

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,10 @@ default void useCluster(ElasticsearchCluster cluster) {
3535
throw new TestClustersException("Task " + getPath() + " can't use test cluster from" + " another project " + cluster);
3636
}
3737

38-
cluster.getNodes().stream().flatMap(node -> node.getDistributions().stream()).forEach(distro -> dependsOn(distro.getExtracted()));
38+
cluster.getNodes()
39+
.stream()
40+
.flatMap(node -> node.getDistributions().stream())
41+
.forEach(distro -> dependsOn(getProject().provider(() -> distro.maybeFreeze())));
3942
cluster.getNodes().forEach(node -> dependsOn((Callable<Collection<Configuration>>) node::getPluginAndModuleConfigurations));
4043
getClusters().add(cluster);
4144
}

‎buildSrc/src/test/java/org/elasticsearch/gradle/DistributionDownloadPluginTests.java

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,6 @@ public void testLocalCurrentVersionIntegTestZip() {
185185
archiveProject.getConfigurations().create("default");
186186
archiveProject.getArtifacts().add("default", new File("doesnotmatter"));
187187
createDistro(project, "distro", VersionProperties.getElasticsearch(), Type.INTEG_TEST_ZIP, null, null, null);
188-
checkPlugin(project);
189188
}
190189

191190
public void testLocalCurrentVersionArchives() {
@@ -200,7 +199,6 @@ public void testLocalCurrentVersionArchives() {
200199
archiveProject.getConfigurations().create("default");
201200
archiveProject.getArtifacts().add("default", new File("doesnotmatter"));
202201
createDistro(project, "distro", VersionProperties.getElasticsearch(), Type.ARCHIVE, platform, flavor, bundledJdk);
203-
checkPlugin(project);
204202
}
205203
}
206204
}
@@ -216,7 +214,6 @@ public void testLocalCurrentVersionPackages() {
216214
packageProject.getConfigurations().create("default");
217215
packageProject.getArtifacts().add("default", new File("doesnotmatter"));
218216
createDistro(project, "distro", VersionProperties.getElasticsearch(), packageType, null, flavor, bundledJdk);
219-
checkPlugin(project);
220217
}
221218
}
222219
}
@@ -294,7 +291,7 @@ private ElasticsearchDistribution createDistro(
294291
if (bundledJdk != null) {
295292
distro.setBundledJdk(bundledJdk);
296293
}
297-
});
294+
}).maybeFreeze();
298295
}
299296

300297
// create a distro and finalize its configuration
@@ -312,12 +309,6 @@ private ElasticsearchDistribution checkDistro(
312309
return distribution;
313310
}
314311

315-
// check the download plugin can be fully configured
316-
private void checkPlugin(Project project) {
317-
DistributionDownloadPlugin plugin = project.getPlugins().getPlugin(DistributionDownloadPlugin.class);
318-
plugin.setupDistributions(project);
319-
}
320-
321312
private void checkBwc(
322313
String projectName,
323314
String config,
@@ -333,7 +324,6 @@ private void checkBwc(
333324
archiveProject.getConfigurations().create(config);
334325
archiveProject.getArtifacts().add(config, new File("doesnotmatter"));
335326
createDistro(project, "distro", version.toString(), type, platform, flavor, true);
336-
checkPlugin(project);
337327
}
338328

339329
private Project createProject(BwcVersions bwcVersions, boolean isInternal) {

0 commit comments

Comments
 (0)