Merge pull request #12378 from vespa-engine/mpolden/prefer-routing-generator-endpoint

Prefer endpoints from routing generator

5 files changed, 41 insertions, 20 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java
index 0073fbdce34..4637de9a32a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java
@@ -71,20 +71,22 @@ public class RoutingController {
     /** Returns zone-scoped endpoints for given deployment */
     public EndpointList endpointsOf(DeploymentId deployment) {
         var endpoints = new LinkedHashSet<Endpoint>();
+        // TODO(mpolden): Remove this once all applications have deployed once and config server passes correct cluster
+        //                id for combined cluster type
+        controller.serviceRegistry().routingGenerator().clusterEndpoints(deployment)
+                  .forEach((cluster, url) -> endpoints.add(Endpoint.of(deployment.applicationId())
+                                                                   .target(cluster, deployment.zoneId())
+                                                                   .routingMethod(RoutingMethod.shared)
+                                                                   .on(Port.fromRoutingMethod(RoutingMethod.shared))
+                                                                   .in(controller.system())));
+        boolean hasSharedEndpoint = !endpoints.isEmpty();
         for (var policy : routingPolicies.get(deployment).values()) {
             if (!policy.status().isActive()) continue;
             for (var routingMethod :  controller.zoneRegistry().routingMethods(policy.id().zone())) {
+                if (hasSharedEndpoint && routingMethod == RoutingMethod.shared) continue;
                 endpoints.add(policy.endpointIn(controller.system(), routingMethod));
             }
         }
-        if (endpoints.isEmpty()) { // TODO(mpolden): Remove this once all applications have deployed once
-            controller.serviceRegistry().routingGenerator().clusterEndpoints(deployment)
-                      .forEach((cluster, url) -> endpoints.add(Endpoint.of(deployment.applicationId())
-                                                                       .target(cluster, deployment.zoneId())
-                                                                       .routingMethod(RoutingMethod.shared)
-                                                                       .on(Port.fromRoutingMethod(RoutingMethod.shared))
-                                                                       .in(controller.system())));
-        }
         return EndpointList.copyOf(endpoints);
     }
 
@@ -93,6 +95,9 @@ public class RoutingController {
         return endpointsOf(controller.applications().requireInstance(instance));
     }
 
+    /** Returns global-scoped endpoints for given instance */
+    // TODO(mpolden): Add a endpointsOf(Instance, DeploymentId) variant of this that only returns global endpoint of
+    //                which deployment is a member
     public EndpointList endpointsOf(Instance instance) {
         var endpoints = new LinkedHashSet<Endpoint>();
         // Add global endpoints provided by rotations
@@ -193,7 +198,7 @@ public class RoutingController {
             // Register names in DNS
             var rotation = rotationRepository.getRotation(assignedRotation.rotationId());
             if (rotation.isPresent()) {
-                endpoints.asList().forEach(endpoint -> {
+                endpoints.forEach(endpoint -> {
                     controller.nameServiceForwarder().createCname(RecordName.from(endpoint.dnsName()),
                                                                   RecordData.fqdn(rotation.get().name()),
                                                                   Priority.normal);
@@ -210,11 +215,11 @@ public class RoutingController {
 
     /** Remove endpoints in DNS for all rotations assigned to given instance */
     public void removeEndpointsInDns(Instance instance) {
-        endpointsOf(instance.id()).requiresRotation()
-                                  .forEach(endpoint -> controller.nameServiceForwarder()
-                                                                 .removeRecords(Record.Type.CNAME,
-                                                                                RecordName.from(endpoint.dnsName()),
-                                                                                Priority.normal));
+        endpointsOf(instance).requiresRotation()
+                             .forEach(endpoint -> controller.nameServiceForwarder()
+                                                            .removeRecords(Record.Type.CNAME,
+                                                                           RecordName.from(endpoint.dnsName()),
+                                                                           Priority.normal));
     }
 
     /** Returns all routing methods supported by this system */
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java
index f8e9603fcd9..d5c1e41ef24 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java
@@ -115,12 +115,13 @@ public class Endpoint {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
         Endpoint endpoint = (Endpoint) o;
-        return url.equals(endpoint.url);
+        return url.equals(endpoint.url) &&
+               routingMethod == endpoint.routingMethod;
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(url);
+        return Objects.hash(url, routingMethod);
     }
 
     @Override
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 20c77ef2bd2..32213a27a83 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -19,6 +19,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCe
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.SourceRevision;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.Record;
+import com.yahoo.vespa.hosted.controller.api.integration.routing.RoutingEndpoint;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.application.AssignedRotation;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
@@ -779,15 +780,25 @@ public class ControllerTest {
     }
 
     @Test
-    public void testDeployWithRoutingGeneratorFallback() {
+    public void testDeployWithRoutingGeneratorEndpoints() {
         var context = tester.newDeploymentContext();
         var applicationPackage = new ApplicationPackageBuilder()
                 .upgradePolicy("default")
                 .environment(Environment.prod)
                 .region("us-west-1")
                 .build();
+
+        var zones = Set.of(systemTest.zone(tester.controller().system()),
+                           stagingTest.zone(tester.controller().system()),
+                           ZoneId.from("prod", "us-west-1"));
+        for (var zone : zones) {
+            tester.controllerTester().serviceRegistry().routingGeneratorMock()
+                  .putEndpoints(context.deploymentIdIn(zone),
+                                List.of(new RoutingEndpoint("http://legacy-endpoint", "hostname",
+                                                            false, "upstreamName")));
+        }
         // Defer load balancer provisioning in all environments so that routing controller uses routing generator
-        context.deferLoadBalancerProvisioningIn(Environment.test, Environment.staging, Environment.prod)
+        context.deferLoadBalancerProvisioningIn(zones.stream().map(ZoneId::environment).collect(Collectors.toSet()))
                .submit(applicationPackage)
                .deploy();
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
index 4c927028dde..5eae68adf5a 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
@@ -191,7 +191,11 @@ public class DeploymentContext {
 
     /** Defer provisioning of load balancers in zones in given environment */
     public DeploymentContext deferLoadBalancerProvisioningIn(Environment... environment) {
-        configServer().deferLoadBalancerProvisioningIn(Set.of(environment));
+        return deferLoadBalancerProvisioningIn(Set.of(environment));
+    }
+
+    public DeploymentContext deferLoadBalancerProvisioningIn(Set<Environment> environments) {
+        configServer().deferLoadBalancerProvisioningIn(environments);
         return this;
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
index 323b86be1d3..cbb93a51bfd 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
@@ -64,7 +64,7 @@ public class ServiceRegistryMock extends AbstractComponent implements ServiceReg
     public ServiceRegistryMock(SystemName system) {
         this.zoneRegistryMock = new ZoneRegistryMock(system);
         this.configServerMock = new ConfigServerMock(zoneRegistryMock);
-        this.routingGeneratorMock = new RoutingGeneratorMock(RoutingGeneratorMock.TEST_ENDPOINTS, zoneRegistryMock);
+        this.routingGeneratorMock = new RoutingGeneratorMock();
     }
 
     @Inject