Allowing custom folder name for plugin installation #848

VachaShah · 2021-06-15T06:29:44Z

Signed-off-by: Vacha Shah vachshah@amazon.com

Description

This PR allows the user to add a custom folder name for plugin cli installation. This custom folder name can be specified in the plugin properties file for the plugin, when this property is specified, the plugin would be installed with the custom folder name otherwise it will be installed with the name of the plugin. This feature would be available for versions after 1.0.0.

The feature includes:

Support for custom folder name for plugin installation after 1.0.0 versions with backwards compatibility.
The install plugin command will display the folder name where the plugin is being installed.
The list plugin command will continue to display the plugin name to maintain backwards compatibility.
The remove plugin command will continue to take in the plugin name to remove it even though the plugin is installed in a custom folder to maintain backwards compatibility.
During installation or removal, when checking if a plugin already exists, any folder previously installed with the plugin name or custom name would be determined.

Testing

For manual testing, I added the customFolderName property in build.gradle for job-scheduler plugin as follows:

opensearchplugin {    
  name 'opensearch-job-scheduler'    
  description 'OpenSearch Job Scheduler plugin'    
  classname 'org.opensearch.jobscheduler.JobSchedulerPlugin'
  customFolderName 'custom-folder'
}

When installing the plugin with the above properties using ./opensearch-plugin install <path to build file>, this resulted in the plugin being installed in folder: custom-folder with an output message as follows:

-> Installed opensearch-job-scheduler with folder name custom-folder

When the plugins are being listed using ./opensearch-plugin list, this resulted in the output: opensearch-job-scheduler.
To remove the plugin, I used ./opensearch-plugin remove opensearch-job-scheduler.

Issues Resolved

#686

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

opensearch-ci-bot · 2021-06-15T06:33:35Z

✅ Gradle Wrapper Validation success 838229ded3c80bc2764a66935638cf71e7cfbf1b

opensearch-ci-bot · 2021-06-15T06:33:35Z

✅ DCO Check Passed 838229ded3c80bc2764a66935638cf71e7cfbf1b

opensearch-ci-bot · 2021-06-15T06:35:44Z

✅ Gradle Precommit success 838229ded3c80bc2764a66935638cf71e7cfbf1b

dblock

Good. Some should haves.

dblock · 2021-06-15T20:36:03Z

buildSrc/src/main/groovy/org/opensearch/gradle/plugin/PluginBuildPlugin.groovy

