Merge pull request #18279 from vespa-engine/olaa/deprovision-excess-parked-hosts

Deprovision nodes if not parked by operator

6 files changed, 78 insertions, 141 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 ec7826658e3..00000000000
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java
+++ /dev/null
@@ -1,62 +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().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().deallocate(host, Agent.ParkedExpirer, "Expired by ParkedExpirer");
-                   });
-
-        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
-    }
-
-}
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..76c8210338e 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, nodeRepository.clock().instant());
             }
         }
     }
 
+    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/ParkedExpirerTest.java
deleted file mode 100644
index bc60801c1d6..00000000000
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java
+++ /dev/null
@@ -1,71 +0,0 @@
-package com.yahoo.vespa.hosted.provision.maintenance;
-
-import com.yahoo.config.provision.Cloud;
-import com.yahoo.config.provision.Environment;
-import com.yahoo.config.provision.Flavor;
-import com.yahoo.config.provision.NodeResources;
-import com.yahoo.config.provision.NodeType;
-import com.yahoo.config.provision.RegionName;
-import com.yahoo.config.provision.SystemName;
-import com.yahoo.config.provision.Zone;
-import com.yahoo.vespa.hosted.provision.Node;
-import com.yahoo.vespa.hosted.provision.node.Agent;
-import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester;
-import com.yahoo.vespa.hosted.provision.testutils.MockHostProvisioner;
-import org.junit.Test;
-
-import java.time.Duration;
-import java.util.List;
-import java.util.stream.Collectors;
-import java.util.stream.IntStream;
-
-import static org.junit.Assert.*;
-
-/**
- * @author olaa
- */
-public class ParkedExpirerTest {
-
-    private ProvisioningTester tester;
-
-    @Test
-    public void noop_if_not_dynamic_provisioning() {
-        tester = getTester(false);
-        populateNodeRepo();
-
-        var expirer = new ParkedExpirer(tester.nodeRepository(), Duration.ofMinutes(4), new TestMetric());
-        expirer.maintain();
-
-        assertEquals(0, tester.nodeRepository().nodes().list(Node.State.dirty).size());
-        assertEquals(25, tester.nodeRepository().nodes().list(Node.State.parked).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();
-
-        assertEquals(4, tester.nodeRepository().nodes().list(Node.State.dirty).size());
-        assertEquals(21, tester.nodeRepository().nodes().list(Node.State.parked).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())
-                             .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
-    }
-
-}
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirerTest.java
new file mode 100644
index 00000000000..786faae24b4
--- /dev/null
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirerTest.java
@@ -0,0 +1,50 @@
+package com.yahoo.vespa.hosted.provision.maintenance;
+
+import com.yahoo.config.provision.Cloud;
+import com.yahoo.config.provision.Environment;
+import com.yahoo.config.provision.Flavor;
+import com.yahoo.config.provision.NodeResources;
+import com.yahoo.config.provision.NodeType;
+import com.yahoo.config.provision.RegionName;
+import com.yahoo.config.provision.SystemName;
+import com.yahoo.config.provision.Zone;
+import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.node.Agent;
+import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester;
+import com.yahoo.vespa.hosted.provision.testutils.MockHostProvisioner;
+import org.junit.Test;
+
+import java.time.Duration;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static org.junit.Assert.*;
+
+/**
+ * @author olaa
+ */
+public class ProvisionedExpirerTest {
+
+    private ProvisioningTester tester;
+
+    @Test
+    public void deprovisions_hosts_if_excessive_expiry() {
+        tester = new ProvisioningTester.Builder().build();
+        populateNodeRepo();
+
+        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 void populateNodeRepo() {
+        var nodes = IntStream.range(0, 25)
+                             .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.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"
     },
     {