No longer allow suspension if in maintenance

If a storage node falls out of Slobrok, it will change from UP to Maintenance
after 60s, then after further 30s go to Down.  Avoid allowing suspension in the
30s grace period just because it is Maintenance mode.

3 files changed, 14 insertions, 17 deletions

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 dd33646dd31..f28fa37c7b7 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
@@ -212,10 +212,8 @@ public class NodeStateChangeChecker {
                     oldWantedState.getState() + ": " + oldWantedState.getDescription());
         }
 
-        switch (clusterState.getNodeState(nodeInfo.getNode()).getState()) {
-            case MAINTENANCE:
-            case DOWN:
-                return Result.allowSettingOfWantedState();
+        if (clusterState.getNodeState(nodeInfo.getNode()).getState() == State.DOWN) {
+            return Result.allowSettingOfWantedState();
         }
 
         if (anotherNodeInGroupAlreadyAllowed(nodeInfo, newDescription)) {
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 712c34eae4b..2ba19d4f5be 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
@@ -215,7 +215,7 @@ public class SetNodeStateTest extends StateRestApiTest {
     @Test
     public void testShouldModifyStorageSafeBlocked() throws Exception {
         // Sets up 2 groups: [0, 2, 4] and [1, 3, 5]
-        setUpMusicGroup(6, false);
+        setUpMusicGroup(6, "");
 
         assertUnitState(1, "user", State.UP, "");
         assertSetUnitState(1, State.MAINTENANCE, null);
@@ -253,10 +253,10 @@ public class SetNodeStateTest extends StateRestApiTest {
     }
 
     @Test
-    public void settingSafeMaintenanceWhenNodeAlreadyInMaintenance() throws Exception {
-        // Sets up 2 groups: [0, 2, 4] and [1, 3, 5], with 1 being in maintenance
-        setUpMusicGroup(6, true);
-        assertUnitState(1, "generated", State.MAINTENANCE, "");
+    public void settingSafeMaintenanceWhenNodeDown() throws Exception {
+        // Sets up 2 groups: [0, 2, 4] and [1, 3, 5], with 1 being down
+        setUpMusicGroup(6, " .1.s:d");
+        assertUnitState(1, "generated", State.DOWN, "");
 
         assertUnitState(1, "user", State.UP, "");
         assertSetUnitState(1, State.MAINTENANCE, null);
@@ -279,7 +279,7 @@ public class SetNodeStateTest extends StateRestApiTest {
         assertSetUnitState(3, State.UP, null);
         // Because 1 is in maintenance, even though user wanted state is UP, trying to set 2 to
         // maintenance will fail.
-        assertSetUnitStateCausesAlreadyInMaintenance(2, State.MAINTENANCE);
+        assertSetUnitStateCausesAnotherNodeHasStateError(2, State.MAINTENANCE);
         assertSetUnitState(5, State.UP, null);
     }
 
@@ -307,14 +307,14 @@ public class SetNodeStateTest extends StateRestApiTest {
     }
 
     private void assertSetUnitStateCausesAlreadyInWantedMaintenance(int index, State state) throws StateRestApiException {
-        assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another storage node wants state MAINTENANCE: ([0-9]+)$");
+        assertSetUnitStateFails(index, state, "^Another storage node wants state MAINTENANCE: ([0-9]+)$");
     }
 
-    private void assertSetUnitStateCausesAlreadyInMaintenance(int index, State state) throws StateRestApiException {
-        assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another storage node has state MAINTENANCE: ([0-9]+)$");
+    private void assertSetUnitStateCausesAnotherNodeHasStateError(int index, State state) throws StateRestApiException {
+        assertSetUnitStateFails(index, state, "^Another storage node has state DOWN: ([0-9]+)$");
     }
 
-    private void assertSetUnitStateCausesAlreadyInMaintenance(int index, State state, String reasonRegex)
+    private void assertSetUnitStateFails(int index, State state, String reasonRegex)
             throws StateRestApiException {
         SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/" + index)
                 .setNewState("user", state.toString().toLowerCase(), "whatever reason.")
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/StateRestApiTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/StateRestApiTest.java
index 14eab503885..cc2c100e105 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/StateRestApiTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/StateRestApiTest.java
@@ -92,15 +92,14 @@ public abstract class StateRestApiTest {
         }, ccSockets);
     }
 
-    protected void setUpMusicGroup(int nodeCount, boolean node1InMaintenance) {
+    protected void setUpMusicGroup(int nodeCount, String node1StateString) {
         books = null;
         Distribution distribution = new Distribution(Distribution.getSimpleGroupConfig(2, nodeCount));
         jsonWriter.setDefaultPathPrefix("/cluster/v2");
         ContentCluster cluster = new ContentCluster("music", distribution.getNodes(), distribution);
         initializeCluster(cluster, distribution.getNodes());
         AnnotatedClusterState baselineState = AnnotatedClusterState
-                .withoutAnnotations(ClusterState.stateFromString("distributor:" + nodeCount + " storage:" + nodeCount +
-                        (node1InMaintenance ? " .1.s:m" : "")));
+                .withoutAnnotations(ClusterState.stateFromString("distributor:" + nodeCount + " storage:" + nodeCount + node1StateString));
         Map<String, AnnotatedClusterState> bucketSpaceStates = new HashMap<>();
         bucketSpaceStates.put("default", AnnotatedClusterState
                 .withoutAnnotations(ClusterState.stateFromString("distributor:" + nodeCount + " storage:" + nodeCount)));