Test-fix testAcceptsMismatchedServerlessBuildHash #121869 #122332

mhl-b · 2025-02-12T04:25:51Z

testAcceptsMismatchedServerlessBuildHash uses wrong system property name "es.serverless" but should be "es.serverless_transport".

elasticsearchmachine · 2025-02-12T04:27:12Z

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

elasticsearchmachine · 2025-02-12T04:28:48Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

mhl-b · 2025-02-12T04:31:36Z

Hey @thecoop, I'm wondering why system property is different, am I missing something. It was like this for 1.5 years.

thecoop · 2025-02-12T08:47:10Z

This might be something @DaveCTurner has been modifying...

mhl-b · 2025-02-12T18:21:02Z

I dont see "es.serverless" property in the code base, probably a mistake. This should be a valid fix.

DiannaHohensee

This makes sense to me, but I had a concern about the reliability of the test before the change (see comment).

DiannaHohensee · 2025-02-12T21:36:31Z

server/src/test/java/org/elasticsearch/transport/TransportServiceHandshakeTests.java

        assumeTrue("Current build needs to be a snapshot", Build.current().isSnapshot());
        assumeTrue("Security manager needs to be disabled", System.getSecurityManager() == null);
-        System.setProperty("es.serverless", Boolean.TRUE.toString());   // security manager blocks this
+        System.setProperty(TransportService.SERVERLESS_TRANSPORT_SYSTEM_PROPERTY, Boolean.TRUE.toString());   // security manager blocks


Does the test reliably fail before this change? You mentioned reproducing it, but I wasn't sure of the frequency.

I'm wondering why this didn't fail in the first place when it was added. Maybe we could bundle the try-block logic into a method, and exercise it once with System.setProperty(TransportService.SERVERLESS_TRANSPORT_SYSTEM_PROPERTY, Boolean.TRUE.toString()); set to true (succeeds) and then a second time set to false (should throw)?

Does the test reliably fail before this change?

yes, test fails every time with

./gradlew ":server:test" --tests "org.elasticsearch.transport.TransportServiceHandshakeTests.testAcceptsMismatchedServerlessBuildHash" -Dtests.seed=8278A46E39005C96 -Dtests.jvm.argline="-Des.entitlements.enabled=" -Dtests.locale=en-001 -Dtests.timezone=US/Pacific -Druntime.java=24

I believe test suppose to test this flag by going through debugger.

elasticsearch/server/src/main/java/org/elasticsearch/transport/TransportService.java

Lines 694 to 698 in c27bc08

private void maybeThrowOnIncompatibleBuild(@Nullable DiscoveryNode node, @Nullable Exception e) {

if (SERVERLESS_TRANSPORT_FEATURE_FLAG == false && isIncompatibleBuild(version, buildHash)) {

throwOnIncompatibleBuild(node, e);

}

}

But it set with "es.serverless_transport".

Maybe we could bundle the try-block

Do you mean put System.setProperty inside try-catch? I dont think System.setProperty throws here. Also if it throws entire test case will not run.

Okay, then it reliably fixes the test 👍 Thanks.

Optionally I'd suggest making the test more robust by also proving the test setup would normally throw, and then verifying that it does not throw with the serverless Property set -- e.g. put the try-block code in a method and call it twice, with and without the Property setting. But that's a test improvement, not a test fix, so I don't feel strongly about it.

I was curious why it started to fail now since all related changes have been around more than 1 year. It turns out the reason is that the test was never executed because of the following line

elasticsearch/server/src/test/java/org/elasticsearch/transport/TransportServiceHandshakeTests.java

Line 379 in 3c18ea6

assumeTrue("Security manager needs to be disabled", System.getSecurityManager() == null);

SecurityManager is removed in JDK 24. Hence it started to fail now with JDK24 while it is skipped when running with all prior JDK versions.

So the test was "muted" for all this time? :)

Yeah I think it definitely has Not ran since #95754

DiannaHohensee

LGTM

DiannaHohensee · 2025-02-12T23:57:36Z

server/src/test/java/org/elasticsearch/transport/TransportServiceHandshakeTests.java

        assumeTrue("Current build needs to be a snapshot", Build.current().isSnapshot());
        assumeTrue("Security manager needs to be disabled", System.getSecurityManager() == null);
-        System.setProperty("es.serverless", Boolean.TRUE.toString());   // security manager blocks this
+        System.setProperty(TransportService.SERVERLESS_TRANSPORT_SYSTEM_PROPERTY, Boolean.TRUE.toString());   // security manager blocks


Okay, then it reliably fixes the test 👍 Thanks.

Optionally I'd suggest making the test more robust by also proving the test setup would normally throw, and then verifying that it does not throw with the serverless Property set -- e.g. put the try-block code in a method and call it twice, with and without the Property setting. But that's a test improvement, not a test fix, so I don't feel strongly about it.

…stic#122332) (cherry picked from commit 49ecbca)

mosche · 2025-02-26T10:29:36Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x
✅	9.0
✅	8.18

Questions ?

Please refer to the Backport tool documentation

…123477) (cherry picked from commit 49ecbca) Co-authored-by: Mikhail Berezovskiy <[email protected]>

…123478) (cherry picked from commit 49ecbca) Co-authored-by: Mikhail Berezovskiy <[email protected]>

