SOLR-17096: Cluster Singleton plugin support in solr.xml #2126

pjmcarthur · 2023-12-07T21:26:21Z

https://issues.apache.org/jira/browse/SOLR-17096

Description

It should be possible to install and manage the configuration of a cluster singleton plugin in an immutable deployment of SolrCloud. Currently, the only method to manage cluster singleton plugin configs is by using the ContainerPluginApi edit APIs, and this is not always possible, especially in the case of immutable SolrCloud deployments.

Solution

This PR changes the ContainerPluginRegistry to use a pluggable source for plugin configs. The pluggable source is determined by a system property, solr.clusterPluginsSource, or by a declaration in solr.xml.

The default source is ZkContainerPluginsSource, which allows plugins to be added, removed or updated at runtime using the ContainerPluginApi. This matches the existing behavior.

A new OOTB plugin implementation is NodeConfigClusterPluginsSource. This plugin supports managing cluster singleton plugins via declarations in solr.xml, and does not support the edit APIs of ContainerPluginApi; these APIs are not registered in the case that node config is used as the plugin source.

An example of solr.xml that supports cluster singleton declarations would be

<clusterPluginsSource class="org.apache.solr.api.NodeConfigClusterPluginsSource"/>
<clusterSingleton name="my.plugin.name" class="my.plugin.class">
  <config>...</config>
</clusterSingleton>

Tests

New tests are added in NodeConfigContainerPluginsSourceTest that spin up clusters and Core containers using the node config plugin source, and verify that container plugin cluster singleton declarations in solr.xml are added correctly to the ContainerPluginsRegistry.

Node configuration tests have been added to TestSolrXml

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

dsmiley

Remember to update TestSolrXml as well.

Overall, I like this!
I think there is a big issue with naming. In JIRA, I advocated for using "Node" nomenclature instead of "Container", at least for anything the user sees (users don't know about CoreContainer; that's an internal thing and a name we regret). But we can punt on renaming until the very end of this PR with respect to class names, to ease review.

solr/core/src/java/org/apache/solr/api/ContainerPluginsSource.java

solr/core/src/java/org/apache/solr/api/ZkContainerPluginsSource.java

solr/core/src/java/org/apache/solr/core/CoreContainer.java

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

solr/core/src/test/org/apache/solr/api/NodeConfigContainerPluginsSourceTest.java

dsmiley · 2023-12-09T01:09:02Z

Oh I totally missed an important concern that I raised with @pvcnt 's previous PR #2016. The configuration is very generic (the specific plugins are effectively untyped in solr.xml). I took this back to the parent JIRA with my comment here: https://issues.apache.org/jira/browse/SOLR-15782?focusedCommentId=17782415&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17782415

solr/core/src/test-files/solr/solr-50-all.xml

solr/core/src/java/org/apache/solr/core/NodeConfig.java

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

solr/core/src/test/org/apache/solr/api/NodeConfigClusterPluginsSourceTest.java

dsmiley

Nice test, by the way. I'm really glad you tested configurability. These plugins are configured internally very unusually compared to the rest of Solr.

@pvcnt could you also take a look at this too? You feared this approach might be too much work, I recall.

@noblepaul this is the start of a move to support these plugins in solr.xml

There is a decision to make on naming... "ClusterPlugins" vs "ContainerPlugins" vs "NodePlugins". And where to document such plugins when they can be configured in two places. I tend to think solr.xml documentation should be primary and only minimally document in the cluster plugin API (where these are currently documented).

dsmiley · 2023-12-22T19:53:54Z

solr/core/src/java/org/apache/solr/api/ClusterPluginsSourceConfigurator.java

