Add the ability to bundle multiple plugins into a meta plugin #28022

jimczi · 2017-12-29T08:49:38Z

This commit adds the ability to package multiple plugins in a single zip.
The zip file for a meta plugin must contains the following structure:

|____elasticsearch/
| |____   <plugin1> <-- The plugin files for plugin1 (the content of the elastisearch directory)
| |____   <plugin2>  <-- The plugin files for plugin2
| |____   meta-plugin-descriptor.properties <-- example contents below

The meta plugin properties descriptor is mandatory and must contain the following properties:

description: simple summary of the meta plugin.
name: the meta plugin name

The installation process installs each plugin in a sub-folder inside the meta plugin directory.
The example above would create the following structure in the plugins directory:

|_____ plugins
| |____   <name_of_the_meta_plugin>
| | |____   meta-plugin-descriptor.properties
| | |____   <plugin1>
| | |____   <plugin2>

If the sub plugins contain a config or a bin directory, they are copied in a sub folder inside the meta plugin config/bin directory.

|_____ config
| |____   <name_of_the_meta_plugin>
| | |____   <plugin1>
| | |____   <plugin2>

|_____ bin
| |____   <name_of_the_meta_plugin>
| | |____   <plugin1>
| | |____   <plugin2>

The sub-plugins are loaded at startup like normal plugins with the same restrictions; they have a separate class loader and a sub-plugin
cannot have the same name than another plugin (or a sub-plugin inside another meta plugin).

It is also not possible to remove a sub-plugin inside a meta plugin, only full removal of the meta plugin is allowed.

Closes #27316

This commit adds the ability to package multiple plugins in a single zip. The zip file for an uber plugin must contains the following structure: ``` |____elasticsearch/ | |____ <plugin1> <-- The plugin files for plugin1 (the content of the elastisearch directory) | |____ <plugin2> <-- The plugin files for plugin2 | |____ uber-plugin-descriptor.properties <-- example contents below ``` The uber-plugin properties descriptor is mandatory and must contain the following properties: * `description`: simple summary of the uber plugin. * `name`: the uber plugin name * `plugins`: a coma separated list of the sub-plugin names bundled in the uber plugin. The installation process installs each sub-plugin in a sub-folder inside the uber plugin directory. The example above would create the following structure in the plugins directory: ``` |_____ plugins | |____ <name_of_the_uber_plugin> | | |____ uber-plugin-descriptor.properties | | |____ <plugin1> | | |____ <plugin2> ``` If the sub plugins contain a config or a bin directory, they are copied in a sub folder inside the uber plugin config/bin directory. ``` |_____ config | |____ <name_of_the_uber_plugin> | | |____ uber-plugin-descriptor.properties | | |____ <plugin1> | | |____ <plugin2> |_____ bin | |____ <name_of_the_uber_plugin> | | |____ uber-plugin-descriptor.properties | | |____ <plugin1> | | |____ <plugin2> ``` The sub-plugins are loaded at startup like normal plugins with the same restrictions; they have a separate class loader and a sub-plugin cannot have the same name than another plugin (or a sub-plugin inside another uber plugin). It is also not possible to remove a sub-plugin inside an uber plugin, only full removal of the uber plugin is allowed. Closes elastic#27316

rjernst

I left some questions. The biggest request is to rename "uber" to "meta".

rjernst · 2018-01-05T19:31:32Z

buildSrc/src/main/resources/uber-plugin-descriptor.properties

@@ -0,0 +1,25 @@
+# Elasticsearch uber plugin descriptor file
+# This file must exist as 'uber-plugin-descriptor.properties' in a folder named `elasticsearch`.


I think better name than "uber" would be "meta" (as in meta physical, it isn't really a plugin)

rjernst · 2018-01-05T19:33:10Z

buildSrc/src/main/resources/uber-plugin-descriptor.properties

+# 'name': the uber plugin name
+name=${name}
+#
+# 'plugins': a coma separated list of the sub-plugin names bundled in the uber plugin


typo: coma -> comma

Also, I would not call these "sub" plugins (that has a connotation of subclassing), but instead "bundled" plugins?

But is this plugins list actually needed? The loading doesn't use it (and shouldn't, no leniency for extra junk at the root). I don't see it used anywhere but in tests.

Agreed, I removed it.

rjernst · 2018-01-05T19:44:10Z

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

    }

    /** check a candidate plugin for jar hell before installing it */
