Adding a view of master history #85941

masseyke · 2022-04-15T21:10:15Z

This commit adds the notion of an in-memory MasterHistory of which nodes have been master for the last 30 minutes that is maintained in memory on each node. It is exposed via the MasterHistoryService. This commit also has a transport action so that you can fetch the master history from any node in the cluster, represented as a List of NodeClients. The list is an ordered list of nodes that have been seen as master for the last 30 minutes, with the oldest first. This action is used by the MasterHistoryService exposed for use via the MasterHistoryService. The local and remote master history representations will be used to determine if the master has been stable as part of the health API.

elasticmachine · 2022-04-15T21:10:18Z

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

elasticsearchmachine · 2022-04-15T21:10:39Z

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

masseyke · 2022-04-15T21:12:10Z

@elasticmachine update branch

masseyke · 2022-04-15T21:16:31Z

This is part of #85624

…i-master-stability-check

andreidan

Thanks for working on this Keith

I left a first round of suggestions/comments

andreidan · 2022-04-19T10:38:31Z

...r/src/main/java/org/elasticsearch/action/admin/cluster/coordination/MasterHistoryAction.java

+        super(NAME, MasterHistoryAction.Response::new);
+    }
+
+    public static class Request extends MasterNodeReadRequest<MasterHistoryAction.Request> {


I believe this could be a ActionRequest and receive a DiscoveryNode as a parameter (just to be sure we're getting the history from the node we want - if there's a master change in the meantime we might be getting the history from another node by the time we submit this as there's a new master) https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/action/cache/FrozenCacheInfoNodeAction.java is a good example

cc @DaveCTurner

More generally I'd suggest adding the code that uses this machinery to this same PR. I think there will be other opportunities to simplify things and perhaps change the message flow but it's hard to see what's needed without the calling code too.

(Acking that this'll make the PR bigger, and we might want to split it up again later, but for now I'd rather see everything)

(Acking that this'll make the PR bigger, and we might want to split it up again later, but for now I'd rather see everything)

Ha yeah originally the calling code was in this PR and I pulled it out. I'll put it back in for now.

I added StableMasterHealthIndicatorService.java to show the possible usage of MasterHistoryService. I'll delete it from this PR before merging.

I believe this could be a ActionRequest

Oops I had originally incorrectly had this as a TransportMasterNodeReadAction and forgot to fix the Request when I fixed the rest. Fixing that now.

and receive a DiscoveryNode as a parameter

I don't think I'm following you here. You mean as an argument to its constructor rather than calling remoteAddress() directly? Or something else?

andreidan · 2022-04-19T10:39:31Z

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

+ */
+public class MasterHistory implements ClusterStateListener, Writeable, Writeable.Reader<MasterHistory> {
+    private List<TimeAndMaster> masterHistory;
+    Supplier<Long> nowSupplier = System::currentTimeMillis; // Can be changed for testing


Since we're working with "things that happened since / in the last X minutes/seconds" we should use nanoTime here as it's monotonically increasing

I recommend ThreadPool#relativeTimeInMillis which is monotonic but easily faked in tests.

I actually think what I did isn't going to work at all the way I've done it if we don't have somewhat-in-sync times across boxes. The reason is the hasSeenMasterInLastNSeconds method. If the times are all only meaningful on the current machine then there's no way to know if we've seen a master in the last 30 seconds on a remote machine, unless we calculate that before we send it over the wire. Might need to rethink this one.

Would it be better to have each of these methods be its own remote call rather than returning a MasterHistory object? I'm stuck on how to answer hasSeenMasterInLastNSeconds without it being its own call. And even then, it could easily take more than 30 seconds to respond, so the answer would be void before we even got it.

if we don't have somewhat-in-sync times across boxes

This is an invariant we have to work with (without specialised hardware we can't do much about it)

I don't completely understand why you'd need to measure and compare time across machines though? Can you maybe explain that?

Machine 1 - computes its local history (ie. masters seen in last X seconds in relation to an origin chosen on machine 1)
Machine 2 - computes its own local history (ie. masters seen in the last X seconds in relation to an origin chosen on machine 2)

Machine 1 might ask Machine 2 about its history, in which case Machine 2 will reply with the history it has in its local "cache" for the last X seconds, according to the origin chosen on Machine 2 (no comparison to Machine 1 time/origin). Machine 1 will receive this history and if it contains repeated changes or master going null/not-null Machine 1 will be able to conclude that Machine 2 is an unstable master (again, Master 1 will work with the entire history it received from Machine 2, without comparing the times to its local seen masters, nor truncating/purging it in any way).

The reason is that in machine 2's history there is no notion of "now". There are just a few events with timestamps that are meaningless to machine 1. Machine 1 can't tell if the newest even was 2 seconds or 2 years ago. I'll change MasterHistory to ship some notion of "now" so that i can answer questions like "has there been a master in the last 30 seconds". I won't need to expose what "now" is -- it'll just be some anchor to answer relative questions like that.

The reason is that in machine 2's history there is no notion of "now".

There is, but it's relative (and local) to Machine 2.

Machine 1 can't tell if the newest even was 2 seconds or 2 years ago.

Maybe I'm missing something but each machine will only hold 30 minutes' worth of history AFAIK

There is, but it's relative (and local) to Machine 2.

Right, but we're talking about Machine 2's history on Machine 1.

Maybe I'm missing something but each machine will only hold 30 minutes' worth of history AFAIK

Yeah it's not an issue for the questions about what happened in the last 30 minutes. It's an issue for the question about what happened on Machine 2 in the last 30 seconds (as seen on machine 1). That's part of the MasterHistory API, but we don't actually have any use case where we need to answer that for a remote machine. Maybe I'll break out an interface for a LocalMasterHistory vs a RemoteMasterHhistory, and only have that method in the Local one. I could also only have the local one be a ClusterChangeEventListener, which would simplify things.

OK I broke it into a MutableMasterHistory that is updated live on the local node, and an ImmutableMasterHistory that is never updated but that can be passed from one node to another.

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

andreidan · 2022-04-19T10:48:19Z

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

+        if (currentMaster == null || currentMaster.equals(previousMaster) == false || masterHistory.isEmpty()) {
+            masterHistory.add(new TimeAndMaster(nowSupplier.get(), currentMaster));
+        }
+        removeOldMasterHistory();


As this is called from the cluster change notified thread and potentially from a public method (ie. getMostRecentNonNullMaster) this poses a multi-thread safety issue.

We either choose a thread-safe collection to store the history (ie. CopyOnWriteArrayList ) or we just purge the old history in the clusterChanged call (ie. have one writer against the history List).

I believe the latter is a good candidate here. What do you think?

Good point! I forgot to come back to that. If we only purge in clusterChanged I'll need to change the read methods to ignore anything old, since we could probably go a very long time between a cluster changed event. But that would work.

I wound up protecting write access from multiple threads because I thought it would be easier to read the code (vs another check for all reads to ignore anything older than 30 minutes). Let me know what you think.

…tion

…eptions encountered while fetching remote history

masseyke · 2022-05-04T17:14:05Z

@elasticmachine update branch

…/elasticsearch into feature/health-api-master-check

DaveCTurner

LGTM (except for the indicator service which we'll work on in a separate PR).

.../test/java/org/elasticsearch/action/admin/cluster/coordination/MasterHistoryActionTests.java

andreidan

LGTM (no indicator though :) )

Thanks for iterating on this Keith ! 🚀

andreidan · 2022-04-29T14:45:36Z

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

+     * more than 30 minutes old). Rather than being scheduled, this method is called whenever the cluster state changes.
+     */
+    private void removeOldMasterHistory(List<TimeAndMaster> newMasterHistory) {
+        if (newMasterHistory.size() < 2) {


Would it be ok to document this locally? It's not immediately obvious IMO

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

andreidan · 2022-05-06T10:25:16Z

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

+        Collections.reverse(masterHistoryCopy);
+        for (TimeAndMaster timeAndMaster : masterHistoryCopy) {
+            if (timeAndMaster.master != null) {
+                return timeAndMaster.master;
+            }
+        }


nit: would iterating backwards avoid some cpu cycles and allocations?

Ha it used to be that way. See #85941 (comment).

andreidan · 2022-05-06T10:27:10Z

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

+    /**
+     * An identity change is when we get notified of a change to a non-null master that is different from the previous non-null master.
+     * Note that a master changes to null on (virtually) every identity change.
+     * So for example:
+     * node1 -> node2 is 1 identity change
+     * node1 -> node2 -> node1 is 2 identity changes
+     * node1 -> node2 -> node2 is 1 identity change (transitions from a node to itself do not count)
+     * node1 -> null -> node1 is 0 identity changes (transitions from a node to itself, even with null in the middle, do not count)
+     * node1 -> null -> node2 is 1 identity change
+     * @param masterHistory The list of nodes that have been master
+     * @return The number of master identity changes as defined above
+     */


YES! This is really nice ❤️ ! Thanks for these docs Keith

andreidan · 2022-05-06T10:28:26Z

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

+     * @param n The number of seconds to look back
+     * @return true if the current master is non-null or if a non-null master was seen in the last n seconds
+     */
+    public boolean hasSeenMasterInLastNSeconds(int n) {


Should we indicate the time value here ?

Suggested change

public boolean hasSeenMasterInLastNSeconds(int n) {

public boolean hasSeenMasterInLastSeconds(int numberOfSeconds) {

andreidan · 2022-05-06T10:31:49Z

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

+     * updated unless this method is called again.
+     * @param node The node whose view of the master history we want to fetch
+     */
+    public void requestRemoteMasterHistory(DiscoveryNode node) {


Sounds good to me.

What I was proposing before was something similar to what we do in the client https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java#L399

e.g.

public void requestRemoteMasterHistory(DiscoveryNode node, ActionListener<MasterHistoryAction.Response>)

I'm happy with the naming suggestion though. Thanks for iterating on this !

andreidan · 2022-05-06T10:33:19Z

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

+                                @Override
+                                public void onResponse(MasterHistoryAction.Response response) {
+                                    long endTime = System.nanoTime();
+                                    logger.trace("Received history from {} in {}", node, TimeValue.timeValueNanos(endTime - startTime));


Suggested change

logger.trace("Received history from {} in {}", node, TimeValue.timeValueNanos(endTime - startTime));

logger.trace("Received history from {} in {} nanos", node, TimeValue.timeValueNanos(endTime - startTime));

TimeValue.timeValueNanos() returns a TimeValue, and TimeValue's toString puts the units into the string. So no need to add " nanos" a second time.

andreidan · 2022-05-06T10:34:26Z

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

+            ConnectionProfile.buildDefaultConnectionProfile(clusterService.getSettings()),
+            new ActionListener<>() {
+                @Override
+                public void onResponse(Transport.Connection connection) {


This is very nice - TIL

masseyke added 5 commits April 12, 2022 15:35

Initial stable master health check

21ecee7

Adding more to the stable master health check

64184d5

Adding a master history service

d035147

removing health check

eb93a15

removing unneeded ToXContent

64465d9

masseyke added >feature :Data Management/Health v8.3.0 labels Apr 15, 2022

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

Update docs/changelog/85941.yaml

baa8dc6

masseyke changed the title ~~Feature/health api master check~~ Adding a view of master history Apr 15, 2022

Merge branch 'master' into feature/health-api-master-check

b3e4fc3

checkstyle

600c717

masseyke mentioned this pull request Apr 15, 2022

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

Closed

17 tasks

masseyke added 10 commits April 15, 2022 16:37

checkstyle

6affcac

Fixing action name

d341574

checkstyle

341b901

Adding master history to list of non-operator actions

2b96e50

Adding a master stability health api check

9772a18

Merge branch 'master' into feature/health-api-master-check

8137972

Merge branch 'feature/health-api-master-check' into feature/health-ap…

c39ae20

…i-master-stability-check

unit testing

69ba2fa

formatting fix

2723b30

Removing unused method

57fbffd

andreidan reviewed Apr 19, 2022

View reviewed changes

making master history threadsafe

5d2d136

masseyke added 9 commits May 3, 2022 08:27

Making TransportMasterHistoryAction an inner class of MasterHistoryAc…

e4e652b

…tion

code review feedback

b167205

Removing removeOldMasterHistory method

698fdce

limiting max size of history

dc1e80d

using settings for master stability values

01098c8

code review feedback

0169967

Only keeping one remote master history in memory, and dumping out exc…

2b35bf8

…eptions encountered while fetching remote history

Adding a version check before calling new transport action

fcd250e

checkstyle

0ff5d04

elasticmachine and others added 6 commits May 5, 2022 02:44

Merge branch 'master' into feature/health-api-master-check

61ec7b0

excluding nulls from the recent_masters list shown to users

efd0cbd

Merge branch 'feature/health-api-master-check' of github.com:masseyke…

6159114

…/elasticsearch into feature/health-api-master-check

Closing connection in ActionListener.runBefore

4aba393

improving summary

f641932

Adding to javadocs

3b697ea

masseyke requested a review from DaveCTurner May 4, 2022 20:44

masseyke added 3 commits May 4, 2022 15:45

Fixing summary

508d51a

Adding a timeout for the remote master history request

3894948

merging master

365a858

DaveCTurner reviewed May 6, 2022

View reviewed changes

.../test/java/org/elasticsearch/action/admin/cluster/coordination/MasterHistoryActionTests.java Show resolved Hide resolved

andreidan approved these changes May 6, 2022

View reviewed changes

masseyke added 4 commits May 6, 2022 09:02

Adding a response mutator to MasterHistoryActionTests

2e67ac7

Code review feedback

4f20633

Code review feedback

e031098

Removing StableMasterHealthIndicatorService classes

ee000ec

masseyke merged commit 4533c45 into elastic:master May 6, 2022

masseyke deleted the feature/health-api-master-check branch May 6, 2022 15:32

masseyke mentioned this pull request May 11, 2022

Master stability health indicator part 1 (when a master has been seen recently) #86524

Merged

masseyke mentioned this pull request Jul 10, 2023

Avoid timeout-and-retry in CoordinationDiagnosticsService and friends #97514

Open

	public boolean hasSeenMasterInLastNSeconds(int n) {
	public boolean hasSeenMasterInLastSeconds(int numberOfSeconds) {

	logger.trace("Received history from {} in {}", node, TimeValue.timeValueNanos(endTime - startTime));
	logger.trace("Received history from {} in {} nanos", node, TimeValue.timeValueNanos(endTime - startTime));

Adding a view of master history #85941

Adding a view of master history #85941

Uh oh!

Conversation

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

Uh oh!

Uh oh!

elasticmachine commented Apr 15, 2022

Uh oh!

elasticsearchmachine commented Apr 15, 2022

Uh oh!

masseyke commented Apr 15, 2022

Uh oh!

masseyke commented Apr 15, 2022

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

andreidan Apr 19, 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!

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!

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!

masseyke commented May 4, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

masseyke commented Apr 15, 2022 •

edited

Loading

andreidan Apr 19, 2022 •

edited

Loading