Allows setting a node safely to maintenance in these two new circumstances:

 1.  The node has state MAINTENANCE with (user) wanted state UP.
 2.  There are other nodes in the same hierarchical group that are set in
     MAINTENANCE with the same description.

Also made the following change.

 3. Deny a request for safe MAINTENANCE or DOWN, if the wanted state is already
    set but with a different description.  If the descriptions are the same, it
    is assumed to be the same operator (e.g. Orchestrator) having changed its
    mind.

8 files changed, 302 insertions, 56 deletions

diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterInfo.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterInfo.java
index b80004b02e7..450ef468122 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterInfo.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterInfo.java
@@ -32,8 +32,10 @@ public class ClusterInfo {
     /** Information about the current state of all nodes - always consists of both sets of nodes in the two maps above */
     private final Map<Node, NodeInfo> allNodeInfo = new TreeMap<>(); // TODO: Remove
 
+    /** Returns non-null iff index is a configured nodes (except perhaps in tests). */
     DistributorNodeInfo getDistributorNodeInfo(int index) { return distributorNodeInfo.get(index); }
 
+    /** Returns non-null iff index is a configured nodes (except perhaps in tests). */
     StorageNodeInfo getStorageNodeInfo(int index) { return storageNodeInfo.get(index); }
 
     NodeInfo getNodeInfo(NodeType type, int index) { return getNodeInfo(new Node(type, index)); }
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 dad2b91d3cc..06d728d5625 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
@@ -7,13 +7,22 @@
 package com.yahoo.vespa.clustercontroller.core;
 
 import com.yahoo.vdslib.distribution.ConfiguredNode;
-import com.yahoo.vdslib.state.*;
 import com.yahoo.vdslib.distribution.Distribution;
 import com.yahoo.vdslib.distribution.Group;
+import com.yahoo.vdslib.state.ClusterState;
+import com.yahoo.vdslib.state.Node;
+import com.yahoo.vdslib.state.NodeState;
+import com.yahoo.vdslib.state.NodeType;
+import com.yahoo.vdslib.state.State;
 import com.yahoo.vespa.clustercontroller.core.status.statuspage.VdsClusterHtmlRenderer;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStateRequest;
 
-import java.util.*;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.TreeMap;
 import java.util.stream.Collectors;
 
 import static com.yahoo.vdslib.state.NodeState.ORCHESTRATOR_RESERVED_DESCRIPTION;
@@ -177,15 +186,18 @@ public class ContentCluster {
      * @param node the node to be checked for upgrad
      * @param clusterState the current cluster state version
      * @param condition the upgrade condition
+     * @param oldState the old/current wanted state
      * @param newState state wanted to be set  @return NodeUpgradePrechecker.Response
      */
     public NodeStateChangeChecker.Result calculateEffectOfNewState(
-            Node node, ClusterState clusterState, SetUnitStateRequest.Condition condition, NodeState oldState, NodeState newState) {
+            Node node, ClusterState clusterState, SetUnitStateRequest.Condition condition,
+            NodeState oldState, NodeState newState) {
 
         NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker(
                 minStorageNodesUp,
                 minRatioOfStorageNodesUp,
                 distribution.getRedundancy(),
+                new HierarchicalGroupVisitingAdapter(distribution),
                 clusterInfo
         );
         return nodeStateChangeChecker.evaluateTransition(node, clusterState, condition, oldState, newState);
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/HierarchicalGroupVisiting.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/HierarchicalGroupVisiting.java
new file mode 100644
index 00000000000..1e8fb9e2ffb
--- /dev/null
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/HierarchicalGroupVisiting.java
@@ -0,0 +1,14 @@
+// Copyright Verizon Media. 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.distribution.GroupVisitor;
+
+@FunctionalInterface
+public interface HierarchicalGroupVisiting {
+    /**
+     * Invoke the visitor for each leaf group of an implied group.  If the group is non-hierarchical
+     * (flat), the visitor will not be invoked.
+     */
+    void visit(GroupVisitor visitor);
+}
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/HierarchicalGroupVisitingAdapter.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/HierarchicalGroupVisitingAdapter.java
new file mode 100644
index 00000000000..b0d69750c77
--- /dev/null
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/HierarchicalGroupVisitingAdapter.java
@@ -0,0 +1,35 @@
+// Copyright Verizon Media. 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.distribution.Distribution;
+import com.yahoo.vdslib.distribution.GroupVisitor;
+
+/**
+ * Exposes {@link Distribution} as a {@link HierarchicalGroupVisiting}.
+ *
+ * @author hakon
+ */
+public class HierarchicalGroupVisitingAdapter implements HierarchicalGroupVisiting {
+    private final Distribution distribution;
+
+    public HierarchicalGroupVisitingAdapter(Distribution distribution) {
+        this.distribution = distribution;
+    }
+
+    @Override
+    public void visit(GroupVisitor visitor) {
+        if (distribution.getRootGroup().isLeafGroup()) {
+            // A flat non-hierarchical cluster
+            return;
+        }
+
+        distribution.visitGroups(group -> {
+            if (group.isLeafGroup()) {
+                return visitor.visitGroup(group);
+            }
+
+            return true;
+        });
+    }
+}
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 eb4a5cc00b0..8901dc8ae6b 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
@@ -1,6 +1,9 @@
 // Copyright 2017 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.lang.MutableBoolean;
+import com.yahoo.vdslib.distribution.ConfiguredNode;
+import com.yahoo.vdslib.distribution.Group;
 import com.yahoo.vdslib.state.ClusterState;
 import com.yahoo.vdslib.state.Node;
 import com.yahoo.vdslib.state.NodeState;
@@ -13,6 +16,7 @@ import com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.SetUnitStat
 
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 
 /**
@@ -28,16 +32,19 @@ public class NodeStateChangeChecker {
     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;
     }
 
@@ -88,7 +95,7 @@ public class NodeStateChangeChecker {
 
     public Result evaluateTransition(
             Node node, ClusterState clusterState, SetUnitStateRequest.Condition condition,
-            NodeState oldState, NodeState newState) {
+            NodeState oldWantedState, NodeState newWantedState) {
         if (condition == SetUnitStateRequest.Condition.FORCE) {
             return Result.allowSettingOfWantedState();
         }
@@ -102,7 +109,7 @@ public class NodeStateChangeChecker {
                     "Requested node type: " + node.getType().toString());
         }
 
-        NodeInfo nodeInfo = clusterInfo.getStorageNodeInfo(node.getIndex());
+        StorageNodeInfo nodeInfo = clusterInfo.getStorageNodeInfo(node.getIndex());
         if (nodeInfo == null) {
             return Result.createDisallowed("Unknown node " + node);
         }
@@ -112,23 +119,34 @@ public class NodeStateChangeChecker {
         // - We ensure that clients that have previously set the wanted state, continue
         //   to see the same conclusion, even though they possibly would have been denied
         //   MUST_SET_WANTED_STATE if re-evaluated. This is important for implementing idempotent clients.
-        if (newState.getState().equals(oldState.getState())) {
+        if (newWantedState.getState().equals(oldWantedState.getState()) &&
+            Objects.equals(newWantedState.getDescription(), oldWantedState.getDescription())) {
             return Result.createAlreadySet();
         }
 
-        switch (newState.getState()) {
+        switch (newWantedState.getState()) {
             case UP:
-                return canSetStateUp(nodeInfo);
+                return canSetStateUp(nodeInfo, oldWantedState);
             case MAINTENANCE:
-                return canSetStateMaintenanceTemporarily(node, clusterState);
+                return canSetStateMaintenanceTemporarily(nodeInfo, clusterState, newWantedState.getDescription());
             case DOWN:
-                return canSetStateDownPermanently(nodeInfo, clusterState);
+                return canSetStateDownPermanently(nodeInfo, clusterState, newWantedState.getDescription());
             default:
-                return Result.createDisallowed("Destination node state unsupported in safe mode: " + newState);
+                return Result.createDisallowed("Destination node state unsupported in safe mode: " + newWantedState);
         }
     }
 
-    private Result canSetStateDownPermanently(NodeInfo nodeInfo, ClusterState clusterState) {
+    private Result canSetStateDownPermanently(NodeInfo nodeInfo, ClusterState clusterState, String newDescription) {
+        NodeState oldWantedState = nodeInfo.getUserWantedState();
+        if (oldWantedState.getState() != State.UP && !oldWantedState.getDescription().equals(newDescription)) {
+            // Refuse to override whatever an operator or unknown entity is doing.
+            //
+            // Note:  The new state&description is NOT equal to the old state&description:
+            // that would have been short-circuited prior to this.
+            return Result.createDisallowed("A conflicting wanted state is already set: " +
+                    oldWantedState.getState() + ": " + oldWantedState.getDescription());
+        }
+
         State reportedState = nodeInfo.getReportedState().getState();
         if (reportedState != State.UP) {
             return Result.createDisallowed("Reported state (" + reportedState
@@ -170,7 +188,12 @@ public class NodeStateChangeChecker {
         return Result.allowSettingOfWantedState();
     }
 
-    private Result canSetStateUp(NodeInfo nodeInfo) {
+    private Result canSetStateUp(NodeInfo nodeInfo, NodeState oldWantedState) {
+        if (oldWantedState.getState() == State.UP) {
+            // The description is not significant when setting wanting to set the state to UP
+            return Result.createAlreadySet();
+        }
+
         if (nodeInfo.getReportedState().getState() != State.UP) {
             return Result.createDisallowed("Refuse to set wanted state to UP, " +
                     "since the reported state is not UP (" +
@@ -180,14 +203,27 @@ public class NodeStateChangeChecker {
         return Result.allowSettingOfWantedState();
     }
 
-    private Result canSetStateMaintenanceTemporarily(Node node, ClusterState clusterState) {
-        if (clusterState.getNodeState(node).getState() == State.DOWN) {
-            return Result.allowSettingOfWantedState();
+    private Result canSetStateMaintenanceTemporarily(StorageNodeInfo nodeInfo, ClusterState clusterState,
+                                                     String newDescription) {
+        NodeState oldWantedState = nodeInfo.getUserWantedState();
+        if (oldWantedState.getState() != State.UP && !oldWantedState.getDescription().equals(newDescription)) {
+            // Refuse to override whatever an operator or unknown entity is doing.  If the description is
+            // identical, we assume it is the same operator.
+            //
+            // Note:  The new state&description is NOT equal to the old state&description:
+            // that would have been short-circuited prior to this.
+            return Result.createDisallowed("A conflicting wanted state is already set: " +
+                    oldWantedState.getState() + ": " + oldWantedState.getDescription());
         }
 
-        Result checkDistributorsResult = checkDistributors(node, clusterState.getVersion());
-        if (!checkDistributorsResult.settingWantedStateIsAllowed()) {
-            return checkDistributorsResult;
+        switch (clusterState.getNodeState(nodeInfo.getNode()).getState()) {
+            case MAINTENANCE:
+            case DOWN:
+                return Result.allowSettingOfWantedState();
+        }
+
+        if (anotherNodeInGroupAlreadyAllowed(nodeInfo, newDescription)) {
+            return Result.allowSettingOfWantedState();
         }
 
         Result ongoingChanges = anyNodeSetToMaintenance();
@@ -195,6 +231,11 @@ public class NodeStateChangeChecker {
             return ongoingChanges;
         }
 
+        Result checkDistributorsResult = checkDistributors(nodeInfo.getNode(), clusterState.getVersion());
+        if (!checkDistributorsResult.settingWantedStateIsAllowed()) {
+            return checkDistributorsResult;
+        }
+
         Result thresholdCheckResult = checkUpThresholds(clusterState);
         if (!thresholdCheckResult.settingWantedStateIsAllowed()) {
             return thresholdCheckResult;
@@ -203,6 +244,43 @@ public class NodeStateChangeChecker {
         return Result.allowSettingOfWantedState();
     }
 
+    private boolean anotherNodeInGroupAlreadyAllowed(StorageNodeInfo nodeInfo, String newDescription) {
+        MutableBoolean alreadyAllowed = new MutableBoolean(false);
+
+        groupVisiting.visit(group -> {
+            if (!groupContainsNode(group, nodeInfo.getNode())) {
+                return true;
+            }
+
+            alreadyAllowed.set(anotherNodeInGroupAlreadyAllowed(group, nodeInfo.getNode(), newDescription));
+
+            // Have found the leaf group we were looking for, halt the visiting.
+            return false;
+        });
+
+        return alreadyAllowed.get();
+    }
+
+    private boolean anotherNodeInGroupAlreadyAllowed(Group group, Node node, String newDescription) {
+        return group.getNodes().stream()
+                .filter(configuredNode -> configuredNode.index() != node.getIndex())
+                .map(configuredNode -> clusterInfo.getStorageNodeInfo(configuredNode.index()))
+                .filter(Objects::nonNull)  // needed for tests only
+                .map(NodeInfo::getUserWantedState)
+                .anyMatch(userWantedState -> userWantedState.getState() == State.MAINTENANCE &&
+                          Objects.equals(userWantedState.getDescription(), newDescription));
+    }
+
+    private static boolean groupContainsNode(Group group, Node node) {
+        for (ConfiguredNode configuredNode : group.getNodes()) {
+            if (configuredNode.index() == node.getIndex()) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     private Result anyNodeSetToMaintenance() {
         for (NodeInfo nodeInfo : clusterInfo.getAllNodeInfo()) {
             if (nodeInfo.getWantedState().getState() == State.MAINTENANCE) {
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 e918f94abbf..2204a03b4aa 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
@@ -61,7 +61,8 @@ public class NodeStateChangeCheckerTest {
     }
 
     private NodeStateChangeChecker createChangeChecker(ContentCluster cluster) {
-        return new NodeStateChangeChecker(minStorageNodesUp, minRatioOfStorageNodesUp, requiredRedundancy, cluster.clusterInfo());
+        return new NodeStateChangeChecker(minStorageNodesUp, minRatioOfStorageNodesUp, requiredRedundancy,
+                visitor -> {}, cluster.clusterInfo());
     }
 
     private ContentCluster createCluster(Collection<ConfiguredNode> nodes) {
@@ -136,7 +137,8 @@ public class NodeStateChangeCheckerTest {
     public void testUnknownStorageNode() {
         ContentCluster cluster = createCluster(createNodes(4));
         NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker(
-                5 /* min storage nodes */, minRatioOfStorageNodesUp, requiredRedundancy, cluster.clusterInfo());
+                5 /* min storage nodes */, minRatioOfStorageNodesUp, requiredRedundancy,
+                visitor -> {}, cluster.clusterInfo());
         NodeStateChangeChecker.Result result = nodeStateChangeChecker.evaluateTransition(
                 new Node(NodeType.STORAGE, 10), defaultAllUpClusterState(), SetUnitStateRequest.Condition.SAFE,
                 UP_NODE_STATE, MAINTENANCE_NODE_STATE);
@@ -161,7 +163,8 @@ public class NodeStateChangeCheckerTest {
         ContentCluster cluster = createCluster(createNodes(4));
         setAllNodesUp(cluster, HostInfo.createHostInfo(createDistributorHostInfo(4, 5, 6)));
         NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker(
-                5 /* min storage nodes */, minRatioOfStorageNodesUp, requiredRedundancy, cluster.clusterInfo());
+                5 /* min storage nodes */, minRatioOfStorageNodesUp, requiredRedundancy, visitor -> {},
+                cluster.clusterInfo());
         NodeStateChangeChecker.Result result = nodeStateChangeChecker.evaluateTransition(
                 nodeStorage, defaultAllUpClusterState(), SetUnitStateRequest.Condition.SAFE,
                 UP_NODE_STATE, MAINTENANCE_NODE_STATE);
@@ -317,10 +320,10 @@ public class NodeStateChangeCheckerTest {
     }
 
     @Test
-    public void testDifferentDescriptionImpliesAlreadySet() {
+    public void testDifferentDescriptionImpliesDenied() {
         NodeStateChangeChecker.Result result = transitionToSameState("foo", "bar");
         assertFalse(result.settingWantedStateIsAllowed());
-        assertTrue(result.wantedStateAlreadySet());
+        assertFalse(result.wantedStateAlreadySet());
     }
 
     private NodeStateChangeChecker.Result transitionToMaintenanceWithOneStorageNodeDown(
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 824a7af55c4..e8ded1971ac 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
@@ -3,6 +3,7 @@ package com.yahoo.vespa.clustercontroller.core.restapiv2;
 
 import com.yahoo.time.TimeBudget;
 import com.yahoo.vdslib.state.NodeType;
+import com.yahoo.vdslib.state.State;
 import com.yahoo.vespa.clustercontroller.core.MasterInterface;
 import com.yahoo.vespa.clustercontroller.core.RemoteClusterControllerTask;
 import com.yahoo.vespa.clustercontroller.core.restapiv2.requests.SetNodeStateRequest;
@@ -25,12 +26,15 @@ import java.time.Clock;
 import java.time.Duration;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.core.Is.is;
 import static org.hamcrest.core.StringContains.containsString;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.any;
@@ -205,30 +209,93 @@ public class SetNodeStateTest extends StateRestApiTest {
         SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/1")
                 .setNewState("user", "maintenance", "whatever reason.")
                 .setCondition(SetUnitStateRequest.Condition.SAFE));
-        assertThat(setResponse.getWasModified(), is(true));
         assertThat(setResponse.getReason(), is("ok"));
+        assertThat(setResponse.getWasModified(), is(true));
     }
 
     @Test
     public void testShouldModifyStorageSafeBlocked() throws Exception {
-        setUp(false);
-        {
-            SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/1")
-                    .setNewState("user", "maintenance", "whatever reason.")
-                    .setCondition(SetUnitStateRequest.Condition.SAFE));
+        // Sets up 2 groups: [0, 2, 4] and [1, 3, 5]
+        setUpBookGroup(6);
+
+        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);
+
+        // 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
+        assertSetUnitState(0, State.MAINTENANCE, null);
+        assertSetUnitState(4, State.MAINTENANCE, null);
+        assertSetUnitStateCausesAlreadyInMaintenance(1, State.MAINTENANCE);  // sanity-check
+
+        // And set second group up again
+        assertSetUnitState(0, State.MAINTENANCE, null);
+        assertSetUnitState(2, State.MAINTENANCE, null);
+        assertSetUnitState(4, State.MAINTENANCE, 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));
+        Response.NodeResponse nodeResponse = (Response.NodeResponse) response;
+        UnitState unitState = nodeResponse.getStatePerType().get(type);
+        assertNotNull("No such type " + type + " at path " + path, unitState);
+        assertEquals(state.toString().toLowerCase(), unitState.getId());
+        assertEquals(reason, unitState.getReason());
+    }
+
+    private void assertSetUnitState(int index, State state, String failureReason) throws StateRestApiException {
+        SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/" + index)
+                .setNewState("user", state.toString().toLowerCase(), "whatever reason.")
+                .setCondition(SetUnitStateRequest.Condition.SAFE));
+        if (failureReason == null) {
             assertThat(setResponse.getReason(), is("ok"));
             assertThat(setResponse.getWasModified(), is(true));
-        }
-        {
-            SetResponse setResponse = restAPI.setUnitState(new SetUnitStateRequestImpl("music/storage/3")
-                    .setNewState("user", "maintenance", "whatever reason.")
-                    .setCondition(SetUnitStateRequest.Condition.SAFE));
-            assertThat(setResponse.getReason(), is(
-                    "There is a node already in maintenance:1"));
+        } else {
+            assertThat(setResponse.getReason(), is(failureReason));
             assertThat(setResponse.getWasModified(), is(false));
         }
     }
 
