Merge pull request #24632 from vespa-engine/mpolden/account-change

Prevent reusing resources across accounts

2 files changed, 84 insertions, 24 deletions

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 de4df1992ee..d28d0321e6c 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<HostSpec> prepared = provisioningTester.prepare(applicationId, spec, capacity);
+        CloudAccount cloudAccount0 = new CloudAccount("000000000000");
+        Capacity capacity0 = Capacity.from(resources, resources, false, true, Optional.of(cloudAccount0));
+        List<HostSpec> prepared = provisioningTester.prepare(applicationId, spec, capacity0);
 
         // Hosts are provisioned in requested account
-        tester.maintainer.maintain();
-        List<ProvisionedHost> 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<String> 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());
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) {