Merge pull request #28434 from vespa-engine/mpolden/cleanup-declared-endpoints

Clean up DNS records for declared endpoints on removal

4 files changed, 120 insertions, 87 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 546e8afd5c4..8bba92f36e3 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
@@ -162,14 +162,7 @@ public class RoutingController {
         return prepared;
     }
 
-    /** Read and return zone- and region-scoped endpoints for given deployment */
-    public EndpointList readEndpointsOf(DeploymentId deployment) {
-        Set<Endpoint> endpoints = new LinkedHashSet<>();
-        for (var policy : routingPolicies.read(deployment)) {
-            endpoints.addAll(endpointsOf(deployment, policy.id().cluster(), policy.generatedEndpoints()).asList());
-        }
-        return EndpointList.copyOf(endpoints);
-    }
+    // -------------- Implicit endpoints (scopes 'zone' and 'weighted') --------------
 
     /** Returns the zone- and region-scoped endpoints of given deployment */
     public EndpointList endpointsOf(DeploymentId deployment, ClusterSpec.Id cluster, List<GeneratedEndpoint> generatedEndpoints) {
@@ -211,36 +204,84 @@ public class RoutingController {
         return EndpointList.copyOf(endpoints);
     }
 
-    /** Read application and return declared endpoints for given instance */
-    public EndpointList readDeclaredEndpointsOf(ApplicationId instance) {
-        if (SystemApplication.matching(instance).isPresent()) return EndpointList.EMPTY;
-        return readDeclaredEndpointsOf(TenantAndApplicationId.from(instance)).instance(instance.instance());
+    /** Read routing policies and return zone- and region-scoped endpoints for given deployment */
+    public EndpointList readEndpointsOf(DeploymentId deployment) {
+        Set<Endpoint> endpoints = new LinkedHashSet<>();
+        for (var policy : routingPolicies.read(deployment)) {
+            endpoints.addAll(endpointsOf(deployment, policy.id().cluster(), policy.generatedEndpoints()).asList());
+        }
+        return EndpointList.copyOf(endpoints);
     }
 
-    /** Read application and return declared endpoints for given application */
-    public EndpointList readDeclaredEndpointsOf(TenantAndApplicationId application) {
-        return readDeclaredEndpointsOf(controller.applications().requireApplication(application));
+    // -------------- Declared endpoints (scopes 'global' and 'application') --------------
+
+    /** Returns global endpoints pointing to given deployments */
+    public EndpointList declaredEndpointsOf(RoutingId routingId, ClusterSpec.Id cluster, List<DeploymentId> deployments, GeneratedEndpoints generatedEndpoints) {
+        var endpoints = new ArrayList<Endpoint>();
+        var directMethods = 0;
+        var availableRoutingMethods = routingMethodsOfAll(deployments);
+        for (var method : availableRoutingMethods) {
+            if (method.isDirect() && ++directMethods > 1) {
+                throw new IllegalArgumentException("Invalid routing methods for " + routingId + ": Exceeded maximum " +
+                                                   "direct methods");
+            }
+            Endpoint.EndpointBuilder builder = Endpoint.of(routingId.instance())
+                                                       .target(routingId.endpointId(), cluster, deployments)
+                                                       .on(Port.fromRoutingMethod(method))
+                                                       .routingMethod(method);
+            endpoints.add(builder.in(controller.system()));
+            for (var ge : generatedEndpoints.cluster(cluster)) {
+                endpoints.add(builder.generatedFrom(ge).in(controller.system()));
+            }
+        }
+        return EndpointList.copyOf(endpoints);
     }
 
+    /** Returns application endpoints pointing to given deployments */
+    public EndpointList declaredEndpointsOf(TenantAndApplicationId application, EndpointId endpoint, ClusterSpec.Id cluster,
+                                            Map<DeploymentId, Integer> deployments, GeneratedEndpoints generatedEndpoints) {
+        ZoneId zone = deployments.keySet().iterator().next().zoneId(); // Where multiple zones are possible, they all have the same routing method.
+        RoutingMethod routingMethod = usesSharedRouting(zone) ? RoutingMethod.sharedLayer4 : RoutingMethod.exclusive;
+        Endpoint.EndpointBuilder builder = Endpoint.of(application)
+                                                   .targetApplication(endpoint,
+                                                                      cluster,
+                                                                      deployments)
+                                                   .routingMethod(routingMethod)
+                                                   .on(Port.fromRoutingMethod(routingMethod));
+        List<Endpoint> endpoints = new ArrayList<>();
+        endpoints.add(builder.in(controller.system()));
+        for (var ge : generatedEndpoints.cluster(cluster)) {
+            endpoints.add(builder.generatedFrom(ge).in(controller.system()));
+        }
+        return EndpointList.copyOf(endpoints);
+    }
+
+    /** Read application and return endpoints for all instances in application */
     public EndpointList readDeclaredEndpointsOf(Application application) {
         return declaredEndpointsOf(application.id(), application.deploymentSpec(), readMultiDeploymentGeneratedEndpoints(application.id()));
     }
 
-    /** Returns endpoints declared in {@link DeploymentSpec} for given application */
+    /** Read application and return declared endpoints for given instance */
+    public EndpointList readDeclaredEndpointsOf(ApplicationId instance) {
+        if (SystemApplication.matching(instance).isPresent()) return EndpointList.EMPTY;
+        Application application = controller.applications().requireApplication(TenantAndApplicationId.from(instance));
+        return readDeclaredEndpointsOf(application).instance(instance.instance());
+    }
+
     private EndpointList declaredEndpointsOf(TenantAndApplicationId application, DeploymentSpec deploymentSpec, GeneratedEndpoints generatedEndpoints) {
         Set<Endpoint> endpoints = new LinkedHashSet<>();
         // Global endpoints
         for (var spec : deploymentSpec.instances()) {
             ApplicationId instance = application.instance(spec.name());
-            spec.endpoints().forEach(declaredEndpoint -> {
+            for (var declaredEndpoint : spec.endpoints()) {
                 RoutingId routingId = RoutingId.of(instance, EndpointId.of(declaredEndpoint.endpointId()));
                 List<DeploymentId> deployments = declaredEndpoint.regions().stream()
                                                                  .map(region -> new DeploymentId(instance,
                                                                                                  ZoneId.from(Environment.prod, region)))
                                                                  .toList();
                 ClusterSpec.Id cluster = ClusterSpec.Id.from(declaredEndpoint.containerId());
-                endpoints.addAll(computeGlobalEndpoints(routingId, cluster, deployments, generatedEndpoints));
-            });
+                endpoints.addAll(declaredEndpointsOf(routingId, cluster, deployments, generatedEndpoints).asList());
+            }
         }
         // Application endpoints
         for (var declaredEndpoint : deploymentSpec.endpoints()) {
@@ -248,24 +289,15 @@ public class RoutingController {
                                                                      .collect(toMap(t -> new DeploymentId(application.instance(t.instance()),
                                                                                                           ZoneId.from(Environment.prod, t.region())),
                                                                                     t -> t.weight()));
-
-            ZoneId zone = deployments.keySet().iterator().next().zoneId(); // Where multiple zones are possible, they all have the same routing method.
-            RoutingMethod routingMethod = usesSharedRouting(zone) ? RoutingMethod.sharedLayer4 : RoutingMethod.exclusive;
             ClusterSpec.Id cluster = ClusterSpec.Id.from(declaredEndpoint.containerId());
-            Endpoint.EndpointBuilder builder = Endpoint.of(application)
-                                                       .targetApplication(EndpointId.of(declaredEndpoint.endpointId()),
-                                                                          cluster,
-                                                                          deployments)
-                                                       .routingMethod(routingMethod)
-                                                       .on(Port.fromRoutingMethod(routingMethod));
-            endpoints.add(builder.in(controller.system()));
-            for (var ge : generatedEndpoints.cluster(cluster)) {
-                endpoints.add(builder.generatedFrom(ge).in(controller.system()));
-            }
+            endpoints.addAll(declaredEndpointsOf(application, EndpointId.of(declaredEndpoint.endpointId()), cluster,
+                                                 deployments, generatedEndpoints).asList());
         }
         return EndpointList.copyOf(endpoints);
     }
 
+    // -------------- Other gunk related to endpoints and routing --------------
+
     /** Read endpoints for use in deployment steps, for given deployments, grouped by their zone */
     public Map<ZoneId, List<Endpoint>> readStepRunnerEndpointsOf(Collection<DeploymentId> deployments) {
         TreeMap<ZoneId, List<Endpoint>> endpoints = new TreeMap<>(Comparator.comparing(ZoneId::value));
@@ -342,8 +374,8 @@ public class RoutingController {
             var deployments = rotation.regions().stream()
                                       .map(region -> new DeploymentId(instance.id(), ZoneId.from(Environment.prod, region)))
                                       .toList();
-            endpointsToRemove.addAll(computeGlobalEndpoints(RoutingId.of(instance.id(), rotation.endpointId()),
-                                                            rotation.clusterId(), deployments, readMultiDeploymentGeneratedEndpoints(application.id())));
+            endpointsToRemove.addAll(declaredEndpointsOf(RoutingId.of(instance.id(), rotation.endpointId()),
+                                                         rotation.clusterId(), deployments, readMultiDeploymentGeneratedEndpoints(application.id())).asList());
         }
         endpointsToRemove.forEach(endpoint -> controller.nameServiceForwarder()
                                                         .removeRecords(Record.Type.CNAME,
@@ -448,29 +480,6 @@ public class RoutingController {
         return Collections.unmodifiableList(routingMethods);
     }
 
-    /** Compute global endpoints for given routing ID, application and deployments */
-    private List<Endpoint> computeGlobalEndpoints(RoutingId routingId, ClusterSpec.Id cluster, List<DeploymentId> deployments, GeneratedEndpoints generatedEndpoints) {
-        var endpoints = new ArrayList<Endpoint>();
-        var directMethods = 0;
-        var availableRoutingMethods = routingMethodsOfAll(deployments);
-        for (var method : availableRoutingMethods) {
-            if (method.isDirect() && ++directMethods > 1) {
-                throw new IllegalArgumentException("Invalid routing methods for " + routingId + ": Exceeded maximum " +
-                                                   "direct methods");
-            }
-            Endpoint.EndpointBuilder builder = Endpoint.of(routingId.instance())
-                                                       .target(routingId.endpointId(), cluster, deployments)
-                                                       .on(Port.fromRoutingMethod(method))
-                                                       .routingMethod(method);
-            endpoints.add(builder.in(controller.system()));
-            for (var ge : generatedEndpoints.cluster(cluster)) {
-                endpoints.add(builder.generatedFrom(ge).in(controller.system()));
-            }
-        }
-        return endpoints;
-    }
-
-
     private boolean tokenEndpointEnabled(ApplicationId instance) {
         return createTokenEndpoint.with(FetchVector.Dimension.APPLICATION_ID, instance.serializedForm()).value();
     }
@@ -487,5 +496,4 @@ public class RoutingController {
         return 'v' + base32 + Endpoint.internalDnsSuffix(system);
     }
 
-
 }
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 eb881519589..2a2c544f44d 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
@@ -4,7 +4,7 @@ package com.yahoo.vespa.hosted.controller.routing;
 import ai.vespa.http.DomainName;
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.provision.ApplicationId;
-import com.yahoo.config.provision.zone.AuthMethod;
+import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.zone.RoutingMethod;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.transaction.Mutex;
@@ -400,10 +400,6 @@ public class RoutingPolicies {
         return updated;
     }
 
-    private static Map<AuthMethod, GeneratedEndpoint> asMap(List<GeneratedEndpoint> generatedEndpoints) {
-        return generatedEndpoints.stream().collect(Collectors.toMap(GeneratedEndpoint::authMethod, Function.identity()));
-    }
-
     /** Update zone DNS record for given policy */
     private void updateZoneDnsOf(RoutingPolicy policy, LoadBalancer loadBalancer, DeploymentId deploymentId) {
         EndpointList zoneEndpoints = controller.routing().endpointsOf(deploymentId,
@@ -509,13 +505,21 @@ public class RoutingPolicies {
 
     /** Remove unreferenced instance endpoints from DNS */
     private void removeGlobalDnsUnreferencedBy(LoadBalancerAllocation allocation, RoutingPolicyList deploymentPolicies, @SuppressWarnings("unused") Mutex lock) {
-        Set<RoutingId> removalCandidates = new HashSet<>(deploymentPolicies.asInstanceRoutingTable().keySet());
+        Map<RoutingId, List<RoutingPolicy>> routingTable = deploymentPolicies.asInstanceRoutingTable();
+        Set<RoutingId> removalCandidates = new HashSet<>(routingTable.keySet());
         Set<RoutingId> activeRoutingIds = instanceRoutingIds(allocation);
         removalCandidates.removeAll(activeRoutingIds);
         for (var id : removalCandidates) {
-            EndpointList endpoints = controller.routing().readDeclaredEndpointsOf(id.instance())
-                                               .not().requiresRotation()
-                                               .named(id.endpointId(), Endpoint.Scope.global);
+            List<RoutingPolicy> policies = routingTable.get(id);
+            Map<ClusterSpec.Id, List<RoutingPolicy>> policyByCluster = policies.stream().collect(Collectors.groupingBy(p -> p.id().cluster()));
+            Set<Endpoint> endpoints = new LinkedHashSet<>();
+            policyByCluster.forEach((cluster, clusterPolicies) -> {
+                List<DeploymentId> deployments = clusterPolicies.stream().map(p -> p.id().deployment()).toList();
+                List<GeneratedEndpoint> generated = clusterPolicies.stream().flatMap(p -> p.generatedEndpoints().stream()).distinct().toList();
+                endpoints.addAll(controller.routing().declaredEndpointsOf(id, cluster, deployments, new GeneratedEndpoints(Map.of(cluster, generated)))
+                                           .not().requiresRotation()
+                                           .named(id.endpointId(), Endpoint.Scope.global).asList());
+            });
             // This removes all ALIAS records having this DNS name. There is no attempt to delete only the entry for the
             // affected zone. Instead, the correct set of records is (re)created by updateGlobalDnsOf
             for (var endpoint : endpoints) {
@@ -541,10 +545,18 @@ public class RoutingPolicies {
         removalCandidates.removeAll(activeRoutingIds);
         for (var id : removalCandidates) {
             TenantAndApplicationId application = TenantAndApplicationId.from(id.instance());
-            EndpointList endpoints = controller.routing()
-                                               .readDeclaredEndpointsOf(application)
-                                               .named(id.endpointId(), Endpoint.Scope.application);
             List<RoutingPolicy> policies = routingTable.get(id);
+            Map<ClusterSpec.Id, List<RoutingPolicy>> policyByCluster = policies.stream().collect(Collectors.groupingBy(p -> p.id().cluster()));
+            Set<Endpoint> endpoints = new LinkedHashSet<>();
+            policyByCluster.forEach((cluster, clusterPolicies) -> {
+                // Weights are not available in this context, but they're not used for anything when removing records
+                Map<DeploymentId, Integer> deployments = clusterPolicies.stream()
+                                                                 .map(p -> p.id().deployment())
+                                                                 .collect(Collectors.toMap(Function.identity(), (ignored) -> 1));
+                List<GeneratedEndpoint> generated = clusterPolicies.stream().flatMap(p -> p.generatedEndpoints().stream()).distinct().toList();
+                endpoints.addAll(controller.routing().declaredEndpointsOf(application, id.endpointId(), cluster,
+                                                                          deployments, new GeneratedEndpoints(Map.of(cluster, generated))).asList());
+            });
             for (var policy : policies) {
                 if (!policy.appliesTo(allocation.deployment)) continue;
                 for (Endpoint endpoint : endpoints) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/rotation/RotationRepository.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/rotation/RotationRepository.java
index d54bdead0bd..79eb115c977 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/rotation/RotationRepository.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/rotation/RotationRepository.java
@@ -3,9 +3,7 @@ package com.yahoo.vespa.hosted.controller.routing.rotation;
 
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.application.api.Endpoint;
-import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
-import com.yahoo.text.Text;
 import com.yahoo.vespa.hosted.controller.ApplicationController;
 import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.application.AssignedRotation;
@@ -22,7 +20,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Function;
-import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
 import static java.util.stream.Collectors.collectingAndThen;
@@ -37,8 +34,6 @@ import static java.util.stream.Collectors.collectingAndThen;
  */
 public class RotationRepository {
 
-    private static final Logger log = Logger.getLogger(RotationRepository.class.getName());
-
     private final Map<RotationId, Rotation> allRotations;
     private final ApplicationController applications;
     private final CuratorDb curator;
@@ -92,7 +87,7 @@ public class RotationRepository {
             var endpointId = EndpointId.of(endpoint.endpointId());
             var assignedRotation = assignedRotationsByEndpointId.get(endpointId);
             RotationId rotationId;
-            if (assignedRotation == null) { // No rotation is assigned to this endpoint
+            if (assignedRotation == null) { // No rotation is assigned to this endpoint, assign from available
                 rotationId = requireNonEmpty(availableRotations).remove(0).id();
             } else { // Rotation already assigned to this endpoint, reuse it
                 rotationId = assignedRotation.rotationId();
@@ -117,14 +112,6 @@ public class RotationRepository {
         return Collections.unmodifiableMap(unassignedRotations);
     }
 
-    private Rotation findAvailableRotation(ApplicationId id, RotationLock lock) {
-        Map<RotationId, Rotation> availableRotations = availableRotations(lock);
-        // Return first available rotation
-        RotationId rotation = requireNonEmpty(availableRotations.keySet()).iterator().next();
-        log.info(Text.format("Offering %s to application %s", rotation, id));
-        return allRotations.get(rotation);
-    }
-
     /** Returns a immutable map of rotation ID to rotation sorted by rotation ID */
     private static Map<RotationId, Rotation> from(RotationsConfig rotationConfig) {
         return rotationConfig.rotations().entrySet().stream()
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 1dfaf2109c7..d029987707f 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
@@ -158,7 +158,7 @@ public class RoutingPoliciesTest {
         tester.assertTargets(context2.instanceId(), EndpointId.of("r0"), 0, zone1);
         assertEquals(1, tester.policiesOf(context2.instanceId()).size());
 
-        // All endpoints for app1 are removed
+        // All global endpoints for app1 are removed
         ApplicationPackage applicationPackage5 = applicationPackageBuilder()
                 .region(zone1.region())
                 .region(zone2.region())
@@ -174,6 +174,17 @@ public class RoutingPoliciesTest {
         assertTrue(policies.asList().stream().allMatch(policy -> policy.instanceEndpoints().isEmpty()),
                 "Rotation membership is removed from all policies");
         assertEquals(1, tester.aliasDataOf(endpoint4).size(), "Rotations for " + context2.application() + " are not removed");
+        assertEquals(List.of("c0.app1.tenant1.aws-us-east-1a.vespa.oath.cloud",
+                             "c0.app1.tenant1.us-central-1.vespa.oath.cloud",
+                             "c0.app1.tenant1.us-west-1.vespa.oath.cloud",
+                             "c0.app2.tenant1.us-west-1-w.vespa.oath.cloud",
+                             "c0.app2.tenant1.us-west-1.vespa.oath.cloud",
+                             "c1.app1.tenant1.aws-us-east-1a.vespa.oath.cloud",
+                             "c1.app1.tenant1.us-central-1.vespa.oath.cloud",
+                             "c1.app1.tenant1.us-west-1.vespa.oath.cloud",
+                             "r0.app2.tenant1.global.vespa.oath.cloud"),
+                     tester.recordNames(),
+                     "Endpoints in DNS matches current config");
     }
 
     @Test
@@ -898,9 +909,24 @@ public class RoutingPoliciesTest {
         tester.assertTargets(application, EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0,
                              Map.of(betaZone5, 1));
         assertTrue(tester.controllerTester().controller().routing()
-                         .readDeclaredEndpointsOf(application)
+                         .readDeclaredEndpointsOf(mainContext.application())
                          .named(EndpointId.of("a1"), Endpoint.Scope.application).isEmpty(),
                 "Endpoint removed");
+        assertEquals(List.of("a0.app1.tenant1.a.vespa.oath.cloud",
+                             "beta.app1.tenant1.north.vespa.oath.cloud",
+                             "beta.app1.tenant1.south.vespa.oath.cloud",
+                             "c0.beta.app1.tenant1.north.vespa.oath.cloud",
+                             "c0.beta.app1.tenant1.south.vespa.oath.cloud",
+                             "c0.main.app1.tenant1.north.vespa.oath.cloud",
+                             "c0.main.app1.tenant1.south.vespa.oath.cloud",
+                             "c1.beta.app1.tenant1.north.vespa.oath.cloud",
+                             "c1.beta.app1.tenant1.south.vespa.oath.cloud",
+                             "c1.main.app1.tenant1.north.vespa.oath.cloud",
+                             "c1.main.app1.tenant1.south.vespa.oath.cloud",
+                             "main.app1.tenant1.north.vespa.oath.cloud",
+                             "main.app1.tenant1.south.vespa.oath.cloud"),
+                     tester.recordNames(),
+                     "Endpoints in DNS matches current config");
 
         // Ensure test deployment only updates endpoint of which it is a member
         betaContext.submit(applicationPackage)
@@ -1242,7 +1268,7 @@ public class RoutingPoliciesTest {
                                    int loadBalancerId, Map<DeploymentId, Integer> deploymentWeights, boolean generated) {
             Map<String, List<DeploymentId>> deploymentsByDnsName = new HashMap<>();
             for (var deployment : deploymentWeights.keySet()) {
-                EndpointList applicationEndpoints = tester.controller().routing().readDeclaredEndpointsOf(application)
+                EndpointList applicationEndpoints = tester.controller().routing().readDeclaredEndpointsOf(tester.controller().applications().requireApplication(application))
                                                           .named(endpointId, Endpoint.Scope.application)
                                                           .targets(deployment)
                                                           .cluster(cluster);