Merge pull request #24573 from vespa-engine/mpolden/lb-account-change

Remove existing load balancer if account changes

6 files changed, 108 insertions, 55 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/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
index a5ebc6b3efc..e5f02b38d29 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
@@ -87,7 +87,7 @@ public class CuratorDatabaseClient {
     }
 
     public List<String> cluster() {
-        return db.cluster().stream().map(HostName::value).collect(Collectors.toUnmodifiableList());
+        return db.cluster().stream().map(HostName::value).toList();
     }
 
     private void initZK() {
@@ -162,7 +162,7 @@ public class CuratorDatabaseClient {
     }
 
     /**
-     * Writes the given nodes to the given state (whether or not they are already in this state or another),
+     * Writes the given nodes to the given state (whether they are already in this state or another),
      * and returns a copy of the incoming nodes in their persisted state.
      *
      * @param  toState the state to write the nodes to
@@ -305,19 +305,18 @@ public class CuratorDatabaseClient {
     }
 
     private String toDir(Node.State state) {
-        switch (state) {
-            case active: return "allocated"; // legacy name
-            case dirty: return "dirty";
-            case failed: return "failed";
-            case inactive: return "deallocated"; // legacy name
-            case parked : return "parked";
-            case provisioned: return "provisioned";
-            case ready: return "ready";
-            case reserved: return "reserved";
-            case deprovisioned: return "deprovisioned";
-            case breakfixed: return "breakfixed";
-            default: throw new RuntimeException("Node state " + state + " does not map to a directory name");
-        }
+        return switch (state) {
+            case active -> "allocated"; // legacy name
+            case dirty -> "dirty";
+            case failed -> "failed";
+            case inactive -> "deallocated"; // legacy name
+            case parked -> "parked";
+            case provisioned -> "provisioned";
+            case ready -> "ready";
+            case reserved -> "reserved";
+            case deprovisioned -> "deprovisioned";
+            case breakfixed -> "breakfixed";
+        };
     }
 
     /** Acquires the single cluster-global, reentrant lock for all non-active nodes */
@@ -513,7 +512,7 @@ public class CuratorDatabaseClient {
         return db.getChildren(loadBalancersPath).stream()
                  .map(LoadBalancerId::fromSerializedForm)
                  .filter(predicate)
-                 .collect(Collectors.toUnmodifiableList());
+                 .toList();
     }
 
     /** Returns a given number of unique provision indices */
@@ -524,7 +523,7 @@ public class CuratorDatabaseClient {
         int firstIndex = (int) provisionIndexCounter.add(count) - count;
         return IntStream.range(0, count)
                         .mapToObj(i -> firstIndex + i)
-                        .collect(Collectors.toList());
+                        .toList();
     }
 
     public CacheStats cacheStats() {
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 30c7e79ab02..bc719b6fafc 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
@@ -117,21 +117,22 @@ public class LoadBalancerSerializer {
     }
 
     private static String asString(LoadBalancer.State state) {
-        switch (state) {
-            case active: return "active";
-            case inactive: return "inactive";
-            case reserved: return "reserved";
-            default: throw new IllegalArgumentException("No serialization defined for state enum '" + state + "'");
-        }
+        return switch (state) {
+            case active -> "active";
+            case inactive -> "inactive";
+            case reserved -> "reserved";
+            case removable -> "removable";
+        };
     }
 
     private static LoadBalancer.State stateFromString(String state) {
-        switch (state) {
-            case "active": return LoadBalancer.State.active;
-            case "inactive": return LoadBalancer.State.inactive;
-            case "reserved": return LoadBalancer.State.reserved;
-            default: throw new IllegalArgumentException("No serialization defined for state string '" + state + "'");
-        }
+        return switch (state) {
+            case "active" -> LoadBalancer.State.active;
+            case "inactive" -> LoadBalancer.State.inactive;
+            case "reserved" -> LoadBalancer.State.reserved;
+            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 a62edec52e4..b2fc3127670 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());
@@ -161,7 +163,7 @@ public class LoadBalancerProvisioner {
         return db.readLoadBalancerIds().stream()
                  .filter(id -> id.application().tenant().equals(tenant) &&
                                id.application().application().equals(application))
-                 .collect(Collectors.toUnmodifiableList());
+                 .toList();
     }
 
     /** Require that load balancer IDs do not clash. This prevents name clashing when compacting endpoint DNS names */
@@ -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(Optional<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;
@@ -293,21 +305,19 @@ public class LoadBalancerProvisioner {
         Set<String> reachable = new LinkedHashSet<>(node.ipConfig().primary());
         // Remove addresses unreachable by the load balancer service
         switch (service.protocol()) {
-            case ipv4:
-                reachable.removeIf(IP::isV6);
-                break;
-            case ipv6:
-                reachable.removeIf(IP::isV4);
-                break;
+            case ipv4 -> reachable.removeIf(IP::isV6);
+            case ipv6 -> reachable.removeIf(IP::isV4);
         }
         return reachable;
     }
 
-    private static void requireInstance(LoadBalancerId id, Optional<LoadBalancerInstance> instance) {
+    private static void requireInstance(LoadBalancerId id, Optional<LoadBalancerInstance> instance, Optional<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 15e07f92292..a88220a2728 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 = new CloudAccount("000000000000");
+        {
+            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().get());
+        assertEquals(cloudAccount0, loadBalancers.first().get().instance().get().cloudAccount().get());
+
+        // 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().get());
     }
 
     private void assertReals(ApplicationId application, ClusterSpec.Id cluster, Node.State... states) {