Code review fixes

3 files changed, 43 insertions, 23 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 4800383b2bb..b36921d37ac 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
@@ -476,10 +476,13 @@ public class NodeRepository extends AbstractComponent {
         Node node = getNode(hostname).orElseThrow(() -> new NotFoundException("No node with hostname \"" + hostname + '"'));
 
         try (Mutex lock = lockUnallocated()) {
-            List<Node> removed = getChildNodes(hostname).stream()
-                    .filter(this::allowedToRemove)
-                    .collect(Collectors.toList());
-            if (allowedToRemove(node)) removed.add(node);
+            List<Node> removed = node.type() != NodeType.host ?
+                    new ArrayList<>() :
+                    getChildNodes(hostname).stream()
+                            .filter(child -> allowedToRemove(child, true))
+                            .collect(Collectors.toList());
+
+            if (allowedToRemove(node, false)) removed.add(node);
             db.removeNodes(removed);
 
             return removed;
@@ -488,14 +491,34 @@ public class NodeRepository extends AbstractComponent {
         }
     }
 
-    private boolean allowedToRemove(Node nodeToRemove) {
-        List<Node.State> legalStates = nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER ?
-                Arrays.asList(Node.State.provisioned, Node.State.ready) :
-                Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked);
+    /**
+     * Allowed to a node delete if:
+     *  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 allowedToRemove(Node nodeToRemove, boolean deletingAsChild) {
+        if (nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER && !deletingAsChild) {
+            if (nodeToRemove.state() != Node.State.ready) {
+                throw new IllegalArgumentException(
+                        String.format("Docker container node %s can only be removed when in state ready", nodeToRemove.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);
 
-        if (! legalStates.contains(nodeToRemove.state())) {
-            throw new IllegalArgumentException(String.format("%s can only be removed from following states: %s",
-                    nodeToRemove.hostname(), legalStates.stream().map(Node.State::name).collect(Collectors.joining(", "))));
+            if (! legalStates.contains(nodeToRemove.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(", "))));
+            }
+        } else {
+            List<Node.State> legalStates = Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked);
+
+            if (! legalStates.contains(nodeToRemove.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(", "))));
+            }
         }
 
         return true;
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
index f7168e132f2..fe3f12b105e 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
@@ -70,23 +70,20 @@ public class NodeRepositoryTest {
     }
 
     @Test
-    public void only_allow_docker_containers_remove_in_provisioned_or_ready() {
+    public void only_allow_docker_containers_remove_in_ready() {
         NodeRepositoryTester tester = new NodeRepositoryTester();
         tester.addNode("id1", "host1", "docker", NodeType.tenant);
-        tester.addNode("id2", "host2", "docker", NodeType.tenant);
-        tester.nodeRepository().fail("host2", Agent.system, "Failed for testing");
 
-        tester.nodeRepository().removeRecursively("host1"); // host1 is in state provisioned
         try {
-            tester.nodeRepository().removeRecursively("host2");
-            fail("Should not be able to delete docker tenant node in state dirty");
+            tester.nodeRepository().removeRecursively("host1"); // host1 is in state provisioned
+            fail("Should not be able to delete docker container node by itself in state provisioned");
         } catch (IllegalArgumentException ignored) {
             // Expected
         }
 
-        tester.nodeRepository().setDirty("host2");
-        tester.nodeRepository().setReady("host2");
-        tester.nodeRepository().removeRecursively("host2");
+        tester.nodeRepository().setDirty("host1");
+        tester.nodeRepository().setReady("host1");
+        tester.nodeRepository().removeRecursively("host1");
     }
 
     @Test
@@ -105,7 +102,7 @@ public class NodeRepositoryTest {
 
         try {
             tester.nodeRepository().removeRecursively("host1");
-            fail("Should not be able to delete host node, one of the children are in state dirty");
+            fail("Should not be able to delete host node, one of the children is in state dirty");
         } catch (IllegalArgumentException ignored) {
             // Expected
         }
@@ -117,7 +114,7 @@ public class NodeRepositoryTest {
 
         // Now node10 and node12 are in provisioned, set node11 to ready, and it should be OK to delete host1
         tester.nodeRepository().setReady("node11");
-        tester.nodeRepository().removeRecursively("node12"); // Remove one of the children first instead
+        tester.nodeRepository().removeRecursively("node11"); // Remove one of the children first instead
         assertEquals(3, tester.nodeRepository().getNodes().size());
 
         tester.nodeRepository().removeRecursively("host1");
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 40ae1e1c2c5..38d3bf46028 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
@@ -401,7 +401,7 @@ public class RestApiTest {
         // Attempt to DELETE a node which is not put in a deletable state first
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host2.yahoo.com",
                                    new byte[0], Request.Method.DELETE),
-                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Failed to delete host2.yahoo.com: host2.yahoo.com can only be removed from following states: provisioned, failed, parked\"}");
+                       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\"}");
 
         // PUT current restart generation with string instead of long
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",