Add node "roles" to allocation explain response #98550

DiannaHohensee · 2023-08-16T21:45:27Z

Report node "roles" in the /_cluster/allocation/explain response.
Nodes with limited sets of roles may affect shard distribution in ways
users did not originally consider, so it is helpful to surface this
information along with node allocation decision explanations.

…esponse

DaveCTurner

The overall structure is great; I left a few superficial comments.

DaveCTurner · 2023-08-17T05:40:25Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+      version: " - 8.9.99"
+      reason: The roles field was introduced in 8.10.0


This will (eventually) land in 8.11 so this bit will need adjusting.

Normally I'd recommend avoiding rebasing PR branches once they're under review, but at the moment the 8.10 freeze has uncovered some issues in CI. You should be able to get a green CI run if you rebase this branch back onto the commit before the version bump (bb2a30f), leaving these lines as they are. Once CI is passing on main again you'll need to merge it in and fix these lines up.

Hmm. I believe a rebase backwards would require a force push to the PR -- as opposed to a merge of the latest that's benign to push. Would not be great to wipe out your comments 😅 Perhaps I should just update this code to 8.11 and wait? Or I might be missing something about the process.

Yeah a force-push would be required, which might confuse Github into losing review context. It doesn't always, but it's a risk. I'm ok with that, it's extremely unusual for main to be this unstable. Or you can indeed just wait for things to settle down before you get a passing CI run.

main has stabilised now so I would expect you can just merge it in and this'll pass CI.

...rTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

DaveCTurner · 2023-08-17T05:48:56Z

...rTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

+        logger.info("--> restarting the stopped node with 'roles' setting added");
+        Settings roleSettings = Settings.builder().putList("node.roles", Arrays.asList("master", "data_hot", "ingest")).build();
+        internalCluster().startNode(Settings.builder().put(settings).put(roleSettings).build());
+        ensureStableCluster(1);


startNode() calls validateClusterFormed() as below, so there's no real need to ensureStableCluster() too (although it doesn't hurt)

at org.elasticsearch.test.InternalTestCluster.validateClusterFormed(InternalTestCluster.java:1220) ~[framework-8.11.0-SNAPSHOT.jar:8.11.0-SNAPSHOT] at org.elasticsearch.test.InternalTestCluster.startAndPublishNodesAndClients(InternalTestCluster.java:1750) ~[framework-8.11.0-SNAPSHOT.jar:8.11.0-SNAPSHOT] at org.elasticsearch.test.InternalTestCluster.startNodes(InternalTestCluster.java:2181) ~[framework-8.11.0-SNAPSHOT.jar:8.11.0-SNAPSHOT] at org.elasticsearch.test.InternalTestCluster.startNode(InternalTestCluster.java:2113) ~[framework-8.11.0-SNAPSHOT.jar:8.11.0-SNAPSHOT] at org.elasticsearch.test.InternalTestCluster.startNode(InternalTestCluster.java:2106) ~[framework-8.11.0-SNAPSHOT.jar:8.11.0-SNAPSHOT]

Oh, nice. So it's only worth calling when restarting a subset of the nodes in a cluster, or stopping nodes, maybe (depending those functions' implementations).

Is it typical for patches to remain focused exclusively on one topic, or are touchups elsewhere OK? For example, I could remove the check here, too, since it's redundant and I've noticed it. Though perhaps validateClusterFormed() and ensureStableCluster() vary enough that the real fix would be a refactor -- which seems too much of a distraction/off topic to do in this patch.

Yes those other calls to ensureStableCluster() also look redundant to me. I think they were necessary when they were added in #22182, but #49248 changed that.

And yes I generally prefer to keep PRs focussed on a single change where possible. Easier on the reviewers, and on future-us if ever we need to look back at the history of changes in this area. A separate PR which removes all those unnecessary ensureStableCluster() calls would be worthwhile.

Thanks for the context.

Separate PR to remove: #98694

...rTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

...r/src/main/java/org/elasticsearch/cluster/routing/allocation/AbstractAllocationDecision.java

DaveCTurner · 2023-08-17T06:03:32Z

...a/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainActionTests.java

+            "data",  // begin roles array
+            "data_cold",
+            "data_content",
+            "data_frozen",
+            "data_hot",
+            "data_warm",
+            "index",
+            "ingest",
+            "master",
+            "ml",
+            "remote_cluster_client",
+            "search",
+            "transform",
+            "voting_only",  // end roles array