+      CoreContainer cc, SolrResourceLoader loader, PluginInfo info) {
+    ClusterPluginsSource clusterPluginsSource = newInstance(loader, resolveClassName(info), cc);
+    if (info != null && info.isEnabled() && info.initArgs != null) {
+      Map<String, Object> config = new HashMap<>();


nit: these two lines can be one; toMap returns its arg.

pvcnt · 2023-12-24T16:56:22Z

@dsmiley I’ll have a more detailed look after my holidays. But one thing we should consider is that this PR follows a different design than what I did for replica placement plugins. For the latter, what goes inside solr.xml is applied as a first layer, and then cluster plugins as overrides. Here it’s one or the other, if my understanding is correct.

I believe this PR is heading towards a promising direction, but we should make sure that cluster singletons and replica placement plugins are at least configured the same.

pjmcarthur · 2024-01-03T18:14:50Z

@pvcnt @dsmiley I think there should be a follow-up PR that addresses the replica placement plugin. My understanding is that from a user's perspective, the configuration of solr.xml wrt. the placement plugin would remain as it is, but the internal mechanics of instantiating the RPP factory would change to align with this PR. I would be able to commit to doing this follow-up PR if it is deemed the right path forward.

pvcnt · 2024-01-03T18:44:31Z

I don't know when Solr 9.5 is going to be released, but IMHO we should make sure that it is not released with two different ways of configuring a cluster plugin in solr.xml, or even not released with a new way of configuring a cluster plugin in solr.xml that would be changing in Solr 9.6 (hence breaking backwards compatibility, etc.). I would hence be in favour of either doing both in a single PR, or starting by refactoring replica placement plugins. It would be a good exercice to validate that the approach can be extended to other cluster plugins.

The more philosophical question I have is about the behaviour. Do we want to be able to choose between solr.xml and plugin API (this PR) or to layer the latter on top of the former (my previous PR). I don't have a strong opinion on this, but that is a structuring question I believe.

dsmiley · 2024-01-04T00:09:15Z

Agreed that 9.5 should not be released without reconciling the two approaches.

I think I would prefer to revert SOLR-17079 as it exists today, then merge this PR SOLR-17096 (after whatever changes in the review process), then do a quick (?) follow-up PR for SOLR-17079 to return the essential functionality building on what's here. Do you think that hypothetical PR would be straight-forward? You raised concerns a while back that what Paul is doing here would maybe be too much work, if I recall.

I don't think it's important/necessary to layer them as you did for replica placement. That adds greater support requirements (testing that both can be used coherently) and nobody has asked for that.

pvcnt · 2024-01-04T11:02:00Z

I believe it would still be interesting to have at least two different plugin types to validate that the generic approach is really generic, and what it takes to add a new type of plugin.

Another remark, it seems that there is no validation that a <clusterSingleton/> tag does indeed defines a cluster singleton. I believe the following would work and define a replica placement plugin:

<clusterSingleton name=".placement-plugin" class="org.apache.solr.cluster.placement.plugins.AffinityPlacementFactory"/>

This is why in SOLR-17079 we decided to go with an ad-hoc solution (i.e., one for every possible cluster plugin) instead of the generic solution. Otherwise, we are back to my initial proposal which is to have an untype <plugin/> tag in solr.xml, which is essentially what this PR does.

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

dsmiley · 2024-01-04T15:30:03Z

I believe it would still be interesting to have at least two different plugin types to validate that the generic approach is really generic, and what it takes to add a new type of plugin.

I don't think it'll be totally generic; a given plugin type might best have some adjustments to create the PluginInfo. For example, the ".placement-plugin" name should be added to the PluginInfo when the XML element is <placementPlugin>. While that could be done in SolrXmlConfig (reading solr.xml), such an idiosyncrasy feels specific to the nature of the cluster edit API/JSON that maybe could be in another spot (unsure).

it seems that there is no validation that a tag does indeed defines a cluster singleton.

I don't recall ever making a mistake of that nature, probably also because the class name is nearly always indicative of the type, so it'd look wrong if it was wrong as well. Validation would be nice but not a requirement. I leave the choice to validate or not up to Paul.

pvcnt · 2024-01-04T16:50:26Z

The point I am raising about validation has actually already been raised by Paul there: https://github.com/apache/solr/pull/2126/files#r1424690508

I think it would be a mistake to not enforce that a <clusterSingleton/> actually defines a cluster singleton and not a replica placement plugin. Otherwise, what is the point of having a typed tag instead of a generic tag for all plugins, besides providing some documentation? Again, with the current implementation, you can perfectly declare a replica placement plugin (or any kind of cluster plugin really) inside a <clusterSingleton/> and it will just work.

pjmcarthur · 2024-01-05T21:30:35Z

I will add the validation.

Regarding piggybacking on the existing disable.configEdit property, I like the idea, but I have one concern.
If there are any Solr deployments out there that set disable.configEdit to true and also have cluster plugins installed, they will have issues after this change. The property value of true would cause the node config plugin source to be used instead of the ZK plugin source, and any plugins that were configured would not be loaded.

Maybe this isn't a real problem, and its unlikely that anyone would configure Solr in this way. You mention the property is not documented, so that could benefit us. I'm not sure how confident we can be about how safe it really is though. I think for that reason I slightly prefer the implicit selection of the pluigin source based on the presence of clusterSingleton declarations in solr.xml, but I'm ok to use the disable.configEdit property if we think its safe.

dsmiley · 2024-01-07T23:05:50Z

Let's imagine someone out there has set disable.configEdit and is also posting to /api/cluster/plugin. After the proposed change here, a call to that API is going to get an error (or should) telling them it's disabled. So they know, and can act. They aren't stuck; they have options. Like don't set disable.configEdit (easiest) or don't POST to /api/cluster/plugin (use solr.xml instead).

…Edit property

solr/core/src/java/org/apache/solr/core/NodeConfig.java

dsmiley · 2024-01-09T03:17:39Z

solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java

@@ -104,9 +105,6 @@
 public class SolrConfigHandler extends RequestHandlerBase
    implements SolrCoreAware, PermissionNameProvider {
  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
-  public static final String CONFIGSET_EDITING_DISABLED_ARG = "disable.configEdit";


@janhoy if you didn't notice, we're elevating this very internal undocumented sys prop to node level to increase the scope beyond config editing of a core live to also include disabling cluster config edit. Obviously the name of this sys prop is poor and your new efforts to standardize suggest we should take the opportunity to have a nicer name?

Sorry for the back & forth here... I had thought let's use disable.configEdit but I don't want to shine a light (document) a badly named config, especially at a time when a config naming standardization effort is underway.

Absent further feedback, I think we should use a new property, like solr.cluster.plugin.mutable using EnvUtils. It's scoped to just the /cluster/plugin API and thus doesn't have a debatable scope (too broad, like all config editing, which should get more agreement/care). Such a narrow scope also suggests moving the setting to ContainerPluginsRegistry or ZkClusterPluginsSource.

dsmiley · 2024-01-11T01:27:46Z

solr/core/src/java/org/apache/solr/api/NodeConfigClusterPluginsSource.java

      Arrays.stream(clusterSingletons)
          .forEach(


Streams can be cool when their power can be leveraged, but if you're simply going to iterate, than really, for(p : clusterSingletons) is best

dsmiley · 2024-01-11T01:31:58Z

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

@@ -159,16 +160,15 @@ public static NodeConfig fromConfig(
    configBuilder.setSolrResourceLoader(loader);
    configBuilder.setUpdateShardHandlerConfig(updateConfig);
    configBuilder.setShardHandlerFactoryConfig(getPluginInfo(root.get("shardHandlerFactory")));
-    configBuilder.setReplicaPlacementFactoryConfig(
-        getPluginInfo(root.get("replicaPlacementFactory")));
+    // configBuilder.setReplicaPlacementFactoryConfig(


a "nocommit" I suppose. BTW that's a magic keyword used by the Lucene & Solr projects that will show up in WIP that is not intended to be committed/merged.

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

solr/CHANGES.txt

solr/core/src/java/org/apache/solr/api/ClusterPluginsSource.java

dsmiley · 2024-01-11T02:00:28Z

solr/core/src/java/org/apache/solr/api/ClusterPluginsSourceConfigurator.java

+ * implementation is {@link ZkClusterPluginsSource}, but can be overridden by a system property, or
+ * by declaring the implementation class in solr.xml
+ */
+public class ClusterPluginsSourceConfigurator implements NamedListInitializedPlugin {


Extending NamedListInitializedPlugin means we have an init() method. But what becomes of those args?

dsmiley · 2024-01-11T02:48:06Z

solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java

@@ -104,9 +105,6 @@
 public class SolrConfigHandler extends RequestHandlerBase
    implements SolrCoreAware, PermissionNameProvider {
  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
-  public static final String CONFIGSET_EDITING_DISABLED_ARG = "disable.configEdit";


Sorry for the back & forth here... I had thought let's use disable.configEdit but I don't want to shine a light (document) a badly named config, especially at a time when a config naming standardization effort is underway.

Absent further feedback, I think we should use a new property, like solr.cluster.plugin.mutable using EnvUtils. It's scoped to just the /cluster/plugin API and thus doesn't have a debatable scope (too broad, like all config editing, which should get more agreement/care). Such a narrow scope also suggests moving the setting to ContainerPluginsRegistry or ZkClusterPluginsSource.

# Conflicts: # solr/CHANGES.txt

pjmcarthur · 2024-01-11T20:23:54Z

@pvcnt FYI - This PR has been updated to handle the replicaPlacementFactory declared in solr.xml in the same way as cluster singleton plugins.

pvcnt

I like the approach, and feel much more comfortable now that replica placement plugins have been integrated.

pvcnt · 2024-01-12T12:42:50Z

solr/core/src/java/org/apache/solr/core/NodeConfig.java

@@ -211,6 +215,17 @@ private NodeConfig(
    if (null == this.solrHome) throw new NullPointerException("solrHome");
    if (null == this.loader) throw new NullPointerException("loader");

+    if (this.clusterPlugins != null
+        && this.clusterPlugins.length > 0
+        && !ClusterPluginsSourceConfigurator.resolveClassName()


Can this be done later in CoreContainer, and rely on a method on the ClusterPluginsSource interface rather than a comparison of the class name?

pvcnt · 2024-01-12T12:44:07Z

solr/core/src/java/org/apache/solr/core/NodeConfig.java

+        && this.clusterPlugins.length > 0
+        && !ClusterPluginsSourceConfigurator.resolveClassName()
+            .equals(NodeConfigClusterPluginsSource.class.getName())) {
+      throw new SolrException(


Maybe it can be a warning instead of an exception? Not entirely sure, but maybe people will have those in solr.xml, change the source later and want to keep them. Or maybe they could be injected into the cluster API as defaults at some point.

I don't think we'd be doing users any favors by allowing/encouraging ambiguity in configs, and ignoring some of it based on settings. IMO the exception is correct here.

pvcnt · 2024-01-12T12:48:37Z

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

+            .map(n -> new PluginInfo(n, n.name(), true, true))
+            .collect(Collectors.toList());
+
+    // Cluster plugin names must be unique


Aren't plugin names supposed to be unique globally, instead of per plugin type? If so, this should be moved outside of this method specific to ClusterSingleton. We should also verify for reserved names, i.e., that a name does not start with a dot.

Indeed, it seems to be that Container/Cluster plugins have names in one namespace, but that's weird -- Solr doesn't work that way anywhere else. I'd rather fix that limitation in a follow-up -- ContainerPluginsRegistry ought to have a a map keyed by type.

pvcnt · 2024-01-12T12:53:41Z

solr/core/src/java/org/apache/solr/api/NodeConfigClusterPluginsSource.java

+      }
+    }
+
+    return pluginInfo.name;


This can return null, and likely generate an NPE later on (?).

It should not be possible to return null here. The name attribute for cluster plugins plugin is mandatory and validated in SolrXmlConfig.
There are some exceptions for built in plugins, like the replica placement plugin, for which it isn't necessary to specify a name in solr.xml because the name is implied. For those cases only, the name is set here.

pvcnt · 2024-01-12T12:54:29Z

solr/core/src/test/org/apache/solr/cluster/placement/impl/PlacementPluginIntegrationTest.java

@@ -124,25 +124,6 @@ public void testConfigurationInSystemProps() {
    cc.shutdown();
  }

-  @Test
-  public void testConfigurationInSolrXml() {


Has this test be moved elsewhere? It is the only end-to-end (well, kindof) verification that the placement plugin configured in solr.xml is indeed taking into account in CoreContainer.

Yes, see NodeConfigPlacementPluginTest. The test requires a ZK aware SolrCloud now because it uses the ContainerPluginRegistry.

dsmiley · 2024-01-15T18:53:56Z

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

-    // configBuilder.setReplicaPlacementFactoryConfig(
-    //    getPluginInfo(root.get("replicaPlacementFactory")));


to be removed?

dsmiley · 2024-01-15T18:57:51Z

solr/core/src/java/org/apache/solr/api/ClusterPluginsSourceConfigurator.java

+ * implementation is {@link ZkClusterPluginsSource}, but can be overridden by teh {@link
+ * ContainerPluginsRegistry#MUTABLE_CLUSTER_PLUGINS} property
+ */
+public class ClusterPluginsSourceConfigurator {


This is just a couple static methods related to ClusterPluginsSource. Shouldn't these be on ClusterPluginsSource instead of having this class to hold them?

dsmiley · 2024-01-15T19:24:40Z

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java

+            .map(n -> new PluginInfo(n, n.name(), true, true))
+            .collect(Collectors.toList());
+
+    // Cluster plugin names must be unique


Indeed, it seems to be that Container/Cluster plugins have names in one namespace, but that's weird -- Solr doesn't work that way anywhere else. I'd rather fix that limitation in a follow-up -- ContainerPluginsRegistry ought to have a a map keyed by type.

dsmiley · 2024-01-16T02:12:25Z

solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java

@@ -75,6 +73,8 @@
 public class ContainerPluginsRegistry implements ClusterPropertiesListener, MapWriter, Closeable {
  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

+  public static final String MUTABLE_CLUSTER_PLUGINS = "solr.cluster.plugin.mutable";


Fresh information: https://cwiki.apache.org/confluence/display/SOLR/System+property+naming+structure
Based on this audit and proposals (not yet formalized, granted), I think "solr.cluster.plugin.edit.enabled" would be the right property name here... ehh @janhoy ?

Yea, one can argue how far the naming standard should go. I was mostly concerned with the lowercase dot separated aspect, with a controlled set of top-level categories. Guess it is nice-to-have to also agree on "enabled/disabled", "edit/mutable", "allowlist/includelist" etc

dsmiley · 2024-01-17T17:39:50Z

There seems to be a test failure. Once that's worked out, I plan to merge!

dsmiley · 2024-01-17T23:12:31Z

The failure is unrelated; see #2204 for a fix.

* Generalize to all "cluster plugins", albeit only supporting <clusterSingleton> and <replicaPlacementFactory> at this time. * Introduced new `solr.cluster.plugin.edit.enabled` boolean setting to disable /cluster/plugin mutability, required for solr.xml's use of those plugins. Co-authored-by: Paul McArthur <[email protected]>

pmcarthur-apache added 2 commits December 7, 2023 12:56

SOLR-17096: Cluster Singletons for immutable deployments

0ca1813

Generalize to all container plugins

c1a61f6

janhoy requested review from janhoy, dsmiley and HoustonPutman December 8, 2023 00:40

dsmiley reviewed Dec 9, 2023

View reviewed changes

use 'clusterSingletons' in solr.xml, some renaming, additional tests

80c6d09

pjmcarthur commented Dec 12, 2023

View reviewed changes

solr/core/src/test-files/solr/solr-50-all.xml Outdated Show resolved Hide resolved

dsmiley reviewed Dec 13, 2023

View reviewed changes

Update reference guide & minor improvements

4ffcf68

dsmiley reviewed Dec 24, 2023

View reviewed changes

dsmiley reviewed Jan 4, 2024

View reviewed changes

solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java Outdated Show resolved Hide resolved

Add validation & determine cluster plugin source using disable.config…

90f6c99

…Edit property

dsmiley reviewed Jan 9, 2024

View reviewed changes

Incorporate placement plugin

993619e

github-actions bot added the documentation Improvements or additions to documentation label Jan 10, 2024

dsmiley reviewed Jan 11, 2024

View reviewed changes

pmcarthur-apache added 2 commits January 11, 2024 09:15

Merge remote-tracking branch 'origin/main' into SOLR-17096

ee625b0

# Conflicts: # solr/CHANGES.txt

Use new system property, address commments

20fce8e

pvcnt reviewed Jan 12, 2024

View reviewed changes

dsmiley changed the title ~~SOLR-17096: Cluster Singleton plugin support for immutable deployments~~ SOLR-17096: Cluster Singleton plugin support in solr.xml Jan 13, 2024

dsmiley reviewed Jan 15, 2024

View reviewed changes

pmcarthur-apache added 2 commits January 15, 2024 12:46

Merge remote-tracking branch 'origin/main' into SOLR-17096

a55c631

fix EnvUtil import & minor refactor

9da55ba

dsmiley reviewed Jan 16, 2024

View reviewed changes

Standardize property name

f752d43

pjmcarthur mentioned this pull request Jan 17, 2024

SOLR-15960: Use ConcurrentHashMap for EnvUtils.camelCaseToDotsMap #2204

Merged

7 tasks

dsmiley merged commit 1de6d08 into apache:main Jan 17, 2024
2 of 3 checks passed

		// configBuilder.setReplicaPlacementFactoryConfig(
		// getPluginInfo(root.get("replicaPlacementFactory")));

SOLR-17096: Cluster Singleton plugin support in solr.xml #2126

SOLR-17096: Cluster Singleton plugin support in solr.xml #2126

Conversation

pjmcarthur commented Dec 7, 2023 • edited Loading

Description

Solution

Tests

Checklist

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley commented Dec 9, 2023

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvcnt commented Dec 24, 2023

pjmcarthur commented Jan 3, 2024

pvcnt commented Jan 3, 2024

dsmiley commented Jan 4, 2024

pvcnt commented Jan 4, 2024

dsmiley commented Jan 4, 2024

pvcnt commented Jan 4, 2024 • edited Loading

pjmcarthur commented Jan 5, 2024

dsmiley commented Jan 7, 2024

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

pjmcarthur commented Jan 11, 2024

pvcnt 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

dsmiley commented Jan 17, 2024

dsmiley commented Jan 17, 2024

pjmcarthur commented Dec 7, 2023 •

edited

Loading

pvcnt commented Jan 4, 2024 •

edited

Loading