Preserve DNS records for inactive load balancers

This change ensures that we preserve DNS records, as long as the underlying load
balancer exists, even if it's inactive.

Inactive routing policies are excluded in the application API so that endpoints
for deleted clusters are not shown in console.

8 files changed, 54 insertions, 33 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/RoutingPolicy.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/RoutingPolicy.java
index a86bbaa317e..e55bbaf6acc 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/RoutingPolicy.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/RoutingPolicy.java
@@ -27,16 +27,18 @@ public class RoutingPolicy {
     private final HostName canonicalName;
     private final Optional<String> dnsZone;
     private final Set<EndpointId> endpoints;
+    private final boolean active;
 
     /** DO NOT USE. Public for serialization purposes */
     public RoutingPolicy(ApplicationId owner, ClusterSpec.Id cluster, ZoneId zone, HostName canonicalName,
-                         Optional<String> dnsZone, Set<EndpointId> endpoints) {
+                         Optional<String> dnsZone, Set<EndpointId> endpoints, boolean active) {
         this.owner = Objects.requireNonNull(owner, "owner must be non-null");
         this.cluster = Objects.requireNonNull(cluster, "cluster must be non-null");
         this.zone = Objects.requireNonNull(zone, "zone must be non-null");
         this.canonicalName = Objects.requireNonNull(canonicalName, "canonicalName must be non-null");
         this.dnsZone = Objects.requireNonNull(dnsZone, "dnsZone must be non-null");
         this.endpoints = ImmutableSortedSet.copyOf(Objects.requireNonNull(endpoints, "endpoints must be non-null"));
+        this.active = active;
     }
 
     /** The application owning this */
@@ -69,6 +71,11 @@ public class RoutingPolicy {
         return endpoints;
     }
 
+    /** Returns whether this is active (the underlying load balancer is in an active state) */
+    public boolean active() {
+        return active;
+    }
+
     /** Returns the endpoint of this */
     public Endpoint endpointIn(SystemName system) {
         return Endpoint.of(owner).target(cluster, zone).on(Port.tls()).directRouting().in(system);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicies.java
index 12108b9d29a..59394b955b8 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicies.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicies.java
@@ -127,7 +127,7 @@ public class RoutingPolicies {
     private RoutingPolicy createPolicy(ApplicationId application, ZoneId zone, LoadBalancer loadBalancer,
                                        Set<EndpointId> endpointIds) {
         var routingPolicy = new RoutingPolicy(application, loadBalancer.cluster(), zone, loadBalancer.hostname(),
-                                              loadBalancer.dnsZone(), endpointIds);
+                                              loadBalancer.dnsZone(), endpointIds, isActive(loadBalancer));
         var name = RecordName.from(routingPolicy.endpointIn(controller.system()).dnsName());
         var data = RecordData.fqdn(loadBalancer.hostname().value());
         controller.nameServiceForwarder().createCname(name, data, Priority.normal);
@@ -197,6 +197,15 @@ public class RoutingPolicies {
         return routingTable;
     }
 
+    private static boolean isActive(LoadBalancer loadBalancer) {
+        switch (loadBalancer.state()) {
+            case reserved: // Count reserved as active as we want callers (application API) to see the endpoint as early
+                           // as possible
+            case active: return true;
+        }
+        return false;
+    }
+
     /** Load balancers allocated to a deployment */
     private static class AllocatedLoadBalancers {
 
@@ -209,9 +218,7 @@ public class RoutingPolicies {
                                        DeploymentSpec deploymentSpec) {
             this.application = application;
             this.zone = zone;
-            this.list = loadBalancers.stream()
-                                     .filter(AllocatedLoadBalancers::shouldUpdatePolicy)
-                                     .collect(Collectors.toUnmodifiableList());
+            this.list = List.copyOf(loadBalancers);
             this.deploymentSpec = deploymentSpec;
         }
 
@@ -232,17 +239,6 @@ public class RoutingPolicies {
                                .collect(Collectors.toSet());
         }
 
-        private static boolean shouldUpdatePolicy(LoadBalancer loadBalancer) {
-            switch (loadBalancer.state()) {
-                case active:
-                case reserved: // This allows DNS updates to happen early, while an application is being prepared.
-                    return true;
-            }
-            // Any other state, such as inactive, is ignored.
-            LOGGER.log(LogLevel.WARNING, "Ignoring load balancer " + loadBalancer.hostname() + " in state " + loadBalancer.state());
-            return false;
-        }
-
     }
 
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializer.java
index 890fa31bc4d..54a3ef7551a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializer.java
@@ -35,6 +35,7 @@ public class RoutingPolicySerializer {
     private static final String zoneField = "zone";
     private static final String dnsZoneField = "dnsZone";
     private static final String rotationsField = "rotations";
+    private static final String activeField = "active";
 
     public Slime toSlime(Set<RoutingPolicy> routingPolicies) {
         var slime = new Slime();
@@ -50,6 +51,7 @@ public class RoutingPolicySerializer {
             policy.endpoints().forEach(endpointId -> {
                 rotationArray.addString(endpointId.id());
             });
+            policyObject.setBool(activeField, policy.active());
         });
         return slime;
     }
@@ -61,12 +63,15 @@ public class RoutingPolicySerializer {
         field.traverse((ArrayTraverser) (i, inspect) -> {
             var endpointIds = new LinkedHashSet<EndpointId>();
             inspect.field(rotationsField).traverse((ArrayTraverser) (j, endpointId) -> endpointIds.add(EndpointId.of(endpointId.asString())));
+            var activeFieldInspector = inspect.field(activeField);
+            // TODO(mpolden): Remove field presence check after January 2020
+            boolean active = !activeFieldInspector.valid() || activeFieldInspector.asBool();
             policies.add(new RoutingPolicy(owner,
                                            ClusterSpec.Id.from(inspect.field(clusterField).asString()),
                                            ZoneId.from(inspect.field(zoneField).asString()),
                                            HostName.from(inspect.field(canonicalNameField).asString()),
                                            Serializers.optionalString(inspect.field(dnsZoneField)),
-                                           endpointIds));
+                                           endpointIds, active));
         });
         return Collections.unmodifiableSet(policies);
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
index 1901cd6bb37..114df26015f 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
@@ -909,6 +909,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
         // Per-cluster rotations
         Set<RoutingPolicy> routingPolicies = controller.applications().routingPolicies().get(instance.id());
         for (RoutingPolicy policy : routingPolicies) {
+            if (!policy.active()) continue;
             policy.rotationEndpointsIn(controller.system()).asList().stream()
                   .map(Endpoint::url)
                   .map(URI::toString)
@@ -1009,6 +1010,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
         // Add endpoint(s) defined by routing policies
         var endpointArray = response.setArray("endpoints");
         for (var policy : controller.applications().routingPolicies().get(deploymentId)) {
+            if (!policy.active()) continue;
             Cursor endpointObject = endpointArray.addObject();
             Endpoint endpoint = policy.endpointIn(controller.system());
             endpointObject.setString("cluster", policy.cluster().value());
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java
index 9ca50e7f22a..0340cb25d6f 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java
@@ -221,13 +221,13 @@ public class InternalStepRunnerTest {
                                                                                                       JobType.systemTest.zone(system()),
                                                                                                       HostName.from("host"),
                                                                                                       Optional.empty(),
-                                                                                                      emptySet())));
+                                                                                                      emptySet(), true)));
         tester.controller().curator().writeRoutingPolicies(app.testerId().id(), Set.of(new RoutingPolicy(app.testerId().id(),
                                                                                                          ClusterSpec.Id.from("default"),
                                                                                                          JobType.systemTest.zone(system()),
                                                                                                          HostName.from("host"),
                                                                                                          Optional.empty(),
-                                                                                                         emptySet())));
+                                                                                                         emptySet(), true)));
         tester.runner().run();;
         assertEquals(succeeded, tester.jobs().last(app.instanceId(), JobType.systemTest).get().steps().get(Step.installReal));
         assertEquals(succeeded, tester.jobs().last(app.instanceId(), JobType.systemTest).get().steps().get(Step.installTester));
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPoliciesTest.java
index 940e4efdeed..f78873bae09 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPoliciesTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPoliciesTest.java
@@ -320,14 +320,6 @@ public class RoutingPoliciesTest {
                                      LoadBalancer.State.active,
                                      Optional.of("dns-zone-1")));
         }
