Merge pull request #4930 from vespa-engine/freva/combine-retired-expirers

Freva/combine retired expirers

9 files changed, 159 insertions, 177 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
index 12ba67eba6d..be792630445 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
@@ -41,7 +41,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
     private final ReservationExpirer reservationExpirer;
     private final InactiveExpirer inactiveExpirer;
     private final RetiredExpirer retiredExpirer;
-    private final RetiredEarlyExpirer retiredEarlyExpirer;
     private final FailedExpirer failedExpirer;
     private final DirtyExpirer dirtyExpirer;
     private final ProvisionedExpirer provisionedExpirer;
@@ -69,8 +68,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         operatorChangeApplicationMaintainer = new OperatorChangeApplicationMaintainer(deployer, nodeRepository, clock, durationFromEnv("operator_change_redeploy_interval").orElse(defaults.operatorChangeRedeployInterval), jobControl);
         zooKeeperAccessMaintainer = new ZooKeeperAccessMaintainer(nodeRepository, curator, durationFromEnv("zookeeper_access_maintenance_interval").orElse(defaults.zooKeeperAccessMaintenanceInterval), jobControl);
         reservationExpirer = new ReservationExpirer(nodeRepository, clock, durationFromEnv("reservation_expiry").orElse(defaults.reservationExpiry), jobControl);
-        retiredExpirer = new RetiredExpirer(nodeRepository, deployer, clock, durationFromEnv("retired_expiry").orElse(defaults.retiredExpiry), jobControl);
-        retiredEarlyExpirer = new RetiredEarlyExpirer(nodeRepository, durationFromEnv("retired_early_interval").orElse(defaults.retiredEarlyInterval), jobControl, deployer, orchestrator);
+        retiredExpirer = new RetiredExpirer(nodeRepository, orchestrator, deployer, clock, durationFromEnv("retired_interval").orElse(defaults.retiredInterval), durationFromEnv("retired_expiry").orElse(defaults.retiredExpiry), jobControl);
         inactiveExpirer = new InactiveExpirer(nodeRepository, clock, durationFromEnv("inactive_expiry").orElse(defaults.inactiveExpiry), jobControl);
         failedExpirer = new FailedExpirer(nodeRepository, zone, clock, durationFromEnv("failed_expirer_interval").orElse(defaults.failedExpirerInterval), jobControl);
         dirtyExpirer = new DirtyExpirer(nodeRepository, clock, durationFromEnv("dirty_expiry").orElse(defaults.dirtyExpiry), jobControl);
@@ -93,7 +91,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         reservationExpirer.deconstruct();
         inactiveExpirer.deconstruct();
         retiredExpirer.deconstruct();
-        retiredEarlyExpirer.deconstruct();
         failedExpirer.deconstruct();
         dirtyExpirer.deconstruct();
         nodeRebooter.deconstruct();
@@ -140,7 +137,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         private final Duration rebootInterval;
         private final Duration nodeRetirerInterval;
         private final Duration metricsInterval;
-        private final Duration retiredEarlyInterval;
+        private final Duration retiredInterval;
 
         private final NodeFailer.ThrottlePolicy throttlePolicy;
 
@@ -155,7 +152,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
                 reservationExpiry = Duration.ofMinutes(20); // same as deployment timeout
                 inactiveExpiry = Duration.ofHours(4); // enough time for the application owner to discover and redeploy
                 retiredExpiry = Duration.ofDays(4); // enough time to migrate data
-                retiredEarlyInterval = Duration.ofMinutes(29);
+                retiredInterval = Duration.ofMinutes(29);
                 failedExpirerInterval = Duration.ofMinutes(10);
                 dirtyExpiry = Duration.ofHours(2); // enough time to clean the node
                 provisionedExpiry = Duration.ofHours(4);
@@ -173,7 +170,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
                 reservationExpiry = Duration.ofMinutes(10); // Need to be long enough for deployment to be finished for all config model versions
                 inactiveExpiry = Duration.ofSeconds(2); // support interactive wipe start over
                 retiredExpiry = Duration.ofMinutes(1);
