Add a simple maintenance state transition constraint for nodes in default space

Avoids transitioning an already up/init/retired node to maintenance mode
when global merges are pending, but allows transitions when the node
is considered down/maintenance in the already published state.

Not stateful, so triggers false positives on cluster controller restart edges.

6 files changed, 133 insertions, 14 deletions

diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetController.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetController.java
index e97690d1105..9f570cc0d20 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetController.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetController.java
@@ -824,7 +824,8 @@ public class FleetController implements NodeStateOrHostInfoChangeHandler, NodeAd
     }
 
     private ClusterStateDeriver createBucketSpaceStateDeriver() {
-        return new MaintenanceWhenPendingGlobalMerges(stateVersionTracker.createMergePendingChecker());
+        return new MaintenanceWhenPendingGlobalMerges(stateVersionTracker.createMergePendingChecker(),
+                UpEdgeMaintenanceTransitionConstraint.forPreviouslyPublishedState(stateVersionTracker.getVersionedClusterState()));
     }
 
     /**
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/MaintenanceTransitionConstraint.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/MaintenanceTransitionConstraint.java
new file mode 100644
index 00000000000..4da6c272660
--- /dev/null
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/MaintenanceTransitionConstraint.java
@@ -0,0 +1,15 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.clustercontroller.core;
+
+/**
+ * Implementations of this interface add constraints to when a node with
+ * pending global merges may be implicitly transitioned to Maintenance
+ * state in the default bucket space. This is to avoid flip-flopping nodes
+ * between being available and in maintenance when merge statistics
+ * change in a running system.
+ */
+public interface MaintenanceTransitionConstraint {
+
+    boolean maintenanceTransitionAllowed(int contentNodeIndex);
+
+}
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/MaintenanceWhenPendingGlobalMerges.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/MaintenanceWhenPendingGlobalMerges.java
index 3af789c41d4..22f9b7436cc 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/MaintenanceWhenPendingGlobalMerges.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/MaintenanceWhenPendingGlobalMerges.java
@@ -8,6 +8,7 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
  * Cluster state deriver which checks if nodes have merges pending for globally
