Merge pull request #11382 from vespa-engine/mpolden/avoid-duplicating-rotation-urls

Avoid duplicating endpoint URLs

2 files changed, 37 insertions, 19 deletions

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 70134d5a86d..4e70ebb971b 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
@@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.controller.maintenance;
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.zone.ZoneId;
+import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
@@ -28,6 +29,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
 /**
@@ -38,6 +40,8 @@ import java.util.stream.Collectors;
  */
 public class RoutingPolicies {
 
+    private static final Logger LOGGER = Logger.getLogger(RoutingPolicies.class.getName());
+
     private final Controller controller;
     private final CuratorDb db;
 
@@ -78,8 +82,8 @@ public class RoutingPolicies {
                                              deploymentSpec);
         try (var lock = db.lockRoutingPolicies()) {
             removeObsoleteEndpointsFromDns(lbs, lock);
-            storePoliciesOf(lbs, lock);
-            removeObsoletePolicies(lbs, lock);
+            var writtenPolicies = storePoliciesOf(lbs, lock);
+            removeObsoletePolicies(lbs, writtenPolicies, lock);
             registerEndpointsInDns(lbs, lock);
         }
     }
@@ -103,8 +107,8 @@ public class RoutingPolicies {
         }
     }
 
-    /** Store routing policies for given route */
-    private void storePoliciesOf(AllocatedLoadBalancers loadBalancers, @SuppressWarnings("unused") Lock lock) {
+    /** Store routing policies for given route. Returns the persisted policies. */
+    private Set<RoutingPolicy> storePoliciesOf(AllocatedLoadBalancers loadBalancers, @SuppressWarnings("unused") Lock lock) {
         var policies = new LinkedHashSet<>(get(loadBalancers.application));
         for (LoadBalancer loadBalancer : loadBalancers.list) {
             var endpointIds = loadBalancers.endpointIdsOf(loadBalancer);
@@ -116,6 +120,7 @@ public class RoutingPolicies {
             }
         }
         db.writeRoutingPolicies(loadBalancers.application, policies);
+        return policies;
     }
 
     /** Create a policy for given load balancer and register a CNAME for it */
@@ -130,8 +135,13 @@ public class RoutingPolicies {
     }
 
     /** Remove obsolete policies for given route and their CNAME records */
-    private void removeObsoletePolicies(AllocatedLoadBalancers loadBalancers, @SuppressWarnings("unused") Lock lock) {
+    private void removeObsoletePolicies(AllocatedLoadBalancers loadBalancers, Set<RoutingPolicy> writtenPolicies,
+                                        @SuppressWarnings("unused") Lock lock) {
         var allPolicies = new LinkedHashSet<>(get(loadBalancers.application));
+        if (!writtenPolicies.equals(allPolicies)) {
+            LOGGER.log(LogLevel.ERROR, String.format("Stale read! This should not happen. Wrote policies %s, but read %s",
+                                                     writtenPolicies, allPolicies));
+        }
         var removalCandidates = new HashSet<>(allPolicies);
         var activeLoadBalancers = loadBalancers.list.stream()
                                                     .map(LoadBalancer::hostname)
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 c8f5720327a..c406ef0bbd8 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
@@ -109,6 +109,7 @@ import java.util.Arrays;
 import java.util.Base64;
 import java.util.Comparator;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -749,35 +750,42 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
             }));
         }
 
-        // Rotation
-        Cursor globalRotationsArray = object.setArray("globalRotations");
+        // Global endpoints
+        var globalEndpointUrls = new LinkedHashSet<String>();
+
+        // Add default global endpoints. These are backed by rotations.
         instance.endpointsIn(controller.system())
                 .scope(Endpoint.Scope.global)
                 .legacy(false) // Hide legacy names
                 .asList().stream()
                 .map(Endpoint::url)
                 .map(URI::toString)
-                .forEach(globalRotationsArray::addString);
-
-        instance.rotations().stream()
-                .map(AssignedRotation::rotationId)
-                .findFirst()
-                .ifPresent(rotation -> object.setString("rotationId", rotation.asString()));
+                .forEach(globalEndpointUrls::add);
 
-        // Per-cluster rotations
+        // Per-cluster endpoints. These are backed by load balancers.
         Set<RoutingPolicy> routingPolicies = controller.applications().routingPolicies().get(instance.id());
-        for (RoutingPolicy policy : routingPolicies) {
+        for (var policy : routingPolicies) {
             policy.rotationEndpointsIn(controller.system()).asList().stream()
                   .map(Endpoint::url)
                   .map(URI::toString)
-                  .forEach(globalRotationsArray::addString);
+                  .forEach(globalEndpointUrls::add);
         }
 
+        var globalRotationsArray = object.setArray("globalRotations");
+        globalEndpointUrls.forEach(globalRotationsArray::addString);
+
+        // Legacy field. Identifies the first assigned rotation, if any.
+        instance.rotations().stream()
+                .map(AssignedRotation::rotationId)
+                .findFirst()
+                .ifPresent(rotation -> object.setString("rotationId", rotation.asString()));
+
+
         // Deployments sorted according to deployment spec
         List<Deployment> deployments = deploymentSpec.instance(instance.name())
-        .map(spec -> new DeploymentSteps(spec, controller::system))
-        .map(steps -> steps.sortedDeployments(instance.deployments().values()))
-        .orElse(List.copyOf(instance.deployments().values()));
+                                                     .map(spec -> new DeploymentSteps(spec, controller::system))
+                                                     .map(steps -> steps.sortedDeployments(instance.deployments().values()))
+                                                     .orElse(List.copyOf(instance.deployments().values()));
 
         Cursor deploymentsArray = object.setArray("deployments");
         for (Deployment deployment : deployments) {