This would need updating if we ever add a new node role, but I don't think the test really cares about the exact roles. How about constructing the expected string dynamically:

Suggested change

"data", // begin roles array

"data_cold",

"data_content",

"data_frozen",

"data_hot",

"data_warm",

"index",

"ingest",

"master",

"ml",

"remote_cluster_client",

"search",

"transform",

"voting_only", // end roles array

cae.getCurrentNode().getRoles().stream().map(r -> '"' + r.roleName() + '"').collect(Collectors.joining(", ", "[", "]")),

(and then the format string just needs "roles": %s)

Sounds great, figuring out the formatting to get everything to work in Java was kicking my ass :)

DiannaHohensee

Thanks for the feedback, David! Responded and pushed an update.

...rTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

DiannaHohensee · 2023-08-17T12:15:13Z

...rTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

+        logger.info("--> restarting the stopped node with 'roles' setting added");
+        Settings roleSettings = Settings.builder().putList("node.roles", Arrays.asList("master", "data_hot", "ingest")).build();
+        internalCluster().startNode(Settings.builder().put(settings).put(roleSettings).build());
+        ensureStableCluster(1);


Oh, nice. So it's only worth calling when restarting a subset of the nodes in a cluster, or stopping nodes, maybe (depending those functions' implementations).

Is it typical for patches to remain focused exclusively on one topic, or are touchups elsewhere OK? For example, I could remove the check here, too, since it's redundant and I've noticed it. Though perhaps validateClusterFormed() and ensureStableCluster() vary enough that the real fix would be a refactor -- which seems too much of a distraction/off topic to do in this patch.

...rTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

DiannaHohensee · 2023-08-17T16:05:21Z

...a/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainActionTests.java

+            "data",  // begin roles array
+            "data_cold",
+            "data_content",
+            "data_frozen",
+            "data_hot",
+            "data_warm",
+            "index",
+            "ingest",
+            "master",
+            "ml",
+            "remote_cluster_client",
+            "search",
+            "transform",
+            "voting_only",  // end roles array


Sounds great, figuring out the formatting to get everything to work in Java was kicking my ass :)

...r/src/main/java/org/elasticsearch/cluster/routing/allocation/AbstractAllocationDecision.java

DiannaHohensee · 2023-08-17T16:26:49Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+      version: " - 8.9.99"
+      reason: The roles field was introduced in 8.10.0


Hmm. I believe a rebase backwards would require a force push to the PR -- as opposed to a merge of the latest that's benign to push. Would not be great to wipe out your comments 😅 Perhaps I should just update this code to 8.11 and wait? Or I might be missing something about the process.

elasticsearchmachine · 2023-08-21T07:56:16Z

Pinging @elastic/es-distributed (Team:Distributed)

idegtiarenko · 2023-08-21T09:25:39Z

...rTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

@@ -1150,6 +1152,42 @@ public void testCannotAllocateStaleReplicaExplanation() throws Exception {
        }
    }

+    public void testExplainRolesOutput() throws Exception {
+        logger.info("--> starting 1 node with 'roles' setting specified");
+        Settings roleSettings = Settings.builder().putList("node.roles", Arrays.asList("master", "data_hot", "ingest")).build();


NIT: This is not important here, but please generally prefer List.of(..) to Arrays.asList(..) as first one creates immutable list while the later allows some modifications

Done. Thanks!

idegtiarenko · 2023-08-21T09:30:34Z

...rTest/java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainIT.java

+            // Check that the "roles" reported are those explicitly set via the node Settings.
+            // Note: list() implicitly consumes the parser START_ARRAY and END_ARRAY tokens.
+            assertEquals(Arrays.asList("data_hot", "ingest", "master"), parser.list());
+        }


Wow, I am surprised we have to do this in this test.
I wonder if it is worth opening a tech debt issue to have AbstractXContentSerializingTestCase for ClusterAllocationExplanation instead of doing parsing in IT?

I'm not currently familiar with how AbstractXContentSerializingTestCase works, so can't really weigh in on this without some context.

Yeah I think there's scope for better testing in this area indeed. Not sure AbstractXContentSerializingTestCase is quite right, that is for checking that things faithfully round-trip through XContent which is a much stronger property than we really need here. There's probably a nicer way to verify specific features of the XContent output tho, but I'm ok with following the existing style in this area right now.

