diff options
author | Jon Bratseth <bratseth@gmail.com> | 2022-10-26 09:16:01 +0200 |
---|---|---|
committer | Jon Bratseth <bratseth@gmail.com> | 2022-10-26 09:16:01 +0200 |
commit | 0c92c2b1c860ce2db13046bc0f478d25e848c155 (patch) | |
tree | ed7909c90f0c7378c02eade91eeaa5adfdad5afa /node-repository | |
parent | 27f3a824c6fce4a829a36e7834abeb087afeba7b (diff) |
Revert "Merge pull request #24573 from vespa-engine/mpolden/lb-account-change"
This reverts commit 29d15635c8270d4596a649fe37a6d8cce78214cc, reversing
changes made to e899053702569af8205d49c7fa5391bfbe6481b5.
Diffstat (limited to 'node-repository')
6 files changed, 55 insertions, 108 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 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..30c7e79ab02 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,21 @@ 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; + 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) { |