Remove load balancers from deployment

5 files changed, 57 insertions, 95 deletions

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 1fb22d28ac7..e4b8a1d9e84 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
@@ -10,7 +10,6 @@ import com.yahoo.config.provision.HostName;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.api.integration.MetricsService;
 import com.yahoo.vespa.hosted.controller.api.integration.MetricsService.ApplicationMetrics;
-import com.yahoo.vespa.hosted.controller.api.integration.configserver.LoadBalancer;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId;
@@ -28,13 +27,11 @@ import com.yahoo.vespa.hosted.controller.rotation.RotationId;
 
 import java.time.Instant;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.OptionalInt;
 import java.util.OptionalLong;
-import java.util.stream.Collectors;
 
 /**
  * An application that has been locked for modification. Provides methods for modifying an application's fields.
@@ -150,8 +147,7 @@ public class LockedApplication {
                                                   previousDeployment.clusterUtils(),
                                                   previousDeployment.clusterInfo(),
                                                   previousDeployment.metrics(),
-                                                  previousDeployment.activity(),
-                                                  previousDeployment.loadBalancers());
+                                                  previousDeployment.activity());
         return with(newDeployment);
     }
 
@@ -254,13 +250,6 @@ public class LockedApplication {
                                      outstandingChange, ownershipIssueId, owner, majorVersion, metrics, rotation, rotationStatus);
     }
 
-    public LockedApplication withLoadBalancersIn(ZoneId zoneId, List<LoadBalancer> loadBalancers) {
-        Map<ClusterSpec.Id, HostName> loadBalancersByCluster = loadBalancers.stream()
-                                                                            .collect(Collectors.toUnmodifiableMap(LoadBalancer::cluster,
-                                                                                                                  LoadBalancer::hostname));
-        return with(deployments.get(zoneId).withLoadBalancers(loadBalancersByCluster));
-    }
-
     /** Don't expose non-leaf sub-objects. */
     private LockedApplication with(Deployment deployment) {
         Map<ZoneId, Deployment> deployments = new LinkedHashMap<>(this.deployments);
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 3fd2932c3e1..51ac25bc321 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
@@ -4,7 +4,6 @@ package com.yahoo.vespa.hosted.controller.application;
 import com.google.common.collect.ImmutableMap;
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.ClusterSpec.Id;
-import com.yahoo.config.provision.HostName;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 
@@ -30,17 +29,16 @@ public class Deployment {
     private final Map<Id, ClusterInfo> clusterInfo;
     private final DeploymentMetrics metrics;
     private final DeploymentActivity activity;
-    private final Map<Id, HostName> loadBalancers;
 
     public Deployment(ZoneId zone, ApplicationVersion applicationVersion, Version version, Instant deployTime) {
         this(zone, applicationVersion, version, deployTime, Collections.emptyMap(), Collections.emptyMap(),
-             DeploymentMetrics.none, DeploymentActivity.none, Collections.emptyMap());
+             DeploymentMetrics.none, DeploymentActivity.none);
     }
 
     public Deployment(ZoneId zone, ApplicationVersion applicationVersion, Version version, Instant deployTime,
                       Map<Id, ClusterUtilization> clusterUtilization, Map<Id, ClusterInfo> clusterInfo,
                       DeploymentMetrics metrics,
-                      DeploymentActivity activity, Map<Id, HostName> loadBalancers) {
+                      DeploymentActivity activity) {
         this.zone = Objects.requireNonNull(zone, "zone cannot be null");
         this.applicationVersion = Objects.requireNonNull(applicationVersion, "applicationVersion cannot be null");
         this.version = Objects.requireNonNull(version, "version cannot be null");
@@ -49,7 +47,6 @@ public class Deployment {
         this.clusterInfo = ImmutableMap.copyOf(Objects.requireNonNull(clusterInfo, "clusterInfo cannot be null"));
         this.metrics = Objects.requireNonNull(metrics, "deploymentMetrics cannot be null");
         this.activity = Objects.requireNonNull(activity, "activity cannot be null");
-        this.loadBalancers = ImmutableMap.copyOf(Objects.requireNonNull(loadBalancers, "loadBalancers cannot be null"));
     }
 
     /** Returns the zone this was deployed to */
@@ -82,33 +79,24 @@ public class Deployment {
         return clusterUtilization;
     }
 
-    public Map<Id, HostName> loadBalancers() {
-        return loadBalancers;
-    }
-
     public Deployment recordActivityAt(Instant instant) {
         return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, clusterInfo, metrics,
-                              activity.recordAt(instant, metrics), loadBalancers);
+                              activity.recordAt(instant, metrics));
     }
 
     public Deployment withClusterUtils(Map<Id, ClusterUtilization> clusterUtilization) {
         return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, clusterInfo, metrics,
-                              activity, loadBalancers);
+                              activity);
     }
 
     public Deployment withClusterInfo(Map<Id, ClusterInfo> newClusterInfo) {
         return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, newClusterInfo, metrics,
-                              activity, loadBalancers);
+                              activity);
     }
 
     public Deployment withMetrics(DeploymentMetrics metrics) {
         return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, clusterInfo, metrics,
-                              activity, loadBalancers);
-    }
-
-    public Deployment withLoadBalancers(Map<Id, HostName> loadBalancers) {
-        return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, clusterInfo, metrics,
-                              activity, loadBalancers);
+                              activity);
     }
 
     /**
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java
index 42e8a572815..c175681a784 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java
@@ -6,7 +6,6 @@ import com.yahoo.config.provision.HostName;
 import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.Application;
-import com.yahoo.vespa.hosted.controller.ApplicationController;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.LoadBalancer;
@@ -16,7 +15,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordData;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordId;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordName;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
-import com.yahoo.vespa.hosted.controller.application.Deployment;
 import com.yahoo.vespa.hosted.controller.application.RoutingPolicy;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 
@@ -24,6 +22,7 @@ import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -33,9 +32,10 @@ import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
 /**
- * Maintains routing policies for all exclusive load balancers in this system.
+ * Maintains DNS records as defined by routing policies for all exclusive load balancers in this system.
  *
  * @author mortent
+ * @author mpolden
  */
 public class RoutingPolicyMaintainer extends Maintainer {
 
@@ -43,7 +43,6 @@ public class RoutingPolicyMaintainer extends Maintainer {
 
     private final NameService nameService;
     private final CuratorDb db;
-    private final ApplicationController applications;
 
     public RoutingPolicyMaintainer(Controller controller,
                                    Duration interval,
@@ -53,39 +52,52 @@ public class RoutingPolicyMaintainer extends Maintainer {
         super(controller, interval, jobControl);
         this.nameService = nameService;
         this.db = db;
-        this.applications = controller.applications();
     }
 
     @Override
     protected void maintain() {
-        updateDnsRecords();
-        removeObsoleteDnsRecords();
+        Map<DeploymentId, List<LoadBalancer>> loadBalancers = loadBalancers(controller().applications().asList());
+        updateDnsRecords(loadBalancers);
+        removeObsoleteDnsRecords(loadBalancers);
     }
 
-    /** Create DNS records for all exclusive load balancers */
-    private void updateDnsRecords() {
-        for (Application application : applications.asList()) {
+    /** Find all exclusive load balancers owned by given applications, grouped by deployment */
+    private Map<DeploymentId, List<LoadBalancer>> loadBalancers(List<Application> applications) {
+        Map<DeploymentId, List<LoadBalancer>> result = new LinkedHashMap<>();
+        for (Application application : applications) {
             for (ZoneId zone : application.deployments().keySet()) {
-                List<LoadBalancer> loadBalancers = findLoadBalancersIn(zone, application.id());
-                if (loadBalancers.isEmpty()) continue;
-
-                applications.lockIfPresent(application.id(), (locked) -> {
-                    applications.store(locked.withLoadBalancersIn(zone, loadBalancers));
-                });
+                DeploymentId deployment = new DeploymentId(application.id(), zone);
+                try {
+                    List<LoadBalancer> loadBalancers = findLoadBalancersIn(deployment);
+                    if (loadBalancers.isEmpty()) continue;
+                    result.put(deployment, loadBalancers);
+                } catch (Exception e) {
+                    log.log(LogLevel.WARNING,
+                            String.format("Got exception fetching load balancers for application: %s, in zone: %s. Retrying in %s",
+                                          application.id().toShortString(), zone.value(), maintenanceInterval()),  e);
+                }
+            }
+        }
+        return Collections.unmodifiableMap(result);
+    }
 
-                try (Lock lock = db.lockRoutingPolicies()) {
-                    Set<RoutingPolicy> policies = new LinkedHashSet<>(db.readRoutingPolicies(application.id()));
-                    for (LoadBalancer loadBalancer : loadBalancers) {
-                        try {
-                            policies.add(registerDnsAlias(application.id(), zone, loadBalancer));
-                        } catch (Exception e) {
-                            log.log(LogLevel.WARNING, "Failed to create or update DNS record for load balancer " +
-                                                      loadBalancer.hostname() + ". Retrying in " + maintenanceInterval(),
-                                    e);
-                        }
+    /** Create DNS records for all exclusive load balancers */
+    private void updateDnsRecords(Map<DeploymentId, List<LoadBalancer>> loadBalancers) {
+        for (Map.Entry<DeploymentId, List<LoadBalancer>> entry : loadBalancers.entrySet()) {
+            ApplicationId application = entry.getKey().applicationId();
+            ZoneId zone = entry.getKey().zoneId();
+            try (Lock lock = db.lockRoutingPolicies()) {
+                Set<RoutingPolicy> policies = new LinkedHashSet<>(db.readRoutingPolicies(application));
+                for (LoadBalancer loadBalancer : entry.getValue()) {
+                    try {
+                        policies.add(registerDnsAlias(application, zone, loadBalancer));
+                    } catch (Exception e) {
+                        log.log(LogLevel.WARNING, "Failed to create or update DNS record for load balancer " +
+                                                  loadBalancer.hostname() + ". Retrying in " + maintenanceInterval(),
+                                e);
                     }
-                    db.writeRoutingPolicies(application.id(), policies);
                 }
+                db.writeRoutingPolicies(application, policies);
             }
         }
     }
@@ -112,29 +124,26 @@ public class RoutingPolicyMaintainer extends Maintainer {
     }
 
     /** Find all load balancers assigned to application in given zone */
-    private List<LoadBalancer> findLoadBalancersIn(ZoneId zone, ApplicationId application) {
+    private List<LoadBalancer> findLoadBalancersIn(DeploymentId deployment) {
         try {
-            return controller().applications().configServer().getLoadBalancers(new DeploymentId(application, zone));
+            return controller().applications().configServer().getLoadBalancers(deployment);
         } catch (Exception e) {
             log.log(LogLevel.WARNING,
                     String.format("Got exception fetching load balancers for application: %s, in zone: %s. Retrying in %s",
-                                  application.toShortString(), zone.value(), maintenanceInterval()),  e);
+                                  deployment.applicationId().toShortString(), deployment.zoneId().value(),
+                                  maintenanceInterval()),  e);
         }
         return Collections.emptyList();
     }
 
     /** Remove all DNS records that point to non-existing load balancers */
-    private void removeObsoleteDnsRecords() {
+    private void removeObsoleteDnsRecords(Map<DeploymentId, List<LoadBalancer>> loadBalancers) {
         try (Lock lock = db.lockRoutingPolicies()) {
             List<RoutingPolicy> removalCandidates = new ArrayList<>(db.readRoutingPolicies());
-            Set<HostName> activeLoadBalancers = controller().applications().asList().stream()
-                                                            .map(Application::deployments)
-                                                            .map(Map::values)
-                                                            .flatMap(Collection::stream)
-                                                            .map(Deployment::loadBalancers)
-                                                            .map(Map::values)
-                                                            .flatMap(Collection::stream)
-                                                            .collect(Collectors.toUnmodifiableSet());
+            Set<HostName> activeLoadBalancers = loadBalancers.values().stream()
+                                                             .flatMap(Collection::stream)
+                                                             .map(LoadBalancer::hostname)
+                                                             .collect(Collectors.toSet());
 
             // Remove any active load balancers
             removalCandidates.removeIf(lb -> activeLoadBalancers.contains(lb.canonicalName()));
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 7f052fd7574..2242b3832de 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
@@ -89,7 +89,6 @@ public class ApplicationSerializer {
     private final String lastWrittenField = "lastWritten";
     private final String lastQueriesPerSecondField = "lastQueriesPerSecond";
     private final String lastWritesPerSecondField = "lastWritesPerSecond";
-    private final String loadBalancers = "loadBalancers";
 
     // DeploymentJobs fields
     private final String projectIdField = "projectId";
@@ -181,7 +180,6 @@ public class ApplicationSerializer {
         deployment.activity().lastWritten().ifPresent(instant -> object.setLong(lastWrittenField, instant.toEpochMilli()));
         deployment.activity().lastQueriesPerSecond().ifPresent(value -> object.setDouble(lastQueriesPerSecondField, value));
         deployment.activity().lastWritesPerSecond().ifPresent(value -> object.setDouble(lastWritesPerSecondField, value));
-        loadBalancersToSlime(deployment.loadBalancers(), object);
     }
 
     private void deploymentMetricsToSlime(DeploymentMetrics metrics, Cursor object) {
@@ -304,13 +302,6 @@ public class ApplicationSerializer {
         });
     }
 
-    private void loadBalancersToSlime(Map<ClusterSpec.Id, HostName> loadBalancerMap, Cursor parentObject) {
-        Cursor root = parentObject.setObject(loadBalancers);
-        for (Map.Entry<ClusterSpec.Id, HostName> entry : loadBalancerMap.entrySet()) {
-            root.setString(entry.getKey().value(), entry.getValue().value());
-        }
-    }
-
     // ------------------ Deserialization
 
     public Application fromSlime(Slime slime) {
@@ -353,8 +344,7 @@ public class ApplicationSerializer {
                               DeploymentActivity.create(optionalInstant(deploymentObject.field(lastQueriedField)),
                                                         optionalInstant(deploymentObject.field(lastWrittenField)),
                                                         optionalDouble(deploymentObject.field(lastQueriesPerSecondField)),
-                                                        optionalDouble(deploymentObject.field(lastWritesPerSecondField))),
-                              loadBalancerMapFromSlime(deploymentObject.field(loadBalancers)));
+                                                        optionalDouble(deploymentObject.field(lastWritesPerSecondField))));
     }
 
     private DeploymentMetrics deploymentMetricsFromSlime(Inspector object) {
@@ -510,12 +500,6 @@ public class ApplicationSerializer {
         return field.valid() ? optionalString(field).map(RotationId::new) : Optional.empty();
     }
 
-    private Map<ClusterSpec.Id, HostName> loadBalancerMapFromSlime(Inspector object) {
-        Map<ClusterSpec.Id, HostName> loadBalancers = new HashMap<>();
-        object.traverse((String name, Inspector value) -> loadBalancers.put(new ClusterSpec.Id(name), HostName.from(value.asString())));
-        return loadBalancers;
-    }
-
     private OptionalLong optionalLong(Inspector field) {
         return field.valid() ? OptionalLong.of(field.asLong()) : OptionalLong.empty();
     }
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 849b251a230..b07e983099a 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
@@ -1,7 +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.collect.ImmutableMap;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.application.api.ValidationOverrides;
@@ -80,7 +79,7 @@ public class ApplicationSerializerTest {
                                        createClusterUtils(3, 0.2), createClusterInfo(3, 4),
                                        new DeploymentMetrics(2, 3, 4, 5, 6, Optional.of(Instant.now().truncatedTo(ChronoUnit.MILLIS))),
                                        DeploymentActivity.create(Optional.of(activityAt), Optional.of(activityAt),
-                                                                 OptionalDouble.of(200), OptionalDouble.of(10)), createLoadBalancers("default", "foo.bar")));
+                                                                 OptionalDouble.of(200), OptionalDouble.of(10))));
 
         OptionalLong projectId = OptionalLong.of(123L);
         List<JobStatus> statusList = new ArrayList<>();
@@ -200,9 +199,6 @@ public class ApplicationSerializerTest {
             Application original6 = writable(original).withOutstandingChange(Change.of(ApplicationVersion.from(new SourceRevision("a", "b", "c"), 42))).get();
             Application serialized6 = applicationSerializer.fromSlime(applicationSerializer.toSlime(original6));
             assertEquals(original6.outstandingChange(), serialized6.outstandingChange());
-
-            assertEquals(1, serialized.deployments().get(zone2).loadBalancers().size());
-            assertEquals(original.deployments().get(zone2).loadBalancers(), serialized.deployments().get(zone2).loadBalancers());
         }
     }
 
@@ -236,10 +232,6 @@ public class ApplicationSerializerTest {
         return result;
     }
 
-    private Map<ClusterSpec.Id, HostName> createLoadBalancers(String clusterId, String hostName) {
-        return ImmutableMap.of(ClusterSpec.Id.from(clusterId), HostName.from(hostName));
-    }
-
     @Test
     public void testCompleteApplicationDeserialization() throws Exception {
         byte[] applicationJson = Files.readAllBytes(testData.resolve("complete-application.json"));