Revert "Move node from provisioned to dirty"

4 files changed, 33 insertions, 53 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 8eac3c1c467..7fa25d30647 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
@@ -54,7 +54,7 @@ import java.util.stream.Collectors;
  * @author bratseth
  */
 // Node state transitions:
-// 1) (new) - > provisioned -> dirty -> ready -> reserved -> active -> inactive -> dirty -> ready
+// 1) (new) - > provisioned -> ready -> reserved -> active -> inactive -> dirty -> ready
 // 2) inactive -> reserved
 // 3) reserved -> dirty
 // 3) * -> failed | parked -> dirty | active | (removed)
@@ -201,7 +201,7 @@ public class NodeRepository extends AbstractComponent {
         }
     }
 
-    // Visible for testing purposes
+    /** Sets a list of nodes ready and returns the nodes in the ready state */
     public List<Node> setReady(List<Node> nodes) {
         for (Node node : nodes)
             if (node.state() != Node.State.provisioned && node.state() != Node.State.dirty)
@@ -211,28 +211,6 @@ public class NodeRepository extends AbstractComponent {
         }
     }
 
-    /**
-     * Set a node ready. The node may be moved to a intermediate state (e.g. dirty) and require a subsequent call to
-     * move to ready.
-     */
-    public Node setReady(Node node) {
-        List<Node.State> legalStates = Arrays.asList(Node.State.provisioned, Node.State.dirty, Node.State.failed, Node.State.parked);
-        if (!legalStates.contains(node.state())) {
-            throw new IllegalArgumentException("Could not set " + node.hostname() + " ready: Not registered as provisioned, " +
-                    "dirty, failed or parked");
-        }
-        if (node.allocation().isPresent()) {
-            throw new IllegalArgumentException("Could not set " + node.hostname() + " ready: Node is allocated and must be " +
-                    "moved to dirty instead");
-        }
-        // Provisioned nodes are moved to dirty. This ensures that any user data is cleaned up and that all packages and
-        // kernel are upgraded.
-        if (node.state() == Node.State.provisioned) {
-            return deallocate(Collections.singletonList(node)).get(0);
-        }
-        return setReady(Collections.singletonList(node)).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); }
 
