Merge pull request #11451 from vespa-engine/mpolden/update-dns-for-inactive-lbs

Preserve DNS records for inactive load balancers

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)