Decommission nodes via dirty

3 files changed, 19 insertions, 17 deletions

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 084f1f5a9ef..8f15a6bf745 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
@@ -194,8 +194,8 @@ public class Nodes {
         Node node = nodeMutex.node();
         if (node.state() != Node.State.provisioned && node.state() != Node.State.dirty)
             illegal("Can not set " + node + " ready. It is not provisioned or dirty.");
-        if (!node.status().wantToDeprovision()) // Do not reset status if wantToDeprovision
-            node = node.withWantToRetire(false, false, false, agent, clock.instant());
+        if (node.status().wantToDeprovision() || node.status().wantToRebuild())
+            return park(node.hostname(), false, agent, reason);
 
         return db.writeTo(Node.State.ready, node, agent, Optional.of(reason));
     }
@@ -313,9 +313,9 @@ public class Nodes {
             return park(node.hostname(), false, agent, reason, transaction);
         } else {
             Node.State toState = Node.State.dirty;
-            if (node.state() == Node.State.parked) {
-                if (node.status().wantToDeprovision()) throw new IllegalArgumentException("Cannot move " + node + " to " + toState + ": It's being deprovisioned");
-                if (node.status().wantToRebuild()) throw new IllegalArgumentException("Cannot move " + node + " to " + toState + ": It's being rebuilt");
+            if (node.state() == Node.State.parked && node.type().isHost()) {
+                if (node.status().wantToDeprovision()) illegal("Cannot move " + node + " to " + toState + ": It's being deprovisioned");
+                if (node.status().wantToRebuild()) illegal("Cannot move " + node + " to " + toState + ": It's being rebuilt");
             }
             return db.writeTo(toState, List.of(node), agent, Optional.of(reason), transaction).get(0);
         }
@@ -859,17 +859,13 @@ public class Nodes {
 
     /** Returns whether node should be parked when deallocated by given agent */
     private static boolean parkOnDeallocationOf(Node node, Agent agent) {
-        if (node.state() == Node.State.parked) return false;
-        if (agent == Agent.operator) return false;
-        if (node.type() == NodeType.tenant && node.status().wantToDeprovision()) return false;
-        boolean retirementRequestedByOperator = node.status().wantToRetire() &&
-                                                node.history().event(History.Event.Type.wantToRetire)
-                                                    .map(History.Event::agent)
-                                                    .map(a -> a == Agent.operator)
-                                                    .orElse(false);
-        return node.status().wantToDeprovision() ||
-               node.status().wantToRebuild() ||
-               retirementRequestedByOperator;
+        return  agent != Agent.operator &&
+               !node.status().wantToDeprovision() &&
+                node.status().wantToRetire() &&
+                node.history().event(History.Event.Type.wantToRetire)
+                              .map(History.Event::agent)
+                              .map(a -> a == Agent.operator)
+                              .orElse(false);
     }
 
     private enum HostOperation {
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 30b44d713a3..afe02aa5e3a 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
@@ -445,6 +445,8 @@ public class HostCapacityMaintainerTest {
         tester.nodeRepository().nodes().setRemovable(hostApp, List.of(hostToRemove.get()), true);
         tester.prepareAndActivateInfraApplication(configSrvApp, hostType.childNodeType());
         tester.prepareAndActivateInfraApplication(hostApp, hostType);
+        tester.nodeRepository().nodes().markNodeAvailableForNewAllocation(nodeToRemove.get().hostname(), Agent.operator, "Readied by host-admin");
+        tester.nodeRepository().nodes().markNodeAvailableForNewAllocation(hostToRemove.get().hostname(), Agent.operator, "Readied by host-admin");
         assertEquals(2, tester.nodeRepository().nodes().list().nodeType(hostType.childNodeType()).state(Node.State.active).size());
         assertSame("Node moves to expected state", Node.State.parked, nodeToRemove.get().state());
         assertSame("Host moves to parked", Node.State.parked, hostToRemove.get().state());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java
index 0d1d138276a..d47b8955b56 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java
@@ -175,10 +175,14 @@ public class InactiveAndFailedExpirerTest {
         List<Node> inactiveNodes = tester.getNodes(applicationId, Node.State.inactive).asList();
         assertEquals(2, inactiveNodes.size());
 
-        // Nodes marked for deprovisioning are moved to parked
+        // Nodes marked for deprovisioning are moved to dirty and then parked when readied by host-admin
         tester.patchNodes(inactiveNodes, (node) -> node.withWantToRetire(true, true, Agent.system, tester.clock().instant()));
         tester.advanceTime(Duration.ofMinutes(11));
         new InactiveExpirer(tester.nodeRepository(), Duration.ofMinutes(10), Map.of(), new TestMetric()).run();
+
+        NodeList expired = tester.nodeRepository().nodes().list(Node.State.dirty);
+        assertEquals(2, expired.size());
+        expired.forEach(node -> tester.nodeRepository().nodes().markNodeAvailableForNewAllocation(node.hostname(), Agent.operator, "Readied by host-admin"));
         assertEquals(2, tester.nodeRepository().nodes().list(Node.State.parked).size());
     }