Create new instances as per spec, and clean up removed ones

3 files changed, 68 insertions, 33 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index ea8f805299e..dd8725da081 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -303,19 +303,23 @@ public class ApplicationController {
      * @throws IllegalArgumentException if the instance already exists, or has an invalid instance name.
      */
     public void createInstance(ApplicationId id) {
+        lockApplicationOrThrow(TenantAndApplicationId.from(id), application -> {
+            store(withNewInstance(application, id));
+        });
+    }
+
+    private LockedApplication withNewInstance(LockedApplication application, ApplicationId id) {
         if (id.instance().isTester())
             throw new IllegalArgumentException("'" + id + "' is a tester application!");
-        lockApplicationOrThrow(TenantAndApplicationId.from(id), application -> {
-            InstanceId.validate(id.instance().value());
+        InstanceId.validate(id.instance().value());
 
-            if (getInstance(id).isPresent())
-                throw new IllegalArgumentException("Could not create '" + id + "': Instance already exists");
-            if (getInstance(dashToUnderscore(id)).isPresent()) // VESPA-1945
-                throw new IllegalArgumentException("Could not create '" + id + "': Instance " + dashToUnderscore(id) + " already exists");
+        if (getInstance(id).isPresent())
+            throw new IllegalArgumentException("Could not create '" + id + "': Instance already exists");
+        if (getInstance(dashToUnderscore(id)).isPresent()) // VESPA-1945
+            throw new IllegalArgumentException("Could not create '" + id + "': Instance " + dashToUnderscore(id) + " already exists");
 
-            store(application.withNewInstance(id.instance()));
-            log.info("Created " + id);
-        });
+        log.info("Created " + id);
+        return application.withNewInstance(id.instance());
     }
 
     public ActivateResult deploy(ApplicationId applicationId, ZoneId zone,
@@ -460,20 +464,33 @@ public class ApplicationController {
         application = application.with(applicationPackage.deploymentSpec());
         application = application.with(applicationPackage.validationOverrides());
 
+        var existingInstances = application.get().instances().keySet();
+        var declaredInstances = applicationPackage.deploymentSpec().instanceNames();
+        for (var name : declaredInstances)
+            if ( ! existingInstances.contains(name))
+                application = withNewInstance(application, application.get().id().instance(name));
+
         // Delete zones not listed in DeploymentSpec, if allowed
         // We do this at deployment time for externally built applications, and at submission time
         // for internally built ones, to be able to return a validation failure message when necessary
-        for (InstanceName instanceName : application.get().instances().keySet()) {
-            application = withoutDeletedDeployments(application, instanceName);
-
-                // Clean up deployment jobs that are no longer referenced by deployment spec
-            DeploymentSpec deploymentSpec = application.get().deploymentSpec();
-            application = application.with(instanceName, instance -> withoutUnreferencedDeploymentJobs(deploymentSpec, instance));
+        for (InstanceName name : existingInstances) {
+            application = withoutDeletedDeployments(application, name);
+            // Clean up deployment jobs that are no longer referenced by deployment spec
+            if (application.get().instances().containsKey(name)) {
+                DeploymentSpec deploymentSpec = application.get().deploymentSpec();
+                application = application.with(name, instance -> withoutUnreferencedDeploymentJobs(deploymentSpec, instance));
+            }
         }
         store(application);
         return application;
     }
 
+    private boolean specIncludes(DeploymentSpec deploymentSpec, InstanceName name, ZoneId zone) {
+        return deploymentSpec.instance(name)
+                             .map(spec -> spec.includes(zone.environment(), Optional.of(zone.region())))
+                             .orElse(false);
+    }
+
     /** Deploy a system application to given zone */
     public void deploy(SystemApplication application, ZoneId zone, Version version) {
         if (application.hasApplicationPackage()) {
@@ -630,27 +647,32 @@ public class ApplicationController {
 
     private LockedApplication withoutDeletedDeployments(LockedApplication application, InstanceName instance) {
         DeploymentSpec deploymentSpec = application.get().deploymentSpec();
-        List<Deployment> deploymentsToRemove = application.get().require(instance).productionDeployments().values().stream()
-                                                          .filter(deployment ->      deploymentSpec.instance(instance).isEmpty()
-                                                                                || ! deploymentSpec.requireInstance(instance).includes(deployment.zone().environment(),
-                                                                                                                                     Optional.of(deployment.zone().region())))
-                                                          .collect(Collectors.toList());
+        List<ZoneId> deploymentsToRemove = application.get().require(instance).productionDeployments().values().stream()
+                                                      .map(Deployment::zone)
+                                                      .filter(zone ->      deploymentSpec.instance(instance).isEmpty()
+                                                                      || ! deploymentSpec.requireInstance(instance).includes(zone.environment(),
+                                                                                                                             Optional.of(zone.region())))
+                                                      .collect(Collectors.toList());
 
-        if (deploymentsToRemove.isEmpty()) return application;
+        if (deploymentsToRemove.isEmpty())
+            return application;
 
         if ( ! application.get().validationOverrides().allows(ValidationId.deploymentRemoval, clock.instant()))
             throw new IllegalArgumentException(ValidationId.deploymentRemoval.value() + ": " + application.get().require(instance) +
                                                " is deployed in " +
                                                deploymentsToRemove.stream()
-                                                                   .map(deployment -> deployment.zone().region().value())
-                                                                   .collect(Collectors.joining(", ")) +
+                                                                  .map(zone -> zone.region().value())
+                                                                  .collect(Collectors.joining(", ")) +
                                                ", but does not include " +
                                                (deploymentsToRemove.size() > 1 ? "these zones" : "this zone") +
                                                " in deployment.xml. " +
                                                ValidationOverrides.toAllowMessage(ValidationId.deploymentRemoval));
-
-        for (Deployment deployment : deploymentsToRemove)
-            application = deactivate(application, instance, deployment.zone());
+        // Remove the instance as well, if it contains only production deployments that are removed now.
+        boolean removeInstance = application.get().require(instance).deployments().size() == deploymentsToRemove.size();
+        for (ZoneId zone : deploymentsToRemove)
+            application = deactivate(application, instance, zone);
+        if (removeInstance)
+            application = application.without(instance);
         return application;
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
index 20e01dc850d..f67cb0877ef 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
@@ -1,9 +1,11 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.deployment;
 
+import com.yahoo.config.application.api.DeploymentInstanceSpec;
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.application.api.DeploymentSpec.Step;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.hosted.controller.Application;
@@ -356,7 +358,7 @@ public class DeploymentTrigger {
                     for (Step step : steps.production()) {
                         List<JobType> stepJobs = steps.toJobs(step);
                         List<JobType> remainingJobs = stepJobs.stream().filter(job -> ! isComplete(change, change, instance, job)).collect(toList());
-                        if (!remainingJobs.isEmpty()) { // Change is incomplete; trigger remaining jobs if ready, or their test jobs if untested.
+                        if ( ! remainingJobs.isEmpty()) { // Change is incomplete; trigger remaining jobs if ready, or their test jobs if untested.
                             for (JobType job : remainingJobs) {
                                 Versions versions = Versions.from(change, application, deploymentFor(instance, job),
                                                                   controller.systemVersion());
@@ -380,7 +382,7 @@ public class DeploymentTrigger {
                         }
                         else { // All jobs are complete; find the time of completion of this step.
                             if (stepJobs.isEmpty()) { // No jobs means this is a delay step.
-                                completedAt = completedAt.map(at -> at.plus(step.delay())).filter(at -> !at.isAfter(clock.instant()));
+                                completedAt = completedAt.map(at -> at.plus(step.delay())).filter(at -> ! at.isAfter(clock.instant()));
                                 reason += " after a delay of " + step.delay();
                             }
                             else {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
index 71ce8457a57..8cec18281ce 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
@@ -6,8 +6,6 @@ import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.zone.ZoneId;
-import com.yahoo.slime.Slime;
-import com.yahoo.vespa.config.SlimeUtils;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
@@ -20,18 +18,16 @@ import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockBuildService;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
-import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId;
 import com.yahoo.vespa.hosted.controller.maintenance.JobControl;
 import com.yahoo.vespa.hosted.controller.maintenance.ReadyJobsTrigger;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
 import org.junit.Ignore;
 import org.junit.Test;
 
-import java.nio.file.Files;
-import java.nio.file.Paths;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
@@ -1235,6 +1231,21 @@ public class DeploymentTriggerTest {
         iTester.newSubmission();
     }
 
+    public void testMultipleInstances() {
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .instances("instance1,instance2")
+                .environment(Environment.prod)
+                .region("us-east-3")
+                .build();
+        InternalDeploymentTester tester = new InternalDeploymentTester();
+        ApplicationVersion version = tester.newSubmission(appId, applicationPackage);
+        tester.deployNewSubmission(appId, version);
+        assertEquals(2, tester.tester().application(appId).instances().size());
+        assertEquals(2, tester.tester().application(appId).productionDeployments().values().stream()
+                              .mapToInt(Collection::size)
+                              .sum());
+    }
+
     /** Verifies that the given job lists have the same jobs, ignoring order of jobs that may have been triggered concurrently. */
     private static void assertJobsInOrder(List<BuildService.BuildJob> expected, List<BuildService.BuildJob> actual) {
         assertEquals(expected.stream().filter(job -> job.jobName().equals(systemTest.jobName())).collect(Collectors.toList()),