Fix test memory leak #88362

grcevski · 2022-07-08T00:39:31Z

I had incorrectly made the assumption that the createComponents will only be called once for ILM plugin initialization. It appears that this isn't the case when we run tests.

This PR fixes how we initialize the ILM immutable state handlers, if the LifecyclePlugin gets re created, only the latest state is reflected in the immutable state handler provider.

Fixes #88337

elasticmachine · 2022-07-08T00:39:34Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

rjernst · 2022-07-08T05:34:05Z

Is there a way we can avoid making production code mutable just for tests?

benwtrent · 2022-07-08T13:37:10Z

...k/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java


 /**
 * ILM Provider implementation for the {@link ImmutableClusterStateHandlerProvider} service interface
 */
 public class ILMImmutableStateHandlerProvider implements ImmutableClusterStateHandlerProvider {
-    private static final Set<ImmutableClusterStateHandler<?>> handlers = ConcurrentHashMap.newKeySet();
+    private static volatile Collection<ImmutableClusterStateHandler<?>> handlers = Collections.emptyList();


Suggested change

private static volatile Collection<ImmutableClusterStateHandler<?>> handlers = Collections.emptyList();

private static final SetOnce<Collection<ImmutableClusterStateHandler<?>>> handlers = new SetOnce<>();

I wonder if this should be a SetOnce<Collection<ImmutableClusterStateHandler<?>>>, then when registerHandlers is called trySet() can be used to atomically update the collection?

benwtrent · 2022-07-08T13:37:51Z

...k/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java


    @Override
    public Collection<ImmutableClusterStateHandler<?>> handlers() {
        return handlers;
    }

    public static void registerHandlers(ImmutableClusterStateHandler<?>... stateHandlers) {
-        handlers.addAll(Arrays.asList(stateHandlers));
+        handlers = List.of(stateHandlers);


Suggested change

handlers = List.of(stateHandlers);

handlers.trySet(List.of(stateHandlers));

Since this completely changes the stored values, it may be good to rename registerHandlers to better indicate that it won't set the values if they are already set.

rjernst · 2022-07-08T13:43:31Z

I think the issue is the use of statics. We don't use them in other places for such registration. These handlers should be captured in instance variables specific to the node, so that many node instances (in tests) can be created.

grcevski · 2022-07-08T18:36:31Z

So I flipped this around a bit and I removed the use of statics. I also added an example code of how this is intended to work in ActionModule.

Essentially, the SPI creates the service provides independently of the plugin constructor, so if we want to avoid any statics, we need a way to pass down what the handlers need to be constructed with. However, the best place to construct the handlers is during the plugin constructor (or createComponents), since then we have all what we'd need to make the handler work.

To avoid passing a superset of what we might need for construction of the immutable state handlers, I did the following:

Let the plugin create the immutable handler just like any other action handler during construction and keep a reference to it
When and if a handler provider is created, it will ask the plugin to deliver a collection of the handlers it likes to expose.
The provider now receives a list of plugins it 'serves' and it asks the plugins for what handlers they'll expose. We have the plugin instance at the time we call for the providers to be created, since we have already loaded the plugins by then.

grcevski · 2022-07-08T18:37:40Z

server/src/main/java/org/elasticsearch/immutablestate/ImmutableClusterStateHandlerProvider.java

-    Collection<ImmutableClusterStateHandler<?>> handlers();
+    Collection<ImmutableClusterStateHandler<?>> handlers(Collection<? extends Plugin> loadedPlugins);
+
+    Collection<Class<? extends Plugin>> providedPlugins();


I need to add docs here.

grcevski · 2022-07-08T18:38:56Z

...k/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java

    @Override
-    public Collection<ImmutableClusterStateHandler<?>> handlers() {
-        return handlers;
+    public Collection<ImmutableClusterStateHandler<?>> handlers(Collection<? extends Plugin> loadedPlugins) {


The provider receives a reference back of its related plugins, which will supply it with what handlers they want to expose.

grcevski · 2022-07-08T18:50:02Z

I think also, maybe I don't need to worry about a collection of plugins here. It should be probably be only one per plugin.

grcevski · 2022-07-11T21:05:46Z

...k/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/ILMImmutableStateHandlerProvider.java

-    @Override
-    public Collection<ImmutableClusterStateHandler<?>> handlers() {
-        return handlers;
+    public ILMImmutableStateHandlerProvider() {


Declaring this constructor to make sure module-info compiles properly, since SPI requires that we use a no-arg constructor. If we switch to using the META-INF/services file, this problem goes away, however our build 'validateModule' task fails, requiring that we also expose this service provider in our module provides section.

rjernst

I left a couple asks on how this looks with Node/ActionModule. LGTM if you agree on the points I left.

rjernst · 2022-07-11T22:45:45Z

server/src/main/java/org/elasticsearch/action/ActionModule.java

+
+        handlers.add(new ImmutableClusterSettingsAction(clusterSettings));
+        pluginHandlers.forEach(h -> handlers.addAll(h.handlers()));
+        // Initialize the controller when merged


The two collections here seem to be locals. Shouldn't they be captured somewhere?

Yeah, this is unused, I brought in the code to demonstrate how we'd use these when I had the earlier approach. I'll remove this code altogether. The values are locals because the initialization of the controller is not here yet, as in the other PR: https://github.com/elastic/elasticsearch/pull/88224/files#diff-b56bed42f5b0025886eb243ecb48678ac048bfba0ef047545dbb6504e357edf2R915.

I agree with your comments, so I'll refactor the code in the other PR to apply your suggestions on how to initialize the immutable handlers in the other PR.

rjernst · 2022-07-11T22:52:01Z

server/src/main/java/org/elasticsearch/node/Node.java

@@ -1055,6 +1055,8 @@ protected Node(

            logger.debug("initializing HTTP handlers ...");
            actionModule.initRestHandlers(() -> clusterService.state().nodesIfRecovered());
+            logger.debug("initializing operator handlers ...");
+            actionModule.initImmutableClusterStateHandlers(pluginsService);


This doesn't seem like the right place in node construction to init these. The action module is here in the line above for other reasons (and can actually be moved up, it only needs the cluster service created). I would init these when the action module is created, as part of the ctor.

rjernst · 2022-07-11T22:53:47Z

server/src/main/java/org/elasticsearch/action/ActionModule.java

+    public void initImmutableClusterStateHandlers(PluginsService pluginsService) {
+        List<ImmutableClusterStateHandler<?>> handlers = new ArrayList<>();
+
+        List<? extends ImmutableClusterStateHandlerProvider> pluginHandlers = pluginsService.loadServiceProviders(


I think I would rather see loadServiceProviders called in Node, and pass in the List of providers to ActionModule. That better matches the other calls in Node to eg filterPlugins. In general, we try to avoid passing the plugin service itself.

rjernst · 2022-07-11T22:54:18Z

server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java

@@ -531,6 +531,10 @@ public TestExtension() {}
            public TestExtension(TestPlugin plugin) {

            }
+
+            public TestExtension(TestPlugin plugin, String anotherArg) {


Is this used somewhere?

Yes, I added a constructor to make the test check that we disallow a service provider with more than 2 constructors. Previously this class was testing for existence of more than 1.

grcevski · 2022-07-12T00:55:00Z

@elasticmachine run elasticsearch-ci/part-2

* upstream/master: Pass IndexMetadata to AllocationDecider.can_remain (elastic#88453) [TSDB] Cache rollup bucket timestamp to reduce rounding cost (elastic#88420) Correct some typos/mistakes in comments/docs (elastic#88446) Make ClusterInfo use immutable maps in all cases (elastic#88447) Reduce map lookups (elastic#88418) Don't index geo_shape field in AbstractBuilderTestCase (elastic#88437) Remove usages of TestGeoShapeFieldMapperPlugin from enrich module (elastic#88440) Fix test memory leak (elastic#88362) Improve error when sorting on incompatible types (elastic#88399) Remove usages of BucketCollector#getLeafCollector(LeafReaderContext) (elastic#88414) Mute ReactiveStorageIT::testScaleWhileShrinking (elastic#88431) Clarify snapshot docs on archive indices (elastic#88417) [Stack Monitoring] Switch cgroup memory fields to keyword (elastic#88260) Fix RealmIdentifier XContent parser (elastic#88410) Make LoggedExec gradle task configuration cache compatible (elastic#87621) Update CorruptedFileIT so that it passes with new allocation strategy (elastic#88314) Update RareClusterStateIT to work with the new shards allocator (elastic#87922) Ensure CreateApiKey always creates a new document (elastic#88413) # Conflicts: # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/RollupShardIndexer.java

Fix memory leak

a81c683

grcevski added >non-issue :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.4.0 labels Jul 8, 2022

benwtrent reviewed Jul 8, 2022

View reviewed changes

Nikola Grcevski added 2 commits July 8, 2022 14:26

Remove static requirement

158d0db

Better naming

12807d5

grcevski commented Jul 8, 2022

View reviewed changes

Nikola Grcevski added 2 commits July 8, 2022 15:01

Simplify with 1:1 mapping

28f3828

Use our createExtensions SPI

11b4f95

grcevski commented Jul 11, 2022

View reviewed changes

rjernst approved these changes Jul 11, 2022

View reviewed changes

Remove unused code

260db95

grcevski merged commit b55a0bc into elastic:master Jul 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test memory leak #88362

Fix test memory leak #88362

grcevski commented Jul 8, 2022

elasticmachine commented Jul 8, 2022

rjernst commented Jul 8, 2022

benwtrent Jul 8, 2022

benwtrent Jul 8, 2022

rjernst commented Jul 8, 2022

grcevski commented Jul 8, 2022

grcevski Jul 8, 2022

grcevski Jul 8, 2022

grcevski commented Jul 8, 2022

grcevski Jul 11, 2022

rjernst left a comment

rjernst Jul 11, 2022

grcevski Jul 11, 2022

rjernst Jul 11, 2022

rjernst Jul 11, 2022

rjernst Jul 11, 2022

grcevski Jul 11, 2022

grcevski commented Jul 12, 2022

	private static volatile Collection<ImmutableClusterStateHandler<?>> handlers = Collections.emptyList();
	private static final SetOnce<Collection<ImmutableClusterStateHandler<?>>> handlers = new SetOnce<>();

	handlers = List.of(stateHandlers);
	handlers.trySet(List.of(stateHandlers));

Fix test memory leak #88362

Fix test memory leak #88362

Conversation

grcevski commented Jul 8, 2022

elasticmachine commented Jul 8, 2022

rjernst commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Jul 8, 2022

grcevski commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grcevski commented Jul 8, 2022

Choose a reason for hiding this comment

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

grcevski commented Jul 12, 2022