-    void jarHellCheck(Path candidate, Path pluginsDir) throws Exception {
+    void jarHellCheck(PluginInfo candidateInfo, Path candidate, Path pluginsDir) throws Exception {


nit: can you please rename candidate to candidateDir to be parallel with candidateInfo?

rjernst · 2018-01-05T19:48:53Z

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

+        // read existing bundles excluding the current candidate.
+        // this does some checks on the installation too.
+        Path excludePath = candidateInfo.getUberPlugin() != null ? candidate.getParent() : candidate;
+        PluginsService.getPluginBundles(pluginsDir, Collections.singleton(excludePath.getFileName().toString()));


I don't think this exclusion should be here once this is synced with SPI work. We need all the bundles in order to do a jarhell check.

rjernst · 2018-01-05T19:50:25Z

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

-                    terminal.println("Elasticsearch keystore is required by plugin [" + info.getName() + "], creating...");
-                    keystore = KeyStoreWrapper.create(new char[0]);
-                    keystore.save(env.configFile());
+            if (UberPluginInfo.isUberPlugin(tmpRoot)) {


Why does installation need anything meta plugin specific? It should just copy all the contents of the elasticsearch directory right, just like a normal plugin right?

It needs to know if we are inside a meta plugin in order to validate each plugin separately.

rjernst · 2018-01-05T19:54:25Z

...bution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java

+                "uber_plugin:fake_plugin1",
+                "- Plugin information:",
+                "Uber Plugin: uber_plugin",
+                "Name: fake_plugin1",


I think we should have better grouping to show these are part of the meta plugin in the output. Perhaps printing the plugin info should be indented a few spaces under a meta plugin?

rjernst · 2018-01-05T19:55:04Z

...bution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java

+                "Plugins directory: " + env.pluginsFile(),
+                "uber_plugin:fake_plugin1",
+                "- Plugin information:",
+                "Uber Plugin: uber_plugin",


It seems silly to print out the meta plugin this is part of, given it is already grouped under the the meta plugin.

rjernst · 2018-01-05T19:55:47Z

core/src/main/java/org/elasticsearch/plugins/PluginInfo.java

                      boolean hasNativeController, boolean requiresKeystore) {
+        this.uberPlugin = uberPlugin;


What is this link back to the meta plugin needed for?

To keep track of the meta plugin name when the node loads the plugin but it's not needed. I removed it from the pr.

jimczi · 2018-01-07T22:57:33Z

Thanks @rjernst . I merged with master and pushed some changes to address your comments. Can you take another look ?

rjernst

Thanks @jimczi, I left some more comments.

rjernst · 2018-01-08T18:26:01Z

buildSrc/src/main/resources/meta-plugin-descriptor.properties

+#
+# meta-foo.zip <-- zip file for the meta plugin, with this structure:
+#|____elasticsearch/
+#| |____   <sub_plugin_1> <-- The plugin files for sub_plugin_1 (the content of the elastisearch directory)


sub -> bundled (as suggested in a previous comment)

rjernst · 2018-01-08T18:31:53Z

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

@@ -87,7 +89,7 @@
 * </ul>
 *
 * Plugins are packaged as zip files. Each packaged plugin must contain a
- * plugin properties file. See {@link PluginInfo}.
+ * plugin properties file See {@link PluginInfo} or a meta plugin properties file See {@link MetaPluginInfo}.


this doesn't really make sense grammar-wise. I suggest:

plugin properties file or a meta plugin properties file. See {@link PluginInfo} and {@link MetaPluginInfo}, respectively.

rjernst · 2018-01-08T18:37:20Z

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

+                IOUtils.rm(tmpRoot);
+                terminal.println("-> Installed " + metaInfo.getName());
+            } else {
+                installPlugin(terminal, isBatch, tmpRoot, env.pluginsFile(), env.binFile(),


If you're going to have normal plugin installation separated into another method, then I would put the code above for meta plugin installation into its own method as well.

rjernst · 2018-01-08T18:38:06Z

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

                }
+                // clean up installation
+                IOUtils.rm(tmpRoot);


SHouldn't this cleanup and printing be down outside of the if/else since it should apply to both types of plugins?

The cleanup is not needed for normal plugin since they are just "moved" from the tmp directory into their final destination. For meta plugin we explicitly create the meta directory in the plugins dir so we need to cleanup the temporary directory at the end (the plugins inside the meta directory are moved to their final destination but the root directory remains untouched).

Hrm, I did wonder about that. Why is it we can't just move the meta plugin like a normal plugin?

I refactored the code to move the meta plugin like a normal plugin:
e3083ba

rjernst · 2018-01-08T18:41:56Z

docs/plugins/authors.asciidoc

+
+It is also possible to bundle multiple plugins into a meta plugin.
+A directory for each sub-plugin must be contained in a directory called `elasticsearch.
+The meta plugin must also contain a file called `meta-plugin-descriptor.properties` in the folder named


folder -> directory (use *nix terminology, not mixing with windows terminology).

rjernst · 2018-01-08T18:43:25Z

docs/plugins/authors.asciidoc

+The meta plugin must also contain a file called `meta-plugin-descriptor.properties` in the folder named
+`elasticsearch`. The format for this file is described in detail here:
+
+https://github.com/elastic/elasticsearch/blob/master/buildSrc/src/main/resources/plugin-descriptor.properties[`/buildSrc/src/main/resources/meta-plugin-descriptor.properties`].


I'm hesitant to add this link. It links to master, but this will be incorrect (as it could be different) once we backport to 6.x.

Why would it be incorrect after the backport ? We do the same for the plugin-descriptor.properties of a normal plugin.

It must be changed on the backport (and every time we branch) to be correct. For example, if we add any properties to the file in master, but the docs for 6.2 reference the master file, it would cause an error when loading (unknown property). I didn't know we do the same for the regular plugin descriptor...that is unfortunate. This is broken IMO.

I pushed 5043500 to print the properties file in the docs instead of linking to a specific file. WDYT ?

This is much better!

rjernst · 2018-01-08T18:44:14Z

plugins/examples/meta-plugin/build.gradle

+
+// A meta plugin packaging example that bundles two plugins in a single zip.
+
+apply plugin: 'elasticsearch.standalone-rest-test'


As a followup, I think we should have a gradle plugin for a metaplugin.

rjernst · 2018-01-08T18:46:34Z

plugins/examples/meta-plugin/build.gradle

+// unzip the rescore plugin in a tmp directory
+task unzipRescorePlugins(type:Copy, dependsOn:':example-plugins:rescore:bundlePlugin') {
+    File pluginsRescore = new File(plugins, 'example-rescore')
+    from { zipTree(project(':example-plugins:rescore').bundlePlugin.outputs.files.singleFile) }


Can you create 2 dummy plugins inside this example (as sub projects within the source here) instead of trying to use other existing examples? What is here now will cause the examples to not be installable at the same time, and we don't need real functionality out of the bundled plugins, just an example that it is possible and they are loaded.

jimczi · 2018-01-09T00:02:07Z

Thanks @rjernst . I left some questions and pushed another commit to address your comments.
I think it's ready for another review.

…le file in master

rjernst

Thanks Jim! LGTM.

* master: Set watermarks in single-node test cases Add the ability to bundle multiple plugins into a meta plugin (elastic#28022) Declare empty package dirs as output dirs Consistent updates of IndexShardSnapshotStatus (elastic#28130)

This commit adds the ability to package multiple plugins in a single zip. The zip file for a meta plugin must contains the following structure: |____elasticsearch/ | |____ <plugin1> <-- The plugin files for plugin1 (the content of the elastisearch directory) | |____ <plugin2> <-- The plugin files for plugin2 | |____ meta-plugin-descriptor.properties <-- example contents below The meta plugin properties descriptor is mandatory and must contain the following properties: description: simple summary of the meta plugin. name: the meta plugin name The installation process installs each plugin in a sub-folder inside the meta plugin directory. The example above would create the following structure in the plugins directory: |_____ plugins | |____ <name_of_the_meta_plugin> | | |____ meta-plugin-descriptor.properties | | |____ <plugin1> | | |____ <plugin2> If the sub plugins contain a config or a bin directory, they are copied in a sub folder inside the meta plugin config/bin directory. |_____ config | |____ <name_of_the_meta_plugin> | | |____ <plugin1> | | |____ <plugin2> |_____ bin | |____ <name_of_the_meta_plugin> | | |____ <plugin1> | | |____ <plugin2> The sub-plugins are loaded at startup like normal plugins with the same restrictions; they have a separate class loader and a sub-plugin cannot have the same name than another plugin (or a sub-plugin inside another meta plugin). It is also not possible to remove a sub-plugin inside a meta plugin, only full removal of the meta plugin is allowed. Closes #27316

jimczi added :Core/Infra/Plugins Plugin API and infrastructure >feature v6.2.0 v7.0.0 labels Dec 29, 2017

jimczi requested a review from rjernst December 29, 2017 08:49

jimczi added 3 commits December 29, 2017 11:37

fix tests

dfe70de

fix test on windows

c8728c3

make sure that the sub-plugins are always listed in the same orde

bcca2b2

rjernst reviewed Jan 5, 2018

View reviewed changes

jimczi added 2 commits January 7, 2018 18:40

rename uber => meta

8ebccab

Merge branch 'master' into feature/uber_plugin

15a6ae4

jimczi changed the title ~~Add the ability to bundle multiple plugins into an uber plugin~~ Add the ability to bundle multiple plugins into a meta plugin Jan 7, 2018

jimczi added 2 commits January 7, 2018 23:27

typo

2565998

fix qa tests

067e271

jimczi added 3 commits January 8, 2018 01:05

restore master assertions

aabfc06

fix another test

98bb465

update error message

33f4aee

rjernst reviewed Jan 8, 2018

View reviewed changes

jimczi added 2 commits January 9, 2018 00:48

generate two dummy plugins in the meta plugin example

25546f2

address reviews

07f9cc9

jimczi added 4 commits January 9, 2018 10:06

meta plugin should be moved into its final destination

e3083ba

validate all bundled plugin names before installing a meta plugin

84604d8

remove debug leftover

43c38e0

include the properties files examples instead of linking to the examp…

5043500

…le file in master

rjernst approved these changes Jan 9, 2018

View reviewed changes

jimczi merged commit 36729d1 into elastic:master Jan 9, 2018

jimczi deleted the feature/uber_plugin branch January 9, 2018 17:28

tylerjl mentioned this pull request Feb 13, 2018

Refactor exists logic for plugin provider voxpupuli/puppet-elasticsearch#920

Merged

5 tasks

jasontedor mentioned this pull request Feb 17, 2018

Fix handling of mandatory meta plugins #28710

Merged

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to bundle multiple plugins into a meta plugin #28022

Add the ability to bundle multiple plugins into a meta plugin #28022

jimczi commented Dec 29, 2017 •

edited

Loading

rjernst left a comment

rjernst Jan 5, 2018

rjernst Jan 5, 2018

rjernst Jan 5, 2018

jimczi Jan 7, 2018

rjernst Jan 5, 2018

rjernst Jan 5, 2018

rjernst Jan 5, 2018

jimczi Jan 7, 2018

rjernst Jan 5, 2018

rjernst Jan 5, 2018

rjernst Jan 5, 2018

jimczi Jan 7, 2018

jimczi commented Jan 7, 2018

rjernst left a comment

rjernst Jan 8, 2018

rjernst Jan 8, 2018

rjernst Jan 8, 2018

rjernst Jan 8, 2018

jimczi Jan 8, 2018

rjernst Jan 9, 2018

jimczi Jan 9, 2018

rjernst Jan 8, 2018

rjernst Jan 8, 2018

jimczi Jan 8, 2018

rjernst Jan 8, 2018

jimczi Jan 9, 2018

rjernst Jan 9, 2018

rjernst Jan 8, 2018

rjernst Jan 8, 2018

jimczi commented Jan 9, 2018

rjernst left a comment

		@@ -0,0 +1,25 @@
		# Elasticsearch uber plugin descriptor file
		# This file must exist as 'uber-plugin-descriptor.properties' in a folder named `elasticsearch`.

		boolean hasNativeController, boolean requiresKeystore) {
		this.uberPlugin = uberPlugin;


		// A meta plugin packaging example that bundles two plugins in a single zip.

		apply plugin: 'elasticsearch.standalone-rest-test'

Add the ability to bundle multiple plugins into a meta plugin #28022

Add the ability to bundle multiple plugins into a meta plugin #28022

Conversation

jimczi commented Dec 29, 2017 • edited Loading

rjernst 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

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

Choose a reason for hiding this comment

jimczi commented Jan 7, 2018

rjernst 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

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

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

jimczi commented Jan 9, 2018

rjernst left a comment

Choose a reason for hiding this comment

jimczi commented Dec 29, 2017 •

edited

Loading