…123476) (cherry picked from commit 49ecbca) Co-authored-by: Mikhail Berezovskiy <[email protected]>

test-fix elastic#121869

f83942e

mhl-b added >test Issues or PRs that are addressing/adding tests Team:Distributed Coordination Meta label for Distributed Coordination team v9.1.0 labels Feb 12, 2025

mhl-b requested a review from thecoop February 12, 2025 04:25

elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Distributed Coordination Meta label for Distributed Coordination team labels Feb 12, 2025

mhl-b added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. Team:Distributed Coordination Meta label for Distributed Coordination team and removed needs:triage Requires assignment of a team area label labels Feb 12, 2025

elasticsearchmachine added Team:Distributed Indexing Meta label for Distributed Indexing team and removed Team:Distributed Coordination Meta label for Distributed Coordination team labels Feb 12, 2025

[CI] Auto commit changes from spotless

07953e6

mhl-b changed the title ~~test-fix #121869~~ Test-fix testAcceptsMismatchedServerlessBuildHash #121869 Feb 12, 2025

mhl-b requested a review from nicktindall February 12, 2025 06:30

DiannaHohensee reviewed Feb 12, 2025

View reviewed changes

DiannaHohensee approved these changes Feb 12, 2025

View reviewed changes

mhl-b merged commit 49ecbca into elastic:main Feb 13, 2025
17 checks passed

ywangd mentioned this pull request Feb 14, 2025

[CI] TransportServiceHandshakeTests testAcceptsMismatchedServerlessBuildHash failing #122565

Closed

mosche added v8.18.1 v8.19.0 labels Feb 26, 2025

mosche added the v9.0.1 label Feb 26, 2025

mosche pushed a commit to mosche/elasticsearch that referenced this pull request Feb 26, 2025

Test-fix testAcceptsMismatchedServerlessBuildHash elastic#121869 (ela…

f0d5982

…stic#122332) (cherry picked from commit 49ecbca)

mosche mentioned this pull request Feb 26, 2025

[8.x] Test-fix testAcceptsMismatchedServerlessBuildHash #121869 (#122332) #123476

Merged

mosche pushed a commit to mosche/elasticsearch that referenced this pull request Feb 26, 2025

Test-fix testAcceptsMismatchedServerlessBuildHash elastic#121869 (ela…

c2a5cb7

…stic#122332) (cherry picked from commit 49ecbca)

mosche mentioned this pull request Feb 26, 2025

[9.0] Test-fix testAcceptsMismatchedServerlessBuildHash #121869 (#122332) #123477

Merged

mosche pushed a commit to mosche/elasticsearch that referenced this pull request Feb 26, 2025

Test-fix testAcceptsMismatchedServerlessBuildHash elastic#121869 (ela…

cf30e5f

…stic#122332) (cherry picked from commit 49ecbca)

mosche mentioned this pull request Feb 26, 2025

[8.18] Test-fix testAcceptsMismatchedServerlessBuildHash #121869 (#122332) #123478

Merged

elasticsearchmachine pushed a commit that referenced this pull request Feb 26, 2025

Test-fix testAcceptsMismatchedServerlessBuildHash #121869 (#122332) (#…

3743dde

…123477) (cherry picked from commit 49ecbca) Co-authored-by: Mikhail Berezovskiy <[email protected]>

elasticsearchmachine pushed a commit that referenced this pull request Feb 26, 2025

Test-fix testAcceptsMismatchedServerlessBuildHash #121869 (#122332) (#…

a62593c

…123478) (cherry picked from commit 49ecbca) Co-authored-by: Mikhail Berezovskiy <[email protected]>

elasticsearchmachine pushed a commit that referenced this pull request Feb 26, 2025

Test-fix testAcceptsMismatchedServerlessBuildHash #121869 (#122332) (#…

78c53db

…123476) (cherry picked from commit 49ecbca) Co-authored-by: Mikhail Berezovskiy <[email protected]>

	private void maybeThrowOnIncompatibleBuild(@Nullable DiscoveryNode node, @Nullable Exception e) {
	if (SERVERLESS_TRANSPORT_FEATURE_FLAG == false && isIncompatibleBuild(version, buildHash)) {
	throwOnIncompatibleBuild(node, e);
	}
	}

Test-fix testAcceptsMismatchedServerlessBuildHash #121869 #122332

Test-fix testAcceptsMismatchedServerlessBuildHash #121869 #122332

Uh oh!

Conversation

mhl-b commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 12, 2025

Uh oh!

elasticsearchmachine commented Feb 12, 2025

Uh oh!

mhl-b commented Feb 12, 2025

Uh oh!

thecoop commented Feb 12, 2025

Uh oh!

mhl-b commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DiannaHohensee left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiannaHohensee Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

mhl-b Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

mhl-b Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

DiannaHohensee Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

ywangd Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

mhl-b Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

ywangd Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiannaHohensee left a comment

Choose a reason for hiding this comment

Uh oh!

DiannaHohensee Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mosche commented Feb 26, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mhl-b commented Feb 12, 2025 •

edited

Loading

mhl-b commented Feb 12, 2025 •

edited

Loading

DiannaHohensee left a comment •

edited

Loading

ywangd Feb 13, 2025 •

edited

Loading