Merge pull request #9246 from vespa-engine/mpolden/remove-stale-policies

Remove stale routing policies

2 files changed, 36 insertions, 29 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java
index 9c4fa8115db..0ddc24147ee 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.hosted.controller.maintenance;
 
 import com.yahoo.config.provision.ApplicationId;
-import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.RotationName;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.log.LogLevel;
@@ -25,6 +24,7 @@ import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -34,7 +34,7 @@ import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
 /**
- * Maintains DNS records as defined by routing policies for all exclusive load balancers in this system.
+ * Maintains routing policies and their DNS records for all exclusive load balancers in this system.
  *
  * @author mortent
  * @author mpolden
@@ -64,10 +64,10 @@ public class RoutingPolicyMaintainer extends Maintainer {
     @Override
     protected void maintain() {
         Map<DeploymentId, List<LoadBalancer>> loadBalancers = findLoadBalancers();
-        removeObsoleteAliases(loadBalancers);
-        registerCnames(loadBalancers);
-        removeObsoleteCnames(loadBalancers);
-        registerAliases();
+        removeObsoleteEndpointsFromDns(loadBalancers);
+        storePolicies(loadBalancers);
+        removeObsoletePolicies(loadBalancers);
+        registerEndpointsInDns();
     }
 
     /** Find all exclusive load balancers in this system, grouped by deployment */
@@ -89,8 +89,8 @@ public class RoutingPolicyMaintainer extends Maintainer {
         return Collections.unmodifiableMap(result);
     }
 
