Determine application version in test environments

8 files changed, 107 insertions, 88 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
index 2c9c4261cbd..5105a016934 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
@@ -149,16 +149,6 @@ public class Application {
                 .min(Comparator.naturalOrder());
     }
 
-    /**
-     * Returns the oldest application version this has deployed in a permanent zone (not test or staging),
-     * or empty version if it is not deployed anywhere
-     */
-    public Optional<ApplicationVersion> oldestDeployedApplicationVersion() {
-        return productionDeployments().values().stream()
-                .map(Deployment::applicationVersion)
-                .min(Comparator.naturalOrder());
-    }
-
     /** Returns the version a new deployment to this zone should use for this application */
     public Version deployVersionIn(ZoneId zone, Controller controller) {
         return change.platform().orElse(versionIn(zone, controller));
@@ -170,17 +160,28 @@ public class Application {
                        .orElse(oldestDeployedVersion().orElse(controller.systemVersion()));
     }
 
-    /** Returns the application version a deployment to this zone should use, or empty if we don't know */
-    public Optional<ApplicationVersion> deployApplicationVersionIn(ZoneId zone) {
-        if (change().application().isPresent()) {
-            return Optional.of(change().application().get());
+    /** Returns the Vespa version to use for the given job */
+    public Version deployVersionFor(DeploymentJobs.JobType jobType, Controller controller) {
+        return jobType == DeploymentJobs.JobType.component
+                ? controller.systemVersion()
+                : deployVersionIn(jobType.zone(controller.system()).get(), controller);
+    }
+
+    /** Returns the application version to use for the given job */
+    public Optional<ApplicationVersion> deployApplicationVersionFor(DeploymentJobs.JobType jobType,
+                                                                    Controller controller,
+                                                                    boolean currentVersion) {
+        // Use last successful version if currentVersion is requested (staging deployment) or when we're not deploying
+        // a new application version
+        if (currentVersion || !change().application().isPresent()) {
+            Optional<ApplicationVersion> version = deploymentJobs().lastSuccessfulApplicationVersionFor(jobType);
+            if (version.isPresent()) {
+                return version;
+            }
         }
-        return applicationVersionIn(zone);
-    }
-
-    /** Returns the application version that is or should be deployed with in the given zone, or empty if unknown. */
-    public Optional<ApplicationVersion> applicationVersionIn(ZoneId zone) {
-        return Optional.ofNullable(deployments().get(zone)).map(Deployment::applicationVersion);
+        return jobType == DeploymentJobs.JobType.component
+                ? Optional.empty()
+                : deployApplicationVersionIn(jobType.zone(controller.system()).get());
     }
 
     /** Returns the global rotation of this, if present */
@@ -208,8 +209,22 @@ public class Application {
         return "application '" + id + "'";
     }
 
+    /** Returns whether changes to this are blocked in the given instant */
     public boolean isBlocked(Instant instant) {
          return ! deploymentSpec().canUpgradeAt(instant) || ! deploymentSpec().canChangeRevisionAt(instant);
     }
 
+    /** Returns the application version a deployment to this zone should use, or empty if we don't know */
+    private Optional<ApplicationVersion> deployApplicationVersionIn(ZoneId zone) {
+        if (change().application().isPresent()) {
+            return Optional.of(change().application().get());
+        }
+        return applicationVersionIn(zone);
+    }
+
+    /** Returns the application version that is or should be deployed with in the given zone, or empty if unknown. */
+    private Optional<ApplicationVersion> applicationVersionIn(ZoneId zone) {
+        return Optional.ofNullable(deployments().get(zone)).map(Deployment::applicationVersion);
+    }
+
 }
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 50d95c232c4..8544031ec2a 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
@@ -23,7 +23,6 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.RevisionId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId;
 import com.yahoo.vespa.hosted.controller.api.integration.athenz.AthenzClientFactory;
 import com.yahoo.vespa.hosted.controller.api.integration.athenz.ZmsClient;
-import com.yahoo.vespa.hosted.controller.api.integration.athenz.ZmsException;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServerClient;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.Log;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.NoInstanceException;
@@ -297,9 +296,9 @@ public class ApplicationController {
                 if (!job.isPresent()) {
                     throw new IllegalArgumentException("Cannot determine job for zone " + zone);
                 }
-                applicationVersion = application.deployApplicationVersion(job.get(), controller,
-                                                                          options.deployCurrentVersion)
-                                                .orElseThrow(() -> new IllegalArgumentException("Cannot determine application version for " + applicationId));
+                applicationVersion = application.deployApplicationVersionFor(job.get(), controller,
+                                                                             options.deployCurrentVersion)
+                                                .orElseThrow(() -> new IllegalArgumentException("Cannot determine application version for " + applicationId + " in " + job.get()));
                 if (canDownloadArtifact(applicationVersion)) {
                     applicationPackage = new ApplicationPackage(
                             artifactRepository.getApplicationPackage(applicationId, applicationVersion.id())
@@ -706,11 +705,9 @@ public class ApplicationController {
 
     /** Returns whether component has reported a version number that is availabe in artifact repository */
     private static boolean canDownloadReportedApplicationVersion(Application application) {
-        return Optional.ofNullable(application.deploymentJobs().jobStatus().get(DeploymentJobs.JobType.component))
-                       .flatMap(JobStatus::lastSuccess)
-                       .map(JobStatus.JobRun::applicationVersion)
-                       .filter(ApplicationController::canDownloadArtifact)
-                       .isPresent();
+        return application.deploymentJobs().lastSuccessfulApplicationVersionFor(DeploymentJobs.JobType.component)
+                          .filter(ApplicationController::canDownloadArtifact)
+                          .isPresent();
     }
 
     /** Verify that each of the production zones listed in the deployment spec exist in this system. */
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java
index 10752b74967..950790e26b6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java
@@ -6,11 +6,11 @@ import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.application.api.ValidationOverrides;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
-import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.api.integration.MetricsService;
 import com.yahoo.vespa.hosted.controller.api.integration.MetricsService.ApplicationMetrics;
 import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId;
+import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.application.ApplicationRotation;
 import com.yahoo.vespa.hosted.controller.application.ApplicationVersion;
 import com.yahoo.vespa.hosted.controller.application.Change;
@@ -142,29 +142,6 @@ public class LockedApplication extends Application {
         return new LockedApplication(new Builder(this).with(rotation));
     }
 
-    public Version deployVersionFor(DeploymentJobs.JobType jobType, Controller controller) {
-        return jobType == JobType.component
-               ? controller.systemVersion()
-               : deployVersionIn(jobType.zone(controller.system()).get(), controller);
-    }
-
-    public Optional<ApplicationVersion> deployApplicationVersion(DeploymentJobs.JobType jobType, Controller controller) {
-        return deployApplicationVersion(jobType, controller, false);
-    }
-
-    public Optional<ApplicationVersion> deployApplicationVersion(DeploymentJobs.JobType jobType, Controller controller,
-                                                                 boolean currentVersion) {
-        if (currentVersion) {
-            Optional<ApplicationVersion> version = oldestDeployedApplicationVersion();
-            if (version.isPresent()) {
-                return version;
-            }
-        }
-        return jobType == JobType.component
-                ? Optional.empty()
-                : deployApplicationVersionIn(jobType.zone(controller.system()).get());
-    }
-
     /** Don't expose non-leaf sub-objects. */
     private LockedApplication with(Deployment deployment) {
         Map<ZoneId, Deployment> deployments = new LinkedHashMap<>(deployments());
@@ -172,7 +149,6 @@ public class LockedApplication extends Application {
         return new LockedApplication(new Builder(this).with(deployments));
     }
 
-
     private static class Builder {
 
         private final ApplicationId applicationId;
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java
index 932777e690d..7e1dcfe2bc6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java
@@ -131,12 +131,11 @@ public class DeploymentJobs {
         return true; // other environments do not have any preconditions
     }
 
-    /** Returns whether the job of the given type has completed successfully for the given change */
-    public boolean isSuccessful(Change change, JobType jobType) {
+    /** Returns the last successful application version for the given job */
+    public Optional<ApplicationVersion> lastSuccessfulApplicationVersionFor(JobType jobType) {
         return Optional.ofNullable(jobStatus().get(jobType))
-                .flatMap(JobStatus::lastSuccess)
-                .filter(status -> status.lastCompletedWas(change))
-                .isPresent();
+                       .flatMap(JobStatus::lastSuccess)
+                       .map(JobStatus.JobRun::applicationVersion);
     }
 
     /**
@@ -148,6 +147,14 @@ public class DeploymentJobs {
 
     public Optional<IssueId> issueId() { return issueId; }
 
+    /** Returns whether the job of the given type has completed successfully for the given change */
+    private boolean isSuccessful(Change change, JobType jobType) {
+        return Optional.ofNullable(jobStatus().get(jobType))
+                       .flatMap(JobStatus::lastSuccess)
+                       .filter(status -> status.lastCompletedWas(change))
+                       .isPresent();
+    }
+
     private static Optional<Long> requireId(Optional<Long> id, String message) {
         Objects.requireNonNull(id, message);
         if ( ! id.isPresent()) {
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 271fbe65dbb..067d8d41f53 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
@@ -353,7 +353,7 @@ public class DeploymentTrigger {
                                              application.change(),
                                              clock.instant(),
                                              application.deployVersionFor(jobType, controller),
-                                             application.deployApplicationVersion(jobType, controller)
+                                             application.deployApplicationVersionFor(jobType, controller, false)
                                                         .orElse(ApplicationVersion.unknown),
                                              reason);
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java
index bd5aef1ec3a..08f96195d2a 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java
@@ -42,8 +42,8 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
     private final Map<URI, Version> versions = new HashMap<>();
 
     private Version defaultVersion = new Version(6, 1, 0);
-    private RuntimeException prepareException = null;
     private Version lastPrepareVersion = null;
+    private RuntimeException prepareException = null;
 
     /** The version given in the previous prepare call, or empty if no call has been made */
     public Optional<Version> lastPrepareVersion() {
@@ -72,7 +72,11 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
     public void setDefaultVersion(Version version) {
         this.defaultVersion = version;
     }
-    
+
+    public Version getDefaultVersion() {
+        return defaultVersion;
+    }
+
     @Override
     public PreparedApplication prepare(DeploymentId deployment, DeployOptions deployOptions, Set<String> rotationCnames,
                                        Set<String> rotationNames, byte[] content) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 46f045df215..1f2a9c4e910 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -57,7 +57,6 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.NoSuchElementException;
 import java.util.Optional;
 import java.util.function.Supplier;
 
@@ -103,7 +102,7 @@ public class ControllerTest {
                 .build();
 
         // staging job - succeeding
-        Version version1 = Version.fromString("6.1"); // Set in config server mock
+        Version version1 = tester.defaultVespaVersion();
         Application app1 = tester.createApplication("app1", "tenant1", 1, 11L);
         tester.notifyJobCompletion(component, app1, true);
         assertEquals("Application version is currently not known",
@@ -219,9 +218,13 @@ public class ControllerTest {
 
         ApplicationVersion applicationVersion = ApplicationVersion.from(source, 101);
         tester.artifactRepository().put(app.id(), applicationPackage, applicationVersion.id());
-        runDeployment(tester, app.id(), applicationVersion, applicationPackage, Optional.of(source), 101);
+        runDeployment(tester, app.id(), applicationVersion, Optional.empty(), Optional.of(source), 101);
         assertEquals("Artifact is downloaded twice in staging and once for other zones", 5,
                      tester.artifactRepository().hits(app.id(), applicationVersion.id()));
+
+        // Application is upgraded. This makes deployment orchestration pick the last successful application version in
+        // zones which do not have permanent deployments, e.g. test and staging
+        runUpgrade(tester, app.id(), applicationVersion);
     }
 
     @Test
@@ -237,7 +240,7 @@ public class ControllerTest {
 
         // Scenario 1: Old fashioned deployment. Simulates existing production deployments
         ApplicationVersion v0 = ApplicationVersion.from(applicationPackage.hash(), source);
-        runDeployment(tester, app.id(), v0, applicationPackage, Optional.empty(), 100);
+        runDeployment(tester, app.id(), v0, Optional.of(applicationPackage), Optional.empty(), 100);
         assertEquals("Nothing downloaded from repository", 0,
                      tester.artifactRepository().hits(app.id(), v0.id()));
 
@@ -245,7 +248,7 @@ public class ControllerTest {
         // the application package from the deployer is used as v0 cannot be downloaded from repository.
         ApplicationVersion v1 = ApplicationVersion.from(source, 101);
         tester.artifactRepository().put(app.id(), applicationPackage, v1.id());
-        runDeployment(tester, app.id(), v1, applicationPackage, Optional.of(source), 101);
+        runDeployment(tester, app.id(), v1, Optional.of(applicationPackage), Optional.of(source), 101);
         assertEquals("Artifact is downloaded once per zone", 4,
                      tester.artifactRepository().hits(app.id(), v1.id()));
         assertEquals("v0 is never downloaded", 0,
@@ -255,7 +258,7 @@ public class ControllerTest {
         // Scenario 3: Both application versions are available in repository
         ApplicationVersion v2 = ApplicationVersion.from(source, 102);
         tester.artifactRepository().put(app.id(), applicationPackage, v2.id());
-        runDeployment(tester, app.id(), v2, applicationPackage, Optional.of(source), 102);
+        runDeployment(tester, app.id(), v2, Optional.empty(), Optional.of(source), 102);
         assertEquals("Previous artifact is downloaded once", 1,
                      tester.artifactRepository().hits(app.id(), v1.id()));
         assertEquals("Artifact is downloaded once per zone", 4,
@@ -852,48 +855,57 @@ public class ControllerTest {
 
     }
 
-    private void runDeployment(DeploymentTester tester, ApplicationId application, ApplicationVersion expectedVersion,
-                               ApplicationPackage applicationPackage, Optional<SourceRevision> sourceRevision,
+    private void runUpgrade(DeploymentTester tester, ApplicationId application, ApplicationVersion version) {
+        Version next = Version.fromString("6.2");
+        tester.upgradeSystem(next);
+        runDeployment(tester, tester.applications().require(application), version, Optional.of(next), Optional.empty());
+    }
+
+    private void runDeployment(DeploymentTester tester, ApplicationId application, ApplicationVersion version,
+                               Optional<ApplicationPackage> applicationPackage, Optional<SourceRevision> sourceRevision,
                                long initialBuildNumber) {
-        Version vespaVersion = Version.fromString("6.1"); // Set in config server mock
         Application app = tester.applications().require(application);
-
-        // Component notifies completion
         tester.notifyJobCompletion(component, app, Optional.empty(), sourceRevision, initialBuildNumber);
         ApplicationVersion change = sourceRevision.map(sr -> ApplicationVersion.from(sr, initialBuildNumber))
-                .orElse(ApplicationVersion.unknown);
+                                                  .orElse(ApplicationVersion.unknown);
         assertEquals(change.id(), tester.controller().applications()
-                .require(application)
-                .change().application().get().id());
+                                        .require(application)
+                                        .change().application().get().id());
+        runDeployment(tester, app, version, Optional.empty(), applicationPackage);
+    }
+
+    private void runDeployment(DeploymentTester tester, Application app, ApplicationVersion version,
+                               Optional<Version> upgrade, Optional<ApplicationPackage> applicationPackage) {
+        Version vespaVersion = upgrade.orElseGet(tester::defaultVespaVersion);
 
         // Deploy in test
-        tester.deployAndNotify(app, applicationPackage, true, systemTest);
-        tester.deployAndNotify(app, applicationPackage, true, stagingTest);
-        //assertEquals(4, applications.require(application).deploymentJobs().jobStatus().size());
+        tester.deployAndNotify(app, applicationPackage, true, true, systemTest);
+        tester.deployAndNotify(app, applicationPackage, true, true, stagingTest);
         assertStatus(JobStatus.initial(stagingTest)
-                             .withTriggering(vespaVersion, expectedVersion, false, "",
+                             .withTriggering(vespaVersion, version, upgrade.isPresent(), "",
                                              tester.clock().instant().minus(Duration.ofMillis(1)))
                              .withCompletion(42, Optional.empty(), tester.clock().instant(),
-                                             tester.controller()), application, tester.controller());
+                                             tester.controller()), app.id(), tester.controller());
 
         // Deploy in production
-        tester.deployAndNotify(app, applicationPackage, true, productionCorpUsEast1);
+        tester.deployAndNotify(app, applicationPackage, true, true, productionCorpUsEast1);
         assertStatus(JobStatus.initial(productionCorpUsEast1)
-                             .withTriggering(vespaVersion, expectedVersion, false, "",
+                             .withTriggering(vespaVersion, version, upgrade.isPresent(), "",
                                              tester.clock().instant().minus(Duration.ofMillis(1)))
                              .withCompletion(42, Optional.empty(), tester.clock().instant(),
-                                             tester.controller()), application, tester.controller());
+                                             tester.controller()), app.id(), tester.controller());
         tester.deployAndNotify(app, applicationPackage, true, true, productionUsEast3);
         assertStatus(JobStatus.initial(productionUsEast3)
-                             .withTriggering(vespaVersion, expectedVersion, false, "",
+                             .withTriggering(vespaVersion, version, upgrade.isPresent(), "",
                                              tester.clock().instant().minus(Duration.ofMillis(1)))
                              .withCompletion(42, Optional.empty(), tester.clock().instant(),
-                                             tester.controller()), application, tester.controller());
+                                             tester.controller()), app.id(), tester.controller());
 
         // Verify deployed version
         app = tester.controller().applications().require(app.id());
         for (Deployment deployment : app.productionDeployments().values()) {
-            assertEquals(expectedVersion, deployment.applicationVersion());
+            assertEquals(version, deployment.applicationVersion());
+            upgrade.ifPresent(v -> assertEquals(v, deployment.version()));
         }
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
index 58f2a21fa6a..c923542b8c9 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
@@ -104,11 +104,15 @@ public class DeploymentTester {
     }
 
     public void upgradeSystem(Version version) {
-        controllerTester().configServer().setDefaultVersion(version);
+        configServer().setDefaultVersion(version);
         updateVersionStatus(version);
         upgrader().maintain();
     }
 
+    public Version defaultVespaVersion() {
+        return configServer().getDefaultVersion();
+    }
+
     public Application createApplication(String applicationName, String tenantName, long projectId, Long propertyId) {
         TenantId tenant = tester.createTenant(tenantName, UUID.randomUUID().toString(), propertyId);
         return tester.createApplication(tenant, applicationName, "default", projectId);
@@ -279,6 +283,10 @@ public class DeploymentTester {
                 deploy(job, application, applicationPackage, false);
             }
             notifyJobCompletion(job, application, success);
+            // Deactivate test deployments after deploy. This replicates the behaviour of the tenant pipeline
+            if (job.isTest()) {
+                controller().applications().deactivate(application, job.zone(controller().system()).get());
+            }
         }
     }