Adding a transport action to get cluster formation info #87306

masseyke · 2022-06-01T22:11:44Z

This PR exposes ClusterFormationFailureHelper.ClusterFormationState in a transport action (ClusterFormationInfoAction), with the intention that it be used as part of the master stability health check. This was started as #85782, but we realized that we needed more fine-grained information than just the string description of the reason the cluster formation failed.

This action will be used in branch 1.2.2.4 of the attached diagram (near the bottom right corner). That is, when we detect that there has been no master node in the last 30 seconds (default), and when PeerFinder does not believe that there is an elected master node, and when the current node is master eligible, we will use this transport action to asynchronously send a request to all master-eligible nodes to gather their ClusterFormationStates.

elasticsearchmachine · 2022-06-01T22:12:08Z

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

…sseyke/elasticsearch into feature/cluster-formation-info-action

elasticsearchmachine · 2022-06-14T17:11:29Z

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

…sseyke/elasticsearch into feature/cluster-formation-info-action

elasticmachine · 2022-06-14T18:17:00Z

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

masseyke · 2022-06-14T18:38:20Z

@elasticmachine update branch

andreidan

Thanks for working on this Keith

Left a few comments

andreidan · 2022-06-16T10:32:49Z

server/src/main/java/org/elasticsearch/cluster/coordination/ClusterFormationFailureHelper.java

