Make Legacy test cluster infrastructure configuration cache compatible #101903

breskeby · 2023-11-08T10:49:08Z

Made some minor optimisations on the way
Do not allow node specific module or plugin configuration in test cluster setup

elasticsearchmachine · 2023-11-08T10:49:32Z

Pinging @elastic/es-delivery (Team:Delivery)

breskeby · 2023-11-08T21:11:12Z

...d-tools-internal/src/main/java/org/elasticsearch/gradle/internal/test/TestWithSslPlugin.java

@@ -85,7 +85,7 @@ public void apply(Project project) {
            NamedDomainObjectContainer<ElasticsearchCluster> clusters = (NamedDomainObjectContainer<ElasticsearchCluster>) project
                .getExtensions()
                .getByName(TestClustersPlugin.EXTENSION_NAME);
-            clusters.all(c -> {
+            clusters.configureEach(c -> {


We accidentally materialised all TestCluster here; fixing since we've been here

breskeby · 2023-11-16T13:59:17Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

@@ -302,84 +287,34 @@ private void setDistributionType(ElasticsearchDistribution distribution, TestDis
        }
    }

-    // package protected so only TestClustersAware can access


Since we never have node specific plugin or module requirements I moved this one level up to ElasticsearchCluster. That reduces the API surface and makes this handling more efficient as we only do this whole configuration setup once per cluster.

breskeby · 2023-11-16T13:59:49Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

        this.plugins.add(plugin.map(RegularFile::getAsFile));
    }

    @Override
    public void plugin(String pluginProjectPath) {
-        plugin(maybeCreatePluginOrModuleDependency(pluginProjectPath, "zip"));
+        throw new IllegalStateException("Not Supported API");


Never used. We should not start now while migrating away from this Build logic

I think this makes more sense to be an UnsupportedOperationException.

breskeby · 2023-11-16T15:05:08Z

build-tools/src/main/java/org/elasticsearch/gradle/ElasticsearchDistribution.java

@@ -55,7 +55,7 @@ public String toString() {
    private final Property<Boolean> failIfUnavailable;
    private final Property<Boolean> preferArchive;
    private final ConfigurableFileCollection extracted;
-    private Action<ElasticsearchDistribution> distributionFinalizer;
+    private transient Action<ElasticsearchDistribution> distributionFinalizer;


This is only used when configuring the distribution. Once this is configured (and cached by the configuration cache) this is no longer required.

breskeby · 2023-11-16T16:32:37Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersPlugin.java

        project.getRootProject().getPluginManager().apply(TestClustersHookPlugin.class);
+        configureArtifactTransforms(project);


Doing this only once per project here and not multiple times for each cluster node

breskeby · 2023-11-16T16:38:45Z

x-pack/plugin/sql/qa/jdbc/build.gradle

@@ -74,7 +74,7 @@ subprojects {
    // Compatibility testing for JDBC driver started with version 7.9.0
    BuildParams.bwcVersions.allIndexCompatible.findAll({ it.onOrAfter(Version.fromString("7.9.0")) && it != VersionProperties.elasticsearchVersion }).each { bwcVersion ->
      def baseName = "v${bwcVersion}"
-      def cluster = testClusters.maybeCreate(baseName)
+      def cluster = testClusters.register(baseName)


creating cluster lazy while been here

breskeby · 2023-11-16T18:36:00Z

...t/groovy/org/elasticsearch/gradle/internal/test/rest/LegacyYamlRestTestPluginFuncTest.groovy

@@ -43,8 +41,6 @@ class LegacyYamlRestTestPluginFuncTest extends AbstractRestResourcesFuncTest {

    def "yamlRestTest executes and copies api and tests to correct source set"() {
        given:
-        // RestIntegTestTask not cc compatible due to
-        configurationCacheCompatible = false


Run tests with configuration cache enabled.

breskeby · 2023-11-16T18:38:39Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersAware.java

        getClusters().add(cluster);
    }

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

-    default void beforeStart() {}
+    default void beforeStart() {
+        getTasksService().get().register(this);


We need to do that here as we cannot apply custom logic within task graph calculation as this is skipped when configuration cache is enabled

But shouldn't the result of the task graph listener get cached? That makes it seem like the state of build services isn't cached.

breskeby · 2023-11-16T18:46:01Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersRegistry.java

+        int claim = claimsInventory.getOrDefault(cluster, 0) + 1;
+        claimsInventory.put(cluster, claim);
+        if (claim > 1) {
+            cluster.setShared(true);


We keep that information in the serialised cluster as the BuildServiceParameters cannot be keeping state across builds

Ok, so I'm correct in my previous comment that build service state isn't cached. Any mutations they make have to be to cached data structures, which presumably means something that's an input to a task? Or a ValueSource?

a Task itself is basically cached. So (nested) beans or fields of a task need to be cacheable

Therefore keep the information in the cluster

breskeby · 2023-11-16T22:15:38Z

...s-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/RestTestBasePlugin.java

@@ -113,7 +113,7 @@ public void apply(Project project) {
        configureArtifactTransforms(project);

        // Create configuration for aggregating historical feature metadata
-        Configuration featureMetadataConfig = project.getConfigurations().create(FEATURES_METADATA_CONFIGURATION, c -> {
+        FileCollection featureMetadataConfig = project.getConfigurations().create(FEATURES_METADATA_CONFIGURATION, c -> {


referenced from SystemPropertyCommandLineArgumentProvider during execution time. Therefore needs to be configuration cache compatible.

mark-vieira · 2023-11-21T21:49:33Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersAware.java

@@ -27,22 +26,27 @@ public interface TestClustersAware extends Task {
    @ServiceReference(REGISTRY_SERVICE_NAME)
    Property<TestClustersRegistry> getRegistery();

+    @ServiceReference(TEST_CLUSTER_TASKS_SERVICE)
+    Property<TestClustersPlugin.TaskEventsService> getTasksService();


These should probably be Provider not Property, no?

it seems to be required to be a property: https://docs.gradle.org/current/userguide/build_services.html#using_a_build_service_from_a_task

mark-vieira · 2023-11-21T21:50:48Z

...d-tools/src/main/java/org/elasticsearch/gradle/testclusters/StandaloneRestIntegTestTask.java

@@ -88,6 +88,7 @@ public WorkResult delete(Object... objects) {

    @Override
    public void beforeStart() {
+        getTasksService().get().register(this);


Maybe instead call super() here?

Not sure what you mean

I guess you mean that logic should move into the TestClusterAware?

mark-vieira · 2023-11-21T21:53:47Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersAware.java

        getClusters().add(cluster);
    }

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

-    default void beforeStart() {}
+    default void beforeStart() {
+        getTasksService().get().register(this);


But shouldn't the result of the task graph listener get cached? That makes it seem like the state of build services isn't cached.

mark-vieira · 2023-11-21T21:55:36Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersPlugin.java

            .getSharedServices()
            .registerIfAbsent(
                THROTTLE_SERVICE_NAME,
                TestClustersThrottle.class,
                spec -> spec.getMaxParallelUsages().set(Math.max(1, project.getGradle().getStartParameter().getMaxWorkerCount() / 2))
            );

-        // register cluster hooks
+        project.getTasks().withType(TestClustersAware.class).configureEach(task -> { task.usesService(testClustersThrottleProvider); });


Is this necessary? This should be accomplished by us overriding getSharedServices

It is to make our getSharedService override work with configuration cache enabled. Otherwise that service won’t be accessible when cc is enabled. We probably want to adjust the count though now within that override

mark-vieira · 2023-11-21T21:57:48Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersRegistry.java

+        int claim = claimsInventory.getOrDefault(cluster, 0) + 1;
+        claimsInventory.put(cluster, claim);
+        if (claim > 1) {
+            cluster.setShared(true);


Ok, so I'm correct in my previous comment that build service state isn't cached. Any mutations they make have to be to cached data structures, which presumably means something that's an input to a task? Or a ValueSource?

mark-vieira · 2023-11-21T22:02:19Z

x-pack/plugin/ml/build.gradle

+// Add an extra licenses directory to the combined notices
+tasks.named('generateNotice').configure {
+  dependsOn "extractNativeLicenses"
+  inputs.dir("${project.buildDir}/extractedNativeLicenses/platform/licenses")


We should probably just use extractNativeLicenses.destinationDir.

made that more gradle idiomatic reusing extractNativeLicenses.destinationDir

elastic#101903) * Move TestCluster plugin and module setup from Node to Cluster * Do not materialize unused TestCluster * Fix lazy test cluster evaluation * Register artifact transforms ones per project * Make task caching for TestClusterAware tasks CC compatible * Move stateful logic out of taskgraph.whenready

breskeby self-assigned this Nov 8, 2023

breskeby added >enhancement :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team labels Nov 8, 2023

elasticsearchmachine added the v8.12.0 label Nov 8, 2023

breskeby changed the title ~~Move TestCluster plugin and module setup from Node to Cluster~~ Rework LegacyTestCluster infrastructure to be configuration cache compatible Nov 8, 2023

breskeby changed the title ~~Rework LegacyTestCluster infrastructure to be configuration cache compatible~~ Rework LegacyTestCluster to be configuration cache compatible Nov 8, 2023

breskeby marked this pull request as draft November 8, 2023 10:50

breskeby force-pushed the rework-cc-test-cluster-infra branch from 2a6c08c to 5965cf6 Compare November 8, 2023 19:29

breskeby commented Nov 8, 2023

View reviewed changes

breskeby force-pushed the rework-cc-test-cluster-infra branch from 6376dbe to 0464caf Compare November 9, 2023 09:58

breskeby mentioned this pull request Nov 10, 2023

Elasticsearch build and Gradle plugins should support Gradle configuration cache #57918

Open

breskeby force-pushed the rework-cc-test-cluster-infra branch from c11c13a to 7f496f2 Compare November 10, 2023 11:37

breskeby commented Nov 16, 2023

View reviewed changes

breskeby force-pushed the rework-cc-test-cluster-infra branch from 73a6a19 to bea07c4 Compare November 16, 2023 14:12

breskeby changed the title ~~Rework LegacyTestCluster to be configuration cache compatible~~ Make Legacy test cluster infrastructure configuration cache compatible Nov 16, 2023

breskeby commented Nov 16, 2023

View reviewed changes

breskeby added >non-issue and removed >enhancement labels Nov 16, 2023

breskeby commented Nov 16, 2023

View reviewed changes

breskeby requested a review from mark-vieira November 16, 2023 22:10

breskeby marked this pull request as ready for review November 16, 2023 22:10

breskeby commented Nov 16, 2023

View reviewed changes

breskeby added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 21, 2023

breskeby force-pushed the rework-cc-test-cluster-infra branch from 0794534 to db509de Compare November 21, 2023 17:04

breskeby requested a review from a team November 21, 2023 19:37

mark-vieira requested changes Nov 21, 2023

View reviewed changes

breskeby force-pushed the rework-cc-test-cluster-infra branch from 013329d to 791c4e1 Compare November 27, 2023 10:54

breskeby added 22 commits November 27, 2023 13:53

Move TestCluster plugin and module setup from Node to Cluster

32ac209

Cleanup

e8d7393

Do not materialize unused TestCluster

f2bf447

WIP

2bd1e4e

Fix module and plugin deps for TestCluster

b36de7e

Fix tests; remove unused plugin

d2b0981

Fix lazy test cluster evaluation

896ec77

Some cleanup; make project transient in Cluster and Node

309a148

Fix formatting

8d57759

WIP

c058223

Explicitly declare usage of TestClusterThrottle

9f7ee0c

Register artifact transforms ones per project

3e23c40

Make task caching for TestClusterAware tasks CC compatible

7ebe400

Tweak build script

0eaf6e4

Move stateful logic out of taskgraph.whenready

2e25241

Add integ test coverage for configuration cache

a7f01f8

Some cleanup

49a4312

More cleanup

2d3096d

fix spotless

9ca15f8

Minor cleanup

c86ccef

Some more cleanup

ca3d2ba

Some more cleanup after review

0cddc75

breskeby force-pushed the rework-cc-test-cluster-infra branch from 791c4e1 to 0cddc75 Compare November 27, 2023 12:53

breskeby merged commit 4a9b4bc into elastic:main Nov 28, 2023
14 checks passed

breskeby deleted the rework-cc-test-cluster-infra branch November 28, 2023 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Legacy test cluster infrastructure configuration cache compatible #101903

Make Legacy test cluster infrastructure configuration cache compatible #101903

breskeby commented Nov 8, 2023 •

edited

Loading

elasticsearchmachine commented Nov 8, 2023

breskeby Nov 8, 2023 •

edited

Loading

breskeby Nov 16, 2023

breskeby Nov 16, 2023

mark-vieira Nov 16, 2023

breskeby Nov 17, 2023

breskeby Nov 16, 2023

breskeby Nov 16, 2023

breskeby Nov 16, 2023

breskeby Nov 16, 2023

breskeby Nov 16, 2023

mark-vieira Nov 21, 2023

breskeby Nov 16, 2023

mark-vieira Nov 21, 2023

breskeby Nov 23, 2023

breskeby Nov 23, 2023

breskeby Nov 16, 2023

mark-vieira Nov 21, 2023

breskeby Nov 23, 2023

mark-vieira Nov 21, 2023

breskeby Nov 22, 2023

breskeby Nov 22, 2023

mark-vieira Nov 21, 2023

mark-vieira Nov 21, 2023

breskeby Nov 22, 2023

mark-vieira Nov 21, 2023

mark-vieira Nov 21, 2023

breskeby Nov 23, 2023

		project.getRootProject().getPluginManager().apply(TestClustersHookPlugin.class);
		configureArtifactTransforms(project);

Make Legacy test cluster infrastructure configuration cache compatible #101903

Make Legacy test cluster infrastructure configuration cache compatible #101903

Conversation

breskeby commented Nov 8, 2023 • edited Loading

elasticsearchmachine commented Nov 8, 2023

breskeby Nov 8, 2023 • edited Loading

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

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

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

breskeby commented Nov 8, 2023 •

edited

Loading

breskeby Nov 8, 2023 •

edited

Loading