Trigger periodic redeployment for apps with readied reindexing

7 files changed, 75 insertions, 24 deletions

diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/Deployer.java b/config-provisioning/src/main/java/com/yahoo/config/provision/Deployer.java
index e9ceb34570e..5a077d18e89 100644
--- a/config-provisioning/src/main/java/com/yahoo/config/provision/Deployer.java
+++ b/config-provisioning/src/main/java/com/yahoo/config/provision/Deployer.java
@@ -60,6 +60,9 @@ public interface Deployer {
     /** Returns the time of last deployed session for this application or empty if there are no deployments */
     Optional<Instant> deployTime(ApplicationId application);
 
+    /** Returns whether the application has reindexing ready to go, which was readied after the given instant. */
+    boolean readiedReindexingAfter(ApplicationId application, Instant instant);
+
     /** Whether the deployer is bootstrapping, some users of the deployer will want to hold off with deployments in that case. */
     default boolean bootstrapping() { return false; }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
index acc195729e0..9533f04107d 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
@@ -45,6 +45,7 @@ import com.yahoo.vespa.config.server.application.Application;
 import com.yahoo.vespa.config.server.application.ApplicationCuratorDatabase;
 import com.yahoo.vespa.config.server.application.ApplicationData;
 import com.yahoo.vespa.config.server.application.ApplicationReindexing;
+import com.yahoo.vespa.config.server.application.ApplicationReindexing.Status;
 import com.yahoo.vespa.config.server.application.ApplicationVersions;
 import com.yahoo.vespa.config.server.application.ClusterReindexing;
 import com.yahoo.vespa.config.server.application.ClusterReindexingStatusClient;
@@ -128,6 +129,7 @@ import static com.yahoo.vespa.config.server.tenant.TenantRepository.HOSTED_VESPA
 import static com.yahoo.vespa.curator.Curator.CompletionWaiter;
 import static com.yahoo.yolean.Exceptions.uncheck;
 import static java.nio.file.Files.readAttributes;
+import static java.util.Comparator.naturalOrder;
 
 /**
  * The API for managing applications.
@@ -468,6 +470,17 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         return Optional.of(createTime);
     }
 
+    @Override
+    public boolean readiedReindexingAfter(ApplicationId id, Instant instant) {
+        Tenant tenant = tenantRepository.getTenant(id.tenant());
+        if (tenant == null) return false;
+
+        return tenant.getApplicationRepo().database().readReindexingStatus(id)
+                     .flatMap(ApplicationReindexing::lastReadiedAt)
+                     .map(readiedAt -> readiedAt.isAfter(instant))
+                     .orElse(false);
+    }
+
     public ApplicationId activate(Tenant tenant,
                                   long sessionId,
                                   TimeoutBudget timeoutBudget,
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationReindexing.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationReindexing.java
index 5ecc7e6c8aa..4c32cccdf20 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationReindexing.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ApplicationReindexing.java
@@ -6,11 +6,14 @@ import com.yahoo.vespa.config.server.maintenance.ReindexingMaintainer;
 
 import java.time.Instant;
 import java.time.temporal.ChronoUnit;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 
+import static java.util.Comparator.naturalOrder;
+
 /**
  * Pending reindexing: convergence to the stored config generation allows reindexing to start.
  * Ready reindexing: reindexing may start after this timestamp.
@@ -94,6 +97,16 @@ public class ApplicationReindexing implements Reindexing {
         return Optional.ofNullable(clusters.get(clusterName)).map(cluster -> cluster.ready().get(documentType));
     }
 
+    /** Instant at which reindexing in this was last readied, unless no reindexing is still pending, in which case this is empty. */
+    public Optional<Instant> lastReadiedAt() {
+        if ( ! enabled) return Optional.empty();
+        if (clusters.values().stream().anyMatch(cluster -> ! cluster.pending().isEmpty())) return Optional.empty();
+        return clusters.values().stream()
+                       .flatMap(cluster -> cluster.ready().values().stream())
+                       .map(Reindexing.Status::ready)
+                       .max(naturalOrder());
+    }
+
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationReindexingTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationReindexingTest.java
index 80ac28e9dbc..972bd86d752 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationReindexingTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/ApplicationReindexingTest.java
@@ -41,6 +41,11 @@ public class ApplicationReindexingTest {
         assertEquals(Optional.empty(),
                      reindexing.status("three", "a"));
 
+        assertEquals(Optional.empty(),
+                     reindexing.lastReadiedAt());
+        assertEquals(Optional.of(Instant.ofEpochMilli(3)),
+                     reindexing.withoutPending("two", "b").lastReadiedAt());
+
         // Remove "a" in "one", and "one" entirely.
         assertEquals(Optional.empty(),
                      reindexing.without("one", "a").status("one", "a"));
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainer.java
index ba71bbf0d42..14693c75436 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainer.java
@@ -13,9 +13,7 @@ import com.yahoo.vespa.hosted.provision.NodeList;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 
 import java.time.Duration;
-import java.time.Instant;
 import java.util.Map;
-import java.util.function.Function;
 
 import static java.util.stream.Collectors.toMap;
 
@@ -41,34 +39,21 @@ public class PeriodicApplicationMaintainer extends ApplicationMaintainer {
     @Override
     protected boolean canDeployNow(ApplicationId application) {
         return deployer().deployTime(application)
-                // Don't deploy if a regular deploy just happened
-                .map(lastDeployTime -> lastDeployTime.isBefore(nodeRepository().clock().instant().minus(minTimeBetweenRedeployments)))
-                // We only know last deploy time for applications that were deployed on this config server,
-                // the rest will be deployed on another config server
-                .orElse(false);
+                         .map(lastDeployTime ->    lastDeployTime.isBefore(nodeRepository().clock().instant().minus(minTimeBetweenRedeployments))
+                                                || deployer().readiedReindexingAfter(application, lastDeployTime))
+                         .orElse(false);
     }
 
     @Override
     protected Map<ApplicationId, String> applicationsNeedingMaintenance() {
         if (deployer().bootstrapping()) return Map.of();
 
-        // Collect all deployment times before sorting as deployments may happen while we build the set, breaking
-        // the comparable contract. Stale times are fine as the time is rechecked in ApplicationMaintainer#deployNow
-        Map<ApplicationId, Instant> deploymentTimes = nodesNeedingMaintenance().stream()
-                                                                               .map(node -> node.allocation().get().owner())
-                                                                               .distinct()
-                                                                               .filter(this::canDeployNow)
-                                                                               .collect(toMap(Function.identity(), this::deployTime));
-
-        return deploymentTimes.entrySet().stream()
-                              .sorted(Map.Entry.comparingByValue())
-                              .map(Map.Entry::getKey)
-                              .filter(this::shouldMaintain)
-                              .collect(toMap(applicationId -> applicationId, applicationId -> "current deployment being too old"));
-    }
-
-    private Instant deployTime(ApplicationId applicationId) {
-        return deployer().deployTime(applicationId).orElse(Instant.EPOCH);
+        return nodesNeedingMaintenance().stream()
+                                        .map(node -> node.allocation().get().owner())
+                                        .distinct()
+                                        .filter(this::shouldMaintain)
+                                        .filter(this::canDeployNow)
+                                        .collect(toMap(applicationId -> applicationId, applicationId -> "current deployment being too old"));
     }
 
     private boolean shouldMaintain(ApplicationId id) {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java
index 4b4e9121be4..0573c6a877e 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockDeployer.java
@@ -50,6 +50,7 @@ public class MockDeployer implements Deployer {
 
     private boolean failActivate = false;
     private boolean bootstrapping = true;
+    private Instant readiedReindexingAt = null;
 
     /** Create a mock deployer which returns empty on every deploy request. */
     @Inject
@@ -91,6 +92,8 @@ public class MockDeployer implements Deployer {
 
     public void setBootstrapping(boolean bootstrapping) { this.bootstrapping = bootstrapping; }
 
+    public void setReadiedReindexingAt(Instant readiedReindexingAt) { this.readiedReindexingAt = readiedReindexingAt; }
+
     @Override
     public Optional<Deployment> deployFromLocalActive(ApplicationId id, boolean bootstrap) {
         return deployFromLocalActive(id, Duration.ofSeconds(60));
@@ -131,6 +134,11 @@ public class MockDeployer implements Deployer {
     }
 
     @Override
+    public boolean readiedReindexingAfter(ApplicationId application, Instant instant) {
+        return readiedReindexingAt != null && readiedReindexingAt.isAfter(instant);
+    }
+
+    @Override
     public boolean bootstrapping() {
         return bootstrapping;
     }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
index 9fea27a2cb0..f92526282d1 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
@@ -182,6 +182,30 @@ public class PeriodicApplicationMaintainerTest {
     }
 
     @Test(timeout = 60_000)
+    public void application_deploy_triggered_by_reindexing_ready() {
+        fixture.activate();
+
+        assertEquals("No deployment expected", 2, fixture.deployer.activations);
+
+        // Holds off on deployments a while after starting
+        fixture.setBootstrapping(false);
+        fixture.runApplicationMaintainer();
+        assertEquals("No deployment expected", 2, fixture.deployer.activations);
+
+        Instant firstDeployTime = clock.instant();
+
+        // Reindexing readied before last deploy time triggers nothing.
+        fixture.deployer.setReadiedReindexingAt(firstDeployTime.minusSeconds(1));
+        fixture.runApplicationMaintainer();
+        assertEquals("No deployment expected", 2, fixture.deployer.activations);
+
+        // Reindexing readied after last deploy time triggers nothing.
+        fixture.deployer.setReadiedReindexingAt(firstDeployTime.plusSeconds(1));
+        fixture.runApplicationMaintainer();
+        assertEquals("No deployment expected", 4, fixture.deployer.activations);
+    }
+
+    @Test(timeout = 60_000)
     public void queues_all_eligible_applications_for_deployment() throws Exception {
         fixture.activate();