Merge pull request #11598 from vespa-engine/mpolden/clear-reals-inactive-lb

Prune reals no longer allocated to application

8 files changed, 69 insertions, 25 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
index f86c05da3c2..682d1419a5c 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
@@ -193,7 +193,7 @@ public class NodeRepository extends AbstractComponent {
 
     /** Returns a filterable list of all load balancers in this repository */
     public LoadBalancerList loadBalancers() {
-        return new LoadBalancerList(database().readLoadBalancers().values());
+        return LoadBalancerList.copyOf(database().readLoadBalancers().values());
     }
 
     public List<Node> getNodes(ApplicationId id, Node.State ... inState) { return db.getNodes(id, inState); }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java
index c1b4fe9b0f2..1e60b689489 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java
@@ -56,6 +56,11 @@ public class LoadBalancerInstance {
         return reals;
     }
 
+    /** Returns a copy of this with reals set to given reals */
+    public LoadBalancerInstance withReals(Set<Real> reals) {
+        return new LoadBalancerInstance(hostname, dnsZone, ports, networks, reals);
+    }
+
     private static Set<Integer> requirePorts(Set<Integer> ports) {
         Objects.requireNonNull(ports, "ports must be non-null");
         if (ports.isEmpty()) {
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 7fd50bf0930..014d3df8d9a 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
@@ -20,7 +20,7 @@ public class LoadBalancerList implements Iterable<LoadBalancer> {
 
     private final List<LoadBalancer> loadBalancers;
 
-    public LoadBalancerList(Collection<LoadBalancer> loadBalancers) {
+    private LoadBalancerList(Collection<LoadBalancer> loadBalancers) {
         this.loadBalancers = List.copyOf(Objects.requireNonNull(loadBalancers, "loadBalancers must be non-null"));
     }
 
@@ -47,6 +47,10 @@ public class LoadBalancerList implements Iterable<LoadBalancer> {
         return new LoadBalancerList(stream.collect(Collectors.toUnmodifiableList()));
     }
 
+    public static LoadBalancerList copyOf(Collection<LoadBalancer> loadBalancers) {
+        return new LoadBalancerList(loadBalancers);
+    }
+
     @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 a8268fe7237..bcb0c901f14 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
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.provision.maintenance;
 
 import com.yahoo.log.LogLevel;
+import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancer;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancer.State;
@@ -11,6 +12,7 @@ import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient;
 
 import java.time.Duration;
 import java.util.ArrayList;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Objects;
 import java.util.stream.Collectors;
@@ -44,8 +46,10 @@ public class LoadBalancerExpirer extends Maintainer {
     protected void maintain() {
         expireReserved();
         removeInactive();
+        pruneReals();
     }
 
+    /** Move reserved load balancer that have expired to inactive */
     private void expireReserved() {
         try (var lock = db.lockLoadBalancers()) {
             var now = nodeRepository().clock().instant();
@@ -57,8 +61,9 @@ public class LoadBalancerExpirer extends Maintainer {
         }
     }
 
+    /** Deprovision inactive load balancers that have expired */
     private void removeInactive() {
-        List<LoadBalancerId> failed = new ArrayList<>();
+        var failed = new ArrayList<LoadBalancerId>();
         Exception lastException = null;
         try (var lock = db.lockLoadBalancers()) {
             var now = nodeRepository().clock().instant();
@@ -67,9 +72,7 @@ public class LoadBalancerExpirer extends Maintainer {
                                           .in(State.inactive)
                                           .changedBefore(expirationTime);
             for (var lb : expired) {
-                if (hasNodes(lb.id())) { // Defer removal if there are still nodes allocated
-                    continue;
-                }
+                if (!allocatedNodes(lb.id()).isEmpty()) continue; // Defer removal if there are still nodes allocated to application
                 try {
                     service.remove(lb.id().application(), lb.id().cluster());
                     db.removeLoadBalancer(lb.id());
@@ -90,8 +93,39 @@ public class LoadBalancerExpirer extends Maintainer {
         }
     }
 
-    private boolean hasNodes(LoadBalancerId loadBalancer) {
-        return nodeRepository().list().owner(loadBalancer.application()).cluster(loadBalancer.cluster()).size() > 0;
+    /** Remove reals from inactive load balancers */
+    private void pruneReals() {
+        var failed = new ArrayList<LoadBalancerId>();
+        Exception lastException = null;
+        try (var lock = db.lockLoadBalancers()) {
+            var deactivated = nodeRepository().loadBalancers().in(State.inactive);
+            for (var lb : deactivated) {
+                var allocatedNodes = allocatedNodes(lb.id()).stream().map(Node::hostname).collect(Collectors.toSet());
+                var reals = new LinkedHashSet<>(lb.instance().reals());
+                // Remove any real no longer allocated to this application
+                reals.removeIf(real -> !allocatedNodes.contains(real.hostname().value()));
+                try {
+                    service.create(lb.id().application(), lb.id().cluster(), reals, true);
+                    db.writeLoadBalancer(lb.with(lb.instance().withReals(reals)));
+                } catch (Exception e) {
+                    failed.add(lb.id());
+                    lastException = e;
+                }
+            }
+        }
+        if (!failed.isEmpty()) {
+            log.log(LogLevel.WARNING, String.format("Failed to remove reals from %d load balancers: %s, retrying in %s",
+                                                    failed.size(),
+                                                    failed.stream()
+                                                          .map(LoadBalancerId::serializedForm)
+                                                          .collect(Collectors.joining(", ")),
+                                                    interval()),
+                    lastException);
+        }
+    }
+
+    private List<Node> allocatedNodes(LoadBalancerId loadBalancer) {
+        return nodeRepository().list().owner(loadBalancer.application()).cluster(loadBalancer.cluster()).asList();
     }
 
 }
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 90daa0ad279..10aff833584 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
@@ -164,7 +164,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
             metricsInterval = Duration.ofMinutes(1);
             infrastructureProvisionInterval = Duration.ofMinutes(1);
             throttlePolicy = NodeFailer.ThrottlePolicy.hosted;
-            loadBalancerExpirerInterval = Duration.ofMinutes(10);
+            loadBalancerExpirerInterval = Duration.ofMinutes(5);
             reservationExpiry = Duration.ofMinutes(20); // Need to be long enough for deployment to be finished for all config model versions
             dynamicProvisionerInterval = Duration.ofMinutes(5);
             osUpgradeActivatorInterval = zone.system().isCd() ? Duration.ofSeconds(30) : Duration.ofMinutes(5);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
index a7b37628289..ee786ea414e 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
@@ -9,10 +9,6 @@ import com.yahoo.config.provision.exception.LoadBalancerServiceException;
 import com.yahoo.log.LogLevel;
 import com.yahoo.transaction.Mutex;
 import com.yahoo.transaction.NestedTransaction;
-import com.yahoo.vespa.flags.BooleanFlag;
-import com.yahoo.vespa.flags.FetchVector;
-import com.yahoo.vespa.flags.FlagSource;
-import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
@@ -28,7 +24,6 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.function.Function;
 import java.util.logging.Logger;
@@ -51,7 +46,7 @@ public class LoadBalancerProvisioner {
     private final CuratorDatabaseClient db;
     private final LoadBalancerService service;
 
-    public LoadBalancerProvisioner(NodeRepository nodeRepository, LoadBalancerService service, FlagSource flagSource) {
+    public LoadBalancerProvisioner(NodeRepository nodeRepository, LoadBalancerService service) {
         this.nodeRepository = nodeRepository;
         this.db = nodeRepository.database();
         this.service = service;
@@ -118,7 +113,7 @@ public class LoadBalancerProvisioner {
         }
     }
 
-    /** Returns load balancers of given application that are no longer referenced by wantedClusters */
+    /** Returns load balancers of given application that are no longer referenced by given clusters */
     private List<LoadBalancer> surplusLoadBalancersOf(ApplicationId application, Set<ClusterSpec.Id> activeClusters) {
         var activeLoadBalancersByCluster = nodeRepository.loadBalancers()
                                                          .owner(application)
@@ -165,13 +160,12 @@ public class LoadBalancerProvisioner {
     }
 
     private LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, List<Node> nodes, boolean force) {
-        Map<HostName, Set<String>> hostnameToIpAdresses = nodes.stream()
-                                                               .collect(Collectors.toMap(node -> HostName.from(node.hostname()),
-                                                                                         this::reachableIpAddresses));
-        Set<Real> reals = new LinkedHashSet<>();
-        hostnameToIpAdresses.forEach((hostname, ipAddresses) -> {
-            ipAddresses.forEach(ipAddress -> reals.add(new Real(hostname, ipAddress)));
-        });
+        var reals = new LinkedHashSet<Real>();
+        for (var node : nodes) {
+            for (var ip : reachableIpAddresses(node)) {
+                reals.add(new Real(HostName.from(node.hostname()), ip));
+            }
+        }
         log.log(LogLevel.INFO, "Creating load balancer for " + cluster + " in " + application.toShortString() +
                                ", targeting: " + reals);
         try {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
index 67d132b4fb8..f7567683776 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
@@ -59,7 +59,7 @@ public class NodeRepositoryProvisioner implements Provisioner {
         this.nodeRepository = nodeRepository;
         this.capacityPolicies = new CapacityPolicies(zone);
         this.zone = zone;
-        this.loadBalancerProvisioner = provisionServiceProvider.getLoadBalancerService().map(lbService -> new LoadBalancerProvisioner(nodeRepository, lbService, flagSource));
+        this.loadBalancerProvisioner = provisionServiceProvider.getLoadBalancerService().map(lbService -> new LoadBalancerProvisioner(nodeRepository, lbService));
         this.preparer = new Preparer(nodeRepository,
                                      zone.environment() == Environment.prod ? SPARE_CAPACITY_PROD : SPARE_CAPACITY_NONPROD,
                                      provisionServiceProvider.getHostProvisioner(),
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 75224021464..12b48fd7a35 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
@@ -17,6 +17,7 @@ import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
@@ -60,9 +61,15 @@ public class LoadBalancerExpirerTest {
         // Expirer defers removal while nodes are still allocated to application
         expirer.maintain();
         assertEquals(3, tester.loadBalancerService().instances().size());
+        assertEquals(2, tester.loadBalancerService().instances().get(lb1).reals().size());
         dirtyNodesOf(app1, cluster1);
 
-        // Expirer defers removal until expiration time passes
+        // Expirer prunes reals before expiration time of load balancer itself
+        expirer.maintain();
+        assertEquals(Set.of(), tester.loadBalancerService().instances().get(lb1).reals());
+        assertEquals(Set.of(), tester.nodeRepository().loadBalancers().owner(lb1.application()).asList().get(0).instance().reals());
+
+        // Expirer defers removal of load balancer until expiration time passes
         expirer.maintain();
         assertTrue("Inactive load balancer not removed", tester.loadBalancerService().instances().containsKey(lb1));