Merge pull request #16494 from vespa-engine/hakonhall/also-deny-maintenance-when-another-node-is-in-maintenance

Also deny maintenance when another node is in maintenance

3 files changed, 55 insertions, 14 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 8901dc8ae6b..acddfa478b1 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
@@ -226,7 +226,7 @@ public class NodeStateChangeChecker {
             return Result.allowSettingOfWantedState();
         }
 
-        Result ongoingChanges = anyNodeSetToMaintenance();
+        Result ongoingChanges = anyNodeSetToMaintenance(clusterState);
         if (!ongoingChanges.settingWantedStateIsAllowed()) {
             return ongoingChanges;
         }
@@ -281,10 +281,13 @@ public class NodeStateChangeChecker {
         return false;
     }
 
-    private Result anyNodeSetToMaintenance() {
+    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());
+            }
             if (nodeInfo.getWantedState().getState() == State.MAINTENANCE) {
-                return Result.createDisallowed("There is a node already in maintenance:" + nodeInfo.getNodeIndex());
+                return Result.createDisallowed("Another node wants maintenance:" + nodeInfo.getNodeIndex());
             }
         }
         return Result.allowSettingOfWantedState();
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 e8ded1971ac..8a9cfb4be98 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
@@ -216,7 +216,7 @@ public class SetNodeStateTest extends StateRestApiTest {
     @Test
     public void testShouldModifyStorageSafeBlocked() throws Exception {
         // Sets up 2 groups: [0, 2, 4] and [1, 3, 5]
-        setUpBookGroup(6);
+        setUpMusicGroup(6, false);
 
         assertUnitState(1, "user", State.UP, "");
         assertSetUnitState(1, State.MAINTENANCE, null);
@@ -224,28 +224,28 @@ 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
+        assertSetUnitStateCausesAlreadyInWantedMaintenance(2, State.MAINTENANCE);  // sanity-check
         assertSetUnitState(5, State.UP, null);
 
         // Now we should be allowed to upgrade second group, while the first group will be denied
         assertSetUnitState(2, State.MAINTENANCE, null);
-        assertSetUnitStateCausesAlreadyInMaintenance(1, State.MAINTENANCE);  // sanity-check
+        assertSetUnitStateCausesAlreadyInWantedMaintenance(1, State.MAINTENANCE);  // sanity-check
         assertSetUnitState(0, State.MAINTENANCE, null);
         assertSetUnitState(4, State.MAINTENANCE, null);
-        assertSetUnitStateCausesAlreadyInMaintenance(1, State.MAINTENANCE);  // sanity-check
+        assertSetUnitStateCausesAlreadyInWantedMaintenance(1, State.MAINTENANCE);  // sanity-check
 
         // And set second group up again
         assertSetUnitState(0, State.MAINTENANCE, null);
@@ -253,6 +253,35 @@ public class SetNodeStateTest extends StateRestApiTest {
         assertSetUnitState(4, State.MAINTENANCE, null);
     }
 
+    @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, "");
+
+        assertUnitState(1, "user", State.UP, "");
+        assertSetUnitState(1, State.MAINTENANCE, null);
+        assertUnitState(1, "user", State.MAINTENANCE, "whatever reason.");
+        assertSetUnitState(1, State.MAINTENANCE, null);  // sanity-check
+
+        // Because 2 is in a different group maintenance should be denied
+        assertSetUnitStateCausesAlreadyInMaintenance(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
+
+        // 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
+        assertSetUnitState(5, State.UP, null);
+    }
+
     private void assertUnitState(int index, String type, State state, String reason) throws StateRestApiException {
         String path = "music/storage/" + index;
         UnitResponse response = restAPI.getState(new StateRequest(path, 0));
@@ -276,15 +305,23 @@ public class SetNodeStateTest extends StateRestApiTest {
         }
     }
 
+    private void assertSetUnitStateCausesAlreadyInWantedMaintenance(int index, State state) throws StateRestApiException {
+        assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another node wants maintenance:([0-9]+)$");
+    }
+
     private void assertSetUnitStateCausesAlreadyInMaintenance(int index, State state) throws StateRestApiException {
+        assertSetUnitStateCausesAlreadyInMaintenance(index, state, "^Another node is already in maintenance:([0-9]+)$");
+    }
+
+    private void assertSetUnitStateCausesAlreadyInMaintenance(int index, State state, String reasonRegex)
+            throws StateRestApiException {
         SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/" + index)
                 .setNewState("user", state.toString().toLowerCase(), "whatever reason.")
                 .setCondition(SetUnitStateRequest.Condition.SAFE));
 
-        String regex = "^There is a node already in maintenance:([0-9]+)$";
-        Matcher matcher = Pattern.compile(regex).matcher(setResponse.getReason());
+        Matcher matcher = Pattern.compile(reasonRegex).matcher(setResponse.getReason());
 
-        String errorMessage = "Expected reason to match '" + regex + "', but got: " + setResponse.getReason() + "'";
+        String errorMessage = "Expected reason to match '" + reasonRegex + "', but got: " + setResponse.getReason() + "'";
         assertTrue(errorMessage, matcher.find());
 
         int alreadyMaintainedIndex = Integer.parseInt(matcher.group(1));
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 4ef6c587348..4d079ec782f 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
@@ -98,7 +98,7 @@ public abstract class StateRestApiTest {
         }, ccSockets);
     }
 
-    protected void setUpBookGroup(int nodeCount) {
+    protected void setUpMusicGroup(int nodeCount, boolean node1InMaintenance) {
         books = null;
         Distribution distribution = new Distribution(Distribution.getSimpleGroupConfig(2, nodeCount));
         jsonWriter.setDefaultPathPrefix("/cluster/v2");
@@ -106,7 +106,8 @@ public abstract class StateRestApiTest {
                 "music", distribution.getNodes(), distribution, 0 /* minStorageNodesUp*/, 0.0 /* minRatioOfStorageNodesUp */);
         initializeCluster(cluster, distribution.getNodes());
         AnnotatedClusterState baselineState = AnnotatedClusterState
-                .withoutAnnotations(ClusterState.stateFromString("distributor:" + nodeCount + " storage:" + nodeCount));
+                .withoutAnnotations(ClusterState.stateFromString("distributor:" + nodeCount + " storage:" + nodeCount +
+                        (node1InMaintenance ? " .1.s:m" : "")));
         Map<String, AnnotatedClusterState> bucketSpaceStates = new HashMap<>();
         bucketSpaceStates.put("default", AnnotatedClusterState
                 .withoutAnnotations(ClusterState.stateFromString("distributor:" + nodeCount + " storage:" + nodeCount)));