Fix hdfs-related IT tests for java24 #122044

ldematte · 2025-02-07T15:16:51Z

HDFS IT tests started failing with Java 24.

The problem is that some hadoop libraries (mainly hdfs-common) contain a call to a SecurityManager related method that has been removed in JDK 24.
A short term solution is to patch hdfs-fixture as we do for hadoop-client-api (same as we did in #119779)

Fixes #121967
Fixes #122377
Fixes #122378

test/fixtures/hdfs-fixture/build.gradle

ldematte · 2025-02-07T15:19:39Z

...-hdfs/hadoop-common-patcher/src/main/java/org/elasticsearch/hdfs/patch/HdfsClassPatcher.java

@@ -41,18 +41,17 @@ public static void main(String[] args) throws Exception {
        try (JarFile jarFile = new JarFile(new File(jarPath))) {
            for (var patcher : patchers.entrySet()) {
                JarEntry jarEntry = jarFile.getJarEntry(patcher.getKey());
-                if (jarEntry == null) {
-                    throw new IllegalArgumentException("path [" + patcher.getKey() + "] not found in [" + jarPath + "]");


This had to go, as some classes are not present in both jars; it's unfortunate, but I think acceptable. If it's not, should we pass the classes to patch via the command line, so it is configurable in each gradle JavaExec task?

we should pass the files to patch down, it should be part of the configuration of the task. we can keep how to actually patch the classes here, but the list of classes to patch be passed in.

However, why would the same class be present in two different jars? That's jarhell, which we should be rejecting already?

I should have said both projects; the hdsf-fixture projects needs a subset of the patches, as it pulls in a different (smaller) set of dependencies.

I'm still missing something. Why do dependencies matter? We should only be patching a given class once.

The classes we need to patch are in the 2 project's dependencies: for hadoop-client-api, they are in both hadoop-common and hadoop-auth; hdfs-fixture however depends only on hadoop-common (via hadoop-minicluster), so trying to patch classes in hadoop-auth result in an error (the class is not there).
But like you said, this can be solved by making the patcher get a list of the class names.

rjernst

I'll let @mark-vieira comment on the gradle, but I suspect we may want this to be a gradle plugin so we don't eg have to duplicate the patchClasses task and setup.

rjernst · 2025-02-07T16:45:55Z

...-hdfs/hadoop-common-patcher/src/main/java/org/elasticsearch/hdfs/patch/HdfsClassPatcher.java

@@ -41,18 +41,17 @@ public static void main(String[] args) throws Exception {
        try (JarFile jarFile = new JarFile(new File(jarPath))) {
            for (var patcher : patchers.entrySet()) {
                JarEntry jarEntry = jarFile.getJarEntry(patcher.getKey());
-                if (jarEntry == null) {
-                    throw new IllegalArgumentException("path [" + patcher.getKey() + "] not found in [" + jarPath + "]");


we should pass the files to patch down, it should be part of the configuration of the task. we can keep how to actually patch the classes here, but the list of classes to patch be passed in.

However, why would the same class be present in two different jars? That's jarhell, which we should be rejecting already?

ldematte · 2025-02-07T17:00:55Z

'll let @mark-vieira comment on the gradle, but I suspect we may want this to be a gradle plugin so we don't eg have to duplicate the patchClasses task and setup.

That would be ideal I think. Maybe we can work on this together, so I learn a bit about gradle plugins!

breskeby · 2025-02-11T14:01:09Z

@ldematte This is more or less the perfect use case to use Gradle Artefact Transforms. The goal of those artefact transforms is to tweak third party dependencies before using them in your build.

The benefits in general are IMO

the transformations is done transparent to the usage as part of the dependency resolution
we have built in caching support
less clutter and custom build logic in our gradle files.
is also that caching is build in supported and the project using these transforms is cleaner.
Also we probably can probably broaden its usage and reusability in the future as we 'patch' other dependencies as well (e.g. jwt)

I pushed a draft on how to port this PR to use artefact transforms to https://github.com/breskeby/elasticsearch/tree/fix-hdfs-tests-java24-artifact-transforms

Happy to talk about this synchronously and help you bring that over the line. Parts of what's living in the build script in my example likely can be moved to a plugin.

breskeby

I strongly prefer using gradle artefact transforms for these kind of dependency patching. Details at #122044 (comment)

ldematte · 2025-02-11T16:44:40Z

Thanks Rene, I'll update it as we discussed and make it parametric, so we can have back the check that the expected classes are all patched.

rjernst · 2025-02-11T19:01:35Z

@breskeby What does the resulting artifact naming look like? One nice thing currently about the way we do this kind of patching is that we can make clear it's elastic-modified (not that we do change the name in this case currently, but in other cases eg with log4j we have tweaked the name).

breskeby · 2025-02-11T19:04:50Z

We can choose the naming in our implementation. In the example branch I just added "patched" to the input jar. So it would be "hadoop-common-2.8.7.jar" -> "hadoop-common-2.8.7-patched.jar"

plugins/repository-hdfs/hadoop-client-api/build.gradle

…arch into fix-hdfs-tests-java24

elasticsearchmachine · 2025-02-19T10:25:05Z

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

ldematte · 2025-02-19T10:26:29Z

I moved the patcher implementation to a transformer as @breskeby suggested, and added the ability to specify (via a parameter) which set of classes to patch for which jar/artifact.

breskeby

This is great and getting rid of hadoop-client-api subproject is a very nice sideeffect. I added a few nitpicks which would be great to be addressed before merging.

Otherwise LGTM.

breskeby · 2025-02-19T11:25:22Z

.../main/java/org/elasticsearch/gradle/internal/dependencies/patches/hdfs/HdfsClassPatcher.java

+                // Add the entry to the new JAR file
+                jos.putNextEntry(new JarEntry(entryName));
+
+                Function<ClassWriter, ClassVisitor> classPatcher = jarPatchers.get(entryName);


I wonder if we should add a check here to verify all expected patches are applied and fail early if not to avoid having undetected changes sneaked into this again by e.g. updating hadoop versions.

I tried to do that and it's a great idea; however there is one big blocker for this: the hadoop-common artifact has one additional "classifier" named tests, which contains only tests of course but is brought in anyway.
I tried to exclude it but apparently it's not possible to do that in gradle (there is limited support for classifiers).
The thing is that hadoop-common is applied to both hadoop-common jars (regular and "tests"), and it will fail on the second, because none of the expected patches will be applied there.

TL;DR: if you have a good way to either exclude hadoop-common:tests or to recognize it and avoid trying to patch it I'm all ears!

We could tweak the regex to match the file name being stricter in a sense of expected name + version regex instead of just a simple string.contains based check to fix to not match -tests artifacts

I thought about that; I discarded it because "2.8.5-tests" is a valid "version" (isn't it? like 1.0.0-beta).
I can allow only the numeric part; it's not super clean but..
I would have preferred excluding the jar completely, since it's only tests (no purpose having it), but seems that this is very hard to do in gradle

Let me try and see the regex

I had to deal with that test classifier stuff with hadoop before and suffered myself. okay. lets just live with that for now. we can revisit later if we feel the need. I will use parts of that stuff later on to fix other places in our codebase.

plugins/repository-hdfs/build.gradle

test/fixtures/hdfs-fixture/build.gradle

…he matched artifacts

…arch into fix-hdfs-tests-java24

ldematte · 2025-02-19T16:13:06Z

.../main/java/org/elasticsearch/gradle/internal/dependencies/patches/hdfs/HdfsClassPatcher.java

+    static final List<JarPatchers> allPatchers = List.of(
+        new JarPatchers(
+            "hadoop-common",
+            Pattern.compile("hadoop-common-(?!.*tests)"),


@breskeby I added regex matching to bring back the "have we applied all patches" check; let me know if you like it or not

elasticsearchmachine · 2025-02-19T17:28:29Z

💔 Backport failed

Status	Branch	Result
❌	9.0	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 122044

This reverts commit decc551.

#122044 solved (temporarily) the issue with HDFS libraries not being compatible with JDK 24. This PR unmutes a test with the same issues Solves #122024

* Fix hdfs-related IT tests for java24 (#122044) * Have ASM recompute frames on patched classes

ldematte added 2 commits February 7, 2025 16:10

Use hadoop patcher for hdfs-fixture too

3fc401d

Merge remote-tracking branch 'upstream/main' into fix-hdfs-tests-java24

d484793

ldematte added >test Issues or PRs that are addressing/adding tests :Delivery/Build Build or test infrastructure :Core/Infra/Core Core issues without another label auto-backport Automatically create backport pull requests when merged v9.0.1 v9.1.0 labels Feb 7, 2025

ldematte requested a review from breskeby February 7, 2025 15:16

ldematte commented Feb 7, 2025

View reviewed changes

test/fixtures/hdfs-fixture/build.gradle Outdated Show resolved Hide resolved

ldematte commented Feb 7, 2025

View reviewed changes

ldematte requested review from mark-vieira and rjernst February 7, 2025 15:19

rjernst reviewed Feb 7, 2025

View reviewed changes

breskeby requested changes Feb 11, 2025

View reviewed changes

ldematte added 3 commits February 17, 2025 11:41

Merge remote-tracking branch 'upstream/main' into fix-hdfs-tests-java24

08f59d6

Try to use gradle transformer, not working

6de0f88

Removing patcher project

537cdd7

ldematte commented Feb 17, 2025

View reviewed changes

plugins/repository-hdfs/hadoop-client-api/build.gradle Outdated Show resolved Hide resolved

elasticsearchmachine and others added 6 commits February 17, 2025 12:18

[CI] Auto commit changes from spotless

1d389bf

Patcher fix

bb497df

Merge branch 'fix-hdfs-tests-java24' of github.com:ldematte/elasticse…

473ee30

…arch into fix-hdfs-tests-java24

Merge remote-tracking branch 'upstream/main' into fix-hdfs-tests-java24

f715051

Fix repository-hdfs gradle to perform transformation directly

4674a0b

Add test classpaths

af8fbd9

ldematte requested a review from breskeby February 19, 2025 10:26

breskeby approved these changes Feb 19, 2025

View reviewed changes

ldematte added 6 commits February 19, 2025 14:53

Reorder gradle file

4646fdc

Regex?

affaf73

Merge branch 'main' into fix-hdfs-tests-java24

ec9e749

Add hdfs3 fixture too

791c56b

Use regex to match artifact; add check that we patch all classes in t…

cecb60b

…he matched artifacts

Merge branch 'fix-hdfs-tests-java24' of github.com:ldematte/elasticse…

256d104

…arch into fix-hdfs-tests-java24

ldematte commented Feb 19, 2025

View reviewed changes

ldematte enabled auto-merge (squash) February 19, 2025 16:17

ldematte merged commit 340a2ce into elastic:main Feb 19, 2025
17 checks passed

ldematte deleted the fix-hdfs-tests-java24 branch February 19, 2025 17:27

elasticsearchmachine added the backport pending label Feb 19, 2025

ldematte added a commit that referenced this pull request Feb 20, 2025

Fix hdfs-related IT tests for java24 (#122044)

decc551

ldematte added a commit that referenced this pull request Feb 20, 2025

Revert "Fix hdfs-related IT tests for java24 (#122044)"

f8a0e89

This reverts commit decc551.

ldematte added a commit to ldematte/elasticsearch that referenced this pull request Feb 20, 2025

Fix hdfs-related IT tests for java24 (elastic#122044)

42cf38b

ldematte mentioned this pull request Feb 20, 2025

[9.0] Fix hdfs-related IT tests for java24 (#122044) #122987

Merged

ldematte added a commit to ldematte/elasticsearch that referenced this pull request Feb 20, 2025

Fix hdfs-related IT tests for java24 (elastic#122044)

de7031a

ldematte mentioned this pull request Feb 20, 2025

[8.x] Fix hdfs-related IT tests for java24 (#122044) #122988

Merged

ldematte added a commit to ldematte/elasticsearch that referenced this pull request Feb 20, 2025

Fix hdfs-related IT tests for java24 (elastic#122044)

4e5de2c

This was referenced Feb 20, 2025

[8.18] Fix hdfs-related IT tests for java24 (#122044) #122989

Merged

Unmute HdfsSearchableSnapshotsIT #122992

Merged

ldematte removed the backport pending label Feb 20, 2025

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

Unmute HdfsSearchableSnapshotsIT (#122992)

f142e48

#122044 solved (temporarily) the issue with HDFS libraries not being compatible with JDK 24. This PR unmutes a test with the same issues Solves #122024

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

[8.x] Fix hdfs-related IT tests for java24 (#122044) (#122988)

129a1e8

* Fix hdfs-related IT tests for java24 (#122044) * Have ASM recompute frames on patched classes

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

[8.18] Fix hdfs-related IT tests for java24 (#122044) (#122989)

e304372

* Fix hdfs-related IT tests for java24 (#122044) * Have ASM recompute frames on patched classes

ldematte added a commit that referenced this pull request Feb 20, 2025

[9.0] Fix hdfs-related IT tests for java24 (#122044) (#122987)

0348122

Fix hdfs-related IT tests for java24 #122044

Fix hdfs-related IT tests for java24 #122044

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldematte commented Feb 7, 2025

Uh oh!

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

Uh oh!

Uh oh!

breskeby left a comment

Choose a reason for hiding this comment

Uh oh!

ldematte commented Feb 11, 2025

Uh oh!

rjernst commented Feb 11, 2025

Uh oh!

breskeby commented Feb 11, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 19, 2025

Uh oh!

ldematte commented Feb 19, 2025

Uh oh!

breskeby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 19, 2025

💔 Backport failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldematte commented Feb 7, 2025 •

edited

Loading

breskeby commented Feb 11, 2025 •

edited

Loading