+    private void assertSetUnitStateCausesAlreadyInMaintenance(int index, State state) 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());
+
+        String errorMessage = "Expected reason to match '" + regex + "', but got: " + setResponse.getReason() + "'";
+        assertTrue(errorMessage, matcher.find());
+
+        int alreadyMaintainedIndex = Integer.parseInt(matcher.group(1));
+        // Example: Say index 1 is in maintenance, and we try to set 2 in maintenance. This should
+        // NOT be allowed, since 2 is in a different group than 1.
+        assertEquals("Tried to set " + index + " in maintenance, but got: " + setResponse.getReason(),
+                index % 2, (alreadyMaintainedIndex + 1) % 2);
+
+        assertThat(setResponse.getWasModified(), is(false));
+    }
+
     @Test
     public void testSetWantedStateOnNodeNotInSlobrok() throws Exception {
         // Node 2 in cluster music does not have a valid NodeInfo due to passing true to setUp
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 faebbf8755d..4ef6c587348 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
@@ -3,14 +3,29 @@ package com.yahoo.vespa.clustercontroller.core.restapiv2;
 
 import com.yahoo.vdslib.distribution.ConfiguredNode;
 import com.yahoo.vdslib.distribution.Distribution;
-import com.yahoo.vdslib.state.*;
-import com.yahoo.vespa.clustercontroller.core.*;
+import com.yahoo.vdslib.state.ClusterState;
+import com.yahoo.vdslib.state.Node;
+import com.yahoo.vdslib.state.NodeState;
+import com.yahoo.vdslib.state.NodeType;
+import com.yahoo.vdslib.state.State;
+import com.yahoo.vespa.clustercontroller.core.AnnotatedClusterState;
+import com.yahoo.vespa.clustercontroller.core.ClusterStateBundle;
+import com.yahoo.vespa.clustercontroller.core.ContentCluster;
+import com.yahoo.vespa.clustercontroller.core.FleetControllerTest;
+import com.yahoo.vespa.clustercontroller.core.NodeInfo;
+import com.yahoo.vespa.clustercontroller.core.RemoteClusterControllerTaskScheduler;
 import com.yahoo.vespa.clustercontroller.core.hostinfo.HostInfo;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.StateRestAPI;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.requests.UnitStateRequest;
 import com.yahoo.vespa.clustercontroller.utils.staterestapi.server.JsonWriter;
 
-import java.util.*;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.stream.Collectors;
 
 // TODO: Author
 public abstract class StateRestApiTest {
@@ -83,6 +98,33 @@ public abstract class StateRestApiTest {
         }, ccSockets);
     }
 