-    /** Create aliases (global rotations) for all current routing policies */
-    private void registerAliases() {
+    /** Create global endpoints for all current routing policies */
+    private void registerEndpointsInDns() {
         try (Lock lock = db.lockRoutingPolicies()) {
             Map<RoutingId, List<RoutingPolicy>> routingTable = routingTableFrom(db.readRoutingPolicies());
 
@@ -115,8 +115,8 @@ public class RoutingPolicyMaintainer extends Maintainer {
         }
     }
 
-    /** Create CNAME records for each individual load balancers */
-    private void registerCnames(Map<DeploymentId, List<LoadBalancer>> loadBalancers) {
+    /** Store routing policies for all load balancers */
+    private void storePolicies(Map<DeploymentId, List<LoadBalancer>> loadBalancers) {
         for (Map.Entry<DeploymentId, List<LoadBalancer>> entry : loadBalancers.entrySet()) {
             ApplicationId application = entry.getKey().applicationId();
             ZoneId zone = entry.getKey().zoneId();
@@ -124,7 +124,7 @@ public class RoutingPolicyMaintainer extends Maintainer {
                 Set<RoutingPolicy> policies = new LinkedHashSet<>(db.readRoutingPolicies(application));
                 for (LoadBalancer loadBalancer : entry.getValue()) {
                     try {
-                        RoutingPolicy policy = registerCname(application, zone, loadBalancer);
+                        RoutingPolicy policy = storePolicy(application, zone, loadBalancer);
                         if (!policies.add(policy)) {
                             policies.remove(policy);
                             policies.add(policy);
@@ -140,8 +140,8 @@ public class RoutingPolicyMaintainer extends Maintainer {
         }
     }
 
-    /** Register DNS alias for given load balancer */
-    private RoutingPolicy registerCname(ApplicationId application, ZoneId zone, LoadBalancer loadBalancer) {
+    /** Store policy for given load balancer and request a CNAME for it */
+    private RoutingPolicy storePolicy(ApplicationId application, ZoneId zone, LoadBalancer loadBalancer) {
         RoutingPolicy routingPolicy = new RoutingPolicy(application, loadBalancer.cluster(), zone,
                                                         loadBalancer.hostname(), loadBalancer.dnsZone(),
                                                         loadBalancer.rotations());
@@ -151,28 +151,33 @@ public class RoutingPolicyMaintainer extends Maintainer {
         return routingPolicy;
     }
 
-    /** Remove all DNS records that point to non-existing load balancers */
-    private void removeObsoleteCnames(Map<DeploymentId, List<LoadBalancer>> loadBalancers) {
+    /** Remove obsolete policies and their CNAME records */
+    private void removeObsoletePolicies(Map<DeploymentId, List<LoadBalancer>> loadBalancers) {
         try (Lock lock = db.lockRoutingPolicies()) {
-            Set<RoutingPolicy> removalCandidates = db.readRoutingPolicies().values().stream()
-                                                     .flatMap(Collection::stream)
-                                                     .collect(Collectors.toSet());
-            Set<HostName> activeLoadBalancers = loadBalancers.values().stream()
-                                                             .flatMap(Collection::stream)
-                                                             .map(LoadBalancer::hostname)
-                                                             .collect(Collectors.toSet());
-
-            // Remove any active load balancers
+            var allPolicies = new HashMap<>(db.readRoutingPolicies());
+            var removalCandidates = allPolicies.values().stream()
+                                               .flatMap(Collection::stream)
+                                               .collect(Collectors.toSet());
+            var activeLoadBalancers = loadBalancers.values().stream()
+                                                   .flatMap(Collection::stream)
+                                                   .map(LoadBalancer::hostname)
+                                                   .collect(Collectors.toSet());
+            // Keep active load balancers by removing them from candidates
             removalCandidates.removeIf(policy -> activeLoadBalancers.contains(policy.canonicalName()));
-            for (RoutingPolicy policy : removalCandidates) {
-                String dnsName = policy.endpointIn(controller().system()).dnsName();
+            for (var policy : removalCandidates) {
+                var dnsName = policy.endpointIn(controller().system()).dnsName();
                 nameServiceForwarder.removeRecords(Record.Type.CNAME, RecordName.from(dnsName), Priority.normal);
+                // Remove stale policy from curator
+                var updatedPolicies = new LinkedHashSet<>(allPolicies.getOrDefault(policy.owner(), Set.of()));
+                updatedPolicies.remove(policy);
+                allPolicies.put(policy.owner(), updatedPolicies);
+                db.writeRoutingPolicies(policy.owner(), updatedPolicies);
             }
         }
     }
 
-    /** Remove global rotations that are not referenced by given load balancers */
-    private void removeObsoleteAliases(Map<DeploymentId, List<LoadBalancer>> loadBalancers) {
+    /** Remove DNS for global endpoints not referenced by given load balancers */
+    private void removeObsoleteEndpointsFromDns(Map<DeploymentId, List<LoadBalancer>> loadBalancers) {
         try (Lock lock = db.lockRoutingPolicies()) {
             Set<RoutingId> removalCandidates = routingTableFrom(db.readRoutingPolicies()).keySet();
             Set<RoutingId> activeRoutingIds = routingIdsFrom(loadBalancers);
@@ -197,7 +202,7 @@ public class RoutingPolicyMaintainer extends Maintainer {
         return Collections.unmodifiableSet(routingIds);
     }
 
-    /** Compute a routing table from given policies. */
+    /** Compute a routing table from given policies */
     private static Map<RoutingId, List<RoutingPolicy>> routingTableFrom(Map<ApplicationId, Set<RoutingPolicy>> routingPolicies) {
         var flattenedPolicies = routingPolicies.values().stream().flatMap(Collection::stream).collect(Collectors.toSet());
         var routingTable = new LinkedHashMap<RoutingId, List<RoutingPolicy>>();
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainerTest.java
index 721ee206f30..14d5dc4e7c3 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainerTest.java
@@ -196,6 +196,8 @@ public class RoutingPolicyMaintainerTest {
                 "c1.app1.tenant1.us-central-1.vespa.oath.cloud"
         );
         assertEquals(expectedRecords, recordNames());
+        assertTrue("Removes stale routing policies " + app2, tester.controller().applications().routingPolicies(app2.id()).isEmpty());
+        assertEquals("Keeps routing policies for " + app1, 4, tester.controller().applications().routingPolicies(app1.id()).size());
     }
 
     private void maintain() {