-                retiredEarlyInterval = Duration.ofMinutes(5);
+                retiredInterval = Duration.ofMinutes(5);
                 failedExpirerInterval = Duration.ofMinutes(10);
                 dirtyExpiry = Duration.ofMinutes(30);
                 provisionedExpiry = Duration.ofHours(4);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredEarlyExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredEarlyExpirer.java
deleted file mode 100644
index 00543058520..00000000000
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredEarlyExpirer.java
+++ /dev/null
@@ -1,99 +0,0 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.hosted.provision.maintenance;
-
-import com.yahoo.collections.ListMap;
-import com.yahoo.config.provision.ApplicationId;
-import com.yahoo.config.provision.Deployer;
-import com.yahoo.config.provision.Deployment;
-import com.yahoo.vespa.applicationmodel.HostName;
-import com.yahoo.vespa.hosted.provision.Node;
-import com.yahoo.vespa.hosted.provision.NodeRepository;
-import com.yahoo.vespa.orchestrator.OrchestrationException;
-import com.yahoo.vespa.orchestrator.Orchestrator;
-
-import java.time.Duration;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.logging.Level;
-import java.util.stream.Collectors;
-
-/**
- * Maintenance job which deactivates retired nodes, if given permission by orchestrator.
- *
- * @author hakon
- */
-// TODO: This should be consolidated with RetiredExpirer. The only difference between this and RetiredExpirer is that
-// this runs more often by default and asks orchestrator for permission to retire nodes.
-public class RetiredEarlyExpirer extends Maintainer {
-
-    private final Deployer deployer;
-    private final Orchestrator orchestrator;
-
-    public RetiredEarlyExpirer(NodeRepository nodeRepository,
-                               Duration interval,
-                               JobControl jobControl,
-                               Deployer deployer,
-                               Orchestrator orchestrator) {
-        super(nodeRepository, interval, jobControl);
-        this.deployer = deployer;
-        this.orchestrator = orchestrator;
-    }
-
-    @Override
-    protected void maintain() {
-        List<Node> activeNodes = nodeRepository().getNodes(Node.State.active);
-
-        ListMap<ApplicationId, Node> retiredNodesByApplication = new ListMap<>();
-        for (Node node : activeNodes) {
-            if (node.allocation().isPresent() && node.allocation().get().membership().retired()) {
-                retiredNodesByApplication.put(node.allocation().get().owner(), node);
-            }
-        }
-
-        for (Map.Entry<ApplicationId, List<Node>> entry : retiredNodesByApplication.entrySet()) {
-            ApplicationId application = entry.getKey();
-            List<Node> retiredNodes = entry.getValue();
-
-            try {
-                Optional<Deployment> deployment = deployer.deployFromLocalActive(application);
-                if ( ! deployment.isPresent()) continue; // this will be done at another config server
-
-                List<Node> nodesToRemove = new ArrayList<>();
-                for (Node node : retiredNodes) {
-                    if (canRemove(node)) {
-                        nodesToRemove.add(node);
-                    }
-                }
-                
-                if (nodesToRemove.isEmpty()) {
-                    continue;
-                }
-
-                nodeRepository().setRemovable(application, nodesToRemove);
-
-                deployment.get().activate();
-
-                String nodeList = nodesToRemove.stream().map(Node::hostname).collect(Collectors.joining(", "));
-                log.info("Redeployed " + application + " to deactivate retired nodes: " +  nodeList);
-            } catch (RuntimeException e) {
-                String nodeList = retiredNodes.stream().map(Node::hostname).collect(Collectors.joining(", "));
-                log.log(Level.WARNING, "Exception trying to deactivate retired nodes from " + application
-                        + ": " + nodeList, e);
-            }
-        }
-    }
-
-    /** Returns whether orchestrator permits given node to be removed */
-    private boolean canRemove(Node node) {
-        try {
-            orchestrator.acquirePermissionToRemove(new HostName(node.hostname()));
-            return true;
-        } catch (OrchestrationException e) {
-            log.info("Did not get permission to remove retired " + node + ": " + e.getMessage());
-            return false;
-        }
-    }
-
-}
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 4c5c8adf576..6d82dfb7b4e 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
@@ -1,69 +1,115 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision.maintenance;
 
