Remove ParkedExpirer, ProvisionedExpirer deprovisions if excessive expiry

5 files changed, 35 insertions, 106 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 cdb5202603a..96373bd764f 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
@@ -67,7 +67,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         maintainers.add(new ScalingSuggestionsMaintainer(nodeRepository, defaults.scalingSuggestionsInterval, metric));
         maintainers.add(new SwitchRebalancer(nodeRepository, defaults.switchRebalancerInterval, metric, deployer));
         maintainers.add(new HostEncrypter(nodeRepository, defaults.hostEncrypterInterval, metric));
-        maintainers.add(new ParkedExpirer(nodeRepository, defaults.parkedExpirerInterval, metric));
 
         provisionServiceProvider.getLoadBalancerService(nodeRepository)
                                 .map(lbService -> new LoadBalancerExpirer(nodeRepository, defaults.loadBalancerExpirerInterval, lbService, metric))
@@ -120,7 +119,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         private final Duration scalingSuggestionsInterval;
         private final Duration switchRebalancerInterval;
         private final Duration hostEncrypterInterval;
-        private final Duration parkedExpirerInterval;
 
         private final NodeFailer.ThrottlePolicy throttlePolicy;
 
@@ -151,7 +149,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
             throttlePolicy = NodeFailer.ThrottlePolicy.hosted;
             inactiveConfigServerExpiry = Duration.ofMinutes(5);
             inactiveControllerExpiry = Duration.ofMinutes(5);
