datafeed: check remote_cluster_client before cluster aliases in start #129601

schase-es · 2025-06-18T04:58:47Z

TransportStartDatafeedAction previously tried to validate remote index cluster names in datafeed jobs, before checking if the local cluster had remote_cluster_client role. Because this role enables retrieval of the remote cluster names, the validation step would always fail with a no-such-cluster exception. This was confusing. This change moves the remote_cluster_client check ahead of cluster name validation, and adds a test.

Closes ES-11841

TransportStartDatafeedAction previously tried to validate remote index cluster names in datafeed jobs, before checking if the local cluster had remote_cluster_client role. Because this role enables retrieval of the remote cluster names, the validation step would always fail with a no-such-cluster exception. This was confusing. This change moves the remote_cluster_client check ahead of cluster name validation, and adds a test. Closes ES-11841

schase-es · 2025-06-18T04:59:55Z

Including David Kyle as the last person who edited these, as an FYI... this area seems to have limited distributed coordination involvement in the past.

nicktindall

Some minor nits, but this LGTM. Please wait until we've got approval from someone in ML before merging though, because I'm not 100% familiar with this code.

(specific question to someone more familiar with the code would be - is this the best place to do the check? Node roles are immutable, so we could do the check when the job is created, but the master could change to one that does have the role between creation and starting, and then everything would be fine? so perhaps start is the right place?)

nicktindall · 2025-06-18T05:08:37Z

...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java

+ * remote_cluster_client role is missing in the local cluster. This prevents remote indices from being
+ * resolved to their cluster names.
+ *
+ * Written to address github issue 121149.


Nit: Perhaps
@see <a href="https://github.com/elastic/elasticsearch/issues/121149">GitHub issue 121149</a>
or similar? I think it'll look nicer in javadoc/mouse-overs

nicktindall · 2025-06-18T05:18:01Z

...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java

+                """);
+            try {
+                localClient.performRequest(startRequest);
+                assertTrue("This request should fail", false);


More succinct to use

ResponseException e = assertThrows(ResponseException.class, () -> localClient.performRequest(startRequest);

Ah! I was looking around for this, but couldn't find it -- I see it's a junit method now

nicktindall · 2025-06-18T05:24:00Z

...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java

+                    .*Datafeed \\[test_datafeed\\] is configured with a remote index pattern\\(s\\) \
+                    \\[remote_cluster:remote_index\\] but the current node \\[local_cluster-0\\] \
+                    is not allowed to connect to remote clusters. Please enable node.remote_cluster_client \
+                    for all machine learning nodes and master-eligible nodes.*""", messages[1]));


I wonder if org.elasticsearch.common.regex.Regex#simpleMatch(java.lang.String, java.lang.String, boolean) might be more forgiving/readable? it just uses wildcard characters rather than regex syntax.

Usually assertThat(X, containsString(Y)) is good enough

assertThat(e.getMessage(), containsString("Datafeed [test_datafeed] is configured with a remote index pattern [remote_cluster:remote_index] but the current node [local_cluster-0] is not allowed to connect to remote clusters. Please enable node.remote_cluster_client for all machine learning nodes and master-eligible nodes."));

davidkyle · 2025-06-18T11:55:58Z

.../plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java

+                                clusterService.getNodeName()
+                            )
+                        )
+                    );


Suggested change

);

);

return;

Good catch!!

davidkyle · 2025-06-18T12:01:37Z

...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java

+                    .*Datafeed \\[test_datafeed\\] is configured with a remote index pattern\\(s\\) \
+                    \\[remote_cluster:remote_index\\] but the current node \\[local_cluster-0\\] \
+                    is not allowed to connect to remote clusters. Please enable node.remote_cluster_client \
+                    for all machine learning nodes and master-eligible nodes.*""", messages[1]));


Usually assertThat(X, containsString(Y)) is good enough

assertThat(e.getMessage(), containsString("Datafeed [test_datafeed] is configured with a remote index pattern [remote_cluster:remote_index] but the current node [local_cluster-0] is not allowed to connect to remote clusters. Please enable node.remote_cluster_client for all machine learning nodes and master-eligible nodes."));

davidkyle · 2025-06-18T12:08:48Z

...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java

+    protected String getTestRestCluster() {
+        return localCluster.getHttpAddresses();
+    }
+
+    @Override
+    protected boolean preserveIndicesUponCompletion() {
+        return true;
+    }
+
+    @Override
+    protected boolean preserveClusterUponCompletion() {
+        return true;
+    }
+
+    @Override
+    protected boolean preserveDataStreamsUponCompletion() {
+        return true;


Are you sure you want these settings? If another test is added to this class these settings may interfere with the test results.

I'm not sure actually -- the test errorred-out without them. This was adapted from the MultiClustersIT test... I'll see if I can learn more about them

Oh I see -- that missing return you flagged in TransportStartDatafeedAction was the culprit -- whatever state that put the thread pools caused an error in clean-up.

davidkyle · 2025-06-18T12:22:31Z

...reams/src/javaRestTest/java/org/elasticsearch/datastreams/DatafeedRemoteClusterClientIT.java

+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.datastreams;


This test should really be in the ml qa suite https://github.com/elastic/elasticsearch/tree/main/x-pack/plugin/ml/qa

The existing multi cluster tests are yaml tests and difficult to adapt. Please create a new folder datafeed-multicluster-tests in ml/qa and move this test to that folder. You will need a new build.gradle in the same folder to run the test, I'm not certain what should be in the gradle file but I'll help you figure it out. As a starting point try this

apply plugin: 'elasticsearch.internal-java-rest-test' dependencies { testImplementation project(path: ':test:test-clusters') javaRestTestImplementation project(path: xpackModule('core')) }

Assuming you use the suggested folder names run the test with this command:

./gradlew :x-pack:plugin:ml:qa:datafeed-multicluster-tests:javaRestTest

Nice test BTW, thanks for adding it

Thanks for these notes David -- this is really helpful. I'm very new here and don't know lots about the testing environment.

- changed doc url to link github issue - added return after continuation send -- oops! - use junit methods for testing message/exception matching - moved the test to the xpack ml/qa folder - added a gradle build file that appears to work - removed excess override settings from RestTest

…-indices

schase-es requested review from nicktindall and davidkyle June 18, 2025 04:58

schase-es added >non-issue :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. backport auto-backport Automatically create backport pull requests when merged v8.19.0 v9.1.0 labels Jun 18, 2025

nicktindall approved these changes Jun 18, 2025

View reviewed changes

davidkyle reviewed Jun 18, 2025

View reviewed changes

schase-es and others added 4 commits June 18, 2025 17:34

[CI] Auto commit changes from spotless

7482dad

Merge branch 'main' into ES-11841_remote-cluster-client-role-datafeed…

6267596

…-indices

Trimmed extra dependencies from gradle

1e44abc

datafeed: check remote_cluster_client before cluster aliases in start #129601

Are you sure you want to change the base?

datafeed: check remote_cluster_client before cluster aliases in start #129601

Conversation

schase-es commented Jun 18, 2025

Uh oh!

schase-es commented Jun 18, 2025

Uh oh!

nicktindall left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

nicktindall left a comment •

edited

Loading