Move the master stability logic into its own service separate from the HealthIndicatorService #87672

masseyke · 2022-06-14T22:32:18Z

For various reasons, we want the logic that decides whether we have had a stable master node separated from the logic that generates the HealthIndicatorResult (which is specific to the health API). This commit separates the logic of the master stability code from the HealthIndicatorService code. Most pressingly, we need to be able to call the stable master logic remotely from within StableMasterHistoryIndicatorService (when there has been no master recently, there is at least one master eligible node, none of those nodes are elected master, and the current node is not master-eligible). This commit splits StableMasterHistoryIndicatorService into StableMasterHistoryIndicatorService and StableMasterService. StableMasterService has a calculate method much like StableMasterHistoryIndicatorService's, but it returns a custom result with only stable-master-specific information. The job of StableMasterHistoryIndicatorService is just to transform this result into a HealthIndicatorResult.

The goal of this change is just to split the logic between StableMasterHistoryIndicatorService and StableMasterService. No externally-visible functionality is being added. A follow-up PR will make StableMasterService available through a transport action (which will include making the output of StableMasterService.calculate() serializable). And a follow-up PR to that will make use of that transport action (in the 1.2.2.3 branch of the diagram at #87482 (comment)).
Relates #85624

elasticsearchmachine · 2022-06-15T19:52:56Z

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

elasticmachine · 2022-06-16T18:55:09Z

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

andreidan

Thanks for working on this Keith.

I think this looks good, just left a few minor questions and suggestions about having the terminology more targeted towards a standalone service (that could be used outside of the health API context) and modularity

andreidan · 2022-06-17T12:44:31Z

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

+ * Since this service needs to be able to run when there is no master at all, it does not depend on the dedicated health node (which
+ * requires the existence of a master).
+ */
+public class StableMasterService implements ClusterStateListener {


As this would now be a standalone service, should we name this something a bit more descriptive (this service will look into discovery and cluster formation - and maybe expose some of those analyses via different public methods at some point, even though I do appreciate now it only has one public method)

Would ClusterAnalysisService/ CoordinationAnalysisService / CoordinationDiagnosticsService be a bit more descriptive?
With a method analyseMasterStability/ diagnoseMasterStability instead of calculate ?
In the same context, would StableMasterDetails be better named something along the lines of CoordinationAnalysisDetails?

These are all off-the-cuff suggestions (naming is, well, hard :) )

cc @DaveCTurner

Yeah I haven't thought of any great names. MasterStabilityAndCoordinationService?

My preference would be towards CoordinationDiagnosticsService as "CoordinationService" and the likes sort of point to this service being the one that handles the cluster coordination (which is what Coordinator does)

Sounds as good as anything. I'll go with that unless someone comes up with something better.

andreidan · 2022-06-17T12:48:34Z

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

+import org.elasticsearch.common.settings.Setting;
+import org.elasticsearch.core.Nullable;
+import org.elasticsearch.core.TimeValue;
+import org.elasticsearch.health.HealthStatus;


This creates a circular dependency - the cluster.coordination package depends on the healthpackage and health depends on cluster.coordination.

I think we should either have a status entity in cluster.coordination and interpret it when this service is consumed, or don't have a status at all but a different way to express the result of this service - a diagnostic /analysis of sorts

Oh good catch. I'll fix that.

andreidan · 2022-06-17T12:50:57Z

server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java

+        StableMasterService.IDENTITY_CHANGES_THRESHOLD_SETTING,
+        StableMasterService.NO_MASTER_TRANSITIONS_THRESHOLD_SETTING,
+        StableMasterService.NODE_HAS_MASTER_LOOKUP_TIMEFRAME_SETTING,


Should we document these settings? (separate body of work - was just wondering if we should work on documenting these as expert-level settings)

I think we ought to. I've been trying to figure out where would be a good place to document these without cluttering more important documentation. Have any ideas? Maybe a settings page for the whole health API?

Shall we have a page under https://www.elastic.co/guide/en/elasticsearch/reference/current/settings.html that's called something around what this service will end up being called?
ie. cluster diagnostics settings?

These settings will need some big warnings like here

I've added an item on #85624 for this work

andreidan

LGTM, thanks for working on this Keith 🚀

andreidan · 2022-06-21T15:23:50Z

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

+     * @param explain Whether to calculate and include the details in the result
+     * @return The StableMasterResult for the given localMasterHistory
+     */
+    private StableMasterResult diagnoseOnHaveNotSeenMasterRecently(MasterHistory localMasterHistory, boolean explain) {


nit: should we name this Diagnostic or something along those lines?

andreidan · 2022-06-21T15:25:06Z

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

+        RED;
+    }
+
+    public record StableMasterDetails(


nit: should we name this DiagnosticDetails?

Oops, yeah. I thought I'd caught all of these.

Ohh for some reason intellij refactoring is silently failing on these! So I only thought I'd changed them!

andreidan · 2022-06-21T15:25:42Z

server/src/main/java/org/elasticsearch/health/HealthStatus.java

        return name().toLowerCase(Locale.ROOT);
    }
+
+    public static HealthStatus fromStableMasterStatus(


nit: should we name this fromDiagnosticStatus ?

Oops, yeah. I thought I'd caught all of these.

andreidan · 2022-06-21T15:29:42Z

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

+     * @param remoteHistoryException An exception that was found when retrieving the remote master history. Can be null
+     * @return The StableMasterDetails
+     */
+    private StableMasterDetails getStableMasterDetailsOnMasterHasFlappedNull(


nit: would "getDetailsOnMasterHasFlappedNull` be a bit more concise?

jbaiera

LGTM, pending the last few name changes left

jbaiera · 2022-06-21T17:55:20Z

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

+        }
+    }
+
+    public record StableMasterResult(CoordinationDiagnosticsStatus status, String summary, StableMasterDetails details) {}


Should this also be CoordinationDiagonosticResult?

Yeah, intellij refactoring failed silently. Search and replace worked though!

…87984) This exposes the CoordinationDiagnosticsService (#87672) through a transport action so that it can be called remotely as part of the health API in the event that: (1) there has been no master recently, (2) there are master-eligible nodes in the cluster, (3) none are elected, and (4) the current node is not master eligible.

masseyke added 2 commits June 14, 2022 17:29

Moving master stability into its own service

57b39f2

Moving master stability into its own service

c48bc49

elasticsearchmachine added the v8.4.0 label Jun 14, 2022

masseyke added 2 commits June 15, 2022 13:07

cleaning up / documenting

1a4013e

Splitting tests

d5cf968

masseyke added >enhancement :Data Management/Health labels Jun 15, 2022

masseyke added 2 commits June 15, 2022 14:52

Update docs/changelog/87672.yaml

9a82471

merging master

e64217e

masseyke requested review from andreidan and jbaiera June 16, 2022 18:54

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

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

removing pointless method

4a61043

andreidan reviewed Jun 17, 2022

View reviewed changes

masseyke added 2 commits June 20, 2022 12:45

code review feedback

db033e3

renaming StableMasterService

33f996d

masseyke requested a review from andreidan June 21, 2022 14:50

andreidan approved these changes Jun 21, 2022

View reviewed changes

jbaiera approved these changes Jun 21, 2022

View reviewed changes

masseyke added 3 commits June 21, 2022 13:13

renaming StableMasterService

394fd39

code review feedback

73fa048

Merge branch 'master' into feature/make-stable-master-service

31f0f55

masseyke merged commit 214bd60 into elastic:master Jun 21, 2022

masseyke deleted the feature/make-stable-master-service branch June 21, 2022 20:12

masseyke mentioned this pull request Jun 23, 2022

Creating a transport action for the CoordinationDiagnosticsService #87984

Merged

Move the master stability logic into its own service separate from the HealthIndicatorService #87672

Move the master stability logic into its own service separate from the HealthIndicatorService #87672

Uh oh!

Conversation

masseyke commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 15, 2022

Uh oh!

elasticmachine commented Jun 16, 2022

Uh oh!

andreidan 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!

andreidan Jun 21, 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!

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!

andreidan Jun 17, 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!

Choose a reason for hiding this comment

Uh oh!

andreidan Jun 21, 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!

andreidan 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!

Choose a reason for hiding this comment

Uh oh!

jbaiera 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

masseyke commented Jun 14, 2022 •

edited

Loading

andreidan Jun 21, 2022 •

edited

Loading

andreidan Jun 17, 2022 •

edited

Loading

andreidan Jun 21, 2022 •

edited

Loading