Remove instance only when no longer referenced, plus some test fixes

3 files changed, 18 insertions, 22 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 dd8725da081..0eca30ddeec 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
@@ -485,12 +485,6 @@ public class ApplicationController {
         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()) {
@@ -667,8 +661,9 @@ public class ApplicationController {
                                                (deploymentsToRemove.size() > 1 ? "these zones" : "this zone") +
                                                " in deployment.xml. " +
                                                ValidationOverrides.toAllowMessage(ValidationId.deploymentRemoval));
-        // 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();
+        // Remove the instance as well, if it is no longer referenced, and contains only production deployments that are removed now.
+        boolean removeInstance =    ! deploymentSpec.instanceNames().contains(instance)
+                                 &&   application.get().require(instance).deployments().size() == deploymentsToRemove.size();
         for (ZoneId zone : deploymentsToRemove)
             application = deactivate(application, instance, zone);
         if (removeInstance)
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
index 701acd3ddd2..45aee670363 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
@@ -6,6 +6,7 @@ import com.yahoo.config.application.api.Endpoint;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
+import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.vespa.hosted.controller.ApplicationController;
 import com.yahoo.vespa.hosted.controller.Instance;
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalDeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalDeploymentTester.java
index 4f17ab650fc..46666144153 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalDeploymentTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalDeploymentTester.java
@@ -251,12 +251,12 @@ public class InternalDeploymentTester {
         runner.advance(currentRun(type));
 
         if (type == JobType.stagingTest) { // Do the initial deployment and installation of the real application.
-            assertEquals(unfinished, jobs.active(id).get().steps().get(Step.installInitialReal));
+            assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installInitialReal));
             currentRun(type).versions().sourcePlatform().ifPresent(version -> tester.configServer().nodeRepository().doUpgrade(deployment, Optional.empty(), version));
             tester.configServer().convergeServices(instanceId, zone);
             setEndpoints(instanceId, zone);
             runner.advance(currentRun(type));
-            assertEquals(Step.Status.succeeded, jobs.active(id).get().steps().get(Step.installInitialReal));
+            assertEquals(Step.Status.succeeded, jobs.run(id).get().steps().get(Step.installInitialReal));
         }
     }
 
@@ -266,7 +266,7 @@ public class InternalDeploymentTester {
         ZoneId zone = type.zone(tester.controller().system());
         DeploymentId deployment = new DeploymentId(instanceId, zone);
 
-        assertEquals(unfinished, jobs.active(id).get().steps().get(Step.installReal));
+        assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installReal));
         tester.configServer().nodeRepository().doUpgrade(deployment, Optional.empty(), currentRun(type).versions().targetPlatform());
         runner.advance(currentRun(type));
     }
@@ -276,21 +276,21 @@ public class InternalDeploymentTester {
         RunId id = currentRun(type).id();
         ZoneId zone = type.zone(tester.controller().system());
 
-        assertEquals(unfinished, jobs.active(id).get().steps().get(Step.installReal));
+        assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installReal));
         tester.configServer().convergeServices(instanceId, zone);
         runner.advance(currentRun(type));
         if ( ! (currentRun(type).versions().sourceApplication().isPresent() && type.isProduction())
             && type != JobType.stagingTest) {
-            assertEquals(unfinished, jobs.active(id).get().steps().get(Step.installReal));
+            assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installReal));
             setEndpoints(instanceId, zone);
         }
         runner.advance(currentRun(type));
         if (type.environment().isManuallyDeployed()) {
-            assertEquals(Step.Status.succeeded, jobs.run(currentRun(type).id()).get().steps().get(Step.installReal));
-            assertTrue(jobs.run(currentRun(type).id()).get().hasEnded());
+            assertEquals(Step.Status.succeeded, jobs.run(id).get().steps().get(Step.installReal));
+            assertTrue(jobs.run(id).get().hasEnded());
             return;
         }
-        assertEquals(Step.Status.succeeded, jobs.active(id).get().steps().get(Step.installReal));
+        assertEquals(Step.Status.succeeded, jobs.run(id).get().steps().get(Step.installReal));
     }
 
     /** Installs tester and starts tests. */
@@ -298,13 +298,13 @@ public class InternalDeploymentTester {
         RunId id = currentRun(type).id();
         ZoneId zone = type.zone(tester.controller().system());
 
-        assertEquals(unfinished, jobs.active(id).get().steps().get(Step.installTester));
+        assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installTester));
         tester.configServer().nodeRepository().doUpgrade(new DeploymentId(testerId.id(), zone), Optional.empty(), currentRun(type).versions().targetPlatform());
         runner.advance(currentRun(type));
-        assertEquals(unfinished, jobs.active(id).get().steps().get(Step.installTester));
+        assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installTester));
         tester.configServer().convergeServices(testerId.id(), zone);
         runner.advance(currentRun(type));
-        assertEquals(unfinished, jobs.active(id).get().steps().get(Step.installTester));
+        assertEquals(unfinished, jobs.run(id).get().steps().get(Step.installTester));
         setEndpoints(testerId.id(), zone);
         runner.advance(currentRun(type));
     }
@@ -315,10 +315,10 @@ public class InternalDeploymentTester {
         ZoneId zone = type.zone(tester.controller().system());
 
         // All installation is complete and endpoints are ready, so tests may begin.
-        assertEquals(Step.Status.succeeded, jobs.active(id).get().steps().get(Step.installTester));
-        assertEquals(Step.Status.succeeded, jobs.active(id).get().steps().get(Step.startTests));
+        assertEquals(Step.Status.succeeded, jobs.run(id).get().steps().get(Step.installTester));
+        assertEquals(Step.Status.succeeded, jobs.run(id).get().steps().get(Step.startTests));
 
-        assertEquals(unfinished, jobs.active(id).get().steps().get(Step.endTests));
+        assertEquals(unfinished, jobs.run(id).get().steps().get(Step.endTests));
         cloud.set(TesterCloud.Status.SUCCESS);
         runner.advance(currentRun(type));
         assertTrue(jobs.run(id).get().hasEnded());