Enable testing for ExtensiblePlugins using classpath plugins #16908

cwperks · 2024-12-23T21:52:44Z

Description

This PR enables testing for ExtensiblePlugins using the classpath plugin pattern present in internalClusterTest.

To do this, this PR tries to load all extensions for classpath plugins by defining an extendedPlugin relationship between a classpath plugin and all other classpath plugins. In normal scenarios, a plugin will declare that it extends another plugin through the extendedPlugins keyword that it places in build.gradle (example). This PR also puts logic in place to ensure that any extensions are only loaded once.

Additional Context

I ran into this issue while using the integration test framework in the security plugin to test out a change I have been experimenting with. The integration test framework utilizes classpath plugins and it was impossible to load an extensible plugin and its extension simultaneously and have the extensions loaded.

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

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.

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2024-12-23T22:28:33Z

❌ Gradle check result for 4176c46: 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 · 2025-01-07T20:04:20Z

@reta Could you take a look at this small PR? Its a PR related to testing. Instead of trying to create relationships for extending -> extended plugins for classpath plugins it haphazardly tries to loadExtensions for classpathPlugins which guarantees that at some point loadExtensions will be called for the correct extended plugin.

@cwperks I am not sure I understand this change:

this is not the way we load plugins (in production) but we change the logic of PluginService

we do have mechanisms to run internal cluster tests with plugins, we should be using it for extensible plugins as well I think

That's the current problem with classpathPlugins, there is no way to define a relationship from classpathPluginA -> classpathPluginB. The reason for that is because its always putting an empty list here and does not call loadExtensions since it always assumes empty list.

This PR creates a haphazard way to solve this problem where each classpathPlugin extends all other classpathPlugins so that it tries to load the extensions. If extensions don't exist it will proceed as normal.

Since its haphazard, the same extensions may be loaded multiple times so this PR also adds logic to ensure they are loaded once. I chose to do it this way to minimize the changes, but if its more desirable to have a mechanism to define relationships between classpathPluginA -> classpathPluginB then I would need to revisit what changes are required.

reta · 2025-01-07T20:31:54Z

That's the current problem with classpathPlugins, there is no way to define a relationship from classpathPluginA -> classpathPluginB. The reason for that is because its always putting an empty list here and does not call loadExtensions since it always assumes empty list.

