From 379e9c8260c09942ef9b084971d09c8b7c54e8b2 Mon Sep 17 00:00:00 2001 From: Martin Polden Date: Thu, 8 Jun 2023 10:42:01 +0200 Subject: Disallow moving deprovisioned hosts --- .../hosted/provision/maintenance/HostCapacityMaintainer.java | 5 +++-- .../main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java | 3 +++ .../hosted/provision/maintenance/HostCapacityMaintainerTest.java | 9 +++++++++ 3 files changed, 15 insertions(+), 2 deletions(-) (limited to 'node-repository') diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostCapacityMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostCapacityMaintainer.java index 3f9e8ef4407..e0012ed0b9d 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostCapacityMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostCapacityMaintainer.java @@ -88,7 +88,7 @@ public class HostCapacityMaintainer extends NodeRepositoryMaintainer { // Group nodes by parent; no parent means it's a host. Map, List> nodesByParent = provisionedSnapshot.stream().collect(groupingBy(Node::parentHostname)); - // Find all hosts that we once thought were empty (first clouse), or whose children are now all removable (second clause). + // Find all hosts that we once thought were empty (first clause), or whose children are now all removable (second clause). List emptyHosts = nodesByParent.get(Optional.empty()).stream() .filter(host -> host.hostEmptyAt().isPresent() || nodesByParent.getOrDefault(Optional.of(host.hostname()), List.of()) @@ -165,7 +165,8 @@ public class HostCapacityMaintainer extends NodeRepositoryMaintainer { 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 host -> host.state() != Node.State.deprovisioned && + (host.state() != Node.State.parked || host.status().wantToDeprovision()); case confighost, controllerhost -> canDeprovision(host); default -> false; }; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java index cc49265506c..5f9c6974204 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java @@ -463,6 +463,9 @@ public class Nodes { illegal("Could not set " + node + " active: Same cluster and index as " + currentActive); } } + if (node.state() == Node.State.deprovisioned) { + illegal(node + " cannot be moved"); + } if (forceDeprovision) node = node.withWantToRetire(true, true, agent, clock.instant()); if (toState == Node.State.deprovisioned) { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/HostCapacityMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/HostCapacityMaintainerTest.java index ead94663807..30831b2ba77 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/HostCapacityMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/HostCapacityMaintainerTest.java @@ -34,6 +34,7 @@ import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.node.Agent; import com.yahoo.vespa.hosted.provision.node.Allocation; import com.yahoo.vespa.hosted.provision.node.Generation; +import com.yahoo.vespa.hosted.provision.node.History; import com.yahoo.vespa.hosted.provision.node.IP; import com.yahoo.vespa.hosted.provision.node.Status; import com.yahoo.vespa.hosted.provision.provisioning.FlavorConfigBuilder; @@ -125,6 +126,14 @@ public class HostCapacityMaintainerTest { assertTrue("Host satisfying 16-24-100-1 is kept", tester.nodeRepository.nodes().node("host3").isPresent()); assertTrue("New 48-128-1000-10 host added", tester.nodeRepository.nodes().node("host100").isPresent()); assertTrue("New 48-128-1000-10 host added", tester.nodeRepository.nodes().node("host101").isPresent()); + + Instant deprovisionedAt = tester.nodeRepository.nodes().node("host2").get().history().event(History.Event.Type.deprovisioned).get().at(); + tester.provisioningTester.clock().advance(Duration.ofSeconds(1)); + tester.maintain(); + assertEquals("Host moves to deprovisioned once", deprovisionedAt, + tester.nodeRepository.nodes().node("host2").get().history() + .event(History.Event.Type.deprovisioned).get().at()); + } @Test -- cgit v1.2.3