Revert "Let outstanding change know the application version"

This reverts commit e2853614b5e365c1607d067661e15e709761f9c6.

9 files changed, 49 insertions, 68 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..f26502e4630 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
@@ -42,7 +42,7 @@ public class Application {
     private final Map<ZoneId, Deployment> deployments;
     private final DeploymentJobs deploymentJobs;
     private final Change change;
-    private final Change outstandingChange;
+    private final boolean outstandingChange;
     private final Optional<IssueId> ownershipIssueId;
     private final ApplicationMetrics metrics;
     private final Optional<RotationId> rotation;
@@ -51,14 +51,14 @@ public class Application {
     public Application(ApplicationId id) {
         this(id, DeploymentSpec.empty, ValidationOverrides.empty, Collections.emptyMap(),
              new DeploymentJobs(Optional.empty(), Collections.emptyList(), Optional.empty()),
-             Change.empty(), Change.empty(), Optional.empty(), new ApplicationMetrics(0, 0),
+             Change.empty(), false, Optional.empty(), new ApplicationMetrics(0, 0),
              Optional.empty());
     }
 
     /** Used from persistence layer: Do not use */
     public Application(ApplicationId id, DeploymentSpec deploymentSpec, ValidationOverrides validationOverrides,
                        List<Deployment> deployments, DeploymentJobs deploymentJobs, Change change,
-                       Change outstandingChange, Optional<IssueId> ownershipIssueId, ApplicationMetrics metrics,
+                       boolean outstandingChange, Optional<IssueId> ownershipIssueId, ApplicationMetrics metrics,
                        Optional<RotationId> rotation) {
         this(id, deploymentSpec, validationOverrides,
              deployments.stream().collect(Collectors.toMap(Deployment::zone, d -> d)),
@@ -67,7 +67,7 @@ public class Application {
 
     Application(ApplicationId id, DeploymentSpec deploymentSpec, ValidationOverrides validationOverrides,
                 Map<ZoneId, Deployment> deployments, DeploymentJobs deploymentJobs, Change change,
-                Change outstandingChange, Optional<IssueId> ownershipIssueId, ApplicationMetrics metrics,
+                boolean outstandingChange, Optional<IssueId> ownershipIssueId, ApplicationMetrics metrics,
                 Optional<RotationId> rotation) {
         Objects.requireNonNull(id, "id cannot be null");
         Objects.requireNonNull(deploymentSpec, "deploymentSpec cannot be null");
@@ -129,7 +129,7 @@ public class Application {
      * Returns whether this has an outstanding change (in the source repository), which
      * has currently not started deploying (because a deployment is (or was) already in progress
      */
-    public Change outstandingChange() { return outstandingChange; }
+    public boolean hasOutstandingChange() { return outstandingChange; }
 
     public Optional<IssueId> ownershipIssueId() {
         return ownershipIssueId;
@@ -173,7 +173,11 @@ public class Application {
     /** 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());
+            ApplicationVersion version = change().application().get();
+            if (version.isUnknown())
+                return Optional.empty();
+            else
+                return Optional.of(version);
         }
         return applicationVersionIn(zone);
     }
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..4b6213733a0 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
@@ -49,7 +49,7 @@ public class LockedApplication extends Application {
     private LockedApplication(Builder builder) {
         super(builder.applicationId, builder.deploymentSpec, builder.validationOverrides,
               builder.deployments, builder.deploymentJobs, builder.deploying,
-              builder.outstandingChange, builder.ownershipIssueId, builder.metrics, builder.rotation);
+              builder.hasOutstandingChange, builder.ownershipIssueId, builder.metrics, builder.rotation);
     }
 
     public LockedApplication withProjectId(long projectId) {
@@ -126,8 +126,8 @@ public class LockedApplication extends Application {
         return new LockedApplication(new Builder(this).withChange(change));
     }
 
-    public LockedApplication withOutstandingChange(Change outstandingChange) {
-        return new LockedApplication(new Builder(this).withOutstandingChange(outstandingChange));
+    public LockedApplication withOutstandingChange(boolean outstandingChange) {
+        return new LockedApplication(new Builder(this).with(outstandingChange));
     }
 
     public LockedApplication withOwnershipIssueId(IssueId issueId) {
@@ -181,7 +181,7 @@ public class LockedApplication extends Application {
         private Map<ZoneId, Deployment> deployments;
         private DeploymentJobs deploymentJobs;
         private Change deploying;
-        private Change outstandingChange;
+        private boolean hasOutstandingChange;
         private Optional<IssueId> ownershipIssueId;
         private ApplicationMetrics metrics;
         private Optional<RotationId> rotation;
@@ -193,7 +193,7 @@ public class LockedApplication extends Application {
             this.deployments = application.deployments();
             this.deploymentJobs = application.deploymentJobs();
             this.deploying = application.change();
-            this.outstandingChange = application.outstandingChange();
+            this.hasOutstandingChange = application.hasOutstandingChange();
             this.ownershipIssueId = application.ownershipIssueId();
             this.metrics = application.metrics();
             this.rotation = application.rotation().map(ApplicationRotation::id);
@@ -224,8 +224,8 @@ public class LockedApplication extends Application {
             return this;
         }
 
-        private Builder withOutstandingChange(Change outstandingChange) {
-            this.outstandingChange = outstandingChange;
+        private Builder with(boolean hasOutstandingChange) {
+            this.hasOutstandingChange = hasOutstandingChange;
             return this;
         }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java
index 216025215d3..13d66c8d083 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java
@@ -31,10 +31,6 @@ public final class Change {
     private Change(Optional<Version> platform, Optional<ApplicationVersion> application) {
         Objects.requireNonNull(platform, "platform cannot be null");
         Objects.requireNonNull(application, "application cannot be null");
-        if (application.isPresent() && application.get().isUnknown()) {
-            // TODO: Require version to be known for application change
-            //throw new IllegalArgumentException("Application version to deploy must be a known version");
-        }
         this.platform = platform;
         this.application = application;
     }
@@ -46,7 +42,7 @@ public final class Change {
         return false;
     }
 
-    /** Returns whether a change should currently be deployed */
+    /** Returns whether a change shoudl currently be deployed */
     public boolean isPresent() {
         return platform.isPresent() || application.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..6560423f3b9 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
@@ -78,8 +78,8 @@ public class DeploymentTrigger {
      */
     public void triggerFromCompletion(JobReport report) {
         applications().lockOrThrow(report.applicationId(), application -> {
-            ApplicationVersion applicationVersion = applicationVersionFrom(report);
-            application = application.withJobCompletion(report, applicationVersion, clock.instant(), controller);
+            application = application.withJobCompletion(report, applicationVersionFrom(report), clock.instant(),
+                                                        controller);
             application = application.withProjectId(report.projectId());
 
             // Handle successful starting and ending
@@ -88,11 +88,15 @@ public class DeploymentTrigger {
                     if (acceptNewApplicationVersionNow(application)) {
                         // Set this as the change we are doing, unless we are already pushing a platform change
                         if ( ! ( application.change().platform().isPresent())) {
+                            ApplicationVersion applicationVersion = ApplicationVersion.unknown;
+                            if (report.sourceRevision().isPresent())
+                                applicationVersion = ApplicationVersion.from(report.sourceRevision().get(),
+                                                                             report.buildNumber());
                             application = application.withChange(Change.of(applicationVersion));
                         }
                     }
                     else { // postpone
-                        applications().store(application.withOutstandingChange(Change.of(applicationVersion)));
+                        applications().store(application.withOutstandingChange(true));
                         return;
                     }
                 }
@@ -260,7 +264,7 @@ public class DeploymentTrigger {
                                                    application.change() + " is already in progress");
             application = application.withChange(change);
             if (change.application().isPresent())
-                application = application.withOutstandingChange(Change.empty());
+                application = application.withOutstandingChange(false);
             application = trigger(JobType.systemTest, application, false, change.toString());
             applications().store(application);
         });
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java
index 9f1738f9560..3dd63a511e1 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java
@@ -4,6 +4,8 @@ package com.yahoo.vespa.hosted.controller.maintenance;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.application.ApplicationList;
+import com.yahoo.vespa.hosted.controller.application.ApplicationVersion;
+import com.yahoo.vespa.hosted.controller.application.Change;
 
 import java.time.Duration;
 
@@ -22,9 +24,9 @@ public class OutstandingChangeDeployer extends Maintainer {
     protected void maintain() {
         ApplicationList applications = ApplicationList.from(controller().applications().asList()).notPullRequest();
         for (Application application : applications.asList()) {
-            if (!application.change().isPresent() && application.outstandingChange().isPresent())
+            if (application.hasOutstandingChange() &&  ! application.change().isPresent())
                 controller().applications().deploymentTrigger().triggerChange(application.id(),
-                                                                              application.outstandingChange());
+                                                                              Change.of(ApplicationVersion.unknown));
         }
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java
index 670338d10e5..4b7993b3b7d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java
@@ -10,7 +10,6 @@ import com.yahoo.slime.ArrayTraverser;
 import com.yahoo.slime.Cursor;
 import com.yahoo.slime.Inspector;
 import com.yahoo.slime.Slime;
-import com.yahoo.slime.Type;
 import com.yahoo.vespa.config.SlimeUtils;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.api.integration.MetricsService.ApplicationMetrics;
@@ -127,8 +126,8 @@ public class ApplicationSerializer {
         root.setString(validationOverridesField, application.validationOverrides().xmlForm());
         deploymentsToSlime(application.deployments().values(), root.setArray(deploymentsField));
         toSlime(application.deploymentJobs(), root.setObject(deploymentJobsField));
-        toSlime(application.change(), root, deployingField);
-        toSlime(application.outstandingChange(), root, outstandingChangeField);
+        toSlime(application.change(), root);
+        root.setBool(outstandingChangeField, application.hasOutstandingChange());
         application.ownershipIssueId().ifPresent(issueId -> root.setString(ownershipIssueIdField, issueId.value()));
         root.setDouble(queryQualityField, application.metrics().queryServiceQuality());
         root.setDouble(writeQualityField, application.metrics().writeServiceQuality());
@@ -244,19 +243,20 @@ public class ApplicationSerializer {
         Cursor object = parent.setObject(jobRunObjectName);
         object.setLong(jobRunIdField, jobRun.get().id());
         object.setString(versionField, jobRun.get().version().toString());
-        toSlime(jobRun.get().applicationVersion(), object.setObject(revisionField));
+        if ( ! jobRun.get().applicationVersion().isUnknown())
+            toSlime(jobRun.get().applicationVersion(), object.setObject(revisionField));
         object.setBool(upgradeField, jobRun.get().upgrade());
         object.setString(reasonField, jobRun.get().reason());
         object.setLong(atField, jobRun.get().at().toEpochMilli());
     }
 
-    private void toSlime(Change deploying, Cursor parentObject, String fieldName) {
+    private void toSlime(Change deploying, Cursor parentObject) {
         if ( ! deploying.isPresent()) return;
 
-        Cursor object = parentObject.setObject(fieldName);
+        Cursor object = parentObject.setObject(deployingField);
         if (deploying.platform().isPresent())
             object.setString(versionField, deploying.platform().get().toString());
-        if (deploying.application().isPresent())
+        if (deploying.application().isPresent() && !deploying.application().get().isUnknown())
             toSlime(deploying.application().get(), object);
     }
 
@@ -271,7 +271,7 @@ public class ApplicationSerializer {
         List<Deployment> deployments = deploymentsFromSlime(root.field(deploymentsField));
         DeploymentJobs deploymentJobs = deploymentJobsFromSlime(root.field(deploymentJobsField));
         Change deploying = changeFromSlime(root.field(deployingField));
-        Change outstandingChange = outstandingChangeFromSlime(root.field(outstandingChangeField));
+        boolean outstandingChange = root.field(outstandingChangeField).asBool();
         Optional<IssueId> ownershipIssueId = optionalString(root.field(ownershipIssueIdField)).map(IssueId::from);
         ApplicationMetrics metrics = new ApplicationMetrics(root.field(queryQualityField).asDouble(),
                                                             root.field(writeQualityField).asDouble());
@@ -391,15 +391,6 @@ public class ApplicationSerializer {
         return change;
     }
 
-    // TODO: Remove and inline after 2018-03-01
-    private Change outstandingChangeFromSlime(Inspector object) {
-        if (object.type() == Type.BOOL) {
-            boolean outstandingChange = object.asBool();
-            return outstandingChange ? Change.of(ApplicationVersion.unknown) : Change.empty();
-        }
-        return changeFromSlime(object);
-    }
-
     private List<JobStatus> jobStatusListFromSlime(Inspector array) {
         List<JobStatus> jobStatusList = new ArrayList<>();
         array.traverse((ArrayTraverser) (int i, Inspector item) -> jobStatusList.add(jobStatusFromSlime(item)));
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..fbc935944f5 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
@@ -852,7 +852,8 @@ public class ControllerTest {
 
     }
 
-    private void runDeployment(DeploymentTester tester, ApplicationId application, ApplicationVersion expectedVersion,
+    private void runDeployment(DeploymentTester tester, ApplicationId application,
+                               ApplicationVersion expectedVersion,
                                ApplicationPackage applicationPackage, Optional<SourceRevision> sourceRevision,
                                long initialBuildNumber) {
         Version vespaVersion = Version.fromString("6.1"); // Set in config server mock
@@ -861,10 +862,10 @@ public class ControllerTest {
         // 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());
 
         // Deploy in test
         tester.deployAndNotify(app, applicationPackage, true, systemTest);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java
index 4bed276fcac..3d34e78c759 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java
@@ -2,18 +2,15 @@
 package com.yahoo.vespa.hosted.controller.maintenance;
 
 import com.yahoo.component.Version;
-import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.api.integration.BuildService;
 import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
-import com.yahoo.vespa.hosted.controller.application.SourceRevision;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester;
 import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb;
 import org.junit.Test;
 
 import java.time.Duration;
 import java.util.List;
-import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -37,15 +34,9 @@ public class OutstandingChangeDeployerTest {
         tester.deploymentTrigger().triggerChange(tester.application("app1").id(), Change.of(version));
 
         assertEquals(Change.of(version), tester.application("app1").change());
-        assertFalse(tester.application("app1").outstandingChange().isPresent());
-        tester.notifyJobCompletion(DeploymentJobs.JobType.component, tester.application("app1"),
-                                   Optional.empty(), Optional.of(new SourceRevision("repo", "master",
-                                                                                    "cafed00d")),
-                                   42);
-
-        Application app = tester.application("app1");
-        assertTrue(app.outstandingChange().isPresent());
-        assertEquals("1.0.42-cafed00d", app.outstandingChange().application().get().id());
+        assertFalse(tester.application("app1").hasOutstandingChange());
+        tester.notifyJobCompletion(DeploymentJobs.JobType.component, tester.application("app1"), true);
+        assertTrue(tester.application("app1").hasOutstandingChange());
         assertEquals(1, tester.buildSystem().jobs().size());
 
         deployer.maintain();
@@ -59,7 +50,7 @@ public class OutstandingChangeDeployerTest {
         assertEquals(1, jobs.size());
         assertEquals(11, jobs.get(0).projectId());
         assertEquals(DeploymentJobs.JobType.systemTest.jobName(), jobs.get(0).jobName());
-        assertFalse(tester.application("app1").outstandingChange().isPresent());
+        assertFalse(tester.application("app1").hasOutstandingChange());
     }
 
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java
index 9c19cdb66ac..35bd63745c4 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java
@@ -86,7 +86,7 @@ public class ApplicationSerializerTest {
                                                validationOverrides,
                                                deployments, deploymentJobs,
                                                Change.of(Version.fromString("6.7")),
-                                               Change.of(ApplicationVersion.from(new SourceRevision("repo", "master", "deadcafe"), 42)),
+                                               true,
                                                Optional.of(IssueId.from("1234")),
                                                new MetricsService.ApplicationMetrics(0.5, 0.9),
                                                Optional.of(new RotationId("my-rotation")));
@@ -113,7 +113,7 @@ public class ApplicationSerializerTest {
         assertEquals(  original.deploymentJobs().jobStatus().get(DeploymentJobs.JobType.stagingTest),
                      serialized.deploymentJobs().jobStatus().get(DeploymentJobs.JobType.stagingTest));
 
-        assertEquals(original.outstandingChange(), serialized.outstandingChange());
+        assertEquals(original.hasOutstandingChange(), serialized.hasOutstandingChange());
 
         assertEquals(original.ownershipIssueId(), serialized.ownershipIssueId());
 
@@ -163,14 +163,6 @@ public class ApplicationSerializerTest {
             Application original4 = writable(original).withChange(Change.empty());
             Application serialized4 = applicationSerializer.fromSlime(applicationSerializer.toSlime(original4));
             assertEquals(original4.change(), serialized4.change());
-
-            Application original5 = writable(original).withChange(Change.of(ApplicationVersion.unknown));
-            Application serialized5 = applicationSerializer.fromSlime(applicationSerializer.toSlime(original5));
-            assertEquals(original5.change(), serialized5.change());
-
-            Application original6 = writable(original).withOutstandingChange(Change.of(ApplicationVersion.unknown));
-            Application serialized6 = applicationSerializer.fromSlime(applicationSerializer.toSlime(original6));
-            assertEquals(original6.outstandingChange(), serialized6.outstandingChange());
         }
     }