Moved setReady from NodesApiHandler to NodeRepository

4 files changed, 25 insertions, 35 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
index 59f24c01a91..778c14229a6 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
@@ -97,9 +97,9 @@ public class NodeRepository extends AbstractComponent {
 
     // ---------------- Query API ----------------------------------------------------------------
 
-    /** 
+    /**
      * Finds and returns the node with the hostname in any of the given states, or empty if not found 
-     * 
+     *
      * @param hostname the full host name of the node
      * @param inState the states the node may be in. If no states are given, it will be returned from any state
      * @return the node, or empty if it was not found in any of the given states
@@ -131,13 +131,13 @@ public class NodeRepository extends AbstractComponent {
     /**
      * Finds and returns all nodes that are children of the given parent node
      *
-     * @param parent Parent node
+     * @param hostname Parent hostname
      * @return List of child nodes
      */
-    public List<Node> getNodes(Node parent) {
+    public List<Node> getChildNodes(String hostname) {
         return zkClient.getNodes().stream()
                 .filter(node -> node.parentHostname()
-                        .map(parentHostname -> parentHostname.equals(parent.hostname()))
+                        .map(parentHostname -> parentHostname.equals(hostname))
                         .orElse(false))
                 .collect(Collectors.toList());
     }
@@ -209,7 +209,7 @@ public class NodeRepository extends AbstractComponent {
         final List<NodeAcl> nodeAcls = new ArrayList<>();
 
         if (children) {
-            final List<Node> childNodes = getNodes(node);
+            final List<Node> childNodes = getChildNodes(node.hostname());
             childNodes.forEach(childNode -> nodeAcls.add(new NodeAcl(childNode, getTrustedNodes(childNode))));
         } else {
             nodeAcls.add(new NodeAcl(node, getTrustedNodes(node)));
@@ -263,6 +263,14 @@ public class NodeRepository extends AbstractComponent {
         }
     }
 
+    public Node setReady(String hostname) {
+        Node nodeToReady = getNode(hostname).orElseThrow(() ->
+                new NotFoundException("Could not move " + hostname + " to ready: Node not found"));
+
+        if (nodeToReady.state() == Node.State.ready) return nodeToReady;
+        return setReady(Collections.singletonList(nodeToReady)).get(0);
+    }
+
     /** Reserve nodes. This method does <b>not</b> lock the node repository */
     public List<Node> reserve(List<Node> nodes) { return zkClient.writeTo(Node.State.reserved, nodes); }
 
@@ -288,8 +296,8 @@ public class NodeRepository extends AbstractComponent {
 
     public void deactivate(ApplicationId application, NestedTransaction transaction) {
         try (Mutex lock = lock(application)) {
-            zkClient.writeTo(Node.State.inactive, 
-                             zkClient.getNodes(application, Node.State.reserved, Node.State.active), 
+            zkClient.writeTo(Node.State.inactive,
+                             zkClient.getNodes(application, Node.State.reserved, Node.State.active),
                              transaction);
         }
     }
@@ -308,7 +316,7 @@ public class NodeRepository extends AbstractComponent {
         return performOn(NodeListFilter.from(nodes), node -> zkClient.writeTo(Node.State.dirty, node));
     }
 
-    /** 
+    /**
      * Set a node dirty, which is in the provisioned, failed or parked state.
      * Use this to clean newly provisioned nodes or to recycle failed nodes which have been repaired or put on hold.
      *
@@ -484,7 +492,7 @@ public class NodeRepository extends AbstractComponent {
 
     /** Returns the time keeper of this system */
     public Clock clock() { return clock; }
-    
+
     /** Create a lock which provides exclusive rights to making changes to the given application */
     public Mutex lock(ApplicationId application) { return zkClient.lock(application); }
 
@@ -498,5 +506,5 @@ public class NodeRepository extends AbstractComponent {
     private Mutex lock(Node node) {
         return node.allocation().isPresent() ? lock(node.allocation().get().owner()) : lockUnallocated();
     }
-    
+
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java
index f65b471dc20..5714faeda9a 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java
@@ -224,7 +224,7 @@ public class NodeFailer extends Maintainer {
             // If the active node that we are trying to fail is of type host, we need to successfully fail all
             // the children nodes running on it before we fail the host
             boolean allTenantNodesFailedOutSuccessfully = true;
-            for (Node failingTenantNode : nodeRepository().getNodes(node)) {
+            for (Node failingTenantNode : nodeRepository().getChildNodes(node.hostname())) {
                 if (failingTenantNode.state() == Node.State.active) {
                     allTenantNodesFailedOutSuccessfully &= failActive(failingTenantNode);
                 } else {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java
index 4b72ae62889..301189ced4d 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java
@@ -27,7 +27,6 @@ import com.yahoo.yolean.Exceptions;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
@@ -96,7 +95,8 @@ public class NodesApiHandler extends LoggingRequestHandler {
         String path = request.getUri().getPath();
         // Check paths to disallow illegal state changes
         if (path.startsWith("/nodes/v2/state/ready/")) {
-            return new MessageResponse(setNodeReady(path));
+            nodeRepository.setReady(lastElement(path));
+            return new MessageResponse("Moved " + lastElement(path) + " to ready");
         }
         else if (path.startsWith("/nodes/v2/state/failed/")) {
             nodeRepository.fail(lastElement(path));
@@ -214,24 +214,6 @@ public class NodesApiHandler extends LoggingRequestHandler {
         }
     }
 
-    // TODO: Move most of this to node repo
-    public String setNodeReady(String path) {
-        String hostname = lastElement(path);
-        if ( nodeRepository.getNode(hostname, Node.State.ready).isPresent())
-            return "Nothing done; " + hostname + " is already ready";
-
-        Optional<Node> node = nodeRepository.getNode(hostname, Node.State.provisioned, Node.State.dirty, Node.State.failed, Node.State.parked);
-
-        if ( ! node.isPresent())
-            throw new IllegalArgumentException("Could not set " + hostname + " ready: Not registered as provisioned, dirty, failed or parked");
-
-        if (node.get().allocation().isPresent())
-            throw new IllegalArgumentException("Could not set " + hostname + " ready: Node is allocated and must be moved to dirty instead");
-
-        nodeRepository.setReady(Collections.singletonList(node.get()));
-        return "Moved " + hostname + " to ready";
-    }
-
     public static NodeFilter toNodeFilter(HttpRequest request) {
         NodeFilter filter = NodeHostFilter.from(HostFilter.from(request.getProperty("hostname"),
                                                                 request.getProperty("flavor"),
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
index f89ccc3c53c..cde77c68433 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
@@ -94,7 +94,7 @@ public class RestApiTest {
         // calling ready again is a noop:
         assertResponse(new Request("http://localhost:8080/nodes/v2/state/ready/host8.yahoo.com",
                                   new byte[0], Request.Method.PUT),
-                       "{\"message\":\"Nothing done; host8.yahoo.com is already ready\"}");
+                       "{\"message\":\"Moved host8.yahoo.com to ready\"}");
 
         // PUT a node in failed ...
         assertResponse(new Request("http://localhost:8080/nodes/v2/state/failed/host3.yahoo.com",
@@ -312,7 +312,7 @@ public class RestApiTest {
                        "{\"message\":\"Moved host1.yahoo.com to failed\"}");
         assertResponse(new Request("http://localhost:8080/nodes/v2/state/ready/host1.yahoo.com",
                                    new byte[0], Request.Method.PUT),
-                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Could not set host1.yahoo.com ready: Node is allocated and must be moved to dirty instead\"}");
+                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Can not set failed node host1.yahoo.com allocated to tenant 'tenant2', application 'application2', instance 'instance2' as 'content/id2/0/0' ready. It is not dirty.\"}");
         // (... while dirty then ready works (the ready move will be initiated by node maintenance))
         assertResponse(new Request("http://localhost:8080/nodes/v2/state/dirty/host1.yahoo.com",
                                    new byte[0], Request.Method.PUT),
@@ -327,7 +327,7 @@ public class RestApiTest {
                        "{\"message\":\"Moved host2.yahoo.com to parked\"}");
         assertResponse(new Request("http://localhost:8080/nodes/v2/state/ready/host2.yahoo.com",
                                    new byte[0], Request.Method.PUT),
-                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Could not set host2.yahoo.com ready: Node is allocated and must be moved to dirty instead\"}");
+                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Can not set parked node host2.yahoo.com allocated to tenant 'tenant2', application 'application2', instance 'instance2' as 'content/id2/0/1' ready. It is not dirty.\"}");
         // (... while dirty then ready works (the ready move will be initiated by node maintenance))
         assertResponse(new Request("http://localhost:8080/nodes/v2/state/dirty/host2.yahoo.com",
                                    new byte[0], Request.Method.PUT),