@@ -97,6 +98,9 @@ class PluginBuildPlugin implements Plugin<Project> {
            if (extension1.classname == null) {
                throw new InvalidUserDataException('classname is a required setting for opensearchplugin')
            }
+            if (extension1.folderName == null) {


Why is folder defaulted to a empty string and not left alone as null?

The folder name property is added in plugin-descriptor.properties so when copying these properties over, the logic needs some value of folder name, by default String gets converted to null and it is not able to read that. Other optional properties like extendedPlugins also have a default added but it is a list so an empty list works for that. However, I have moved this default for folder name to PluginPropertiesExtension.java so the check here would not be needed.

You could fix this by assigning the value in the constructor. Then you don't even need a method that returns id or folderName depending on whether the latter is set.

dblock · 2021-06-15T20:37:48Z

distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java

@@ -864,16 +878,11 @@ private PluginInfo installPlugin(Terminal terminal, boolean isBatch, Path tmpRoo
        }
        PluginSecurity.confirmPolicyExceptions(terminal, permissions, isBatch);

-        final Path destination = env.pluginsFile().resolve(info.getName());
+        String folderName = (info.getFolderName() == null || info.getFolderName().isEmpty()) ? info.getName() : info.getFolderName();


Should be a method on info, e.g. info.getTargetFolderName() for example, and reused in a couple of places where we try to do this condition.

Added a method getTargetFolderName() for PluginInfo.

This is not a must have, but you could probably do what I am suggesting above, set the value of folderName in the constructor and then you don't even need getTargetFolderName - folderName would have the correct value all the time, defaulting to id.

dblock · 2021-06-15T20:38:18Z

...ibution/tools/plugin-cli/src/test/java/org/opensearch/plugins/InstallPluginCommandTests.java

+                    "classname",
+                    "FakePlugin",
+                    "folderName",
+                    "customFolder"


Since it's a folder, let's change the example to custom-folder, we don't want to encourage camelCase folder names.

Thats true! Updated all the custom folder names to kebab-case.

dblock · 2021-06-15T20:39:37Z

...ibution/tools/plugin-cli/src/test/java/org/opensearch/plugins/InstallPluginCommandTests.java

+    }
+
+    static void writePlugin(String name, boolean withCustomFolder, Path structure, String... additionalProps) throws IOException {
+        if (withCustomFolder) {


Pass in the actual folder name (or null) instead of a boolean, and set it instead of doing this if. Avoids duplicating this.

Updated this.

opensearch-ci-bot · 2021-06-16T06:23:38Z

✅ DCO Check Passed fc944e6d78281cd183c91826abd8467f00c728aa

opensearch-ci-bot · 2021-06-16T06:23:38Z

✅ Gradle Wrapper Validation success fc944e6d78281cd183c91826abd8467f00c728aa

opensearch-ci-bot · 2021-06-16T06:30:54Z

✅ Gradle Precommit success fc944e6d78281cd183c91826abd8467f00c728aa

dblock

Looks a lot cleaner, just some small stuff.

buildSrc/src/main/java/org/opensearch/gradle/plugin/PluginPropertiesExtension.java

dblock · 2021-06-16T14:12:59Z

...ibution/tools/plugin-cli/src/test/java/org/opensearch/plugins/InstallPluginCommandTests.java

            ),
            Arrays.stream(additionalProps)
        ).toArray(String[]::new);
+        if (folderName != null) {


Oh yes I missed this, removed it.

dblock

I'll hit approve, that empty if needs to be removed but feel free to merge otherwise.

dblock · 2021-06-16T14:16:30Z

We'll need a corresponding doc change I believe for 1.1, cc: @aetter, maybe point @VachaShah where to make it?

opensearch-ci-bot · 2021-06-16T19:09:59Z

✅ Gradle Wrapper Validation success 3f256cbc83359f6f46866102859ecfc775f33b27

opensearch-ci-bot · 2021-06-16T19:10:48Z

✅ DCO Check Passed 3f256cbc83359f6f46866102859ecfc775f33b27

opensearch-ci-bot · 2021-06-16T19:19:07Z

✅ Gradle Precommit success 3f256cbc83359f6f46866102859ecfc775f33b27

dblock · 2021-06-16T19:21:29Z

start gradle check

opensearch-ci-bot · 2021-06-16T19:36:56Z

❌ Gradle Check failure 3f256cbc83359f6f46866102859ecfc775f33b27
Log 242

Reports 242

dblock · 2021-06-16T19:38:54Z

@VachaShah not sure what's up with CI failures ^

VachaShah · 2021-06-16T20:09:12Z

@VachaShah not sure what's up with CI failures ^

I will look into these.

adnapibar · 2021-06-16T20:26:10Z

I don't think changing the behavior of how opensearch-plugin command works is a good idea. The list subcommand should list the name of the plugins and not the folder name. Similarly to remove a plugin, the remove subcommand should take the name of the plugin and not a folder name.

VachaShah · 2021-06-16T20:35:50Z

I don't think changing the behavior of how opensearch-plugin command works is a good idea. The list subcommand should list the name of the plugins and not the folder name. Similarly to remove a plugin, the remove subcommand should take the name of the plugin and not a folder name.

We are not changing the behavior of the list or remove commands. They still work the same way as before, I added the behavior in the description just to show the behavior. This is just to provide the user a way to customize the folder name during installation.

adnapibar · 2021-06-16T21:00:04Z

I don't think changing the behavior of how opensearch-plugin command works is a good idea. The list subcommand should list the name of the plugins and not the folder name. Similarly to remove a plugin, the remove subcommand should take the name of the plugin and not a folder name.

We are not changing the behavior of the list or remove commands. They still work the same way as before, I added the behavior in the description just to show the behavior. This is just to provide the user a way to customize the folder name during installation.

Got it - Just to confirm, if I install a plugin with a custom folder name, I can still remove it using the plugin name? And when I run 'opensearch-pugin list` it still lists the original plugin name?

VachaShah · 2021-06-16T21:18:13Z

I don't think changing the behavior of how opensearch-plugin command works is a good idea. The list subcommand should list the name of the plugins and not the folder name. Similarly to remove a plugin, the remove subcommand should take the name of the plugin and not a folder name.

We are not changing the behavior of the list or remove commands. They still work the same way as before, I added the behavior in the description just to show the behavior. This is just to provide the user a way to customize the folder name during installation.

Got it - Just to confirm, if I install a plugin with a custom folder name, I can still remove it using the plugin name? And when I run 'opensearch-pugin list` it still lists the original plugin name?

No the list command lists the folder name and the remove command uses the folder name to remove the plugin.

adnapibar · 2021-06-16T21:22:53Z

No the list command lists the folder name and the remove command uses the folder name to remove the plugin.

Okay, that's what I was referring to as a change in behavior. I think, we need to make this change in a way that keeps the old behavior as it is and doesn't break it.

adnapibar · 2021-06-16T23:23:12Z

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

@@ -116,6 +119,7 @@ public PluginInfo(final StreamInput in) throws IOException {
            javaVersion = "1.8";
        }
        this.classname = in.readString();
+        this.folderName = in.readString();


I think this needs to be scoped to a version e.g. if a version is after 1.0.0, then read it from the input stream otherwise use the default value for bwc.

Also, you need to add this field to the writeTo method for writing to the output stream .

I missed that in my CR, good call.

dblock · 2021-06-17T00:02:57Z

No the list command lists the folder name and the remove command uses the folder name to remove the plugin.

Okay, that's what I was referring to as a change in behavior. I think, we need to make this change in a way that keeps the old behavior as it is and doesn't break it.

So we think users today think of adding/removing as using the ID not the folder. They just happen to be the same and the code uses folder. Sounds like you should be able to specify either. Rabi is right.

VachaShah · 2021-06-17T21:42:37Z

Currently for this feature, the things that have been done are:

Allow the user to add a property to plugin-descriptor.properties for custom folder name.
The Install command checks if the target folder name exists which is id of the plugin if no custom folder name is specified. It allows the user to install the plugin with a custom folder name if no such folder exists. Also adding the individual check for both the name of the plugin and the folder name specified to see if the plugin exists either way.
Adding a scoping for the version for this new property.

Other cases that would be required to maintain the backwards compatibility as mentioned by @adnapibar:

The list command should be backwards compatible i.e. it should still list the name of the plugin even if the plugin is installed using custom folder name.
The remove command should be backwards compatible i.e. it should still take the name of the plugin as the input even though the plugin to be removed is installed in a custom folder. For this, we might have to scan each plugin folder in order to match the plugin name with the one being passed in the command to determine which plugin needs to be removed since we do not have the custom folder name information in the command.
There can be a case where the same plugin is installed with different folder names which will break OpenSearch, to prevent this, we would have to scan each plugin folder to match the name of the plugin being installed to prevent it from being installed multiple times.
There might also be some other manual use cases other than above which would have to be tested to make sure this feature works as expected.

@adnapibar @dblock Any thoughts?

dblock · 2021-06-18T11:54:03Z

Other cases that would be required to maintain the backwards compatibility as mentioned by @adnapibar:

Generally I think we need to fix everything that is broken by the change, so the entire list below.

The list command should be backwards compatible i.e. it should still list the name of the plugin even if the plugin is installed using custom folder name.

Maybe we can add showing the plugin ID, name and folder to avoid confusion? I am not personally familiar with how this command works.

The remove command should be backwards compatible i.e. it should still take the name of the plugin as the input even though the plugin to be removed is installed in a custom folder. For this, we might have to scan each plugin folder in order to match the plugin name with the one being passed in the command to determine which plugin needs to be removed since we do not have the custom folder name information in the command.

Agreed.

There can be a case where the same plugin is installed with different folder names which will break OpenSearch, to prevent this, we would have to scan each plugin folder to match the name of the plugin being installed to prevent it from being installed multiple times.

I think when removing a plugin scanning the list of all known folder names is a good idea.

adnapibar · 2021-07-20T23:28:32Z

start gradle check

opensearch-ci-bot · 2021-07-20T23:55:23Z

❌ Gradle Check failure e7daf15
Log 340

Reports 340

VachaShah · 2021-07-21T00:10:56Z

Gradle check failing with the following errors:


ERROR: for 4dbbd8ce867ac035f900dc1997079f33_azure-fixture__azure-fixture-other_1  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for 4dbbd8ce867ac035f900dc1997079f33_azure-fixture__azure-fixture_1  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for 4dbbd8ce867ac035f900dc1997079f33_azure-fixture__azure-fixture-repositories-metering_1  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for azure-fixture-other  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for azure-fixture  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)

ERROR: for azure-fixture-repositories-metering  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=60)
An HTTP request took too long to complete. Retry with --verbose to obtain debug information.
If you encounter this issue regularly because of slow network conditions, consider setting COMPOSE_HTTP_TIMEOUT to a higher value (current value: 60).
No such container: d1d0f77c05b7815db8f82a97b779ee5685ba4a1868aaabf589e939b1ee787b80

