Merge pull request #18220 from vespa-engine/mpolden/lb-cleanup

Log load balancer state transitions

8 files changed, 90 insertions, 57 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java
index fba0993f2f9..40f9b330634 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java
@@ -29,10 +29,7 @@ public interface LoadBalancerService {
     Protocol protocol();
 
     /** Returns whether load balancers created by this service can forward traffic to given node and cluster type */
-    default boolean canForwardTo(NodeType nodeType, ClusterSpec.Type clusterType) {
-        return (nodeType == NodeType.tenant && clusterType.isContainer()) ||
-               nodeType.isConfigServerLike();
-    }
+    boolean supports(NodeType nodeType, ClusterSpec.Type clusterType);
 
     /** Load balancer protocols */
     enum Protocol {
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 b912087da46..f752cbc4349 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
@@ -5,6 +5,7 @@ import com.google.common.collect.ImmutableSet;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.HostName;
+import com.yahoo.config.provision.NodeType;
 
 import java.util.Collections;
 import java.util.HashMap;
@@ -18,6 +19,7 @@ public class LoadBalancerServiceMock implements LoadBalancerService {
 
     private final Map<LoadBalancerId, LoadBalancerInstance> instances = new HashMap<>();
     private boolean throwOnCreate = false;
+    private boolean supportsProvisioning = true;
 
     public Map<LoadBalancerId, LoadBalancerInstance> instances() {
         return Collections.unmodifiableMap(instances);
@@ -28,6 +30,18 @@ public class LoadBalancerServiceMock implements LoadBalancerService {
         return this;
     }
 
+    public LoadBalancerServiceMock supportsProvisioning(boolean supportsProvisioning) {
+        this.supportsProvisioning = supportsProvisioning;
+        return this;
+    }
+
+    @Override
+    public boolean supports(NodeType nodeType, ClusterSpec.Type clusterType) {
+        if (!supportsProvisioning) return false;
+        return (nodeType == NodeType.tenant && clusterType.isContainer()) ||
+               nodeType.isConfigServerLike();
+    }
+
     @Override
     public Protocol protocol() {
         return Protocol.ipv4;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/PassthroughLoadBalancerService.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/PassthroughLoadBalancerService.java
index 7667672e470..9a6a65eca69 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/PassthroughLoadBalancerService.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/PassthroughLoadBalancerService.java
@@ -3,6 +3,7 @@ package com.yahoo.vespa.hosted.provision.lb;
 
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
+import com.yahoo.config.provision.NodeType;
 
 import java.util.Comparator;
 import java.util.Optional;
@@ -35,4 +36,9 @@ public class PassthroughLoadBalancerService implements LoadBalancerService {
         return Protocol.ipv4;
     }
 
+    @Override
+    public boolean supports(NodeType nodeType, ClusterSpec.Type clusterType) {
+        return true;
+    }
+
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java
index 33a3c138d70..e17e5a5a449 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java
@@ -66,7 +66,7 @@ public class SharedLoadBalancerService implements LoadBalancerService {
     }
 
     @Override
-    public boolean canForwardTo(NodeType nodeType, ClusterSpec.Type clusterType) {
+    public boolean supports(NodeType nodeType, ClusterSpec.Type clusterType) {
         // Shared routing layer only supports routing to tenant nodes
         return nodeType == NodeType.tenant && clusterType.isContainer();
     }
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 ac9d8d6671a..c3d6f5c42b8 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
@@ -65,7 +65,7 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer {
         Instant now = nodeRepository().clock().instant();
         Instant expiry = now.minus(reservedExpiry);
         patchLoadBalancers(lb -> lb.state() == State.reserved && lb.changedAt().isBefore(expiry),
-                           lb -> db.writeLoadBalancer(lb.with(State.inactive, now)));
+                           lb -> db.writeLoadBalancer(lb.with(State.inactive, now), lb.state()));
     }
 
     /** Deprovision inactive load balancers that have expired */
@@ -114,7 +114,7 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer {
                 attempts.add(1);
                 LOG.log(Level.INFO, () -> "Removing reals from inactive load balancer " + lb.id() + ": " + Sets.difference(lb.instance().get().reals(), reals));
                 service.create(new LoadBalancerSpec(lb.id().application(), lb.id().cluster(), reals), true);
-                db.writeLoadBalancer(lb.with(lb.instance().map(instance -> instance.withReals(reals))));
+                db.writeLoadBalancer(lb.with(lb.instance().map(instance -> instance.withReals(reals))), lb.state());
             } catch (Exception e) {
                 failed.add(lb.id());
                 lastException.set(e);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
index 364af6308a1..0205cc6c818 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
@@ -482,18 +482,29 @@ public class CuratorDatabaseClient {
         return read(loadBalancerPath(id), LoadBalancerSerializer::fromJson);
     }
 
-    public void writeLoadBalancer(LoadBalancer loadBalancer) {
+    public void writeLoadBalancer(LoadBalancer loadBalancer, LoadBalancer.State fromState) {
         NestedTransaction transaction = new NestedTransaction();
-        writeLoadBalancers(List.of(loadBalancer), transaction);
+        writeLoadBalancers(List.of(loadBalancer), fromState, transaction);
         transaction.commit();
     }
 
-    public void writeLoadBalancers(Collection<LoadBalancer> loadBalancers, NestedTransaction transaction) {
+    public void writeLoadBalancers(Collection<LoadBalancer> loadBalancers, LoadBalancer.State fromState, NestedTransaction transaction) {
         CuratorTransaction curatorTransaction = db.newCuratorTransactionIn(transaction);
         loadBalancers.forEach(loadBalancer -> {
             curatorTransaction.add(createOrSet(loadBalancerPath(loadBalancer.id()),
                                                LoadBalancerSerializer.toJson(loadBalancer)));
         });
+        transaction.onCommitted(() -> {
+            for (var lb : loadBalancers) {
+                if (lb.state() == fromState) continue;
+                if (fromState == null) {
+                    log.log(Level.INFO, () -> "Creating " + lb.id() + " in " + lb.state());
+                } else {
+                    log.log(Level.INFO, () -> "Moving " + lb.id() + " from " + fromState +
+                                              " to " + lb.state());
+                }
+            }
+        });
     }
 
     public void removeLoadBalancer(LoadBalancerId loadBalancer) {
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 c114aa58a05..b6600574f94 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
@@ -7,7 +7,6 @@ import com.yahoo.config.provision.ApplicationTransaction;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.NodeType;
-import com.yahoo.config.provision.ProvisionLock;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.exception.LoadBalancerServiceException;
 import com.yahoo.transaction.NestedTransaction;
@@ -61,7 +60,7 @@ public class LoadBalancerProvisioner {
         for (var id : db.readLoadBalancerIds()) {
             try (var lock = db.lock(id.application())) {
                 var loadBalancer = db.readLoadBalancer(id);
-                loadBalancer.ifPresent(db::writeLoadBalancer);
+                loadBalancer.ifPresent(lb -> db.writeLoadBalancer(lb, lb.state()));
             }
         }
     }
@@ -77,15 +76,13 @@ public class LoadBalancerProvisioner {
      * Calling this for irrelevant node or cluster types is a no-op.
      */
     public void prepare(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes) {
-        if (!service.canForwardTo(requestedNodes.type(), cluster.type())) return; // Nothing to provision for this node and cluster type
+        if (!service.supports(requestedNodes.type(), cluster.type())) return; // Nothing to provision for this node and cluster type
         if (application.instance().isTester()) return; // Do not provision for tester instances
         try (var lock = db.lock(application)) {
             ClusterSpec.Id clusterId = effectiveId(cluster);
-            NodeList nodes = nodesOf(clusterId, application);
             LoadBalancerId loadBalancerId = requireNonClashing(new LoadBalancerId(application, clusterId));
-            ApplicationTransaction transaction = new ApplicationTransaction(new ProvisionLock(application, lock), new NestedTransaction());
-            provision(transaction, loadBalancerId, nodes, false);
-            transaction.nested().commit();
+            NodeList nodes = nodesOf(clusterId, application);
+            prepare(loadBalancerId, nodes);
         }
     }
 
@@ -100,14 +97,18 @@ public class LoadBalancerProvisioner {
      * Calling this when no load balancer has been prepared for given cluster is a no-op.
      */
     public void activate(Set<ClusterSpec> clusters, ApplicationTransaction transaction) {
+        Set<ClusterSpec.Id> activatingClusters = clusters.stream()
+                                                         .map(LoadBalancerProvisioner::effectiveId)
+                                                         .collect(Collectors.toSet());
         for (var cluster : loadBalancedClustersOf(transaction.application()).entrySet()) {
-            // Provision again to ensure that load balancer instance is re-configured with correct nodes
-            provision(transaction, cluster.getKey(), cluster.getValue());
+            if (!activatingClusters.contains(cluster.getKey())) continue;
+
+            Node clusterNode = cluster.getValue().first().get();
+            if (!service.supports(clusterNode.type(), clusterNode.allocation().get().membership().cluster().type())) continue;
+            activate(transaction, cluster.getKey(), cluster.getValue());
         }
         // Deactivate any surplus load balancers, i.e. load balancers for clusters that have been removed
-        var surplusLoadBalancers = surplusLoadBalancersOf(transaction.application(), clusters.stream()
-                                                                                      .map(LoadBalancerProvisioner::effectiveId)
-                                                                                      .collect(Collectors.toSet()));
+        var surplusLoadBalancers = surplusLoadBalancersOf(transaction.application(), activatingClusters);
         deactivate(surplusLoadBalancers, transaction.nested());
     }
 
@@ -140,7 +141,7 @@ public class LoadBalancerProvisioner {
         var deactivatedLoadBalancers = loadBalancers.stream()
                                                     .map(lb -> lb.with(LoadBalancer.State.inactive, now))
                                                     .collect(Collectors.toList());
-        db.writeLoadBalancers(deactivatedLoadBalancers, transaction);
+        db.writeLoadBalancers(deactivatedLoadBalancers, LoadBalancer.State.active, transaction);
     }
 
     /** Find all load balancer IDs owned by given tenant and application */
@@ -165,52 +166,41 @@ public class LoadBalancerProvisioner {
         return loadBalancerId;
     }
 
-    /** Idempotently provision a load balancer for given application and cluster */
-    private void provision(ApplicationTransaction transaction, LoadBalancerId id, NodeList nodes, boolean activate) {
+    private void prepare(LoadBalancerId id, NodeList nodes) {
         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
-
-        Set<Real> reals = realsOf(nodes);
-        Optional<LoadBalancerInstance> instance = provisionInstance(id, reals, loadBalancer);
+        Optional<LoadBalancerInstance> instance = provisionInstance(id, nodes, loadBalancer);
         LoadBalancer newLoadBalancer;
+        LoadBalancer.State fromState = null;
         if (loadBalancer.isEmpty()) {
             newLoadBalancer = new LoadBalancer(id, instance, LoadBalancer.State.reserved, now);
         } else {
-            LoadBalancer.State state = activate && instance.isPresent()
-                    ? LoadBalancer.State.active
-                    : loadBalancer.get().state();
-            newLoadBalancer = loadBalancer.get().with(instance).with(state, now);
-            if (loadBalancer.get().state() != newLoadBalancer.state()) {
-                log.log(Level.INFO, () -> "Moving " + newLoadBalancer.id() + " from " + loadBalancer.get().state() +
-                                          " to " + newLoadBalancer.state());
-            }
-        }
-
-        if (activate) {
-            db.writeLoadBalancers(List.of(newLoadBalancer), transaction.nested());
-        } else {
-            // Always store load balancer so that LoadBalancerExpirer can expire partially provisioned load balancers
-            db.writeLoadBalancer(newLoadBalancer);
-        }
-
-        // Signal that load balancer is not ready yet
-        if (instance.isEmpty()) {
-            throw new LoadBalancerServiceException("Could not (re)configure " + id + ", targeting: " +
-                                                   reals + ". The operation will be retried on next deployment",
-                                                   null);
+            newLoadBalancer = loadBalancer.get().with(instance);
+            fromState = newLoadBalancer.state();
         }
+        // Always store load balancer so that LoadBalancerExpirer can expire partially provisioned load balancers
+        db.writeLoadBalancer(newLoadBalancer, fromState);
+        requireInstance(id, instance);
     }
 
-    private void provision(ApplicationTransaction transaction, ClusterSpec.Id clusterId, NodeList nodes) {
-        provision(transaction, new LoadBalancerId(transaction.application(), clusterId), nodes, true);
+    private void activate(ApplicationTransaction transaction, ClusterSpec.Id cluster, NodeList nodes) {
+        Instant now = nodeRepository.clock().instant();
+        LoadBalancerId id = new LoadBalancerId(transaction.application(), cluster);
+        Optional<LoadBalancer> loadBalancer = db.readLoadBalancer(id);
+        if (loadBalancer.isEmpty()) throw new IllegalArgumentException("Could not active load balancer that was never prepared: " + id);
+
+        Optional<LoadBalancerInstance> instance = provisionInstance(id, nodes, loadBalancer);
+        LoadBalancer.State state = instance.isPresent() ? LoadBalancer.State.active : loadBalancer.get().state();
+        LoadBalancer newLoadBalancer = loadBalancer.get().with(instance).with(state, now);
+        db.writeLoadBalancers(List.of(newLoadBalancer), loadBalancer.get().state(), transaction.nested());
+        requireInstance(id, instance);
     }
 
     /** Provision or reconfigure a load balancer instance, if necessary */
-    private Optional<LoadBalancerInstance> provisionInstance(LoadBalancerId id, Set<Real> reals,
-                                                             Optional<LoadBalancer> currentLoadBalancer) {
+    private Optional<LoadBalancerInstance> provisionInstance(LoadBalancerId id, NodeList nodes, Optional<LoadBalancer> currentLoadBalancer) {
+        Set<Real> reals = realsOf(nodes);
         if (hasReals(currentLoadBalancer, reals)) return currentLoadBalancer.get().instance();
-        log.log(Level.INFO, () -> "Creating " + id + ", targeting: " + reals);
+        log.log(Level.INFO, () -> "Provisioning instance for " + id + ", targeting: " + reals);
         try {
             return Optional.of(service.create(new LoadBalancerSpec(id.application(), id.cluster(), reals),
                                               allowEmptyReals(currentLoadBalancer)));
@@ -241,7 +231,7 @@ public class LoadBalancerProvisioner {
 
     /** Returns real servers for given nodes */
     private Set<Real> realsOf(NodeList nodes) {
-        var reals = new LinkedHashSet<Real>();
+        Set<Real> reals = new LinkedHashSet<Real>();
         for (var node : nodes) {
             for (var ip : reachableIpAddresses(node)) {
                 reals.add(new Real(HostName.from(node.hostname()), ip));
@@ -289,6 +279,14 @@ public class LoadBalancerProvisioner {
         return reachable;
     }
 
+    private static void requireInstance(LoadBalancerId id, Optional<LoadBalancerInstance> instance) {
+        if (instance.isEmpty()) {
+            // Signal that load balancer is not ready yet
+            throw new LoadBalancerServiceException("Could not (re)configure " + id + ". The operation will be retried on next deployment",
+                                                   null);
+        }
+    }
+
     private static ClusterSpec.Id effectiveId(ClusterSpec cluster) {
         return cluster.combinedId().orElse(cluster.id());
     }
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 bdc3bdfd816..16fe5ef241a 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
@@ -300,6 +300,13 @@ public class LoadBalancerProvisionerTest {
         assertTrue("Load balancer has instance", loadBalancers.get(0).instance().isPresent());
     }
 
+    @Test
+    public void provisioning_load_balancer_for_unsupported_cluster_fails_gracefully() {
+        tester.loadBalancerService().supportsProvisioning(false);
+        tester.activate(app1, prepare(app1, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs"))));
+        assertTrue("No load balancer provisioned", tester.nodeRepository().loadBalancers().list(app1).asList().isEmpty());
+    }
+
     private void dirtyNodesOf(ApplicationId application) {
         tester.nodeRepository().nodes().deallocate(tester.nodeRepository().nodes().list().owner(application).asList(), Agent.system, this.getClass().getSimpleName());
     }