From d86c52405d4738f823b90b545c756ed53b6e4a73 Mon Sep 17 00:00:00 2001 From: Martin Polden Date: Tue, 25 Oct 2022 13:26:28 +0200 Subject: Remove existing load balancer if account changes --- .../provisioning/LoadBalancerProvisionerTest.java | 40 +++++++++++++++++++--- 1 file changed, 36 insertions(+), 4 deletions(-) (limited to 'node-repository/src/test/java/com') 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) { -- cgit v1.2.3 From 56907fe7dab5bab3ec09d96c5b23e4ba1c4ba0b6 Mon Sep 17 00:00:00 2001 From: Martin Polden Date: Fri, 28 Oct 2022 11:15:01 +0200 Subject: Never offer hosts in wrong account --- .../provision/provisioning/NodeAllocation.java | 1 + .../DynamicProvisioningMaintainerTest.java | 68 +++++++++++++++------- 2 files changed, 49 insertions(+), 20 deletions(-) (limited to 'node-repository/src/test/java/com') diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java index 56038ff1fe9..fe634a77997 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java @@ -122,6 +122,7 @@ class NodeAllocation { if ( candidate.state() == Node.State.active && candidate.wantToFail()) continue; // don't accept; causes failing if ( indexes.contains(membership.index())) continue; // duplicate index (just to be sure) if ( requiredHostFlavor.isPresent() && ! candidate.parent.map(node -> node.flavor().name()).equals(requiredHostFlavor)) continue; + if ( candidate.parent.isPresent() && ! candidate.parent.get().cloudAccount().equals(requestedNodes.cloudAccount())) continue; // wrong account boolean resizeable = requestedNodes.considerRetiring() && candidate.isResizable; boolean acceptToRetire = acceptToRetire(candidate); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java index bced4daed34..ecc2264cf94 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java @@ -44,6 +44,7 @@ import org.junit.Test; import java.time.Duration; import java.time.Instant; +import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Optional; @@ -437,16 +438,15 @@ public class DynamicProvisioningMaintainerTest { final ApplicationId hostApp; final ApplicationId configSrvApp; switch (hostType) { - case confighost: + case confighost -> { hostApp = new ConfigServerHostApplication().getApplicationId(); configSrvApp = new ConfigServerApplication().getApplicationId(); - break; - case controllerhost: + } + case controllerhost -> { hostApp = new ControllerHostApplication().getApplicationId(); configSrvApp = new ControllerApplication().getApplicationId(); - break; - default: - throw new IllegalArgumentException("Unexpected config server host like node type: " + hostType); + } + default -> throw new IllegalArgumentException("Unexpected config server host like node type: " + hostType); } Cloud cloud = Cloud.builder().dynamicProvisioning(true).build(); @@ -551,23 +551,32 @@ public class DynamicProvisioningMaintainerTest { // Deployment requests capacity in custom account ClusterSpec spec = ProvisioningTester.contentClusterSpec(); ClusterResources resources = new ClusterResources(2, 1, new NodeResources(16, 24, 100, 1)); - CloudAccount cloudAccount = new CloudAccount("012345678912"); - Capacity capacity = Capacity.from(resources, resources, false, true, Optional.of(cloudAccount)); - List prepared = provisioningTester.prepare(applicationId, spec, capacity); + CloudAccount cloudAccount0 = new CloudAccount("000000000000"); + Capacity capacity0 = Capacity.from(resources, resources, false, true, Optional.of(cloudAccount0)); + List prepared = provisioningTester.prepare(applicationId, spec, capacity0); // Hosts are provisioned in requested account - tester.maintainer.maintain(); - List newHosts = tester.hostProvisioner.provisionedHosts(); - assertEquals(2, newHosts.size()); - assertTrue(newHosts.stream().allMatch(host -> host.cloudAccount().equals(cloudAccount))); - for (var host : newHosts) { - provisioningTester.nodeRepository().nodes().setReady(host.hostHostname(), Agent.operator, getClass().getSimpleName()); - } - provisioningTester.prepareAndActivateInfraApplication(DynamicProvisioningTester.tenantHostApp, NodeType.host); - NodeList activeHosts = provisioningTester.nodeRepository().nodes().list(Node.State.active).nodeType(NodeType.host); - assertEquals(2, activeHosts.size()); - assertTrue(activeHosts.stream().allMatch(host -> host.cloudAccount().equals(cloudAccount))); + provisionHostsIn(cloudAccount0, 2, tester); + assertEquals(2, provisioningTester.activate(applicationId, prepared).size()); + NodeList allNodes0 = tester.nodeRepository.nodes().list(); + + // Redeployment in different account provisions a new set of hosts + CloudAccount cloudAccount1 = new CloudAccount("100000000000"); + Capacity capacity1 = Capacity.from(resources, resources, false, true, Optional.of(cloudAccount1)); + prepared = provisioningTester.prepare(applicationId, spec, capacity1); + provisionHostsIn(cloudAccount1, 2, tester); assertEquals(2, provisioningTester.activate(applicationId, prepared).size()); + + // No nodes or hosts are reused + NodeList allNodes1 = tester.nodeRepository.nodes().list(); + NodeList activeNodes0 = allNodes0.state(Node.State.active).owner(applicationId); + NodeList activeNodes1 = allNodes1.state(Node.State.active).owner(applicationId); + assertTrue("New set of nodes is activated", + Collections.disjoint(activeNodes0.asList(), + activeNodes1.asList())); + assertTrue("New set of parents are used", + Collections.disjoint(allNodes0.parentsOf(activeNodes0).asList(), + allNodes1.parentsOf(activeNodes1).asList())); } @Test @@ -627,6 +636,25 @@ public class DynamicProvisioningMaintainerTest { assertEquals(0, tester.nodeRepository.nodes().list().rebuilding(true).size()); } + private void provisionHostsIn(CloudAccount cloudAccount, int count, DynamicProvisioningTester tester) { + tester.maintainer.maintain(); + List provisionedHostnames = tester.hostProvisioner.provisionedHosts().stream() + .filter(host -> host.cloudAccount().equals(cloudAccount)) + .map(ProvisionedHost::hostHostname) + .toList(); + assertEquals(count, provisionedHostnames.size()); + for (var hostname : provisionedHostnames) { + tester.provisioningTester.nodeRepository().nodes().setReady(hostname, Agent.operator, getClass().getSimpleName()); + } + tester.provisioningTester.prepareAndActivateInfraApplication(DynamicProvisioningTester.tenantHostApp, NodeType.host); + NodeList activeHosts = tester.provisioningTester.nodeRepository().nodes() + .list(Node.State.active) + .nodeType(NodeType.host) + .matching(host -> provisionedHostnames.contains(host.hostname())); + assertTrue(activeHosts.stream().allMatch(host -> host.cloudAccount().equals(cloudAccount))); + assertEquals(count, activeHosts.size()); + } + private void assertCfghost3IsActive(DynamicProvisioningTester tester) { assertEquals(5, tester.nodeRepository.nodes().list(Node.State.active).size()); assertEquals(3, tester.nodeRepository.nodes().list(Node.State.active).nodeType(NodeType.confighost).size()); -- cgit v1.2.3