Relaxes jarHell check for optionally extended plugins #17893
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17893 +/- ##
============================================
+ Coverage 72.52% 72.55% +0.02%
+ Complexity 67031 67014 -17
============================================
Files 5470 5470
Lines 309707 309707
Branches 45052 45052
============================================
+ Hits 224617 224704 +87
+ Misses 66774 66591 -183
- Partials 18316 18412 +96 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This JAR hell check is indeed something which every OpenSearch plugin dev has nightmares about ;-) I tried to look into the code to understand what condition is verified by the particular check. It looks a bit like a check for a directed acyclic graph, but so far I did not have the time to completely understand the code. Could you maybe post the |
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
DarshitChanpura
force-pushed
the
relax-jar-hell-extended-plugins
branch
from
9c3daf7 to
b4d0299
Compare
|
@nibix Here are plugin-descriptor.properties from my local install for AD, JS and Sec plugin. Anomaly-Detection plugin-descriptor.properties### mandatory elements for all plugins:
#
# 'description': simple summary of the plugin
description=OpenSearch anomaly detector plugin
#
# 'version': plugin's version
version=3.0.0.0-beta1-SNAPSHOT
#
# 'name': the plugin name
name=opensearch-anomaly-detection
#
# 'classname': the name of the class to load, fully-qualified
classname=org.opensearch.timeseries.TimeSeriesAnalyticsPlugin
#
# 'java.version': version of java the code is built against
# use the system property java.specification.version
# version string must be a sequence of nonnegative decimal integers
# separated by "."'s and may have leading zeros
java.version=21
#
# 'opensearch.version': semantic version of opensearch the plugin is compatible with
# does not include -SNAPSHOT if compiled against a snapshot build
opensearch.version=3.0.0
#
### optional elements for plugins:
#
# 'custom.foldername': the custom name of the folder in which the plugin is installed
custom.foldername=
#
# 'extended.plugins': other plugins this plugin extends through SPI
extended.plugins=lang-painless,opensearch-job-scheduler,opensearch-security;optional=true
#
# 'has.native.controller': whether or not the plugin has a native controller
has.native.controller=falseJob Scheduler plugin-descriptor.properties### mandatory elements for all plugins:
#
# 'description': simple summary of the plugin
description=OpenSearch Job Scheduler plugin
#
# 'version': plugin's version
version=3.0.0.0-beta1-SNAPSHOT
#
# 'name': the plugin name
name=opensearch-job-scheduler
#
# 'classname': the name of the class to load, fully-qualified
classname=org.opensearch.jobscheduler.JobSchedulerPlugin
#
# 'java.version': version of java the code is built against
# use the system property java.specification.version
# version string must be a sequence of nonnegative decimal integers
# separated by "."'s and may have leading zeros
java.version=21
#
# 'opensearch.version': semantic version of opensearch the plugin is compatible with
# does not include -SNAPSHOT if compiled against a snapshot build
opensearch.version=3.0.0
#
### optional elements for plugins:
#
# 'custom.foldername': the custom name of the folder in which the plugin is installed
custom.foldername=
#
# 'extended.plugins': other plugins this plugin extends through SPI
extended.plugins=
#
# 'has.native.controller': whether or not the plugin has a native controller
has.native.controller=false
Security plugin-descriptor.properties### mandatory elements for all plugins:
#
# 'description': simple summary of the plugin
description=Provide access control related features for OpenSearch
#
# 'version': plugin's version
version=3.0.0.0-beta1-SNAPSHOT
#
# 'name': the plugin name
name=opensearch-security
#
# 'classname': the name of the class to load, fully-qualified
classname=org.opensearch.security.OpenSearchSecurityPlugin
#
# 'java.version': version of java the code is built against
# use the system property java.specification.version
# version string must be a sequence of nonnegative decimal integers
# separated by "."'s and may have leading zeros
java.version=21
#
# 'opensearch.version': semantic version of opensearch the plugin is compatible with
# does not include -SNAPSHOT if compiled against a snapshot build
opensearch.version=3.0.0
#
### optional elements for plugins:
#
# 'custom.foldername': the custom name of the folder in which the plugin is installed
custom.foldername=
#
# 'extended.plugins': other plugins this plugin extends through SPI
extended.plugins=
#
# 'has.native.controller': whether or not the plugin has a native controller
has.native.controller=false
|
DarshitChanpura
requested review from
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
bugmakerrrrrr,
cwperks,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
|
❌ Gradle check result for f17c456: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
The build seems to be failing for tasks unrelated to change in this PR FAILURE: Build completed with 2 failures.
1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':server:test'.
> There were failing tests. See the report at: file:///var/jenkins/workspace/gradle-check/search/server/build/reports/tests/test/index.html
==============================================================================
2: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':plugins:transport-reactor-netty4:javaRestTest'.
> There were failing tests. See the report at: file:///var/jenkins/workspace/gradle-check/search/plugins/transport-reactor-netty4/build/reports/tests/javaRestTest/index.html
==============================================================================
BUILD FAILED in 36m 8s
3211 actionable tasks: 3203 executed, 2 from cache, 6 up-to-date
Gradle Check Failed! |
> Task :server:test
Tests with failures:
- org.opensearch.search.aggregations.startree.RangeAggregatorTests.testRangeAggregation
15536 tests completed, 1 failed, 65 skipped
> Task :distribution:packages:buildDeb
> Task :server:test FAILED
...
> Task :plugins:transport-reactor-netty4:javaRestTest
REPRODUCE WITH: ./gradlew ':plugins:transport-reactor-netty4:javaRestTest' --tests "org.opensearch.rest.ReactorNetty4StreamingStressIT.testCloseClientStreamingRequest" -Dtests.seed=7F074DF9C12C2605 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=bs-Cyrl-BA -Dtests.timezone=Asia/Srednekolymsk -Druntime.java=21
ReactorNetty4StreamingStressIT > testCloseClientStreamingRequest FAILED
java.lang.Exception: Test abandoned because suite timeout was reached.
at __randomizedtesting.SeedInfo.seed([7F074DF9C12C2605]:0)
REPRODUCE WITH: ./gradlew ':plugins:transport-reactor-netty4:javaRestTest' --tests "org.opensearch.rest.ReactorNetty4StreamingStressIT.testCloseClientStreamingRequest" -Dtests.seed=7F074DF9C12C2605 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=bs-Cyrl-BA -Dtests.timezone=Asia/Srednekolymsk -Druntime.java=21
ReactorNetty4StreamingStressIT > classMethod FAILED
java.lang.Exception: Suite timeout exceeded (>= 1200000 msec).
at __randomizedtesting.SeedInfo.seed([7F074DF9C12C2605]:0) |
Local run for `:plugins:transport-reactor-netty4:javaRestTest` :➜ OpenSearch git:(relax-jar-hell-extended-plugins) ./gradlew clean :plugins:transport-reactor-netty4:javaRestTest
Starting a Gradle Daemon, 1 busy and 1 incompatible Daemons could not be reused, use --status for details
> Task :buildSrc:compileJava
Note: XXXX/OpenSearch/buildSrc/src/main/java/org/opensearch/gradle/agent/JavaAgent.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :buildSrc:compileGroovy
Note: XXXX/OpenSearch/buildSrc/src/main/groovy/org/opensearch/gradle/test/TestWithSslPlugin.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: XXXX/OpenSearch/buildSrc/src/main/groovy/org/opensearch/gradle/test/TestWithSslPlugin.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
> Configure project :
========================= WARNING =========================
Backwards compatibility tests are disabled!
See https://github.com/opensearch-project/OpenSearch/issues/4173
===========================================================
=======================================
OpenSearch Build Hamster says Hello!
Gradle Version : 8.13
OS Info : Mac OS X 15.4 (aarch64)
JDK Version : 21 (Eclipse Temurin JDK)
JAVA_HOME : XXXX/.sdkman/candidates/java/21.0.4-tem
Random Testing Seed : B4024075360986AD
In FIPS 140 mode : false
=======================================
> Task :libs:agent-sm:agent:compileJava
...
28 warnings
> Task :server:compileJava
Note: Processing Log4j annotations
Note: Annotations processed
Note: Processing OpenSearch Api annotations
Note: Processing Log4j annotations
Note: No elements to process
Note: Processing OpenSearch Api annotations
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
> Task :modules:reindex:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :plugins:transport-reactor-netty4:compileJava
Note: XXXX/OpenSearch/plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransport.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :modules:aggs-matrix-stats:compileJava
Note: XXXX/OpenSearch/modules/aggs-matrix-stats/src/main/java/org/opensearch/search/aggregations/matrix/MatrixAggregationModulePlugin.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :modules:lang-expression:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :modules:lang-mustache:compileJava
Note: XXXX/OpenSearch/modules/lang-mustache/src/main/java/org/opensearch/script/mustache/MustacheScriptEngine.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :modules:geo:compileJava
Note: XXXXOpenSearch/modules/geo/src/main/java/org/opensearch/geo/GeoModulePlugin.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
> Task :modules:parent-join:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :modules:lang-painless:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :modules:repository-url:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :distribution:tools:upgrade-cli:compileJava
Note: XXXX/OpenSearch/distribution/tools/upgrade-cli/src/main/java/org/opensearch/tools/cli/upgrade/DetectEsInstallationTask.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :distribution:tools:plugin-cli:compileJava
Note: XXXX/OpenSearch/distribution/tools/plugin-cli/src/main/java/org/opensearch/tools/cli/plugin/InstallPluginCommand.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :modules:percolator:compileJava
Note: XXXX/OpenSearch/modules/percolator/src/main/java/org/opensearch/percolator/PercolateQueryBuilder.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :modules:transport-netty4:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :modules:analysis-common:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :test:framework:compileJava
Note: Processing Log4j annotations
Note: Annotations processed
Note: Processing Log4j annotations
Note: No elements to process
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
> Task :plugins:transport-reactor-netty4:compileJavaRestTestJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :plugins:transport-reactor-netty4:javaRestTest
WARNING: Using incubator modules: jdk.incubator.vector
[Incubating] Problems report is available at: file:///XXXX/OpenSearch/build/reports/problems/problems-report.html
BUILD SUCCESSFUL in 1m 36s
372 actionable tasks: 285 executed, 23 from cache, 64 up-to-date
➜ OpenSearch git:(relax-jar-hell-extended-plugins)
Local run for `:server:test` :➜ OpenSearch git:(relax-jar-hell-extended-plugins) ./gradlew :server:test --tests 'org.opensearch.search.aggregations.startree.RangeAggregatorTests.testRangeAggregation'
> Configure project :
========================= WARNING =========================
Backwards compatibility tests are disabled!
See https://github.com/opensearch-project/OpenSearch/issues/4173
===========================================================
=======================================
OpenSearch Build Hamster says Hello!
Gradle Version : 8.13
OS Info : Mac OS X 15.4 (aarch64)
JDK Version : 21 (Eclipse Temurin JDK)
JAVA_HOME : XXXX/.sdkman/candidates/java/21.0.4-tem
Random Testing Seed : ED1CF78D9ABCB15D
In FIPS 140 mode : false
=======================================
> Task :server:compileTestJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
> Task :server:test
WARNING: Using incubator modules: jdk.incubator.vector
[Incubating] Problems report is available at: file:///XXXX/OpenSearch/build/reports/problems/problems-report.html
BUILD SUCCESSFUL in 29s
62 actionable tasks: 6 executed, 1 from cache, 55 up-to-date
➜ OpenSearch git:(relax-jar-hell-extended-plugins)
|
Indeed, there is no issue if the types in question are not used. However, I do not believe that there's a simple way to test this automatically. |
I don't think this is the case. Its not the case at compile time and I don't see the jars included at runtime either. Edit: Taking a deeper look at https://github.com/opensearch-project/OpenSearch/blob/main/libs/plugin-classloader/src/main/java/org/opensearch/plugin/classloader/ExtendedPluginsClassLoader.java |
|
❌ Gradle check result for f17c456: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
@cwperks and I did some offline experimentation to deep-dive, and it does seem like extended-plugin's jars are in fact made available to extending plugin. I guess that is similar to declaring compileOnly dependency on SPI and then at run-time the SPI classes be supplied by the extended plugin. However, in the experimentation, we also discovered that the ImmutableList class was able to be deterministically resolved to opensearch-job-scheduler. Next, we removed guava completely from JS, then the ImmutableList class resolved to guava jar supplied by security plugin. Our understanding is that the conflicting classes are resolved deterministically from extended-plugins, most-likely in an alphabetical order for the extended plugin name. @cwperks will be adding a comment soon with more details. |
|
I learned something new through analyzing the classloading here. When pluginA extends pluginB, then pluginA will be able to resolve to any class on pluginB's runtime classpath (at runtime) because of how the parent classloader is assembled. Anomaly detection already takes advantage of this with a compileOnly dependency on guava. At runtime, AD is able to resolve to classes from guava because guava is available on job-scheduler's runtime classpath and how the parent classloader is configured in PluginsService. Both AD and Job-Scheduler do not rely on the version catalog for their version of guava, so there definitely could be version mismatch already happening. By applying the change in this PR, we have verified that guava deps would still be available on the runtime classpath for AD and its resolved in the order in which the plugin is extended in the extending plugin's build.gradle file. Since there already isn't enforcement that the same version of the dep is used at compile time and runtime, I don't think the additional check to make sure that optionally extended plugins have disjointed deps adds much value. I think this PR is reasonable. |
|
❕ Gradle check result for f17c456: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
I would disagree in this regard; even though might be not the case in this particular setup, the additonal check makes sure that |
…ject#17893) * Relaxes jarHell check for optionally extended plugins Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds changelog entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes Changelog entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Harsh Kothari <techarsh@amazon.com>
…ject#17893) * Relaxes jarHell check for optionally extended plugins Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds changelog entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Fixes Changelog entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Harsh Kothari <techarsh@amazon.com>
3 participants
Description
While testing opensearch-project/security#5240 on local-cluster setup, I ran into jarHell issues for extended plugins. This is because until now we only had plugins extend 1 plugin, mainly
opensearch-job-schedulerin addition tolang-painless, except security-analytics which extendsopensearch-alertingas well.With this new feature, we require
opensearch-securitywith optional flag set to true for all resource declaring plugins aiming to implement access control to the resources defined by them. Now, when we tried to installopensearch-anomaly-detectionplugin with relevant changes, we encountered jarHell for common dependencies:Upon taking a deeper dive I was able to root-cause it to PluginsService#checkBundleJarHell(). While I'm not 100% aware on why this check was added, I firmly believe that this check can be relaxed. So for now I have relaxed it for optionally extended plugins but if there is consensus we should skip the check for all extended plugins.
Following is the log with this change:
Thoughts?
Related Issues
Unblocks opensearch-project/security#4500
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.