-        // Add an inactive load balancers that should be ignored
-        loadBalancers.add(new LoadBalancer("inactive-LB-0-Z-" + zone.value(),
-                                           application,
-                                           ClusterSpec.Id.from("c0"),
-                                           HostName.from("lb-0--" + application.serializedForm() +
-                                                         "--" + zone.value()),
-                                           LoadBalancer.State.inactive,
-                                           Optional.of("dns-zone-1")));
         return loadBalancers;
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java
index e99cc302ffe..23355bd6033 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java
@@ -6,6 +6,7 @@ import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.zone.ZoneId;
+import com.yahoo.vespa.config.SlimeUtils;
 import com.yahoo.vespa.hosted.controller.application.EndpointId;
 import com.yahoo.vespa.hosted.controller.application.RoutingPolicy;
 import org.junit.Test;
@@ -15,6 +16,7 @@ import java.util.Optional;
 import java.util.Set;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 /**
  * @author mortent
@@ -24,7 +26,7 @@ public class RoutingPolicySerializerTest {
     private final RoutingPolicySerializer serializer = new RoutingPolicySerializer();
 
     @Test
-    public void test_serialization() {
+    public void serialization() {
         var owner = ApplicationId.defaultId();
         var endpoints = Set.of(EndpointId.of("r1"), EndpointId.of("r2"));
         var policies = ImmutableSet.of(new RoutingPolicy(owner,
@@ -32,13 +34,13 @@ public class RoutingPolicySerializerTest {
                                                          ZoneId.from("prod", "us-north-1"),
                                                          HostName.from("long-and-ugly-name"),
                                                          Optional.of("zone1"),
-                                                         endpoints),
+                                                         endpoints, true),
                                        new RoutingPolicy(owner,
                                                          ClusterSpec.Id.from("my-cluster2"),
                                                          ZoneId.from("prod", "us-north-2"),
                                                          HostName.from("long-and-ugly-name-2"),
                                                          Optional.empty(),
-                                                         endpoints));
+                                                         endpoints, false));
         var serialized = serializer.fromSlime(owner, serializer.toSlime(policies));
         assertEquals(policies.size(), serialized.size());
         for (Iterator<RoutingPolicy> it1 = policies.iterator(), it2 = serialized.iterator(); it1.hasNext();) {
@@ -50,7 +52,18 @@ public class RoutingPolicySerializerTest {
             assertEquals(expected.canonicalName(), actual.canonicalName());
             assertEquals(expected.dnsZone(), actual.dnsZone());
             assertEquals(expected.endpoints(), actual.endpoints());
+            assertEquals(expected.active(), actual.active());
         }
     }
 
+    @Test
+    public void legacy_serialization() {
+        var json = "{\"routingPolicies\":[{\"cluster\":\"default\",\"zone\":\"prod.us-north-1\"," +
+                   "\"canonicalName\":\"lb-0\"," +
+                   "\"dnsZone\":\"dns-zone-id\",\"rotations\":[]}]}";
+        var serialized = serializer.fromSlime(ApplicationId.defaultId(), SlimeUtils.jsonToSlime(json));
+        assertTrue(serialized.iterator().next().active());
+
+    }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
index 27305f8956f..eba619253b9 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
@@ -1430,12 +1430,18 @@ public class ApplicationApiTest extends ControllerContainerTest {
                 .region("us-west-1")
                 .build();
         app.submit(applicationPackage).deploy();
-        RoutingPolicy policy = new RoutingPolicy(app.instanceId(),
+        Set<RoutingPolicy> policies = Set.of(new RoutingPolicy(app.instanceId(),
                                                  ClusterSpec.Id.from("default"),
                                                  ZoneId.from(Environment.prod, RegionName.from("us-west-1")),
                                                  HostName.from("lb-0-canonical-name"),
-                                                 Optional.of("dns-zone-1"), Set.of(EndpointId.of("c0")));
-        tester.controller().curator().writeRoutingPolicies(app.instanceId(), Set.of(policy));
+                                                 Optional.of("dns-zone-1"), Set.of(EndpointId.of("c0")), true),
+                                           // Inactive policy is not included
+                                           new RoutingPolicy(app.instanceId(),
+                                                             ClusterSpec.Id.from("deleted-cluster"),
+                                                             ZoneId.from(Environment.prod, RegionName.from("us-west-1")),
+                                                             HostName.from("lb-1-canonical-name"),
+                                                             Optional.of("dns-zone-1"), Set.of(), false));
+        tester.controller().curator().writeRoutingPolicies(app.instanceId(), policies);
 
         // GET application
         tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/instance1", GET)