Force load balancer reconfig when in non-active state

6 files changed, 81 insertions, 13 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 6f45403f0e6..f6398c04e61 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
@@ -13,8 +13,17 @@ import java.util.Set;
  */
 public interface LoadBalancerService {
 
-    /** Create a load balancer for given application cluster. Implementations are expected to be idempotent */
-    LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals);
+    /**
+     * Create a load balancer for given application cluster. Implementations are expected to be idempotent
+     *
+     * @param application Application owning the LB
+     * @param cluster     Target cluster of the LB
+     * @param reals       Reals that should be configured on the LB
+     * @param force       Whether reconfiguration should be forced (e.g. allow configuring an empty set of reals on a
+     *                    pre-existing load balancer).
+     * @return The provisioned load balancer instance
+     */
+    LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals, boolean force);
 
     /** Permanently remove load balancer for given application cluster */
     void remove(ApplicationId application, ClusterSpec.Id cluster);
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 e89a4dc8bf8..91f02a31f6b 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
@@ -29,10 +29,10 @@ public class LoadBalancerServiceMock implements LoadBalancerService {
     }
 
     @Override
-    public LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals) {
+    public LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals, boolean force) {
         var id = new LoadBalancerId(application, cluster);
         var oldInstance = instances.get(id);
-        if (oldInstance != null && !oldInstance.reals().isEmpty() && reals.isEmpty()) {
+        if (!force && oldInstance != null && !oldInstance.reals().isEmpty() && reals.isEmpty()) {
             throw new IllegalArgumentException("Refusing to remove all reals from load balancer " + id);
         }
         var instance = new LoadBalancerInstance(
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 87b7c73386e..331ffe7e202 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
@@ -32,7 +32,7 @@ public class SharedLoadBalancerService implements LoadBalancerService {
     }
 
     @Override
-    public LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals) {
+    public LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals, boolean force) {
         final var proxyNodes = nodeRepository.getNodes(NodeType.proxy);
         proxyNodes.sort(hostnameComparator);
 
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 05b7042ccdc..0828f3369a2 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
@@ -106,7 +106,8 @@ public class LoadBalancerProvisioner {
                 var loadBalancer = db.readLoadBalancers().get(id);
                 if (loadBalancer == null && activate) return; // Nothing to activate as this load balancer was never prepared
 
-                var instance = create(application, clusterId, allocatedContainers(application, clusterId));
+                var force = loadBalancer != null && loadBalancer.state() != LoadBalancer.State.active;
+                var instance = create(application, clusterId, allocatedContainers(application, clusterId), force);
                 if (loadBalancer == null) {
                     loadBalancer = new LoadBalancer(id, instance, LoadBalancer.State.reserved, now);
                 } else {
@@ -118,7 +119,7 @@ public class LoadBalancerProvisioner {
         }
     }
 
-    private LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, List<Node> nodes) {
+    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));
@@ -129,7 +130,7 @@ public class LoadBalancerProvisioner {
         log.log(LogLevel.INFO, "Creating load balancer for " + cluster + " in " + application.toShortString() +
                                ", targeting: " + reals);
         try {
-            return service.create(application, cluster, reals);
+            return service.create(application, cluster, reals, force);
         } catch (Exception e) {
             throw new LoadBalancerServiceException("Failed to (re)configure load balancer for " + cluster + " in " +
                                                    application + ", targeting: " + reals + ". The operation will be " +
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java
index 40c307c6bef..5344fbc3c5f 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java
@@ -29,7 +29,7 @@ public class SharedLoadBalancerServiceTest {
     @Test
     public void test_create_lb() {
         tester.makeReadyNodes(2, "default", NodeType.proxy);
-        final var lb = loadBalancerService.create(applicationId, clusterId, reals);
+        final var lb = loadBalancerService.create(applicationId, clusterId, reals, false);
 
         assertEquals(HostName.from("host-1.yahoo.com"), lb.hostname());
         assertEquals(Optional.empty(), lb.dnsZone());
@@ -39,7 +39,7 @@ public class SharedLoadBalancerServiceTest {
 
     @Test(expected = IllegalStateException.class)
     public void test_exception_on_missing_proxies() {
-        loadBalancerService.create(applicationId, clusterId, reals);
+        loadBalancerService.create(applicationId, clusterId, reals, false);
     }
 
     @Test
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 3cee00f6a84..77273f98f76 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
@@ -16,17 +16,21 @@ import com.yahoo.vespa.hosted.provision.lb.LoadBalancer;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancerInstance;
 import com.yahoo.vespa.hosted.provision.lb.Real;
 import com.yahoo.vespa.hosted.provision.node.Agent;
+import com.yahoo.vespa.hosted.provision.node.IP;
 import org.junit.Test;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 
@@ -135,8 +139,38 @@ public class LoadBalancerProvisionerTest {
     }
 
     @Test
+    public void provision_load_balancers_with_dynamic_node_provisioning() {
+        var nodes = prepare(app1, Capacity.fromCount(2, new NodeResources(1, 1, 1), false, true),
+                                           true,
+                                           clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs")));
+        Supplier<LoadBalancer> lb = () -> tester.nodeRepository().loadBalancers().owner(app1).asList().get(0);
+        assertTrue("Load balancer provisioned with empty reals", tester.loadBalancerService().instances().get(lb.get().id()).reals().isEmpty());
+        assignIps(tester.nodeRepository().getNodes(app1));
+        tester.activate(app1, nodes);
+        assertFalse("Load balancer is reconfigured with reals", tester.loadBalancerService().instances().get(lb.get().id()).reals().isEmpty());
+
+        // Application is removed, nodes are deleted and load balancer is deactivated
+        NestedTransaction removeTransaction = new NestedTransaction();
+        tester.provisioner().remove(removeTransaction, app1);
+        removeTransaction.commit();
+        tester.nodeRepository().database().removeNodes(tester.nodeRepository().getNodes());
+        assertTrue("Nodes are deleted", tester.nodeRepository().getNodes().isEmpty());
+        assertSame("Load balancer is deactivated", LoadBalancer.State.inactive, lb.get().state());
+
+        // Application is redeployed
+        nodes = prepare(app1, Capacity.fromCount(2, new NodeResources(1, 1, 1), false, true),
+                        true,
+                        clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs")));
+        assertTrue("Load balancer is reconfigured with empty reals", tester.loadBalancerService().instances().get(lb.get().id()).reals().isEmpty());
+        assignIps(tester.nodeRepository().getNodes(app1));
+        tester.activate(app1, nodes);
+        assertFalse("Load balancer is reconfigured with reals", tester.loadBalancerService().instances().get(lb.get().id()).reals().isEmpty());
+    }
+
+    @Test
     public void does_not_provision_load_balancers_for_non_tenant_node_type() {
         tester.activate(infraApp1, prepare(infraApp1, Capacity.fromRequiredNodeType(NodeType.host),
+                                           false,
                                            clusterRequest(ClusterSpec.Type.container,
                                                           ClusterSpec.Id.from("tenant-host"))));
         assertTrue("No load balancer provisioned", tester.loadBalancerService().instances().isEmpty());
@@ -156,11 +190,15 @@ public class LoadBalancerProvisionerTest {
     }
 
     private Set<HostSpec> prepare(ApplicationId application, ClusterSpec... specs) {
-        return prepare(application, Capacity.fromCount(2, new NodeResources(1, 1, 1), false, true), specs);
+        return prepare(application, Capacity.fromCount(2, new NodeResources(1, 1, 1), false, true), false, specs);
     }
 
-    private Set<HostSpec> prepare(ApplicationId application, Capacity capacity, ClusterSpec... specs) {
-        tester.makeReadyNodes(specs.length * 2, "d-1-1-1", capacity.type());
+    private Set<HostSpec> prepare(ApplicationId application, Capacity capacity, boolean dynamicDockerNodes, ClusterSpec... specs) {
+        if (dynamicDockerNodes) {
+            makeDynamicDockerNodes(specs.length * 2, capacity.type());
+        } else {
+            tester.makeReadyNodes(specs.length * 2, "d-1-1-1", capacity.type());
+        }
         Set<HostSpec> allNodes = new LinkedHashSet<>();
         for (ClusterSpec spec : specs) {
             allNodes.addAll(tester.prepare(application, spec, capacity, 1, false));
@@ -168,6 +206,26 @@ public class LoadBalancerProvisionerTest {
         return allNodes;
     }
 
+    private void makeDynamicDockerNodes(int n, NodeType nodeType) {
+        List<Node> nodes = new ArrayList<>(n);
+        for (int i = 1; i <= n; i++) {
+            var node = Node.createDockerNode(Set.of(), Set.of(), "node" + i, Optional.empty(),
+                                             NodeResources.fromLegacyName("d-1-1-1"), nodeType);
+            nodes.add(node);
+        }
+        nodes = tester.nodeRepository().database().addNodesInState(nodes, Node.State.reserved);
+        nodes = tester.nodeRepository().setDirty(nodes, Agent.system, getClass().getSimpleName());
+        tester.nodeRepository().setReady(nodes, Agent.system, getClass().getSimpleName());
+    }
+
+    private void assignIps(List<Node> nodes) {
+        try (var lock = tester.nodeRepository().lockAllocation()) {
+            for (int i = 0; i < nodes.size(); i++) {
+                tester.nodeRepository().write(nodes.get(i).with(IP.Config.EMPTY.with(Set.of("127.0.0." + i))), lock);
+            }
+        }
+    }
+
     private static ClusterSpec clusterRequest(ClusterSpec.Type type, ClusterSpec.Id id) {
         return ClusterSpec.request(type, id, Version.fromString("6.42"), false);
     }