diff options
author | Martin Polden <mpolden@mpolden.no> | 2022-07-12 10:34:57 +0200 |
---|---|---|
committer | Martin Polden <mpolden@mpolden.no> | 2022-07-12 10:34:57 +0200 |
commit | d01c10d78f8ef2cb8d31a31220a5ffd9beae0ee3 (patch) | |
tree | f2d0efe1dd9a9d9fcb19f569c068b8f8e5314fd4 | |
parent | 3e4ec608f84cdb73b394b4d468e5d575016b51d7 (diff) |
Node with allocation must be parked to allow deprovisioning
2 files changed, 48 insertions, 23 deletions
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java index cdc4416f90b..1d11d67c7cb 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java @@ -186,24 +186,37 @@ public class DynamicProvisioningMaintainer extends NodeRepositoryMaintainer { .collect(Collectors.toList()); } - private List<Node> candidatesForRemoval(List<Node> nodes) { - Map<String, Node> hostsByHostname = new HashMap<>(nodes.stream() - .filter(node -> switch (node.type()) { - case host -> - // TODO: Mark empty tenant hosts as wanttoretire & wanttodeprovision elsewhere, then handle as confighost here - node.state() != Node.State.parked || node.status().wantToDeprovision(); - case confighost, controllerhost -> node.state() == Node.State.parked && node.status().wantToDeprovision(); - default -> false; - }) - .collect(Collectors.toMap(Node::hostname, Function.identity()))); + private static List<Node> candidatesForRemoval(List<Node> nodes) { + Map<String, Node> removableHostsByHostname = new HashMap<>(); + for (var node : nodes) { + if (canRemoveHost(node)) { + removableHostsByHostname.put(node.hostname(), node); + } + } + for (var node : nodes) { + if (node.parentHostname().isPresent() && !canRemoveNode(node)) { + removableHostsByHostname.remove(node.parentHostname().get()); + } + } + return List.copyOf(removableHostsByHostname.values()); + } - nodes.stream() - .filter(node -> node.allocation().isPresent() && !node.status().wantToDeprovision()) - .flatMap(node -> node.parentHostname().stream()) - .distinct() - .forEach(hostsByHostname::remove); + private static boolean canRemoveHost(Node host) { + return switch (host.type()) { + // TODO: Mark empty tenant hosts as wanttoretire & wanttodeprovision elsewhere, then handle as confighost here + case host -> host.state() != Node.State.parked || host.status().wantToDeprovision(); + case confighost, controllerhost -> canDeprovision(host); + default -> false; + }; + } + + private static boolean canRemoveNode(Node node) { + if (node.type().isHost()) throw new IllegalArgumentException("Node " + node + " is not a child"); + return node.allocation().isEmpty() || canDeprovision(node); + } - return List.copyOf(hostsByHostname.values()); + private static boolean canDeprovision(Node node) { + return node.status().wantToDeprovision() && node.state() == Node.State.parked; } private Map<String, Node> findSharedHosts(NodeList nodeList) { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java index 7f4171ee8c0..8b363881fde 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java @@ -576,17 +576,29 @@ public class DynamicProvisioningMaintainerTest { tester.hostProvisioner.with(Behaviour.failProvisioning); Node host4 = tester.addNode("host4", Optional.empty(), NodeType.host, Node.State.parked); Node host41 = tester.addNode("host4-1", Optional.of("host4"), NodeType.tenant, Node.State.parked, DynamicProvisioningTester.tenantApp); - tester.nodeRepository.nodes().deprovision("host4", Agent.operator, Instant.now()); + Node host42 = tester.addNode("host4-2", Optional.of("host4"), NodeType.tenant, Node.State.active, DynamicProvisioningTester.tenantApp); - assertEquals(Optional.of(true), tester.nodeRepository.nodes().node("host4").map(n -> n.status().wantToDeprovision())); - assertEquals(Optional.of(Node.State.parked), tester.nodeRepository.nodes().node("host4").map(Node::state)); - assertEquals(Optional.of(true), tester.nodeRepository.nodes().node("host4-1").map(n -> n.status().wantToDeprovision())); - assertEquals(Optional.of(Node.State.parked), tester.nodeRepository.nodes().node("host4-1").map(Node::state)); + // Host and children are marked for deprovisioning + tester.nodeRepository.nodes().deprovision("host4", Agent.operator, Instant.now()); + for (var node : List.of(host4, host41, host42)) { + assertTrue(tester.nodeRepository.nodes().node(node.hostname()).map(n -> n.status().wantToDeprovision()).get()); + } + // Host and children remain parked because one child is still active tester.maintainer.maintain(); + for (var node : List.of(host4, host41)) { + assertEquals(Node.State.parked, tester.nodeRepository.nodes().node(node.hostname()).get().state()); + } + assertEquals(Node.State.active, tester.nodeRepository.nodes().node(host42.hostname()).get().state()); - assertEquals(Optional.empty(), tester.nodeRepository.nodes().node("host4")); - assertEquals(Optional.empty(), tester.nodeRepository.nodes().node("host4-1")); + // Last child is parked + tester.nodeRepository.nodes().park(host42.hostname(), true, Agent.system, getClass().getSimpleName()); + + // Host and children can now be removed + tester.maintainer.maintain(); + for (var node : List.of(host4, host41, host42)) { + assertTrue(node.hostname() + " removed", tester.nodeRepository.nodes().node(node.hostname()).isEmpty()); + } } private void assertCfghost3IsActive(DynamicProvisioningTester tester) { |