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

Revert mpolden/lb account change

7 files changed, 56 insertions, 112 deletions

diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/exception/LoadBalancerServiceException.java b/config-provisioning/src/main/java/com/yahoo/config/provision/exception/LoadBalancerServiceException.java
index 71da64c6500..c44424dd42a 100644
--- a/config-provisioning/src/main/java/com/yahoo/config/provision/exception/LoadBalancerServiceException.java
+++ b/config-provisioning/src/main/java/com/yahoo/config/provision/exception/LoadBalancerServiceException.java
@@ -10,10 +10,6 @@ import com.yahoo.config.provision.TransientException;
  */
 public class LoadBalancerServiceException extends TransientException {
 
-    public LoadBalancerServiceException(String message) {
-        super(message);
-    }
-
     public LoadBalancerServiceException(String message, Throwable cause) {
         super(message, cause);
     }
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 ad97ee887c9..4befa14942a 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 {
 
-        /** The load balancer has been provisioned and reserved for an application */
+        /** This load balancer has been provisioned and reserved for an application */
         reserved,
 
         /**
@@ -81,9 +81,6 @@ 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 06dc4263839..36ca58e6ccd 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;
 
-    public LoadBalancerExpirer(NodeRepository nodeRepository, Duration interval, LoadBalancerService service, Metric metric) {
+    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,24 +57,26 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer {
     @Override
     protected double maintain() {
         expireReserved();
-        return (deprovisionRemovable() + pruneReals()) / 2;
+        return ( removeInactive() + 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 -> canDeactivate(lb, expiry),
+        patchLoadBalancers(lb -> lb.state() == State.reserved && lb.changedAt().isBefore(expiry),
                            lb -> db.writeLoadBalancer(lb.with(State.inactive, now), lb.state()));
     }
 
-    /** Deprovision removable load balancers */
-    private double deprovisionRemovable() {
+    /** Deprovision inactive load balancers that have expired */
+    private double removeInactive() {
         MutableInteger attempts = new MutableInteger(0);
         var failed = new ArrayList<LoadBalancerId>();
         var lastException = new AtomicReference<Exception>();
         var expiry = nodeRepository().clock().instant().minus(inactiveExpiry);
-        patchLoadBalancers(lb -> canRemove(lb, expiry), lb -> {
+        patchLoadBalancers(lb -> lb.state() == State.inactive &&
+                                 lb.changedAt().isBefore(expiry) &&
+                                 allocatedNodes(lb.id()).isEmpty(), lb -> {
             try {
                 attempts.add(1);
                 log.log(Level.INFO, () -> "Removing expired inactive " + lb.id());
@@ -143,16 +145,6 @@ 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 e5f02b38d29..a5ebc6b3efc 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).toList();
+        return db.cluster().stream().map(HostName::value).collect(Collectors.toUnmodifiableList());
     }
 
     private void initZK() {
@@ -162,7 +162,7 @@ public class CuratorDatabaseClient {
     }
 
     /**
-     * Writes the given nodes to the given state (whether they are already in this state or another),
+     * Writes the given nodes to the given state (whether or not 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,18 +305,19 @@ public class CuratorDatabaseClient {
     }
 
     private String toDir(Node.State state) {
-        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";
-        };
+        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");
+        }
     }
 
     /** Acquires the single cluster-global, reentrant lock for all non-active nodes */
@@ -512,7 +513,7 @@ public class CuratorDatabaseClient {
         return db.getChildren(loadBalancersPath).stream()
                  .map(LoadBalancerId::fromSerializedForm)
                  .filter(predicate)
-                 .toList();
+                 .collect(Collectors.toUnmodifiableList());
     }
 
     /** Returns a given number of unique provision indices */
@@ -523,7 +524,7 @@ public class CuratorDatabaseClient {
         int firstIndex = (int) provisionIndexCounter.add(count) - count;
         return IntStream.range(0, count)
                         .mapToObj(i -> firstIndex + i)
-                        .toList();
+                        .collect(Collectors.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 bc719b6fafc..5e0f92bc08a 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,22 +117,22 @@ public class LoadBalancerSerializer {
     }
 
     private static String asString(LoadBalancer.State state) {
-        return switch (state) {
-            case active -> "active";
-            case inactive -> "inactive";
-            case reserved -> "reserved";
-            case removable -> "removable";
-        };
+        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 + "'");
+        }
     }
 
     private static LoadBalancer.State stateFromString(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 + "'");
-        };
+        switch (state) {
+            case "active": return LoadBalancer.State.active;
+            case "inactive": return LoadBalancer.State.inactive;
+            case "reserved": return LoadBalancer.State.reserved;
+            case "removable": return LoadBalancer.State.inactive; // Read future value
+            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 b2fc3127670..a62edec52e4 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,8 +48,6 @@ 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());
@@ -163,7 +161,7 @@ public class LoadBalancerProvisioner {
         return db.readLoadBalancerIds().stream()
                  .filter(id -> id.application().tenant().equals(tenant) &&
                                id.application().application().equals(application))
-                 .toList();
+                 .collect(Collectors.toUnmodifiableList());
     }
 
     /** Require that load balancer IDs do not clash. This prevents name clashing when compacting endpoint DNS names */
@@ -192,14 +190,9 @@ public class LoadBalancerProvisioner {
             newLoadBalancer = loadBalancer.get().with(instance);
             fromState = newLoadBalancer.state();
         }
-        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
+        // Always store load balancer so that LoadBalancerExpirer can expire partially provisioned load balancers
         db.writeLoadBalancer(newLoadBalancer, fromState);
-        requireInstance(id, instance, cloudAccount);
+        requireInstance(id, instance);
     }
 
     private void activate(ApplicationTransaction transaction, ClusterSpec.Id cluster, NodeList nodes) {
@@ -213,7 +206,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, loadBalancer.get().instance().get().cloudAccount());
+        requireInstance(id, instance);
     }
 
     /** Provision or reconfigure a load balancer instance, if necessary */
@@ -283,11 +276,6 @@ 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;
@@ -305,19 +293,21 @@ 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);
-            case ipv6 -> reachable.removeIf(IP::isV4);
+            case ipv4:
+                reachable.removeIf(IP::isV6);
+                break;
+            case ipv6:
+                reachable.removeIf(IP::isV4);
+                break;
         }
         return reachable;
     }
 
-    private static void requireInstance(LoadBalancerId id, Optional<LoadBalancerInstance> instance, Optional<CloudAccount> cloudAccount) {
+    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");
-        }
-        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");
+            throw new LoadBalancerServiceException("Could not (re)configure " + id + ". The operation will be retried on next deployment",
+                                                   null);
         }
     }
 
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 a88220a2728..15e07f92292 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,13 +20,10 @@ 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;
@@ -316,41 +313,12 @@ public class LoadBalancerProvisionerTest {
     @Test
     public void load_balancer_with_custom_cloud_account() {
         ClusterResources resources = new ClusterResources(3, 1, nodeResources);
-        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(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
+        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"))));
-        loadBalancers = tester.nodeRepository().loadBalancers().list();
+        LoadBalancerList loadBalancers = tester.nodeRepository().loadBalancers().list();
         assertEquals(1, loadBalancers.size());
-        assertEquals(cloudAccount1, loadBalancers.first().get().instance().get().cloudAccount().get());
+        assertEquals(cloudAccount, loadBalancers.first().get().instance().get().cloudAccount().get());
     }
 
     private void assertReals(ApplicationId application, ClusterSpec.Id cluster, Node.State... states) {