Implement few operator handlers #88097
Conversation
Implement operator handlers for cluster state and ILM
grcevski
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| protected Set<String> modifiedKeys(Request request) { | ||
| return Collections.emptySet(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Here's an example of how these two methods operatorHandlerName and modifiedKeys could be used in doExecute below to verify that an operation is allowed to execute:
There was a problem hiding this comment.
Regarding the validateForOperatorState(...) validation method in the referenced link,
I do think the logic looks good, but I do think this also needs to be validated as part
of a cluster state update. Right now the validation there only occurs on the action/transport
level and there is no guarantee that what is validated at that point in time is also
true when the update if really performed. I think it is a good pre-check, that is likely
to catch almost all cases when resources are updates that are being managed operator state.
For example in order to guarantee that no operator state managed ilm policies are modified
via API. The UpdateLifecyclePolicyTask/DeleteLifecyclePolicyTask should do this validation
check as well as part of their respective execute(...) method.
grcevski
added
the
:Data Management/ILM+SLM
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
There was a problem hiding this comment.
The ILM parts look good, I left some minor comments around it.
I also commented on your comment, which is a bit more involved,
but I think falls a bit outside of this PR, but I think is important.
| * Internally it uses {@link TransportPutLifecycleAction} and | ||
| * {@link TransportDeleteLifecycleAction} to add, update and delete ILM policies. | ||
| */ | ||
| public class ImmutableLifecycleAction implements ImmutableClusterStateHandler<LifecyclePolicy> { |
There was a problem hiding this comment.
Maybe move this class to the org.elasticsearch.xpack.ilm.action package?
That way the newly added constructors to UpdateLifecyclePolicyTask and DeleteLifecyclePolicyTask can have package level visibility? And should avoid accidental use outside this package.
| /** | ||
| * Used by the {@link org.elasticsearch.immutablestate.ImmutableClusterStateHandler} for ILM | ||
| * {@link ImmutableLifecycleAction} | ||
| * @param policyName |
There was a problem hiding this comment.
Maybe remove @param line here? Since it doesn't add documentation.
| * <p> | ||
| * It disables verbose logging and has no filtered headers. | ||
| * | ||
| * @param request |
There was a problem hiding this comment.
Remove empty @param lines here?
| /** | ||
| * ILM Provider implementation for the {@link ImmutableClusterStateHandlerProvider} service interface | ||
| */ | ||
| public class ILMImmutableStateHandlerProvider implements ImmutableClusterStateHandlerProvider { |
There was a problem hiding this comment.
Maybe move this class to org.elasticsearch.xpack.ilm.action or org.elasticsearch.xpack.ilm package?
Since this package only contains a single class.
|
|
||
| public UpdateLifecyclePolicyTask( | ||
| Request request, | ||
| ActionListener<AcknowledgedResponse> listener, | ||
| XPackLicenseState licenseState, | ||
| Map<String, String> filteredHeaders, | ||
| NamedXContentRegistry xContentRegistry, | ||
| Client client | ||
| Client client, | ||
| boolean verboseLogging |
There was a problem hiding this comment.
Maybe remove the verboseLogging param here?
and just set the verboseLogging to true?
Then the other constructor can't reuse this constructor, but
then for regular usage of this class, verbose logging is guaranteed to be enabled.
| protected Set<String> modifiedKeys(Request request) { | ||
| return Collections.emptySet(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Regarding the validateForOperatorState(...) validation method in the referenced link,
I do think the logic looks good, but I do think this also needs to be validated as part
of a cluster state update. Right now the validation there only occurs on the action/transport
level and there is no guarantee that what is validated at that point in time is also
true when the update if really performed. I think it is a good pre-check, that is likely
to catch almost all cases when resources are updates that are being managed operator state.
For example in order to guarantee that no operator state managed ilm policies are modified
via API. The UpdateLifecyclePolicyTask/DeleteLifecyclePolicyTask should do this validation
check as well as part of their respective execute(...) method.
| return handlers; | ||
| } | ||
|
|
||
| public static void handler(ImmutableClusterStateHandler<?> handler) { |
There was a problem hiding this comment.
I think does need to be invoked and should be provided a ImmutableLifecycleAction instance, right?
There was a problem hiding this comment.
Ah nice catch, I should've brought in this change in this PR. https://github.com/elastic/elasticsearch/pull/86224/files#diff-245140b1f56e32ce2f87c455562395af21e0b6730984b7077dd236b7086e5164R275
|
|
||
| private final ClusterSettings clusterSettings; | ||
|
|
||
| public ImmutableClusterSettingsAction(ClusterSettings clusterSettings) { |
There was a problem hiding this comment.
Should this also get wired up like was done here: https://github.com/elastic/elasticsearch/pull/86224/files#diff-b56bed42f5b0025886eb243ecb48678ac048bfba0ef047545dbb6504e357edf2R901 ?
There was a problem hiding this comment.
Yes, 100%. I left that part to happen in my follow-up draft PR I have now based on this one here: https://github.com/elastic/elasticsearch/pull/88224/files#diff-b56bed42f5b0025886eb243ecb48678ac048bfba0ef047545dbb6504e357edf2R905
After I wire them up, I call the initialize on the controller, which I haven't brought in yet from the initial WIP PR. Once we merge this in, that's the next one.
|
Thanks @ChrisHegarty and @martijnvg ! |
Implement operator handlers for cluster state and ILM.
These operator handlers are the first of the operator handlers we'll be implementing, one specific to cluster state settings and another dealing with cluster state entities (ILM).
Relates to #86224.