Minor refactoring and start of some new test

2 files changed, 46 insertions, 25 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 60671c96474..54240330de3 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
@@ -92,9 +92,9 @@ public class NodeStateChangeChecker {
             return new Result(Action.ALREADY_SET, "Basic preconditions fulfilled and new state is already effective");
         }
 
-        public boolean settingWantedStateIsAllowed() {
-            return action == Action.MUST_SET_WANTED_STATE;
-        }
+        public boolean settingWantedStateIsAllowed() { return action == Action.MUST_SET_WANTED_STATE; }
+
+        public boolean settingWantedStateIsNotAllowed() { return ! settingWantedStateIsAllowed(); }
 
         public boolean wantedStateAlreadySet() {
             return action == Action.ALREADY_SET;
@@ -200,7 +200,7 @@ public class NodeStateChangeChecker {
 
     private Result canSetStateUp(NodeInfo nodeInfo, NodeState oldWantedState) {
         if (oldWantedState.getState() == UP) {
-            // The description is not significant when setting wanting to set the state to UP
+            // The description is not significant when wanting to set the state to UP
             return createAlreadySet();
         }
 
@@ -228,7 +228,7 @@ public class NodeStateChangeChecker {
 
         if (maxNumberOfGroupsAllowedToBeDown == -1) {
             var otherGroupCheck = anotherNodeInAnotherGroupHasWantedState(nodeInfo);
-            if (!otherGroupCheck.settingWantedStateIsAllowed()) {
+            if (otherGroupCheck.settingWantedStateIsNotAllowed()) {
                 return otherGroupCheck;
             }
             if (anotherNodeInGroupAlreadyAllowed(nodeInfo, newDescription)) {
@@ -246,13 +246,13 @@ public class NodeStateChangeChecker {
         }
 
         Result allNodesAreUpCheck = checkAllNodesAreUp(clusterState);
-        if (!allNodesAreUpCheck.settingWantedStateIsAllowed()) {
+        if (allNodesAreUpCheck.settingWantedStateIsNotAllowed()) {
             log.log(FINE, "allNodesAreUpCheck: " + allNodesAreUpCheck);
             return allNodesAreUpCheck;
         }
 
         Result checkDistributorsResult = checkDistributors(nodeInfo.getNode(), clusterState.getVersion());
-        if (!checkDistributorsResult.settingWantedStateIsAllowed()) {
+        if (checkDistributorsResult.settingWantedStateIsNotAllowed()) {
             log.log(FINE, "checkDistributors: "+ checkDistributorsResult);
             return checkDistributorsResult;
         }
@@ -271,7 +271,7 @@ public class NodeStateChangeChecker {
             groupVisiting.visit(group -> {
                 if (!groupContainsNode(group, nodeInfo.getNode())) {
                     Result result = otherNodeInGroupHasWantedState(group);
-                    if (!result.settingWantedStateIsAllowed()) {
+                    if (result.settingWantedStateIsNotAllowed()) {
                         anotherNodeHasWantedState.set(result);
                         // Have found a node that is suspended, halt the visiting
                         return false;
@@ -283,7 +283,7 @@ public class NodeStateChangeChecker {
 
             return anotherNodeHasWantedState.asOptional().orElseGet(Result::allowSettingOfWantedState);
         } else {
-            // Return a disallow-result if there is another node with a wanted state
+            // Returns a disallow-result if there is another node with a wanted state
             return otherNodeHasWantedState(nodeInfo);
         }
     }
@@ -335,7 +335,7 @@ public class NodeStateChangeChecker {
         } else {
             // Return a disallow-result if there is another node with a wanted state
             var otherNodeHasWantedState = otherNodeHasWantedState(nodeInfo);
-            if ( ! otherNodeHasWantedState.settingWantedStateIsAllowed())
+            if (otherNodeHasWantedState.settingWantedStateIsNotAllowed())
                 return Optional.of(otherNodeHasWantedState);
         }
         return Optional.empty();
@@ -484,7 +484,8 @@ public class NodeStateChangeChecker {
         return allowSettingOfWantedState();
     }
 
-    private Result checkStorageNodesForDistributor(DistributorNodeInfo distributorNodeInfo, List<StorageNode> storageNodes, Node node) {
+    private Result checkStorageNodesForDistributor(DistributorNodeInfo distributorNodeInfo, Node node) {
+        List<StorageNode> storageNodes = distributorNodeInfo.getHostInfo().getDistributor().getStorageNodes();
         for (StorageNode storageNode : storageNodes) {
             if (storageNode.getIndex() == node.getIndex()) {
                 Integer minReplication = storageNode.getMinCurrentReplicationFactorOrNull();
@@ -526,9 +527,8 @@ public class NodeStateChangeChecker {
                                                + ") as fleetcontroller (" + clusterStateVersion + ")");
             }
 
-            List<StorageNode> storageNodes = distributorNodeInfo.getHostInfo().getDistributor().getStorageNodes();
-            Result storageNodesResult = checkStorageNodesForDistributor(distributorNodeInfo, storageNodes, node);
-            if (!storageNodesResult.settingWantedStateIsAllowed()) {
+            Result storageNodesResult = checkStorageNodesForDistributor(distributorNodeInfo, node);
+            if (storageNodesResult.settingWantedStateIsNotAllowed()) {
                 return storageNodesResult;
             }
         }
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 c4fd7cb69b9..43687d51937 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
@@ -13,7 +13,6 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.List;
 
 import static com.yahoo.vdslib.state.NodeType.DISTRIBUTOR;
@@ -168,12 +167,9 @@ public class NodeStateChangeCheckerTest {
 
     @Test
     void testMaintenanceAllowedFor2Of4Groups() {
-        // 4 groups with 1 node in each group
-        Collection<ConfiguredNode> nodes = createNodes(4);
-        StorDistributionConfig config = createDistributionConfig(4, 4);
-
         int maxNumberOfGroupsAllowedToBeDown = 2;
-        var cluster = new ContentCluster("Clustername", nodes, new Distribution(config), maxNumberOfGroupsAllowedToBeDown);
+        // 4 groups with 1 node in each group
+        var cluster = createCluster(4, 4, maxNumberOfGroupsAllowedToBeDown);
         setAllNodesUp(cluster, HostInfo.createHostInfo(createDistributorHostInfo(4, 5, 6)));
         var nodeStateChangeChecker = createChangeChecker(cluster);
 
@@ -219,12 +215,9 @@ public class NodeStateChangeCheckerTest {
 
     @Test
     void testMaintenanceAllowedFor2Of4Groups8Nodes() {
-        // 4 groups with 2 nodes in each group
-        Collection<ConfiguredNode> nodes = createNodes(8);
-        StorDistributionConfig config = createDistributionConfig(8, 4);
-
         int maxNumberOfGroupsAllowedToBeDown = 2;
-        var cluster = new ContentCluster("Clustername", nodes, new Distribution(config), maxNumberOfGroupsAllowedToBeDown);
+        // 4 groups with 2 nodes in each group
+        var cluster = createCluster(8, 4, maxNumberOfGroupsAllowedToBeDown);
         setAllNodesUp(cluster, HostInfo.createHostInfo(createDistributorHostInfo(4, 5, 6)));
         var nodeStateChangeChecker = createChangeChecker(cluster);
 
@@ -271,6 +264,34 @@ public class NodeStateChangeCheckerTest {
             setStorageNodeWantedStateToMaintenance(cluster, nodeIndex);
         }
 
+        // 2 nodes in group 0 and 2 nodes in group 1 in maintenance, try to set storage node 4 in group 2 to maintenance, should fail
+        {
+            ClusterState clusterState = clusterState(String.format("version:%d distributor:8 storage:8 .0.s:m .1.s:m .2.s:m .3.s:m", currentClusterStateVersion));
+            int nodeIndex = 4;
+            Node node = new Node(STORAGE, nodeIndex);
+            Result result = nodeStateChangeChecker.evaluateTransition(node, clusterState, SAFE, UP_NODE_STATE, MAINTENANCE_NODE_STATE);
+            assertFalse(result.settingWantedStateIsAllowed(), result.toString());
+            assertFalse(result.wantedStateAlreadySet());
+            assertEquals("At most 2 groups can have wanted state: [0, 1]", result.getReason());
+        }
+
+        // 2 nodes in group 0 up again but buckets not in sync and 2 nodes in group 1 in maintenance,
+        // try to set storage node 4 in group 2 to maintenance
+        /* WIP
+        {
+            ClusterState clusterState = clusterState(String.format("version:%d distributor:8 storage:8 .2.s:m .3.s:m", currentClusterStateVersion));
+            setStorageNodeWantedState(cluster, 0, UP, "");
+            setStorageNodeWantedState(cluster, 1, UP, "");
+            int nodeIndex = 4;
+            Node node = new Node(STORAGE, nodeIndex);
+            Result result = nodeStateChangeChecker.evaluateTransition(node, clusterState, SAFE, UP_NODE_STATE, MAINTENANCE_NODE_STATE);
+            assertFalse(result.settingWantedStateIsAllowed(), result.toString());
+            assertFalse(result.wantedStateAlreadySet());
+            assertEquals("At most 2 groups can have wanted state: [0, 1]", result.getReason());
+        }
+
+         */
+
         // 2 nodes in group 0 in maintenance, storage node 3 in group 1 is in maintenance with another description
         // (set in maintenance by operator), try to set storage node 3 in group 1 to maintenance, should bew allowed
         {