Act on the readable subset of applications when appropriate

14 files changed, 83 insertions, 39 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index 7cd4deb9bdb..e02c1b9f5dd 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -181,7 +181,18 @@ public class ApplicationController {
 
     /** Returns a snapshot of all applications */
     public List<Application> asList() {
-        return curator.readApplications();
+        return curator.readApplications(false);
+    }
+
+    /**
+     * Returns a snapshot of all readable applications. Unlike {@link ApplicationController#asList()} this ignores
+     * applications that cannot currently be read (e.g. due to serialization issues) and may return an incomplete
+     * snapshot.
+     *
+     * This should only be used in cases where acting on a subset of applications is better than none.
+     */
+    public List<Application> readable() {
+        return curator.readApplications(true);
     }
 
     /** Returns the ID of all known applications. */
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
index 615be3bcde1..9b71439e01d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
@@ -263,7 +263,7 @@ public class DeploymentTrigger {
 
     /** Returns the set of all jobs which have changes to propagate from the upstream steps. */
     private List<Job> computeReadyJobs() {
-        return jobs.deploymentStatuses(ApplicationList.from(applications().asList())
+        return jobs.deploymentStatuses(ApplicationList.from(applications().readable())
                                                       .withProjectId() // Need to keep this, as we have applications with deployment spec that shouldn't be orchestrated.
                                                       .withDeploymentSpec())
                    .withChanges()
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
index e696cb2d033..7c5c8d55e2a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
@@ -194,16 +194,9 @@ public class JobController {
         locked(id, run -> run.with(testerCertificate));
     }
 
-    /** Returns a list of all applications which have registered. */
-    public List<TenantAndApplicationId> applications() {
-        return copyOf(controller.applications().asList().stream()
-                                .map(Application::id)
-                                .iterator());
-    }
-
     /** Returns a list of all instances of applications which have registered. */
     public List<ApplicationId> instances() {
-        return copyOf(controller.applications().asList().stream()
+        return copyOf(controller.applications().readable().stream()
                                 .flatMap(application -> application.instances().values().stream())
                                 .map(Instance::id)
                                 .iterator());
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java
index 312d885cf53..9c426d23d82 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java
@@ -3,9 +3,9 @@ package com.yahoo.vespa.hosted.controller.maintenance;
 
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.Application;
-import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.ApplicationController;
 import com.yahoo.vespa.hosted.controller.Controller;
+import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.api.integration.organization.ApplicationSummary;
 import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId;
 import com.yahoo.vespa.hosted.controller.api.integration.organization.OwnershipIssues;
@@ -46,7 +46,7 @@ public class ApplicationOwnershipConfirmer extends Maintainer {
 
     /** File an ownership issue with the owners of all applications we know about. */
     private void confirmApplicationOwnerships() {
-        ApplicationList.from(controller().applications().asList())
+        applications()
                        .withProjectId()
                        .withProductionDeployment()
                        .asList()
@@ -86,7 +86,7 @@ public class ApplicationOwnershipConfirmer extends Maintainer {
 
     /** Escalate ownership issues which have not been closed before a defined amount of time has passed. */
     private void ensureConfirmationResponses() {
-        for (Application application : controller().applications().asList())
+        for (Application application : applications())
             application.ownershipIssueId().ifPresent(issueId -> {
                 try {
                     Tenant tenant = tenantOf(application.id());
@@ -99,7 +99,7 @@ public class ApplicationOwnershipConfirmer extends Maintainer {
     }
 
     private void updateConfirmedApplicationOwners() {
-        ApplicationList.from(controller().applications().asList())
+        applications()
                        .withProjectId()
                        .withProductionDeployment()
                        .asList()
@@ -114,6 +114,10 @@ public class ApplicationOwnershipConfirmer extends Maintainer {
                        });
     }
 
+    private ApplicationList applications() {
+        return ApplicationList.from(controller().applications().readable());
+    }
+
     private User determineAssignee(Application application) {
         return application.owner().orElse(null);
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java
index 7756e6b23a7..3160e7aef1d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java
@@ -24,7 +24,7 @@ public class DeploymentExpirer extends Maintainer {
 
     @Override
     protected void maintain() {
-        for (Application application : controller().applications().asList())
+        for (Application application : controller().applications().readable())
             for (Instance instance : application.instances().values())
                 for (Deployment deployment : instance.deployments().values()) {
                     if ( ! isExpired(deployment)) continue;
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java
index 3963040f6b5..94c3f3af98e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java
@@ -51,7 +51,7 @@ public class DeploymentIssueReporter extends Maintainer {
 
     /** Returns the applications to maintain issue status for. */
     private List<Application> applications() {
-        return ApplicationList.from(controller().applications().asList())
+        return ApplicationList.from(controller().applications().readable())
                               .withProjectId()
                               .asList();
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
index 808961257ad..1ce1e7f4c0c 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
@@ -46,7 +46,7 @@ public class DeploymentMetricsMaintainer extends Maintainer {
         // Run parallel stream inside a custom ForkJoinPool so that we can control the number of threads used
         ForkJoinPool pool = new ForkJoinPool(applicationsToUpdateInParallel);
         pool.submit(() ->
-            applications.asList().parallelStream().forEach(application -> {
+            applications.readable().parallelStream().forEach(application -> {
                 for (Instance instance : application.instances().values())
                     for (Deployment deployment : instance.deployments().values()) {
                         attempts.incrementAndGet();
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporter.java
index a4ace4cf518..4c1dd56ee64 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporter.java
@@ -71,8 +71,8 @@ public class MetricsReporter extends Maintainer {
     }
 
     private void reportDeploymentMetrics() {
-        ApplicationList applications = ApplicationList.from(controller().applications().asList())
-                                                  .withProductionDeployment();
+        ApplicationList applications = ApplicationList.from(controller().applications().readable())
+                                                      .withProductionDeployment();
         DeploymentStatusList deployments = controller().jobController().deploymentStatuses(applications);
 
         metric.set(DEPLOYMENT_FAIL_METRIC, deploymentFailRatio(deployments) * 100, metric.createContext(Map.of()));
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java
index 01bafc25816..83deb71e663 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java
@@ -20,7 +20,7 @@ public class OutstandingChangeDeployer extends Maintainer {
 
     @Override
     protected void maintain() {
-        for (Application application : ApplicationList.from(controller().applications().asList())
+        for (Application application : ApplicationList.from(controller().applications().readable())
                                                       .withProductionDeployment()
                                                       .withDeploymentSpec()
                                                       .asList())
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RotationStatusUpdater.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RotationStatusUpdater.java
index 1d1d5e93dd8..35131d3db80 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RotationStatusUpdater.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RotationStatusUpdater.java
@@ -44,9 +44,9 @@ public class RotationStatusUpdater extends Maintainer {
         var failures = new AtomicInteger(0);
         var attempts = new AtomicInteger(0);
         var lastException = new AtomicReference<Exception>(null);
-        var instancesWithRotations = ApplicationList.from(applications.asList()).hasRotation().asList().stream()
-                .flatMap(application -> application.instances().values().stream())
-                .filter(instance -> ! instance.rotations().isEmpty());
+        var instancesWithRotations = ApplicationList.from(applications.readable()).hasRotation().asList().stream()
+                                                    .flatMap(application -> application.instances().values().stream())
+                                                    .filter(instance -> ! instance.rotations().isEmpty());
 
         // Run parallel stream inside a custom ForkJoinPool so that we can control the number of threads used
         var pool = new ForkJoinPool(applicationsToUpdateInParallel);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
index ec975a6a5d5..1e22c59636a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
@@ -103,7 +103,7 @@ public class Upgrader extends Maintainer {
 
     /** Returns a list of all production application instances, except those which are pinned, which we should not manipulate here. */
     private InstanceList instances() {
-        return InstanceList.from(controller().jobController().deploymentStatuses(ApplicationList.from(controller().applications().asList())))
+        return InstanceList.from(controller().jobController().deploymentStatuses(ApplicationList.from(controller().applications().readable())))
                            .withProductionDeployment()
                            .unpinned();
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
index eb86b1028e2..4fdd66ee100 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
@@ -9,6 +9,7 @@ import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.zone.ZoneId;
+import com.yahoo.log.LogLevel;
 import com.yahoo.path.Path;
 import com.yahoo.slime.Slime;
 import com.yahoo.slime.SlimeUtils;
@@ -18,13 +19,13 @@ import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMetadata;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId;
-import com.yahoo.vespa.hosted.controller.routing.GlobalRouting;
-import com.yahoo.vespa.hosted.controller.routing.RoutingPolicy;
 import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLog;
 import com.yahoo.vespa.hosted.controller.deployment.Run;
 import com.yahoo.vespa.hosted.controller.deployment.Step;
 import com.yahoo.vespa.hosted.controller.dns.NameServiceQueue;
+import com.yahoo.vespa.hosted.controller.routing.GlobalRouting;
+import com.yahoo.vespa.hosted.controller.routing.RoutingPolicy;
 import com.yahoo.vespa.hosted.controller.routing.RoutingPolicyId;
 import com.yahoo.vespa.hosted.controller.routing.ZoneRoutingPolicy;
 import com.yahoo.vespa.hosted.controller.tenant.Tenant;
@@ -38,6 +39,7 @@ import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.nio.ByteBuffer;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
@@ -352,26 +354,37 @@ public class CuratorDb {
         return read(applicationPath(application), applicationSerializer::fromSlime);
     }
 
-    public List<Application> readApplications() {
-        return readApplications(ignored -> true);
+    public List<Application> readApplications(boolean canFail) {
+        return readApplications(ignored -> true, canFail);
     }
 
     public List<Application> readApplications(TenantName name) {
-        return readApplications(application -> application.tenant().equals(name));
-    }
-
-    private List<Application> readApplications(Predicate<TenantAndApplicationId> applicationFilter) {
-        return readApplicationIds().stream()
-                                   .filter(applicationFilter)
-                                   .sorted()
-                                   .map(this::readApplication)
-                                   .flatMap(Optional::stream)
-                                   .collect(Collectors.toUnmodifiableList());
+        return readApplications(application -> application.tenant().equals(name), false);
+    }
+
+    private List<Application> readApplications(Predicate<TenantAndApplicationId> applicationFilter, boolean canFail) {
+        var applicationIds = readApplicationIds();
+        var applications = new ArrayList<Application>(applicationIds.size());
+        for (var id : applicationIds) {
+            if (!applicationFilter.test(id)) continue;
+            try {
+                readApplication(id).ifPresent(applications::add);
+            } catch (Exception e) {
+                if (canFail) {
+                    log.log(LogLevel.ERROR, "Failed to read application '" + id + "', this must be fixed through " +
+                                            "manual intervention", e);
+                } else {
+                    throw e;
+                }
+            }
+        }
+        return Collections.unmodifiableList(applications);
     }
 
     public List<TenantAndApplicationId> readApplicationIds() {
         return curator.getChildren(applicationRoot).stream()
                       .map(TenantAndApplicationId::fromSerialized)
+                      .sorted()
                       .collect(toUnmodifiableList());
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java
index d27bc581f75..5dde9516ab5 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java
@@ -89,7 +89,7 @@ public class DeploymentApiHandler extends LoggingRequestHandler {
         Cursor platformArray = root.setArray("versions");
         var versionStatus = controller.versionStatus();
         var systemVersion = versionStatus.systemVersion().map(VespaVersion::versionNumber).orElse(Vtag.currentVersion);
-        Map<ApplicationId, JobList> jobs = controller.jobController().deploymentStatuses(ApplicationList.from(controller.applications().asList()), systemVersion)
+        Map<ApplicationId, JobList> jobs = controller.jobController().deploymentStatuses(ApplicationList.from(controller.applications().readable()), systemVersion)
                                                      .asList().stream()
                                                      .flatMap(status -> status.instanceJobs().entrySet().stream())
                                                      .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, Map.Entry::getValue));
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index ad8c7d7c328..c04f745c043 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -16,6 +16,7 @@ import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.zone.RoutingMethod;
 import com.yahoo.config.provision.zone.ZoneId;
+import com.yahoo.path.Path;
 import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions;
@@ -36,6 +37,7 @@ import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentContext;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester;
 import com.yahoo.vespa.hosted.controller.integration.ZoneApiMock;
+import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb;
 import com.yahoo.vespa.hosted.controller.rotation.RotationId;
 import com.yahoo.vespa.hosted.controller.rotation.RotationLock;
 import org.junit.Test;
@@ -939,4 +941,25 @@ public class ControllerTest {
                                                        .rotations().get(0).clusterId());
     }
 
+    @Test
+    public void testReadableApplications() {
+        var db = new MockCuratorDb();
+        var tester = new DeploymentTester(new ControllerTester(db));
+
+        // Create two applications
+        tester.newDeploymentContext("t1", "a1", "default");
+        tester.newDeploymentContext("t2", "a2", "default");
+        assertEquals(2, tester.applications().readable().size());
+
+        // Write invalid data to one application
+        db.curator().set(Path.fromString("/controller/v1/applications/t2:a2"), new byte[]{(byte) 0xDE, (byte) 0xAD});
+        assertEquals(1, tester.applications().readable().size());
+
+        try {
+            tester.applications().asList();
+            fail("Expected exception");
+        } catch (Exception ignored) {
+        }
+    }
+
 }