Merge branch 'master' into freva/add-bandwidth-to-node-resources

# Conflicts:
#	node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java

6 files changed, 146 insertions, 68 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
index 4ec2bac0159..f21231236d4 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
@@ -64,7 +64,12 @@ public class NodeList implements Iterable<Node> {
 
     /** Returns the subset of nodes assigned to the given cluster type */
     public NodeList type(ClusterSpec.Type type) {
-        return filter(node -> node.allocation().get().membership().cluster().type().equals(type));
+        return filter(node -> node.allocation().isPresent() && node.allocation().get().membership().cluster().type().equals(type));
+    }
+
+    /** Returns the subset of nodes assigned to the given cluster */
+    public NodeList cluster(ClusterSpec.Id cluster) {
+        return filter(node -> node.allocation().isPresent() && node.allocation().get().membership().cluster().id().equals(cluster));
     }
 
     /** Returns the subset of nodes owned by the given application */
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 7d7f8d479fe..a8268fe7237 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
@@ -1,9 +1,7 @@
 // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision.maintenance;
 
-import com.yahoo.config.provision.ApplicationId;
 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;
@@ -49,7 +47,7 @@ public class LoadBalancerExpirer extends Maintainer {
     }
 
     private void expireReserved() {
-        try (Lock lock = db.lockLoadBalancers()) {
+        try (var lock = db.lockLoadBalancers()) {
             var now = nodeRepository().clock().instant();
             var expirationTime = now.minus(reservedExpiry);
             var expired = nodeRepository().loadBalancers()
@@ -62,14 +60,14 @@ public class LoadBalancerExpirer extends Maintainer {
     private void removeInactive() {
         List<LoadBalancerId> failed = new ArrayList<>();
         Exception lastException = null;
-        try (Lock lock = db.lockLoadBalancers()) {
+        try (var lock = db.lockLoadBalancers()) {
             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
+                if (hasNodes(lb.id())) { // Defer removal if there are still nodes allocated
                     continue;
                 }
                 try {
@@ -92,8 +90,8 @@ public class LoadBalancerExpirer extends Maintainer {
         }
     }
 
-    private boolean hasNodes(ApplicationId application) {
-        return !nodeRepository().getNodes(application).isEmpty();
+    private boolean hasNodes(LoadBalancerId loadBalancer) {
+        return nodeRepository().list().owner(loadBalancer.application()).cluster(loadBalancer.cluster()).size() > 0;
     }
 
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Activator.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Activator.java
index 1e83c2c9176..725a3426eae 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Activator.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Activator.java
@@ -42,7 +42,7 @@ class Activator {
     public void activate(ApplicationId application, Collection<HostSpec> hosts, NestedTransaction transaction) {
         try (Mutex lock = nodeRepository.lock(application)) {
             activateNodes(application, hosts, transaction, lock);
-            activateLoadBalancers(application, hosts, lock);
+            activateLoadBalancers(application, hosts, transaction, lock);
         }
     }
 
@@ -91,17 +91,17 @@ class Activator {
     }
 
     /** Activate load balancers */
-    private void activateLoadBalancers(ApplicationId application, Collection<HostSpec> hosts,
+    private void activateLoadBalancers(ApplicationId application, Collection<HostSpec> hosts, NestedTransaction transaction,
                                        @SuppressWarnings("unused") Mutex applicationLock) {
-        loadBalancerProvisioner.ifPresent(provisioner -> provisioner.activate(application, clustersOf(hosts)));
+        loadBalancerProvisioner.ifPresent(provisioner -> provisioner.activate(application, clustersOf(hosts), applicationLock, transaction));
     }
 
-    private static List<ClusterSpec> clustersOf(Collection<HostSpec> hosts) {
+    private static Set<ClusterSpec> clustersOf(Collection<HostSpec> hosts) {
         return hosts.stream()
                     .map(HostSpec::membership)
                     .flatMap(Optional::stream)
                     .map(ClusterMembership::cluster)
-                    .collect(Collectors.toUnmodifiableList());
+                    .collect(Collectors.toUnmodifiableSet());
     }
 
     private static void validateParentHosts(ApplicationId application, NodeList nodes, List<Node> potentialChildren) {
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 ea30fba9798..693fa254ac3 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
@@ -5,6 +5,7 @@ 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 com.yahoo.config.provision.exception.LoadBalancerServiceException;
 import com.yahoo.log.LogLevel;
 import com.yahoo.transaction.Mutex;
 import com.yahoo.transaction.NestedTransaction;
@@ -14,16 +15,18 @@ import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancer;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancerId;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancerInstance;
-import com.yahoo.config.provision.exception.LoadBalancerServiceException;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancerService;
 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.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;
 import java.util.stream.Collectors;
 
@@ -70,7 +73,9 @@ public class LoadBalancerProvisioner {
     public void prepare(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes) {
         if (requestedNodes.type() != NodeType.tenant) return; // Nothing to provision for this node type
         if (cluster.type() != ClusterSpec.Type.container) return; // Nothing to provision for this cluster type
-        provision(application, cluster.id(), false);
+        try (var loadBalancersLock = db.lockLoadBalancers()) {
+            provision(application, cluster.id(), false, loadBalancersLock);
+        }
     }
 
     /**
@@ -79,12 +84,21 @@ public class LoadBalancerProvisioner {
      * If a load balancer for the cluster already exists, it will be reconfigured based on the currently allocated
      * nodes and the load balancer itself will be moved to {@link LoadBalancer.State#active}.
      *
+     * Load balancers for clusters that are no longer in given clusters are deactivated.
+     *
      * Calling this when no load balancer has been prepared for given cluster is a no-op.
      */
-    public void activate(ApplicationId application, List<ClusterSpec> clusters) {
-        for (var clusterId : containerClusterIdsOf(clusters)) {
-            // Provision again to ensure that load balancer instance re-configured with correct nodes
-            provision(application, clusterId, true);
+    public void activate(ApplicationId application, Set<ClusterSpec> clusters,
+                         @SuppressWarnings("unused") Mutex applicationLock, NestedTransaction transaction) {
+        try (var loadBalancersLock = db.lockLoadBalancers()) {
+            var containerClusters = containerClusterOf(clusters);
+            for (var clusterId : containerClusters) {
+                // Provision again to ensure that load balancer instance is re-configured with correct nodes
+                provision(application, clusterId, true, loadBalancersLock);
+            }
+            // Deactivate any surplus load balancers, i.e. load balancers for clusters that have been removed
+            var surplusLoadBalancers = surplusLoadBalancersOf(application, containerClusters);
+            deactivate(surplusLoadBalancers, transaction);
         }
     }
 
@@ -93,38 +107,57 @@ public class LoadBalancerProvisioner {
      * load balancer(s).
      */
     public void deactivate(ApplicationId application, NestedTransaction transaction) {
-        try (Mutex applicationLock = nodeRepository.lock(application)) {
+        try (var applicationLock = nodeRepository.lock(application)) {
             try (Mutex loadBalancersLock = db.lockLoadBalancers()) {
-                var now = nodeRepository.clock().instant();
-                var deactivatedLoadBalancers = nodeRepository.loadBalancers().owner(application).asList().stream()
-                                                             .map(lb -> lb.with(LoadBalancer.State.inactive, now))
-                                                             .collect(Collectors.toList());
-                db.writeLoadBalancers(deactivatedLoadBalancers, transaction);
+                deactivate(nodeRepository.loadBalancers().owner(application).asList(), transaction);
             }
         }
     }
 
+    /** Returns load balancers of given application that are no longer referenced by wantedClusters */
+    private List<LoadBalancer> surplusLoadBalancersOf(ApplicationId application, Set<ClusterSpec.Id> activeClusters) {
+        var activeLoadBalancersByCluster = nodeRepository.loadBalancers()
+                                                         .owner(application)
+                                                         .in(LoadBalancer.State.active)
+                                                         .asList()
+                                                         .stream()
+                                                         .collect(Collectors.toMap(lb -> lb.id().cluster(),
+                                                                                   Function.identity()));
+        var surplus = new ArrayList<LoadBalancer>();
+        for (var kv : activeLoadBalancersByCluster.entrySet()) {
+            if (activeClusters.contains(kv.getKey())) continue;
+            surplus.add(kv.getValue());
+        }
+        return Collections.unmodifiableList(surplus);
+    }
+
+    private void deactivate(List<LoadBalancer> loadBalancers, NestedTransaction transaction) {
+        var now = nodeRepository.clock().instant();
+        var deactivatedLoadBalancers = loadBalancers.stream()
+                                                    .map(lb -> lb.with(LoadBalancer.State.inactive, now))
+                                                    .collect(Collectors.toList());
+        db.writeLoadBalancers(deactivatedLoadBalancers, transaction);
+    }
+
+
     /** Idempotently provision a load balancer for given application and cluster */
-    private void provision(ApplicationId application, ClusterSpec.Id clusterId, boolean activate) {
-        try (var applicationLock = nodeRepository.lock(application)) {
-            try (var loadBalancersLock = db.lockLoadBalancers()) {
-                var id = new LoadBalancerId(application, clusterId);
-                var now = nodeRepository.clock().instant();
-                var 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 = create(application, clusterId, allocatedContainers(application, clusterId), force);
-                LoadBalancer newLoadBalancer;
-                if (loadBalancer.isEmpty()) {
-                    newLoadBalancer = new LoadBalancer(id, instance, LoadBalancer.State.reserved, now);
-                } else {
-                    var newState = activate ? LoadBalancer.State.active : loadBalancer.get().state();
-                    newLoadBalancer = loadBalancer.get().with(instance).with(newState, now);
-                }
-                db.writeLoadBalancer(newLoadBalancer);
-            }
+    private void provision(ApplicationId application, ClusterSpec.Id clusterId, boolean activate,
+                           @SuppressWarnings("unused") Mutex loadBalancersLock) {
+        var id = new LoadBalancerId(application, clusterId);
+        var now = nodeRepository.clock().instant();
+        var 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 = create(application, clusterId, allocatedContainers(application, clusterId), force);
+        LoadBalancer newLoadBalancer;
+        if (loadBalancer.isEmpty()) {
+            newLoadBalancer = new LoadBalancer(id, instance, LoadBalancer.State.reserved, now);
+        } else {
+            var newState = activate ? LoadBalancer.State.active : loadBalancer.get().state();
+            newLoadBalancer = loadBalancer.get().with(instance).with(newState, now);
         }
+        db.writeLoadBalancer(newLoadBalancer);
     }
 
     private LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, List<Node> nodes, boolean force) {
@@ -171,11 +204,11 @@ public class LoadBalancerProvisioner {
         return reachable;
     }
 
-    private static List<ClusterSpec.Id> containerClusterIdsOf(List<ClusterSpec> clusters) {
+    private static Set<ClusterSpec.Id> containerClusterOf(Set<ClusterSpec> clusters) {
         return clusters.stream()
                        .filter(c -> c.type() == ClusterSpec.Type.container)
                        .map(ClusterSpec::id)
-                       .collect(Collectors.toUnmodifiableList());
+                       .collect(Collectors.toUnmodifiableSet());
     }
 
 }
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 b23af955f42..f8f60635ea9 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
@@ -14,9 +14,11 @@ import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester;
 import org.junit.Test;
 
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -32,21 +34,23 @@ public class LoadBalancerExpirerTest {
     private ProvisioningTester tester = new ProvisioningTester.Builder().build();
 
     @Test
-    public void test_remove_inactive() {
+    public void expire_inactive() {
         LoadBalancerExpirer expirer = new LoadBalancerExpirer(tester.nodeRepository(),
                                                               Duration.ofDays(1),
                                                               tester.loadBalancerService());
         Supplier<Map<LoadBalancerId, LoadBalancer>> loadBalancers = () -> tester.nodeRepository().database().readLoadBalancers();
 
-        // Deploy two applications with load balancers
-        ClusterSpec.Id cluster = ClusterSpec.Id.from("qrs");
+        // Deploy two applications with a total of three load balancers
+        ClusterSpec.Id cluster1 = ClusterSpec.Id.from("qrs");
+        ClusterSpec.Id cluster2 = ClusterSpec.Id.from("qrs2");
         ApplicationId app1 = tester.makeApplicationId();
         ApplicationId app2 = tester.makeApplicationId();
-        LoadBalancerId lb1 = new LoadBalancerId(app1, cluster);
-        LoadBalancerId lb2 = new LoadBalancerId(app2, cluster);
-        deployApplication(app1, cluster);
-        deployApplication(app2, cluster);
-        assertEquals(2, loadBalancers.get().size());
+        LoadBalancerId lb1 = new LoadBalancerId(app1, cluster1);
+        LoadBalancerId lb2 = new LoadBalancerId(app2, cluster1);
+        LoadBalancerId lb3 = new LoadBalancerId(app2, cluster2);
+        deployApplication(app1, cluster1);
+        deployApplication(app2, cluster1, cluster2);
+        assertEquals(3, loadBalancers.get().size());
 
         // Remove one application deactivates load balancers for that application
         removeApplication(app1);
@@ -55,8 +59,8 @@ public class LoadBalancerExpirerTest {
 
         // Expirer defers removal while nodes are still allocated to application
         expirer.maintain();
-        assertEquals(2, tester.loadBalancerService().instances().size());
-        dirtyNodesOf(app1);
+        assertEquals(3, tester.loadBalancerService().instances().size());
+        dirtyNodesOf(app1, cluster1);
 
         // Expirer defers removal until expiration time passes
         expirer.maintain();
@@ -70,10 +74,25 @@ public class LoadBalancerExpirerTest {
         // Active load balancer is left alone
         assertSame(LoadBalancer.State.active, loadBalancers.get().get(lb2).state());
         assertTrue("Active load balancer is not removed", tester.loadBalancerService().instances().containsKey(lb2));
+
+        // A single cluster is removed
+        deployApplication(app2, cluster1);
+        expirer.maintain();
+        assertEquals(LoadBalancer.State.inactive, loadBalancers.get().get(lb3).state());
+
+        // Expirer defers removal while nodes are still allocated to cluster
+        expirer.maintain();
+        assertEquals(2, tester.loadBalancerService().instances().size());
+        dirtyNodesOf(app2, cluster2);
+
+        // Expirer removes load balancer for removed cluster
+        tester.clock().advance(Duration.ofHours(1));
+        expirer.maintain();
+        assertFalse("Inactive load balancer removed", tester.loadBalancerService().instances().containsKey(lb3));
     }
 
     @Test
-    public void test_expire_reserved() {
+    public void expire_reserved() {
         LoadBalancerExpirer expirer = new LoadBalancerExpirer(tester.nodeRepository(),
                                                               Duration.ofDays(1),
                                                               tester.loadBalancerService());
@@ -84,7 +103,7 @@ public class LoadBalancerExpirerTest {
         ClusterSpec.Id cluster = ClusterSpec.Id.from("qrs");
         ApplicationId app = tester.makeApplicationId();
         LoadBalancerId lb = new LoadBalancerId(app, cluster);
-        deployApplication(app, cluster, false);
+        deployApplication(app, false, cluster);
 
         // Provisions load balancer in reserved
         assertSame(LoadBalancer.State.reserved, loadBalancers.get().get(lb).state());
@@ -94,7 +113,7 @@ public class LoadBalancerExpirerTest {
         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);
+        dirtyNodesOf(app, cluster);
         tester.clock().advance(Duration.ofHours(1));
         expirer.maintain();
         assertSame(LoadBalancer.State.inactive, loadBalancers.get().get(lb).state());
@@ -109,8 +128,12 @@ public class LoadBalancerExpirerTest {
         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());
+    private void dirtyNodesOf(ApplicationId application, ClusterSpec.Id cluster) {
+        tester.nodeRepository().setDirty(tester.nodeRepository().getNodes(application).stream()
+                                               .filter(node -> node.allocation().isPresent())
+                                               .filter(node -> node.allocation().get().membership().cluster().id().equals(cluster))
+                                               .collect(Collectors.toList()),
+                                         Agent.system, this.getClass().getSimpleName());
     }
 
     private void removeApplication(ApplicationId application) {
@@ -119,16 +142,19 @@ public class LoadBalancerExpirerTest {
         transaction.commit();
     }
 
-    private void deployApplication(ApplicationId application, ClusterSpec.Id cluster) {
-        deployApplication(application, cluster, true);
+    private void deployApplication(ApplicationId application, ClusterSpec.Id... clusters) {
+        deployApplication(application, true, clusters);
     }
 
-    private void deployApplication(ApplicationId application, ClusterSpec.Id cluster, boolean activate) {
+    private void deployApplication(ApplicationId application, boolean activate, ClusterSpec.Id... clusters) {
         tester.makeReadyNodes(10, "d-1-1-1");
-        List<HostSpec> hosts = tester.prepare(application, ClusterSpec.request(ClusterSpec.Type.container, cluster,
-                                                                               Vtag.currentVersion, false),
-                                              2, 1,
-                                              new NodeResources(1, 1, 1, 0.3));
+        List<HostSpec> hosts = new ArrayList<>();
+        for (var cluster : clusters) {
+            hosts.addAll(tester.prepare(application, ClusterSpec.request(ClusterSpec.Type.container, cluster,
+                                                                         Vtag.currentVersion, false),
+                                        2, 1,
+                                        new NodeResources(1, 1, 1, 0.3)));
+        }
         if (activate) {
             tester.activate(application, hosts);
         }
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 d058832ba17..f63fa797f2f 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
@@ -116,6 +116,22 @@ public class LoadBalancerProvisionerTest {
                                             .collect(Collectors.toList());
         assertEquals(activeContainers, reals);
 
+        // Cluster removal deactivates relevant load balancer
+        tester.activate(app1, prepare(app1, clusterRequest(ClusterSpec.Type.container, containerCluster1)));
+        assertEquals(2, lbApp1.get().size());
+        assertEquals("Deactivated load balancer for cluster " + containerCluster2, LoadBalancer.State.inactive,
+                     lbApp1.get().stream()
+                           .filter(lb -> lb.id().cluster().equals(containerCluster2))
+                           .map(LoadBalancer::state)
+                           .findFirst()
+                           .get());
+        assertEquals("Load balancer for cluster " + containerCluster1 + " remains active", LoadBalancer.State.active,
+                     lbApp1.get().stream()
+                           .filter(lb -> lb.id().cluster().equals(containerCluster1))
+                           .map(LoadBalancer::state)
+                           .findFirst()
+                           .get());
+
         // Application is removed, nodes and load balancer are deactivated
         NestedTransaction removeTransaction = new NestedTransaction();
         tester.provisioner().remove(removeTransaction, app1);