idegtiarenko · 2023-08-21T09:41:55Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+      cluster.allocation_explain:
+        body: { "index": "test", "shard": 0, "primary": true }
+
+  - is_true: current_node.roles


Roles are added to other places as well:

allocate unassigned section (if present)

move shard section (if present)

each node of the per node explanation

I believe the first 2 are not present since this explain is executed against allocated shard that could stay on the current node. I do not think they require additional test case and could be covered by serialization test, see https://github.com/elastic/elasticsearch/pull/98550/files#r1299861004

I wonder about the last one. I think it could be covered with additional:
- is_true: node_allocation_decisions.0.roles
unless I miss something.

I tried adding - is_true: node_allocation_decisions.0.roles and node_allocation_decisions is null in this test case. I saw current_node testing in this file, so that was easy to add. I haven't looked into how node_allocation_decisions might be evoked: left that to the the Java unit/integration tests.

The YAML tests (sometimes) run in a one-node cluster so we can't assert anything about the other changed locations here.

I think we could check the node_allocation_decisions bit in ClusterAllocationExplainIT#testExplainRolesOutput if you'd like, by starting a second node with different roles (after creating the index, so we know which node has the shard). I didn't see an easy way to get nodes in the AllocateUnassignedDecision or MoveDecision here, it might be easier to do that in a unit test, but tbh it's the node_allocation_decision that really matters.

Extended ClusterAllocationExplainIT#testExplainRolesOutput to test node_allocation_decisions 👍

…tier

elasticsearchmachine · 2023-08-21T19:29:34Z

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

DaveCTurner · 2023-08-21T20:22:58Z

docs/reference/tab-widgets/troubleshooting/data/diagnose-unassigned-shards.asciidoc

@@ -106,6 +106,7 @@ The response will look like this:
      "node_id" : "8qt2rY-pT6KNZB3-hGfLnw",
      "node_name" : "node-0",
      "transport_address" : "127.0.0.1:9401",
+      "roles": [data, data_cold, data_content, data_frozen, data_hot, data_warm, ingest, master, ml, remote_cluster_client, transform]


nit: (not picked up by the test framework because this example is skipped)

Suggested change

"roles": [data, data_cold, data_content, data_frozen, data_hot, data_warm, ingest, master, ml, remote_cluster_client, transform]

"roles": [data, data_cold, data_content, data_frozen, data_hot, data_warm, ingest, master, ml, remote_cluster_client, transform],

You could also reasonably list fewer roles here, maybe just data_content, data_hot, I don't think it helps the docs to have them all.

idegtiarenko

👍 from my side. Leaving it to David to give a final approval

elasticsearchmachine · 2023-08-23T01:07:03Z

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

DaveCTurner

LGTM great work @DiannaHohensee

DiannaHohensee self-assigned this Aug 16, 2023

elasticsearchmachine added needs:triage Requires assignment of a team area label v8.11.0 labels Aug 16, 2023

Report a node's "roles" setting in the /_cluster/allocation/explain r…

7f79eb6

…esponse

DiannaHohensee changed the title ~~Report the node's "roles" setting in the /_cluster/allocation/explain response~~ Report a node's "roles" setting in the /_cluster/allocation/explain response Aug 16, 2023

DiannaHohensee force-pushed the cluster-allocation-explain-report-node-data-tier branch from 30b1085 to 7f79eb6 Compare August 16, 2023 21:57

cleanup

9b14bdd

DiannaHohensee added the Team:Distributed Meta label for distributed team label Aug 16, 2023

elasticsearchmachine removed the Team:Distributed Meta label for distributed team label Aug 16, 2023

DaveCTurner reviewed Aug 17, 2023

View reviewed changes

DiannaHohensee added 2 commits August 17, 2023 12:29

improvements re David's PR feedback

9a5c6be

missed something

9f05f3b

DiannaHohensee commented Aug 17, 2023

View reviewed changes

romseygeek added :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) and removed needs:triage Requires assignment of a team area label labels Aug 21, 2023

elasticsearchmachine added the Team:Distributed Meta label for distributed team label Aug 21, 2023

idegtiarenko reviewed Aug 21, 2023

View reviewed changes