> Task :test:fixtures:azure-fixture:composeUp FAILED

> Task :test:fixtures:krb5kdc-fixture:composeDown

ERROR: for 1518ece52461a6dc45a5e1e577f9b5a2_krb5kdc-fixture__hdfs_1  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=70)

ERROR: for 1518ece52461a6dc45a5e1e577f9b5a2_krb5kdc-fixture__peppa_1  UnixHTTPConnectionPool(host='localhost', port=None): Read timed out. (read timeout=70)
An HTTP request took too long to complete. Retry with --verbose to obtain debug information.
If you encounter this issue regularly because of slow network conditions, consider setting COMPOSE_HTTP_TIMEOUT to a higher value (current value: 60).

> Task :test:fixtures:krb5kdc-fixture:composeDown FAILED
build complete, generating: /var/CITOOL/workflow/OpenSearch_CI/PR_Checks/Gradle_Check/search/build/340.tar.bz2

FAILURE: Build completed with 2 failures.

1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':test:fixtures:azure-fixture:composeUp'.
> Exit-code 1 when calling /usr/bin/docker-compose, stdout: N/A

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
==============================================================================

2: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':test:fixtures:krb5kdc-fixture:composeDown'.
> Exit-code 1 when calling /usr/bin/docker-compose, stdout:

