Fix handling of mandatory meta plugins #28710

jasontedor · 2018-02-17T00:31:22Z

This commit fixes an issue with setting plugin.mandatory to include a meta-plugin. The issue here is that the names that we collect are the underlying plugins, not the meta-plugin. We should not use the underlying plugins instead using the names of non-meta plugins and the names of meta-plugins. This commit addresses this. The strategy here is that when we look at the installed plugins on the filesystem, we keep track of which ones are meta-plugins and carry this information up to where check which plugins are installed against the mandatory plugins.

Relates #28022

This commit fixes an issue with setting plugin.mandatory to include a meta-plugin. The issue here is that the names that we collect are the underlying plugins, not the meta-plugin. We should not use the underlying plugins instead using the names of non-meta plugins and the names of meta-plugins. This commit addresses this. The strategy here is that when we look at the installed plugins on the filesystem, we keep track of which ones are meta-plugins and carry this information up to where check which plugins are installed against the mandatory plugins.

jimczi

LGTM, I left a minor comment and a question.

jimczi · 2018-02-19T09:48:42Z

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

-     * Extracts all installed plugin directories from the provided {@code rootPath} expanding meta plugins if needed.
+     * Represents a meta-plugin and the {@link Bundle}s corresponding to its constituents.
+     */
+    static class MetaBundle implements BundleCollection{


nit: missing space after BundleCollection

Thanks @jimczi I will correct this.

I pushed 368f5f7.

jimczi · 2018-02-19T09:50:48Z

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

+                            pluginsList.add(bundle.plugin);
+                        }
+                        seenBundles.addAll(bundles);
+                        pluginsNames.add(plugin.name());


This means that a plugin inside a meta plugin cannot be set as mandatory ? I don't know if this is a problem or not though.

You are correct. I think this is okay though, it’s limited to the user-facing plugins (what they can install and uninstall). Do you agree with that?

agreed, thanks for asking.

* master: Enable selecting adaptive selection stats Remove leftover mention of file-based scripts Fix threading issue on listener notification (elastic#28730) Revisit deletion policy after release the last snapshot (elastic#28627) Remove unused method Track deletes only in the tombstone map instead of maintaining as copy (elastic#27868) [Docs] Correct typo in README.textile (elastic#28716) Fix AdaptiveSelectionStats serialization bug (elastic#28718) TEST: Fix InternalEngine#testAcquireIndexCommit Add note on temporary directory for Windows service Added coming annotation and breaking changes link to release notes script Remove leftover PR link for previously disabled bwc tests Separate acquiring safe commit and last commit (elastic#28271) Fix BWC issue of the translog last modified age stats

This commit fixes an issue with setting plugin.mandatory to include a meta-plugin. The issue here is that the names that we collect are the underlying plugins, not the meta-plugin. We should not use the underlying plugins instead using the names of non-meta plugins and the names of meta-plugins. This commit addresses this. The strategy here is that when we look at the installed plugins on the filesystem, we keep track of which ones are meta-plugins and carry this information up to where check which plugins are installed against the mandatory plugins. Relates #28710

jasontedor · 2018-02-20T14:00:09Z

Thanks @jimczi.

This reverts commit 62c5723. Relates elastic/elasticsearch#28710

This commit fixes an issue with setting plugin.mandatory to include a meta-plugin. The issue here is that the names that we collect are the underlying plugins, not the meta-plugin. We should not use the underlying plugins instead using the names of non-meta plugins and the names of meta-plugins. This commit addresses this. The strategy here is that when we look at the installed plugins on the filesystem, we keep track of which ones are meta-plugins and carry this information up to where check which plugins are installed against the mandatory plugins. Relates elastic#28710

Commit 7a051f3 reverted the use of a more specific plugin name for the mandatory plugin check (e.g. specifying `mandatory.plugin=x-pack` instead of ``mandatory.plugin=x-pack-security`), however, this was more specific names are still required for benchmarking release versions <6.3. Selectively use a more specific name for the mandatory plugin check depending on the Elasticsearch version. Relates: 7a051f3 Relates: elastic/elasticsearch#28710

Commit 7a051f3 reverted the use of a more specific plugin name for the mandatory plugin check (e.g. specifying `mandatory.plugin=x-pack` instead of ``mandatory.plugin=x-pack-security`), however, more specific names are still required for benchmarking release versions <6.3. Selectively use a more specific name for the mandatory plugin check depending on the Elasticsearch version. Also add test cases for both scenarios. Relates: 7a051f3 Relates: elastic/elasticsearch#28710

Commit 7a051f3 reverted the use of a more specific plugin name for the mandatory plugin check (e.g. specifying `mandatory.plugin=x-pack` instead of `mandatory.plugin=x-pack-security`), however, more specific names are still required for benchmarking release versions <6.3. Selectively use a more specific name for the mandatory plugin check depending on the Elasticsearch version. Also add test cases for both scenarios. Relates: #446 Relates: 7a051f3 Relates: elastic/elasticsearch#28710

Relates elastic/elasticsearch#28710

jasontedor added 2 commits February 16, 2018 19:25

Fix naming

54c61e0

jasontedor added >bug review :Core/Infra/Plugins Plugin API and infrastructure v7.0.0 v6.3.0 labels Feb 17, 2018

jasontedor requested review from rjernst and jimczi February 17, 2018 00:31

jasontedor added 6 commits February 16, 2018 19:32

Preserve comment

3f9215e

Fix finality

bb22f28

Fix checkstyle

8fadee8

Change exception type

7524337

Fix typo in test

4ba7cd3

Remove extraneous newline

c819e2d

jimczi approved these changes Feb 19, 2018

View reviewed changes

jasontedor added 5 commits February 19, 2018 14:51

Fix whitespace

368f5f7

Fix loading security policy

8545d2a

Fix formatting

ed07d55

Add test for findPluginDirs

42008ff

jasontedor merged commit 94594f1 into elastic:master Feb 20, 2018

jasontedor deleted the mandatory-meta-plugins branch February 20, 2018 13:59

danielmitterdorfer added a commit to elastic/rally that referenced this pull request Feb 21, 2018

Revert "Use more specific name for mandatory plugin check"

7a051f3

This reverts commit 62c5723. Relates elastic/elasticsearch#28710

dliappis mentioned this pull request Mar 26, 2018

Use more specific name for mandatory plugin check selectively elastic/rally#446

Merged

danielmitterdorfer added a commit to elastic/rally-teams that referenced this pull request May 8, 2018

No meta-plugin check for 5.x

87d7a60

Relates elastic/elasticsearch#28710

colings86 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

Fix handling of mandatory meta plugins #28710

Fix handling of mandatory meta plugins #28710

jasontedor commented Feb 17, 2018

jimczi left a comment

jimczi Feb 19, 2018

jasontedor Feb 19, 2018

jasontedor Feb 19, 2018

jimczi Feb 19, 2018

jasontedor Feb 19, 2018

jimczi Feb 19, 2018

jasontedor commented Feb 20, 2018

Fix handling of mandatory meta plugins #28710

Fix handling of mandatory meta plugins #28710

Conversation

jasontedor commented Feb 17, 2018

jimczi left a comment

Choose a reason for hiding this comment

jimczi Feb 19, 2018

Choose a reason for hiding this comment

jasontedor Feb 19, 2018

Choose a reason for hiding this comment

jasontedor Feb 19, 2018

Choose a reason for hiding this comment

jimczi Feb 19, 2018

Choose a reason for hiding this comment

jasontedor Feb 19, 2018

Choose a reason for hiding this comment

jimczi Feb 19, 2018

Choose a reason for hiding this comment

jasontedor commented Feb 20, 2018