Convert die with dignity test to new rest test infra #97734

rjernst · 2023-07-17T21:41:06Z

This commit rewrites the DieWithDignity test to use the new test infra. A side effect of this change is that it no longer relies on jps, which appears to have issues on Windows.

closes #77282

This commit rewrites the DieWithDignity test to use the new test infra. A side effect of this change is that it no longer relies on jps, which appears to have issues on Windows. closes elastic#77282

elasticsearchmachine · 2023-07-17T21:41:30Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

mark-vieira

Couple minor comments, otherwise LGTM. Have we done any testing on Windows here? This seems like the kind of thing that would be sensitive to platform differences and I don't see anything expressly forbidding Windows execution.

mark-vieira · 2023-07-17T22:16:04Z

test/external-modules/die-with-dignity/build.gradle

 GradleUtils.extendSourceSet(project, "main", "javaRestTest", tasks.named("javaRestTest"))

 tasks.named("javaRestTest").configure {
  it.onlyIf("snapshot build") { BuildParams.isSnapshotBuild() }


Why can't these tests run against release builds?

The test uses a test external-module, which are only bundled with snapshots. This is also why we must use the default distribution. We could probably rework this, but I'd like to not change that here.

Nevermind, figured it out.

mark-vieira · 2023-07-17T22:16:32Z

test/external-modules/die-with-dignity/build.gradle

-  // disable exit on out of memory error to let DieWithDignityIT verify that OOM handling without that works (including OOMs that are not caused by
-  // memory like native threads. We leave it to the JVM to test that exit on OOM works via the flag.
-  jvmArgs '-XX:-ExitOnOutOfMemoryError'
+  usesDefaultDistribution()


Is it strictly necessary for these tests to use the default distribution? Could they not use the integ-test distro?

mark-vieira · 2023-07-17T22:17:27Z

...th-dignity/src/javaRestTest/java/org/elasticsearch/qa/die_with_dignity/DieWithDignityIT.java


+    @ClassRule
+    public static ElasticsearchCluster cluster = ElasticsearchCluster.local()
+        .nodes(1)


FYI, this is superfluous since by default, unless explicitly specified, it'll create a single-node cluster.

jakelandis · 2023-07-18T22:18:14Z

@rjernst - any hints to what why this test fails when run with fips -Dtests.fips.enabled=true ?



[2023-07-19T07:59:52,206][INFO ][o.e.q.d.DieWithDignityIT ] [testDieWithDignity] before test |  
-- | --


[2023-07-19T07:59:52,206][INFO ][o.e.q.d.DieWithDignityIT ] [testDieWithDignity] before test	
[2023-07-19T07:59:52,213][INFO ][o.e.h.n.s.HealthNodeTaskExecutor] [test-cluster-0] Node [{test-cluster-0}{-Eadd_T3SUiaIIamFHEE3g}] is selected as the current health node.	
[2023-07-19T07:59:52,244][INFO ][o.e.t.c.l.WaitForHttpResource] [testDieWithDignity] Got successful response [200] from URL [http://127.0.0.1:38835/_cluster/health?wait_for_nodes=>=1&wait_for_status=yellow]	
[2023-07-19T07:59:52,247][INFO ][o.e.q.d.DieWithDignityIT ] [testDieWithDignity] initializing REST clients against [http://127.0.0.1:38835/]	
Jul 19, 2023 7:59:52 AM org.bouncycastle.jsse.provider.ProvKeyManagerFactorySpi getDefaultKeyStore	
INFO: Initializing empty key store	
Jul 19, 2023 7:59:52 AM org.bouncycastle.jsse.provider.ProvTrustManagerFactorySpi getDefaultTrustStore	
INFO: Initializing with trust store at path: /dev/shm/elastic+elasticsearch+pull-request+part-1-fips/test/external-modules/die-with-dignity/build/fips-resources/cacerts.bcfks	
[2023-07-19T07:59:52,323][INFO ][o.e.l.ClusterStateLicenseService] [test-cluster-0] license [667be6cc-d634-4ec6-b521-e31ff038e859] mode [trial] - valid	
java.lang.OutOfMemoryError: Requested array size exceeds VM limit	
Dumping heap to /dev/shm/elastic+elasticsearch+pull-request+part-1-fips/test/external-modules/die-with-dignity/build/testrun/javaRestTest/temp/test-cluster10166251179603076161/test-cluster-0/logs/java_pid313092.hprof ...	
Heap dump file created [154368866 bytes in 0.541 secs]	
Terminating due to java.lang.OutOfMemoryError: Requested array size exceeds VM limit	
Enter password for the elasticsearch keystore : 	
ERROR: Elasticsearch exited unexpectedly, with exit code 3	
[2023-07-19T07:59:53,793][INFO ][o.e.q.d.DieWithDignityIT ] [testDieWithDignity] after test	
REPRODUCE WITH: ./gradlew ':test:external-modules:test-die-with-dignity:javaRestTest' --tests "org.elasticsearch.qa.die_with_dignity.DieWithDignityIT.testDieWithDignity" -Dtests.seed=AD752CA150BF7783 -Dtests.locale=id -Dtests.timezone=Australia/Lord_Howe -Druntime.java=20 -Dtests.fips.enabled=true

mark-vieira · 2023-07-18T22:25:32Z

Sure enough, this just started failing on FIPS.

https://gradle-enterprise.elastic.co/scans/tests?search.tags=elastic%2Belasticsearch%2Bmain%2Bperiodic%2Bjava-fips-matrix&search.timeZoneId=America/Los_Angeles&tests.container=org.elasticsearch.qa.die_with_dignity.DieWithDignityIT

rjernst · 2023-07-19T04:09:17Z

The exit code is not what I would expect:

1> Terminating due to java.lang.OutOfMemoryError: Requested array size exceeds VM limit |  
2> ERROR: Elasticsearch exited unexpectedly, with exit code 3

But our uncaught exception handler (which handles termination from Errors) halts with code 127 on OOM. The test fails because it does not see the log messages the uncaught exception handler emits.

Is FIPS replacing our uncaught exception handler somehow? Or using some other trickiness to intercept the OOM and not allow our uncaught exception handler to proceed?

@tvernum @ChrisHegarty any ideas on what FIPS might be doing or ways our uncaught exception handler might be overhsadowed?

tvernum · 2023-07-19T08:20:28Z

Well, that was fun to debug...

It turns out that it has nothing to do with the uncaught exception handler. It was entirely because (on FIPS) this line was ineffective:

elasticsearch/test/external-modules/die-with-dignity/src/javaRestTest/java/org/elasticsearch/qa/die_with_dignity/DieWithDignityIT.java

Line 40 in 1d995eb

.jvmArg("-XX:-ExitOnOutOfMemoryError")

And the reason is due to a minor bug in the way the test cluster framework puts together the environment properties.
The FIPS build has additional system properties (to configure BCFIPS) and the combination of the args was broken.

I opened #97776

Convert die with dignity test to new rest test infra

4de0837

This commit rewrites the DieWithDignity test to use the new test infra. A side effect of this change is that it no longer relies on jps, which appears to have issues on Windows. closes elastic#77282

rjernst added >test Issues or PRs that are addressing/adding tests :Core/Infra/Core Core issues without another label v8.10.0 labels Jul 17, 2023

rjernst requested a review from mark-vieira July 17, 2023 21:41

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 17, 2023

spotless

79b8a11

mark-vieira approved these changes Jul 17, 2023

View reviewed changes

rjernst added 2 commits July 17, 2023 15:38

Use smaller distro

2c65250

simplify

f4a1e7e

rjernst merged commit 1d995eb into elastic:main Jul 17, 2023

rjernst deleted the test/die-with-dignity-pid4 branch July 17, 2023 23:49

jakelandis mentioned this pull request Jul 18, 2023

Update certs for PKI tests and re-enable tests #97766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Convert die with dignity test to new rest test infra #97734

Convert die with dignity test to new rest test infra #97734

Uh oh!

rjernst commented Jul 17, 2023

Uh oh!

elasticsearchmachine commented Jul 17, 2023

Uh oh!

mark-vieira left a comment

Uh oh!

mark-vieira Jul 17, 2023

Uh oh!

rjernst Jul 17, 2023

Uh oh!

rjernst Jul 17, 2023

Uh oh!

mark-vieira Jul 17, 2023

Uh oh!

mark-vieira Jul 17, 2023

Uh oh!

jakelandis commented Jul 18, 2023

Uh oh!

mark-vieira commented Jul 18, 2023

Uh oh!

rjernst commented Jul 19, 2023

Uh oh!

tvernum commented Jul 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Convert die with dignity test to new rest test infra #97734

Convert die with dignity test to new rest test infra #97734

Uh oh!

Conversation

rjernst commented Jul 17, 2023

Uh oh!

elasticsearchmachine commented Jul 17, 2023

Uh oh!

mark-vieira left a comment

Choose a reason for hiding this comment

Uh oh!

mark-vieira Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

rjernst Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

rjernst Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

mark-vieira Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

mark-vieira Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

jakelandis commented Jul 18, 2023

Uh oh!

mark-vieira commented Jul 18, 2023

Uh oh!

rjernst commented Jul 19, 2023

Uh oh!

tvernum commented Jul 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants