Correctly keep master in master history if it was active at the begin…

…ning of the master history period (elastic#86708)

This commit fixes a problem with the MasterHistory class. If we have had a single master for a week and then that
master went to null 5 seconds ago, we want to say that we have had a non-null master within the last 30 seconds.
However before this change, MasterHistory.hasSeenMasterInLastNSeconds(30) would return false in that case
because the non-null master had a start time much more than 30 seconds ago. This fix recognizes that a node is
master up until the start time of the master that replaces it. This required changing the methods that pruned the
master history based on time so that we do not prune masters that are in any way active during the time period.

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

@@ -23,7 +23,6 @@
 import java.util.Objects;
 import java.util.concurrent.TimeUnit;
 import java.util.function.LongSupplier;
-import java.util.stream.Collectors;
 
 /**
  * This class represents a node's view of the history of which nodes have been elected master over the last 30 minutes. It is kept in
@@ -77,7 +76,14 @@ public void clusterChanged(ClusterChangedEvent event) {
             int startIndex = Math.max(0, sizeAfterAddingNewMaster - MAX_HISTORY_SIZE);
             for (int i = startIndex; i < masterHistory.size(); i++) {
                 TimeAndMaster timeAndMaster = masterHistory.get(i);
-                if (timeAndMaster.startTimeMillis >= oldestRelevantHistoryTime) {
+                final long currentMasterEndTime;
+                if (i < masterHistory.size() - 1) {
+                    // We treat the start time of the next master as the end time of this current master
+                    currentMasterEndTime = masterHistory.get(i + 1).startTimeMillis;
+                } else {
+                    currentMasterEndTime = Long.MAX_VALUE; // This current master has no end time
+                }
+                if (currentMasterEndTime >= oldestRelevantHistoryTime) {
                     newMasterHistory.add(timeAndMaster);
                 }
             }
@@ -175,38 +181,65 @@ public static int getNumberOfMasterIdentityChanges(List<DiscoveryNode> masterHis
     }
 
     /**
-     * Returns true if a non-null master was seen at any point in the last nSeconds seconds, or if the last-seen master was more than
-     * nSeconds seconds ago and non-null.
+     * Returns true if a non-null master existed at any point in the last nSeconds seconds. Note that this could be a master whose start
+     * time was more than nSeconds ago, as long as either it is still master or the next master took over less than nSeconds ago.
      * @param nSeconds 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 nSeconds seconds
      */
     public boolean hasSeenMasterInLastNSeconds(int nSeconds) {
+        if (getMostRecentMaster() != null) {
+            return true;
+        }
         List<TimeAndMaster> masterHistoryCopy = getRecentMasterHistory(masterHistory);
         long now = currentTimeMillisSupplier.getAsLong();
         TimeValue nSecondsTimeValue = new TimeValue(nSeconds, TimeUnit.SECONDS);
         long nSecondsAgo = now - nSecondsTimeValue.getMillis();
-        return getMostRecentMaster() != null
-            || masterHistoryCopy.stream()
-                .filter(timeAndMaster -> timeAndMaster.master != null)
-                .anyMatch(timeAndMaster -> timeAndMaster.startTimeMillis > nSecondsAgo);
+
+        /*
+         * We traverse the list backwards (since it is ordered by time ascending). Once we find an entry whose
+         * timeAndMaster.startTimeMillis was more than nSeconds ago we can stop because it is not possible that any more of the nodes
+         * we'll see have ended within the last nSeconds.
+         */
+        for (int i = masterHistoryCopy.size() - 1; i >= 0; i--) {
+            TimeAndMaster timeAndMaster = masterHistoryCopy.get(i);
+            if (timeAndMaster.master != null) {
+                return true;
+            }
+            if (timeAndMaster.startTimeMillis < nSecondsAgo) {
+                break;
+            }
+        }
+        return false;
     }
 
     /*
-     * This method creates a copy of masterHistory that only has entries from more than maxHistoryAge before now (but leaves the newest
-     * entry in even if it is more than maxHistoryAge).
+     * This method creates a mutable copy of masterHistory that only has entries that have been active in the recent past
+     * (maxHistoryAge). In this case, "active" means that either the master's start time has been within maxHistoryAge, or the master was
+     *  replaced by another master within maxHistoryAge.
      */
     private List<TimeAndMaster> getRecentMasterHistory(List<TimeAndMaster> history) {
         if (history.size() < 2) {
             return history;
         }
         long now = currentTimeMillisSupplier.getAsLong();
         long oldestRelevantHistoryTime = now - maxHistoryAge.getMillis();
-        TimeAndMaster mostRecent = history.isEmpty() ? null : history.get(history.size() - 1);
-        List<TimeAndMaster> filteredHistory = history.stream()
-            .filter(timeAndMaster -> timeAndMaster.startTimeMillis > oldestRelevantHistoryTime)
-            .collect(Collectors.toList());
-        if (filteredHistory.isEmpty() && mostRecent != null) { // The most recent entry was more than 30 minutes ago
-            filteredHistory.add(mostRecent);
+
+        List<TimeAndMaster> filteredHistory = new ArrayList<>();
+        for (int i = 0; i < history.size(); i++) {
+            TimeAndMaster timeAndMaster = history.get(i);
+            final long endTime;
+            /*
+             * The end time of this timeAndMaster is the start time of the next one in the list. If there is no next one, this is the
+             * current timeAndMaster, so there is no endTime (so it is set to Long.MAX_VALUE).
+             */
+            if (i < history.size() - 1) {
+                endTime = history.get(i + 1).startTimeMillis;
+            } else {
+                endTime = Long.MAX_VALUE;
+            }
+            if (endTime >= oldestRelevantHistoryTime) {
+                filteredHistory.add(timeAndMaster);
+            }
         }
         return filteredHistory;
     }

server/src/test/java/org/elasticsearch/cluster/coordination/MasterHistoryTests.java

@@ -18,12 +18,14 @@
 import org.elasticsearch.cluster.routing.RoutingTable;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.threadpool.ThreadPool;
 import org.junit.Before;
 
 import java.net.UnknownHostException;
 import java.util.UUID;
+import java.util.concurrent.TimeUnit;
 
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.lessThanOrEqualTo;
@@ -113,6 +115,76 @@ public void testTime() {
         assertThat(masterHistory.getMostRecentMaster(), equalTo(node3MasterClusterState.nodes().getMasterNode()));
         when(threadPool.relativeTimeInMillis()).thenReturn(System.currentTimeMillis());
         assertThat(masterHistory.getMostRecentMaster(), equalTo(node3MasterClusterState.nodes().getMasterNode()));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, nullMasterClusterState, node3MasterClusterState));
+        assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, node1MasterClusterState, nullMasterClusterState));
+    }
+
+    public void testHasSeenMasterInLastNSeconds() {
+        var clusterService = mock(ClusterService.class);
+        when(clusterService.getSettings()).thenReturn(Settings.EMPTY);
+        ThreadPool threadPool = mock(ThreadPool.class);
+        MasterHistory masterHistory = new MasterHistory(threadPool, clusterService);
+
+        /*
+         * 60 minutes ago we get these master changes:
+         * null -> node1 -> node2 -> node3
+         * Except for when only null had been master, there has been a non-null master node in the last 5 seconds all along
+         */
+        long sixtyMinutesAgo = System.currentTimeMillis() - new TimeValue(60, TimeUnit.MINUTES).getMillis();
+        when(threadPool.relativeTimeInMillis()).thenReturn(sixtyMinutesAgo);
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, nullMasterClusterState, nullMasterClusterState));
+        assertFalse(masterHistory.hasSeenMasterInLastNSeconds(5));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, node1MasterClusterState, nullMasterClusterState));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, node2MasterClusterState, node1MasterClusterState));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, node3MasterClusterState, node2MasterClusterState));
+        assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
+
+        /*
+         * 40 minutes ago we get these master changes (the master was node3 when this section began):
+         * null -> node1 -> null -> null -> node1
+         * There has been a non-null master for the last 5 seconds every step at this time
+         */
+        long fourtyMinutesAgo = System.currentTimeMillis() - new TimeValue(40, TimeUnit.MINUTES).getMillis();
+        when(threadPool.relativeTimeInMillis()).thenReturn(fourtyMinutesAgo);
+        assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, nullMasterClusterState, node3MasterClusterState));
+        assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, node1MasterClusterState, nullMasterClusterState));
+        assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, nullMasterClusterState, node1MasterClusterState));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, nullMasterClusterState, nullMasterClusterState));
+        assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, node1MasterClusterState, nullMasterClusterState));
+        assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
+
+        /*
+         * 6 seconds ago we get these master changes (it had been set to node1 previously):
+         * null -> null
+         * Even though the last non-null master was more
+         * than 5 seconds ago (and more than the age of history we keep, 30 minutes), the transition from it to null was just now, so we
+         * still say that there has been a master recently.
+         */
+        long sixSecondsAgo = System.currentTimeMillis() - new TimeValue(6, TimeUnit.SECONDS).getMillis();
+        when(threadPool.relativeTimeInMillis()).thenReturn(sixSecondsAgo);
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, nullMasterClusterState, node1MasterClusterState));
+        assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, nullMasterClusterState, nullMasterClusterState));
+        assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
+
+        /*
+         * Right now we get these master changes (the master was null when this section began):
+         * null -> node1
+         * Even before the first transition to null, we have no longer seen a non-null master within the last 5 seconds (because we last
+         * transitioned from a non-null master 6 seconds ago). After the transition to node1, we again have seen a non-null master in the
+         *  last 5 seconds.
+         */
+        long now = System.currentTimeMillis();
+        when(threadPool.relativeTimeInMillis()).thenReturn(now);
+        assertFalse(masterHistory.hasSeenMasterInLastNSeconds(5));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, nullMasterClusterState, nullMasterClusterState));
+        assertFalse(masterHistory.hasSeenMasterInLastNSeconds(5));
+        masterHistory.clusterChanged(new ClusterChangedEvent(TEST_SOURCE, node1MasterClusterState, nullMasterClusterState));
         assertTrue(masterHistory.hasSeenMasterInLastNSeconds(5));
     }