diff options
author | Martin Polden <mpolden@mpolden.no> | 2022-10-25 13:26:28 +0200 |
---|---|---|
committer | Martin Polden <mpolden@mpolden.no> | 2022-10-28 11:19:40 +0200 |
commit | d86c52405d4738f823b90b545c756ed53b6e4a73 (patch) | |
tree | 0391e0c9632bdec175162dde4a66379407980b5d /node-repository | |
parent | c7caa094988600fa95727195bd4a99ce8d64d12f (diff) |
Remove existing load balancer if account changes
Diffstat (limited to 'node-repository')
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) { |