Merge pull request #9857 from vespa-engine/mpolden/lb-expiry

Expire reserved load balancers

6 files changed, 103 insertions, 21 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java
index 369366a1f08..6f7b7c4d57d 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java
@@ -51,7 +51,7 @@ public class LoadBalancer {
             throw new IllegalArgumentException("Invalid changeAt: '" + changedAt + "' is before existing value '" +
                                                this.changedAt + "'");
         }
-        if (this.state == State.active && state == State.reserved) {
+        if (this.state != State.reserved && state == State.reserved) {
             throw new IllegalArgumentException("Invalid state transition: " + this.state + " -> " + state);
         }
         return new LoadBalancer(id, instance, state, changedAt);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java
index c0bb53ddfe4..7fd50bf0930 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java
@@ -3,18 +3,20 @@ package com.yahoo.vespa.hosted.provision.lb;
 
 import com.yahoo.config.provision.ApplicationId;
 
+import java.time.Instant;
 import java.util.Collection;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Objects;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 /**
- * A filterable load balancer list.
+ * A filterable load balancer list. This is immutable.
  *
  * @author mpolden
  */
-public class LoadBalancerList {
+public class LoadBalancerList implements Iterable<LoadBalancer> {
 
     private final List<LoadBalancer> loadBalancers;
 
@@ -27,9 +29,14 @@ public class LoadBalancerList {
         return of(loadBalancers.stream().filter(lb -> lb.id().application().equals(application)));
     }
 
-    /** Returns the subset of load balancers that are inactive */
-    public LoadBalancerList inactive() {
-        return of(loadBalancers.stream().filter(lb -> lb.state() == LoadBalancer.State.inactive));
+    /** Returns the subset of load balancers that are in given state */
+    public LoadBalancerList in(LoadBalancer.State state) {
+        return of(loadBalancers.stream().filter(lb -> lb.state() == state));
+    }
+
+    /** Returns the subset of load balancers that last changed before given instant */
+    public LoadBalancerList changedBefore(Instant instant) {
+        return of(loadBalancers.stream().filter(lb -> lb.changedAt().isBefore(instant)));
     }
 
     public List<LoadBalancer> asList() {
@@ -40,4 +47,9 @@ public class LoadBalancerList {
         return new LoadBalancerList(stream.collect(Collectors.toUnmodifiableList()));
     }
 
+    @Override
+    public Iterator<LoadBalancer> iterator() {
+        return loadBalancers.iterator();
+    }
+
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java
index d6b392c4d64..7d7f8d479fe 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java
@@ -6,6 +6,7 @@ import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancer;
+import com.yahoo.vespa.hosted.provision.lb.LoadBalancer.State;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancerId;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancerService;
 import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient;
@@ -17,15 +18,21 @@ import java.util.Objects;
 import java.util.stream.Collectors;
 
 /**
- * Periodically remove inactive load balancers permanently.
+ * Periodically expire load balancers.
  *
- * When an application is removed, any associated load balancers are only deactivated. This maintainer ensures that
- * underlying load balancer instances are eventually freed.
+ * Load balancers expire from the following states:
+ *
+ * {@link LoadBalancer.State#inactive}: An application is removed and load balancers are deactivated.
+ * {@link LoadBalancer.State#reserved}: An prepared application is never successfully activated, thus never activating
+ *                                      any prepared load balancers.
  *
  * @author mpolden
  */
 public class LoadBalancerExpirer extends Maintainer {
 
+    private static final Duration reservedExpiry = Duration.ofHours(1);
+    private static final Duration inactiveExpiry = Duration.ofHours(1);
+
     private final LoadBalancerService service;
     private final CuratorDatabaseClient db;
 
@@ -37,22 +44,39 @@ public class LoadBalancerExpirer extends Maintainer {
 
     @Override
     protected void maintain() {
+        expireReserved();
         removeInactive();
     }
 
+    private void expireReserved() {
+        try (Lock lock = db.lockLoadBalancers()) {
+            var now = nodeRepository().clock().instant();
+            var expirationTime = now.minus(reservedExpiry);
+            var expired = nodeRepository().loadBalancers()
+                                          .in(State.reserved)
+                                          .changedBefore(expirationTime);
+            expired.forEach(lb -> db.writeLoadBalancer(lb.with(State.inactive, now)));
+        }
+    }
+
     private void removeInactive() {
         List<LoadBalancerId> failed = new ArrayList<>();
         Exception lastException = null;
         try (Lock lock = db.lockLoadBalancers()) {
-            for (LoadBalancer loadBalancer : nodeRepository().loadBalancers().inactive().asList()) {
-                if (hasNodes(loadBalancer.id().application())) { // Defer removal if there are still nodes allocated to application
+            var now = nodeRepository().clock().instant();
+            var expirationTime = now.minus(inactiveExpiry);
+            var expired = nodeRepository().loadBalancers()
+                                          .in(State.inactive)
+                                          .changedBefore(expirationTime);
+            for (var lb : expired) {
+                if (hasNodes(lb.id().application())) { // Defer removal if there are still nodes allocated to application
                     continue;
                 }
                 try {
-                    service.remove(loadBalancer.id().application(), loadBalancer.id().cluster());
-                    db.removeLoadBalancer(loadBalancer.id());
+                    service.remove(lb.id().application(), lb.id().cluster());
+                    db.removeLoadBalancer(lb.id());
                 } catch (Exception e) {
-                    failed.add(loadBalancer.id());
+                    failed.add(lb.id());
                     lastException = e;
                 }
             }
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 25549abe9ed..b71c7c7ec81 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
@@ -7,7 +7,6 @@ import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.HostLivenessTracker;
 import com.yahoo.config.provision.InfraDeployer;
-import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.vespa.flags.FlagSource;
@@ -176,7 +175,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
             metricsInterval = Duration.ofMinutes(1);
             infrastructureProvisionInterval = Duration.ofMinutes(1);
             throttlePolicy = NodeFailer.ThrottlePolicy.hosted;
-            loadBalancerExpiry = Duration.ofHours(1);
+            loadBalancerExpiry = Duration.ofMinutes(10);
             reservationExpiry = Duration.ofMinutes(20); // Need to be long enough for deployment to be finished for all config model versions
             hostProvisionerInterval = Duration.ofMinutes(5);
             hostDeprovisionerInterval = Duration.ofMinutes(5);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java
index c0ea64d4c68..33faf1eaf76 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java
@@ -33,7 +33,7 @@ public class LoadBalancerExpirerTest {
     private ProvisioningTester tester = new ProvisioningTester.Builder().build();
 
     @Test
-    public void test_maintain() {
+    public void test_remove_inactive() {
         LoadBalancerExpirer expirer = new LoadBalancerExpirer(tester.nodeRepository(),
                                                               Duration.ofDays(1),
                                                               tester.loadBalancerService());
@@ -57,9 +57,14 @@ public class LoadBalancerExpirerTest {
         // Expirer defers removal while nodes are still allocated to application
         expirer.maintain();
         assertEquals(2, tester.loadBalancerService().instances().size());
-
-        // Expirer removes load balancers once nodes are deallocated
         dirtyNodesOf(app1);
+
+        // Expirer defers removal until expiration time passes
+        expirer.maintain();
+        assertTrue("Inactive load balancer not removed", tester.loadBalancerService().instances().containsKey(lb1));
+
+        // Expirer removes load balancers once expiration time passes
+        tester.clock().advance(Duration.ofHours(1));
         expirer.maintain();
         assertFalse("Inactive load balancer removed", tester.loadBalancerService().instances().containsKey(lb1));
 
@@ -68,6 +73,43 @@ public class LoadBalancerExpirerTest {
         assertTrue("Active load balancer is not removed", tester.loadBalancerService().instances().containsKey(lb2));
     }
 
+    @Test
+    public void test_expire_reserved() {
+        LoadBalancerExpirer expirer = new LoadBalancerExpirer(tester.nodeRepository(),
+                                                              Duration.ofDays(1),
+                                                              tester.loadBalancerService());
+        Supplier<Map<LoadBalancerId, LoadBalancer>> loadBalancers = () -> tester.nodeRepository().database().readLoadBalancers();
+
+
+        // Prepare application
+        ClusterSpec.Id cluster = ClusterSpec.Id.from("qrs");
+        ApplicationId app = tester.makeApplicationId();
+        LoadBalancerId lb = new LoadBalancerId(app, cluster);
+        deployApplication(app, cluster, false);
+
+        // Provisions load balancer in reserved
+        assertSame(LoadBalancer.State.reserved, loadBalancers.get().get(lb).state());
+
+        // Expirer does nothing
+        expirer.maintain();
+        assertSame(LoadBalancer.State.reserved, loadBalancers.get().get(lb).state());
+
+        // Application never activates and nodes are dirtied. Expirer moves load balancer to inactive after timeout
+        dirtyNodesOf(app);
+        tester.clock().advance(Duration.ofHours(1));
+        expirer.maintain();
+        assertSame(LoadBalancer.State.inactive, loadBalancers.get().get(lb).state());
+
+        // Expirer does nothing as inactive expiration time has not yet passed
+        expirer.maintain();
+        assertSame(LoadBalancer.State.inactive, loadBalancers.get().get(lb).state());
+
+        // Expirer removes inactive load balancer
+        tester.clock().advance(Duration.ofHours(1));
+        expirer.maintain();
+        assertFalse("Inactive load balancer removed", loadBalancers.get().containsKey(lb));
+    }
+
     private void dirtyNodesOf(ApplicationId application) {
         tester.nodeRepository().setDirty(tester.nodeRepository().getNodes(application), Agent.system, this.getClass().getSimpleName());
     }
@@ -79,12 +121,18 @@ public class LoadBalancerExpirerTest {
     }
 
     private void deployApplication(ApplicationId application, ClusterSpec.Id cluster) {
+        deployApplication(application, cluster, true);
+    }
+
+    private void deployApplication(ApplicationId application, ClusterSpec.Id cluster, boolean activate) {
         tester.makeReadyNodes(10, "d-1-1-1");
         List<HostSpec> hosts = tester.prepare(application, ClusterSpec.request(ClusterSpec.Type.container, cluster,
                                                                                Vtag.currentVersion, false, Collections.emptySet()),
                                               2, 1,
                                               new NodeResources(1, 1, 1));
-        tester.activate(application, hosts);
+        if (activate) {
+            tester.activate(application, hosts);
+        }
     }
 
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerAllocationTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerAllocationTest.java
index 50e19e15da5..1d8ca6aa57a 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerAllocationTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerAllocationTest.java
@@ -386,7 +386,6 @@ public class DynamicDockerAllocationTest {
         tester.activate(application, hosts1);
 
         NodeResources resources = new NodeResources(1.5, 8, 50);
-        System.out.println("Redeploying with " + resources);
         List<HostSpec> hosts2 = tester.prepare(application, cluster, Capacity.fromCount(2, resources), 1);
         tester.activate(application, hosts2);