adnapibar · 2021-07-21T00:33:49Z

start gradle check

opensearch-ci-bot · 2021-07-21T00:57:25Z

✅ Gradle Check success e7daf15
Log 341

Reports 341

nknize · 2021-08-20T14:15:58Z

@VachaShah looks like this is labeled v1.1.0 but was never backported to 1.x. Can you open a PR to backport before it's forgotten?

VachaShah · 2021-08-20T18:11:17Z

@VachaShah looks like this is labeled v1.1.0 but was never backported to 1.x. Can you open a PR to backport before it's forgotten?

Sure will do.

…ct#848) Signed-off-by: Vacha Shah <vachshah@amazon.com>

Signed-off-by: Vacha Shah <vachshah@amazon.com>

…ct#848) Signed-off-by: Vacha Shah <vachshah@amazon.com>

ylwu-amzn · 2021-08-31T17:26:44Z

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

     * @param extendedPlugins       other plugins this plugin extends through SPI
     * @param hasNativeController   whether or not the plugin has a native controller
     */
    public PluginInfo(String name, String description, String version, Version opensearchVersion, String javaVersion,
-                      String classname, List<String> extendedPlugins, boolean hasNativeController) {
+                      String classname, String customFolderName, List<String> extendedPlugins, boolean hasNativeController) {


Is it possible to keep the original constructor and add a new constructor to support customFolderName ? This change caused AD test case failed. Not a big problem, but we'd better make the change backward compatible.

Yes, we should have done that. Let's not break the interfaces like these :(

/cc: @VachaShah

@ylwu-amzn if you can PR this that'd be much appreciated, if not open an issue so we don't forget?

@dblock busy with AD plugin feature development. Will open an issue to track this.

Created an issue: #1190

lukas-vlcek · 2022-06-29T10:48:53Z

Hi,
Has this feature been documented? I was not able to find any mention of customFolderName in the official documentation. Maybe it is not meant to be wildly used/adopted by plugin authors?

dblock · 2022-07-05T16:19:48Z

I think we missed docs, I opened opensearch-project/documentation-website#756 (feel free to do the same for anything missing).

…ype (opensearch-project#848) * Fixing cluster stats response for role types and adding search role type Signed-off-by: Vacha Shah <vachshah@amazon.com> * Add CHANGELOG Signed-off-by: Vacha Shah <vachshah@amazon.com> --------- Signed-off-by: Vacha Shah <vachshah@amazon.com>

dblock requested changes Jun 15, 2021

View reviewed changes

VachaShah force-pushed the allow_custom_folder_name_for_plugin_installation branch from 838229d to fc944e6 Compare June 16, 2021 06:19

VachaShah requested a review from dblock June 16, 2021 06:32

dblock requested changes Jun 16, 2021

View reviewed changes

dblock approved these changes Jun 16, 2021

View reviewed changes

VachaShah force-pushed the allow_custom_folder_name_for_plugin_installation branch from fc944e6 to 3f256cb Compare June 16, 2021 19:08

dblock approved these changes Jun 16, 2021

View reviewed changes

adnapibar reviewed Jun 16, 2021

View reviewed changes

VachaShah requested a review from adnapibar July 20, 2021 22:58

adnapibar approved these changes Jul 20, 2021

View reviewed changes

abbashus merged commit 19e54d6 into opensearch-project:main Jul 21, 2021

VachaShah mentioned this pull request Jul 22, 2021

Add documentation for the custom folder name feature for plugins opensearch-project/opensearch-plugins#68

Closed

VachaShah deleted the allow_custom_folder_name_for_plugin_installation branch July 22, 2021 21:54

VachaShah added a commit to VachaShah/OpenSearch that referenced this pull request Aug 20, 2021

Allowing custom folder name for plugin installation (opensearch-proje…

05f3276

…ct#848) Signed-off-by: Vacha Shah <vachshah@amazon.com>

VachaShah mentioned this pull request Aug 20, 2021

[Backport-1.x] Allowing custom folder name for plugin installation (#848) #1116

Merged

5 tasks

VachaShah added the pending backport Identifies an issue or PR that still needs to be backported label Aug 20, 2021

CEHENKLE pushed a commit that referenced this pull request Aug 23, 2021

Allowing custom folder name for plugin installation (#848) (#1116)

f73e66e

Signed-off-by: Vacha Shah <vachshah@amazon.com>

VachaShah removed the pending backport Identifies an issue or PR that still needs to be backported label Aug 23, 2021

nknize pushed a commit to nknize/OpenSearch that referenced this pull request Aug 25, 2021

Allowing custom folder name for plugin installation (opensearch-proje…

497bf27

…ct#848) Signed-off-by: Vacha Shah <vachshah@amazon.com>

nknize mentioned this pull request Aug 25, 2021

[1.0] Allowing custom folder name for plugin installation (#848) #1155

Closed

ylwu-amzn reviewed Aug 31, 2021

View reviewed changes

ylwu-amzn mentioned this pull request Aug 31, 2021

Keep original constructor of PluginInfo while we support custom folder name #1190

Closed

VachaShah mentioned this pull request Sep 2, 2021

Kept the original constructor for PluginInfo to maintain bwc #1206

Merged

5 tasks

lukas-vlcek mentioned this pull request Jun 29, 2022

Installing the exporter on k8s env Aiven-Open/prometheus-exporter-plugin-for-opensearch#67

Open

dblock added the documentation pending Tracks issues which have PRs merged but documentation changes pending label Jul 5, 2022

dblock mentioned this pull request Jul 5, 2022

[FEATURE] Document custom folders opensearch-project/documentation-website#756

Closed

Allowing custom folder name for plugin installation #848

Allowing custom folder name for plugin installation #848

Conversation

VachaShah commented Jun 15, 2021 • edited Loading

Description

Testing

Issues Resolved

Check List

opensearch-ci-bot commented Jun 15, 2021

opensearch-ci-bot commented Jun 15, 2021

opensearch-ci-bot commented Jun 15, 2021

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 16, 2021

opensearch-ci-bot commented Jun 16, 2021

opensearch-ci-bot commented Jun 16, 2021

dblock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

dblock commented Jun 16, 2021 • edited Loading

opensearch-ci-bot commented Jun 16, 2021

opensearch-ci-bot commented Jun 16, 2021

opensearch-ci-bot commented Jun 16, 2021

dblock commented Jun 16, 2021

opensearch-ci-bot commented Jun 16, 2021

dblock commented Jun 16, 2021

VachaShah commented Jun 16, 2021

adnapibar commented Jun 16, 2021

VachaShah commented Jun 16, 2021

adnapibar commented Jun 16, 2021

VachaShah commented Jun 16, 2021

adnapibar commented Jun 16, 2021

adnapibar Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Jun 17, 2021

VachaShah commented Jun 17, 2021

dblock commented Jun 18, 2021

adnapibar commented Jul 20, 2021

opensearch-ci-bot commented Jul 20, 2021

VachaShah commented Jul 21, 2021

adnapibar commented Jul 21, 2021

opensearch-ci-bot commented Jul 21, 2021

nknize commented Aug 20, 2021

VachaShah commented Aug 20, 2021

Choose a reason for hiding this comment

dblock Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukas-vlcek commented Jun 29, 2022

dblock commented Jul 5, 2022

VachaShah commented Jun 15, 2021 •

edited

Loading

dblock Jun 16, 2021 •

edited

Loading

dblock left a comment •

edited

Loading

dblock commented Jun 16, 2021 •

edited

Loading

adnapibar Jun 16, 2021 •

edited

Loading

dblock Aug 31, 2021 •

edited

Loading