Merge pull request #27341 from vespa-engine/mpolden/disallow-move

Disallow moving deprovisioned hosts

3 files changed, 15 insertions, 2 deletions

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<Optional<String>, List<Node>> 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<Node> emptyHosts = nodesByParent.get(Optional.<String>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