+                in.readBoolean(),
+                readStatusInfo(in),
+                in.readList(
+                    in1 -> new JoinStatus(


could we have a more meaningful name for the inner reader?

... or maybe make JoinStatus readable from the wire too?

JoinStatus is now Writeable and readable from the wire.

andreidan · 2022-06-16T10:34:08Z

server/src/main/java/org/elasticsearch/cluster/coordination/ClusterFormationFailureHelper.java

+            );
+        }
+
+        private static StatusInfo readStatusInfo(StreamInput in) throws IOException {


Would it be worth making StatusInfo Writeable?

andreidan · 2022-06-16T10:43:40Z

...ain/java/org/elasticsearch/action/admin/cluster/coordination/ClusterFormationInfoAction.java

+    /**
+     * This transport action fetches the ClusterFormationState from a remote node.
+     */
+    public static class TransportAction extends HandledTransportAction<


See @DaveCTurner 's comment on the previous PR

https://github.com/elastic/elasticsearch/pull/85782/files#r849933849

This is good tho, it's just a simple HandledTransportAction here.

Ah of course - I just realised it does the needful

transportService.registerRequestHandler(actionName, executor, false, canTripCircuitBreaker, requestReader, new TransportHandler());

Sorry for the noise Keith

andreidan · 2022-06-16T10:45:58Z

...ain/java/org/elasticsearch/action/admin/cluster/coordination/ClusterFormationInfoAction.java

+
+    public static class Request extends ActionRequest {
+
+        public Request() {}


We'll have to request this information from all master eligible nodes so the request should be able to express that?

It's just a point-to-point request which I think makes sense as-is. The caller will have to do the work of sending one to each master of course, but that's ok.

Okido, sounds good

andreidan · 2022-06-16T10:53:02Z

server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java

@@ -284,7 +284,7 @@ public Coordinator(
        this.nodeHealthService = nodeHealthService;
    }

-    private ClusterFormationState getClusterFormationState() {
+    public ClusterFormationState getClusterFormationState() {


@DaveCTurner does it make sense to send this over if the discovery cluster formation timeout has not lapsed yet?

Yes I think so. If we only just lost the master then it might be a little incomplete but I expect it to converge pretty quickly.

andreidan · 2022-06-16T10:54:37Z

server/src/main/java/org/elasticsearch/cluster/coordination/ClusterFormationFailureHelper.java

+            );
+        }
+
+        private static boolean calculateHasDiscoveredQuorum(


nit: would it make sense to obey the isXXX/hasXXX naming convention for methods returning boolean?

In this case, we're trying to calculate the value to set hasDiscoveredQuorum to. I think if I named this hasDiscoveredQuorum() the expectation would be that this is a simple getter on that field. But it's not -- it's meant to be called just once to calculate what that field ought to be. Subsequent access would be through the record's access method to the hasDiscoveredQuorum field.

andreidan

LGTM, thanks for working on this Keith

andreidan · 2022-06-17T10:21:42Z

...ain/java/org/elasticsearch/action/admin/cluster/coordination/ClusterFormationInfoAction.java

+    /**
+     * This transport action fetches the ClusterFormationState from a remote node.
+     */
+    public static class TransportAction extends HandledTransportAction<


Ah of course - I just realised it does the needful

transportService.registerRequestHandler(actionName, executor, false, canTripCircuitBreaker, requestReader, new TransportHandler());

Sorry for the noise Keith

andreidan · 2022-06-17T10:38:15Z

server/src/main/java/org/elasticsearch/monitor/StatusInfo.java

+        out.writeOptionalString(status == null ? null : status.name());
+        out.writeOptionalString(info);


Are we changing the semantics for StatusInfo ? I don't think these fields are optional now. I think we should keep them non-null (ie. mandatory).

There was nothing preventing them from being null. I thought it was very unlikely that someone would pass in nulls (what would be the point?). But if they do, I don't want it to blow up during serialization. From what I can tell there's no good way to enforce null checks on a record. What do you think?

I think we shouldn't change the behaviour here.
Usually, we mark nullable fields with @Nullable or use Optional when we accept null, but neither is present on this object.

IMO we should enforce/relax semantics in a separate and deliberate effort.

I caught up with @dakrone on this and we were thinking of aligning the object to the requirements (ie. non-null fields) via Objects.requireNonNull calls in the constructor, alongside strict (non-optional) serialisation.

What do you think @masseyke ?

I'm all for enforcing non-nullable fields. I'll change the serialization to assume no nulls, and we can enforce it on a subsequent PR.

Adding a transport action to get cluster formation info

e440d95

masseyke added >feature :Data Management/Health v8.4.0 labels Jun 1, 2022

Update docs/changelog/87306.yaml

9370be3

masseyke mentioned this pull request Jun 1, 2022

Transport action for ClusterFormationFailureHelper warnings #85782

Closed

masseyke added 3 commits June 1, 2022 17:35

checkstyle

9dc64b1

Merge branch 'feature/cluster-formation-info-action' of github.com:ma…

bbf6c05

…sseyke/elasticsearch into feature/cluster-formation-info-action

Adding to Constants

cf56347

masseyke added >enhancement and removed >feature labels Jun 14, 2022

masseyke added 5 commits June 14, 2022 12:11

Update docs/changelog/87306.yaml

5d40b9e

Merge branch 'master' into feature/cluster-formation-info-action

0dc6ee1

cleaning up

82d03bd

Merge branch 'feature/cluster-formation-info-action' of github.com:ma…

92ca92c

…sseyke/elasticsearch into feature/cluster-formation-info-action

cleaning up

0c0ae34

masseyke marked this pull request as ready for review June 14, 2022 18:16

elasticmachine added the Team:Data Management Meta label for data/management team label Jun 14, 2022

masseyke requested review from andreidan and DaveCTurner June 14, 2022 18:17

masseyke mentioned this pull request Jun 14, 2022

Cluster coordination indicator - report if the master is stable and an impact/troubleshoot guide otherwise #85624

Closed

17 tasks

Merge branch 'master' into feature/cluster-formation-info-action

2039978

andreidan requested changes Jun 16, 2022

View reviewed changes

masseyke added 2 commits June 16, 2022 08:54

Making JoinStatus Writeable

dab27ac

Making StatusInfo Writeable

d2133be

masseyke requested a review from andreidan June 16, 2022 14:45

andreidan approved these changes Jun 17, 2022

View reviewed changes

not checking for nulls in serialization

c99a92c

masseyke merged commit 5061f90 into elastic:master Jun 20, 2022

masseyke deleted the feature/cluster-formation-info-action branch June 20, 2022 19:08

masseyke mentioned this pull request Jun 29, 2022

Adding logic to master_is_stable indicator to check for discovery problems #88020

Merged

masseyke mentioned this pull request Jul 11, 2022

Polling cluster formation state for master-is-stable health indicator #88397

Merged

masseyke mentioned this pull request Aug 9, 2022

Adding a check to the master stability health API when there is no master and the current node is not master eligible #89219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a transport action to get cluster formation info #87306

Adding a transport action to get cluster formation info #87306

masseyke commented Jun 1, 2022 •

edited

Loading

elasticsearchmachine commented Jun 1, 2022

elasticsearchmachine commented Jun 14, 2022

elasticmachine commented Jun 14, 2022

masseyke commented Jun 14, 2022

andreidan left a comment

andreidan Jun 16, 2022

DaveCTurner Jun 16, 2022

masseyke Jun 16, 2022

andreidan Jun 16, 2022

andreidan Jun 16, 2022

DaveCTurner Jun 16, 2022

andreidan Jun 17, 2022

andreidan Jun 16, 2022

DaveCTurner Jun 16, 2022

andreidan Jun 17, 2022

andreidan Jun 16, 2022

DaveCTurner Jun 16, 2022

andreidan Jun 16, 2022

masseyke Jun 16, 2022

andreidan left a comment

andreidan Jun 17, 2022

andreidan Jun 17, 2022

masseyke Jun 20, 2022

andreidan Jun 20, 2022

andreidan Jun 20, 2022

masseyke Jun 20, 2022


		public static class Request extends ActionRequest {

		public Request() {}

		out.writeOptionalString(status == null ? null : status.name());
		out.writeOptionalString(info);

Adding a transport action to get cluster formation info #87306

Adding a transport action to get cluster formation info #87306

Conversation

masseyke commented Jun 1, 2022 • edited Loading

elasticsearchmachine commented Jun 1, 2022

elasticsearchmachine commented Jun 14, 2022

elasticmachine commented Jun 14, 2022

masseyke commented Jun 14, 2022

andreidan 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

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

andreidan 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

masseyke commented Jun 1, 2022 •

edited

Loading