-            parkedExpirerInterval = Duration.ofMinutes(30);
 
             if (zone.environment().isProduction() && ! zone.system().isCd()) {
                 inactiveExpiry = Duration.ofHours(4); // enough time for the application owner to discover and redeploy
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java
deleted file mode 100644
index dec1f49145b..00000000000
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java
+++ /dev/null
@@ -1,70 +0,0 @@
-// Copyright Verizon Media. 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.config.provision.NodeType;
-import com.yahoo.jdisc.Metric;
-import com.yahoo.vespa.hosted.provision.Node;
-import com.yahoo.vespa.hosted.provision.NodeList;
-import com.yahoo.vespa.hosted.provision.NodeRepository;
-import com.yahoo.vespa.hosted.provision.node.Agent;
-import com.yahoo.vespa.hosted.provision.node.History;
-
-import java.time.Duration;
-import java.time.Instant;
-import java.util.Comparator;
-import java.util.logging.Logger;
-
-/**
- *
- * Expires parked nodes in dynamically provisioned zones.
- * If number of parked hosts exceed MAX_ALLOWED_PARKED_HOSTS, recycle in a queue order
- *
- * @author olaa
- */
-public class ParkedExpirer extends NodeRepositoryMaintainer {
-
-    private static final int MAX_ALLOWED_PARKED_HOSTS = 20;
-    private static final Logger log = Logger.getLogger(ParkedExpirer.class.getName());
-
-    private final NodeRepository nodeRepository;
-
-    ParkedExpirer(NodeRepository nodeRepository, Duration interval, Metric metric) {
-        super(nodeRepository, interval, metric);
-        this.nodeRepository = nodeRepository;
-    }
-
-    @Override
-    protected double maintain() {
-        if (!nodeRepository.zone().getCloud().dynamicProvisioning())
-            return 1.0;
-
-        NodeList parkedHosts = nodeRepository.nodes()
-                                             .list(Node.State.parked)
-                                             .nodeType(NodeType.host)
-                                             .not().matching(this::parkedByOperator)
-                                             .not().deprovisioning();
-        int hostsToExpire = Math.max(0, parkedHosts.size() - MAX_ALLOWED_PARKED_HOSTS);
-        parkedHosts.sortedBy(Comparator.comparing(this::parkedAt))
-                   .first(hostsToExpire)
-                   .forEach(host -> {
-                       log.info("Allowed number of parked nodes exceeded. Recycling " + host.hostname());
-                       nodeRepository.nodes().deprovision(host.hostname(), Agent.ParkedExpirer, Instant.now());
-                   });
-
-        return 1.0;
-    }
-
-    private Instant parkedAt(Node node) {
-        return node.history().event(History.Event.Type.parked)
-                   .map(History.Event::at)
-                   .orElse(Instant.EPOCH); // Should not happen
-    }
-
-    private boolean parkedByOperator(Node node) {
-        return node.history().event(History.Event.Type.parked)
-                .map(History.Event::agent)
-                .map(Agent.operator::equals)
-                .orElse(false);
-    }
-
-}
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java
index 856d534bbd2..c71eef0abea 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java
@@ -4,15 +4,18 @@ package com.yahoo.vespa.hosted.provision.maintenance;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.NodeList;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.hosted.provision.node.History;
 
 import java.time.Duration;
+import java.time.Instant;
 import java.util.List;
 
 /**
  * This moves nodes of type {@link NodeType#host} from provisioned to parked if they have been in provisioned too long.
+ * Parked hosts are deprovisioned as well, if too many hosts are being expired.
  *
  * Only {@link NodeType#host} is moved because any number of nodes of that type can exist. Other node types such as
  * {@link NodeType#confighost} have a fixed number and thus cannot be replaced while the fixed number of nodes exist in
@@ -22,17 +25,40 @@ import java.util.List;
  */
 public class ProvisionedExpirer extends Expirer {
 
+    private final NodeRepository nodeRepository;
+    private static final int MAXIMUM_ALLOWED_EXPIRED_HOSTS = 20;
+
     ProvisionedExpirer(NodeRepository nodeRepository, Duration dirtyTimeout, Metric metric) {
         super(Node.State.provisioned, History.Event.Type.provisioned, nodeRepository, dirtyTimeout, metric);
+        this.nodeRepository = nodeRepository;
     }
 
     @Override
     protected void expire(List<Node> expired) {
+        int previouslyExpired = numberOfPreviouslyExpired();
         for (Node expiredNode : expired) {
-            if (expiredNode.type() == NodeType.host) {
-                nodeRepository().nodes().parkRecursively(expiredNode.hostname(), Agent.ProvisionedExpirer, "Node is stuck in provisioned");
+            if (expiredNode.type() != NodeType.host)
+                continue;
+            nodeRepository().nodes().parkRecursively(expiredNode.hostname(), Agent.ProvisionedExpirer, "Node is stuck in provisioned");
+            if (MAXIMUM_ALLOWED_EXPIRED_HOSTS < ++previouslyExpired) {
+                nodeRepository.nodes().deprovision(expiredNode.hostname(), Agent.ProvisionedExpirer, Instant.now());
             }
         }
     }
 
+    private int numberOfPreviouslyExpired() {
+        return nodeRepository.nodes()
+                .list(Node.State.parked)
+                .nodeType(NodeType.host)
+                .matching(this::parkedByProvisionedExpirer)
+                .not().deprovisioning()
+                .size();
+    }
+
+    private boolean parkedByProvisionedExpirer(Node node) {
+        return node.history().event(History.Event.Type.parked)
+                .map(History.Event::agent)
+                .map(Agent.ProvisionedExpirer::equals)
+                .orElse(false);
+    }
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirerTest.java
index c85a38cc6aa..786faae24b4 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirerTest.java
@@ -24,48 +24,27 @@ import static org.junit.Assert.*;
 /**
  * @author olaa
  */
-public class ParkedExpirerTest {
+public class ProvisionedExpirerTest {
 
     private ProvisioningTester tester;
 
     @Test
-    public void noop_if_not_dynamic_provisioning() {
-        tester = getTester(false);
+    public void deprovisions_hosts_if_excessive_expiry() {
+        tester = new ProvisioningTester.Builder().build();
         populateNodeRepo();
 
-        var expirer = new ParkedExpirer(tester.nodeRepository(), Duration.ofMinutes(4), new TestMetric());
-        expirer.maintain();
-
-        assertEquals(1, tester.nodeRepository().nodes().list().deprovisioning().size());
-        assertEquals(24, tester.nodeRepository().nodes().list().not().deprovisioning().size());
-    }
-
-    @Test
-    public void recycles_correct_subset_of_parked_hosts() {
-        tester = getTester(true);
-        populateNodeRepo();
-
-        var expirer = new ParkedExpirer(tester.nodeRepository(), Duration.ofMinutes(4), new TestMetric());
-        expirer.maintain();
+        tester.clock().advance(Duration.ofMinutes(5));
+        new ProvisionedExpirer(tester.nodeRepository(), Duration.ofMinutes(4), new TestMetric()).maintain();
 
         assertEquals(5, tester.nodeRepository().nodes().list().deprovisioning().size());
         assertEquals(20, tester.nodeRepository().nodes().list().not().deprovisioning().size());
-
-    }
-
-    private ProvisioningTester getTester(boolean dynamicProvisioning) {
-        var zone = new Zone(Cloud.builder().dynamicProvisioning(dynamicProvisioning).build(), SystemName.main, Environment.prod, RegionName.from("us-east"));
-        return new ProvisioningTester.Builder().zone(zone)
-                .hostProvisioner(dynamicProvisioning ? new MockHostProvisioner(List.of()) : null)
-                .build();
     }
 
     private void populateNodeRepo() {
         var nodes = IntStream.range(0, 25)
-                             .mapToObj(i -> Node.create("id-" + i, "host-" + i, new Flavor(NodeResources.unspecified()), Node.State.parked, NodeType.host).build())
+                             .mapToObj(i -> Node.create("id-" + i, "host-" + i, new Flavor(NodeResources.unspecified()), Node.State.provisioned, NodeType.host).build())
                              .collect(Collectors.toList());
-        tester.nodeRepository().database().addNodesInState(nodes, Node.State.parked, Agent.system);
-        tester.nodeRepository().nodes().deprovision(nodes.get(0).hostname(), Agent.system, tester.clock().instant()); // Deprovisioning host is not recycled
+        tester.nodeRepository().database().addNodesInState(nodes, Node.State.provisioned, Agent.system);
     }
 
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/maintenance.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/maintenance.json
index 26d711945c6..72224ef3cba 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/maintenance.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/maintenance.json
@@ -43,9 +43,6 @@
       "name": "OsUpgradeActivator"
     },
     {
-      "name": "ParkedExpirer"
-    },
-    {
       "name": "PeriodicApplicationMaintainer"
     },
     {