Merge pull request #16425 from vespa-engine/mpolden/less-lb-create

Avoid reconfiguring load balancer on unchanged reals

3 files changed, 53 insertions, 20 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java
index f4d689056c3..b912087da46 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java
@@ -17,11 +17,17 @@ import java.util.Optional;
 public class LoadBalancerServiceMock implements LoadBalancerService {
 
     private final Map<LoadBalancerId, LoadBalancerInstance> instances = new HashMap<>();
+    private boolean throwOnCreate = false;
 
     public Map<LoadBalancerId, LoadBalancerInstance> instances() {
         return Collections.unmodifiableMap(instances);
     }
 
+    public LoadBalancerServiceMock throwOnCreate(boolean throwOnCreate) {
+        this.throwOnCreate = throwOnCreate;
+        return this;
+    }
+
     @Override
     public Protocol protocol() {
         return Protocol.ipv4;
@@ -29,6 +35,7 @@ public class LoadBalancerServiceMock implements LoadBalancerService {
 
     @Override
     public LoadBalancerInstance create(LoadBalancerSpec spec, boolean force) {
+        if (throwOnCreate) throw new IllegalStateException("Did not expect a new load balancer to be created");
         var id = new LoadBalancerId(spec.application(), spec.cluster());
         var oldInstance = instances.get(id);
         if (!force && oldInstance != null && !oldInstance.reals().isEmpty() && spec.reals().isEmpty()) {
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 cfcaa1f83c0..356b27dfe2c 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
@@ -24,11 +24,13 @@ import com.yahoo.vespa.hosted.provision.lb.Real;
 import com.yahoo.vespa.hosted.provision.node.IP;
 import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient;
 
+import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
 import java.util.logging.Level;
@@ -166,12 +168,10 @@ public class LoadBalancerProvisioner {
 
     /** Idempotently provision a load balancer for given application and cluster */
     private void provision(ApplicationTransaction transaction, LoadBalancerId id, List<Node> nodes, boolean activate) {
-        var now = nodeRepository.clock().instant();
-        var loadBalancer = db.readLoadBalancer(id);
+        Instant now = nodeRepository.clock().instant();
+        Optional<LoadBalancer> loadBalancer = db.readLoadBalancer(id);
         if (loadBalancer.isEmpty() && activate) return; // Nothing to activate as this load balancer was never prepared
-
-        var force = loadBalancer.isPresent() && loadBalancer.get().state() != LoadBalancer.State.active;
-        var instance = provisionInstance(id, nodes, force);
+        LoadBalancerInstance instance = provisionInstance(id, realsOf(nodes), loadBalancer);
         LoadBalancer newLoadBalancer;
         if (loadBalancer.isEmpty()) {
             newLoadBalancer = new LoadBalancer(id, instance, LoadBalancer.State.reserved, now);
@@ -189,16 +189,14 @@ public class LoadBalancerProvisioner {
         provision(transaction, new LoadBalancerId(transaction.application(), clusterId), nodes, true);
     }
 
-    private LoadBalancerInstance provisionInstance(LoadBalancerId id, List<Node> nodes, boolean force) {
-        var reals = new LinkedHashSet<Real>();
-        for (var node : nodes) {
-            for (var ip : reachableIpAddresses(node)) {
-                reals.add(new Real(HostName.from(node.hostname()), ip));
-            }
-        }
+    /** Provision or reconfigure a load balancer instance, if necessary */
+    private LoadBalancerInstance provisionInstance(LoadBalancerId id, Set<Real> reals,
+                                                   Optional<LoadBalancer> currentLoadBalancer) {
+        if (hasReals(currentLoadBalancer, reals)) return currentLoadBalancer.get().instance();
         log.log(Level.FINE, "Creating " + id + ", targeting: " + reals);
         try {
-            return service.create(new LoadBalancerSpec(id.application(), id.cluster(), reals), force);
+            return service.create(new LoadBalancerSpec(id.application(), id.cluster(), reals),
+                                  allowEmptyReals(currentLoadBalancer));
         } catch (Exception e) {
             throw new LoadBalancerServiceException("Failed to (re)configure " + id + ", targeting: " +
                                                    reals + ". The operation will be retried on next deployment", e);
@@ -224,6 +222,17 @@ public class LoadBalancerProvisioner {
         return nodes.stream().collect(Collectors.groupingBy(node -> effectiveId(node.allocation().get().membership().cluster())));
     }
 
+    /** Returns real servers for given nodes */
+    private Set<Real> realsOf(List<Node> nodes) {
+        var reals = new LinkedHashSet<Real>();
+        for (var node : nodes) {
+            for (var ip : reachableIpAddresses(node)) {
+                reals.add(new Real(HostName.from(node.hostname()), ip));
+            }
+        }
+        return reals;
+    }
+
     /** Returns a list of the non-compactable IDs of given load balancer */
     private static List<String> withoutCompactableIds(LoadBalancerId id) {
         List<String> ids = new ArrayList<>(2);
@@ -236,6 +245,17 @@ public class LoadBalancerProvisioner {
         return ids;
     }
 
+    /** Returns whether load balancer has given reals */
+    private static boolean hasReals(Optional<LoadBalancer> loadBalancer, Set<Real> reals) {
+        if (loadBalancer.isEmpty()) return false;
+        return loadBalancer.get().instance().reals().equals(reals);
+    }
+
+    /** Returns whether to allow given load balancer to have no reals */
+    private static boolean allowEmptyReals(Optional<LoadBalancer> loadBalancer) {
+        return loadBalancer.isPresent() && loadBalancer.get().state() != LoadBalancer.State.active;
+    }
+
     /** Find IP addresses reachable by the load balancer service */
     private Set<String> reachableIpAddresses(Node node) {
         Set<String> reachable = new LinkedHashSet<>(node.ipConfig().primary());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
index ec08ed15ab4..14f068d89a7 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
@@ -92,7 +92,7 @@ public class LoadBalancerProvisionerTest {
         assertEquals(containers.get().get(1).hostname(), get(loadBalancer.instance().reals(), 1).hostname().value());
         assertSame("State is unchanged", LoadBalancer.State.active, loadBalancer.state());
 
-        // Add another container cluster
+        // Add another container cluster to first app
         ClusterSpec.Id containerCluster2 = ClusterSpec.Id.from("qrs2");
         tester.activate(app1, prepare(app1,
                                       clusterRequest(ClusterSpec.Type.container, containerCluster1),
@@ -133,7 +133,7 @@ public class LoadBalancerProvisionerTest {
                            .findFirst()
                            .get());
 
-        // Application is removed, nodes and load balancer are deactivated
+        // Entire application is removed: Nodes and load balancer are deactivated
         tester.remove(app1);
         dirtyNodesOf(app1);
         assertTrue("No nodes are allocated to " + app1, tester.nodeRepository().getNodes(app1, Node.State.reserved, Node.State.active).isEmpty());
@@ -146,11 +146,17 @@ public class LoadBalancerProvisionerTest {
                                       clusterRequest(ClusterSpec.Type.container, containerCluster1),
                                       clusterRequest(ClusterSpec.Type.content, contentCluster)));
         assertSame("Re-activated load balancer for " + containerCluster1, LoadBalancer.State.active,
-                    lbApp1.get().stream()
-                                 .filter(lb -> lb.id().cluster().equals(containerCluster1))
-                                 .map(LoadBalancer::state)
-                                 .findFirst()
-                                 .orElseThrow());
+                   lbApp1.get().stream()
+                         .filter(lb -> lb.id().cluster().equals(containerCluster1))
+                         .map(LoadBalancer::state)
+                         .findFirst()
+                         .orElseThrow());
+
+        // Next redeploy does not create a new load balancer instance
+        tester.loadBalancerService().throwOnCreate(true);
+        tester.activate(app1, prepare(app1,
+                                      clusterRequest(ClusterSpec.Type.container, containerCluster1),
+                                      clusterRequest(ClusterSpec.Type.content, contentCluster)));
     }
 
     @Test