Understand Gradle 4.8 test failure around setup of security policies

[alpar-t]

With Gradle 4.8 running on JDK 10 this happens (reformatted for readability):

java.lang.RuntimeException: unable to install test security manager
	at org.elasticsearch.bootstrap.BootstrapForTesting.<clinit>(BootstrapForTesting.java:170)
	at org.elasticsearch.test.ESTestCase.<clinit>(ESTestCase.java:197)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:374)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$2.run(RandomizedRunner.java:592)
Caused by: java.lang.IllegalStateException: codebase property already set: codebase.plugin-classloader -> 
file:/home/alpar/work/elastic/elasticsearch_master_ci/libs/plugin-classloader/build/distributions/plugin-classloader-7.0.0-alpha1-SNAPSHOT.jar, cannot set to 
file:/home/alpar/work/elastic/elasticsearch_master_ci/libs/plugin-classloader/build/distributions/plugin-classloader-7.0.0-alpha1-SNAPSHOT.jar
	at org.elasticsearch.bootstrap.Security.readPolicy(Security.java:233)
	at org.elasticsearch.bootstrap.BootstrapForTesting.<clinit>(BootstrapForTesting.java:145)
	... 4 more

then most other tests fail with something like:

Exception in thread "Thread-3" java.lang.NoClassDefFoundError: Could not initialize class org.elasticsearch.test.ESTestCase
    at java.base/java.lang.Thread.run(Thread.java:844)

The reading of policies from Security is done trough a static {} block in BootstrapForTesting which should guarantee that it is only initialized once. There doesn't seem to be any other code paths that exercise this, the only time Security.readPolicy is in BootstrapForTesting and Bootstrap that starts Elasticsearch, even if the tests were to do that, it doesn't have a chance to run before the tests are bootstrap.

It must be some strange intimacy between Gradle 4.8 and JDK 10 OpenJDK Runtime Environment (build 10.0.1+10) in my case, as the issue only reproduces with:
./gradlew clean check
It does not reproduce with:

• ./gradlew :server:test
• ./gradlew :server:test ran 100 times
• Gradle 4.7
• Gradle 4.8 with JDK 11ea (OpenJDK Runtime Environment 18.9 (build 11-ea+17))

Gradle seems to change the order of tasks a lot and will run many other tasks, including some tests before the :server:tests ( which probably doesn't make sense ). Not sure this is relevant.

In order to be able to move to the new Gradle version #31271 has a work around.
I'm writhing this up in the hopes of revisiting it to provide more clarity.
Note that as of this writing the Gradle 4.8 PR mentioned above is not merged.

Relates to #31230.

[elasticmachine]

Pinging @elastic/es-core-infra

[alpar-t]

the problem is not reproducible with a change such as to alter task ordering:

diff --git a/build.gradle b/build.gradle
index 43d59e7b249..de1facab3ca 100644
--- a/build.gradle
+++ b/build.gradle
@@ -585,3 +585,15 @@ compareGradleBuilds {
     arguments = ["-Dbuild.compare_friendly=true"]
   }
 }
+
+allprojects {
+    afterEvaluate {
+        tasks.all {
+            try {
+                if (it.getPath().startsWith(":server") == false && it.getPath().startsWith(":client") == false && it.getPath().startsWith(":libs") == false && it.getPath().startsWith(":test") ==false && it.getPath().startsWith(":build-tools") == false ) {
+                    it.mustRunAfter { project(':server').test  }
+                }
+            } catch (e) {println e}
+        }
+    }
+}

[alpar-t]

@nik9000 Has some comments on #31271 around this: not reproducible with older versions of Gradle, we may be running the tests with the Gradle runner instead of the RandomizedTestRunner, definitely a must-fix to get this in.

[alpar-t]

I can confirm the root issue is indeed that we are running the tests with the Gradle runner. In fact all the test tasks are with the Gradle runner instead of RandomizedTestingTask.

[nik9000]

Really!? Some of them looked right to me. Weird!

[alpar-t]

