From 0dae17d6c5efa8165ef8af7a8298d8f3281149b6 Mon Sep 17 00:00:00 2001 From: Martin Polden Date: Wed, 18 Dec 2019 15:27:32 +0100 Subject: Simplify --- .../provisioning/LoadBalancerProvisioner.java | 22 ++++++++-------------- .../provisioning/NodeRepositoryProvisioner.java | 2 +- 2 files changed, 9 insertions(+), 15 deletions(-) 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 a7b37628289..ee786ea414e 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 @@ -9,10 +9,6 @@ import com.yahoo.config.provision.exception.LoadBalancerServiceException; import com.yahoo.log.LogLevel; import com.yahoo.transaction.Mutex; import com.yahoo.transaction.NestedTransaction; -import com.yahoo.vespa.flags.BooleanFlag; -import com.yahoo.vespa.flags.FetchVector; -import com.yahoo.vespa.flags.FlagSource; -import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeList; import com.yahoo.vespa.hosted.provision.NodeRepository; @@ -28,7 +24,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; -import java.util.Map; import java.util.Set; import java.util.function.Function; import java.util.logging.Logger; @@ -51,7 +46,7 @@ public class LoadBalancerProvisioner { private final CuratorDatabaseClient db; private final LoadBalancerService service; - public LoadBalancerProvisioner(NodeRepository nodeRepository, LoadBalancerService service, FlagSource flagSource) { + public LoadBalancerProvisioner(NodeRepository nodeRepository, LoadBalancerService service) { this.nodeRepository = nodeRepository; this.db = nodeRepository.database(); this.service = service; @@ -118,7 +113,7 @@ public class LoadBalancerProvisioner { } } - /** Returns load balancers of given application that are no longer referenced by wantedClusters */ + /** Returns load balancers of given application that are no longer referenced by given clusters */ private List surplusLoadBalancersOf(ApplicationId application, Set activeClusters) { var activeLoadBalancersByCluster = nodeRepository.loadBalancers() .owner(application) @@ -165,13 +160,12 @@ public class LoadBalancerProvisioner { } private LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, List nodes, boolean force) { - Map> hostnameToIpAdresses = nodes.stream() - .collect(Collectors.toMap(node -> HostName.from(node.hostname()), - this::reachableIpAddresses)); - Set reals = new LinkedHashSet<>(); - hostnameToIpAdresses.forEach((hostname, ipAddresses) -> { - ipAddresses.forEach(ipAddress -> reals.add(new Real(hostname, ipAddress))); - }); + var reals = new LinkedHashSet(); + for (var node : nodes) { + for (var ip : reachableIpAddresses(node)) { + reals.add(new Real(HostName.from(node.hostname()), ip)); + } + } log.log(LogLevel.INFO, "Creating load balancer for " + cluster + " in " + application.toShortString() + ", targeting: " + reals); try { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java index 67d132b4fb8..f7567683776 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java @@ -59,7 +59,7 @@ public class NodeRepositoryProvisioner implements Provisioner { this.nodeRepository = nodeRepository; this.capacityPolicies = new CapacityPolicies(zone); this.zone = zone; - this.loadBalancerProvisioner = provisionServiceProvider.getLoadBalancerService().map(lbService -> new LoadBalancerProvisioner(nodeRepository, lbService, flagSource)); + this.loadBalancerProvisioner = provisionServiceProvider.getLoadBalancerService().map(lbService -> new LoadBalancerProvisioner(nodeRepository, lbService)); this.preparer = new Preparer(nodeRepository, zone.environment() == Environment.prod ? SPARE_CAPACITY_PROD : SPARE_CAPACITY_NONPROD, provisionServiceProvider.getHostProvisioner(), -- cgit v1.2.3 From 6838ebd5ad1bcc98669f6ecf112408bf441655ca Mon Sep 17 00:00:00 2001 From: Martin Polden Date: Wed, 18 Dec 2019 15:41:37 +0100 Subject: Reduce LoadBalancerExpirer interval --- .../vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java index 90daa0ad279..10aff833584 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java @@ -164,7 +164,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent { metricsInterval = Duration.ofMinutes(1); infrastructureProvisionInterval = Duration.ofMinutes(1); throttlePolicy = NodeFailer.ThrottlePolicy.hosted; - loadBalancerExpirerInterval = Duration.ofMinutes(10); + loadBalancerExpirerInterval = Duration.ofMinutes(5); reservationExpiry = Duration.ofMinutes(20); // Need to be long enough for deployment to be finished for all config model versions dynamicProvisionerInterval = Duration.ofMinutes(5); osUpgradeActivatorInterval = zone.system().isCd() ? Duration.ofSeconds(30) : Duration.ofMinutes(5); -- cgit v1.2.3 From c5e77a5d27cea6f96166c9beca6149c15f947bb9 Mon Sep 17 00:00:00 2001 From: Martin Polden Date: Wed, 18 Dec 2019 16:03:13 +0100 Subject: Prune reals no longer allocated to application --- .../vespa/hosted/provision/NodeRepository.java | 2 +- .../hosted/provision/lb/LoadBalancerInstance.java | 5 +++ .../hosted/provision/lb/LoadBalancerList.java | 6 ++- .../provision/maintenance/LoadBalancerExpirer.java | 46 +++++++++++++++++++--- .../maintenance/LoadBalancerExpirerTest.java | 9 ++++- 5 files changed, 59 insertions(+), 9 deletions(-) diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java index f86c05da3c2..682d1419a5c 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java @@ -193,7 +193,7 @@ public class NodeRepository extends AbstractComponent { /** Returns a filterable list of all load balancers in this repository */ public LoadBalancerList loadBalancers() { - return new LoadBalancerList(database().readLoadBalancers().values()); + return LoadBalancerList.copyOf(database().readLoadBalancers().values()); } public List getNodes(ApplicationId id, Node.State ... inState) { return db.getNodes(id, inState); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java index c1b4fe9b0f2..1e60b689489 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java @@ -56,6 +56,11 @@ public class LoadBalancerInstance { return reals; } + /** Returns a copy of this with reals set to given reals */ + public LoadBalancerInstance withReals(Set reals) { + return new LoadBalancerInstance(hostname, dnsZone, ports, networks, reals); + } + private static Set requirePorts(Set ports) { Objects.requireNonNull(ports, "ports must be non-null"); if (ports.isEmpty()) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java index 7fd50bf0930..014d3df8d9a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java @@ -20,7 +20,7 @@ public class LoadBalancerList implements Iterable { private final List loadBalancers; - public LoadBalancerList(Collection loadBalancers) { + private LoadBalancerList(Collection loadBalancers) { this.loadBalancers = List.copyOf(Objects.requireNonNull(loadBalancers, "loadBalancers must be non-null")); } @@ -47,6 +47,10 @@ public class LoadBalancerList implements Iterable { return new LoadBalancerList(stream.collect(Collectors.toUnmodifiableList())); } + public static LoadBalancerList copyOf(Collection loadBalancers) { + return new LoadBalancerList(loadBalancers); + } + @Override public Iterator iterator() { return loadBalancers.iterator(); 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 a8268fe7237..bcb0c901f14 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 @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.provision.maintenance; import com.yahoo.log.LogLevel; +import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.lb.LoadBalancer; import com.yahoo.vespa.hosted.provision.lb.LoadBalancer.State; @@ -11,6 +12,7 @@ import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient; import java.time.Duration; import java.util.ArrayList; +import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; import java.util.stream.Collectors; @@ -44,8 +46,10 @@ public class LoadBalancerExpirer extends Maintainer { protected void maintain() { expireReserved(); removeInactive(); + pruneReals(); } + /** Move reserved load balancer that have expired to inactive */ private void expireReserved() { try (var lock = db.lockLoadBalancers()) { var now = nodeRepository().clock().instant(); @@ -57,8 +61,9 @@ public class LoadBalancerExpirer extends Maintainer { } } + /** Deprovision inactive load balancers that have expired */ private void removeInactive() { - List failed = new ArrayList<>(); + var failed = new ArrayList(); Exception lastException = null; try (var lock = db.lockLoadBalancers()) { var now = nodeRepository().clock().instant(); @@ -67,9 +72,7 @@ public class LoadBalancerExpirer extends Maintainer { .in(State.inactive) .changedBefore(expirationTime); for (var lb : expired) { - if (hasNodes(lb.id())) { // Defer removal if there are still nodes allocated - continue; - } + if (!allocatedNodes(lb.id()).isEmpty()) continue; // Defer removal if there are still nodes allocated to application try { service.remove(lb.id().application(), lb.id().cluster()); db.removeLoadBalancer(lb.id()); @@ -90,8 +93,39 @@ public class LoadBalancerExpirer extends Maintainer { } } - private boolean hasNodes(LoadBalancerId loadBalancer) { - return nodeRepository().list().owner(loadBalancer.application()).cluster(loadBalancer.cluster()).size() > 0; + /** Remove reals from inactive load balancers */ + private void pruneReals() { + var failed = new ArrayList(); + Exception lastException = null; + try (var lock = db.lockLoadBalancers()) { + var deactivated = nodeRepository().loadBalancers().in(State.inactive); + for (var lb : deactivated) { + var allocatedNodes = allocatedNodes(lb.id()).stream().map(Node::hostname).collect(Collectors.toSet()); + var reals = new LinkedHashSet<>(lb.instance().reals()); + // Remove any real no longer allocated to this application + reals.removeIf(real -> !allocatedNodes.contains(real.hostname().value())); + try { + service.create(lb.id().application(), lb.id().cluster(), reals, true); + db.writeLoadBalancer(lb.with(lb.instance().withReals(reals))); + } catch (Exception e) { + failed.add(lb.id()); + lastException = e; + } + } + } + if (!failed.isEmpty()) { + log.log(LogLevel.WARNING, String.format("Failed to remove reals from %d load balancers: %s, retrying in %s", + failed.size(), + failed.stream() + .map(LoadBalancerId::serializedForm) + .collect(Collectors.joining(", ")), + interval()), + lastException); + } + } + + private List allocatedNodes(LoadBalancerId loadBalancer) { + return nodeRepository().list().owner(loadBalancer.application()).cluster(loadBalancer.cluster()).asList(); } } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java index 75224021464..12b48fd7a35 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java @@ -17,6 +17,7 @@ import java.time.Duration; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -60,9 +61,15 @@ public class LoadBalancerExpirerTest { // Expirer defers removal while nodes are still allocated to application expirer.maintain(); assertEquals(3, tester.loadBalancerService().instances().size()); + assertEquals(2, tester.loadBalancerService().instances().get(lb1).reals().size()); dirtyNodesOf(app1, cluster1); - // Expirer defers removal until expiration time passes + // Expirer prunes reals before expiration time of load balancer itself + expirer.maintain(); + assertEquals(Set.of(), tester.loadBalancerService().instances().get(lb1).reals()); + assertEquals(Set.of(), tester.nodeRepository().loadBalancers().owner(lb1.application()).asList().get(0).instance().reals()); + + // Expirer defers removal of load balancer until expiration time passes expirer.maintain(); assertTrue("Inactive load balancer not removed", tester.loadBalancerService().instances().containsKey(lb1)); -- cgit v1.2.3