Allow host removal only if no children

2 files changed, 16 insertions, 1 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java
index b467108066d..2c9765ff665 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java
@@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.provision.maintenance;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.Deployment;
+import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.applicationmodel.HostName;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
@@ -84,11 +85,17 @@ public class RetiredExpirer extends Maintainer {
     }
 
     /**
-     * Checks if the node can be removed, this is allowed if either of these are true:
+     * Checks if the node can be removed:
+     * if the node is {@link NodeType#host}, it will only be removed if it has no children
+     * Otherwise, a removal is allowed if either of these are true:
      * - The node has been in state {@link History.Event.Type#retired} for longer than {@link #retiredDuration}
      * - Orchestrator allows it
      */
     private boolean canRemove(Node node) {
+        if (node.type() == NodeType.host) {
+            return nodeRepository().getChildNodes(node.hostname()).isEmpty();
+        }
+
         Optional<Instant> timeOfRetiredEvent = node.history().event(History.Event.Type.retired).map(History.Event::at);
         Optional<Instant> retireAfter = timeOfRetiredEvent.map(retiredEvent -> retiredEvent.plus(retiredDuration));
         boolean shouldRetireNowBecauseExpried = retireAfter.map(time -> time.isBefore(clock.instant())).orElse(false);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java
index 375500528f1..857385a9b77 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java
@@ -29,6 +29,7 @@ import com.yahoo.vespa.hosted.provision.testutils.MockDeployer;
 import com.yahoo.vespa.hosted.provision.testutils.MockNameResolver;
 import com.yahoo.vespa.orchestrator.OrchestrationException;
 import com.yahoo.vespa.orchestrator.Orchestrator;
+import org.junit.Before;
 import org.junit.Test;
 
 import java.time.Duration;
@@ -41,6 +42,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -62,6 +64,12 @@ public class RetiredExpirerTest {
 
     private static final Duration RETIRED_EXPIRATION = Duration.ofHours(12);
 
+    @Before
+    public void setup() throws OrchestrationException {
+        // By default, orchestrator should deny all request for suspension so we can test expiration
+        doThrow(new RuntimeException()).when(orchestrator).acquirePermissionToRemove(any());
+    }
+
     @Test
     public void ensure_retired_nodes_time_out() {
         createReadyNodes(7, nodeRepository, nodeFlavors);