Keep track of desired nodes cluster membership #84165

fcofdez · 2022-02-18T14:35:14Z

This commit adds tracking for desired nodes cluster membership.

When desired nodes are updated they are matched against the current
cluster members. Additionally when a node joins the cluster the
desired nodes cluster membership is updated.

elasticmachine · 2022-02-18T14:35:16Z

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

elasticsearchmachine · 2022-02-18T14:35:39Z

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

fcofdez · 2022-02-21T08:51:18Z

...a/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesAction.java

@@ -140,8 +140,13 @@ static ClusterState updateDesiredNodes(ClusterState currentState, UpdateDesiredN
            }
        }

+        DesiredNodesMetadata.Builder desiredNodesMetadataBuilder = new DesiredNodesMetadata.Builder(proposedDesiredNodes);


I think we should take into account previous desired nodes marked as members. I'll fix it.

Tackled in 42d504a

henningandersen

Did not get through it all, but wanted to relay a few early comments here.

henningandersen · 2022-02-21T15:57:55Z

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

-        history_id: "test"
-        version: 2
-        nodes:
-          - { settings: { node: { name: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version }


I think the new contains assertion no longer catches if there are too many desired nodes, perhaps we can add a check that there are only 2?

It looks like we are trying to preserve order anyway, so perhaps we could even keep matching against the list?

henningandersen · 2022-02-21T16:06:45Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodes.java

+        final String historyId = in.readString();
+        final long version = in.readLong();
+        if (in.getVersion().onOrAfter(Version.CURRENT)) {
+            return new DesiredNodes(historyId, version, in.readMap(StreamInput::readString, DesiredNode::new));


I think we should preserve order here too? Or perhaps even switch to an ordered map (TreeeMap) to ensure a consistent ordering?

I wonder if we even need to change the serialization here? I think we are sending the externalId twice now and might as well send it as a list and then reconstruct it on the receiver side?

I was thinking about that too. I would expect that the number of desired nodes would be fairly low in most cases. Let me revert this change.

henningandersen · 2022-02-21T16:10:09Z

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

+  - contains: { nodes: { settings: { node: { external_id: "instance-000187" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } }
+  - contains: { nodes: { settings: { node: { external_id: "instance-000188" } }, processors: 8, memory: "64gb", storage: "128gb", node_version: $es_version } }
+---
+"Test update desired nodes is idempotent with different order":


👍 , but would be nice to even split this out into a separate PR? Or is the change too coupled with the rest of this PR?

Sure, let me split this PR 👍

I opened a new PR #84227 with these changes and removed them from this PR. ~~I think is better to hold this PR review until we get that one merged.~~

elasticsearchmachine · 2022-02-22T15:14:29Z

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

fcofdez · 2022-02-24T16:09:45Z

~~I'm moving this to draft since I found a bug while I was working in a related PR~~

This commit adds tracking for desired nodes cluster membership. When desired nodes are updated they are matched against the current cluster members. Additionally when a node joins the cluster the desired nodes cluster membership is updated.

elasticsearchmachine · 2022-02-25T17:36:45Z

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

henningandersen

Sorry for the delay here. I wonder if we should make cluster membership a dynamically calculated property instead rather than persist it into metadata? For instance the current PR will IIUC keep all nodes as members if all nodes are abruptly shutdown and only some are started? Also at a higher level, I think knowledge about the current nodes in the cluster should not go into metadata to avoid persisting such information.

I can see a potential need for whether we ever saw a desired node, which would need to be in metadata. But I think the purpose here is different?

Left a couple random comments, not too important at this time.

henningandersen · 2022-03-20T13:06:50Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMetadata.java

    }

    private final DesiredNodes latestDesiredNodes;
+    private final Set<DesiredNode> clusterMembers;
+    private final Set<DesiredNode> notClusterMembers;

    public DesiredNodesMetadata(DesiredNodes latestDesiredNodes) {


I think we should make this private now that we have the Builder?

henningandersen · 2022-03-20T13:08:32Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMetadata.java

+        private DesiredNodes desiredNodes;
+        private final Set<DesiredNode> clusterMembers;
+
+        public Builder(DesiredNodesMetadata desiredNodesMetadata) {


nit: We often add a builder() method to the outer class. I like that, since it make the client code slightly shorter and reads slightly better I think.

…rship

fcofdez · 2022-04-25T10:12:27Z

@henningandersen I've followed a different approach where the membership is tracked in-memory, let me know if you prefer this approach. Thanks!

henningandersen

Thanks, this directions looks fine. I am not sure we need the quarantining/member removal though, I think we should simply remove that.

henningandersen · 2022-04-29T11:25:15Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodes.java

@@ -34,7 +36,7 @@
 import static java.lang.String.format;
 import static org.elasticsearch.node.Node.NODE_EXTERNAL_ID_SETTING;

-public class DesiredNodes implements Writeable, ToXContentObject {
+public class DesiredNodes extends AbstractCollection<DesiredNode> implements Writeable, ToXContentObject {


I think most of the uses here can work with just Iterable and I prefer that to not add a bunch of methods to the DesiredNodes interface that does not work.

henningandersen · 2022-04-29T11:29:47Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMembershipTracker.java

+                for (DiscoveryNode removedNode : nodesDelta.removedNodes()) {
+                    final var desiredNode = desiredNodes.find(removedNode.getExternalId());
+                    moveToQuarantineIfMember(desiredNode);
+                }


I wonder if this is necessary. If we ever saw a desired node and it disappears, we expect it to reappear or a new desired nodes state to be set. Anything else is a bug in the orchestration?

From the data tier allocation decider, we start allocating shards to the cold node(s) once we see the first one. But in case a node then unexpectedly drops out, I think we would not want to start allocating them to less preferred tiers. That could potentially cause issues on higher tiers more than help. And it would sort of hide that there is an issue, since if you look at health and allocation, everything looks good.

henningandersen · 2022-04-29T11:39:52Z

server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java

@@ -37,6 +38,7 @@
 public class DiscoveryNode implements Writeable, ToXContentFragment {

    static final String COORDINATING_ONLY = "coordinating_only";
+    public static final Version EXTERNAL_ID_VERSION = Version.CURRENT;


I'd normally put in the specific version here during development to not have to remember to alter it before merging it.

henningandersen · 2022-04-29T11:47:24Z

server/src/test/java/org/elasticsearch/cluster/node/DiscoveryNodeTests.java

@@ -205,6 +207,7 @@ public void testDiscoveryNodeToXContent() {
        assertThat(topLevelMap.toString(), detailsMap.remove("name"), equalTo("test-name"));
        assertThat(topLevelMap.toString(), detailsMap.remove("ephemeral_id"), equalTo("test-ephemeral-id"));
        assertThat(topLevelMap.toString(), detailsMap.remove("transport_address"), equalTo(transportAddress.toString()));
+        assertThat(topLevelMap.toString(), detailsMap.remove("external_id"), withExternalId ? equalTo("test-external-id") : equalTo(""));


Should it not be test-name when not supplying external id?

henningandersen · 2022-04-29T11:49:20Z

server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodesMembershipTrackerTests.java

+    private DesiredNode newDesiredNode(String nodeName) {
+        return new DesiredNode(
+            Settings.builder().put(Node.NODE_NAME_SETTING.getKey(), nodeName).build(),
+            16,


Let us randomize this too?

henningandersen · 2022-04-29T12:05:53Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMembershipTracker.java

+import java.util.HashSet;
+import java.util.Set;
+
+public class DesiredNodesMembershipTracker implements ClusterStateListener {


I think this needs to be a service in order for various components to be able to ask it for the members? We can also turn it into a service when we add the first use of this.

…rship

henningandersen

A few more things around master failover and removing nodes.

henningandersen · 2022-04-29T15:01:14Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMembershipService.java

+                for (DiscoveryNode removedNode : nodesDelta.removedNodes()) {
+                    final var desiredNode = desiredNodes.find(removedNode.getExternalId());
+                    if (desiredNode != null) {
+                        members.remove(desiredNode);
+                    }
+                }


I think we should remove this. If we ever saw the node it is a member until a desired nodes update is made that removes it.

henningandersen · 2022-04-29T15:01:52Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMembershipService.java

+                    }
+                }
+
+                members.removeAll(unknownDesiredNodes);


Let us also remove this.

henningandersen · 2022-04-29T15:04:01Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMembershipService.java

+
+    @Override
+    public synchronized void clusterChanged(ClusterChangedEvent event) {
+        if (event.localNodeMaster()) {


I wonder if we should just maintain this always, regardless of who is master? That would ensure it is readily available on failover. Otherwise, I worry that we need another special case for the failover case, i.e., when node was not previously master, since we only consider the delta of nodes below.

Yes, I think it should be fine if we keep track of this in all nodes and as you mention it should simplify things

henningandersen · 2022-04-29T15:05:30Z

server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodesMembershipServiceTests.java

+        }
+    }
+
+    public void testMasterDemotionClearsMembers() {


I think we need a test demonstrating that a promoted master has the right set of members?

…rship

fcofdez · 2022-05-02T08:30:57Z

@elasticmachine run elasticsearch-ci/part-1
(It was a Gradle connectivity issue)

henningandersen

LGTM.

henningandersen · 2022-05-02T17:02:39Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMembershipService.java

+            final var previousDesiredNodes = DesiredNodes.latestFromClusterState(event.previousState());
+            if (previousDesiredNodes != null) {


Do we need this condition? Seems more expensive than just clearing members always?

henningandersen · 2022-05-02T17:06:27Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNodesMembershipService.java

+    }
+
+    // visible for testing
+    synchronized int trackedMembers() {


Suggested change

synchronized int trackedMembers() {

synchronized int trackedMembersCount() {

…rship

fcofdez · 2022-05-03T09:26:16Z

@elasticmachine run elasticsearch-ci/part-2
(Unrelated LDAP failure)

fcofdez · 2022-05-03T12:06:56Z

Thanks Henning!

This commit adds desired nodes status tracking to the cluster state. Previously status was tracked in-memory by DesiredNodesMembershipService this approach had certain limitations, and made the consumer code more complex. This takes a simpler approach to keep the status updated when the desired nodes are updated or when a new node joins, storing the status in the cluster state, this allows to consume that information easily where it is necessary. Additionally, this commit moves test code from depending directly of DesiredNodes which can be seen as an internal data structure to rely more on UpdateDesiredNodesRequest. Relates #84165

fcofdez added >enhancement :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.2.0 labels Feb 18, 2022

fcofdez mentioned this pull request Feb 18, 2022

Track cluster membership in desired nodes #83976

Closed

fcofdez commented Feb 21, 2022

View reviewed changes

fcofdez requested review from henningandersen and DaveCTurner February 21, 2022 10:29

henningandersen reviewed Feb 21, 2022

View reviewed changes

fcofdez force-pushed the desired-nodes-membership branch from 1446da3 to 66532d8 Compare February 22, 2022 15:13

fcofdez marked this pull request as draft February 24, 2022 15:59

fcofdez force-pushed the desired-nodes-membership branch from d9d76b8 to 35d68ef Compare February 25, 2022 15:17

Fix tests

399ce6c

fcofdez marked this pull request as ready for review February 25, 2022 17:36

Update docs/changelog/84165.yaml

4a7d347

henningandersen reviewed Mar 20, 2022

View reviewed changes

salvatore-campagna added v8.3.0 and removed v8.2.0 labels Mar 30, 2022

fcofdez added 6 commits April 13, 2022 12:07

Merge remote-tracking branch 'origin/master' into desired-nodes-membe…

155f38c

…rship

Revert changes

fe95277

Revert

59a7db6

Merge remote-tracking branch 'origin/master' into desired-nodes-membe…

cf84c0a

…rship

Keep track of desired nodes membership in memory

abecf79

Merge remote-tracking branch 'origin/master' into desired-nodes-membe…

4fbe09b

…rship

Merge remote-tracking branch 'origin/master' into desired-nodes-membe…

4815413

…rship

fcofdez requested a review from henningandersen April 25, 2022 10:11

henningandersen reviewed Apr 29, 2022

View reviewed changes

fcofdez added 2 commits April 29, 2022 14:47

Merge remote-tracking branch 'origin/master' into desired-nodes-membe…

2f14d89

…rship

Review comments

cf6269a

fcofdez requested a review from henningandersen April 29, 2022 14:30

henningandersen reviewed Apr 29, 2022

View reviewed changes

fcofdez added 3 commits April 29, 2022 18:25

Simplify

4f2a0b4

Take into account more cases

093e1b9

Merge remote-tracking branch 'origin/master' into desired-nodes-membe…

a186cfc

…rship

fcofdez requested a review from henningandersen May 2, 2022 14:54

henningandersen approved these changes May 2, 2022

View reviewed changes

fcofdez added 2 commits May 3, 2022 10:30

nits

902776b

Merge remote-tracking branch 'origin/master' into desired-nodes-membe…

0cf892b

…rship

fcofdez merged commit ce9819f into elastic:master May 3, 2022

fcofdez mentioned this pull request Jun 8, 2022

Keep track of desired nodes status in cluster state #87474

Merged

		final var previousDesiredNodes = DesiredNodes.latestFromClusterState(event.previousState());
		if (previousDesiredNodes != null) {

	synchronized int trackedMembers() {
	synchronized int trackedMembersCount() {

Keep track of desired nodes cluster membership #84165

Keep track of desired nodes cluster membership #84165

Uh oh!

Conversation

fcofdez commented Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 18, 2022

Uh oh!

elasticsearchmachine commented Feb 18, 2022

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!

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!

fcofdez Feb 22, 2022 • 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!

elasticsearchmachine commented Feb 22, 2022

Uh oh!

fcofdez commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 25, 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!

fcofdez commented Apr 25, 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!

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!

fcofdez commented May 2, 2022

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

fcofdez commented Feb 18, 2022 •

edited

Loading

fcofdez Feb 22, 2022 •

edited

Loading

fcofdez commented Feb 24, 2022 •

edited

Loading