@@ -24,9 +25,12 @@ public class MaintenanceWhenPendingGlobalMerges implements ClusterStateDeriver {
     private static final String bucketSpaceToDerive = FixedBucketSpaces.defaultSpace();
 
     private final MergePendingChecker mergePendingChecker;
+    private final MaintenanceTransitionConstraint maintenanceTransitionConstraint;
 
-    public MaintenanceWhenPendingGlobalMerges(MergePendingChecker mergePendingChecker) {
+    public MaintenanceWhenPendingGlobalMerges(MergePendingChecker mergePendingChecker,
+                                              MaintenanceTransitionConstraint maintenanceTransitionConstraint) {
         this.mergePendingChecker = mergePendingChecker;
+        this.maintenanceTransitionConstraint = maintenanceTransitionConstraint;
     }
 
     @Override
@@ -34,25 +38,26 @@ public class MaintenanceWhenPendingGlobalMerges implements ClusterStateDeriver {
         if (!bucketSpace.equals(bucketSpaceToDerive)) {
             return baselineState.clone();
         }
-        Set<Integer> incompleteNodeIndices = nodesWithMergesNotDone(baselineState.getClusterState());
+        Set<Integer> incompleteNodeIndices = availableContentNodes(baselineState.getClusterState()).stream()
+                .filter(nodeIndex -> mayHaveMergesPending(bucketSpaceToCheck, nodeIndex))
+                .filter(maintenanceTransitionConstraint::maintenanceTransitionAllowed)
+                .collect(Collectors.toSet());
+
         if (incompleteNodeIndices.isEmpty()) {
             return baselineState.clone();
         }
         return setNodesInMaintenance(baselineState, incompleteNodeIndices);
     }
 
-    private Set<Integer> nodesWithMergesNotDone(ClusterState baselineState) {
-        final Set<Integer> incompleteNodes = new HashSet<>();
+    private static Set<Integer> availableContentNodes(ClusterState baselineState) {
+        final Set<Integer> availableNodes = new HashSet<>();
         final int nodeCount = baselineState.getNodeCount(NodeType.STORAGE);
         for (int nodeIndex = 0; nodeIndex < nodeCount; ++nodeIndex) {
-            // FIXME should only set nodes into maintenance if they've not yet been up in the cluster
-            // state since they came back as Reported state Up!
-            // Must be implemented before this state deriver is enabled in production.
-            if (contentNodeIsAvailable(baselineState, nodeIndex) && mayHaveMergesPending(bucketSpaceToCheck, nodeIndex)) {
-                incompleteNodes.add(nodeIndex);
+            if (contentNodeIsAvailable(baselineState, nodeIndex)) {
+                availableNodes.add(nodeIndex);
             }
         }
-        return incompleteNodes;
+        return availableNodes;
     }
 
     private AnnotatedClusterState setNodesInMaintenance(AnnotatedClusterState baselineState,
@@ -69,7 +74,7 @@ public class MaintenanceWhenPendingGlobalMerges implements ClusterStateDeriver {
                 nodeStateReasons);
     }
 
-    private boolean contentNodeIsAvailable(ClusterState state, int nodeIndex) {
+    private static boolean contentNodeIsAvailable(ClusterState state, int nodeIndex) {
         return state.getNodeState(Node.ofStorage(nodeIndex)).getState().oneOf("uir");
     }
 
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/UpEdgeMaintenanceTransitionConstraint.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/UpEdgeMaintenanceTransitionConstraint.java
new file mode 100644
index 00000000000..35cfb76f0ce
--- /dev/null
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/UpEdgeMaintenanceTransitionConstraint.java
@@ -0,0 +1,37 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.clustercontroller.core;
+
+import com.yahoo.vdslib.state.ClusterState;
+import com.yahoo.vdslib.state.Node;
+import com.yahoo.vdslib.state.NodeState;
+
+/**
+ * Simple implementation which considers a node eligible for being set to maintenance iff
+ * it was down or in maintenance in the cluster state previously published. This avoids
+ * taking down nodes with pending global merges if these have already passed through an
+ * up-edge, but runs the risk of taking down a large set of nodes when the cluster
+ * controller is restarted (due to not knowing the previously published state).
+ *
+ * Output from this checker must always be combined with other constraints to decide whether
+ * a node should be in maintenance; used alone it would most certainly cause havoc.
+ *
+ * Considered sufficient for beta testing use cases.
+ */
+public class UpEdgeMaintenanceTransitionConstraint implements MaintenanceTransitionConstraint {
+
+    private final ClusterState previouslyPublishedDerivedState;
+
+    private UpEdgeMaintenanceTransitionConstraint(ClusterState previouslyPublishedDerivedState) {
+        this.previouslyPublishedDerivedState = previouslyPublishedDerivedState;
+    }
+
+    public static UpEdgeMaintenanceTransitionConstraint forPreviouslyPublishedState(ClusterState state) {
+        return new UpEdgeMaintenanceTransitionConstraint(state);
+    }
+
+    @Override
+    public boolean maintenanceTransitionAllowed(int contentNodeIndex) {
+        NodeState prevState = previouslyPublishedDerivedState.getNodeState(Node.ofStorage(contentNodeIndex));
+        return prevState.getState().oneOf("dm");
+    }
+}
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MaintenanceWhenPendingGlobalMergesTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MaintenanceWhenPendingGlobalMergesTest.java
index a2e661aa161..3b052a1bcb2 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MaintenanceWhenPendingGlobalMergesTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MaintenanceWhenPendingGlobalMergesTest.java
@@ -19,8 +19,13 @@ import static org.mockito.Mockito.when;
 public class MaintenanceWhenPendingGlobalMergesTest {
 
     private static class Fixture {
-        public MergePendingChecker mockPendingChecker = mock(MergePendingChecker.class);
-        public MaintenanceWhenPendingGlobalMerges deriver = new MaintenanceWhenPendingGlobalMerges(mockPendingChecker);
+        MergePendingChecker mockPendingChecker = mock(MergePendingChecker.class);
+        MaintenanceTransitionConstraint mockEligibilityChecker = mock(MaintenanceTransitionConstraint.class);
+        MaintenanceWhenPendingGlobalMerges deriver = new MaintenanceWhenPendingGlobalMerges(mockPendingChecker, mockEligibilityChecker);
+
+        Fixture() {
+            when(mockEligibilityChecker.maintenanceTransitionAllowed(anyInt())).thenReturn(true);
+        }
     }
 
     private static String defaultSpace() {
@@ -102,4 +107,14 @@ public class MaintenanceWhenPendingGlobalMergesTest {
                 .reason(NODE_TOO_UNSTABLE, 2).build()));
     }
 
+    @Test
+    public void node_with_pending_merges_only_set_to_maintenance_if_eligible() {
+        Fixture f = new Fixture();
+        Arrays.asList(1, 2, 3).forEach(idx -> when(f.mockPendingChecker.mayHaveMergesPending(globalSpace(), idx)).thenReturn(true));
+        Arrays.asList(1, 2, 4).forEach(idx -> when(f.mockEligibilityChecker.maintenanceTransitionAllowed(idx)).thenReturn(false));
+        AnnotatedClusterState derived = f.deriver.derivedFrom(stateFromString("distributor:5 storage:5"), defaultSpace());
+        assertThat(derived, equalTo(AnnotatedClusterStateBuilder.ofState("distributor:5 storage:5 .3.s:m")
+                .reason(MAY_HAVE_MERGES_PENDING, 3).build()));
+    }
+
 }
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/UpEdgeMaintenanceTransitionConstraintTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/UpEdgeMaintenanceTransitionConstraintTest.java
new file mode 100644
index 00000000000..86cd0d8f288
--- /dev/null
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/UpEdgeMaintenanceTransitionConstraintTest.java
@@ -0,0 +1,46 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.clustercontroller.core;
+
+import com.yahoo.vdslib.state.ClusterState;
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class UpEdgeMaintenanceTransitionConstraintTest {
+
+    private static UpEdgeMaintenanceTransitionConstraint makeChecker(String state) {
+        return UpEdgeMaintenanceTransitionConstraint.forPreviouslyPublishedState(ClusterState.stateFromString(state));
+    }
+
+    private static boolean nodeMayTransitionToMaintenanceInState(int contentNodeIndex, String state) {
+        UpEdgeMaintenanceTransitionConstraint checker = makeChecker(state);
+        return checker.maintenanceTransitionAllowed(contentNodeIndex);
+    }
+
+    @Test
+    public void eligible_when_previous_state_is_down() {
+        assertTrue(nodeMayTransitionToMaintenanceInState(1, "distributor:5 storage:5 .1.s:d"));
+    }
+
+    @Test
+    public void eligible_when_previous_state_is_maintenance() {
+        assertTrue(nodeMayTransitionToMaintenanceInState(1, "distributor:5 storage:5 .1.s:m"));
+    }
+
+    @Test
+    public void not_eligible_when_previous_state_is_up() {
+        assertFalse(nodeMayTransitionToMaintenanceInState(0, "distributor:5 storage:5"));
+    }
+
+    @Test
+    public void not_eligible_when_previous_state_is_initializing() {
+        assertFalse(nodeMayTransitionToMaintenanceInState(0, "distributor:5 storage:5 .0.s:i"));
+    }
+
+    @Test
+    public void not_eligible_when_previous_state_is_retired() {
+        assertFalse(nodeMayTransitionToMaintenanceInState(0, "distributor:5 storage:5 .0.s:r"));
+    }
+
+}