Never remove all members from global endpoints

3 files changed, 77 insertions, 15 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/GlobalRouting.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/GlobalRouting.java
index 244686c85e4..7ea651c7bd5 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/GlobalRouting.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/GlobalRouting.java
@@ -27,7 +27,10 @@ public class GlobalRouting {
         this.changedAt = Objects.requireNonNull(changedAt, "changedAt must be non-null");
     }
 
-    /** The current status of this */
+    /**
+     * The wanted status of this. The system will try to set this status, but there are constraints that may lead to
+     * the effective status not matching this. See {@link RoutingPolicies}.
+     */
     public Status status() {
         return status;
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java
index cb45c982a0b..8657a601837 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java
@@ -17,7 +17,6 @@ import com.yahoo.vespa.hosted.controller.dns.NameServiceQueue.Priority;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
@@ -91,13 +90,6 @@ public class RoutingPolicies {
         }
     }
 
-    /** Returns whether global DNS records should be updated for given application and zone */
-    private boolean updateGlobalDnsOf(ApplicationId application, ZoneId zone) {
-        if (application.instance().isTester()) return false;
-        if (!zone.environment().isProduction()) return false;
-        return true;
-    }
-
     /** Set the status of all global endpoints in given zone */
     public void setGlobalRoutingStatus(ZoneId zone, GlobalRouting.Status status) {
         try (var lock = db.lockRoutingPolicies()) {
@@ -141,13 +133,18 @@ public class RoutingPolicies {
                 // - zone level (ZoneRoutingPolicy)
                 // - deployment level (RoutingPolicy)
                 // - application package level (deployment.xml)
-                if (anyOut(zonePolicy.globalRouting(), policy.status().globalRouting()) ||
-                    inactiveZones.contains(policy.id().zone())) {
+                if (isConfiguredOut(policy, zonePolicy, inactiveZones)) {
                     staleTargets.add(target);
                 } else {
                     targets.add(target);
                 }
             }
+            // If all targets are configured out, all targets are set in. We do this because otherwise removing 100% of
+            // the ALIAS records would cause the global endpoint to stop resolving entirely (NXDOMAIN).
+            if (targets.isEmpty() && !staleTargets.isEmpty()) {
+                targets.addAll(staleTargets);
+                staleTargets.clear();
+            }
             if (!targets.isEmpty()) {
                 var endpoint = RoutingPolicy.globalEndpointOf(routeEntry.getKey().application(),
                                                               routeEntry.getKey().endpointId(), controller.system());
@@ -238,10 +235,15 @@ public class RoutingPolicies {
         return Collections.unmodifiableMap(routingTable);
     }
 
-    private static boolean anyOut(GlobalRouting... globalRouting) {
-        return Arrays.stream(globalRouting)
-                     .map(GlobalRouting::status)
-                     .anyMatch(status -> status == GlobalRouting.Status.out);
+    /** Returns whether the global routing status of given policy is configured to be {@link GlobalRouting.Status#out} */
+    private static boolean isConfiguredOut(RoutingPolicy policy, ZoneRoutingPolicy zonePolicy, Set<ZoneId> inactiveZones) {
+        // A deployment is can be configured out at any of the following levels:
+        // - zone level (ZoneRoutingPolicy)
+        // - deployment level (RoutingPolicy)
+        // - application package level (deployment.xml)
+        return zonePolicy.globalRouting().status() == GlobalRouting.Status.out ||
+               policy.status().globalRouting().status() == GlobalRouting.Status.out ||
+               inactiveZones.contains(policy.id().zone());
     }
 
     private static boolean isActive(LoadBalancer loadBalancer) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
index 89b7e13e392..a27b2e1084a 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
@@ -45,6 +45,7 @@ import java.util.Set;
 import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -532,6 +533,62 @@ public class RoutingPoliciesTest {
         context.completeRollout();
         tester.assertTargets(context.instanceId(), endpointId, 0, prodZone);
     }
+
+    @Test
+    public void changing_global_routing_status_never_removes_all_members() {
+        var tester = new RoutingPoliciesTester();
+        var context = tester.newDeploymentContext("tenant1", "app1", "default");
+
+        // Provision load balancers and deploy application
+        tester.provisionLoadBalancers(1, context.instanceId(), zone1, zone2);
+        var applicationPackage = new ApplicationPackageBuilder()
+                .region(zone1.region())
+                .region(zone2.region())
+                .endpoint("r0", "c0", zone1.region().value(), zone2.region().value())
+                .build();
+        context.submit(applicationPackage).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
+
+        // Global DNS record is created, pointing to all configured zones
+        tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1, zone2);
+
+        // Global routing status is overridden for one deployment
+        tester.routingPolicies().setGlobalRoutingStatus(context.deploymentIdIn(zone1), GlobalRouting.Status.out,
+                                                        GlobalRouting.Agent.tenant);
+        context.flushDnsUpdates();
+        tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone2);
+
+        // Setting other deployment out implicitly sets all deployments in
+        tester.routingPolicies().setGlobalRoutingStatus(context.deploymentIdIn(zone2), GlobalRouting.Status.out,
+                                                        GlobalRouting.Agent.tenant);
+        context.flushDnsUpdates();
+        tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1, zone2);
+
+        // One inactive deployment is put back in. Global DNS record now points to the only active deployment
+        tester.routingPolicies().setGlobalRoutingStatus(context.deploymentIdIn(zone1), GlobalRouting.Status.in,
+                                                        GlobalRouting.Agent.tenant);
+        context.flushDnsUpdates();
+        tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1);
+
+        // Setting zone (containing active deployment) out puts all deployments in
+        tester.routingPolicies().setGlobalRoutingStatus(zone1, GlobalRouting.Status.out);
+        context.flushDnsUpdates();
+        assertEquals(GlobalRouting.Status.out, tester.routingPolicies().get(zone1).globalRouting().status());
+        tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1, zone2);
+
+        // Setting zone back in removes the currently inactive deployment
+        tester.routingPolicies().setGlobalRoutingStatus(zone1, GlobalRouting.Status.in);
+        context.flushDnsUpdates();
+        tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1);
+
+        // Inactive deployment is set in
+        tester.routingPolicies().setGlobalRoutingStatus(context.deploymentIdIn(zone2), GlobalRouting.Status.in,
+                                                        GlobalRouting.Agent.tenant);
+        context.flushDnsUpdates();
+        for (var policy : tester.routingPolicies().get(context.instanceId()).values()) {
+            assertSame(GlobalRouting.Status.in, policy.status().globalRouting().status());
+        }
+        tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1, zone2);
+    }
     
     private static List<LoadBalancer> createLoadBalancers(ZoneId zone, ApplicationId application, int count) {
         List<LoadBalancer> loadBalancers = new ArrayList<>();