Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] OpenSearch moves plugin's bin, config into root directory #2963

Closed
sruti1312 opened this issue Apr 18, 2022 · 7 comments
Closed

[BUG] OpenSearch moves plugin's bin, config into root directory #2963

sruti1312 opened this issue Apr 18, 2022 · 7 comments
Labels
bug Something isn't working Plugins v2.0.0 Version 2.0.0

Comments

@sruti1312
Copy link
Contributor

Describe the bug
OpenSearch moves the plugins config and bin directories into root bin and config folder. This behaviour is similar with all plugins and the expected behaviour.

https:/opensearch-project/OpenSearch/blob/1.3.0/distribution/tools/plu[…]/src/main/java/org/opensearch/plugins/InstallPluginCommand.java

Recently all plugins including Performance Analyzer changed the directory names as a part of this issue. After the renaming, the config and bin folders are under the ROOT/config/PLUGIN_NAME/ directory. Opening this issue to start a discussion on whether it is better for plugin's config and bin folders to be retained under plugins folder over moving them under ROOT directory.

(22-04-18 18:13:04) <0> [~/opensearch-2.0.0-rc1]  
dev-dsk-partsrut-2b-18e5f650 % ll config 
-rw-r----- 1 partsrut amazon 2.4K Jan 1 1970 jvm.options
drwxr-x--- 2 partsrut amazon 4.0K Jan 1 1970 jvm.options.d
-rw-r----- 1 partsrut amazon 12K Jan 1 1970 log4j2.properties
drwxr-x--- 2 partsrut amazon 4.0K Apr 15 22:40 opensearch-notifications
drwxr-x--- 2 partsrut amazon 4.0K Apr 15 22:40 opensearch-notifications-core
drwxr-x--- 2 partsrut amazon 4.0K Apr 15 22:40 opensearch-observability
drwxr-x--- 2 partsrut amazon 4.0K Apr 15 22:40 opensearch-performance-analyzer
drwxr-x--- 2 partsrut amazon 4.0K Apr 15 22:40 opensearch-reports-scheduler
drwxr-x--- 2 partsrut amazon 4.0K Apr 15 22:40 opensearch-security
-rw-r----- 1 partsrut amazon 2.8K Jan 1 1970 opensearch.yml
(22-04-18 19:53:24) <0> [~/opensearch-2.0.0-rc1/bin]  
dev-dsk-partsrut-2b-18e5f650 % ll
-rwxr-xr-x 1 partsrut amazon 3.0K Jan  1  1970 opensearch
-rwxr-xr-x 1 partsrut amazon 1.1K Jan  1  1970 opensearch-cli
-rwxr-xr-x 1 partsrut amazon 5.2K Jan  1  1970 opensearch-env
-rwxr-xr-x 1 partsrut amazon 1.8K Jan  1  1970 opensearch-env-from-file
-rwxr-xr-x 1 partsrut amazon  202 Jan  1  1970 opensearch-keystore
-rwxr-xr-x 1 partsrut amazon  136 Jan  1  1970 opensearch-node
drwxr-xr-x 2 partsrut amazon 4.0K Apr 18 18:17 opensearch-performance-analyzer
-rwxr-xr-x 1 partsrut amazon  190 Jan  1  1970 opensearch-plugin
-rwxr-xr-x 1 partsrut amazon  128 Jan  1  1970 opensearch-shard
-rwxr-xr-x 1 partsrut amazon  192 Jan  1  1970 opensearch-upgrade
-rwxr-xr-x 1 partsrut amazon  544 Apr 15 22:40 performance-analyzer-agent-cli

To Reproduce
Steps to reproduce the behavior:
Sample bundle tarball:

wget https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/2.0.0-rc1/2509/linux/x64/tar/dist/opensearch/opensearch-2.0.0-rc1-linux-x64.tar.gz
bin/opensearch-plugin install https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/2.0.0-rc1/2509/linux/x64/tar/builds/opensearch/plugins/opensearch-performance-analyzer-2.0.0.0-rc1.zip

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

  • OS: [e.g. iOS]
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.

@sruti1312 sruti1312 added bug Something isn't working untriaged labels Apr 18, 2022
@peterzhuamazon
Copy link
Member

opensearch-reports-scheduler and opensearch-observability are having this behavior since the very early stage of opensearch, and others in the list above are new to it.

Plugin owner who is not familiar with this behavior please take a look and make sure your plugin still runs as expected.

Thanks.

@saratvemulapalli
Copy link
Member

saratvemulapalli commented Apr 18, 2022

Thanks @sruti1312 for writing this up and a pointer to the code.

From OpenSearch plugin install tool, it has explicit handling of bin and config directories.

 * A plugin may also contain an optional {@code bin} directory which contains scripts. The
 * scripts will be installed into a subdirectory of the opensearch bin directory, using
 * the name of the plugin, and the scripts will be marked executable.
 * <p>
 * A plugin may also contain an optional {@code config} directory which contains configuration
 * files specific to the plugin. The config files be installed into a subdirectory of the
 * opensearch config directory, using the name of the plugin. If any files to be installed
 * already exist, they will be skipped.

My initial thoughts when I looked at this problem is: all configuration consumed by plugins stay in the plugin root directory.

But looking deeper and reading code from InstallPluginCommand.java handles it explicitly as it validates the files and makes sure OpenSearch has right permissions setup for config and bin directories.

Also RemovePluginCommand.java handles removal of plugin config and bin directories cleanly.

Given this information, this doesn't feel like a bug and its more of a feature.

Path forward

I understand this impacts 2.0. I see couple of options:

  1. Let config and bin directories live in OpenSearch root for all plugins. (eg: opensearch/config/perf-analyzer/perf-analyzer.yml)
    • Needs work in PA to point to new paths.
    • This needs work in opensearch-build to take care of PA bin.
    • For all plugins which moved to new directory structure, they have to verify the location is picked up correctly.
      See comment from @peterzhuamazon
  2. Let every plugin have its own config and bin directories within their plugin folder. (eg: opensearch/plugins/perf-analyzer/config/perf-analyzer.yml
    • Needs work in OpenSearch InstallPluginCommand and exclude handling of these directories.
    • Plugins need to ensure the permissions for these directories are setup correctly.

My thoughts, I would go with 1. because OpenSearch does the heavy lifting of setting up the right permissions and gives a clean experience for customers rather than plugins handling them.
@sruti1312 @peterzhuamazon what do you guys think?

@saratvemulapalli saratvemulapalli added v2.0.0 Version 2.0.0 Plugins and removed untriaged labels Apr 18, 2022
@sruti1312
Copy link
Contributor Author

I also think option 1 is more neater.

@CEHENKLE
Copy link
Member

Yeah, I think 1 is right as well. Seems like 2 is giving us a lot of places for bugs to creep in.

If we're coalescing around 1, @sruti1312 , @peterzhuamazon what is the lift to get this ready to go in PA and build?

@sruti1312
Copy link
Contributor Author

@CEHENKLE I am working on opening PR's on PA and opensearch-build package to close on this by today.

@dblock
Copy link
Member

dblock commented Apr 19, 2022

Anything left to do here @sruti1312? Close?

@saratvemulapalli
Copy link
Member

Closing this issue as the PR is merged in opensearch-build and no action items in core.

@sruti1312 feel free to reopen this one if you see more problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Plugins v2.0.0 Version 2.0.0
Projects
None yet
Development

No branches or pull requests

5 participants