Always activate through MockDeployer

3 files changed, 51 insertions, 76 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java
index 299dc66c547..e628e823025 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java
@@ -15,7 +15,6 @@ import com.yahoo.vespa.hosted.provision.provisioning.NodeRepositoryProvisioner;
 import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -41,7 +40,7 @@ public class MockDeployer implements Deployer {
     @Inject
     @SuppressWarnings("unused")
     public MockDeployer() {
-        this(null, Clock.systemUTC(), Collections.emptyMap());
+        this(null, Clock.systemUTC(), Map.of());
     }
 
     /**
@@ -53,7 +52,7 @@ public class MockDeployer implements Deployer {
                         Map<ApplicationId, ApplicationContext> applications) {
         this.provisioner = provisioner;
         this.clock = clock;
-        this.applications = applications;
+        this.applications = new HashMap<>(applications);
     }
 
     public ReentrantLock lock() {
@@ -73,8 +72,8 @@ public class MockDeployer implements Deployer {
             throw new RuntimeException(e);
         }
         try {
-            lastDeployTimes.put(id, clock.instant());
-            return Optional.of(new MockDeployment(provisioner, applications.get(id)));
+            return Optional.ofNullable(applications.get(id))
+                    .map(application -> new MockDeployment(provisioner, application));
         } finally {
             lock.unlock();
         }
@@ -90,6 +89,13 @@ public class MockDeployer implements Deployer {
         return Optional.ofNullable(lastDeployTimes.get(application));
     }
 
+    public void removeApplication(ApplicationId applicationId) {
+        new MockDeployment(provisioner, new ApplicationContext(applicationId, List.of())).activate();
+
+        applications.remove(applicationId);
+        lastDeployTimes.remove(applicationId);
+    }
+
     public class MockDeployment implements Deployment {
 
         private final NodeRepositoryProvisioner provisioner;
@@ -116,6 +122,7 @@ public class MockDeployer implements Deployer {
             try (NestedTransaction t = new NestedTransaction()) {
                 provisioner.activate(t, application.id(), preparedHosts);
                 t.commit();
+                lastDeployTimes.put(application.id, clock.instant());
             }
         }
 
@@ -136,7 +143,7 @@ public class MockDeployer implements Deployer {
         }
 
         public ApplicationContext(ApplicationId id, ClusterSpec cluster, Capacity capacity, int groups) {
-            this(id, Collections.singletonList(new ClusterContext(id, cluster, capacity, groups)));
+            this(id, List.of(new ClusterContext(id, cluster, capacity, groups)));
         }
 
         public ApplicationId id() { return id; }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java
index 5979ed37a98..e1ac0430ee4 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java
@@ -9,7 +9,6 @@ import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.DockerImage;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.Flavor;
-import com.yahoo.config.provision.HostSpec;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.NodeFlavors;
 import com.yahoo.config.provision.NodeResources;
@@ -18,10 +17,8 @@ import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.test.ManualClock;
-import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.mock.MockCurator;
-import com.yahoo.vespa.curator.transaction.CuratorTransaction;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
@@ -36,7 +33,6 @@ import org.junit.Test;
 
 import java.time.Duration;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -62,33 +58,34 @@ public class OperatorChangeApplicationMaintainerTest {
                                                  new MockNameResolver().mockAnyLookup(),
                                                  DockerImage.fromString("docker-registry.domain.tld:8080/dist/vespa"),
                                                  true);
-        this.fixture = new Fixture(zone, nodeRepository, nodeFlavors, curator);
+        this.fixture = new Fixture(zone, nodeRepository, nodeFlavors);
 
         createReadyNodes(15, this.fixture.nodeResources, nodeRepository);
         createHostNodes(2, nodeRepository, nodeFlavors);
 
         // Create applications
         fixture.activate();
-        OperatorChangeApplicationMaintainer maintainer = new OperatorChangeApplicationMaintainer(fixture.deployer, nodeRepository, clock, Duration.ofMinutes(1));
+        assertEquals("Initial applications are deployed", 2, fixture.deployer.redeployments);
+        OperatorChangeApplicationMaintainer maintainer = new OperatorChangeApplicationMaintainer(fixture.deployer, nodeRepository, Duration.ofMinutes(1));
         
         clock.advance(Duration.ofMinutes(2));
         maintainer.maintain();
-        assertEquals("No changes -> no redeployments", 0, fixture.deployer.redeployments);
+        assertEquals("No changes -> no redeployments", 2, fixture.deployer.redeployments);
 
         nodeRepository.fail(nodeRepository.getNodes(fixture.app1).get(3).hostname(), Agent.system, "Failing to unit test");
         clock.advance(Duration.ofMinutes(2));
         maintainer.maintain();
-        assertEquals("System change -> no redeployments", 0, fixture.deployer.redeployments);
+        assertEquals("System change -> no redeployments", 2, fixture.deployer.redeployments);
 
         clock.advance(Duration.ofSeconds(1));
         nodeRepository.fail(nodeRepository.getNodes(fixture.app2).get(4).hostname(), Agent.operator, "Manual node failing");
         clock.advance(Duration.ofMinutes(2));
         maintainer.maintain();
-        assertEquals("Operator change -> redeployment", 1, fixture.deployer.redeployments);
+        assertEquals("Operator change -> redeployment", 3, fixture.deployer.redeployments);
 
         clock.advance(Duration.ofMinutes(2));
         maintainer.maintain();
-        assertEquals("No further operator changes -> no (new) redeployments", 1, fixture.deployer.redeployments);
+        assertEquals("No further operator changes -> no (new) redeployments", 3, fixture.deployer.redeployments);
     }
 
     private void createReadyNodes(int count, NodeRepository nodeRepository, NodeFlavors nodeFlavors) {
@@ -118,50 +115,33 @@ public class OperatorChangeApplicationMaintainerTest {
 
     private class Fixture {
 
-        final NodeResources nodeResources = new NodeResources(2, 8, 50);
         final NodeRepository nodeRepository;
-        final NodeRepositoryProvisioner provisioner;
-        final Curator curator;
+        final MockDeployer deployer;
 
+        final NodeResources nodeResources = new NodeResources(2, 8, 50);
         final ApplicationId app1 = ApplicationId.from(TenantName.from("foo1"), ApplicationName.from("bar"), InstanceName.from("fuz"));
         final ApplicationId app2 = ApplicationId.from(TenantName.from("foo2"), ApplicationName.from("bar"), InstanceName.from("fuz"));
         final ClusterSpec clusterApp1 = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("test"), Version.fromString("6.42"), false);
         final ClusterSpec clusterApp2 = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("test"), Version.fromString("6.42"), false);
         final int wantedNodesApp1 = 5;
         final int wantedNodesApp2 = 7;
-        MockDeployer deployer; // created on activation
 
-        Fixture(Zone zone, NodeRepository nodeRepository, NodeFlavors flavors, Curator curator) {
+        Fixture(Zone zone, NodeRepository nodeRepository, NodeFlavors flavors) {
             this.nodeRepository = nodeRepository;
-            this.curator = curator;
-            this.provisioner = new NodeRepositoryProvisioner(nodeRepository, flavors, zone, new MockProvisionServiceProvider(), new InMemoryFlagSource());
+            NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(
+                    nodeRepository, flavors, zone, new MockProvisionServiceProvider(), new InMemoryFlagSource());
+
+            Map<ApplicationId, MockDeployer.ApplicationContext> apps = Map.of(
+                    app1, new MockDeployer.ApplicationContext(app1, clusterApp1, Capacity.fromCount(wantedNodesApp1, nodeResources), 1),
+                    app2, new MockDeployer.ApplicationContext(app2, clusterApp2, Capacity.fromCount(wantedNodesApp2, nodeResources), 1));
+            this.deployer = new MockDeployer(provisioner, nodeRepository.clock(), apps);
         }
 
         void activate() {
-            activate(app1, clusterApp1, wantedNodesApp1, provisioner);
-            activate(app2, clusterApp2, wantedNodesApp2, provisioner);
+            deployer.deployFromLocalActive(app1, false).get().activate();
+            deployer.deployFromLocalActive(app2, false).get().activate();
             assertEquals(wantedNodesApp1, nodeRepository.getNodes(app1, Node.State.active).size());
             assertEquals(wantedNodesApp2, nodeRepository.getNodes(app2, Node.State.active).size());
-
-            Map<ApplicationId, MockDeployer.ApplicationContext> apps = new HashMap<>();
-            apps.put(app1, new MockDeployer.ApplicationContext(app1, clusterApp1,
-                                                               Capacity.fromCount(wantedNodesApp1, nodeResources), 1));
-            apps.put(app2, new MockDeployer.ApplicationContext(app2, clusterApp2,
-                                                               Capacity.fromCount(wantedNodesApp2, nodeResources), 1));
-            this.deployer = new MockDeployer(provisioner, nodeRepository.clock(), apps);
-        }
-
-        private void activate(ApplicationId applicationId, ClusterSpec cluster, int nodeCount, NodeRepositoryProvisioner provisioner) {
-            List<HostSpec> hosts = provisioner.prepare(applicationId, cluster, Capacity.fromCount(nodeCount, nodeResources), 1, null);
-            NestedTransaction transaction = new NestedTransaction().add(new CuratorTransaction(curator));
-            provisioner.activate(transaction, applicationId, hosts);
-            transaction.commit();
-        }
-
-        void remove(ApplicationId application) {
-            NestedTransaction transaction = new NestedTransaction().add(new CuratorTransaction(curator));
-            provisioner.remove(transaction, application);
-            transaction.commit();
         }
 
         NodeList getNodes(Node.State ... states) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
index dff18912288..0923d8df71a 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
@@ -10,7 +10,6 @@ import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.DockerImage;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.Flavor;
-import com.yahoo.config.provision.HostSpec;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.NodeFlavors;
 import com.yahoo.config.provision.NodeResources;
@@ -19,10 +18,8 @@ import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.test.ManualClock;
-import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.mock.MockCurator;
-import com.yahoo.vespa.curator.transaction.CuratorTransaction;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
@@ -46,7 +43,6 @@ import java.util.Map;
 import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
 
 /**
  * @author bratseth
@@ -68,7 +64,7 @@ public class PeriodicApplicationMaintainerTest {
                                                  new MockNameResolver().mockAnyLookup(),
                                                  DockerImage.fromString("docker-registry.domain.tld:8080/dist/vespa"),
                                                  true);
-        this.fixture = new Fixture(zone, nodeRepository, nodeFlavors, curator);
+        this.fixture = new Fixture(zone, nodeRepository, nodeFlavors);
 
         createReadyNodes(15, fixture.nodeResources, nodeRepository);
         createHostNodes(2, nodeRepository, nodeFlavors);
@@ -149,27 +145,32 @@ public class PeriodicApplicationMaintainerTest {
     public void application_deploy_inhibits_redeploy_for_a_while() {
         fixture.activate();
 
+        assertEquals("No deployment expected", 2, fixture.deployer.redeployments);
+
         // Holds off on deployments a while after starting
         fixture.runApplicationMaintainer();
-        assertFalse("No deployment expected", fixture.deployer.lastDeployTime(fixture.app1).isPresent());
-        assertFalse("No deployment expected", fixture.deployer.lastDeployTime(fixture.app2).isPresent());
+        assertEquals("No deployment expected", 2, fixture.deployer.redeployments);
+
         // Exhaust initial wait period
         clock.advance(Duration.ofMinutes(30).plus(Duration.ofSeconds(1)));
 
         // First deployment of applications
         fixture.runApplicationMaintainer();
+        assertEquals("No deployment expected", 4, fixture.deployer.redeployments);
         Instant firstDeployTime = clock.instant();
         assertEquals(firstDeployTime, fixture.deployer.lastDeployTime(fixture.app1).get());
         assertEquals(firstDeployTime, fixture.deployer.lastDeployTime(fixture.app2).get());
         clock.advance(Duration.ofMinutes(5));
         fixture.runApplicationMaintainer();
         // Too soon: Not redeployed:
+        assertEquals("No deployment expected", 4, fixture.deployer.redeployments);
         assertEquals(firstDeployTime, fixture.deployer.lastDeployTime(fixture.app1).get());
         assertEquals(firstDeployTime, fixture.deployer.lastDeployTime(fixture.app2).get());
 
         clock.advance(Duration.ofMinutes(30));
         fixture.runApplicationMaintainer();
         // Redeployed:
+        assertEquals("No deployment expected", 6, fixture.deployer.redeployments);
         assertEquals(clock.instant(), fixture.deployer.lastDeployTime(fixture.app1).get());
         assertEquals(clock.instant(), fixture.deployer.lastDeployTime(fixture.app2).get());
     }
@@ -238,12 +239,10 @@ public class PeriodicApplicationMaintainerTest {
 
     private class Fixture {
 
-        final NodeResources nodeResources = new NodeResources(2, 8, 50);
         final NodeRepository nodeRepository;
-        final NodeRepositoryProvisioner provisioner;
-        final Curator curator;
         final MockDeployer deployer;
 
+        final NodeResources nodeResources = new NodeResources(2, 8, 50);
         final ApplicationId app1 = ApplicationId.from(TenantName.from("foo1"), ApplicationName.from("bar"), InstanceName.from("fuz"));
         final ApplicationId app2 = ApplicationId.from(TenantName.from("foo2"), ApplicationName.from("bar"), InstanceName.from("fuz"));
         final ClusterSpec clusterApp1 = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("test"), Version.fromString("6.42"), false);
@@ -253,39 +252,28 @@ public class PeriodicApplicationMaintainerTest {
 
         private final TestablePeriodicApplicationMaintainer maintainer;
 
-        Fixture(Zone zone, NodeRepository nodeRepository, NodeFlavors flavors, Curator curator) {
+        Fixture(Zone zone, NodeRepository nodeRepository, NodeFlavors flavors) {
             this.nodeRepository = nodeRepository;
-            this.curator = curator;
-            this.provisioner =  new NodeRepositoryProvisioner(nodeRepository, flavors, zone, new MockProvisionServiceProvider(), new InMemoryFlagSource());
-
-            Map<ApplicationId, MockDeployer.ApplicationContext> apps = new HashMap<>();
-            apps.put(app1, new MockDeployer.ApplicationContext(app1, clusterApp1,
-                                                               Capacity.fromCount(wantedNodesApp1, nodeResources), 1));
-            apps.put(app2, new MockDeployer.ApplicationContext(app2, clusterApp2,
-                                                               Capacity.fromCount(wantedNodesApp2, nodeResources), 1));
+            NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(
+                    nodeRepository, flavors, zone, new MockProvisionServiceProvider(), new InMemoryFlagSource());
+
+            Map<ApplicationId, MockDeployer.ApplicationContext> apps = Map.of(
+                    app1, new MockDeployer.ApplicationContext(app1, clusterApp1, Capacity.fromCount(wantedNodesApp1, nodeResources), 1),
+                    app2, new MockDeployer.ApplicationContext(app2, clusterApp2, Capacity.fromCount(wantedNodesApp2, nodeResources), 1));
             this.deployer = new MockDeployer(provisioner, nodeRepository.clock(), apps);
             this.maintainer = new TestablePeriodicApplicationMaintainer(deployer, nodeRepository, Duration.ofDays(1), // Long duration to prevent scheduled runs during test
                                                                         Duration.ofMinutes(30));
         }
 
         void activate() {
-            activate(app1, clusterApp1, wantedNodesApp1, provisioner);
-            activate(app2, clusterApp2, wantedNodesApp2, provisioner);
+            deployer.deployFromLocalActive(app1, false).get().activate();
+            deployer.deployFromLocalActive(app2, false).get().activate();
             assertEquals(wantedNodesApp1, nodeRepository.getNodes(app1, Node.State.active).size());
             assertEquals(wantedNodesApp2, nodeRepository.getNodes(app2, Node.State.active).size());
         }
 
-        private void activate(ApplicationId applicationId, ClusterSpec cluster, int nodeCount, NodeRepositoryProvisioner provisioner) {
-            List<HostSpec> hosts = provisioner.prepare(applicationId, cluster, Capacity.fromCount(nodeCount, nodeResources), 1, null);
-            NestedTransaction transaction = new NestedTransaction().add(new CuratorTransaction(curator));
-            provisioner.activate(transaction, applicationId, hosts);
-            transaction.commit();
-        }
-
         void remove(ApplicationId application) {
-            NestedTransaction transaction = new NestedTransaction().add(new CuratorTransaction(curator));
-            provisioner.remove(transaction, application);
-            transaction.commit();
+            deployer.removeApplication(application);
         }
 
         void runApplicationMaintainer() {