+    protected void setUpBookGroup(int nodeCount) {
+        books = null;
+        Distribution distribution = new Distribution(Distribution.getSimpleGroupConfig(2, nodeCount));
+        jsonWriter.setDefaultPathPrefix("/cluster/v2");
+        ContentCluster cluster = new ContentCluster(
+                "music", distribution.getNodes(), distribution, 0 /* minStorageNodesUp*/, 0.0 /* minRatioOfStorageNodesUp */);
+        initializeCluster(cluster, distribution.getNodes());
+        AnnotatedClusterState baselineState = AnnotatedClusterState
+                .withoutAnnotations(ClusterState.stateFromString("distributor:" + nodeCount + " storage:" + nodeCount));
+        Map<String, AnnotatedClusterState> bucketSpaceStates = new HashMap<>();
+        bucketSpaceStates.put("default", AnnotatedClusterState
+                .withoutAnnotations(ClusterState.stateFromString("distributor:" + nodeCount + " storage:" + nodeCount)));
+        bucketSpaceStates.put("global", baselineState);
+        music = new ClusterControllerMock(cluster, baselineState.getClusterState(),
+                ClusterStateBundle.of(baselineState, bucketSpaceStates), 0, 0);
+        ccSockets = new TreeMap<>();
+        ccSockets.put(0, new ClusterControllerStateRestAPI.Socket("localhost", 80));
+        restAPI = new ClusterControllerStateRestAPI(new ClusterControllerStateRestAPI.FleetControllerResolver() {
+            @Override
+            public Map<String, RemoteClusterControllerTaskScheduler> getFleetControllers() {
+                Map<String, RemoteClusterControllerTaskScheduler> fleetControllers = new LinkedHashMap<>();
+                fleetControllers.put(music.context.cluster.getName(), music);
+                return fleetControllers;
+            }
+        }, ccSockets);
+    }
+
     private void initializeCluster(ContentCluster cluster, Collection<ConfiguredNode> nodes) {
         for (ConfiguredNode configuredNode : nodes) {
             for (NodeType type : NodeType.getTypes()) {
@@ -93,12 +135,12 @@ public abstract class StateRestApiTest {
 
                 NodeInfo nodeInfo = cluster.clusterInfo().setRpcAddress(new Node(type, configuredNode.index()), "rpc:" + type + "/" + configuredNode);
                 nodeInfo.setReportedState(reported, 10);
-                nodeInfo.setHostInfo(HostInfo.createHostInfo(getHostInfo()));
+                nodeInfo.setHostInfo(HostInfo.createHostInfo(getHostInfo(nodes)));
             }
         }
     }
 
-    private String getHostInfo() {
+    private String getHostInfo(Collection<ConfiguredNode> nodes) {
         return "{\n" +
                 "    \"cluster-state-version\": 0,\n" +
                 "    \"metrics\": {\n" +
@@ -125,22 +167,15 @@ public abstract class StateRestApiTest {
                 "    },\n" +
                 "    \"distributor\": {\n" +
                 "        \"storage-nodes\": [\n" +
+
+                nodes.stream()
+                        .map(configuredNode ->
                 "            {\n" +
-                "                \"node-index\": 1,\n" +
-                "                \"min-current-replication-factor\": 2\n" +
-                "            },\n" +
-                "            {\n" +
-                "                \"node-index\": 3,\n" +
-                "                \"min-current-replication-factor\": 2\n" +
-                "            },\n" +
-                "            {\n" +
-                "                \"node-index\": 5,\n" +
-                "                \"min-current-replication-factor\": 2\n" +
-                "            },\n" +
-                "            {\n" +
-                "                \"node-index\": 7,\n" +
+                "                \"node-index\": " + configuredNode.index() + ",\n" +
                 "                \"min-current-replication-factor\": 2\n" +
-                "            }\n" +
+                "            }")
+                        .collect(Collectors.joining(",\n")) + "\n" +
+
                 "        ]\n" +
                 "    }\n" +
                 "}";