Merge pull request #17076 from vespa-engine/mpolden/deployment-attempt

17 files changed, 204 insertions, 127 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 51b7a24b5e4..4351ac17001 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
@@ -329,18 +329,24 @@ public class ApplicationController {
         });
     }
 
-    public LockedApplication withNewInstance(LockedApplication application, ApplicationId id) {
-        if (id.instance().isTester())
-            throw new IllegalArgumentException("'" + id + "' is a tester application!");
-        InstanceId.validate(id.instance().value());
+    /** Fetches the requested application package from the artifact store(s). */
+    public ApplicationPackage getApplicationPackage(ApplicationId id, ApplicationVersion version) {
+        return new ApplicationPackage(applicationStore.get(id.tenant(), id.application(), version));
+    }
 
-        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");
+    /** Returns given application with a new instance */
+    public LockedApplication withNewInstance(LockedApplication application, ApplicationId instance) {
+        if (instance.instance().isTester())
+            throw new IllegalArgumentException("'" + instance + "' is a tester application!");
+        InstanceId.validate(instance.instance().value());
 
-        log.info("Created " + id);
-        return application.withNewInstance(id.instance());
+        if (getInstance(instance).isPresent())
+            throw new IllegalArgumentException("Could not create '" + instance + "': Instance already exists");
+        if (getInstance(dashToUnderscore(instance)).isPresent()) // VESPA-1945
+            throw new IllegalArgumentException("Could not create '" + instance + "': Instance " + dashToUnderscore(instance) + " already exists");
+
+        log.info("Created " + instance);
+        return application.withNewInstance(instance.instance());
     }
 
     /** Deploys an application package for an existing application instance. */
@@ -369,14 +375,7 @@ public class ApplicationController {
             try (Lock lock = lock(applicationId)) {
                 LockedApplication application = new LockedApplication(requireApplication(applicationId), lock);
                 Instance instance = application.get().require(job.application().instance());
-
-                Deployment deployment = instance.deployments().get(zone);
-                if (   zone.environment().isProduction() && deployment != null
-                    && (   platform.compareTo(deployment.version()) < 0 && ! instance.change().isPinned()
-                        || revision.compareTo(deployment.applicationVersion()) < 0 && ! (revision.isUnknown() && controller.system().isCd())))
-                    throw new IllegalArgumentException(String.format("Rejecting deployment of application %s to %s, as the requested versions (platform: %s, application: %s)" +
-                                                                     " are older than the currently deployed (platform: %s, application: %s).",
-                                                                     job.application(), zone, platform, revision, deployment.version(), deployment.applicationVersion()));
+                rejectOldChange(instance, platform, revision, job, zone);
 
                 if (   ! applicationPackage.trustedCertificates().isEmpty()
                     &&   run.testerCertificate().isPresent())
@@ -403,21 +402,6 @@ public class ApplicationController {
         }
     }
 
-    private QuotaUsage deploymentQuotaUsage(ZoneId zoneId, ApplicationId applicationId) {
-        var application = configServer.nodeRepository().getApplication(zoneId, applicationId);
-        return DeploymentQuotaCalculator.calculateQuotaUsage(application);
-    }
-
-    private ApplicationPackage getApplicationPackage(ApplicationId application, ZoneId zone, ApplicationVersion revision) {
-        return new ApplicationPackage(revision.isUnknown() ? applicationStore.getDev(application, zone)
-                                                           : applicationStore.get(application.tenant(), application.application(), revision));
-    }
-
-    /** Fetches the requested application package from the artifact store(s). */
-    public ApplicationPackage getApplicationPackage(ApplicationId id, ApplicationVersion version) {
-        return new ApplicationPackage(applicationStore.get(id.tenant(), id.application(), version));
-    }
-
     /** Stores the deployment spec and validation overrides from the application package, and runs cleanup. */
     public LockedApplication storeWithUpdatedConfig(LockedApplication application, ApplicationPackage applicationPackage) {
         applicationPackageValidator.validate(application.get(), applicationPackage, clock.instant());
@@ -683,6 +667,11 @@ public class ApplicationController {
         }
     }
 
+    /** Sets suspension status of the given deployment in its zone. */
+    public void setSuspension(DeploymentId deploymentId, boolean suspend) {
+        configServer.setSuspension(deploymentId, suspend);
+    }
+
     /** Deactivate application in the given zone */
     public void deactivate(ApplicationId id, ZoneId zone) {
         lockApplicationOrThrow(TenantAndApplicationId.from(id),
@@ -710,14 +699,6 @@ public class ApplicationController {
 
     public DeploymentTrigger deploymentTrigger() { return deploymentTrigger; }
 
-    private TenantAndApplicationId dashToUnderscore(TenantAndApplicationId id) {
-        return TenantAndApplicationId.from(id.tenant().value(), id.application().value().replaceAll("-", "_"));
-    }
-
-    private ApplicationId dashToUnderscore(ApplicationId id) {
-        return dashToUnderscore(TenantAndApplicationId.from(id)).instance(id.instance());
-    }
-
     /**
      * Returns a lock which provides exclusive rights to changing this application.
      * Any operation which stores an application need to first acquire this lock, then read, modify
@@ -798,6 +779,38 @@ public class ApplicationController {
         }
     }
 
+    private void rejectOldChange(Instance instance, Version platform, ApplicationVersion revision, JobId job, ZoneId zone) {
+        Deployment deployment = instance.deployments().get(zone);
+        if (deployment == null) return;
+        if (!zone.environment().isProduction()) return;
+
+        boolean platformIsOlder = platform.compareTo(deployment.version()) < 0 && !instance.change().isPinned();
+        boolean revisionIsOlder = revision.compareTo(deployment.applicationVersion()) < 0 &&
+                                  !(revision.isUnknown() && controller.system().isCd());
+        if (platformIsOlder || revisionIsOlder)
+            throw new IllegalArgumentException(String.format("Rejecting deployment of application %s to %s, as the requested versions (platform: %s, application: %s)" +
+                                                             " are older than the currently deployed (platform: %s, application: %s).",
+                                                             job.application(), zone, platform, revision, deployment.version(), deployment.applicationVersion()));
+    }
+
+    private TenantAndApplicationId dashToUnderscore(TenantAndApplicationId id) {
+        return TenantAndApplicationId.from(id.tenant().value(), id.application().value().replaceAll("-", "_"));
+    }
+
+    private ApplicationId dashToUnderscore(ApplicationId id) {
+        return dashToUnderscore(TenantAndApplicationId.from(id)).instance(id.instance());
+    }
+
+    private QuotaUsage deploymentQuotaUsage(ZoneId zoneId, ApplicationId applicationId) {
+        var application = configServer.nodeRepository().getApplication(zoneId, applicationId);
+        return DeploymentQuotaCalculator.calculateQuotaUsage(application);
+    }
+
+    private ApplicationPackage getApplicationPackage(ApplicationId application, ZoneId zone, ApplicationVersion revision) {
+        return new ApplicationPackage(revision.isUnknown() ? applicationStore.getDev(application, zone)
+                                              : applicationStore.get(application.tenant(), application.application(), revision));
+    }
+
     /*
      * Get the AthenzUser from this principal or Optional.empty if this does not represent a user.
      */
@@ -855,9 +868,4 @@ public class ApplicationController {
         return Map.copyOf(warnings);
     }
 
-    /** Sets suspension status of the given deployment in its zone. */
-    public void setSuspension(DeploymentId deploymentId, boolean suspend) {
-        configServer.setSuspension(deploymentId, suspend);
-    }
-
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Instance.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Instance.java
index fc124947e5d..025b785a693 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Instance.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Instance.java
@@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.application.AssignedRotation;
 import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
+import com.yahoo.vespa.hosted.controller.application.DeploymentActivity;
 import com.yahoo.vespa.hosted.controller.application.DeploymentMetrics;
 import com.yahoo.vespa.hosted.controller.application.QuotaUsage;
 import com.yahoo.vespa.hosted.controller.rotation.RotationStatus;
@@ -66,7 +67,10 @@ public class Instance {
                                       Instant instant, Map<DeploymentMetrics.Warning, Integer> warnings, QuotaUsage quotaUsage) {
         // Use info from previous deployment if available, otherwise create a new one.
         Deployment previousDeployment = deployments.getOrDefault(zone, new Deployment(zone, applicationVersion,
-                                                                                      version, instant));
+                                                                                      version, instant,
+                                                                                      DeploymentMetrics.none,
+                                                                                      DeploymentActivity.none,
+                                                                                      QuotaUsage.none));
         Deployment newDeployment = new Deployment(zone, applicationVersion, version, instant,
                                                   previousDeployment.metrics().with(warnings),
                                                   previousDeployment.activity(),
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Deployment.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Deployment.java
index 800680d7327..3d17a7f8681 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Deployment.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Deployment.java
@@ -24,10 +24,6 @@ public class Deployment {
     private final DeploymentActivity activity;
     private final QuotaUsage quota;
 
-    public Deployment(ZoneId zone, ApplicationVersion applicationVersion, Version version, Instant deployTime) {
-        this(zone, applicationVersion, version, deployTime, DeploymentMetrics.none, DeploymentActivity.none, QuotaUsage.none);
-    }
-
     public Deployment(ZoneId zone, ApplicationVersion applicationVersion, Version version, Instant deployTime,
                       DeploymentMetrics metrics,  DeploymentActivity activity, QuotaUsage quota) {
         this.zone = Objects.requireNonNull(zone, "zone cannot be null");
@@ -76,6 +72,25 @@ public class Deployment {
     }
 
     @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        Deployment that = (Deployment) o;
+        return zone.equals(that.zone) &&
+               applicationVersion.equals(that.applicationVersion) &&
+               version.equals(that.version) &&
+               deployTime.equals(that.deployTime) &&
+               metrics.equals(that.metrics) &&
+               activity.equals(that.activity) &&
+               quota.equals(that.quota);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(zone, applicationVersion, version, deployTime, metrics, activity, quota);
+    }
+
+    @Override
     public String toString() {
         return "deployment to " + zone + " of " + applicationVersion + " on version " + version + " at " + deployTime;
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentActivity.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentActivity.java
index 03c08509a5e..71f0d64c43a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentActivity.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentActivity.java
@@ -61,6 +61,19 @@ public class DeploymentActivity {
                                       activeRate(metrics.writesPerSecond(), lastWritesPerSecond));
     }
 
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        DeploymentActivity that = (DeploymentActivity) o;
+        return lastQueried.equals(that.lastQueried) && lastWritten.equals(that.lastWritten) && lastQueriesPerSecond.equals(that.lastQueriesPerSecond) && lastWritesPerSecond.equals(that.lastWritesPerSecond);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(lastQueried, lastWritten, lastQueriesPerSecond, lastWritesPerSecond);
+    }
+
     public static DeploymentActivity create(Optional<Instant> queriedAt, Optional<Instant> writtenAt,
                                             OptionalDouble lastQueriesPerSecond, OptionalDouble lastWritesPerSecond) {
         if (queriedAt.isEmpty() && writtenAt.isEmpty()) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentMetrics.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentMetrics.java
index 7a50184e7a4..094cb9a19b0 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentMetrics.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentMetrics.java
@@ -111,6 +111,25 @@ public class DeploymentMetrics {
                                      writeLatencyMills, instant, warnings);
     }
 
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        DeploymentMetrics that = (DeploymentMetrics) o;
+        return Double.compare(that.queriesPerSecond, queriesPerSecond) == 0 &&
+               Double.compare(that.writesPerSecond, writesPerSecond) == 0 &&
+               Double.compare(that.documentCount, documentCount) == 0 &&
+               Double.compare(that.queryLatencyMillis, queryLatencyMillis) == 0 &&
+               Double.compare(that.writeLatencyMills, writeLatencyMills) == 0 &&
+               instant.equals(that.instant) &&
+               warnings.equals(that.warnings);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(queriesPerSecond, writesPerSecond, documentCount, queryLatencyMillis, writeLatencyMills, instant, warnings);
+    }
+
     /** Types of deployment warnings. We currently have only one */
     public enum Warning {
         all
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/QuotaUsage.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/QuotaUsage.java
index 13384b63c84..1e070d5a66b 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/QuotaUsage.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/QuotaUsage.java
@@ -1,12 +1,14 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.application;
 
+import java.util.Objects;
 import java.util.OptionalDouble;
 
 /**
  * @author ogronnesby
  */
 public class QuotaUsage {
+
     public static final QuotaUsage none = new QuotaUsage(0.0);
 
     private final double rate;
@@ -39,6 +41,19 @@ public class QuotaUsage {
     }
 
     @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        QuotaUsage that = (QuotaUsage) o;
+        return Double.compare(that.rate, rate) == 0;
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(rate);
+    }
+
+    @Override
     public String toString() {
         return "QuotaUsage{" +
                 "rate=" + rate +
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java
index 37de7369452..e5316788802 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java
@@ -1,13 +1,18 @@
 // 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.maintenance;
 
+import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.Instance;
+import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobId;
+import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
+import com.yahoo.vespa.hosted.controller.deployment.Run;
 import com.yahoo.yolean.Exceptions;
 
 import java.time.Duration;
+import java.util.Optional;
 import java.util.logging.Level;
 
 /**
@@ -28,7 +33,7 @@ public class DeploymentExpirer extends ControllerMaintainer {
         for (Application application : controller().applications().readable()) {
             for (Instance instance : application.instances().values())
                 for (Deployment deployment : instance.deployments().values()) {
-                    if (!isExpired(deployment)) continue;
+                    if (!isExpired(deployment, instance.id())) continue;
 
                     try {
                         log.log(Level.INFO, "Expiring deployment of " + instance.id() + " in " + deployment.zone());
@@ -45,10 +50,19 @@ public class DeploymentExpirer extends ControllerMaintainer {
     }
 
     /** Returns whether given deployment has expired according to its TTL */
-    private boolean isExpired(Deployment deployment) {
+    private boolean isExpired(Deployment deployment, ApplicationId instance) {
         if (deployment.zone().environment().isProduction()) return false; // Never expire production deployments
-        return controller().zoneRegistry().getDeploymentTimeToLive(deployment.zone())
-                           .map(timeToLive -> deployment.at().plus(timeToLive).isBefore(controller().clock().instant()))
+
+        Optional<Duration> ttl = controller().zoneRegistry().getDeploymentTimeToLive(deployment.zone());
+        if (ttl.isEmpty()) return false;
+
+        Optional<JobId> jobId = JobType.from(controller().system(), deployment.zone())
+                                       .map(type -> new JobId(instance, type));
+        if (jobId.isEmpty()) return false;
+
+        return controller().jobController().last(jobId.get())
+                           .flatMap(Run::end)
+                           .map(end -> end.plus(ttl.get()).isBefore(controller().clock().instant()))
                            .orElse(false);
     }
 
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 52fbe26bd7c..24b553e5153 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
@@ -1,10 +1,6 @@
 // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.persistence;
 
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
-import com.google.common.hash.Hashing;
-import com.google.common.util.concurrent.UncheckedExecutionException;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.application.api.ValidationOverrides;
@@ -53,7 +49,6 @@ import java.util.Optional;
 import java.util.OptionalInt;
 import java.util.OptionalLong;
 import java.util.Set;
-import java.util.concurrent.ExecutionException;
 
 /**
  * Serializes {@link Application}s to/from slime.
@@ -297,7 +292,7 @@ public class ApplicationSerializer {
         Inspector root = slime.get();
 
         TenantAndApplicationId id = TenantAndApplicationId.fromSerialized(root.field(idField).asString());
-        Instant createdAt = Instant.ofEpochMilli(root.field(createdAtField).asLong());
+        Instant createdAt = Serializers.instant(root.field(createdAtField));
         DeploymentSpec deploymentSpec = DeploymentSpec.fromXml(root.field(deploymentSpecField).asString(), false);
         ValidationOverrides validationOverrides = ValidationOverrides.fromXml(root.field(validationOverridesField).asString());
         Optional<IssueId> deploymentIssueId = Serializers.optionalString(root.field(deploymentIssueField)).map(IssueId::from);
@@ -356,7 +351,7 @@ public class ApplicationSerializer {
         return new Deployment(zoneIdFromSlime(deploymentObject.field(zoneField)),
                               applicationVersionFromSlime(deploymentObject.field(applicationPackageRevisionField)),
                               Version.fromString(deploymentObject.field(versionField).asString()),
-                              Instant.ofEpochMilli(deploymentObject.field(deployTimeField).asLong()),
+                              Serializers.instant(deploymentObject.field(deployTimeField)),
                               deploymentMetricsFromSlime(deploymentObject.field(deploymentMetricsField)),
                               DeploymentActivity.create(Serializers.optionalInstant(deploymentObject.field(lastQueriedField)),
                                                         Serializers.optionalInstant(deploymentObject.field(lastWrittenField)),
@@ -366,9 +361,7 @@ public class ApplicationSerializer {
     }
 
     private DeploymentMetrics deploymentMetricsFromSlime(Inspector object) {
-        Optional<Instant> instant = object.field(deploymentMetricsUpdateTime).valid() ?
-                Optional.of(Instant.ofEpochMilli(object.field(deploymentMetricsUpdateTime).asLong())) :
-                Optional.empty();
+        Optional<Instant> instant = Serializers.optionalInstant(object.field(deploymentMetricsUpdateTime));
         return new DeploymentMetrics(object.field(deploymentMetricsQPSField).asDouble(),
                                      object.field(deploymentMetricsWPSField).asDouble(),
                                      object.field(deploymentMetricsDocsField).asDouble(),
@@ -391,7 +384,7 @@ public class ApplicationSerializer {
         object.traverse((ArrayTraverser) (idx, statusObject) -> statusMap.put(new RotationId(statusObject.field(rotationIdField).asString()),
                                                                               new RotationStatus.Targets(
                                                                                       singleRotationStatusFromSlime(statusObject.field(statusField)),
-                                                                                      Instant.ofEpochMilli(statusObject.field(lastUpdatedField).asLong()))));
+                                                                                      Serializers.instant(statusObject.field(lastUpdatedField)))));
         return RotationStatus.from(statusMap);
     }
 
@@ -440,7 +433,7 @@ public class ApplicationSerializer {
         object.field(jobStatusField).traverse((ArrayTraverser) (__, jobPauseObject) ->
                 JobType.fromOptionalJobName(jobPauseObject.field(jobTypeField).asString())
                        .ifPresent(jobType -> jobPauses.put(jobType,
-                                                           Instant.ofEpochMilli(jobPauseObject.field(pausedUntilField).asLong()))));
+                                                           Serializers.instant(jobPauseObject.field(pausedUntilField)))));
         return jobPauses;
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java
index b411f460568..7ea722bf5de 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/AuditLogSerializer.java
@@ -7,7 +7,6 @@ import com.yahoo.slime.Inspector;
 import com.yahoo.slime.Slime;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLog;
 
-import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -52,7 +51,7 @@ public class AuditLogSerializer {
         Cursor root = slime.get();
         root.field(entriesField).traverse((ArrayTraverser) (i, entryObject) -> {
             entries.add(new AuditLog.Entry(
-                    Instant.ofEpochMilli(entryObject.field(atField).asLong()),
+                    Serializers.instant(entryObject.field(atField)),
                     entryObject.field(principalField).asString(),
                     methodFrom(entryObject.field(methodField)),
                     entryObject.field(resourceField).asString(),
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerVersionSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerVersionSerializer.java
index 0e8b6087901..30fcc0e40c6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerVersionSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerVersionSerializer.java
@@ -5,8 +5,6 @@ import com.yahoo.component.Version;
 import com.yahoo.slime.Slime;
 import com.yahoo.vespa.hosted.controller.versions.ControllerVersion;
 
-import java.time.Instant;
-
 /**
  * Serializer for {@link com.yahoo.vespa.hosted.controller.versions.ControllerVersion}.
  *
@@ -38,7 +36,7 @@ public class ControllerVersionSerializer {
         var root = slime.get();
         var version = Version.fromString(root.field(VERSION_FIELD).asString());
         var commitSha = root.field(COMMIT_SHA_FIELD).asString();
-        var commitDate = Instant.ofEpochMilli(root.field(COMMIT_DATE_FIELD).asLong());
+        var commitDate = Serializers.instant(root.field(COMMIT_DATE_FIELD));
         return new ControllerVersion(version, commitSha, commitDate);
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/LogSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/LogSerializer.java
index fffe781e6e1..6416d077ce4 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/LogSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/LogSerializer.java
@@ -13,7 +13,6 @@ import com.yahoo.vespa.hosted.controller.deployment.Step;
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
-import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
@@ -93,7 +92,7 @@ class LogSerializer {
 
     private LogEntry fromSlime(Inspector entryObject) {
         return new LogEntry(entryObject.field(idField).asLong(),
-                            Instant.ofEpochMilli(entryObject.field(timestampField).asLong()),
+                            Serializers.instant(entryObject.field(timestampField)),
                             typeOf(entryObject.field(typeField).asString()),
                             entryObject.field(messageField).asString());
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java
index 1aa229984a8..0ecd86a4a38 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java
@@ -121,9 +121,7 @@ class RunSerializer {
             // For historical reasons are the step details stored in a separate JSON structure from the step statuses.
             Inspector stepDetailsField = detailsField.field(step);
             Inspector startTimeValue = stepDetailsField.field(startTimeField);
-            Optional<Instant> startTime = startTimeValue.valid() ?
-                    Optional.of(instantOf(startTimeValue.asLong())) :
-                    Optional.empty();
+            Optional<Instant> startTime = Serializers.optionalInstant(startTimeValue);
 
             steps.put(typedStep, new StepInfo(typedStep, stepStatusOf(status.asString()), startTime));
         });
@@ -132,7 +130,7 @@ class RunSerializer {
                                  runObject.field(numberField).asLong()),
                        steps,
                        versionsFromSlime(runObject.field(versionsField)),
-                       Instant.ofEpochMilli(runObject.field(startField).asLong()),
+                       Serializers.instant(runObject.field(startField)),
                        Serializers.optionalInstant(runObject.field(endField)),
                        runStatusOf(runObject.field(statusField).asString()),
                        runObject.field(lastTestRecordField).asLong(),
@@ -259,7 +257,7 @@ class RunSerializer {
         applicationVersion.commit().ifPresent(commit -> versionsObject.setString(commitField, commit));
     }
 
-    // Don't change this — introduce a separate array with new values if needed.
+    // Don't change this - introduce a separate array with new values if needed.
     private void toSlime(ConvergenceSummary summary, Cursor summaryArray) {
         summaryArray.addLong(summary.nodes());
         summaryArray.addLong(summary.down());
@@ -341,10 +339,6 @@ class RunSerializer {
         return instant.toEpochMilli();
     }
 
-    static Instant instantOf(Long epochMillis) {
-        return Instant.ofEpochMilli(epochMillis);
-    }
-
     static String valueOf(RunStatus status) {
         switch (status) {
             case running                    : return "running";
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/Serializers.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/Serializers.java
index b254732f324..7c8a09e244e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/Serializers.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/Serializers.java
@@ -20,6 +20,10 @@ public class Serializers {
 
     private Serializers() {}
 
+    public static Instant instant(Inspector field) {
+        return Instant.ofEpochMilli(field.asLong());
+    }
+
     public static OptionalLong optionalLong(Inspector field) {
         return field.valid() ? OptionalLong.of(field.asLong()) : OptionalLong.empty();
     }
@@ -37,13 +41,11 @@ public class Serializers {
     }
 
     public static Optional<Instant> optionalInstant(Inspector field) {
-        var value = optionalLong(field);
-        return value.isPresent() ? Optional.of(Instant.ofEpochMilli(value.getAsLong())) : Optional.empty();
+        return optionalLong(field).stream().mapToObj(Instant::ofEpochMilli).findFirst();
     }
 
     public static Optional<Duration> optionalDuration(Inspector field) {
-        var value = optionalLong(field);
-        return value.isPresent() ? Optional.of(Duration.ofMillis(value.getAsLong())) : Optional.empty();
+        return optionalLong(field).stream().mapToObj(Duration::ofMillis).findFirst();
     }
 
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializer.java
index 8cb87b8a72d..8e97368624d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializer.java
@@ -151,14 +151,14 @@ public class TenantSerializer {
         Property property = new Property(tenantObject.field(propertyField).asString());
         Optional<PropertyId> propertyId = SlimeUtils.optionalString(tenantObject.field(propertyIdField)).map(PropertyId::new);
         Optional<Contact> contact = contactFrom(tenantObject.field(contactField));
-        Instant createdAt = Instant.ofEpochMilli(tenantObject.field(createdAtField).asLong());
+        Instant createdAt = Serializers.instant(tenantObject.field(createdAtField));
         LastLoginInfo lastLoginInfo = lastLoginInfoFromSlime(tenantObject.field(lastLoginInfoField));
         return new AthenzTenant(name, domain, property, propertyId, contact, createdAt, lastLoginInfo);
     }
 
     private CloudTenant cloudTenantFrom(Inspector tenantObject) {
         TenantName name = TenantName.from(tenantObject.field(nameField).asString());
-        Instant createdAt = Instant.ofEpochMilli(tenantObject.field(createdAtField).asLong());
+        Instant createdAt = Serializers.instant(tenantObject.field(createdAtField));
         LastLoginInfo lastLoginInfo = lastLoginInfoFromSlime(tenantObject.field(lastLoginInfoField));
         Optional<Principal> creator = SlimeUtils.optionalString(tenantObject.field(creatorField)).map(SimplePrincipal::new);
         BiMap<PublicKey, Principal> developerKeys = developerKeysFromSlime(tenantObject.field(pemDeveloperKeysField));
@@ -227,7 +227,7 @@ public class TenantSerializer {
     private LastLoginInfo lastLoginInfoFromSlime(Inspector lastLoginInfoObject) {
         Map<LastLoginInfo.UserLevel, Instant> lastLoginByUserLevel = new HashMap<>();
         lastLoginInfoObject.traverse((String name, Inspector value) ->
-                lastLoginByUserLevel.put(userLevelOf(name), Instant.ofEpochMilli(value.asLong())));
+                lastLoginByUserLevel.put(userLevelOf(name), Serializers.instant(value)));
         return new LastLoginInfo(lastLoginByUserLevel);
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/VersionStatusSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/VersionStatusSerializer.java
index 6eb5b8fadcd..12d15aa7cdd 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/VersionStatusSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/VersionStatusSerializer.java
@@ -7,19 +7,14 @@ import com.yahoo.slime.ArrayTraverser;
 import com.yahoo.slime.Cursor;
 import com.yahoo.slime.Inspector;
 import com.yahoo.slime.Slime;
-import com.yahoo.vespa.hosted.controller.deployment.Run;
-import com.yahoo.vespa.hosted.controller.versions.DeploymentStatistics;
 import com.yahoo.vespa.hosted.controller.versions.NodeVersions;
 import com.yahoo.vespa.hosted.controller.versions.VersionStatus;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
 
-import java.time.Instant;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
-import java.util.stream.Collectors;
 
 /**
  * Serializer for {@link VersionStatus}.
@@ -111,7 +106,7 @@ public class VersionStatusSerializer {
         var version = Version.fromString(object.field(deploymentStatisticsField).field(versionField).asString());
         return new VespaVersion(version,
                                 object.field(releaseCommitField).asString(),
-                                Instant.ofEpochMilli(object.field(committedAtField).asLong()),
+                                Serializers.instant(object.field(committedAtField)),
                                 object.field(isControllerVersionField).asBool(),
                                 object.field(isSystemVersionField).asBool(),
                                 object.field(isReleasedField).asBool(),
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirerTest.java
index d718dc6b9cf..232521c9609 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirerTest.java
@@ -10,13 +10,15 @@ import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
 import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester;
+import com.yahoo.vespa.hosted.controller.deployment.Run;
+import com.yahoo.vespa.hosted.controller.deployment.RunStatus;
 import org.junit.Test;
 
 import java.time.Duration;
-import java.util.List;
-import java.util.stream.Collectors;
+import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
 
 /**
  * @author bratseth
@@ -27,12 +29,9 @@ public class DeploymentExpirerTest {
 
     @Test
     public void testDeploymentExpiry() {
-        tester.controllerTester().zoneRegistry().setDeploymentTimeToLive(
-                ZoneId.from(Environment.dev, RegionName.from("us-east-1")),
-                Duration.ofDays(14)
-        );
-        DeploymentExpirer expirer = new DeploymentExpirer(tester.controller(), Duration.ofDays(10)
-        );
+        ZoneId devZone = ZoneId.from(Environment.dev, RegionName.from("us-east-1"));
+        tester.controllerTester().zoneRegistry().setDeploymentTimeToLive(devZone, Duration.ofDays(14));
+        DeploymentExpirer expirer = new DeploymentExpirer(tester.controller(), Duration.ofDays(1));
         var devApp = tester.newDeploymentContext("tenant1", "app1", "default");
         var prodApp = tester.newDeploymentContext("tenant2", "app2", "default");
 
@@ -45,27 +44,42 @@ public class DeploymentExpirerTest {
 
         // Deploy prod
         prodApp.submit(appPackage).deploy();
-
-        assertEquals(1, permanentDeployments(devApp.instance()).size());
-        assertEquals(1, permanentDeployments(prodApp.instance()).size());
+        assertEquals(1, permanentDeployments(devApp.instance()));
+        assertEquals(1, permanentDeployments(prodApp.instance()));
 
         // Not expired at first
         expirer.maintain();
-        assertEquals(1, permanentDeployments(devApp.instance()).size());
-        assertEquals(1, permanentDeployments(prodApp.instance()).size());
+        assertEquals(1, permanentDeployments(devApp.instance()));
+        assertEquals(1, permanentDeployments(prodApp.instance()));
+
+        // Deploy dev unsuccessfully a few days before expiry
+        tester.clock().advance(Duration.ofDays(12));
+        tester.configServer().throwOnNextPrepare(new RuntimeException(getClass().getSimpleName()));
+        tester.jobs().deploy(devApp.instanceId(), JobType.devUsEast1, Optional.empty(), appPackage);
+        Run lastRun = tester.jobs().last(devApp.instanceId(), JobType.devUsEast1).get();
+        assertSame(RunStatus.error, lastRun.status());
+        Deployment deployment = tester.applications().requireInstance(devApp.instanceId())
+                                      .deployments().get(devZone);
+        assertEquals("Time of last run is after time of deployment", Duration.ofDays(12),
+                     Duration.between(deployment.at(), lastRun.end().get()));
+
+        // Dev application does not expire based on time of successful deployment
+        tester.clock().advance(Duration.ofDays(2));
+        expirer.maintain();
+        assertEquals(1, permanentDeployments(devApp.instance()));
+        assertEquals(1, permanentDeployments(prodApp.instance()));
 
-        // The dev application is removed
-        tester.clock().advance(Duration.ofDays(15));
+        // Dev application expires when enough time has passed since most recent attempt
+        tester.clock().advance(Duration.ofDays(12));
         expirer.maintain();
-        assertEquals(0, permanentDeployments(devApp.instance()).size());
-        assertEquals(1, permanentDeployments(prodApp.instance()).size());
+        assertEquals(0, permanentDeployments(devApp.instance()));
+        assertEquals(1, permanentDeployments(prodApp.instance()));
     }
 
-    private List<Deployment> permanentDeployments(Instance instance) {
-        return tester.controller().applications().getInstance(instance.id()).get().deployments().values().stream()
-                     .filter(deployment -> deployment.zone().environment() != Environment.test &&
-                                      deployment.zone().environment() != Environment.staging)
-                     .collect(Collectors.toList());
+    private long permanentDeployments(Instance instance) {
+        return tester.controller().applications().requireInstance(instance.id()).deployments().values().stream()
+                     .filter(deployment -> !deployment.zone().environment().isTest())
+                     .count();
     }
 
 }
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 16a17d96c03..2dcf012ac6d 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
@@ -95,7 +95,8 @@ public class ApplicationSerializerTest {
                 .from(new SourceRevision("repo1", "branch1", "commit1"), 32, "a@b",
                       Version.fromString("6.3.1"), Instant.ofEpochMilli(496));
         Instant activityAt = Instant.parse("2018-06-01T10:15:30.00Z");
-        deployments.add(new Deployment(zone1, applicationVersion1, Version.fromString("1.2.3"), Instant.ofEpochMilli(3))); // One deployment without cluster info and utils
+        deployments.add(new Deployment(zone1, applicationVersion1, Version.fromString("1.2.3"), Instant.ofEpochMilli(3),
+                                       DeploymentMetrics.none, DeploymentActivity.none, QuotaUsage.none));
         deployments.add(new Deployment(zone2, applicationVersion2, Version.fromString("1.2.3"), Instant.ofEpochMilli(5),
                                        new DeploymentMetrics(2, 3, 4, 5, 6,
                                                              Optional.of(Instant.now().truncatedTo(ChronoUnit.MILLIS)),
@@ -160,14 +161,8 @@ public class ApplicationSerializerTest {
         assertEquals(RotationStatus.EMPTY, serialized.require(id3.instance()).rotationStatus());
 
         assertEquals(2, serialized.require(id1.instance()).deployments().size());
-        assertEquals(original.require(id1.instance()).deployments().get(zone1).applicationVersion(), serialized.require(id1.instance()).deployments().get(zone1).applicationVersion());
-        assertEquals(original.require(id1.instance()).deployments().get(zone2).applicationVersion(), serialized.require(id1.instance()).deployments().get(zone2).applicationVersion());
-        assertEquals(original.require(id1.instance()).deployments().get(zone1).version(), serialized.require(id1.instance()).deployments().get(zone1).version());
-        assertEquals(original.require(id1.instance()).deployments().get(zone2).version(), serialized.require(id1.instance()).deployments().get(zone2).version());
-        assertEquals(original.require(id1.instance()).deployments().get(zone1).at(), serialized.require(id1.instance()).deployments().get(zone1).at());
-        assertEquals(original.require(id1.instance()).deployments().get(zone2).at(), serialized.require(id1.instance()).deployments().get(zone2).at());
-        assertEquals(original.require(id1.instance()).deployments().get(zone2).activity().lastQueried().get(), serialized.require(id1.instance()).deployments().get(zone2).activity().lastQueried().get());
-        assertEquals(original.require(id1.instance()).deployments().get(zone2).activity().lastWritten().get(), serialized.require(id1.instance()).deployments().get(zone2).activity().lastWritten().get());
+        assertEquals(original.require(id1.instance()).deployments().get(zone1), serialized.require(id1.instance()).deployments().get(zone1));
+        assertEquals(original.require(id1.instance()).deployments().get(zone2), serialized.require(id1.instance()).deployments().get(zone2));
 
         assertEquals(original.require(id1.instance()).jobPause(JobType.systemTest),
                      serialized.require(id1.instance()).jobPause(JobType.systemTest));