@@ -297,7 +275,7 @@ public class NodeRepository extends AbstractComponent {
      * Fails this node and returns it in its new state.
      *
      * @return the node in its new state
-     * @throws NoSuchNodeException if the node is not found
+     * @throws NotFoundException if the node is not found
      */
     public Node fail(String hostname) {
         return move(hostname, Node.State.failed);
@@ -307,7 +285,7 @@ public class NodeRepository extends AbstractComponent {
      * Parks this node and returns it in its new state.
      *
      * @return the node in its new state
-     * @throws NoSuchNodeException if the node is not found
+     * @throws NotFoundException if the node is not found
      */
     public Node park(String hostname) {
         return move(hostname, Node.State.parked);
@@ -317,7 +295,7 @@ public class NodeRepository extends AbstractComponent {
      * Moves a previously failed or parked node back to the active state.
      *
      * @return the node in its new state
-     * @throws NoSuchNodeException if the node is not found
+     * @throws NotFoundException if the node is not found
      */
     public Node reactivate(String hostname) {
         return move(hostname, Node.State.active);
@@ -326,7 +304,7 @@ public class NodeRepository extends AbstractComponent {
     public Node move(String hostname, Node.State toState) {
         Optional<Node> node = getNode(hostname);
         if ( ! node.isPresent())
-            throw new NoSuchNodeException("Could not move " + hostname + " to " + toState + ": Node not found");
+            throw new NotFoundException("Could not move " + hostname + " to " + toState + ": Node not found");
         try (Mutex lock = lock(node.get())) {
             return zkClient.writeTo(toState, node.get());
         }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NoSuchNodeException.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NotFoundException.java
index 355116586d9..0e76c0760e2 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NoSuchNodeException.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NotFoundException.java
@@ -6,8 +6,10 @@ package com.yahoo.vespa.hosted.provision;
  *
  * @author musum
  */
-public class NoSuchNodeException extends RuntimeException {
+public class NotFoundException extends RuntimeException {
 
-    public NoSuchNodeException(String message) { super(message); }
+    public NotFoundException(String message) { super(message); }
+
+    public NotFoundException(String message, Throwable cause) { super(message, cause); }
 
 }
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 eb4b4fbff44..715f96e8923 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
@@ -12,7 +12,6 @@ import com.yahoo.slime.ArrayTraverser;
 import com.yahoo.slime.Inspector;
 import com.yahoo.slime.Slime;
 import com.yahoo.vespa.config.SlimeUtils;
-import com.yahoo.vespa.hosted.provision.NoSuchNodeException;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.node.NodeFlavors;
@@ -28,6 +27,7 @@ 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;
@@ -67,7 +67,7 @@ public class NodesApiHandler extends LoggingRequestHandler {
                 case PATCH: return handlePATCH(request);
                 default: return ErrorResponse.methodNotAllowed("Method '" + request.getMethod() + "' is not supported");
             }
-        } catch (NotFoundException | NoSuchNodeException e) {
+        } catch (NotFoundException | com.yahoo.vespa.hosted.provision.NotFoundException e) {
             return ErrorResponse.notFoundError(Exceptions.toMessageString(e));
         } catch (IllegalArgumentException e) {
             return ErrorResponse.badRequest(Exceptions.toMessageString(e));
@@ -95,7 +95,7 @@ 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(maybeSetNodeReady(path));
+            return new MessageResponse(setNodeReady(path));
         }
         else if (path.startsWith("/nodes/v2/state/failed/")) {
             nodeRepository.fail(lastElement(path));
@@ -216,17 +216,22 @@ public class NodesApiHandler extends LoggingRequestHandler {
         }
     }
 
-    private String maybeSetNodeReady(String path) {
+    // TODO: Move most of this to node repo
+    public String setNodeReady(String path) {
         String hostname = lastElement(path);
-        Optional<Node> node = nodeRepository.getNode(hostname);
-        if (!node.isPresent()) {
-            throw new NotFoundException("No node with hostname '" + hostname + "'");
-        }
-        if (node.get().state() == Node.State.ready) {
+        if ( nodeRepository.getNode(hostname, Node.State.ready).isPresent())
             return "Nothing done; " + hostname + " is already ready";
-        }
-        Node updatedNode = nodeRepository.setReady(node.get());
-        return "Moved " + hostname + " to " + updatedNode.state().name();
+
+        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) {
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 ec5ac266330..cc28c90b9c4 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
@@ -82,21 +82,16 @@ public class RestApiTest {
         assertFile(new Request("http://localhost:8080/nodes/v2/node/host11.yahoo.com"), "node11.json");
         assertFile(new Request("http://localhost:8080/nodes/v2/node/parent2.yahoo.com"), "parent2.json");
 
-        // PUT provisioned node ready moves the node to dirty
+        // PUT nodes ready
         assertResponse(new Request("http://localhost:8080/nodes/v2/state/ready/host8.yahoo.com",
                                    new byte[0], Request.Method.PUT),
-                       "{\"message\":\"Moved host8.yahoo.com to dirty\"}");
+                       "{\"message\":\"Moved host8.yahoo.com to ready\"}");
         assertResponseContains(new Request("http://localhost:8080/nodes/v2/node/host8.yahoo.com"),
-                                           "\"state\":\"dirty\"");
-        // calling ready again readies the node
-        assertResponse(new Request("http://localhost:8080/nodes/v2/state/ready/host8.yahoo.com",
-                                  new byte[0], Request.Method.PUT),
-                "{\"message\":\"Moved host8.yahoo.com to ready\"}");
-
+                                           "\"state\":\"ready\"");
         // 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\"}");
+                                  new byte[0], Request.Method.PUT),
+                       "{\"message\":\"Nothing done; host8.yahoo.com is already ready\"}");
 
         // PUT a node in failed ...
         assertResponse(new Request("http://localhost:8080/nodes/v2/state/failed/host8.yahoo.com",
@@ -255,7 +250,7 @@ public class RestApiTest {
                 "{\"message\":\"Added 1 nodes to the provisioned state\"}");
         assertResponse(new Request("http://localhost:8080/nodes/v2/state/ready/" + hostname,
                                    new byte[0], Request.Method.PUT),
-                       "{\"message\":\"Moved foo.yahoo.com to dirty\"}");
+                       "{\"message\":\"Moved foo.yahoo.com to ready\"}");
         Pattern responsePattern = Pattern.compile("\\{\"trustedNodes\":\\[" +
                 "\\{\"hostname\":\"cfg1\",\"ipAddress\":\".+?\"}," +
                 "\\{\"hostname\":\"cfg2\",\"ipAddress\":\".+?\"}," +