Hide sub-obejcts that aren't leaves, to reduce risk of coding errors'

7 files changed, 89 insertions, 75 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 449558e4054..9d26a52286f 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
@@ -9,6 +9,7 @@ import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId;
+import com.yahoo.vespa.hosted.controller.application.ApplicationRevision;
 import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.Change.VersionChange;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
@@ -143,6 +144,19 @@ public class Application {
                 .orElse(oldestDeployedVersion().orElse(controller.systemVersion()));
     }
 
+    /** Returns the revision a new deployment to this zone should use for this application, or empty if we don't know */
+    public Optional<ApplicationRevision> deployRevisionIn(Zone zone) {
+        if (deploying().isPresent() && deploying().get() instanceof Change.ApplicationChange)
+            return ((Change.ApplicationChange) deploying().get()).revision();
+
+        return revisionIn(zone);
+    }
+
+    /** Returns the revision this application is or should be deployed with in the given zone, or empty if unknown. */
+    public Optional<ApplicationRevision> revisionIn(Zone zone) {
+        return Optional.ofNullable(deployments().get(zone)).map(Deployment::revision);
+    }
+
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
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 5c6cea43e76..a1face4ecdc 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
@@ -319,13 +319,12 @@ public class ApplicationController {
                     // * When someone else triggered the job, we need to store a stand-in triggering event.
                     // * When this is the system test job, we need to record the new revision, for future use.
                     JobStatus.JobRun triggering = getOrCreateTriggering(application, version, jobType.get());
-                    application = application.with(application.deploymentJobs()
-                                                           .withTriggering(jobType.get(),
-                                                                           application.deploying(),
-                                                                           version,
-                                                                           Optional.of(revision),
-                                                                           triggering.reason(),
-                                                                           triggering.at()));
+                    application = application.withJobTriggering(jobType.get(),
+                                                                application.deploying(),
+                                                                triggering.at(),
+                                                                version,
+                                                                Optional.of(revision),
+                                                                triggering.reason());
                 }
 
                 // Delete zones not listed in DeploymentSpec, if allowed
@@ -358,14 +357,8 @@ public class ApplicationController {
                     configserverClient.prepare(deploymentId, options, rotationInDns.cnames(), rotationInDns.rotations(), 
                                                applicationPackage.zippedContent());
             preparedApplication.activate();
+            application = application.withNewDeployment(zone, revision, version, clock.instant());
 
-            // Use info from previous deployments is available
-            Deployment previousDeployment = application.deployments().getOrDefault(zone, new Deployment(zone, revision, version, clock.instant()));
-            Deployment newDeployment = new Deployment(zone, revision, version, clock.instant(),
-                                                      previousDeployment.clusterUtils(), 
-                                                      previousDeployment.clusterInfo(), previousDeployment.metrics());
-
-            application = application.with(newDeployment);
             store(application);
 
             return new ActivateResult(new RevisionId(applicationPackage.hash()), preparedApplication.prepareResponse());
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 3ef904ebf85..c5cf5ef96c8 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
@@ -3,15 +3,18 @@ package com.yahoo.vespa.hosted.controller;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.application.api.ValidationOverrides;
+import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId;
 import com.yahoo.vespa.hosted.controller.application.ApplicationRevision;
 import com.yahoo.vespa.hosted.controller.application.Change;
-import com.yahoo.vespa.hosted.controller.application.Change.ApplicationChange;
+import com.yahoo.vespa.hosted.controller.application.ClusterInfo;
+import com.yahoo.vespa.hosted.controller.application.ClusterUtilization;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType;
+import com.yahoo.vespa.hosted.controller.application.DeploymentMetrics;
 
 import java.time.Instant;
 import java.util.LinkedHashMap;
@@ -63,19 +66,20 @@ public class LockedApplication extends Application {
                                                      deploying(), hasOutstandingChange(), ownershipIssueId()), lock);
     }
 
-    public LockedApplication withJobTriggering(DeploymentJobs.JobType type, Optional<Change> change,
-                                               String reason, Instant triggerTime, Controller controller) {
+    public LockedApplication withJobTriggering(JobType type, Optional<Change> change,
+                                               Instant triggerTime, Version version, Optional<ApplicationRevision> revision, String reason) {
         return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), deployments(),
                                                      deploymentJobs().withTriggering(type,
                                                                                      change,
-                                                                                     deployVersionFor(type, controller),
-                                                                                     deployRevisionFor(type, controller),
+                                                                                     version,
+                                                                                     revision,
                                                                                      reason,
                                                                                      triggerTime),
                                                      deploying(), hasOutstandingChange(), ownershipIssueId()), lock);
     }
 
