Convert testclusters to use distro download plugin #44253

rjernst · 2019-07-12T03:31:33Z

Test clusters currently has its own set of logic for dealing with
finding different versions of Elasticsearch, downloading them, and
extracting them. This commit converts testclusters to use the
DistributionDownloadPlugin.

Test clusters currently has its own set of logic for dealing with finding different versions of Elasticsearch, downloading them, and extracting them. This commit converts testclusters to use the DistributionDownloadPlugin.

elasticmachine · 2019-07-12T03:31:35Z

Pinging @elastic/es-core-infra

alpar-t

Looks great!

I have one major concern as it seems we are loosing an important optimization with this change.

alpar-t · 2019-07-12T06:30:35Z

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

@@ -338,9 +339,9 @@ public boolean isProcessAlive() {
        return nodes.stream().noneMatch(node -> node.isProcessAlive() == false);
    }

-    void eachVersionedDistribution(BiConsumer<String, Distribution> consumer) {
+    void eachVersionedDistribution(BiConsumer<Version, TestDistribution> consumer) {


not 100% sure but this might no longer be used now

You're right. This can now be removed.

alpar-t · 2019-07-12T06:40:47Z

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

+
+        project.getPlugins().apply(DistributionDownloadPlugin.class);
+        @SuppressWarnings("unchecked")
+        NamedDomainObjectContainer<ElasticsearchDistribution> distros =


I was thinking of suggesting a ElasticsearchDistributionContainer type so that we can get this by type, but I don't think Gradle offers a way to construct that. Maybe @mark-vieira knows of a way.

You can. You have to extend AbstractNamedDomainObjectContainer which is technically internal but it's not an uncommon thing to do. The other option is to just create a default container, and delegate to it from your custom one. This is cumbersome in Java but could be done easily w/o boilderplate if the implementation was Groovy using something like @Delegate.

alpar-t · 2019-07-12T07:31:32Z

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

@@ -60,22 +60,23 @@
    private final String clusterName;
    private final NamedDomainObjectContainer<ElasticsearchNode> nodes;
    private final File workingDirBase;
-    private final File artifactsExtractDir;
+    private final Function<Integer, ElasticsearchDistribution> distributionFactory;


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.

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.

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.

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

alpar-t · 2019-07-12T08:39:17Z

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

@@ -60,22 +60,23 @@
    private final String clusterName;
    private final NamedDomainObjectContainer<ElasticsearchNode> nodes;
    private final File workingDirBase;
-    private final File artifactsExtractDir;
+    private final Function<Integer, ElasticsearchDistribution> distributionFactory;


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.

mark-vieira

Few comments, otherwise this looks great! Running tests that only use the integ-test distro are much faster now and overall this PR remove more code than it added which is always a step in the right direction.

mark-vieira · 2019-07-12T18:10:15Z

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

-        return files;
+    private Iterable<File> getDistributionClasspath() {
+        FileTree jarFiles = distribution.getExtracted().getFileTree(project).matching(filter -> filter.include("**/*.jar"));
+        return () -> {


This is very clever 👍. Thing is, it doesn't actually do what we want, lol. So my intuition was incorrect and returning an Iterable doesn't actually defer anything at task graph calculation time. That's because this iterable could be some other Buildable type, Gradle has to iterate this collection and add each entry as input. Due to type erasure we have no idea at runtime what this is a collection of, so we have to rip through it.

The interesting thing here though is that our assumption that calling getSingleFile() would cause resolution is actually incorrect. This doesn't actually trigger any work. It just returns a path to the empty directory as we are just resolving the configuration, but not it's artifacts. So all this effort to defer that was in vain 🤦‍♂ .

With that knowledge in hand we might be able to simplify some of this logic now that we knowing calling getSingleFile() on a configuration which includes a single directory artifact is fine.

mark-vieira · 2019-07-12T18:18:31Z

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

    }

    private NamedDomainObjectContainer<ElasticsearchCluster> createTestClustersContainerExtension(Project project) {
+
+        project.getPlugins().apply(DistributionDownloadPlugin.class);


I'd move this up to the first line of the plugin apply() method. It makes it very obvious that this other plugin is a "dependency" of this plugin.

mark-vieira · 2019-07-12T18:23:04Z

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

@@ -338,9 +339,9 @@ public boolean isProcessAlive() {
        return nodes.stream().noneMatch(node -> node.isProcessAlive() == false);
    }

-    void eachVersionedDistribution(BiConsumer<String, Distribution> consumer) {
+    void eachVersionedDistribution(BiConsumer<Version, TestDistribution> consumer) {


You're right. This can now be removed.

rjernst · 2019-07-13T00:33:54Z

@mark-vieira @atorok I think I have addressed all your feedback. One small change I had to make was to move how the test distribution is defaulted. Before this happened inside RestIntegTestTask. However, that required using an afterEvaluate to set it for qa tests, which use the oss distro (and have a sysprop to change it for all qa tests). Using the integ test zip by default is now done directly within ElasticsearchNode (see https://github.com/elastic/elasticsearch/pull/44253/files#diff-a704b6e9e08a9ccfcc05e2ae59844495R163).

rjernst · 2019-07-14T02:11:39Z

@elasticmachine run elasticsearch-ci/2

rjernst · 2019-07-14T03:52:42Z

@elasticmachine run elasticsearch-ci/bwc

rjernst · 2019-07-14T08:47:17Z

@elasticmachine run elasticsearch-ci/bwc

alpar-t

LGTM

Test clusters currently has its own set of logic for dealing with finding different versions of Elasticsearch, downloading them, and extracting them. This commit converts testclusters to use the DistributionDownloadPlugin.

Convert testclusters to use distro download plugin

47407aa

Test clusters currently has its own set of logic for dealing with finding different versions of Elasticsearch, downloading them, and extracting them. This commit converts testclusters to use the DistributionDownloadPlugin.

rjernst added :Delivery/Build Build or test infrastructure >refactoring v8.0.0 v7.4.0 labels Jul 12, 2019

rjernst requested review from alpar-t and mark-vieira July 12, 2019 03:31

Merge branch 'master' into distro_tests13

16584eb

alpar-t requested changes Jul 12, 2019

View reviewed changes

alpar-t reviewed Jul 12, 2019

View reviewed changes

alpar-t approved these changes Jul 12, 2019

View reviewed changes

mark-vieira approved these changes Jul 12, 2019

View reviewed changes

address feedback, move default distro and version into node

6d897bd

rjernst added 3 commits July 13, 2019 11:03

use setters for setting defaults

3b2ff7d

solidify naming of test distro enum

95d0b1a

fix third party audit tests

c28dbbf

Merge branch 'master' into distro_tests13

870e109

alpar-t approved these changes Jul 15, 2019

View reviewed changes

rjernst merged commit 7e87c41 into elastic:master Jul 15, 2019

rjernst deleted the distro_tests13 branch July 15, 2019 17:39

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 testclusters to use distro download plugin #44253

Convert testclusters to use distro download plugin #44253

rjernst commented Jul 12, 2019

elasticmachine commented Jul 12, 2019

alpar-t left a comment

alpar-t Jul 12, 2019

mark-vieira Jul 12, 2019

alpar-t Jul 12, 2019

mark-vieira Jul 12, 2019

alpar-t Jul 12, 2019

rjernst Jul 12, 2019

alpar-t Jul 12, 2019

mark-vieira Jul 12, 2019

alpar-t Jul 12, 2019

mark-vieira left a comment

mark-vieira Jul 12, 2019

mark-vieira Jul 12, 2019

mark-vieira Jul 12, 2019

rjernst commented Jul 13, 2019

rjernst commented Jul 14, 2019

rjernst commented Jul 14, 2019

rjernst commented Jul 14, 2019

alpar-t left a comment

Convert testclusters to use distro download plugin #44253

Convert testclusters to use distro download plugin #44253

Conversation

rjernst commented Jul 12, 2019

elasticmachine commented Jul 12, 2019

alpar-t 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

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

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

rjernst commented Jul 13, 2019

rjernst commented Jul 14, 2019

rjernst commented Jul 14, 2019

rjernst commented Jul 14, 2019

alpar-t left a comment

Choose a reason for hiding this comment