-import com.yahoo.collections.ListMap;
 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;
 import com.yahoo.vespa.hosted.provision.node.History;
+import com.yahoo.vespa.orchestrator.OrchestrationException;
+import com.yahoo.vespa.orchestrator.Orchestrator;
 
 import java.time.Clock;
 import java.time.Duration;
+import java.time.Instant;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.logging.Level;
+import java.util.stream.Collectors;
 
 /**
- * Maintenance job which deactivates nodes which has been retired.
- * This should take place after the system has been given sufficient time to migrate data to other nodes.
- * <p>
- * As these nodes are active, and therefore part of the configuration the impacted applications must be
- * reconfigured after inactivation.
+ * Maintenance job which deactivates retired nodes, if given permission by orchestrator, or
+ * after giving the system has been given sufficient time to migrate data to other nodes.
  *
- * @author bratseth
+ * @author hakon
  */
-public class RetiredExpirer extends Expirer {
+public class RetiredExpirer extends Maintainer {
 
-    private final NodeRepository nodeRepository;
     private final Deployer deployer;
+    private final Orchestrator orchestrator;
+    private final Duration retiredExpiry;
+    private final Clock clock;
 
-    public RetiredExpirer(NodeRepository nodeRepository, Deployer deployer, Clock clock, 
-                          Duration retiredDuration, JobControl jobControl) {
-        super(Node.State.active, History.Event.Type.retired, nodeRepository, clock, retiredDuration, jobControl);
-        this.nodeRepository = nodeRepository;
+    public RetiredExpirer(NodeRepository nodeRepository,
+                          Orchestrator orchestrator,
+                          Deployer deployer,
+                          Clock clock,
+                          Duration maintenanceInterval,
+                          Duration retiredExpiry,
+                          JobControl jobControl) {
+        super(nodeRepository, maintenanceInterval, jobControl);
         this.deployer = deployer;
+        this.orchestrator = orchestrator;
+        this.retiredExpiry = retiredExpiry;
+        this.clock = clock;
     }
 
     @Override
-    protected void expire(List<Node> expired) {
-        // Only expire nodes which are retired. Do one application at the time.
-        ListMap<ApplicationId, Node> applicationNodes = new ListMap<>();
-        for (Node node : expired) {
-            if (node.allocation().isPresent() && node.allocation().get().membership().retired())
-                applicationNodes.put(node.allocation().get().owner(), node);
-        }
+    protected void maintain() {
+        List<Node> activeNodes = nodeRepository().getNodes(Node.State.active);
+
+        Map<ApplicationId, List<Node>> retiredNodesByApplication = activeNodes.stream()
+                .filter(node -> node.allocation().isPresent())
+                .filter(node -> node.allocation().get().membership().retired())
+                .collect(Collectors.groupingBy(node -> node.allocation().get().owner()));
 
-        for (Map.Entry<ApplicationId, List<Node>> entry : applicationNodes.entrySet()) {
+        for (Map.Entry<ApplicationId, List<Node>> entry : retiredNodesByApplication.entrySet()) {
             ApplicationId application = entry.getKey();
-            List<Node> nodesToRemove = entry.getValue();
+            List<Node> retiredNodes = entry.getValue();
+
             try {
                 Optional<Deployment> deployment = deployer.deployFromLocalActive(application);
                 if ( ! deployment.isPresent()) continue; // this will be done at another config server
 
-                nodeRepository.setRemovable(application, nodesToRemove);
+                List<Node> nodesToRemove = retiredNodes.stream().filter(this::canRemove).collect(Collectors.toList());
+                if (nodesToRemove.isEmpty()) {
+                    continue;
+                }
+
+                nodeRepository().setRemovable(application, nodesToRemove);
 
                 deployment.get().activate();
 
-                log.info("Redeployed " + application + " to deactivate " + nodesToRemove.size() + " retired nodes");
-            }
-            catch (RuntimeException e) {
-                log.log(Level.WARNING, "Exception trying to remove previously retired nodes " + nodesToRemove +
-                        "from " + application, e);
+                String nodeList = nodesToRemove.stream().map(Node::hostname).collect(Collectors.joining(", "));
+                log.info("Redeployed " + application + " to deactivate retired nodes: " +  nodeList);
+            } catch (RuntimeException e) {
+                String nodeList = retiredNodes.stream().map(Node::hostname).collect(Collectors.joining(", "));
+                log.log(Level.WARNING, "Exception trying to deactivate retired nodes from " + application
+                        + ": " + nodeList, e);
             }
         }
     }
 
+    /**
+     * 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 #retiredExpiry}
+     * - 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(retiredExpiry));
+        boolean shouldRetireNowBecauseExpried = retireAfter.map(time -> time.isBefore(clock.instant())).orElse(false);
+        if (shouldRetireNowBecauseExpried) {
+            return true;
+        }
+
+        try {
+            orchestrator.acquirePermissionToRemove(new HostName(node.hostname()));
+            return true;
+        } catch (OrchestrationException e) {
+            log.info("Did not get permission to remove retired " + node + ": " + e.getMessage());
+            return false;
+        }
+    }
+
 }
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 9c5c86317aa..a79b0916cbe 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
@@ -17,6 +17,8 @@ import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.node.History;
 import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester;
 import com.yahoo.vespa.hosted.provision.testutils.MockDeployer;
+import com.yahoo.vespa.orchestrator.OrchestrationException;
+import com.yahoo.vespa.orchestrator.Orchestrator;
 import org.junit.Test;
 
 import java.time.Duration;
@@ -29,6 +31,9 @@ import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
 
 /**
  * @author bratseth
@@ -109,7 +114,7 @@ public class InactiveAndFailedExpirerTest {
     }
 
     @Test
-    public void node_that_wants_to_retire_is_moved_to_parked() {
+    public void node_that_wants_to_retire_is_moved_to_parked() throws OrchestrationException {
         ProvisioningTester tester = new ProvisioningTester(new Zone(Environment.prod, RegionName.from("us-east")));
         ClusterSpec cluster = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("test"), 
                                                   Version.fromString("6.42"));
@@ -142,7 +147,10 @@ public class InactiveAndFailedExpirerTest {
                                                             1)
                 )
         );
-        new RetiredExpirer(tester.nodeRepository(), deployer, tester.clock(), Duration.ofMinutes(10), new JobControl(tester.nodeRepository().database())).run();
+        Orchestrator orchestrator = mock(Orchestrator.class);
+        doThrow(new RuntimeException()).when(orchestrator).acquirePermissionToRemove(any());
+        new RetiredExpirer(tester.nodeRepository(), tester.orchestrator(), deployer, tester.clock(), Duration.ofDays(30),
+                Duration.ofMinutes(10), new JobControl(tester.nodeRepository().database())).run();
         assertEquals(1, tester.nodeRepository().getNodes(Node.State.inactive).size());
 
         // Inactive times out and one node is moved to parked
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java
index 04e782c5562..3b9bf498869 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java
@@ -23,6 +23,8 @@ import com.yahoo.vespa.hosted.provision.provisioning.FlavorSpareChecker;
 import com.yahoo.vespa.hosted.provision.provisioning.NodeRepositoryProvisioner;
 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 java.time.Duration;
 import java.util.ArrayList;
@@ -38,7 +40,9 @@ import java.util.Optional;
 import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
+import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -59,6 +63,7 @@ public class NodeRetirerTester {
     // Use LinkedHashMap to keep order in which applications were deployed
     private final Map<ApplicationId, MockDeployer.ApplicationContext> apps = new LinkedHashMap<>();
 
+    private final Orchestrator orchestrator = mock(Orchestrator.class);
     private RetiredExpirer retiredExpirer;
     private InactiveExpirer inactiveExpirer;
     private int nextNodeId = 0;
@@ -71,6 +76,12 @@ public class NodeRetirerTester {
         NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone);
         deployer = new MockDeployer(provisioner, apps);
         flavors = nodeFlavors.getFlavors().stream().sorted(Comparator.comparing(Flavor::name)).collect(Collectors.toList());
+
+        try {
+            doThrow(new RuntimeException()).when(orchestrator).acquirePermissionToRemove(any());
+        } catch (OrchestrationException e) {
+            e.printStackTrace();
+        }
     }
 
     NodeRetirer makeNodeRetirer(RetirementPolicy policy) {
@@ -112,7 +123,7 @@ public class NodeRetirerTester {
 
     void iterateMaintainers() {
         if (retiredExpirer == null) {
-            retiredExpirer = new RetiredExpirer(nodeRepository, deployer, clock, Duration.ofMinutes(10), jobControl);
+            retiredExpirer = new RetiredExpirer(nodeRepository, orchestrator, deployer, clock, Duration.ofDays(30), Duration.ofMinutes(10), jobControl);
             inactiveExpirer = new InactiveExpirer(nodeRepository, clock, Duration.ofMinutes(10), jobControl);
         }
 
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 931f2a8f275..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
@@ -6,6 +6,7 @@ import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ApplicationName;
 import com.yahoo.config.provision.Capacity;
 import com.yahoo.config.provision.ClusterSpec;
+import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.DockerImage;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.HostSpec;
@@ -28,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;
@@ -40,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;
@@ -50,17 +53,25 @@ import static org.mockito.Mockito.verify;
 public class RetiredExpirerTest {
 
     private Curator curator = new MockCurator();
+    private final ManualClock clock = new ManualClock();
+    private final Zone zone = new Zone(Environment.prod, RegionName.from("us-east"));
+    private final NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default");
+    private final NodeRepository nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone,
+            new MockNameResolver().mockAnyLookup(),
+            new DockerImage("docker-registry.domain.tld:8080/dist/vespa"));
+    private final NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone);
+    private final Orchestrator orchestrator = mock(Orchestrator.class);
+
+    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() {
-        ManualClock clock = new ManualClock();
-        Zone zone = new Zone(Environment.prod, RegionName.from("us-east"));
-        NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default");
-        NodeRepository nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone,
-                                                           new MockNameResolver().mockAnyLookup(),
-                                                           new DockerImage("docker-registry.domain.tld:8080/dist/vespa"));
-        NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone);
-
         createReadyNodes(7, nodeRepository, nodeFlavors);
         createHostNodes(4, nodeRepository, nodeFlavors);
 
@@ -81,7 +92,7 @@ public class RetiredExpirerTest {
         MockDeployer deployer =
             new MockDeployer(provisioner,
                              Collections.singletonMap(applicationId, new MockDeployer.ApplicationContext(applicationId, cluster, Capacity.fromNodeCount(wantedNodes, Optional.of("default")), 1)));
-        new RetiredExpirer(nodeRepository, deployer, clock, Duration.ofHours(12), new JobControl(nodeRepository.database())).run();
+        createRetiredExpirer(deployer).run();
         assertEquals(3, nodeRepository.getNodes(applicationId, Node.State.active).size());
         assertEquals(4, nodeRepository.getNodes(applicationId, Node.State.inactive).size());
         assertEquals(1, deployer.redeployments);
@@ -93,14 +104,6 @@ public class RetiredExpirerTest {
 
     @Test
     public void ensure_retired_groups_time_out() {
-        ManualClock clock = new ManualClock();
-        Zone zone = new Zone(Environment.prod, RegionName.from("us-east"));
-        NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default");
-        NodeRepository nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone,
-                                                           new MockNameResolver().mockAnyLookup(),
-                                                           new DockerImage("docker-registry.domain.tld:8080/dist/vespa"));
-        NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone);
-
         createReadyNodes(8, nodeRepository, nodeFlavors);
         createHostNodes(4, nodeRepository, nodeFlavors);
 
@@ -117,7 +120,7 @@ public class RetiredExpirerTest {
         MockDeployer deployer =
             new MockDeployer(provisioner,
                              Collections.singletonMap(applicationId, new MockDeployer.ApplicationContext(applicationId, cluster, Capacity.fromNodeCount(2, Optional.of("default")), 1)));
-        new RetiredExpirer(nodeRepository, deployer, clock, Duration.ofHours(12), new JobControl(nodeRepository.database())).run();
+        createRetiredExpirer(deployer).run();
         assertEquals(2, nodeRepository.getNodes(applicationId, Node.State.active).size());
         assertEquals(6, nodeRepository.getNodes(applicationId, Node.State.inactive).size());
         assertEquals(1, deployer.redeployments);
@@ -129,14 +132,6 @@ public class RetiredExpirerTest {
 
     @Test
     public void ensure_early_inactivation() throws OrchestrationException {
-        ManualClock clock = new ManualClock();
-        Zone zone = new Zone(Environment.prod, RegionName.from("us-east"));
-        NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default");
-        NodeRepository nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone,
-                                                           new MockNameResolver().mockAnyLookup(),
-                                                           new DockerImage("docker-registry.domain.tld:8080/dist/vespa"));
-        NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone);
-
         createReadyNodes(7, nodeRepository, nodeFlavors);
         createHostNodes(4, nodeRepository, nodeFlavors);
 
@@ -153,14 +148,13 @@ public class RetiredExpirerTest {
         assertEquals(0, nodeRepository.getNodes(applicationId, Node.State.inactive).size());
 
         // Cause inactivation of retired nodes
-        clock.advance(Duration.ofHours(30)); // Retire period spent
         MockDeployer deployer =
                 new MockDeployer(provisioner,
                         Collections.singletonMap(
                                 applicationId,
                                 new MockDeployer.ApplicationContext(applicationId, cluster, Capacity.fromNodeCount(wantedNodes, Optional.of("default")), 1)));
 
-        Orchestrator orchestrator = mock(Orchestrator.class);
+
         // Allow the 1st and 3rd retired nodes permission to inactivate
         doNothing()
                 .doThrow(new OrchestrationException("Permission not granted 1"))
@@ -168,18 +162,27 @@ public class RetiredExpirerTest {
                 .doThrow(new OrchestrationException("Permission not granted 2"))
                 .when(orchestrator).acquirePermissionToRemove(any());
 
-        new RetiredEarlyExpirer(
-                nodeRepository,
-                Duration.ofDays(30),
-                new JobControl(nodeRepository.database()),
-                deployer,
-                orchestrator).run();
+        RetiredExpirer retiredExpirer = createRetiredExpirer(deployer);
+        retiredExpirer.run();
         assertEquals(5, nodeRepository.getNodes(applicationId, Node.State.active).size());
         assertEquals(2, nodeRepository.getNodes(applicationId, Node.State.inactive).size());
         assertEquals(1, deployer.redeployments);
-
         verify(orchestrator, times(4)).acquirePermissionToRemove(any());
 
+        // Running it again has no effect
+        retiredExpirer.run();
+        assertEquals(5, nodeRepository.getNodes(applicationId, Node.State.active).size());
+        assertEquals(2, nodeRepository.getNodes(applicationId, Node.State.inactive).size());
+        assertEquals(1, deployer.redeployments);
+        verify(orchestrator, times(6)).acquirePermissionToRemove(any());
+
+        clock.advance(RETIRED_EXPIRATION.plusMinutes(1));
+        retiredExpirer.run();
+        assertEquals(3, nodeRepository.getNodes(applicationId, Node.State.active).size());
+        assertEquals(4, nodeRepository.getNodes(applicationId, Node.State.inactive).size());
+        assertEquals(2, deployer.redeployments);
+        verify(orchestrator, times(6)).acquirePermissionToRemove(any());
+
         // inactivated nodes are not retired
         for (Node node : nodeRepository.getNodes(applicationId, Node.State.inactive))
             assertFalse(node.allocation().get().membership().retired());
@@ -210,4 +213,14 @@ public class RetiredExpirerTest {
         nodeRepository.setReady(nodes);
     }
 
+    private RetiredExpirer createRetiredExpirer(Deployer deployer) {
+        return new RetiredExpirer(
+                nodeRepository,
+                orchestrator,
+                deployer,
+                clock,
+                Duration.ofDays(30), /* Maintenance interval, use large value so it never runs by itself */
+                RETIRED_EXPIRATION,
+                new JobControl(nodeRepository.database()));
+    }
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
index 4a20cb3e318..9587fedb7b2 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
@@ -11,8 +11,8 @@ import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.maintenance.JobControl;
-import com.yahoo.vespa.hosted.provision.testutils.MockDeployer;
 import com.yahoo.vespa.hosted.provision.maintenance.RetiredExpirer;
+import com.yahoo.vespa.hosted.provision.testutils.MockDeployer;
 import org.junit.Ignore;
 import org.junit.Test;
 
@@ -136,7 +136,8 @@ public class MultigroupProvisioningTest {
                              Collections.singletonMap(application1, 
                                                       new MockDeployer.ApplicationContext(application1, cluster(), 
                                                                                           Capacity.fromNodeCount(8, Optional.of("large")), 1)));
-        new RetiredExpirer(tester.nodeRepository(), deployer, tester.clock(), Duration.ofHours(12), new JobControl(tester.nodeRepository().database())).run();
+        new RetiredExpirer(tester.nodeRepository(), tester.orchestrator(), deployer, tester.clock(), Duration.ofDays(30),
+                Duration.ofHours(12), new JobControl(tester.nodeRepository().database())).run();
 
         assertEquals(8, tester.getNodes(application1, Node.State.inactive).flavor("small").size());
         deploy(application1, 8, 8, "large", tester);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
index 3c932f492db..ffd98cc6a1a 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
@@ -29,6 +29,7 @@ import com.yahoo.vespa.hosted.provision.node.Allocation;
 import com.yahoo.vespa.hosted.provision.node.filter.NodeHostFilter;
 import com.yahoo.vespa.hosted.provision.persistence.NameResolver;
 import com.yahoo.vespa.hosted.provision.testutils.MockNameResolver;
+import com.yahoo.vespa.orchestrator.Orchestrator;
 
 import java.io.IOException;
 import java.time.temporal.TemporalAmount;
@@ -45,6 +46,9 @@ import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
 
 /**
  * A test utility for provisioning tests.
@@ -57,6 +61,7 @@ public class ProvisioningTester implements AutoCloseable {
     private final NodeFlavors nodeFlavors;
     private final ManualClock clock;
     private final NodeRepository nodeRepository;
+    private final Orchestrator orchestrator;
     private final NodeRepositoryProvisioner provisioner;
     private final CapacityPolicies capacityPolicies;
     private final ProvisionLogger provisionLogger;
@@ -80,6 +85,8 @@ public class ProvisioningTester implements AutoCloseable {
             this.curator = curator;
             this.nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone, nameResolver,
                                                      new DockerImage("docker-registry.domain.tld:8080/dist/vespa"));
+            this.orchestrator = mock(Orchestrator.class);
+            doThrow(new RuntimeException()).when(orchestrator).acquirePermissionToRemove(any());
             this.provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone, clock,
                     (x,y) -> allocationSnapshots.add(new AllocationSnapshot(new NodeList(x), "Provision tester", y)));
             this.capacityPolicies = new CapacityPolicies(zone, nodeFlavors);
@@ -124,6 +131,7 @@ public class ProvisioningTester implements AutoCloseable {
 
     public void advanceTime(TemporalAmount duration) { clock.advance(duration); }
     public NodeRepository nodeRepository() { return nodeRepository; }
+    public Orchestrator orchestrator() { return orchestrator; }
     public ManualClock clock() { return clock; }
     public NodeRepositoryProvisioner provisioner() { return provisioner; }
     public CapacityPolicies capacityPolicies() { return capacityPolicies; }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json
index c09829e7f85..979d846704e 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json
@@ -31,9 +31,6 @@
       "name":"ProvisionedExpirer"
     },
     {
-      "name":"RetiredEarlyExpirer"
-    },
-    {
       "name":"MetricsReporter"
     },
     {