Thorough GC of job resources

11 files changed, 59 insertions, 35 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 05d5c737c9b..790d6d00035 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
@@ -134,7 +134,6 @@ public class Controller extends AbstractComponent {
         // Record the version of this controller
         curator().writeControllerVersion(this.hostname(), Vtag.currentVersion);
 
-        jobController.collectGarbage();
         jobController.updateStorage();
     }
     
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 329f7ac62a4..abcde079271 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
@@ -16,9 +16,11 @@ import com.yahoo.vespa.hosted.controller.application.JobStatus;
 import com.yahoo.vespa.hosted.controller.application.SourceRevision;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
@@ -59,8 +61,8 @@ public class JobController {
         for (ApplicationId id : applications())
             for (JobType type : jobs(id)) {
                 locked(id, type, runs -> {
+                    last(id, type).ifPresent(last -> locked(last.id(), run -> run));
                 });
-                last(id, type).ifPresent(last -> locked(last.id(), run -> run));
             }
     }
 
@@ -205,21 +207,33 @@ public class JobController {
     public void unregister(ApplicationId id) {
         controller.applications().lockIfPresent(id, application -> {
             controller.applications().store(application.withBuiltInternally(false));
+            jobs(id).forEach(type -> {
+                try (Lock __ = curator.lock(id, type)) {
+                    last(id, type).ifPresent(last -> active(last.id()).ifPresent(active -> finish(active.id())));
+                }
+            });
         });
     }
 
+    /** Deletes stale data and tester deployments for applications which are unknown, or no longer built internally. */
     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);
