Fail safe maintenance if other nodes are not up

4 files changed, 52 insertions, 88 deletions

diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java
index 01ec8d65c28..1b78833480c 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java
@@ -203,8 +203,6 @@ public class ContentCluster {
             NodeState oldState, NodeState newState) {
 
         NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker(
-                minStorageNodesUp,
-                minRatioOfStorageNodesUp,
                 distribution.getRedundancy(),
                 new HierarchicalGroupVisitingAdapter(distribution),
                 clusterInfo
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java
index acddfa478b1..f3e7941fcd2 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeChecker.java
@@ -29,20 +29,14 @@ public class NodeStateChangeChecker {
     public static final String BUCKETS_METRIC_NAME = "vds.datastored.bucket_space.buckets_total";
     public static final Map<String, String> BUCKETS_METRIC_DIMENSIONS = Map.of("bucketSpace", "default");
 
-    private final int minStorageNodesUp;
-    private final double minRatioOfStorageNodesUp;
     private final int requiredRedundancy;
     private final HierarchicalGroupVisiting groupVisiting;
     private final ClusterInfo clusterInfo;
 
     public NodeStateChangeChecker(
-            int minStorageNodesUp,
-            double minRatioOfStorageNodesUp,
             int requiredRedundancy,
             HierarchicalGroupVisiting groupVisiting,
             ClusterInfo clusterInfo) {
-        this.minStorageNodesUp = minStorageNodesUp;
-        this.minRatioOfStorageNodesUp = minRatioOfStorageNodesUp;
         this.requiredRedundancy = requiredRedundancy;
         this.groupVisiting = groupVisiting;
         this.clusterInfo = clusterInfo;
@@ -159,9 +153,9 @@ public class NodeStateChangeChecker {
                     + currentState);
         }
 
-        Result thresholdCheckResult = checkUpThresholds(clusterState);
-        if (!thresholdCheckResult.settingWantedStateIsAllowed()) {
-            return thresholdCheckResult;
+        Result allNodesAreUpCheck = checkAllNodesAreUp(clusterState);
+        if (!allNodesAreUpCheck.settingWantedStateIsAllowed()) {
+            return allNodesAreUpCheck;
         }
 
         HostInfo hostInfo = nodeInfo.getHostInfo();
@@ -226,9 +220,9 @@ public class NodeStateChangeChecker {
             return Result.allowSettingOfWantedState();
         }
 
-        Result ongoingChanges = anyNodeSetToMaintenance(clusterState);
-        if (!ongoingChanges.settingWantedStateIsAllowed()) {
-            return ongoingChanges;
+        Result allNodesAreUpCheck = checkAllNodesAreUp(clusterState);
+        if (!allNodesAreUpCheck.settingWantedStateIsAllowed()) {
+            return allNodesAreUpCheck;
         }
 
         Result checkDistributorsResult = checkDistributors(nodeInfo.getNode(), clusterState.getVersion());
@@ -236,11 +230,6 @@ public class NodeStateChangeChecker {
             return checkDistributorsResult;
         }
 
-        Result thresholdCheckResult = checkUpThresholds(clusterState);
-        if (!thresholdCheckResult.settingWantedStateIsAllowed()) {
-            return thresholdCheckResult;
-        }
-
         return Result.allowSettingOfWantedState();
     }
 
@@ -281,48 +270,38 @@ public class NodeStateChangeChecker {
         return false;
     }
 
-    private Result anyNodeSetToMaintenance(ClusterState clusterState) {
-        for (NodeInfo nodeInfo : clusterInfo.getAllNodeInfo()) {
-            if (clusterState.getNodeState(nodeInfo.getNode()).getState() == State.MAINTENANCE) {
-                return Result.createDisallowed("Another node is already in maintenance:" + nodeInfo.getNodeIndex());
+    private Result checkAllNodesAreUp(ClusterState clusterState) {
+        // This method verifies both storage nodes and distributors are up (or retired).
+        // The complicated part is making a summary error message.
+
+        for (NodeInfo storageNodeInfo : clusterInfo.getStorageNodeInfo()) {
+            State wantedState = storageNodeInfo.getUserWantedState().getState();
+            if (wantedState != State.UP && wantedState != State.RETIRED) {
+                return Result.createDisallowed("Another storage node wants state " +
+                        wantedState.toString().toUpperCase() + ": " + storageNodeInfo.getNodeIndex());
             }
-            if (nodeInfo.getWantedState().getState() == State.MAINTENANCE) {
-                return Result.createDisallowed("Another node wants maintenance:" + nodeInfo.getNodeIndex());
+
+            State state = clusterState.getNodeState(storageNodeInfo.getNode()).getState();
+            if (state != State.UP && state != State.RETIRED) {
+                return Result.createDisallowed("Another storage node has state " + state.toString().toUpperCase() +
+                        ": " + storageNodeInfo.getNodeIndex());
             }
         }
-        return Result.allowSettingOfWantedState();
-    }
 
-    private int contentNodesWithAvailableNodeState(ClusterState clusterState) {
-        final int nodeCount = clusterState.getNodeCount(NodeType.STORAGE);
-        int upNodesCount = 0;
-        for (int i = 0; i < nodeCount; ++i) {
-            final Node node = new Node(NodeType.STORAGE, i);
-            final State state = clusterState.getNodeState(node).getState();
-            if (state == State.UP || state == State.RETIRED || state == State.INITIALIZING) {
-                upNodesCount++;
+        for (NodeInfo distributorNodeInfo : clusterInfo.getDistributorNodeInfo()) {
+            State wantedState = distributorNodeInfo.getUserWantedState().getState();
+            if (wantedState != State.UP && wantedState != State.RETIRED) {
+                return Result.createDisallowed("Another distributor wants state " + wantedState.toString().toUpperCase() +
+                        ": " + distributorNodeInfo.getNodeIndex());
             }
-        }
-        return upNodesCount;
-    }
 
-    private Result checkUpThresholds(ClusterState clusterState) {
-        if (clusterInfo.getStorageNodeInfo().size() < minStorageNodesUp) {
-            return Result.createDisallowed("There are only " + clusterInfo.getStorageNodeInfo().size() +
-                    " storage nodes up, while config requires at least " + minStorageNodesUp);
+            State state = clusterState.getNodeState(distributorNodeInfo.getNode()).getState();
+            if (state != State.UP && state != State.RETIRED) {
+                return Result.createDisallowed("Another distributor has state " + state.toString().toUpperCase() +
+                        ": " + distributorNodeInfo.getNodeIndex());
+            }
         }
 
-        final int nodesCount = clusterInfo.getStorageNodeInfo().size();
-        final int upNodesCount = contentNodesWithAvailableNodeState(clusterState);
-
-        if (nodesCount == 0) {
-            return Result.createDisallowed("No storage nodes in cluster state");
-        }
-        if (((double)upNodesCount) / nodesCount < minRatioOfStorageNodesUp) {
-            return Result.createDisallowed("Not enough storage nodes running: " + upNodesCount
-                    + " of " +  nodesCount + " storage nodes are up which is less that the required fraction of "
-                    + minRatioOfStorageNodesUp);
-        }
         return Result.allowSettingOfWantedState();
     }
 
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java
index 7ed4b4b3226..34e52eb82c4 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeStateChangeCheckerTest.java
@@ -16,9 +16,7 @@ import org.junit.Test;
 import java.text.ParseException;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
 
 import static org.hamcrest.core.Is.is;
 import static org.hamcrest.core.StringContains.containsString;
@@ -32,10 +30,8 @@ import static org.mockito.Mockito.when;
 
 public class NodeStateChangeCheckerTest {
 
-    private static final int minStorageNodesUp = 3;
     private static final int requiredRedundancy = 4;
     private static final int currentClusterStateVersion = 2;
-    private static final double minRatioOfStorageNodesUp = 0.9;
 
     private static final Node nodeDistributor = new Node(NodeType.DISTRIBUTOR, 1);
     private static final Node nodeStorage = new Node(NodeType.STORAGE, 1);
@@ -61,30 +57,14 @@ public class NodeStateChangeCheckerTest {
     }
 
     private NodeStateChangeChecker createChangeChecker(ContentCluster cluster) {
-        return new NodeStateChangeChecker(minStorageNodesUp, minRatioOfStorageNodesUp, requiredRedundancy,
-                visitor -> {}, cluster.clusterInfo());
+        return new NodeStateChangeChecker(requiredRedundancy, visitor -> {}, cluster.clusterInfo());
     }
 
     private ContentCluster createCluster(Collection<ConfiguredNode> nodes) {
         Distribution distribution = mock(Distribution.class);
         Group group = new Group(2, "to");
         when(distribution.getRootGroup()).thenReturn(group);
-        return new ContentCluster("Clustername", nodes, distribution, minStorageNodesUp, 0.0);
-    }
-
-    private StorageNodeInfo createStorageNodeInfo(int index, State state) {
-        Distribution distribution = mock(Distribution.class);
-        Group group = new Group(2, "to");
-        when(distribution.getRootGroup()).thenReturn(group);
-
-        String clusterName = "Clustername";
-        Set<ConfiguredNode> configuredNodeIndexes = new HashSet<>();
-        ContentCluster cluster = new ContentCluster(clusterName, configuredNodeIndexes, distribution, minStorageNodesUp, 0.0);
-
-        String rpcAddress = "";
-        StorageNodeInfo storageNodeInfo = new StorageNodeInfo(cluster, index, false, rpcAddress, distribution);
-        storageNodeInfo.setReportedState(new NodeState(NodeType.STORAGE, state), 3 /* time */);
-        return storageNodeInfo;
+        return new ContentCluster("Clustername", nodes, distribution, 3, 0.0);
     }
 
     private String createDistributorHostInfo(int replicationfactor1, int replicationfactor2, int replicationfactor3) {
@@ -137,8 +117,7 @@ public class NodeStateChangeCheckerTest {
     public void testUnknownStorageNode() {
         ContentCluster cluster = createCluster(createNodes(4));
         NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker(
-                5 /* min storage nodes */, minRatioOfStorageNodesUp, requiredRedundancy,
-                visitor -> {}, cluster.clusterInfo());
+                requiredRedundancy, visitor -> {}, cluster.clusterInfo());
         NodeStateChangeChecker.Result result = nodeStateChangeChecker.evaluateTransition(
                 new Node(NodeType.STORAGE, 10), defaultAllUpClusterState(), SetUnitStateRequest.Condition.SAFE,
                 UP_NODE_STATE, MAINTENANCE_NODE_STATE);
@@ -160,17 +139,23 @@ public class NodeStateChangeCheckerTest {
 
     @Test
     public void testCanUpgradeSafeMissingStorage() {
+        // Create a content cluster with 4 nodes, and storage node with index 3 down.
         ContentCluster cluster = createCluster(createNodes(4));
         setAllNodesUp(cluster, HostInfo.createHostInfo(createDistributorHostInfo(4, 5, 6)));
+        cluster.clusterInfo().getStorageNodeInfo(3).setReportedState(new NodeState(NodeType.STORAGE, State.DOWN), 0);
+        ClusterState clusterStateWith3Down = clusterState(String.format(
+                "version:%d distributor:4 storage:4 .3.s:d",
+                currentClusterStateVersion));
+
+        // We should then be denied setting storage node 1 safely to maintenance.
         NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker(
-                5 /* min storage nodes */, minRatioOfStorageNodesUp, requiredRedundancy, visitor -> {},
-                cluster.clusterInfo());
+                requiredRedundancy, visitor -> {}, cluster.clusterInfo());
         NodeStateChangeChecker.Result result = nodeStateChangeChecker.evaluateTransition(
-                nodeStorage, defaultAllUpClusterState(), SetUnitStateRequest.Condition.SAFE,
+                nodeStorage, clusterStateWith3Down, SetUnitStateRequest.Condition.SAFE,
                 UP_NODE_STATE, MAINTENANCE_NODE_STATE);
         assertFalse(result.settingWantedStateIsAllowed());
         assertFalse(result.wantedStateAlreadySet());
-        assertThat(result.getReason(), is("There are only 4 storage nodes up, while config requires at least 5"));
+        assertThat(result.getReason(), is("Another storage node has state DOWN: 3"));
     }
 
     @Test
@@ -402,7 +387,7 @@ public class NodeStateChangeCheckerTest {
         NodeStateChangeChecker.Result result = transitionToMaintenanceWithOneStorageNodeDown(otherIndex);
         assertFalse(result.settingWantedStateIsAllowed());
         assertFalse(result.wantedStateAlreadySet());
-        assertThat(result.getReason(), containsString("Not enough storage nodes running"));
+        assertThat(result.getReason(), containsString("Another storage node has state DOWN: 2"));
     }
 
     @Test
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java
index 8a9cfb4be98..5bb7897b8e7 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java
@@ -206,7 +206,7 @@ public class SetNodeStateTest extends StateRestApiTest {
     @Test
     public void testShouldModifyStorageSafeOk() throws Exception {
         setUp(false);
-        SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/1")
+        SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/2")
                 .setNewState("user", "maintenance", "whatever reason.")
                 .setCondition(SetUnitStateRequest.Condition.SAFE));
         assertThat(setResponse.getReason(), is("ok"));
@@ -265,20 +265,22 @@ public class SetNodeStateTest extends StateRestApiTest {
         assertSetUnitState(1, State.MAINTENANCE, null);  // sanity-check
 
         // Because 2 is in a different group maintenance should be denied
-        assertSetUnitStateCausesAlreadyInMaintenance(2, State.MAINTENANCE);
+        assertSetUnitStateCausesAlreadyInWantedMaintenance(2, State.MAINTENANCE);
 
         // Because 3 and 5 are in the same group as 1, these should be OK
         assertSetUnitState(3, State.MAINTENANCE, null);
         assertUnitState(1, "user", State.MAINTENANCE, "whatever reason.");  // sanity-check
         assertUnitState(3, "user", State.MAINTENANCE, "whatever reason.");  // sanity-check
         assertSetUnitState(5, State.MAINTENANCE, null);
-        assertSetUnitStateCausesAlreadyInMaintenance(2, State.MAINTENANCE);  // sanity-check
+        assertSetUnitStateCausesAlreadyInWantedMaintenance(2, State.MAINTENANCE);  // sanity-check
 
         // Set all to up
         assertSetUnitState(1, State.UP, null);
         assertSetUnitState(1, State.UP, null); // sanity-check
         assertSetUnitState(3, State.UP, null);
-        assertSetUnitStateCausesAlreadyInMaintenance(2, State.MAINTENANCE);  // sanity-check
+        // Because 1 is in maintenance, even though user wanted state is UP, trying to set 2 to
+        // maintenance will fail.
+        assertSetUnitStateCausesAlreadyInMaintenance(2, State.MAINTENANCE);
         assertSetUnitState(5, State.UP, null);
     }
 
@@ -306,11 +308,11 @@ public class SetNodeStateTest extends StateRestApiTest {
     }
 
     private void assertSetUnitStateCausesAlreadyInWantedMaintenance(int index, State state) throws StateRestApiException {
-        assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another node wants maintenance:([0-9]+)$");
+        assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another storage node wants state MAINTENANCE: ([0-9]+)$");
     }
 
     private void assertSetUnitStateCausesAlreadyInMaintenance(int index, State state) throws StateRestApiException {
-        assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another node is already in maintenance:([0-9]+)$");
+        assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another storage node has state MAINTENANCE: ([0-9]+)$");
     }
 
     private void assertSetUnitStateCausesAlreadyInMaintenance(int index, State state, String reasonRegex)