I extended the randomized test runner plugin to include a sanity check (commit is d6e4c56, not pushed as of this writing) with this, the problem reproduces in ~15 seconds. I then tried to isolate it in https://github.com/atorok/gradle48_jdk11_replace_task and with this I found that it also reproduces on JDK 10, but apparently less frequently. That means we should have seen it in CI already. I'm not sure why we didn't. Maybe it was such a rare occurrence that it was discounted, or the frequency at which it occurs for the ES build vs. my isolated build is much lower. I'll submit an issue for Gradle, but from the forums I take it that the task replacement is fuzzy and bound to break, there are some APIs but it's not quite a finished feature.

@nik9000 My previous comment is not quite right. I assumed many = all, but it's the case, e.x. from 3 runs I just did, first one had 44 wrong test task, the second one had none, the third one had 42.

[alpar-t]

The reason we are not seeing this in CI is that it's specific to Gradle 4.8

[alpar-t]

Maybe we should be thinking of creating a task with a different name than test for the randomized tests. I.e. move to have say utest and integTest tasks. We can have test.dependsOn utest just so running gradle test runs them, because old habits die hard. I don't think it makes sense to replace the original test task, since the configuration of the randomized test is so different, it's obviously a different thing, not compatible at all. Seeing configuration applied to utest rather than test will hint that something different is used. I was able to quickly assemble a proof of concept of how this will work.

Ideally to make this totally clean, it would be nice to rename the source sets and create source sets for integration tests as well. So we have src/utest/java and src/integTest/java and use the java-base so we can still use source sets but avoid the original test tasks to be generated at all.

[nik9000]

Making a utest task seems like a fairly drastic solution to gradle's problem. I think we'd want to think a long while before moving all of the source sets around too. We share code between integration and unit tests and the mechanism that we have now gets the job done fairly well.

If we're doing something fundamentally against the way gradle wants to work then I think it'd be worth looking into but I don't think we are.

[nik9000]

Hmmm. I wonder if we could stop the java task from adding the test task in the first place. Moving the source sets would do it but it seems like a big change for what ought to be a smaller problem. But gradle....

[alpar-t]

Moving around the source sets is not a must. We can start just with a name change. I wanted to look into not creating test at all but ran out of time. I know there's a base Java plugin which we can use as long as we re-implement parts of the config, we should be able to pull it off, but it will add even more specialization to our already specialized build. I don't particularly like that we are replacing a gradle convention with something incompatible with it as the props on the random test runner are different.

…

On Tue, Jun 19, 2018, 16:36 Nik Everett ***@***.***> wrote: Hmmm. I wonder if we could stop the java task from adding the test task in the first place. Moving the source sets would do it but it seems like a big change for what ought to be a smaller problem. But gradle.... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31324 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACcmFIPuzna-HEyHUm-0-ZBWjxrY_1L-ks5t-P5VgaJpZM4Unjy0> .

[alpar-t]

java-base doesn't help much as it only brings the concept of source sets. Looking at how the Java plugin is implemented, there's no way to prevent it from creating test, however I did find that Gradle 4.8 uses the "lazy task" API 'tasks.createLatherwhereas .7 hadtasks.create`. This helped me find gradle/gradle-native#710. The mechanism proposed there will not work for us, as it proposes to reduce replace functionality to being able to replace only with tasks of a compatible interface.
So I do think it's the case that we are doing something Gradle doesn't want us to do.
I also think it's not a best practice to replace tasks, especially the ones in the gradle API with others implementing a different interface. Replacing task actions, or tasks that don't have an interface for configuration would be a different matter.

I think the best way forward is to disable the test task and create the randomized test runner as utest, making test depend on utest so one can still run ./gradlew test for convenience. Build scripts will configure utest instead of test - a change for the better imho as it makes things explicit.

[alpar-t]

Thanks to additional diagnostics and information provided by gradle/gradle#5730, we now have a working implementation that replaces the test task. #31496 will help us move in the direction of using the Gradle runner.