diff options
author | Morten Tokle <mortent@verizonmedia.com> | 2021-02-09 09:01:47 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-02-09 09:01:47 +0100 |
commit | eb847b3bd3730a52284ea385701dd96899f61e44 (patch) | |
tree | 3db05e78df5d130ae6fe8511f8f833a2a07ae07e /node-repository/src | |
parent | ce0fa329bb1b96101b75ffe23ea2e034771c9a64 (diff) | |
parent | c1656df01c4026c7f2a4414ccdcd03cac96e9490 (diff) |
Merge pull request #16425 from vespa-engine/mpolden/less-lb-create
Avoid reconfiguring load balancer on unchanged reals
Diffstat (limited to 'node-repository/src')
3 files changed, 53 insertions, 20 deletions
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java index f4d689056c3..b912087da46 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java @@ -17,11 +17,17 @@ import java.util.Optional; public class LoadBalancerServiceMock implements LoadBalancerService { private final Map<LoadBalancerId, LoadBalancerInstance> instances = new HashMap<>(); + private boolean throwOnCreate = false; public Map<LoadBalancerId, LoadBalancerInstance> instances() { return Collections.unmodifiableMap(instances); } + public LoadBalancerServiceMock throwOnCreate(boolean throwOnCreate) { + this.throwOnCreate = throwOnCreate; + return this; + } + @Override public Protocol protocol() { return Protocol.ipv4; @@ -29,6 +35,7 @@ public class LoadBalancerServiceMock implements LoadBalancerService { @Override public LoadBalancerInstance create(LoadBalancerSpec spec, boolean force) { + if (throwOnCreate) throw new IllegalStateException("Did not expect a new load balancer to be created"); var id = new LoadBalancerId(spec.application(), spec.cluster()); var oldInstance = instances.get(id); if (!force && oldInstance != null && !oldInstance.reals().isEmpty() && spec.reals().isEmpty()) { 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 cfcaa1f83c0..356b27dfe2c 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 @@ -24,11 +24,13 @@ import com.yahoo.vespa.hosted.provision.lb.Real; import com.yahoo.vespa.hosted.provision.node.IP; import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient; +import java.time.Instant; import java.util.ArrayList; import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.logging.Level; @@ -166,12 +168,10 @@ public class LoadBalancerProvisioner { /** Idempotently provision a load balancer for given application and cluster */ private void provision(ApplicationTransaction transaction, LoadBalancerId id, List<Node> nodes, boolean activate) { - var now = nodeRepository.clock().instant(); - var loadBalancer = db.readLoadBalancer(id); + Instant now = nodeRepository.clock().instant(); + Optional<LoadBalancer> loadBalancer = db.readLoadBalancer(id); if (loadBalancer.isEmpty() && activate) return; // Nothing to activate as this load balancer was never prepared - - var force = loadBalancer.isPresent() && loadBalancer.get().state() != LoadBalancer.State.active; - var instance = provisionInstance(id, nodes, force); + LoadBalancerInstance instance = provisionInstance(id, realsOf(nodes), loadBalancer); LoadBalancer newLoadBalancer; if (loadBalancer.isEmpty()) { newLoadBalancer = new LoadBalancer(id, instance, LoadBalancer.State.reserved, now); @@ -189,16 +189,14 @@ public class LoadBalancerProvisioner { provision(transaction, new LoadBalancerId(transaction.application(), clusterId), nodes, true); } - private LoadBalancerInstance provisionInstance(LoadBalancerId id, List<Node> nodes, boolean force) { - var reals = new LinkedHashSet<Real>(); - for (var node : nodes) { - for (var ip : reachableIpAddresses(node)) { - reals.add(new Real(HostName.from(node.hostname()), ip)); - } - } + /** Provision or reconfigure a load balancer instance, if necessary */ + private LoadBalancerInstance provisionInstance(LoadBalancerId id, Set<Real> reals, + Optional<LoadBalancer> currentLoadBalancer) { + if (hasReals(currentLoadBalancer, reals)) return currentLoadBalancer.get().instance(); log.log(Level.FINE, "Creating " + id + ", targeting: " + reals); try { - return service.create(new LoadBalancerSpec(id.application(), id.cluster(), reals), force); + return service.create(new LoadBalancerSpec(id.application(), id.cluster(), reals), + allowEmptyReals(currentLoadBalancer)); } catch (Exception e) { throw new LoadBalancerServiceException("Failed to (re)configure " + id + ", targeting: " + reals + ". The operation will be retried on next deployment", e); @@ -224,6 +222,17 @@ public class LoadBalancerProvisioner { return nodes.stream().collect(Collectors.groupingBy(node -> effectiveId(node.allocation().get().membership().cluster()))); } + /** Returns real servers for given nodes */ + private Set<Real> realsOf(List<Node> nodes) { + var reals = new LinkedHashSet<Real>(); + for (var node : nodes) { + for (var ip : reachableIpAddresses(node)) { + reals.add(new Real(HostName.from(node.hostname()), ip)); + } + } + return reals; + } + /** Returns a list of the non-compactable IDs of given load balancer */ private static List<String> withoutCompactableIds(LoadBalancerId id) { List<String> ids = new ArrayList<>(2); @@ -236,6 +245,17 @@ public class LoadBalancerProvisioner { return ids; } + /** Returns whether load balancer has given reals */ + private static boolean hasReals(Optional<LoadBalancer> loadBalancer, Set<Real> reals) { + if (loadBalancer.isEmpty()) return false; + return loadBalancer.get().instance().reals().equals(reals); + } + + /** Returns whether to allow given load balancer to have no reals */ + private static boolean allowEmptyReals(Optional<LoadBalancer> loadBalancer) { + return loadBalancer.isPresent() && loadBalancer.get().state() != LoadBalancer.State.active; + } + /** Find IP addresses reachable by the load balancer service */ private Set<String> reachableIpAddresses(Node node) { Set<String> reachable = new LinkedHashSet<>(node.ipConfig().primary()); 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 ec08ed15ab4..14f068d89a7 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 @@ -92,7 +92,7 @@ public class LoadBalancerProvisionerTest { assertEquals(containers.get().get(1).hostname(), get(loadBalancer.instance().reals(), 1).hostname().value()); assertSame("State is unchanged", LoadBalancer.State.active, loadBalancer.state()); - // Add another container cluster + // Add another container cluster to first app ClusterSpec.Id containerCluster2 = ClusterSpec.Id.from("qrs2"); tester.activate(app1, prepare(app1, clusterRequest(ClusterSpec.Type.container, containerCluster1), @@ -133,7 +133,7 @@ public class LoadBalancerProvisionerTest { .findFirst() .get()); - // Application is removed, nodes and load balancer are deactivated + // Entire application is removed: Nodes and load balancer are deactivated tester.remove(app1); dirtyNodesOf(app1); assertTrue("No nodes are allocated to " + app1, tester.nodeRepository().getNodes(app1, Node.State.reserved, Node.State.active).isEmpty()); @@ -146,11 +146,17 @@ public class LoadBalancerProvisionerTest { clusterRequest(ClusterSpec.Type.container, containerCluster1), clusterRequest(ClusterSpec.Type.content, contentCluster))); assertSame("Re-activated load balancer for " + containerCluster1, LoadBalancer.State.active, - lbApp1.get().stream() - .filter(lb -> lb.id().cluster().equals(containerCluster1)) - .map(LoadBalancer::state) - .findFirst() - .orElseThrow()); + lbApp1.get().stream() + .filter(lb -> lb.id().cluster().equals(containerCluster1)) + .map(LoadBalancer::state) + .findFirst() + .orElseThrow()); + + // Next redeploy does not create a new load balancer instance + tester.loadBalancerService().throwOnCreate(true); + tester.activate(app1, prepare(app1, + clusterRequest(ClusterSpec.Type.container, containerCluster1), + clusterRequest(ClusterSpec.Type.content, contentCluster))); } @Test |