DiannaHohensee added 4 commits August 21, 2023 13:05

Merge branch 'main' into cluster-allocation-explain-report-node-data-…

6a274a3

…tier

parsing and version changes

7930fef

cleanup

d374126

formatting

cd0e2aa

DiannaHohensee requested review from DaveCTurner and idegtiarenko August 21, 2023 19:21

DiannaHohensee added the >enhancement label Aug 21, 2023

Update docs/changelog/98550.yaml

5320723

trying to fix docs

e92f20c

DaveCTurner mentioned this pull request Aug 21, 2023

[TEST] Remove redundant cluster checks #98694

Merged

DaveCTurner reviewed Aug 21, 2023

View reviewed changes

idegtiarenko approved these changes Aug 22, 2023

View reviewed changes

correcting asciidoc files

e7d2abe

DiannaHohensee changed the title ~~Report a node's "roles" setting in the /_cluster/allocation/explain response~~ Add node "roles" to allocation explain response Aug 23, 2023

Update docs/changelog/98550.yaml

63bd2d4

extend testExplainRolesOuput to test node_allocation_decisions

4eca1c1

DaveCTurner approved these changes Aug 23, 2023

View reviewed changes

DiannaHohensee merged commit a25e176 into elastic:main Aug 23, 2023
12 checks passed

DiannaHohensee mentioned this pull request Aug 23, 2023

Allocation Explain API to report Node Data Tier #97859

Closed

pquentin mentioned this pull request Sep 6, 2024

Add roles to cluster.allocation_explain response elastic/elasticsearch-specification#2874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add node "roles" to allocation explain response #98550

Add node "roles" to allocation explain response #98550

DiannaHohensee commented Aug 16, 2023 •

edited

Loading

DaveCTurner left a comment

DaveCTurner Aug 17, 2023

DiannaHohensee Aug 17, 2023

DaveCTurner Aug 17, 2023

DaveCTurner Aug 21, 2023

DiannaHohensee Aug 21, 2023

DaveCTurner Aug 17, 2023

DiannaHohensee Aug 17, 2023

DaveCTurner Aug 17, 2023 •

edited

Loading

DiannaHohensee Aug 21, 2023

DaveCTurner Aug 17, 2023

DiannaHohensee Aug 17, 2023

DiannaHohensee left a comment

DiannaHohensee Aug 17, 2023

DiannaHohensee Aug 17, 2023

DiannaHohensee Aug 17, 2023

elasticsearchmachine commented Aug 21, 2023

idegtiarenko Aug 21, 2023

DiannaHohensee Aug 21, 2023

idegtiarenko Aug 21, 2023

DiannaHohensee Aug 21, 2023

DaveCTurner Aug 21, 2023

idegtiarenko Aug 21, 2023 •

edited

Loading

DiannaHohensee Aug 21, 2023

DaveCTurner Aug 21, 2023

DiannaHohensee Aug 23, 2023

elasticsearchmachine commented Aug 21, 2023

DaveCTurner Aug 21, 2023

DiannaHohensee Aug 22, 2023

idegtiarenko left a comment

elasticsearchmachine commented Aug 23, 2023

DaveCTurner left a comment

		version: " - 8.9.99"
		reason: The roles field was introduced in 8.10.0

	"roles": [data, data_cold, data_content, data_frozen, data_hot, data_warm, ingest, master, ml, remote_cluster_client, transform]
	"roles": [data, data_cold, data_content, data_frozen, data_hot, data_warm, ingest, master, ml, remote_cluster_client, transform],

Add node "roles" to allocation explain response #98550

Add node "roles" to allocation explain response #98550

Conversation

DiannaHohensee commented Aug 16, 2023 • edited Loading

DaveCTurner 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

Choose a reason for hiding this comment

DaveCTurner Aug 17, 2023 • edited Loading

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

DiannaHohensee 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

elasticsearchmachine commented Aug 21, 2023

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

idegtiarenko Aug 21, 2023 • edited Loading

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

elasticsearchmachine commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idegtiarenko left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Aug 23, 2023

DaveCTurner left a comment

Choose a reason for hiding this comment

DiannaHohensee commented Aug 16, 2023 •

edited

Loading

DaveCTurner Aug 17, 2023 •

edited

Loading

idegtiarenko Aug 21, 2023 •

edited

Loading