Use a concrete zone in LatencyAliasTarget

This lets the `NameService` control the translation into an appropriate region.

2 files changed, 101 insertions, 29 deletions

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 306c2a1f1e2..2fa931f2219 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
@@ -177,34 +177,34 @@ public class RoutingPolicies {
     private void updateGlobalDnsOf(Collection<RoutingPolicy> routingPolicies, Set<ZoneId> inactiveZones, @SuppressWarnings("unused") Lock lock) {
         Map<RoutingId, List<RoutingPolicy>> routingTable = routingTableFrom(routingPolicies);
         for (Map.Entry<RoutingId, List<RoutingPolicy>> routeEntry : routingTable.entrySet()) {
-            Map<LatencyAliasTarget, Set<AliasTarget>> targets = computeTargets(routeEntry.getValue(), inactiveZones);
-            // Create a weighted ALIAS per zone, pointing to all targets within that zone
-            targets.forEach(((latencyTarget, weightedTargets) -> {
-                controller.nameServiceForwarder().createAlias(RecordName.from(latencyTarget.name().value()),
-                                                              weightedTargets,
+            Map<RegionEndpoint, Set<AliasTarget>> targets = computeRegionEndpoints(routeEntry.getValue(), inactiveZones);
+            // Create a weighted ALIAS per region, pointing to all zones within the same region
+            targets.forEach(((regionEndpoint, weightedTargets) -> {
+                controller.nameServiceForwarder().createAlias(RecordName.from(regionEndpoint.dnsName), weightedTargets,
                                                               Priority.normal);
             }));
-            // Create global latency-based ALIAS pointing to each weighted ALIAS
+            // Create global latency-based ALIAS pointing to each per-region weighted ALIAS
             var endpoints = controller.routing().endpointsOf(routeEntry.getKey().application())
                                       .named(routeEntry.getKey().endpointId())
                                       .not().requiresRotation();
-            Set<AliasTarget> latencyTargets = Collections.unmodifiableSet(targets.keySet());
+            Set<AliasTarget> latencyTargets = targets.keySet().stream()
+                                                     .map(regionEndpoint -> new LatencyAliasTarget(HostName.from(regionEndpoint.dnsName),
+                                                                                                   regionEndpoint.dnsZone,
+                                                                                                   regionEndpoint.zone))
+                                                     .collect(Collectors.toSet());
             endpoints.forEach(endpoint -> controller.nameServiceForwarder().createAlias(RecordName.from(endpoint.dnsName()),
                                                                                         latencyTargets, Priority.normal));
         }
     }
 
-    /** Compute latency and associated weighted targets from given policies */
-    private Map<LatencyAliasTarget, Set<AliasTarget>> computeTargets(List<RoutingPolicy> policies, Set<ZoneId> inactiveZones) {
-        Map<LatencyAliasTarget, Set<AliasTarget>> targets = new LinkedHashMap<>();
+    /** Compute region endpoints and their targets from given policies */
+    private Map<RegionEndpoint, Set<AliasTarget>> computeRegionEndpoints(List<RoutingPolicy> policies, Set<ZoneId> inactiveZones) {
+        Map<RegionEndpoint, Set<AliasTarget>> targets = new LinkedHashMap<>();
         RoutingMethod routingMethod = RoutingMethod.exclusive;
         for (var policy : policies) {
             if (policy.dnsZone().isEmpty()) continue;
             if (!controller.zoneRegistry().routingMethods(policy.id().zone()).contains(routingMethod)) continue;
             Endpoint weighted = policy.weightedEndpointIn(controller.system(), routingMethod);
-            LatencyAliasTarget latencyTarget = new LatencyAliasTarget(HostName.from(weighted.dnsName()),
-                                                                      policy.dnsZone().get(),
-                                                                      weighted.zones().get(0));
             // Do not route to zone if global routing status is set out at:
             // - zone level (ZoneRoutingPolicy)
             // - deployment level (RoutingPolicy)
@@ -215,10 +215,11 @@ public class RoutingPolicies {
                 weight = 0; // A record with 0 weight will not received traffic. If all records within a group have 0
                             // weight, traffic is routed to all records with equal probability.
             }
+            var regionEndpoint = new RegionEndpoint(weighted, policy.dnsZone().get(), policy.id().zone());
             var weightedTarget = new WeightedAliasTarget(policy.canonicalName(), policy.dnsZone().get(),
                                                          policy.id().zone(), weight);
-            Set<AliasTarget> weightedTargets = targets.computeIfAbsent(latencyTarget, (k) -> new LinkedHashSet<>());
-            weightedTargets.add(weightedTarget);
+            targets.computeIfAbsent(regionEndpoint, (k) -> new LinkedHashSet<>())
+                   .add(weightedTarget);
         }
         return Collections.unmodifiableMap(targets);
     }
@@ -334,6 +335,35 @@ public class RoutingPolicies {
         return false;
     }
 
+    /** Represents a region-wide endpoint */
+    private static class RegionEndpoint {
+
+        private final String dnsName;
+        private final String dnsZone;
+        private final ZoneId zone;
+
+        public RegionEndpoint(Endpoint endpoint, String dnsZone, ZoneId zone) {
+            this.dnsName = Objects.requireNonNull(endpoint).dnsName();
+            this.dnsZone = Objects.requireNonNull(dnsZone);
+            this.zone = Objects.requireNonNull(zone);
+            if (endpoint.scope() != Endpoint.Scope.weighted) throw new IllegalArgumentException("Region endpoint must be weighted");
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (o == null || getClass() != o.getClass()) return false;
+            RegionEndpoint that = (RegionEndpoint) o;
+            return dnsName.equals(that.dnsName);
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(dnsName);
+        }
+
+    }
+
     /** Load balancers allocated to a deployment */
     private static class LoadBalancerAllocation {
 
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 14de468028e..ca4bcf1a354 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
@@ -13,6 +13,7 @@ import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.zone.RoutingMethod;
+import com.yahoo.config.provision.zone.ZoneApi;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
@@ -45,8 +46,10 @@ import java.time.Instant;
 import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -60,9 +63,10 @@ import static org.junit.Assert.assertTrue;
  */
 public class RoutingPoliciesTest {
 
-    private final ZoneId zone1 = ZoneId.from("prod", "us-west-1");
-    private final ZoneId zone2 = ZoneId.from("prod", "us-central-1");
-    private final ZoneId zone3 = ZoneId.from("prod", "us-east-3");
+    private static final ZoneId zone1 = ZoneId.from("prod", "us-west-1");
+    private static final ZoneId zone2 = ZoneId.from("prod", "us-central-1");
+    private static final ZoneId zone3 = ZoneId.from("prod", "aws-us-east-1a");
+    private static final ZoneId zone4 = ZoneId.from("prod", "aws-us-east-1b");
 
     private final ApplicationPackage applicationPackage = applicationPackageBuilder().region(zone1.region())
                                                                                      .region(zone2.region())
@@ -141,6 +145,30 @@ public class RoutingPoliciesTest {
     }
 
     @Test
+    public void global_routing_policies_with_duplicate_region() {
+        var tester = new RoutingPoliciesTester();
+        var context1 = tester.newDeploymentContext("tenant1", "app1", "default");
+        int clustersPerZone = 2;
+        int numberOfDeployments = 3;
+        var applicationPackage = applicationPackageBuilder()
+                .region(zone1.region())
+                .region(zone3.region())
+                .region(zone4.region())
+                .endpoint("r0", "c0")
+                .endpoint("r1", "c1")
+                .build();
+        tester.provisionLoadBalancers(clustersPerZone, context1.instanceId(), zone1, zone3, zone4);
+
+        // Creates alias records
+        context1.submit(applicationPackage).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
+        tester.assertTargets(context1.instanceId(), EndpointId.of("r0"), 0, zone1, zone3, zone4);
+        tester.assertTargets(context1.instanceId(), EndpointId.of("r1"), 1, zone1, zone3, zone4);
+        assertEquals("Routing policy count is equal to cluster count",
+                     numberOfDeployments * clustersPerZone,
+                     tester.policiesOf(context1.instance().id()).size());
+    }
+
+    @Test
     public void zone_routing_policies() {
         zone_routing_policies(false);
         zone_routing_policies(true);
@@ -595,8 +623,14 @@ public class RoutingPoliciesTest {
 
         public RoutingPoliciesTester(DeploymentTester tester) {
             this.tester = tester;
-            // Make all zones directly routed
-            tester.controllerTester().zoneRegistry().exclusiveRoutingIn(tester.controllerTester().zoneRegistry().zones().all().zones());
+            List<ZoneApi> zones = new ArrayList<>(tester.controllerTester().zoneRegistry().zones().all().zones());
+            zones.add(ZoneApiMock.from(zone3));
+            zones.add(ZoneApiMock.from(zone4));
+            tester.controllerTester().zoneRegistry()
+                  .setZones(zones)
+                  .exclusiveRoutingIn(zones);
+            tester.controllerTester().configServer().bootstrap(tester.controllerTester().zoneRegistry().zones().all().ids(),
+                                                               SystemApplication.all());
             ((InMemoryFlagSource) tester.controllerTester().controller().flagSource()).withBooleanFlag(Flags.WEIGHTED_DNS_PER_REGION.id(), true);
         }
 
@@ -655,28 +689,36 @@ public class RoutingPoliciesTest {
 
         private void assertTargets(ApplicationId application, EndpointId endpointId, ClusterSpec.Id clusterId, int loadBalancerId, ZoneId... zones) {
             Set<String> latencyTargets = new HashSet<>();
+            Map<String, List<ZoneId>> zonesByRegionEndpoint = new HashMap<>();
             for (var zone : zones) {
                 Endpoint weighted = tester.controller().routing().endpointsOf(new DeploymentId(application, zone))
                                           .scope(Endpoint.Scope.weighted)
                                           .named(EndpointId.of(clusterId.value()))
                                           .asList()
                                           .get(0);
-                String latencyTarget = "latency/" + weighted.dnsName() + "/dns-zone-1/" +
-                                       weighted.zones().get(0).value();
-                String weightedTarget = "weighted/lb-" + loadBalancerId + "--" + application.serializedForm() + "--" +
-                                        zone.value() + "/dns-zone-1/" + zone.value() + "/1";
-                assertEquals("Weighted target points to load balancer",
-                             List.of(weightedTarget),
-                             aliasDataOf(weighted.dnsName()));
-                latencyTargets.add(latencyTarget);
+                zonesByRegionEndpoint.computeIfAbsent(weighted.dnsName(), (k) -> new ArrayList<>())
+                                     .add(zone);
             }
+            zonesByRegionEndpoint.forEach((regionEndpoint, zonesInRegion) -> {
+                List<String> weightedTargets = zonesInRegion.stream()
+                                                            .map(z -> "weighted/lb-" + loadBalancerId + "--" +
+                                                                      application.serializedForm() + "--" + z.value() +
+                                                                      "/dns-zone-1/" + z.value() + "/1")
+                                                            .collect(Collectors.toList());
+                assertEquals("Weighted endpoint " + regionEndpoint + " points to load balancer",
+                             weightedTargets,
+                             aliasDataOf(regionEndpoint));
+                ZoneId zone = zonesInRegion.get(0);
+                String latencyTarget = "latency/" + regionEndpoint + "/dns-zone-1/" + zone.value();
+                latencyTargets.add(latencyTarget);
+            });
             String globalEndpoint = tester.controller().routing().endpointsOf(application)
                                           .named(endpointId)
                                           .targets(List.of(zones))
                                           .primary()
                                           .map(Endpoint::dnsName)
                                           .orElse("<none>");
-            assertEquals("Global endpoint " + globalEndpoint + " points to expected weighted targets",
+            assertEquals("Global endpoint " + globalEndpoint + " points to expected latency targets",
                          latencyTargets, Set.copyOf(aliasDataOf(globalEndpoint)));
 
         }