Disallow removal of allocated nodes

2 files changed, 34 insertions, 21 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 8710b47b914..3b133e9f6c8 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
@@ -31,9 +31,9 @@ import com.yahoo.vespa.hosted.provision.restapi.v2.NotFoundException;
 import java.time.Clock;
 import java.time.Duration;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.EnumSet;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -149,7 +149,7 @@ public class NodeRepository extends AbstractComponent {
      * @return the node, or empty if it was not found in any of the given states
      */
     public List<Node> getNodes(Node.State ... inState) {
-        return db.getNodes(inState).stream().collect(Collectors.toList());
+        return new ArrayList<>(db.getNodes(inState));
     }
     /**
      * Finds and returns the nodes of the given type in any of the given states.
@@ -544,13 +544,15 @@ public class NodeRepository extends AbstractComponent {
 
     private List<Node> removeRecursively(Node node, boolean force) {
         try (Mutex lock = lockUnallocated()) {
-            List<Node> removed = !node.type().isDockerHost() ?
-                    new ArrayList<>() :
-                    getChildNodes(node.hostname()).stream()
-                            .filter(child -> force || verifyRemovalIsAllowed(child, true))
-                            .collect(Collectors.toList());
+            List<Node> removed = new ArrayList<>();
 
-            if (force || verifyRemovalIsAllowed(node, false)) removed.add(node);
+             if (node.type().isDockerHost()) {
+                 getChildNodes(node.hostname()).stream()
+                                               .filter(child -> force || canRemove(child, true))
+                                               .forEach(removed::add);
+             }
+
+            if (force || canRemove(node, false)) removed.add(node);
             db.removeNodes(removed);
 
             return removed;
@@ -560,32 +562,38 @@ public class NodeRepository extends AbstractComponent {
     }
 
     /**
-     * Allowed to a node delete if:
-     *  Non-docker-container node: iff in state provisioned|failed|parked
+     * Returns whether given node can be removed. Removal is allowed if:
+     *  Tenant node: node is unallocated
+     *  Non-Docker-container node: iff in state provisioned|failed|parked
      *  Docker-container-node:
      *    If only removing the container node: node in state ready
      *    If also removing the parent node: child is in state provisioned|failed|parked|ready
      */
-    private boolean verifyRemovalIsAllowed(Node nodeToRemove, boolean deletingAsChild) {
-        if (nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER && !deletingAsChild) {
-            if (nodeToRemove.state() != Node.State.ready) {
+    private boolean canRemove(Node node, boolean deletingAsChild) {
+        if (node.type() == NodeType.tenant && node.allocation().isPresent()) {
+            throw new IllegalArgumentException("Node is currently allocated and cannot be removed: " +
+                                               node.allocation().get());
+        }
+        if (node.flavor().getType() == Flavor.Type.DOCKER_CONTAINER && !deletingAsChild) {
+            if (node.state() != Node.State.ready) {
                 throw new IllegalArgumentException(
-                        String.format("Docker container node %s can only be removed when in state ready", nodeToRemove.hostname()));
+                        String.format("Docker container %s can only be removed when in ready state", node.hostname()));
             }
 
-        } else if (nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER) {
-            List<Node.State> legalStates = Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked, Node.State.ready);
+        } else if (node.flavor().getType() == Flavor.Type.DOCKER_CONTAINER) {
+            Set<Node.State> legalStates = EnumSet.of(Node.State.provisioned, Node.State.failed, Node.State.parked,
+                                                     Node.State.ready);
 
-            if (! legalStates.contains(nodeToRemove.state())) {
+            if (! legalStates.contains(node.state())) {
                 throw new IllegalArgumentException(String.format("Child node %s can only be removed from following states: %s",
-                        nodeToRemove.hostname(), legalStates.stream().map(Node.State::name).collect(Collectors.joining(", "))));
+                        node.hostname(), legalStates.stream().map(Node.State::name).collect(Collectors.joining(", "))));
             }
         } else {
-            List<Node.State> legalStates = Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked);
+            Set<Node.State> legalStates = EnumSet.of(Node.State.provisioned, Node.State.failed, Node.State.parked);
 
-            if (! legalStates.contains(nodeToRemove.state())) {
+            if (! legalStates.contains(node.state())) {
                 throw new IllegalArgumentException(String.format("Node %s can only be removed from following states: %s",
-                        nodeToRemove.hostname(), legalStates.stream().map(Node.State::name).collect(Collectors.joining(", "))));
+                        node.hostname(), legalStates.stream().map(Node.State::name).collect(Collectors.joining(", "))));
             }
         }
 
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 ec09805ff5d..bee9bf3625a 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
@@ -405,6 +405,11 @@ public class RestApiTest {
                                    new byte[0], Request.Method.DELETE),
                        400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Failed to delete host2.yahoo.com: Node host2.yahoo.com can only be removed from following states: provisioned, failed, parked\"}");
 
+        // Attempt to DELETE allocated node
+        assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
+                                   new byte[0], Request.Method.DELETE),
+                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Failed to delete host4.yahoo.com: Node is currently allocated and cannot be removed: allocated to tenant3.application3.instance3 as 'content/id3/0/0'\"}");
+
         // PUT current restart generation with string instead of long
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
                                    Utf8.toBytes("{\"currentRestartGeneration\": \"1\"}"), Request.Method.PATCH),