To my understanding, the classpathPlugins were quick patch to support limited plugin testing scenarios. May be the cleaner way would be to introduce the alternative constructor to PluginsService which accepts Collection<PluginInfo> classpathPlugins instead of raw Plugins?

    public PluginsService(
        Settings settings,
        Path configPath,
        Path modulesDirectory,
        Path pluginsDirectory,
        Collection<PluginInfo> classpathPlugins
    ) {

The best option would certainly to not build on top of that and use dedicated test plugin, packaged and installed as a plugin instead, I believe.

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2025-01-08T02:46:18Z

❌ Gradle check result for 7ddd9a3: 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?

github-actions · 2025-01-08T03:56:30Z

❌ Gradle check result for 7ddd9a3: 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?

github-actions · 2025-01-08T05:32:34Z

✅ Gradle check result for 7ddd9a3: SUCCESS

server/src/main/java/org/opensearch/plugins/PluginsService.java

reta · 2025-01-21T15:13:51Z

test/framework/src/main/java/org/opensearch/node/MockNode.java

+        final Collection<Class<? extends Plugin>> classpathPlugins,
+        final Path configPath,
+        final boolean forbidPrivateIndexSettings,
+        final Map<Class<? extends Plugin>, Class<? extends Plugin>> extendedPlugins


Why do we need extendedPlugins here if we already have classpathPlugins ? If we need to use PluginInfo here, let's do that instead

Because extendedPlugins contains the relationships between the classpath plugins.

i.e. classpathPluginA extends classpathPluginB.

But this has to pass though PluginInfo::extendedPlugins, right? This is how the relationship between plugins is designed?

It does in one of the constructors in this class. I chose to support multiple different constructors to continue to support use-cases such as this. For the majority of tests using classpathPlugins they don't need to declare extensions. For test cases that would need to declare extensions, they can use the respective constructor that supports it.

I would suggest to have 2 constructors, as we did for Node:

one that accepts final Collection<Class<? extends Plugin>> classpathPlugins,

one that accepts final Collection<PluginInfo> classpathPlugins,

Got it, I will need some time to analyze the different usages of the 7 different constructors in this class. It looks like this class has evolved over time to support many different scenarios for testing.

github-actions · 2025-01-21T15:25:05Z

❌ Gradle check result for 07df814: 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?

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2025-01-21T17:12:31Z

✅ Gradle check result for df48c29: SUCCESS

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2025-01-21T22:05:44Z

✅ Gradle check result for a4937bd: SUCCESS

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2025-01-22T02:51:32Z

❌ Gradle check result for 49ec944: 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?

Signed-off-by: Craig Perkins <[email protected]>

github-actions · 2025-01-22T04:02:24Z

❕ Gradle check result for f9bc2ae: 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.

server/src/main/java/org/opensearch/plugins/PluginsService.java

reta · 2025-01-22T19:33:35Z

test/framework/src/main/java/org/opensearch/test/NodeConfigurationSource.java

@@ -63,4 +64,9 @@ public Path nodeConfigPath(int nodeOrdinal) {
    public Collection<Class<? extends Plugin>> nodePlugins() {
        return Collections.emptyList();
    }
+
+    /** Returns a map corresponding to plugin dependencies to other classpath plugins */
+    public Map<Class<? extends Plugin>, Class<? extends Plugin>> extendedPlugins() {


I think this is the last place where we need PluginInfo (from classpath): public Class<PluginInfo> classpathPlugins() {

cwperks added 7 commits October 14, 2024 23:50

Make extended plugins optional

44154b5

Signed-off-by: Craig Perkins <[email protected]>

Make extended plugins optional

9501854

Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'main' into optional-extended-plugins

9b3f406

Load extensions for classpath plugins

44a4042

Signed-off-by: Craig Perkins <[email protected]>

Ensure only single instance for each classpath extension

7b4d7d7

Signed-off-by: Craig Perkins <[email protected]>

Add test for classpath plugin extended plugin loading

1614706

Signed-off-by: Craig Perkins <[email protected]>

Enable testing for ExtensiblePlugins using classpath plugins

4176c46

Signed-off-by: Craig Perkins <[email protected]>

Define PluginInfos from test files for classpath plugins

7ddd9a3

Signed-off-by: Craig Perkins <[email protected]>

This was referenced Jan 11, 2025

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStoreStatsIT #14310

Open

[AUTOCUT] Gradle Check Flaky Test Report for ResourceAwareTasksTests #14293

Open

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStorePinnedTimestampsGarbageCollectionIT #16088

Open

Merge branch 'main' into extend-classpath-plugins

07df814

reta reviewed Jan 21, 2025

View reviewed changes

server/src/main/java/org/opensearch/plugins/PluginsService.java Show resolved Hide resolved

reta reviewed Jan 21, 2025

View reviewed changes

server/src/main/java/org/opensearch/plugins/PluginsService.java Outdated Show resolved Hide resolved

reta reviewed Jan 21, 2025

View reviewed changes

Add testing constructor for PluginsService

df48c29

Signed-off-by: Craig Perkins <[email protected]>

This was referenced Jan 21, 2025

[AUTOCUT] Gradle Check Flaky Test Report for SpecificClusterManagerNodesIT #15944

Open

[AUTOCUT] Gradle Check Flaky Test Report for RemoteIndexShardTests #16116

Open

Single call to loadExtensions

a4937bd

Signed-off-by: Craig Perkins <[email protected]>

cwperks added 2 commits January 21, 2025 20:52

Merge branch 'main' into extend-classpath-plugins

49ec944

Reduce number of MockNode constructors

4cf427c

Signed-off-by: Craig Perkins <[email protected]>

Create 2 public constructors for MockNode

f9bc2ae

Signed-off-by: Craig Perkins <[email protected]>

This was referenced Jan 22, 2025

[AUTOCUT] Gradle Check Flaky Test Report for MinimumClusterManagerNodesIT #14289

Open

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStoreIT #16145

Open

reta reviewed Jan 22, 2025

View reviewed changes

server/src/main/java/org/opensearch/plugins/PluginsService.java Show resolved Hide resolved

reta reviewed Jan 22, 2025

View reviewed changes

opensearch-ci-bot mentioned this pull request Jan 24, 2025

[AUTOCUT] Gradle Check Flaky Test Report for AzureBlobStoreRepositoryTests #14291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable testing for ExtensiblePlugins using classpath plugins #16908

Enable testing for ExtensiblePlugins using classpath plugins #16908

cwperks commented Dec 23, 2024

github-actions bot commented Dec 23, 2024

cwperks commented Jan 7, 2025

reta commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

reta Jan 21, 2025

cwperks Jan 21, 2025 •

edited

Loading

reta Jan 21, 2025

cwperks Jan 21, 2025

reta Jan 21, 2025

cwperks Jan 21, 2025

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

reta Jan 22, 2025

Enable testing for ExtensiblePlugins using classpath plugins #16908

Are you sure you want to change the base?

Enable testing for ExtensiblePlugins using classpath plugins #16908

Conversation

cwperks commented Dec 23, 2024

Description

Additional Context

Check List

github-actions bot commented Dec 23, 2024

cwperks commented Jan 7, 2025

reta commented Jan 7, 2025 • edited Loading

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

reta Jan 21, 2025

Choose a reason for hiding this comment

cwperks Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

reta Jan 21, 2025

Choose a reason for hiding this comment

cwperks Jan 21, 2025

Choose a reason for hiding this comment

reta Jan 21, 2025

Choose a reason for hiding this comment

cwperks Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

reta Jan 22, 2025

Choose a reason for hiding this comment

reta commented Jan 7, 2025 •

edited

Loading

cwperks Jan 21, 2025 •

edited

Loading