Read from new application paths, and change deletion of instances, with updated tests

4 files changed, 122 insertions, 65 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 bb28839cb79..ba9f8e809c8 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
@@ -742,7 +742,6 @@ public class ApplicationController {
                                                    application.get().require(applicationId.instance()).deployments().keySet().stream().map(ZoneId::toString)
                                                               .sorted().collect(Collectors.joining(", ")));
 
-            curator.removeApplication(applicationId);
             applicationStore.removeAll(applicationId);
             applicationStore.removeAll(TesterId.of(applicationId));
 
@@ -755,6 +754,7 @@ public class ApplicationController {
                              controller.nameServiceForwarder().removeRecords(Record.Type.CNAME, RecordName.from(name), Priority.normal);
                          });
             });
+            curator.storeWithoutInstance(application.without(applicationId.instance()).get(), applicationId);
 
             log.info("Deleted " + application);
         });
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 a3de98a9582..76a146fb1e6 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
@@ -15,7 +15,6 @@ import com.yahoo.vespa.config.SlimeUtils;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.Application;
-import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.api.integration.certificates.ApplicationCertificate;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId;
@@ -345,8 +344,7 @@ public class CuratorDb {
     }
 
     public Optional<Application> readApplication(TenantAndApplicationId application) {
-        List<Instance> instances = readInstances(id -> TenantAndApplicationId.from(id).equals(application));
-        return Application.aggregate(instances);
+        return readSlime(applicationPath(application)).map(applicationSerializer::fromSlime);
     }
 
     public List<Application> readApplications() {
@@ -357,41 +355,28 @@ public class CuratorDb {
         return readApplications(application -> application.tenant().equals(name));
     }
 
-    private Stream<TenantAndApplicationId> readTenantAndApplicationIds() {
-        return readInstanceIds().map(TenantAndApplicationId::from).distinct();
-    }
-
     private List<Application> readApplications(Predicate<TenantAndApplicationId> applicationFilter) {
-        return readTenantAndApplicationIds().filter(applicationFilter)
-                                            .sorted()
-                                            .map(this::readApplication)
-                                            .flatMap(Optional::stream)
-                                            .collect(Collectors.toUnmodifiableList());
-    }
-
-    private Optional<Instance> readInstance(ApplicationId application) {
-        return readSlime(oldApplicationPath(application)).map(instanceSerializer::fromSlime);
+        return readApplicationIds().filter(applicationFilter)
+                                   .sorted()
+                                   .map(this::readApplication)
+                                   .flatMap(Optional::stream)
+                                   .collect(Collectors.toUnmodifiableList());
     }
 
-    private Stream<ApplicationId> readInstanceIds() {
+    // TODO jonmv: Clear out old instance data here
+    private Stream<TenantAndApplicationId> readApplicationIds() {
         return curator.getChildren(applicationRoot).stream()
-                      .filter(id -> id.split(":").length == 3)
-                      .map(ApplicationId::fromSerializedForm);
+                      .filter(id -> id.split(":").length == 2)
+                      .map(TenantAndApplicationId::fromSerialized);
     }
 
-    private List<Instance> readInstances(Predicate<ApplicationId> applicationFilter) {
-        return readInstanceIds().filter(applicationFilter)
-                                .sorted()
-                                .map(this::readInstance)
-                                .flatMap(Optional::stream)
-                                .collect(Collectors.toUnmodifiableList());
-    }
-
-    public void removeApplication(ApplicationId id) {
-        // WARNING: This is part of a multi-step data move operation, so don't touch!!!
-        curator.delete(oldApplicationPath(id));
-        if (readApplication(TenantAndApplicationId.from(id)).isEmpty())
-            curator.delete(applicationPath(TenantAndApplicationId.from(id)));
+    // TODO jonmv: Refactor when instance split operation is done
+    public void storeWithoutInstance(Application application, ApplicationId instanceId) {
+        curator.delete(oldApplicationPath(instanceId));
+        if (application.instances().isEmpty())
+                curator.delete(applicationPath(application.id()));
+        else
+            writeApplication(application);
     }
 
     /**
@@ -415,7 +400,7 @@ public class CuratorDb {
      * Remove everything under instance root                                    DONE
      * Stop locking applications on instance level                              DONE
      *
-     * Read Application with instances from application path (with filter)
+     * Read Application with instances from application path (with filter)      DONE
      *
      * Stop writing Instance to old application path
      * Remove unused parts of Instance (Used only for legacy serialization)
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 5d82225c9d5..075451f60bf 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
@@ -853,18 +853,19 @@ public class ControllerTest {
         MockCuratorDb newDb = new MockCuratorDb();
         OldMockCuratorDb oldDb = new OldMockCuratorDb(newDb.curator());
 
-        oldDb.writeInstance(old1);
-        oldDb.writeInstance(old2);
+        oldDb.writeApplication(Application.aggregate(List.of(old1, old2)).orElseThrow());
 
-        Application application = newDb.readApplication(TenantAndApplicationId.from("t1", "a1")).orElseThrow();
+        Application application = oldDb.readApplication(TenantAndApplicationId.from("t1", "a1")).orElseThrow();
         Instance new1 = application.legacy(InstanceName.defaultName());
         String new1Serialized = new String(JsonFormat.toJsonBytes(instanceSerializer.toSlime(new1)), UTF_8);
         assertEquals(old1Serialized, new1Serialized);
 
         LockedApplication locked = new LockedApplication(application, newDb.lock(application.id()));
-        newDb.writeApplication(locked.with(new ApplicationMetrics(8, 9)).get());
-        Instance mod1 = oldDb.readInstance(old1.id()).orElseThrow();
-        Instance mod2 = oldDb.readInstance(old2.id()).orElseThrow();
+        oldDb.writeApplication(locked.with(new ApplicationMetrics(8, 9)).get());
+
+        Application newApp = newDb.readApplication(application.id()).orElseThrow();
+        Instance mod1 = newApp.legacy(old1.name());
+        Instance mod2 = newApp.legacy(old2.name());
 
         old1 = old1.with(new ApplicationMetrics(8, 9));
         old1Serialized = new String(JsonFormat.toJsonBytes(instanceSerializer.toSlime(old1)), UTF_8);
@@ -878,12 +879,14 @@ public class ControllerTest {
         assertEquals(old2.deployments(), mod2.deployments());
         assertEquals(old2.deploymentJobs().jobStatus(), mod2.deploymentJobs().jobStatus());
 
-        newDb.removeApplication(old1.id());
+        application = new LockedApplication(application, newDb.lock(application.id())).without(old1.name()).get();
+        newDb.storeWithoutInstance(application, old1.id());
         assertEquals(1, newDb.readApplication(application.id()).orElseThrow().instances().size());
-        newDb.removeApplication(old2.id());
+        assertEquals(1, oldDb.readApplication(application.id()).orElseThrow().instances().size());
+        application = new LockedApplication(application, newDb.lock(application.id())).without(old2.name()).get();
+        newDb.storeWithoutInstance(application, old2.id());
         assertTrue(newDb.readApplication(application.id()).isEmpty());
-        assertTrue(oldDb.readInstance(old1.id()).isEmpty());
-        assertTrue(oldDb.readInstance(old2.id()).isEmpty());
+        assertTrue(oldDb.readApplication(application.id()).isEmpty());
     }
 
     private void runUpgrade(DeploymentTester tester, ApplicationId application, ApplicationVersion version) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OldCuratorDb.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OldCuratorDb.java
index 8aea72a9ac4..f7fdd91d74e 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OldCuratorDb.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OldCuratorDb.java
@@ -6,6 +6,7 @@ import com.google.inject.Inject;
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.HostName;
+import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.path.Path;
@@ -13,11 +14,13 @@ import com.yahoo.slime.Slime;
 import com.yahoo.vespa.config.SlimeUtils;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.Lock;
+import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.api.integration.certificates.ApplicationCertificate;
 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.application.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;
@@ -63,7 +66,7 @@ import static java.util.stream.Collectors.collectingAndThen;
  */
 public class OldCuratorDb {
 
-    private static final Logger log = Logger.getLogger(OldCuratorDb.class.getName());
+    private static final Logger log = Logger.getLogger(CuratorDb.class.getName());
     private static final Duration deployLockTimeout = Duration.ofMinutes(30);
     private static final Duration defaultLockTimeout = Duration.ofMinutes(5);
     private static final Duration defaultTryLockTimeout = Duration.ofSeconds(1);
@@ -83,6 +86,7 @@ public class OldCuratorDb {
     private final ControllerVersionSerializer controllerVersionSerializer = new ControllerVersionSerializer();
     private final ConfidenceOverrideSerializer confidenceOverrideSerializer = new ConfidenceOverrideSerializer();
     private final TenantSerializer tenantSerializer = new TenantSerializer();
+    private final ApplicationSerializer applicationSerializer = new ApplicationSerializer();
     private final InstanceSerializer instanceSerializer = new InstanceSerializer();
     private final RunSerializer runSerializer = new RunSerializer();
     private final OsVersionSerializer osVersionSerializer = new OsVersionSerializer();
@@ -133,6 +137,10 @@ public class OldCuratorDb {
         return lock(lockPath(name), defaultLockTimeout.multipliedBy(2));
     }
 
+    public Lock lock(TenantAndApplicationId id) {
+        return lock(lockPath(id), defaultLockTimeout.multipliedBy(2));
+    }
+
     public Lock lock(ApplicationId id) {
         return lock(lockPath(id), defaultLockTimeout.multipliedBy(2));
     }
@@ -329,45 +337,97 @@ public class OldCuratorDb {
         curator.delete(tenantPath(name));
     }
 
-    // -------------- Application ---------------------------------------------
+    // -------------- Applications ---------------------------------------------
 
-    public void writeInstance(Instance instance) {
-        curator.set(applicationPath(instance.id()), asJson(instanceSerializer.toSlime(instance)));
-        curator.set(instancePath(instance.id()), asJson(instanceSerializer.toSlime(instance)));
+    public void writeApplication(Application application) {
+        curator.set(applicationPath(application.id()), asJson(applicationSerializer.toSlime(application)));
+        for (InstanceName name : application.instances().keySet()) {
+            curator.set(oldApplicationPath(application.id().instance(name)),
+                        asJson(instanceSerializer.toSlime(application.legacy(name))));
+        }
     }
 
-    public Optional<Instance> readInstance(ApplicationId application) {
-        return readSlime(applicationPath(application)).map(instanceSerializer::fromSlime);
+    public Optional<Application> readApplication(TenantAndApplicationId application) {
+        List<Instance> instances = readInstances(id -> TenantAndApplicationId.from(id).equals(application));
+        return Application.aggregate(instances);
     }
 
-    public List<Instance> readInstances() {
-        return readInstances(ignored -> true);
+    public List<Application> readApplications() {
+        return readApplications(ignored -> true);
     }
 
-    public List<Instance> readInstances(TenantName name) {
-        return readInstances(application -> application.tenant().equals(name));
+    public List<Application> readApplications(TenantName name) {
+        return readApplications(application -> application.tenant().equals(name));
+    }
+
+    private Stream<TenantAndApplicationId> readTenantAndApplicationIds() {
+        return readInstanceIds().map(TenantAndApplicationId::from).distinct();
+    }
+
+    private List<Application> readApplications(Predicate<TenantAndApplicationId> applicationFilter) {
+        return readTenantAndApplicationIds().filter(applicationFilter)
+                                            .sorted()
+                                            .map(this::readApplication)
+                                            .flatMap(Optional::stream)
+                                            .collect(Collectors.toUnmodifiableList());
+    }
+
+    private Optional<Instance> readInstance(ApplicationId application) {
+        return readSlime(oldApplicationPath(application)).map(instanceSerializer::fromSlime);
     }
 
     private Stream<ApplicationId> readInstanceIds() {
         return curator.getChildren(applicationRoot).stream()
                       .filter(id -> id.split(":").length == 3)
-                      .distinct()
                       .map(ApplicationId::fromSerializedForm);
     }
 
-    private List<Instance> readInstances(Predicate<ApplicationId> instanceFilter) {
-        return readInstanceIds().filter(instanceFilter)
+    private List<Instance> readInstances(Predicate<ApplicationId> applicationFilter) {
+        return readInstanceIds().filter(applicationFilter)
+                                .sorted()
                                 .map(this::readInstance)
                                 .flatMap(Optional::stream)
                                 .collect(Collectors.toUnmodifiableList());
     }
 
-    public void removeInstance(ApplicationId id) {
+    public void removeApplication(ApplicationId id) {
         // WARNING: This is part of a multi-step data move operation, so don't touch!!!
-        curator.delete(applicationPath(id));
-        curator.delete(instancePath(id));
+        curator.delete(oldApplicationPath(id));
+        if (readApplication(TenantAndApplicationId.from(id)).isEmpty())
+            curator.delete(applicationPath(TenantAndApplicationId.from(id)));
     }
 
+    public void clearInstanceRoot() {
+        curator.delete(instanceRoot);
+    }
+
+    /**
+     * Migration plan:
+     *
+     * Add filter for reading only Instance from old application path           RELEASED
+     * Write Instance to instance and old application path                      RELEASED
+     *
+     * Lock on application level for instance mutations                         MERGED
+     *
+     * Write Instance to instance and application and old application paths     DONE    TO CHANGE   DONE
+     * Read Instance from instance path                                         DONE    TO REMOVE   DONE
+     * Duplicate Application from Instance, with helper classes                 DONE
+     * Write Application to instance and application and old application paths  DONE    TO CHANGE   DONE
+     * Read Application from instance path                                      DONE    TO REMOVE   DONE
+     * Use Application where applicable                                         DONE    !!!
+     * Lock instances and application on same level: tenant + application       DONE    TO CHANGE   DONE
+     * When reading an application, read all instances, and aggregate them      DONE
+     * Write application with instances to application path                     DONE
+     * Write all instances of an application to old application path            DONE
+     * Remove everything under instance root                                    DONE
+     *
+     * Read Application with instances from application path (with filter)
+     * Stop locking applications on instance level
+     *
+     * Stop writing Instance to old application path
+     * Remove unused parts of Instance (Used only for legacy serialization)
+     */
+
     // -------------- Job Runs ------------------------------------------------
 
     public void writeLastRun(Run run) {
@@ -503,10 +563,14 @@ public class OldCuratorDb {
                 .append(tenant.value());
     }
 
-    private Path lockPath(ApplicationId application) {
+    private Path lockPath(TenantAndApplicationId application) {
         return lockPath(application.tenant())
-                .append(application.application().value())
-                .append(application.instance().value());
+                .append(application.application().value());
+    }
+
+    private Path lockPath(ApplicationId instance) {
+        return lockPath(TenantAndApplicationId.from(instance))
+                .append(instance.instance().value());
     }
 
     private Path lockPath(ApplicationId application, ZoneId zone) {
@@ -583,7 +647,11 @@ public class OldCuratorDb {
         return tenantRoot.append(name.value());
     }
 
-    private static Path applicationPath(ApplicationId application) {
+    private static Path applicationPath(TenantAndApplicationId id) {
+        return applicationRoot.append(id.serialized());
+    }
+
+    private static Path oldApplicationPath(ApplicationId application) {
         return applicationRoot.append(application.serializedForm());
     }
 
@@ -616,3 +684,4 @@ public class OldCuratorDb {
     }
 
 }
+