Remove existing load balancer if account changes

5 files changed, 78 insertions, 20 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java
index 4befa14942a..ad97ee887c9 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java
@@ -68,7 +68,7 @@ public class LoadBalancer {
 
     public enum State {
 
-        /** This load balancer has been provisioned and reserved for an application */
+        /** The load balancer has been provisioned and reserved for an application */
         reserved,
 
         /**
@@ -81,6 +81,9 @@ public class LoadBalancer {
         /** The load balancer is in active use by an application */
         active,
 
+        /** The load balancer should be removed immediately by {@link LoadBalancerExpirer} */
+        removable,
+
     }
 
 }
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 36ca58e6ccd..06dc4263839 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
@@ -48,7 +48,7 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer {
     private final LoadBalancerService service;
     private final CuratorDatabaseClient db;
 
-    LoadBalancerExpirer(NodeRepository nodeRepository, Duration interval, LoadBalancerService service, Metric metric) {
+    public LoadBalancerExpirer(NodeRepository nodeRepository, Duration interval, LoadBalancerService service, Metric metric) {
         super(nodeRepository, interval, metric);
         this.service = Objects.requireNonNull(service, "service must be non-null");
         this.db = nodeRepository.database();
@@ -57,26 +57,24 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer {
     @Override
     protected double maintain() {
         expireReserved();
-        return ( removeInactive() + pruneReals() ) / 2;
+        return (deprovisionRemovable() + pruneReals()) / 2;
     }
 
     /** Move reserved load balancer that have expired to inactive */
     private void expireReserved() {
         Instant now = nodeRepository().clock().instant();
         Instant expiry = now.minus(reservedExpiry);
-        patchLoadBalancers(lb -> lb.state() == State.reserved && lb.changedAt().isBefore(expiry),
+        patchLoadBalancers(lb -> canDeactivate(lb, expiry),
                            lb -> db.writeLoadBalancer(lb.with(State.inactive, now), lb.state()));
     }
 
-    /** Deprovision inactive load balancers that have expired */
-    private double removeInactive() {
+    /** Deprovision removable load balancers */
+    private double deprovisionRemovable() {
         MutableInteger attempts = new MutableInteger(0);
         var failed = new ArrayList<LoadBalancerId>();
         var lastException = new AtomicReference<Exception>();
         var expiry = nodeRepository().clock().instant().minus(inactiveExpiry);
-        patchLoadBalancers(lb -> lb.state() == State.inactive &&
-                                 lb.changedAt().isBefore(expiry) &&
-                                 allocatedNodes(lb.id()).isEmpty(), lb -> {
+        patchLoadBalancers(lb -> canRemove(lb, expiry), lb -> {
             try {
                 attempts.add(1);
                 log.log(Level.INFO, () -> "Removing expired inactive " + lb.id());
@@ -145,6 +143,16 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer {
         }
     }
 
+    private boolean canRemove(LoadBalancer lb, Instant expiry) {
+        return lb.state() == State.removable || (lb.state() == State.inactive &&
+                                                 lb.changedAt().isBefore(expiry) &&
+                                                 allocatedNodes(lb.id()).isEmpty());
+    }
+
+    private boolean canDeactivate(LoadBalancer lb, Instant expiry) {
+        return lb.state() == State.reserved && lb.changedAt().isBefore(expiry);
+    }
+
     private List<Node> allocatedNodes(LoadBalancerId loadBalancer) {
         return nodeRepository().nodes().list().owner(loadBalancer.application()).cluster(loadBalancer.cluster()).asList();
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java
index 92b492df86f..4078a3e80d7 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java
@@ -124,6 +124,7 @@ public class LoadBalancerSerializer {
             case active -> "active";
             case inactive -> "inactive";
             case reserved -> "reserved";
+            case removable -> "removable";
         };
     }
 
@@ -132,7 +133,7 @@ public class LoadBalancerSerializer {
             case "active" -> LoadBalancer.State.active;
             case "inactive" -> LoadBalancer.State.inactive;
             case "reserved" -> LoadBalancer.State.reserved;
-            case "removable" -> LoadBalancer.State.inactive; // Read future value
+            case "removable" -> LoadBalancer.State.removable;
             default -> throw new IllegalArgumentException("No serialization defined for state string '" + state + "'");
         };
     }
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 1eb8df0ca2b..0763d19bae3 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
@@ -48,6 +48,8 @@ import java.util.stream.Collectors;
 // 1) (new) -> reserved -> active
 // 2) active | reserved -> inactive
 // 3) inactive -> active | (removed)
+// 4) active | reserved | inactive -> removable
+// 5) removable -> (removed)
 public class LoadBalancerProvisioner {
 
     private static final Logger log = Logger.getLogger(LoadBalancerProvisioner.class.getName());
@@ -190,9 +192,14 @@ public class LoadBalancerProvisioner {
             newLoadBalancer = loadBalancer.get().with(instance);
             fromState = newLoadBalancer.state();
         }
-        // Always store load balancer so that LoadBalancerExpirer can expire partially provisioned load balancers
+        if (!inAccount(cloudAccount, newLoadBalancer)) {
+            // We have a load balancer, but in the wrong account. Load balancer must be removed before we can
+            // provision a new one in the wanted account
+            newLoadBalancer = newLoadBalancer.with(LoadBalancer.State.removable, now);
+        }
+        // Always store the load balancer. LoadBalancerExpirer will remove unwanted ones
         db.writeLoadBalancer(newLoadBalancer, fromState);
-        requireInstance(id, instance);
+        requireInstance(id, instance, cloudAccount);
     }
 
     private void activate(ApplicationTransaction transaction, ClusterSpec.Id cluster, NodeList nodes) {
@@ -206,7 +213,7 @@ public class LoadBalancerProvisioner {
         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);
+        requireInstance(id, instance, loadBalancer.get().instance().get().cloudAccount());
     }
 
     /** Provision or reconfigure a load balancer instance, if necessary */
@@ -276,6 +283,11 @@ public class LoadBalancerProvisioner {
         return ids;
     }
 
+    /** Returns whether load balancer is provisioned in given account */
+    private static boolean inAccount(CloudAccount cloudAccount, LoadBalancer loadBalancer) {
+        return loadBalancer.instance().isEmpty() || loadBalancer.instance().get().cloudAccount().equals(cloudAccount);
+    }
+
     /** Returns whether load balancer has given reals */
     private static boolean hasReals(Optional<LoadBalancer> loadBalancer, Set<Real> reals) {
         if (loadBalancer.isEmpty()) return false;
@@ -299,11 +311,13 @@ public class LoadBalancerProvisioner {
         return reachable;
     }
 
-    private static void requireInstance(LoadBalancerId id, Optional<LoadBalancerInstance> instance) {
+    private static void requireInstance(LoadBalancerId id, Optional<LoadBalancerInstance> instance, CloudAccount cloudAccount) {
         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);
+            throw new LoadBalancerServiceException("Could not (re)configure " + id + ". The operation will be retried on next deployment");
+        }
+        if (!instance.get().cloudAccount().equals(cloudAccount)) {
+            throw new LoadBalancerServiceException("Could not (re)configure " + id + " due to change in cloud account. The operation will be retried on next deployment");
         }
     }
 
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 7706895ebca..d3a8ca2dcc1 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
@@ -20,10 +20,13 @@ import com.yahoo.vespa.hosted.provision.NodeList;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancer;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancerList;
 import com.yahoo.vespa.hosted.provision.lb.Real;
+import com.yahoo.vespa.hosted.provision.maintenance.LoadBalancerExpirer;
+import com.yahoo.vespa.hosted.provision.maintenance.TestMetric;
 import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.hosted.provision.node.IP;
 import org.junit.Test;
 
+import java.time.Duration;
 import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -313,12 +316,41 @@ public class LoadBalancerProvisionerTest {
     @Test
     public void load_balancer_with_custom_cloud_account() {
         ClusterResources resources = new ClusterResources(3, 1, nodeResources);
-        CloudAccount cloudAccount = new CloudAccount("012345678912");
-        Capacity capacity = Capacity.from(resources, resources, false, true, Optional.of(cloudAccount));
-        tester.activate(app1, prepare(app1, capacity, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("c1"))));
+        CloudAccount cloudAccount0 = CloudAccount.empty;
+        {
+            Capacity capacity = Capacity.from(resources, resources, false, true, Optional.of(cloudAccount0));
+            tester.activate(app1, prepare(app1, capacity, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("c1"))));
+        }
         LoadBalancerList loadBalancers = tester.nodeRepository().loadBalancers().list();
         assertEquals(1, loadBalancers.size());
-        assertEquals(cloudAccount, loadBalancers.first().get().instance().get().cloudAccount());
+        assertEquals(cloudAccount0, loadBalancers.first().get().instance().get().cloudAccount());
+
+        // Changing account fails if there is an existing LB in the previous account.
+        CloudAccount cloudAccount1 = new CloudAccount("111111111111");
+        Capacity capacity = Capacity.from(resources, resources, false, true, Optional.of(cloudAccount1));
+        try {
+            prepare(app1, capacity, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("c1")));
+            fail("Expected exception");
+        } catch (LoadBalancerServiceException e) {
+            assertTrue(e.getMessage().contains("due to change in cloud account"));
+        }
+
+        // Existing LB is removed
+        loadBalancers = tester.nodeRepository().loadBalancers().list();
+        assertEquals(1, loadBalancers.size());
+        assertSame(LoadBalancer.State.removable, loadBalancers.first().get().state());
+        LoadBalancerExpirer expirer = new LoadBalancerExpirer(tester.nodeRepository(),
+                                                              Duration.ofDays(1),
+                                                              tester.loadBalancerService(),
+                                                              new TestMetric());
+        expirer.run();
+        assertEquals(0, tester.nodeRepository().loadBalancers().list().in(LoadBalancer.State.removable).size());
+
+        // Next deployment provisions a new LB
+        tester.activate(app1, prepare(app1, capacity, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("c1"))));
+        loadBalancers = tester.nodeRepository().loadBalancers().list();
+        assertEquals(1, loadBalancers.size());
+        assertEquals(cloudAccount1, loadBalancers.first().get().instance().get().cloudAccount());
     }
 
     private void assertReals(ApplicationId application, ClusterSpec.Id cluster, Node.State... states) {