-                          }
-                  });
+        Set<ApplicationId> applicationsToBuild = new HashSet<>(applications());
+        curator.applicationsWithJobs().stream()
+               .filter(id -> ! applicationsToBuild.contains(id))
+               .forEach(id -> {
+                   try {
+                       for (JobType type : jobs(id))
+                           try (Lock __ = curator.lock(id, type);
+                                Lock ___ = curator.lock(id, type, Step.deactivateTester)) {
+                               deactivateTester(id, type);
+                               curator.deleteJobData(id, type);
+                           }
+                   }
+                   catch (TimeoutException e) {
+                       return; // Don't remove the data if we couldn't deactivate all testers.
+                   }
+                   curator.deleteJobData(id);
+               });
     }
 
     // TODO jvenstad: Urgh, clean this up somehow?
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobProfile.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobProfile.java
index c36e0b3a39f..120c4f282ec 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobProfile.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobProfile.java
@@ -15,12 +15,13 @@ import static com.yahoo.vespa.hosted.controller.deployment.Step.*;
  */
 public enum JobProfile {
 
+    // TODO jvenstad: runTests is not a run-always step, as it really means: check if tests are done, and store whatever is ready.
     systemTest(EnumSet.of(deployReal,
                           installReal,
                           deployTester,
                           installTester,
                           startTests),
-               EnumSet.of(runTests,
+               EnumSet.of(endTests,
                           deactivateTester,
                           deactivateReal,
                           report)),
@@ -32,7 +33,7 @@ public enum JobProfile {
                            deployTester,
                            installTester,
                            startTests),
-                EnumSet.of(runTests,
+                EnumSet.of(endTests,
                            deactivateTester,
                            deactivateReal,
                            report)),
@@ -42,7 +43,7 @@ public enum JobProfile {
                           deployTester,
                           installTester,
                           startTests),
-               EnumSet.of(runTests,
+               EnumSet.of(endTests,
                           deactivateTester,
                           report));
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Step.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Step.java
index 2048c5ab353..1ef49ee7499 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Step.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Step.java
@@ -2,7 +2,6 @@ package com.yahoo.vespa.hosted.controller.deployment;
 
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.EnumSet;
 import java.util.List;
 
 /**
@@ -46,16 +45,16 @@ public enum Step {
     /** Ask the tester to run its tests. */
     startTests(installReal, installTester),
 
-    /** See that the tests are done running and store the test results. */
-    runTests(startTests),
+    /** See that the tests are done running. */
+    endTests(startTests),
 
     /** Delete the real application -- used for test deployments. */
-    deactivateReal(deployInitialReal, deployReal, runTests),
+    deactivateReal(deployInitialReal, deployReal, endTests),
 
     /** Deactivate the tester. */
-    deactivateTester(deployTester, runTests),
+    deactivateTester(deployTester, endTests),
 
-    /** Report completion to deployment orchestration machinery. */
+    /** Report completion to the deployment orchestration machinery. */
     report(deactivateReal, deactivateTester);
 
 
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 3ac880e436b..cf20084a1d3 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
@@ -53,7 +53,7 @@ public class InternalStepRunner implements StepRunner {
             case installReal: return installReal(id);
             case installTester: return installTester(id);
             case startTests: return startTests(id);
-            case runTests: return storeData(id);
+            case endTests: return endTests(id);
             case deactivateReal: return deactivateReal(id);
             case deactivateTester: return deactivateTester(id);
             case report: return report(id);
@@ -139,7 +139,7 @@ public class InternalStepRunner implements StepRunner {
         throw new AssertionError();
     }
 
-    private Status storeData(RunId id) {
+    private Status endTests(RunId id) {
         // Update test logs.
         // If tests are done, return test results.
         throw new AssertionError();
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java
index 1434c40bdee..6649f705821 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java
@@ -47,6 +47,7 @@ public class JobRunner extends Maintainer {
     @Override
     protected void maintain() {
         jobs.active().forEach(this::advance);
+        jobs.collectGarbage();
     }
 
     @Override
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 dcbc9479845..49e8d7498ab 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
@@ -329,6 +329,16 @@ public class CuratorDb {
         curator.delete(lastRunPath(id, type));
     }
 
+    public void deleteJobData(ApplicationId id) {
+        curator.delete(jobRoot.append(id.serializedForm()));
+    }
+
+    public List<ApplicationId> applicationsWithJobs() {
+        return curator.getChildren(jobRoot).stream()
+                      .map(ApplicationId::fromSerializedForm)
+                      .collect(Collectors.toList());
+    }
+
     // -------------- Provisioning (called by internal code) ------------------
 
     @SuppressWarnings("unused")
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java
index c7d782c8302..7df60278390 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java
@@ -31,7 +31,7 @@ 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;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.startTests;
-import static com.yahoo.vespa.hosted.controller.deployment.Step.runTests;
+import static com.yahoo.vespa.hosted.controller.deployment.Step.endTests;
 
 /**
  * Serialises and deserialises RunStatus objects for persistent storage.
@@ -113,7 +113,7 @@ public class RunSerializer {
             case installTester      : return "IT" ;
             case deactivateTester   : return "DAT";
             case startTests         : return "ST" ;
-            case runTests           : return "RT" ;
+            case endTests           : return "ET" ;
             case report             : return "R"  ;
             default                 : throw new AssertionError("No value defined for '" + step + "'!");
         }
@@ -130,7 +130,7 @@ public class RunSerializer {
             case "IT"  : return installTester     ;
             case "DAT" : return deactivateTester  ;
             case "ST"  : return startTests        ;
-            case "RT"  : return runTests;
+            case "ET"  : return endTests          ;
             case "R"   : return report            ;
             default    : throw new IllegalArgumentException("No step defined by '" + step + "'!");
         }
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 a43b0e05f11..406e64feb24 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
@@ -37,7 +37,7 @@ 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;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.startTests;
-import static com.yahoo.vespa.hosted.controller.deployment.Step.runTests;
+import static com.yahoo.vespa.hosted.controller.deployment.Step.endTests;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -49,7 +49,7 @@ import static org.junit.Assert.fail;
 public class JobRunnerTest {
 
     @Test
-    public void testMultiThreadedExecutionFinishes() throws InterruptedException {
+    public void multiThreadedExecutionFinishes() 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.
@@ -79,7 +79,7 @@ public class JobRunnerTest {
     }
 
     @Test
-    public void testStepLogic() {
+    public void stepLogic() {
         DeploymentTester tester = new DeploymentTester();
         JobController jobs = tester.controller().jobController();
         Map<Step, Status> outcomes = new EnumMap<>(Step.class);
@@ -122,10 +122,10 @@ public class JobRunnerTest {
         // Starting tests allows storing data.
         outcomes.put(startTests, succeeded);
         runner.maintain();
-        assertEquals(Arrays.asList(runTests), run.get().readySteps());
+        assertEquals(Arrays.asList(endTests), run.get().readySteps());
 
         // Storing data allows deactivating tester.
-        outcomes.put(runTests, succeeded);
+        outcomes.put(endTests, succeeded);
         runner.maintain();
         assertEquals(Arrays.asList(deactivateReal, deactivateTester), run.get().readySteps());
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializerTest.java
index 01acc401a1d..12640a5e8fa 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializerTest.java
@@ -29,7 +29,7 @@ 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;
 import static com.yahoo.vespa.hosted.controller.deployment.Step.startTests;
-import static com.yahoo.vespa.hosted.controller.deployment.Step.runTests;
+import static com.yahoo.vespa.hosted.controller.deployment.Step.endTests;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -70,7 +70,7 @@ public class RunSerializerTest {
                     .put(installTester, unfinished)
                     .put(deactivateTester, failed)
                     .put(startTests, succeeded)
-                    .put(runTests, unfinished)
+                    .put(endTests, unfinished)
                     .put(report, failed)
                     .build(),
                      run.steps());
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/testdata/run-status.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/testdata/run-status.json
index e31d14ac181..d659bd9fff0 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/testdata/run-status.json
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/testdata/run-status.json
@@ -14,7 +14,7 @@
       "IT":  "U",
       "DAT": "F",
       "ST":  "S",
-      "RT":  "U",
+      "ET":  "U",
       "R":   "F"
     }
   }