Collect garbage on controller restart

4 files changed, 41 insertions, 9 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
index 631125d9368..05d5c737c9b 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
@@ -133,6 +133,9 @@ public class Controller extends AbstractComponent {
 
         // Record the version of this controller
         curator().writeControllerVersion(this.hostname(), Vtag.currentVersion);
+
+        jobController.collectGarbage();
+        jobController.updateStorage();
     }
     
     /** Returns the instance controlling tenants */
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 1ad89cb7a72..329f7ac62a4 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
@@ -5,7 +5,9 @@ import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
+import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
 import com.yahoo.vespa.hosted.controller.api.integration.LogStore;
+import com.yahoo.vespa.hosted.controller.api.integration.configserver.NoInstanceException;
 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.ApplicationVersion;
@@ -24,6 +26,7 @@ import java.util.function.UnaryOperator;
 import java.util.stream.Stream;
 
 import static com.google.common.collect.ImmutableList.copyOf;
+import static com.yahoo.vespa.hosted.controller.maintenance.InternalStepRunner.testerOf;
 
 /**
  * A singleton owned by the controller, which contains the state and methods for controlling deployment jobs.
@@ -198,18 +201,37 @@ public class JobController {
         });
     }
 
-    /** Unregisters the given application and deletes all associated data. */
+    /** Unregisters the given application and makes all associated data eligible for garbage collection. */
     public void unregister(ApplicationId id) {
         controller.applications().lockIfPresent(id, application -> {
             controller.applications().store(application.withBuiltInternally(false));
-            for (JobType type : jobs(id))
-                try (Lock __ = curator.lock(id, type)) {
-                    curator.deleteJobData(id, type);
-                    // TODO jvenstad: Deactivate tester applications?
-                }
         });
     }
 
+    public void collectGarbage() {
+        controller.applications().asList().stream()
+                  .filter(application -> ! application.deploymentJobs().builtInternally())
+                  .map(Application::id)
+                  .forEach(id -> {
+                      for (JobType type : jobs(id))
+                          try (Lock __ = curator.lock(id, type)) {
+                          if ( ! active(last(id, type).get().id()).isPresent())
+                              deactivateTester(id, type);
+                              curator.deleteJobData(id, type);
+                          }
+                  });
+    }
+
+    // TODO jvenstad: Urgh, clean this up somehow?
+    public void deactivateTester(ApplicationId id, JobType type) {
+        try {
+            controller.configServer().deactivate(new DeploymentId(testerOf(id), type.zone(controller.system()).get()));
+        }
+        catch (NoInstanceException ignored) {
+            // ok; already gone
+        }
+    }
+
     // TODO jvenstad: Find a more appropriate way of doing this, at least when this is the only build service.
     private long nextBuild(ApplicationId id) {
         return 1 + controller.applications().require(id).deploymentJobs()
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/InternalStepRunner.java
index 41d719f37ed..3ac880e436b 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/InternalStepRunner.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/InternalStepRunner.java
@@ -30,7 +30,8 @@ import static com.yahoo.vespa.hosted.controller.deployment.Step.Status.unfinishe
  */
 public class InternalStepRunner implements StepRunner {
 
-    private static ApplicationId testerOf(ApplicationId id) {
+    // TODO jvenstad: Move this tester logic to the application controller, perhaps?
+    public static ApplicationId testerOf(ApplicationId id) {
         return ApplicationId.from(id.tenant().value(),
                                   id.application().value(),
                                   id.instance().value() + "-t");
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java
index 67c1854e15a..a43b0e05f11 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java
@@ -32,6 +32,7 @@ import static com.yahoo.vespa.hosted.controller.deployment.Step.deactivateReal;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.deactivateTester;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.deployReal;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.deployTester;
+import static com.yahoo.vespa.hosted.controller.deployment.Step.installInitialReal;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.installReal;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.installTester;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.report;
@@ -51,8 +52,10 @@ public class JobRunnerTest {
     public void testMultiThreadedExecutionFinishes() throws InterruptedException {
         DeploymentTester tester = new DeploymentTester();
         JobController jobs = tester.controller().jobController();
+        // Fail the installation of the initial version of the real application in staging tests, and succeed everything else.
+        StepRunner stepRunner = (step, id) -> id.type() == stagingTest && step.get() == installInitialReal ? failed : succeeded;
         JobRunner runner = new JobRunner(tester.controller(), Duration.ofDays(1), new JobControl(tester.controller().curator()),
-                                         Executors.newFixedThreadPool(32), sleepy(new DummyStepRunner()));
+                                         Executors.newFixedThreadPool(32), sleepy(stepRunner));
 
         ApplicationId id = tester.createApplication("real", "tenant", 1, 1L).id();
         jobs.submit(id, new SourceRevision("repo", "branch", "bada55"), new byte[0], new byte[0]);
@@ -68,8 +71,11 @@ public class JobRunnerTest {
         assertTrue(jobs.last(id, systemTest).get().steps().values().stream().allMatch(unfinished::equals));
         runner.maintain();
         assertFalse(jobs.last(id, systemTest).get().hasEnded());
-        Thread.sleep(1000); // I'm so sorry, but I want to test this. Takes ~100ms "on my machine".
+        assertFalse(jobs.last(id, stagingTest).get().hasEnded());
+        Thread.sleep(500); // I'm so sorry, but I want to test this. Takes ~100ms "on my machine".
         assertTrue(jobs.last(id, systemTest).get().steps().values().stream().allMatch(succeeded::equals));
+        assertTrue(jobs.last(id, stagingTest).get().hasEnded());
+        assertTrue(jobs.last(id, stagingTest).get().hasFailed());
     }
 
     @Test