Operator/autoscaling by grcevski · Pull Request #89708 · elastic/elasticsearch

grcevski · 2022-08-29T20:36:14Z

This PR adds support for /_autoscaling/policy for file based settings. The PR uses similar approach in how we handled ILM policies, where we load the state handlers through SPI.

Relates to #89183

We can now load the handler when the plugin loads.

We can now load the handler when the plugin loads in tests

elasticsearchmachine · 2022-08-29T20:36:38Z

Hi @grcevski, I've created a changelog YAML for you.

elasticsearchmachine · 2022-08-29T20:36:38Z

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

grcevski · 2022-08-29T20:39:34Z

server/src/main/java/org/elasticsearch/cluster/service/ClusterService.java


    private RerouteService rerouteService;

+    private AllocationService allocationService;


I'm looking for suggestions if this can be made better. Right now the allocationDeciders are injected in the TransportActions that autoscaling uses, but I can't use injection since we made a design decision to use SPI for the reserved state handlers.

Essentially, I need a way to get to the allocationDeciders to be able to create/pass the validator in to the transport action transformation methods.

grcevski · 2022-08-29T20:40:35Z

...a/org/elasticsearch/xpack/autoscaling/LocalStateReservedAutoscalingStateHandlerProvider.java

+/**
+ * Autoscaling provider implementation for the {@link ReservedClusterStateHandlerProvider} service interface
+ */
+public class LocalStateReservedAutoscalingStateHandlerProvider implements ReservedClusterStateHandlerProvider {


This is a clone of the regular plugin SPI state handler provider for integration test purposes.

grcevski · 2022-08-29T20:41:54Z

...rc/main/java/org/elasticsearch/xpack/autoscaling/action/ReservedAutoscalingPolicyAction.java

+            state = TransportPutAutoscalingPolicyAction.putAutoscalingPolicy(
+                state,
+                request,
+                policyValidatorHolder.get(clusterService.getAllocationService().getAllocationDeciders())


Perhaps there's a better way to get to the allocationDeciders without injection?

henningandersen

I mostly focused on how to provide the allocation deciders and left a comment on that. I'd prefer to defer the rest of the review until that is tackled.

henningandersen · 2022-08-31T17:34:38Z

server/src/main/java/org/elasticsearch/cluster/service/ClusterService.java

        return rerouteService;
    }

+    public void setAllocationService(AllocationService allocationService) {


I think I'd prefer to either:

Support injection for the reserved state plugins.

Add an explicit AllocationDecidersProvider object that is passed to Plugin.createComponents that can be used to access this.

Putting it here is polluting this class I think with responsibilities that does not concern it.

I think I'd prefer to either:

Support injection for the reserved state plugins.

Add an explicit AllocationDecidersProvider object that is passed to Plugin.createComponents that can be used to access this.

Putting it here is polluting this class I think with responsibilities that does not concern it.

Thanks for reviewing this Henning. I took approach 2. and added the AllocationDeciders provider.

grcevski · 2022-09-01T13:35:12Z

@elasticsearchmachine run elasticsearch-ci/bwc

henningandersen

I got what I asked for 🙂 , unfortunately it resulted in quite many changed files. Can we make the allocationDeciders change in a separate PR first, such that this PR gets easier to review? I added a couple of comments to the allocationDeciders change that should be carried over to the new PR as well.

henningandersen · 2022-09-01T17:32:49Z

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

                    repositoriesServiceReference::get,
-                    tracer
+                    tracer,
+                    clusterModule.getAllocationService()::getAllocationDeciders


Since getAllcationDeciders returns a final field, I think this means that we have the list of deciders already and might as well pass the object directly instead of a supplier of it?

henningandersen · 2022-09-04T18:39:41Z

x-pack/plugin/autoscaling/src/main/java/org/elasticsearch/xpack/autoscaling/Autoscaling.java

+        var capacityServiceHolder = new AutoscalingCalculateCapacityService.Holder(this);
+        reservedAutoscalingPolicyAction.set(new ReservedAutoscalingPolicyAction(capacityServiceHolder, allocationDecidersSupplier));


Since we now have the list of deciders directly, could we not as well just set it directly here rather than pass it around in several places? I.e., add:

this.allocationDeciders.set(deciders);

and remove it from the argument to ReservedAutoscalingPolicyAction as well as from being injected into the two transport services that gets it (only to pass it to the plugin).

grcevski · 2022-09-06T18:02:55Z

I got what I asked for 🙂 , unfortunately it resulted in quite many changed files. Can we make the allocationDeciders change in a separate PR first, such that this PR gets easier to review? I added a couple of comments to the allocationDeciders change that should be carried over to the new PR as well.

Sounds good. I'll split this off in another PR and mark this in draft until that's merged. Thanks again for the review!

grcevski · 2022-09-07T17:53:47Z

server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsService.java


-    // package private for testing
-    Path operatorSettingsDir() {
+    public Path operatorSettingsDir() {


Exposed for easier test writing.

grcevski · 2022-09-07T17:56:03Z

...a/org/elasticsearch/xpack/autoscaling/LocalStateReservedAutoscalingStateHandlerProvider.java

+/**
+ * Autoscaling provider implementation for the {@link ReservedClusterStateHandlerProvider} service interface
+ */
+public class LocalStateReservedAutoscalingStateHandlerProvider implements ReservedClusterStateHandlerProvider {


Mock version of the SPI handler provider so that we can write Java integration tests. It implements equals and hashcode so we deduplicate the plugin, as it can be discovered multiple times in the MockNode because all plugins are loaded by the same classloader.

Should we not fix that in MockNode instead? I am not sure I follow the equals/hashCode problem, but sounds like we are relying on MockNode using a Set to collect these providers?

elasticsearchmachine · 2022-09-07T17:57:41Z

Hi @grcevski, I've updated the changelog YAML for you.

grcevski · 2022-09-07T19:51:21Z

@elasticsearchmachine run elasticsearch-ci/part-1

…rch into operator/autoscaling

henningandersen

LGTM.

Left a number of comments I'd prefer to see included.

henningandersen · 2022-09-12T11:26:23Z

...a/org/elasticsearch/xpack/autoscaling/LocalStateReservedAutoscalingStateHandlerProvider.java

+import java.util.Objects;
+
+/**
+ * Autoscaling provider implementation for the {@link ReservedClusterStateHandlerProvider} service interface


Can you add a comment explaining why we need this test override? It is not clear to me and I'd rather read it than find out.

henningandersen · 2022-09-12T11:27:14Z

...a/org/elasticsearch/xpack/autoscaling/LocalStateReservedAutoscalingStateHandlerProvider.java

+/**
+ * Autoscaling provider implementation for the {@link ReservedClusterStateHandlerProvider} service interface
+ */
+public class LocalStateReservedAutoscalingStateHandlerProvider implements ReservedClusterStateHandlerProvider {


Should we not fix that in MockNode instead? I am not sure I follow the equals/hashCode problem, but sounds like we are relying on MockNode using a Set to collect these providers?

henningandersen · 2022-09-12T11:29:39Z

x-pack/plugin/autoscaling/src/main/java/org/elasticsearch/xpack/autoscaling/Autoscaling.java

        this.clusterServiceHolder.set(clusterService);
        this.allocationDeciders.set(allocationDeciders);
        var capacityServiceHolder = new AutoscalingCalculateCapacityService.Holder(this);
+        this.reservedAutoscalingPolicyAction.set(new ReservedAutoscalingPolicyAction(capacityServiceHolder));


Can we not simply defer the construction of this until reservedClusterStateHandlers? That seems more intuitive to me.

I avoided doing that because then I'd have to also keep a local reference to the AutoscalingCalculateCapacityService.Holder, I need it to construct the action object.

henningandersen · 2022-09-12T11:33:26Z

...rc/main/java/org/elasticsearch/xpack/autoscaling/action/ReservedAutoscalingPolicyAction.java

+
+    @Override
+    public TransformState transform(Object source, TransformState prevState) throws Exception {
+        var requests = prepare(source);


Can we do the casting here rather than in prepare? That avoids SuppressWarnings and also seems more appropriate to deal with input validation immediately in this method.

henningandersen · 2022-09-12T11:34:25Z

...rc/main/java/org/elasticsearch/xpack/autoscaling/action/ReservedAutoscalingPolicyAction.java

+    }
+
+    @SuppressWarnings("unchecked")
+    public Collection<PutAutoscalingPolicyAction.Request> prepare(Object input) {


I think this can be private?

henningandersen · 2022-09-12T11:34:43Z

...rc/main/java/org/elasticsearch/xpack/autoscaling/action/ReservedAutoscalingPolicyAction.java

+        return NAME;
+    }
+
+    @SuppressWarnings("unchecked")


Can this be moved to the statement?

henningandersen · 2022-09-12T11:42:59Z

...rc/main/java/org/elasticsearch/xpack/autoscaling/action/ReservedAutoscalingPolicyAction.java

+    }
+
+    @Override
+    public TransformState transform(Object source, TransformState prevState) throws Exception {


Could this not get the right type instead? Seems it is T at the interface level and the call in ReservedStateUpdateTask can be massaged to fix this. The service does "guarantee" it, since it only passes output of fromXContent to this method.

On the caller side in ReservedStateUpdateTask I have a composite object containing the data for all of the handlers. I parse everything first in a map of String key to data type required by the handler, but I can't retain the type information because the data types are in classes provided by plugins and the server doesn't know of them.

henningandersen · 2022-09-12T11:50:28Z

...src/test/java/org/elasticsearch/xpack/autoscaling/action/ReservedAutoscalingPolicyTests.java

+
+        prevState = updatedState;
+        updatedState = processJSON(action, prevState, json);
+        assertThat(updatedState.keys(), containsInAnyOrder("my_autoscaling_policy", "my_autoscaling_policy_1"));


Can we also validate that the roles and deciders are correct in cluster state?

Nikola Grcevski added 6 commits August 29, 2022 16:30

Add reserved action for autoscaling

59a4deb

Add SPI handler provider

c903228

We can now load the handler when the plugin loads.

[TEST] Add SPI handler provider for test

0a87ab8

We can now load the handler when the plugin loads in tests

Make some methods public for testing

25c0fa1

[TEST] Add autoscaling handler test

5f177cf

[TEST] Add autoscaling integration test

637d63f

grcevski added >enhancement :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team Team:Distributed Meta label for distributed team. v8.5.0 labels Aug 29, 2022

elasticsearchmachine removed the Team:Distributed Meta label for distributed team. label Aug 29, 2022

Update docs/changelog/89708.yaml

6eb0fda

grcevski commented Aug 29, 2022

View reviewed changes

Update 89708.yaml

ed30627

grcevski mentioned this pull request Aug 29, 2022

File Based Settings for Elasticsearch #89183

Closed

10 tasks

grcevski requested a review from henningandersen August 29, 2022 20:43

henningandersen reviewed Aug 31, 2022

View reviewed changes

Add allocation deciders supplier to createComponents

12fdcea

grcevski requested a review from a team as a code owner August 31, 2022 23:27

grcevski requested review from AndersonQ and aleksmaus and removed request for a team August 31, 2022 23:27

Merge branch 'main' into operator/autoscaling

f4e39b6

cmacknz removed request for AndersonQ and aleksmaus August 31, 2022 23:53

grcevski requested a review from henningandersen September 1, 2022 14:30

henningandersen reviewed Sep 4, 2022

View reviewed changes

Merge branch 'main' into operator/autoscaling

c652c66

grcevski marked this pull request as draft September 6, 2022 18:03

grcevski mentioned this pull request Sep 6, 2022

Add allocation deciders in createComponents #89836

Merged

Nikola Grcevski added 2 commits September 7, 2022 09:29

Merge main

782ded9

Adjust code to match main

d93d715

grcevski commented Sep 7, 2022

View reviewed changes

grcevski marked this pull request as ready for review September 7, 2022 17:57

grcevski requested a review from henningandersen September 7, 2022 17:57

Update docs/changelog/89708.yaml

0018f75

Nikola Grcevski added 2 commits September 7, 2022 20:06

Merge branch 'main' into operator/autoscaling

b14b182

Merge branch 'operator/autoscaling' of github.com:grcevski/elasticsea…

5801ad4

…rch into operator/autoscaling

henningandersen approved these changes Sep 12, 2022

View reviewed changes

Nikola Grcevski added 2 commits September 12, 2022 16:33

Merge branch 'main' into operator/autoscaling

944b8fd

Address feedback

700b558

grcevski merged commit 9d774d9 into elastic:main Sep 13, 2022

grcevski deleted the operator/autoscaling branch September 13, 2022 15:56


		private RerouteService rerouteService;

		private AllocationService allocationService;

		var capacityServiceHolder = new AutoscalingCalculateCapacityService.Holder(this);
		reservedAutoscalingPolicyAction.set(new ReservedAutoscalingPolicyAction(capacityServiceHolder, allocationDecidersSupplier));

Conversation

grcevski commented Aug 29, 2022

Uh oh!

elasticsearchmachine commented Aug 29, 2022

Uh oh!

elasticsearchmachine commented Aug 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grcevski commented Sep 1, 2022

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grcevski commented Sep 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 7, 2022

Uh oh!

grcevski commented Sep 7, 2022

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants