diff options
author | valerijf <valerijf@oath.com> | 2017-08-30 09:24:26 +0200 |
---|---|---|
committer | valerijf <valerijf@oath.com> | 2017-08-30 09:24:26 +0200 |
commit | 388de264b5e1c775fbf603ae5db16e0121aad27c (patch) | |
tree | b1b368736eb7c3d02da8308756af42dc53627a14 /node-repository | |
parent | 8af3bc056d712a3a1bc6b99bef3ddf5be3b89e94 (diff) |
Code review fixes
Diffstat (limited to 'node-repository')
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", |