From 78ab44925f3bf5d4f8fd628e4c2766f54b326f88 Mon Sep 17 00:00:00 2001 From: jonmv Date: Wed, 8 Feb 2023 14:59:05 +0100 Subject: Provision LB resources on prepare, (re)configure on activate --- .../hosted/provision/lb/LoadBalancerInstance.java | 4 +- .../hosted/provision/lb/LoadBalancerService.java | 11 -- .../provision/lb/LoadBalancerServiceMock.java | 22 ---- .../provision/maintenance/LoadBalancerExpirer.java | 9 +- .../provisioning/LoadBalancerProvisioner.java | 116 +++++++++++---------- .../provisioning/LoadBalancerProvisionerTest.java | 23 ++-- 6 files changed, 82 insertions(+), 103 deletions(-) (limited to 'node-repository') 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 2856a38075b..ee8ecbada7f 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 @@ -97,8 +97,8 @@ public class LoadBalancerInstance { return new LoadBalancerInstance(hostname, ipAddress, dnsZone, ports, networks, reals, settings, serviceId, cloudAccount); } - public LoadBalancerInstance withServiceId(PrivateServiceId serviceId) { - return new LoadBalancerInstance(hostname, ipAddress, dnsZone, ports, networks, reals, settings, Optional.of(serviceId), cloudAccount); + public LoadBalancerInstance withSettings(ZoneEndpoint settings) { + return new LoadBalancerInstance(hostname, ipAddress, dnsZone, ports, networks, reals, settings, serviceId, cloudAccount); } private static Set requirePorts(Set ports) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java index 17b7b16141e..82f679bbe34 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java @@ -29,17 +29,6 @@ public interface LoadBalancerService { */ LoadBalancerInstance configure(LoadBalancerSpec spec, boolean force); - /** - * Create a load balancer from the given specification. Implementations are expected to be idempotent - * - * @param spec Load balancer specification - * @param force Whether reconfiguration should be forced (e.g. allow configuring an empty set of reals on a - * pre-existing load balancer). - * @return The provisioned load balancer instance - */ - // TODO jonmv: remove - default LoadBalancerInstance create(LoadBalancerSpec spec, boolean force) { provision(spec); return configure(spec, force); } - /** Permanently remove given load balancer */ void remove(LoadBalancer loadBalancer); 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 e3510baf026..2649ddc37aa 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 @@ -88,28 +88,6 @@ public class LoadBalancerServiceMock implements LoadBalancerService { return instance; } - @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()) { - throw new IllegalArgumentException("Refusing to remove all reals from load balancer " + id); - } - var instance = new LoadBalancerInstance( - Optional.of(DomainName.of("lb-" + spec.application().toShortString() + "-" + spec.cluster().value())), - Optional.empty(), - Optional.of(new DnsZone("zone-id-1")), - Collections.singleton(4443), - ImmutableSet.of("10.2.3.0/24", "10.4.5.0/24"), - spec.reals(), - spec.settings(), - spec.settings().isPrivateEndpoint() ? Optional.of(PrivateServiceId.of("service")) : Optional.empty(), - spec.cloudAccount()); - instances.put(id, instance); - return instance; - } - @Override public void remove(LoadBalancer loadBalancer) { instances.remove(loadBalancer.id()); 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 44c92434a0f..b1beb615bc3 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 @@ -9,6 +9,7 @@ import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.lb.LoadBalancer; import com.yahoo.vespa.hosted.provision.lb.LoadBalancer.State; import com.yahoo.vespa.hosted.provision.lb.LoadBalancerId; +import com.yahoo.vespa.hosted.provision.lb.LoadBalancerInstance; import com.yahoo.vespa.hosted.provision.lb.LoadBalancerService; import com.yahoo.vespa.hosted.provision.lb.LoadBalancerSpec; import com.yahoo.vespa.hosted.provision.persistence.CuratorDb; @@ -111,10 +112,10 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer { try { attempts.add(1); LOG.log(Level.INFO, () -> "Removing reals from inactive load balancer " + lb.id() + ": " + Sets.difference(lb.instance().get().reals(), reals)); - service.create(new LoadBalancerSpec(lb.id().application(), lb.id().cluster(), reals, - lb.instance().get().settings(), lb.instance().get().cloudAccount()), - true); - db.writeLoadBalancer(lb.with(lb.instance().map(instance -> instance.withReals(reals))), lb.state()); + LoadBalancerInstance instance = service.configure(new LoadBalancerSpec(lb.id().application(), lb.id().cluster(), reals, + lb.instance().get().settings(), lb.instance().get().cloudAccount()), + true); + db.writeLoadBalancer(lb.with(Optional.of(instance)), lb.state()); } catch (Exception e) { failed.add(lb.id()); lastException.set(e); 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 fc03ddcc3b0..4cdcda9fd6a 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 @@ -92,8 +92,7 @@ public class LoadBalancerProvisioner { try (var lock = db.lock(application)) { ClusterSpec.Id clusterId = effectiveId(cluster); LoadBalancerId loadBalancerId = requireNonClashing(new LoadBalancerId(application, clusterId)); - NodeList nodes = nodesOf(clusterId, application); - prepare(loadBalancerId, nodes, cluster.zoneEndpoint(), requestedNodes.cloudAccount()); + prepare(loadBalancerId, cluster.zoneEndpoint(), requestedNodes.cloudAccount()); } } @@ -189,27 +188,22 @@ public class LoadBalancerProvisioner { return loadBalancerId; } - private void prepare(LoadBalancerId id, NodeList nodes, ZoneEndpoint zoneEndpoint, CloudAccount cloudAccount) { + private void prepare(LoadBalancerId id, ZoneEndpoint zoneEndpoint, CloudAccount cloudAccount) { Instant now = nodeRepository.clock().instant(); Optional loadBalancer = db.readLoadBalancer(id); - Optional instance = provisionInstance(id, nodes, loadBalancer, zoneEndpoint, cloudAccount, true); LoadBalancer newLoadBalancer; - LoadBalancer.State fromState = null; - if (loadBalancer.isEmpty()) { - newLoadBalancer = new LoadBalancer(id, instance, LoadBalancer.State.reserved, now); - } else { - 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); + LoadBalancer.State fromState = loadBalancer.map(LoadBalancer::state).orElse(null); + if ( loadBalancer.isPresent() + && ( ! inAccount(cloudAccount, loadBalancer.get()) + || ! hasCorrectVisibility(loadBalancer.get(), zoneEndpoint))) { + // We have a load balancer, but with the wrong account or visibility. + // Load balancer must be removed before we can provision a new one with the wanted visibility + newLoadBalancer = loadBalancer.get().with(LoadBalancer.State.removable, now); } - if (!hasCorrectVisibility(newLoadBalancer, zoneEndpoint)) { - // We have a load balancer, but with the wrong visibility. Load balancer must be removed before we can - // provision a new one with the wanted visibility - newLoadBalancer = newLoadBalancer.with(LoadBalancer.State.removable, now); + else { + Optional instance = provisionInstance(id, loadBalancer, zoneEndpoint, cloudAccount); + newLoadBalancer = loadBalancer.isEmpty() ? new LoadBalancer(id, instance, LoadBalancer.State.reserved, now) + : loadBalancer.get().with(instance); } // Always store the load balancer. LoadBalancerExpirer will remove unwanted ones db.writeLoadBalancer(newLoadBalancer, fromState); @@ -228,42 +222,57 @@ public class LoadBalancerProvisioner { if (loadBalancer.isEmpty()) throw new IllegalArgumentException("Could not activate load balancer that was never prepared: " + id); if (loadBalancer.get().instance().isEmpty()) throw new IllegalArgumentException("Activating " + id + ", but prepare never provisioned a load balancer instance"); - Optional instance = provisionInstance(id, nodes, loadBalancer, settings, loadBalancer.get().instance().get().cloudAccount(), false); + Optional instance = configureInstance(id, nodes, loadBalancer.get(), settings, loadBalancer.get().instance().get().cloudAccount()); 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, newLoadBalancer, loadBalancer.get().instance().get().cloudAccount(), settings); } - /** Provision or reconfigure a load balancer instance, if necessary */ - private Optional provisionInstance(LoadBalancerId id, NodeList nodes, + /** Provision a load balancer instance, if necessary */ + private Optional provisionInstance(LoadBalancerId id, Optional currentLoadBalancer, ZoneEndpoint zoneEndpoint, - CloudAccount cloudAccount, - boolean preparing) { + CloudAccount cloudAccount) { + Set reals = currentLoadBalancer.flatMap(LoadBalancer::instance) + .map(LoadBalancerInstance::reals) + .orElse(Set.of()); // Targeted reals are changed on activation. + ZoneEndpoint settings = new ZoneEndpoint(zoneEndpoint.isPublicEndpoint(), + zoneEndpoint.isPrivateEndpoint(), + currentLoadBalancer.flatMap(LoadBalancer::instance) + .map(LoadBalancerInstance::settings) + .map(ZoneEndpoint::allowedUrns) + .orElse(List.of())); // Allowed URNs are changed on activation. + if ( currentLoadBalancer.isPresent() + && currentLoadBalancer.get().instance().isPresent() + && currentLoadBalancer.get().instance().get().settings().equals(settings)) + return currentLoadBalancer.get().instance(); + + log.log(Level.INFO, () -> "Provisioning instance for " + id); + try { + return Optional.of(service.provision(new LoadBalancerSpec(id.application(), id.cluster(), reals, settings, cloudAccount))); + } catch (Exception e) { + log.log(Level.WARNING, e, () -> "Could not provision " + id + ". The operation will be retried on next deployment"); + } + return Optional.empty(); // Will cause activation to fail, but lets us proceed with more preparations. + } + + /** Provision or reconfigure a load balancer instance, if necessary */ + private Optional configureInstance(LoadBalancerId id, NodeList nodes, + LoadBalancer currentLoadBalancer, + ZoneEndpoint zoneEndpoint, + CloudAccount cloudAccount) { boolean shouldDeactivateRouting = deactivateRouting.with(FetchVector.Dimension.APPLICATION_ID, id.application().serializedForm()) .value(); - Set reals; - if (shouldDeactivateRouting) { - reals = Set.of(); - } else { - reals = realsOf(nodes); - } - if (isUpToDate(currentLoadBalancer, reals, zoneEndpoint, preparing)) - return currentLoadBalancer.get().instance(); - log.log(Level.INFO, () -> "Provisioning instance for " + id + ", targeting: " + reals); + Set reals = shouldDeactivateRouting ? Set.of() : realsOf(nodes); + if (isUpToDate(currentLoadBalancer, reals, zoneEndpoint)) + return currentLoadBalancer.instance(); + + log.log(Level.INFO, () -> "Configuring instance for " + id + ", targeting: " + reals); try { - // Override settings at activation, otherwise keep existing ones. - ZoneEndpoint settings = ! preparing ? zoneEndpoint - : currentLoadBalancer.flatMap(LoadBalancer::instance) - .map(LoadBalancerInstance::settings) - .orElse(zoneEndpoint); - LoadBalancerInstance created = service.create(new LoadBalancerSpec(id.application(), id.cluster(), reals, settings, cloudAccount), - shouldDeactivateRouting || allowEmptyReals(currentLoadBalancer)); - if (created.serviceId().isEmpty() && currentLoadBalancer.flatMap(LoadBalancer::instance).flatMap(LoadBalancerInstance::serviceId).isPresent()) - created = created.withServiceId(currentLoadBalancer.flatMap(LoadBalancer::instance).flatMap(LoadBalancerInstance::serviceId).get()); - return Optional.of(created); + return Optional.of(service.configure(new LoadBalancerSpec(id.application(), id.cluster(), reals, zoneEndpoint, cloudAccount), + shouldDeactivateRouting || currentLoadBalancer.state() != LoadBalancer.State.active)); } catch (Exception e) { log.log(Level.WARNING, e, () -> "Could not (re)configure " + id + ", targeting: " + reals + ". The operation will be retried on next deployment"); @@ -319,16 +328,11 @@ public class LoadBalancerProvisioner { } /** Returns whether load balancer has given reals, and settings if specified */ - private static boolean isUpToDate(Optional loadBalancer, Set reals, ZoneEndpoint zoneEndpoint, boolean preparing) { - if (loadBalancer.isEmpty()) return false; - if (loadBalancer.get().instance().isEmpty()) return false; - return loadBalancer.get().instance().get().reals().equals(reals) - && (preparing || loadBalancer.get().instance().get().settings().equals(zoneEndpoint)); - } - - /** Returns whether to allow given load balancer to have no reals */ - private static boolean allowEmptyReals(Optional loadBalancer) { - return loadBalancer.isPresent() && loadBalancer.get().state() != LoadBalancer.State.active; + private static boolean isUpToDate(LoadBalancer loadBalancer, Set reals, ZoneEndpoint zoneEndpoint) { + if (loadBalancer.instance().isEmpty()) + throw new IllegalStateException("Expected a load balancer instance to be present, for " + loadBalancer.id()); + return loadBalancer.instance().get().reals().equals(reals) + && loadBalancer.instance().get().settings().equals(zoneEndpoint); } /** Find IP addresses reachable by the load balancer service */ @@ -345,12 +349,12 @@ public class LoadBalancerProvisioner { private static void requireInstance(LoadBalancerId id, LoadBalancer loadBalancer, CloudAccount cloudAccount, ZoneEndpoint zoneEndpoint) { if (loadBalancer.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"); + throw new LoadBalancerServiceException("Could not provision " + id + ". The operation will be retried on next deployment"); } - if (!inAccount(cloudAccount, loadBalancer)) { + if ( ! inAccount(cloudAccount, loadBalancer)) { throw new LoadBalancerServiceException("Could not (re)configure " + id + " due to change in cloud account. The operation will be retried on next deployment"); } - if (!hasCorrectVisibility(loadBalancer, zoneEndpoint)) { + if ( ! hasCorrectVisibility(loadBalancer, zoneEndpoint)) { throw new LoadBalancerServiceException("Could not (re)configure " + id + " due to change in load balancer visibility. 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 0aa2a001bfa..f2aceac922c 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 @@ -16,6 +16,7 @@ import com.yahoo.config.provision.ZoneEndpoint; import com.yahoo.config.provision.ZoneEndpoint.AccessType; import com.yahoo.config.provision.ZoneEndpoint.AllowedUrn; import com.yahoo.config.provision.exception.LoadBalancerServiceException; +import com.yahoo.docproc.jdisc.metric.NullMetric; import com.yahoo.transaction.NestedTransaction; import com.yahoo.vespa.flags.InMemoryFlagSource; import com.yahoo.vespa.flags.PermanentFlags; @@ -42,6 +43,7 @@ import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; @@ -72,8 +74,9 @@ public class LoadBalancerProvisionerTest { clusterRequest(ClusterSpec.Type.container, containerCluster), clusterRequest(ClusterSpec.Type.content, contentCluster)); assertEquals(1, lbApp1.get().size()); - assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbApp1.get().get(0).instance().get().reals().size()); + assertEquals("Prepare provisions load balancer without any nodes", 0, lbApp1.get().get(0).instance().get().reals().size()); tester.activate(app1, nodes); + assertEquals("Activate configures load balancer with reserved nodes", 2, lbApp1.get().get(0).instance().get().reals().size()); tester.activate(app2, prepare(app2, clusterRequest(ClusterSpec.Type.container, containerCluster))); assertEquals(1, lbApp2.get().size()); assertReals(app1, containerCluster, Node.State.active); @@ -177,10 +180,10 @@ public class LoadBalancerProvisionerTest { tester.activateTenantHosts(); var nodes = tester.prepare(app1, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs")), 2 , 1, resources); Supplier lb = () -> tester.nodeRepository().loadBalancers().list(app1).asList().get(0); - assertTrue("Load balancer provisioned with empty reals", tester.loadBalancerService().instances().get(lb.get().id()).reals().isEmpty()); + assertEquals("Load balancer provisioned with empty reals", Set.of(), tester.loadBalancerService().instances().get(lb.get().id()).reals()); assignIps(tester.nodeRepository().nodes().list().owner(app1)); tester.activate(app1, nodes); - assertFalse("Load balancer is reconfigured with reals", tester.loadBalancerService().instances().get(lb.get().id()).reals().isEmpty()); + assertNotEquals("Load balancer is reconfigured with reals", Set.of(), tester.loadBalancerService().instances().get(lb.get().id()).reals()); // Application is removed, nodes are deleted and load balancer is deactivated tester.remove(app1); @@ -190,12 +193,15 @@ public class LoadBalancerProvisionerTest { assertTrue("Nodes are deleted", tester.nodeRepository().nodes().list().nodeType(NodeType.tenant).isEmpty()); assertSame("Load balancer is deactivated", LoadBalancer.State.inactive, lb.get().state()); + // Load balancer reals are removed. + new LoadBalancerExpirer(tester.nodeRepository(), Duration.ofDays(1), tester.loadBalancerService(), new NullMetric()).run(); + // Application is redeployed - nodes = tester.prepare(app1, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs")), 2 , 1, resources); - assertTrue("Load balancer is reconfigured with empty reals", tester.loadBalancerService().instances().get(lb.get().id()).reals().isEmpty()); + nodes = tester.prepare(app1, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs")), 2, 1, resources); + assertEquals("Load balancer is reconfigured with empty reals", Set.of(), tester.loadBalancerService().instances().get(lb.get().id()).reals()); assignIps(tester.nodeRepository().nodes().list().owner(app1)); tester.activate(app1, nodes); - assertFalse("Load balancer is reconfigured with reals", tester.loadBalancerService().instances().get(lb.get().id()).reals().isEmpty()); + assertNotEquals("Load balancer is reconfigured with reals", Set.of(), tester.loadBalancerService().instances().get(lb.get().id()).reals()); } @Test @@ -221,8 +227,9 @@ public class LoadBalancerProvisionerTest { var combinedId = ClusterSpec.Id.from("container1"); var nodes = prepare(app1, clusterRequest(ClusterSpec.Type.combined, ClusterSpec.Id.from("content1"), Optional.of(combinedId), ZoneEndpoint.defaultEndpoint)); assertEquals(1, lbs.get().size()); - assertEquals("Prepare provisions load balancer with reserved nodes", 2, lbs.get().get(0).instance().get().reals().size()); + assertEquals("Prepare provisions load balancer wihtout reserved nodes", 0, lbs.get().get(0).instance().get().reals().size()); tester.activate(app1, nodes); + assertEquals("Activate configures load balancer with reserved nodes", 2, lbs.get().get(0).instance().get().reals().size()); assertSame(LoadBalancer.State.active, lbs.get().get(0).state()); assertEquals(combinedId, lbs.get().get(0).id().cluster()); } @@ -431,8 +438,8 @@ public class LoadBalancerProvisionerTest { ClusterSpec.Type clusterType = nodeType == NodeType.config ? ClusterSpec.Type.admin : ClusterSpec.Type.container; var nodes = prepare(application, Capacity.fromRequiredNodeType(nodeType), clusterRequest(clusterType, cluster)); assertEquals(1, lbs.get().size()); - assertEquals("Prepare provisions load balancer with reserved nodes", 3, lbs.get().get(0).instance().get().reals().size()); tester.activate(application, nodes); + assertEquals("Prepare provisions load balancer with reserved nodes", 3, lbs.get().get(0).instance().get().reals().size()); assertSame(LoadBalancer.State.active, lbs.get().get(0).state()); assertEquals(cluster, lbs.get().get(0).id().cluster()); } -- cgit v1.2.3