Limit name service calls in DnsMaintainer

4 files changed, 77 insertions, 11 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java
index b38f55826d6..5045ab0877c 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java
@@ -75,7 +75,7 @@ public class ControllerMaintenance extends AbstractComponent {
         clusterUtilizationMaintainer = new ClusterUtilizationMaintainer(controller, Duration.ofHours(2), jobControl);
         deploymentMetricsMaintainer = new DeploymentMetricsMaintainer(controller, Duration.ofMinutes(5), jobControl);
         applicationOwnershipConfirmer = new ApplicationOwnershipConfirmer(controller, Duration.ofHours(12), jobControl, ownershipIssues);
-        dnsMaintainer = new DnsMaintainer(controller, Duration.ofHours(12), jobControl, nameService);
+        dnsMaintainer = new DnsMaintainer(controller, Duration.ofMinutes(5), jobControl, nameService);
         systemUpgrader = new SystemUpgrader(controller, Duration.ofMinutes(1), jobControl);
         jobRunner = new JobRunner(controller, Duration.ofMinutes(2), jobControl);
         osUpgraders = osUpgraders(controller, jobControl);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainer.java
index 15b238dca15..36d83b8ee6a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainer.java
@@ -12,7 +12,12 @@ import com.yahoo.vespa.hosted.controller.rotation.RotationLock;
 import com.yahoo.vespa.hosted.controller.rotation.RotationRepository;
 
 import java.time.Duration;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
 import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.logging.Logger;
 
 /**
@@ -25,6 +30,7 @@ public class DnsMaintainer extends Maintainer {
     private static final Logger log = Logger.getLogger(DnsMaintainer.class.getName());
 
     private final NameService nameService;
+    private final AtomicInteger rotationIndex = new AtomicInteger(0);
 
     public DnsMaintainer(Controller controller, Duration interval, JobControl jobControl,
                          NameService nameService) {
@@ -40,7 +46,7 @@ public class DnsMaintainer extends Maintainer {
     protected void maintain() {
         try (RotationLock lock = rotationRepository().lock()) {
             Map<RotationId, Rotation> unassignedRotations = rotationRepository().availableRotations(lock);
-            unassignedRotations.values().forEach(this::removeDnsAlias);
+            rotationToCheckOf(unassignedRotations.values()).ifPresent(this::removeDnsAlias);
         }
     }
 
@@ -57,6 +63,22 @@ public class DnsMaintainer extends Maintainer {
                    });
     }
 
+    /**
+     * Returns the rotation that should be checked in this run. We check only one rotation per run to avoid running into
+     * rate limits that may be imposed by the {@link NameService} implementation.
+     */
+    private Optional<Rotation> rotationToCheckOf(Collection<Rotation> rotations) {
+        if (rotations.isEmpty()) return Optional.empty();
+        List<Rotation> rotationList = new ArrayList<>(rotations);
+        int index = rotationIndex.getAndUpdate((i)-> {
+            if (i < rotationList.size() - 1) {
+                return ++i;
+            }
+            return 0;
+        });
+        return Optional.of(rotationList.get(index));
+    }
+
     /** Returns whether we can update the given record */
     private static boolean canUpdate(Record record) {
         String recordName = record.name().asString();
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
index 1e5e642cfd4..fe2394d872d 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
@@ -67,6 +67,8 @@ import static org.junit.Assert.assertNotNull;
  */
 public final class ControllerTester {
 
+    public static final int availableRotations = 10;
+
     private final AthenzDbMock athenzDb;
     private final ManualClock clock;
     private final ConfigServerMock configServer;
@@ -332,7 +334,7 @@ public final class ControllerTester {
 
     private static RotationsConfig defaultRotationsConfig() {
         RotationsConfig.Builder builder = new RotationsConfig.Builder();
-        for (int i = 1; i <= 10; i++) {
+        for (int i = 1; i <= availableRotations; i++) {
             String id = String.format("%02d", i);
             builder = builder.rotations("rotation-id-" + id, "rotation-fqdn-" + id);
         }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java
index 3d2a3ccd842..69b183d9711 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java
@@ -6,16 +6,24 @@ import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.vespa.athenz.api.OktaAccessToken;
 import com.yahoo.vespa.hosted.controller.Application;
+import com.yahoo.vespa.hosted.controller.ControllerTester;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.Record;
+import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordData;
+import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordId;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordName;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
+import com.yahoo.vespa.hosted.controller.application.GlobalDnsName;
 import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester;
 import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb;
+import com.yahoo.vespa.hosted.controller.rotation.Rotation;
+import com.yahoo.vespa.hosted.controller.rotation.RotationId;
+import org.junit.Before;
 import org.junit.Test;
 
 import java.time.Duration;
+import java.util.Map;
 import java.util.Optional;
 import java.util.function.Function;
 
@@ -30,13 +38,20 @@ import static org.junit.Assert.assertTrue;
  */
 public class DnsMaintainerTest {
 
+    private DeploymentTester tester;
+    private DnsMaintainer maintainer;
+
+    @Before
+    public void before() {
+        tester = new DeploymentTester();
+        maintainer = new DnsMaintainer(tester.controller(), Duration.ofHours(12),
+                                       new JobControl(new MockCuratorDb()),
+                                       tester.controllerTester().nameService());
+    }
+
     @Test
     public void removes_record_for_unassigned_rotation() {
-        DeploymentTester tester = new DeploymentTester();
         Application application = tester.createApplication("app1", "tenant1", 1, 1L);
-        DnsMaintainer maintainer = new DnsMaintainer(tester.controller(), Duration.ofHours(12),
-                                                     new JobControl(new MockCuratorDb()),
-                                                     tester.controllerTester().nameService());
 
         ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
                 .environment(Environment.prod)
@@ -53,7 +68,7 @@ public class DnsMaintainerTest {
 
         // Deploy application
         tester.deployCompletely(application, applicationPackage);
-        assertEquals(3, tester.controllerTester().nameService().records().size());
+        assertEquals(3, records().size());
 
         Optional<Record> record = findCname.apply("app1--tenant1.global.vespa.yahooapis.com");
         assertTrue(record.isPresent());
@@ -89,12 +104,39 @@ public class DnsMaintainerTest {
         tester.applications().deleteApplication(application.id(), Optional.of(new OktaAccessToken("okta-token")));
 
         // DnsMaintainer removes records
-        maintainer.maintain();
+        for (int i = 0; i < ControllerTester.availableRotations; i++) {
+            maintainer.maintain();
+        }
         assertFalse("DNS record removed", findCname.apply("app1--tenant1.global.vespa.yahooapis.com").isPresent());
-        maintainer.maintain();
         assertFalse("DNS record removed", findCname.apply("app1--tenant1.global.vespa.oath.cloud").isPresent());
-        maintainer.maintain();
         assertFalse("DNS record removed", findCname.apply("app1.tenant1.global.vespa.yahooapis.com").isPresent());
     }
 
+    @Test
+    public void rate_limit_record_removal() {
+        // Create stale records
+        int staleTotal = ControllerTester.availableRotations;
+        for (int i = 1; i <= staleTotal; i++) {
+            Rotation r = rotation(i);
+            tester.controllerTester().nameService().createCname(RecordName.from("stale-record-" + i + "." +
+                                                                                GlobalDnsName.OATH_DNS_SUFFIX),
+                                                                RecordData.from(r.name() + "."));
+        }
+
+        // One record is removed per run
+        for (int i = 1; i <= staleTotal*2; i++) {
+            maintainer.run();
+            assertEquals(Math.max(staleTotal - i, 0), records().size());
+        }
+    }
+
+    private Map<RecordId, Record> records() {
+        return tester.controllerTester().nameService().records();
+    }
+
+    private static Rotation rotation(int n) {
+        String id = String.format("%02d", n);
+        return new Rotation(new RotationId("rotation-id-" + id), "rotation-fqdn-" + id);
+    }
+
 }