Run cluster info requests on local node #120982

nielsbauman · 2025-01-28T05:20:30Z

The actions GetIndex, GetMapping, and ExplainLifecycle (ILM) solely need the cluster state, they can run on any node.

The actions `GetIndex`, `GetMapping`, and `ExplainLifecycle` (ILM) solely need the cluster state, they can run on any node.

github-actions · 2025-01-28T05:20:40Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2025-01-28T05:20:54Z

Pinging @elastic/es-data-management (Team:Data Management)

elasticsearchmachine · 2025-01-28T05:20:55Z

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

nielsbauman · 2025-01-28T05:26:12Z

rest-api-spec/src/main/resources/rest-api-spec/api/indices.get_field_mapping.json

-      },
-      "local":{
-        "type":"boolean",
-        "description":"Return local information, do not retrieve the state from master node (default: false)"


This param for the GetFieldMapping API was deprecated in #55014 and I think #57265 aimed to propagate that deprecation to the REST API spec, but it changed get_mapping.json instead of get_field_mapping.json. #55100 removed the local param for the field mapping API but the regular mapping API still supports the local param. That's why I'm deleting the param here and deprecating it in get_mapping.json below.

Could you pull this out to a separate PR?

nielsbauman · 2025-01-28T05:27:30Z

server/src/internalClusterTest/java/org/elasticsearch/action/IndicesRequestIT.java

@@ -516,16 +514,6 @@ public void testDeleteIndex() {
        assertSameIndices(deleteIndexRequest, TransportDeleteIndexAction.TYPE.name());
    }

-    public void testGetMappings() {


I deleted this test because it didn't seem to make much sense to keep it when we run the action on the local node -- and it was failing.

nielsbauman · 2025-01-28T05:40:25Z

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexRequest.java

+    /**
+     * Even though this action is executed on the local node, we still need to be able to read an incoming request as it's used across
+     * clusters.
+     */


This one is worth discussing. It looks like the GetIndexAction is used across clusters:

elasticsearch/server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexAction.java

Line 19 in a59c182

public static final RemoteClusterActionType<GetIndexResponse> REMOTE_TYPE = new RemoteClusterActionType<>(NAME, GetIndexResponse::new);

which means we'll need to support transport serialization for both GetIndexRequest and GetIndexResponse. That in turn meant I had to remove final LocalClusterStateRequest#writeTo (see further down this PR). I don't really see a way around this but any suggestions are welcome.

We could also add final back into LocalClusterStateReuqest#writeTo and add a method like writeToRemoteCluster to something along those lines, to make it more explicit that that method is required for cross-cluster requests.

nielsbauman · 2025-01-28T05:45:16Z

...in/ilm/src/main/java/org/elasticsearch/xpack/ilm/action/TransportExplainLifecycleAction.java

@@ -177,7 +172,7 @@ static IndexLifecycleExplainResponse getIndexLifecycleExplainResponse(
                    lifecycleState.phase(),
                    lifecycleState.action(),
                    // treat a missing policy as if the index is in the error step
-                    indexLifecycleService.policyExists(policyName) == false ? ErrorStep.NAME : lifecycleState.step(),
+                    policyExists == false ? ErrorStep.NAME : lifecycleState.step(),


The IndexLifecycleService only runs on the master node, which meant that policyExists was always false. I changed it to use the Metadata to check for policy existence.

DaveCTurner

TBH I'm not really sure what the ClusterInfoAction abstraction is for. Aren't there lots of other things that could be such an action? They don't really return info about a cluster either.

I think I'd rather we tried to separate out the remote-cluster side of this somehow. Maybe set up something that proxies to the local-cluster action and handles the wire protocol.

DaveCTurner · 2025-01-28T08:06:56Z

server/src/main/java/org/elasticsearch/action/support/local/LocalClusterStateRequest.java

    @Override
-    public final void writeTo(StreamOutput out) throws IOException {
+    public void writeTo(StreamOutput out) throws IOException {


Yeah I don't really like removing this final. I think I'd rather we just made TransportGetIndexAction extends HandledTransportAction rather than trying to generalize TransportClusterInfoAction for this one special case.

Hmm that means we'd have to re-implement/copy the code to wait for cluster blocks. Also, the TransportClusterStateAction faces the same issue -- and that one also needs to wait for the master in the cluster state.
We can try extracting that waiting logic so TransportLocalClusterStateAction, TransportGetIndexAction, and TransportClusterStateAction can share it, but that might get a bit ugly.

nielsbauman · 2025-01-28T22:25:02Z

@DaveCTurner

TBH I'm not really sure what the ClusterInfoAction abstraction is for.

I guess it's only to avoid storing indices and indicesOptions in extending classes. But I agree that that value seems rather minimal as we have plenty of classes that define those fields themselves. I'd also be OK with getting rid of the ClusterInfoAction abstraction first (in separate PRs) and converting these requests individually, but that would be more of a standalone cleanup as I don't think it is strongly related to what we're trying to do here.

DaveCTurner · 2025-01-29T15:52:44Z

I don't think it is strongly related to what we're trying to do here.

It'd mean that we no longer had GetIndexRequest extends ClusterInfoRequest and therefore had to make the abstract ClusterInfoRequest serializable even though all its other implementations are local-only.

I think the proxy action idea should work out ok: separate out the remote-cluster get-index action and register this remote-only action with the transport service, then have it simply delegate into the local-node-only get-index action.

nielsbauman · 2025-02-04T00:10:03Z

@DaveCTurner alright, I'll work on removing the ClusterInfo abstraction.

About your proxy action idea, the proxy action would use the same request and response classes, right? Where would you define the wire (de)serialization?

DaveCTurner · 2025-02-04T10:42:15Z

I would have a different request class for the proxy action, extending TransportRequest rather than ClusterInfoRequest. That way you can define the wire protocol there, plus some simple way to convert it into the ClusterInfoRequest for local execution.

I believe you will be able to register the proxy action with the TransportService and the genuine action with the ActionModule under the same action name.

nielsbauman · 2025-02-14T10:07:05Z

Wouldn't that require duplicating all the fields between the GetIndexRequest (plus the ones in ClusterInfoRequest) and the remote request class? Those fields would then also have to be kept in sync to avoid incompatibilities between the local and remote requests.

I thought about the alternative of making TransportGetIndexAction extend HandledTransportAction instead as that would allow us to keep the serialization without any issues. However, that would mean that we'd have to copy over the execution code of TransportLocalClusterStateAction that waits for the cluster block etc.

DaveCTurner · 2025-02-17T08:23:31Z

Wouldn't that require duplicating all the fields

I'd expect us to pull out a record for all the common fields rather than just blindly duplicating them one-by-one.

making TransportGetIndexAction extend HandledTransportAction instead

Yeah that seems like it'd lead to worse duplication problems.

nielsbauman · 2025-03-26T10:32:32Z

Closing this in favor of #125652 as that's the last remaining ClusterInfo action that is converted to run on the local node.

The serialization issue we talked about in this PR is no longer relevant as the cross-cluster usage of the GET index API is obsolete and will be removed soon. See #125652 (comment)

Run cluster info requests on local node

08ae48e

The actions `GetIndex`, `GetMapping`, and `ExplainLifecycle` (ILM) solely need the cluster state, they can run on any node.

nielsbauman added >enhancement :Data Management/Indices APIs APIs to create and manage indices and templates Team:Data Management Meta label for data/management team v9.0.0 labels Jan 28, 2025

Update docs/changelog/120982.yaml

d4a9413

elasticsearchmachine and others added 2 commits January 28, 2025 05:28

[CI] Auto commit changes from spotless

e61990f

Fix license header

67e44b8

nielsbauman commented Jan 28, 2025

View reviewed changes

Remove redundant test

a15e197

DaveCTurner reviewed Jan 28, 2025

View reviewed changes

elasticsearchmachine added v9.1.0 and removed v9.0.0 labels Jan 30, 2025

This was referenced Feb 18, 2025

Run TransportExplainLifecycleAction on local node #122885

Merged

Run TransportGetMappingsAction on local node #122921

Merged

nielsbauman closed this Mar 26, 2025

nielsbauman deleted the local-cluster-info branch March 26, 2025 10:32

Run cluster info requests on local node #120982

Run cluster info requests on local node #120982

Uh oh!

Conversation

nielsbauman commented Jan 28, 2025

Uh oh!

github-actions bot commented Jan 28, 2025

Uh oh!

elasticsearchmachine commented Jan 28, 2025

Uh oh!

elasticsearchmachine commented Jan 28, 2025

Uh oh!

nielsbauman Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

nielsbauman Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

nielsbauman Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

nielsbauman Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

nielsbauman Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

nielsbauman commented Jan 28, 2025

Uh oh!

DaveCTurner commented Jan 29, 2025

Uh oh!

nielsbauman commented Feb 4, 2025

Uh oh!

DaveCTurner commented Feb 4, 2025

Uh oh!

nielsbauman commented Feb 14, 2025

Uh oh!

DaveCTurner commented Feb 17, 2025

Uh oh!

nielsbauman commented Mar 26, 2025

Uh oh!

Uh oh!