-    public LockedApplication with(Deployment deployment) {
+    /** Don't expose non-leaf sub-objects. */
+    private LockedApplication with(Deployment deployment) {
         Map<Zone, Deployment> deployments = new LinkedHashMap<>(deployments());
         deployments.put(deployment.zone(), deployment);
         return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(),
@@ -83,10 +87,33 @@ public class LockedApplication extends Application {
                                                      hasOutstandingChange(), ownershipIssueId()), lock);
     }
 
-    public LockedApplication with(DeploymentJobs deploymentJobs) {
-        return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(),
-                                                     deployments(), deploymentJobs, deploying(),
-                                                     hasOutstandingChange(), ownershipIssueId()), lock);
+    public LockedApplication withNewDeployment(Zone zone, ApplicationRevision revision, Version version, Instant instant) {
+        // Use info from previous deployments is available
+        Deployment previousDeployment = deployments().getOrDefault(zone, new Deployment(zone, revision, version, instant));
+        Deployment newDeployment = new Deployment(zone, revision, version, instant,
+                                                  previousDeployment.clusterUtils(),
+                                                  previousDeployment.clusterInfo(),
+                                                  previousDeployment.metrics());
+        return with(newDeployment);
+    }
+
+    public LockedApplication withClusterUtilization(Zone zone, Map<ClusterSpec.Id, ClusterUtilization> clusterUtilization) {
+        Deployment deployment = deployments().get(zone);
+        if (deployment == null) return this;    // No longer deployed in this zone.
+        return with(deployment.withClusterUtils(clusterUtilization));
+    }
+
+    public LockedApplication withClusterInfo(Zone zone, Map<ClusterSpec.Id, ClusterInfo> clusterInfo) {
+        Deployment deployment = deployments().get(zone);
+        if (deployment == null) return this;    // No longer deployed in this zone.
+        return with(deployment.withClusterInfo(clusterInfo));
+
+    }
+
+    public LockedApplication with(Zone zone, DeploymentMetrics deploymentMetrics) {
+        Deployment deployment = deployments().get(zone);
+        if (deployment == null) return this;    // No longer deployed in this zone.
+        return with(deployment.withMetrics(deploymentMetrics));
     }
 
     public LockedApplication withoutDeploymentIn(Zone zone) {
@@ -134,29 +161,16 @@ public class LockedApplication extends Application {
                                                      hasOutstandingChange(), Optional.of(issueId)), lock);
     }
 
-    private Version deployVersionFor(DeploymentJobs.JobType jobType, Controller controller) {
+    public Version deployVersionFor(DeploymentJobs.JobType jobType, Controller controller) {
         return jobType == JobType.component
                ? controller.systemVersion()
                : deployVersionIn(jobType.zone(controller.system()).get(), controller);
     }
 
-    private Optional<ApplicationRevision> deployRevisionFor(DeploymentJobs.JobType jobType, Controller controller) {
+    public Optional<ApplicationRevision> deployRevisionFor(DeploymentJobs.JobType jobType, Controller controller) {
         return jobType == JobType.component
                ? Optional.empty()
                : deployRevisionIn(jobType.zone(controller.system()).get());
     }
 
-    /** Returns the revision a new deployment to this zone should use for this application, or empty if we don't know */
-    private Optional<ApplicationRevision> deployRevisionIn(Zone zone) {
-        if (deploying().isPresent() && deploying().get() instanceof ApplicationChange)
-            return ((Change.ApplicationChange) deploying().get()).revision();
-
-        return revisionIn(zone);
-    }
-
-    /** Returns the revision this application is or should be deployed with in the given zone, or empty if unknown. */
-    private Optional<ApplicationRevision> revisionIn(Zone zone) {
-        return Optional.ofNullable(deployments().get(zone)).map(Deployment::revision);
-    }
-
 }
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 7f11754a8f4..c89ae8e7745 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
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.controller.deployment;
 
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.ApplicationId;
-import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.curator.Lock;
@@ -324,7 +323,12 @@ public class DeploymentTrigger {
                                application.deploying().map(d -> "deploying " + d).orElse("restarted deployment"),
                                reason));
         buildSystem.addJob(application.id(), jobType, first);
