Extract common check

2 files changed, 31 insertions, 23 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 7f2d9ba9611..c9d9645735c 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
@@ -99,6 +99,11 @@ public class NodeStateChangeChecker {
                     "Requested node type: " + node.getType().toString());
         }
 
+        NodeInfo nodeInfo = clusterInfo.getStorageNodeInfo(node.getIndex());
+        if (nodeInfo == null) {
+            return Result.createDisallowed("Unknown node " + node);
+        }
+
         // If the new state and description equals the existing, we're done. This is done for 2 cases:
         // - We can short-circuit setting of a new wanted state, which e.g. hits ZooKeeper.
         // - We ensure that clients that have previously set the wanted state, continue
@@ -110,32 +115,27 @@ public class NodeStateChangeChecker {
 
         switch (newState.getState()) {
             case UP:
-                return canSetStateUp(node, oldState.getState());
+                return canSetStateUp(nodeInfo, oldState.getState());
             case MAINTENANCE:
                 return canSetStateMaintenanceTemporarily(node, clusterState);
             case DOWN:
-                return canSetStateDownPermanently(node, clusterState);
+                return canSetStateDownPermanently(nodeInfo, clusterState);
             default:
-                return Result.createDisallowed("Safe only supports state UP and MAINTENANCE, you tried: " + newState);
+                return Result.createDisallowed("Destination node state unsupported in safe mode: " + newState);
         }
     }
 
-    private Result canSetStateDownPermanently(Node node, ClusterState clusterState) {
-        StorageNodeInfo nodeInfo = clusterInfo.getStorageNodeInfo(node.getIndex());
-        if (nodeInfo == null) {
-            return Result.createDisallowed("Unknown node " + node);
-        }
-
+    private Result canSetStateDownPermanently(NodeInfo nodeInfo, ClusterState clusterState) {
         State reportedState = nodeInfo.getReportedState().getState();
         if (reportedState != State.UP) {
             return Result.createDisallowed("Reported state (" + reportedState
                     + ") is not UP, so no bucket data is available");
         }
 
-        NodeState currentState = clusterState.getNodeState(node);
-        if (currentState.getState() != State.RETIRED) {
+        State currentState = clusterState.getNodeState(nodeInfo.getNode()).getState();
+        if (currentState != State.RETIRED) {
             return Result.createDisallowed("Only retired nodes are allowed to be set to DOWN in safe mode - is "
-                    + currentState.getState());
+                    + currentState);
         }
 
         Result thresholdCheckResult = checkUpThresholds(clusterState);
@@ -148,14 +148,14 @@ public class NodeStateChangeChecker {
         int clusterControllerVersion = clusterState.getVersion();
         if (hostInfoNodeVersion == null || hostInfoNodeVersion != clusterControllerVersion) {
             return Result.createDisallowed("Cluster controller at version " + clusterControllerVersion
-                    + " got info for storage node " + node.getIndex() + " at a different version "
+                    + " got info for storage node " + nodeInfo.getNodeIndex() + " at a different version "
                     + hostInfoNodeVersion);
         }
 
         Optional<Metrics.Value> bucketsMetric = hostInfo.getMetrics().getValue(BUCKETS_METRIC_NAME);
         if (!bucketsMetric.isPresent() || bucketsMetric.get().getLast() == null) {
             return Result.createDisallowed("Missing last value of the " + BUCKETS_METRIC_NAME +
-                    " metric for storage node " + node.getIndex());
+                    " metric for storage node " + nodeInfo.getNodeIndex());
         }
 
         long lastBuckets = bucketsMetric.get().getLast();
@@ -166,27 +166,22 @@ public class NodeStateChangeChecker {
         return Result.allowSettingOfWantedState();
     }
 
-    private Result canSetStateUp(Node node, State oldState) {
+    private Result canSetStateUp(NodeInfo nodeInfo, State oldState) {
         if (oldState != State.MAINTENANCE) {
             return Result.createDisallowed("Refusing to set wanted state to up when it is currently in " + oldState);
         }
 
-        if (clusterInfo.getNodeInfo(node).getReportedState().getState() != State.UP) {
+        if (nodeInfo.getReportedState().getState() != State.UP) {
             return Result.createDisallowed("Refuse to set wanted state to UP, " +
                     "since the reported state is not UP (" +
-                    clusterInfo.getNodeInfo(node).getReportedState().getState() + ")");
+                    nodeInfo.getReportedState().getState() + ")");
         }
 
         return Result.allowSettingOfWantedState();
     }
 
     private Result canSetStateMaintenanceTemporarily(Node node, ClusterState clusterState) {
-        NodeInfo nodeInfo = clusterInfo.getNodeInfo(node);
-        if (nodeInfo == null) {
-            return Result.createDisallowed("Unknown node " + node);
-        }
-        NodeState currentState = clusterState.getNodeState(node);
-        if (currentState.getState() == State.DOWN) {
+        if (clusterState.getNodeState(node).getState() == State.DOWN) {
             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 2f89a9d0c27..0d2f12dfdcd 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
@@ -133,6 +133,19 @@ public class NodeStateChangeCheckerTest {
     }
 
     @Test
+    public void testUnknownStorageNode() {
+        ContentCluster cluster = createCluster(createNodes(4));
+        NodeStateChangeChecker nodeStateChangeChecker = new NodeStateChangeChecker(
+                5 /* min storage nodes */, minRatioOfStorageNodesUp, requiredRedundancy, cluster.clusterInfo());
+        NodeStateChangeChecker.Result result = nodeStateChangeChecker.evaluateTransition(
+                new Node(NodeType.STORAGE, 10), defaultAllUpClusterState(), SetUnitStateRequest.Condition.SAFE,
+                UP_NODE_STATE, MAINTENANCE_NODE_STATE);
+        assertFalse(result.settingWantedStateIsAllowed());
+        assertFalse(result.wantedStateAlreadySet());
+        assertThat(result.getReason(), is("Unknown node storage.10"));
+    }
+
+    @Test
     public void testSafeSetStateDistributors() {
         NodeStateChangeChecker nodeStateChangeChecker = createChangeChecker(createCluster(createNodes(1)));
         NodeStateChangeChecker.Result result = nodeStateChangeChecker.evaluateTransition(