Merge pull request #8123 from vespa-engine/mpolden/disallow-removing-allocated-nodes

Disallow removal of allocated nodes

10 files changed, 66 insertions, 58 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
index 5f0bac7f194..5ef5a46dc45 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
@@ -86,14 +86,18 @@ public class NodeList {
     }
 
     /** Returns the child nodes of the given parent node */
-    public NodeList childrenOf(Node parent) {
+    public NodeList childrenOf(String hostname) {
         return nodes.stream()
                     .filter(n -> n.parentHostname()
-                                  .map(hostName -> hostName.equals(parent.hostname()))
+                                  .map(hostName -> hostName.equals(hostname))
                                   .orElse(false))
                     .collect(collectingAndThen(Collectors.toList(), NodeList::new));
     }
 
+    public NodeList childrenOf(Node parent) {
+        return childrenOf(parent.hostname());
+    }
+
     public int size() { return nodes.size(); }
 
     /** Returns the immutable list of nodes in this */
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..dca4ea331fd 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.
@@ -162,18 +162,9 @@ public class NodeRepository extends AbstractComponent {
         return db.getNodes(inState).stream().filter(node -> node.type().equals(type)).collect(Collectors.toList());
     }
 
-    /**
-     * Finds and returns all nodes that are children of the given parent node
-     *
-     * @param hostname Parent hostname
-     * @return List of child nodes
-     */
-    public List<Node> getChildNodes(String hostname) {
-        return db.getNodes().stream()
-                .filter(node -> node.parentHostname()
-                        .map(parentHostname -> parentHostname.equals(hostname))
-                        .orElse(false))
-                .collect(Collectors.toList());
+    /** Returns a filterable list of all nodes in this repository */
+    public NodeList list() {
+        return new NodeList(getNodes());
     }
 
     public List<Node> getNodes(ApplicationId id, Node.State ... inState) { return db.getNodes(id, inState); }
@@ -255,7 +246,7 @@ public class NodeRepository extends AbstractComponent {
      * @return List of node ACLs
      */
     public List<NodeAcl> getNodeAcls(Node node, boolean children) {
-        NodeList candidates = new NodeList(getNodes());
+        NodeList candidates = list();
         if (children) {
             return candidates.childrenOf(node).asList().stream()
                              .map(childNode -> getNodeAcl(childNode, candidates))
@@ -413,7 +404,7 @@ public class NodeRepository extends AbstractComponent {
 
         List<Node> nodesToDirty =
                 (nodeToDirty.type().isDockerHost() ?
-                        Stream.concat(getChildNodes(hostname).stream(), Stream.of(nodeToDirty)) :
+                        Stream.concat(list().childrenOf(hostname).asList().stream(), Stream.of(nodeToDirty)) :
                         Stream.of(nodeToDirty))
                 .filter(node -> node.state() != Node.State.dirty)
                 .collect(Collectors.toList());
@@ -483,9 +474,9 @@ public class NodeRepository extends AbstractComponent {
     }
 
     private List<Node> moveRecursively(String hostname, Node.State toState, Agent agent, Optional<String> reason) {
-        List<Node> moved = getChildNodes(hostname).stream()
-                .map(child -> move(child, toState, agent, reason))
-                .collect(Collectors.toList());
+        List<Node> moved = list().childrenOf(hostname).asList().stream()
+                                         .map(child -> move(child, toState, agent, reason))
+                                         .collect(Collectors.toList());
 
         moved.add(move(hostname, toState, agent, reason));
         return moved;
@@ -544,13 +535,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 (node.type().isDockerHost()) {
+                 list().childrenOf(node).asList().stream()
+                       .filter(child -> force || canRemove(child, true))
+                       .forEach(removed::add);
+             }
 
-            if (force || verifyRemovalIsAllowed(node, false)) removed.add(node);
+            if (force || canRemove(node, false)) removed.add(node);
             db.removeNodes(removed);
 
             return removed;
@@ -560,32 +553,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/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java
index 01eef3c2aa1..280f723f268 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java
@@ -101,10 +101,10 @@ public class FailedExpirer extends Maintainer {
         for (Node candidate : nodes) {
             if (hasHardwareIssue(candidate)) {
                 List<String> unparkedChildren = !candidate.type().isDockerHost() ? Collections.emptyList() :
-                        nodeRepository.getChildNodes(candidate.hostname()).stream()
-                                .filter(node -> node.state() != Node.State.parked)
-                                .map(Node::hostname)
-                                .collect(Collectors.toList());
+                        nodeRepository.list().childrenOf(candidate).asList().stream()
+                                      .filter(node -> node.state() != Node.State.parked)
+                                      .map(Node::hostname)
+                                      .collect(Collectors.toList());
 
                 if (unparkedChildren.isEmpty()) {
                     nodeRepository.park(candidate.hostname(), Agent.system,
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 0dabe252406..17351ab3c10 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
@@ -298,7 +298,7 @@ public class NodeFailer extends Maintainer {
             // the children nodes running on it before we fail the host
             boolean allTenantNodesFailedOutSuccessfully = true;
             String reasonForChildFailure = "Failing due to parent host " + node.hostname() + " failure: " + reason;
-            for (Node failingTenantNode : nodeRepository().getChildNodes(node.hostname())) {
+            for (Node failingTenantNode : nodeRepository().list().childrenOf(node).asList()) {
                 if (failingTenantNode.state() == Node.State.active) {
                     allTenantNodesFailedOutSuccessfully &= failActive(failingTenantNode, reasonForChildFailure);
                 } else {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java
index 6036399d83e..dcd623a3212 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java
@@ -93,9 +93,9 @@ public class RetiredExpirer extends Maintainer {
     private boolean canRemove(Node node) {
         if (node.type().isDockerHost()) {
             if (nodeRepository()
-                        .getChildNodes(node.hostname()).stream()
-                        .allMatch(child -> child.state() == Node.State.parked ||
-                                child.state() == Node.State.failed)) {
+                    .list().childrenOf(node).asList().stream()
+                    .allMatch(child -> child.state() == Node.State.parked ||
+                                       child.state() == Node.State.failed)) {
                 log.info("Docker host " + node + " has no non-parked/failed children");
                 return true;
             }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
index c2e2ea36c13..5e98466fd7c 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
@@ -161,7 +161,7 @@ class NodeAllocation {
      */
     private boolean exclusiveTo(TenantName tenant, Optional<String> parentHostname) {
         if ( ! parentHostname.isPresent()) return true;
-        for (Node nodeOnHost : nodeRepository.getChildNodes(parentHostname.get())) {
+        for (Node nodeOnHost : nodeRepository.list().childrenOf(parentHostname.get()).asList()) {
             if ( ! nodeOnHost.allocation().isPresent()) continue;
 
             if ( nodeOnHost.allocation().get().membership().cluster().isExclusive() &&
@@ -174,7 +174,7 @@ class NodeAllocation {
     private boolean hostsOnly(TenantName tenant, Optional<String> parentHostname) {
         if ( ! parentHostname.isPresent()) return true; // yes, as host is exclusive
 
-        for (Node nodeOnHost : nodeRepository.getChildNodes(parentHostname.get())) {
+        for (Node nodeOnHost : nodeRepository.list().childrenOf(parentHostname.get()).asList()) {
             if ( ! nodeOnHost.allocation().isPresent()) continue;
             if ( ! nodeOnHost.allocation().get().owner().tenant().equals(tenant))
                 return false;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java
index f013620f88d..3c54c480c44 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java
@@ -49,7 +49,7 @@ public class NodePatcher {
             this.node = node;
             this.nodeRepository = nodeRepository;
             this.children = node.type().isDockerHost() ?
-                    nodeRepository.getChildNodes(node.hostname()) :
+                    nodeRepository.list().childrenOf(node).asList() :
                     Collections.emptyList();
         }
         catch (IOException e) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java
index b13e5c418f9..2374eda8bca 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java
@@ -52,8 +52,8 @@ public class NodeFailerTest {
         });
 
         // The host should have 2 nodes in active and 1 ready
-        Map<Node.State, List<String>> hostnamesByState = tester.nodeRepository.getChildNodes(hostWithHwFailure).stream()
-                .collect(Collectors.groupingBy(Node::state, Collectors.mapping(Node::hostname, Collectors.toList())));
+        Map<Node.State, List<String>> hostnamesByState = tester.nodeRepository.list().childrenOf(hostWithHwFailure).asList().stream()
+                                                                              .collect(Collectors.groupingBy(Node::state, Collectors.mapping(Node::hostname, Collectors.toList())));
         assertEquals(2, hostnamesByState.get(Node.State.active).size());
         assertEquals(1, hostnamesByState.get(Node.State.ready).size());
 
@@ -70,8 +70,8 @@ public class NodeFailerTest {
         expectedHostnamesByState1Iter.put(Node.State.failed,
                 Arrays.asList(hostnamesByState.get(Node.State.active).get(0), hostnamesByState.get(Node.State.ready).get(0)));
         expectedHostnamesByState1Iter.put(Node.State.active, hostnamesByState.get(Node.State.active).subList(1, 2));
-        Map<Node.State, List<String>> hostnamesByState1Iter = tester.nodeRepository.getChildNodes(hostWithHwFailure).stream()
-                .collect(Collectors.groupingBy(Node::state, Collectors.mapping(Node::hostname, Collectors.toList())));
+        Map<Node.State, List<String>> hostnamesByState1Iter = tester.nodeRepository.list().childrenOf(hostWithHwFailure).asList().stream()
+                                                                                   .collect(Collectors.groupingBy(Node::state, Collectors.mapping(Node::hostname, Collectors.toList())));
         assertEquals(expectedHostnamesByState1Iter, hostnamesByState1Iter);
 
         // Suspend the second of the active nodes
@@ -82,8 +82,8 @@ public class NodeFailerTest {
         tester.failer.run();
 
         // All of the children should be failed now
-        Set<Node.State> childStates2Iter = tester.nodeRepository.getChildNodes(hostWithHwFailure).stream()
-                .map(Node::state).collect(Collectors.toSet());
+        Set<Node.State> childStates2Iter = tester.nodeRepository.list().childrenOf(hostWithHwFailure).asList().stream()
+                                                                .map(Node::state).collect(Collectors.toSet());
         assertEquals(Collections.singleton(Node.State.failed), childStates2Iter);
         // The host itself is still active as it too must be allowed to suspend
         assertEquals(Node.State.active, tester.nodeRepository.getNode(hostWithHwFailure).get().state());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisioningTest.java
index ff51869273d..a0198f9c3f7 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisioningTest.java
@@ -135,7 +135,7 @@ public class DynamicDockerProvisioningTest {
 
         Map<Integer, Integer> numberOfChildrenStat = new HashMap<>();
         for (Node node : dockerHosts) {
-            int nofChildren = tester.nodeRepository().getChildNodes(node.hostname()).size();
+            int nofChildren = tester.nodeRepository().list().childrenOf(node).size();
             if (!numberOfChildrenStat.containsKey(nofChildren)) {
                 numberOfChildrenStat.put(nofChildren, 0);
             }
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),