-        return application.withJobTriggering(jobType, application.deploying(), reason, clock.instant(), controller);
+        return application.withJobTriggering(jobType,
+                                             application.deploying(),
+                                             clock.instant(),
+                                             application.deployVersionFor(jobType, controller),
+                                             application.deployRevisionFor(jobType, controller),
+                                             reason);
     }
 
     /** Returns true if the given proposed job triggering should be effected */
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ClusterInfoMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ClusterInfoMaintainer.java
index 275aedfc812..f27ca0e40e5 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ClusterInfoMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ClusterInfoMaintainer.java
@@ -87,22 +87,20 @@ public class ClusterInfoMaintainer extends Maintainer {
     @Override
     protected void maintain() {
         for (Application application : controller().applications().asList()) {
-            try (Lock lock = controller().applications().lock(application.id())) {
-                Optional<LockedApplication> lockedApplication = controller.applications().get(application.id(), lock);
-                if (!lockedApplication.isPresent()) continue; // application removed
-                
-                for (Deployment deployment : lockedApplication.get().deployments().values()) {
-                    DeploymentId deploymentId = new DeploymentId(application.id(), deployment.zone());
-                    try {
-                        NodeList nodes = controller().applications().configserverClient().getNodeList(deploymentId);
-                        Map<ClusterSpec.Id, ClusterInfo> clusterInfo = getClusterInfo(nodes, deployment.zone());
-                        controller.applications().store(lockedApplication.get()
-                                                                         .with(deployment.withClusterInfo(clusterInfo)));
-                    } 
-                    catch (IOException | IllegalArgumentException e) {
-                        log.log(Level.WARNING, "Failing getting cluster info of for " + deploymentId, e);
+            for (Deployment deployment : application.deployments().values()) {
+                DeploymentId deploymentId = new DeploymentId(application.id(), deployment.zone());
+                try {
+                    NodeList nodes = controller().applications().configserverClient().getNodeList(deploymentId);
+                    Map<ClusterSpec.Id, ClusterInfo> clusterInfo = getClusterInfo(nodes, deployment.zone());
+                    try (Lock lock = controller().applications().lock(application.id())) {
+                        controller.applications().get(application.id(), lock)
+                                .ifPresent(lockedApplication -> controller.applications().store(
+                                        lockedApplication.withClusterInfo(deployment.zone(), clusterInfo)));
                     }
                 }
+                catch (IOException | IllegalArgumentException e) {
+                    log.log(Level.WARNING, "Failing getting cluster info of for " + deploymentId, e);
+                }
             }
         }
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ClusterUtilizationMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ClusterUtilizationMaintainer.java
index 60b890f10fb..6ceb12da3b9 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ClusterUtilizationMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ClusterUtilizationMaintainer.java
@@ -7,7 +7,6 @@ import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
-import com.yahoo.vespa.hosted.controller.LockedApplication;
 import com.yahoo.vespa.hosted.controller.api.integration.MetricsService;
 import com.yahoo.vespa.hosted.controller.application.ClusterUtilization;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
@@ -15,7 +14,6 @@ import com.yahoo.vespa.hosted.controller.application.Deployment;
 import java.time.Duration;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Optional;
 
 /**
  * Fetch utilization metrics and update applications with this data.
@@ -47,13 +45,14 @@ public class ClusterUtilizationMaintainer extends Maintainer {
     @Override
     protected void maintain() {
         for (Application application : controller().applications().asList()) {
-            try (Lock lock = controller().applications().lock(application.id())) {
-                Optional<LockedApplication> lockedApplication = controller.applications().get(application.id(), lock);
-                if (!lockedApplication.isPresent()) continue; // application removed
-                for (Deployment deployment : application.deployments().values()) {
-                    Map<ClusterSpec.Id, ClusterUtilization> clusterUtilization = getUpdatedClusterUtilizations(application.id(), deployment.zone());
-                    controller.applications().store(lockedApplication.get()
-                                                                     .with(deployment.withClusterUtils(clusterUtilization)));
+            for (Deployment deployment : application.deployments().values()) {
+
+                Map<ClusterSpec.Id, ClusterUtilization> clusterUtilization = getUpdatedClusterUtilizations(application.id(), deployment.zone());
+
+                try (Lock lock = controller().applications().lock(application.id())) {
+                    controller.applications().get(application.id(), lock)
+                            .ifPresent(lockedApplication -> controller.applications().store(
+                                    lockedApplication.withClusterUtilization(deployment.zone(), clusterUtilization)));
                 }
             }
         }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
index 2e6e378272d..4e14c501d80 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
@@ -42,17 +42,9 @@ public class DeploymentMetricsMaintainer extends Maintainer {
                                                                          metrics.documentCount(), metrics.queryLatencyMillis(), metrics.writeLatencyMillis());
 
                     try (Lock lock = controller().applications().lock(application.id())) {
-
-                        // Deployment or application may have changed (or be gone) now:
-                        Optional<LockedApplication> lockedApplication = controller().applications()
-                                                                                    .get(application.id(), lock);
-                        if (!lockedApplication.isPresent()) continue;
-
-                        deployment = lockedApplication.get().deployments().get(deployment.zone());
-                        if (deployment == null) continue;
-
-                        controller().applications().store(lockedApplication.get()
-                                                                           .with(deployment.withMetrics(appMetrics)));
+                        controller().applications().get(application.id(), lock)
+                                .ifPresent(lockedApplication -> controller().applications().store(
+                                        lockedApplication.with(deployment.zone(), appMetrics)));
                     }
                 }
                 catch (UncheckedIOException e) {