In Harbormaster, release artifacts as soon as no waiting/running build steps will use them

Summary:
Ref T11153. If you have a build plan like this:

  - Lease machine A.
  - Lease machine B.
  - Run client-tests on machine A.
  - Run server-tests on machine B.

...and we get machine A quickly, then finish the tests, we currently do not release machine A until the whole plan finishes.

In the best case, this wastes resources (something else could be using that machine for a while).

In a worse case, this wastes a lot of resources (if machine B is slow to acquire, or the server tests are much slower than the client tests, machine A will get tied up for a really long time).

In the absolute worst case, this might deadlock things.

Instead, release artifacts as soon as no waiting/running steps take them as inputs. In this case, we'd release machine A as soon as we finished running the client tests.

In the case where machines A and B are resources of the same type, this should prevent deadlocks. In all cases, this should improve build throughput at least somewhat.

Test Plan:
I wrote this build plan which runs a "fast" step (10 seconds) and a "slow" step (120 seconds):

{F1691190}

Before the patch, running this build plan held the lease on the "fast" machine for the full 120 seconds, then released both leases at the same time at the very end.

After this patch, I ran this plan and observed the "fast" lease get released after 10 seconds, while the "slow" lease was held for the full 120.

(Also added some `var_dump()` into things to sanity check the logic; it appeared correct.)

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11153

Differential Revision: https://secure.phabricator.com/D16145

resources/sql/autopatches/20160617.harbormaster.01.arelease.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildartifact
+  ADD isReleased BOOL NOT NULL;

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

@@ -9,6 +9,7 @@ final class HarbormasterBuildEngine extends Phobject {
   private $build;
   private $viewer;
   private $newBuildTargets = array();
+  private $artifactReleaseQueue = array();
   private $forceBuildableUpdate;
 
   public function setForceBuildableUpdate($force_buildable_update) {
@@ -94,6 +95,8 @@ final class HarbormasterBuildEngine extends Phobject {
       $this->updateBuildable($build->getBuildable());
     }
 
+    $this->releaseQueuedArtifacts();
+
     // If we are no longer building for any reason, release all artifacts.
     if (!$build->isBuilding()) {
       $this->releaseAllArtifacts($build);
@@ -149,20 +152,21 @@ final class HarbormasterBuildEngine extends Phobject {
   }
 
   private function updateBuildSteps(HarbormasterBuild $build) {
-    $targets = id(new HarbormasterBuildTargetQuery())
+    $all_targets = id(new HarbormasterBuildTargetQuery())
       ->setViewer($this->getViewer())
       ->withBuildPHIDs(array($build->getPHID()))
       ->withBuildGenerations(array($build->getBuildGeneration()))
       ->execute();
 
-    $this->updateWaitingTargets($targets);
+    $this->updateWaitingTargets($all_targets);
 
-    $targets = mgroup($targets, 'getBuildStepPHID');
+    $targets = mgroup($all_targets, 'getBuildStepPHID');
 
     $steps = id(new HarbormasterBuildStepQuery())
       ->setViewer($this->getViewer())
       ->withBuildPlanPHIDs(array($build->getBuildPlan()->getPHID()))
       ->execute();
+    $steps = mpull($steps, null, 'getPHID');
 
     // Identify steps which are in various states.
 
@@ -252,6 +256,12 @@ final class HarbormasterBuildEngine extends Phobject {
       return;
     }
 
+    // Release any artifacts which are not inputs to any remaining build
+    // step. We're done with these, so something else is free to use them.
+    $ongoing_phids = array_keys($queued + $waiting + $underway);
+    $ongoing_steps = array_select_keys($steps, $ongoing_phids);
+    $this->releaseUnusedArtifacts($all_targets, $ongoing_steps);
+
     // Identify all the steps which are ready to run (because all their
     // dependencies are complete).
 
@@ -294,6 +304,59 @@ final class HarbormasterBuildEngine extends Phobject {
   }
 
 
+  /**
+   * Release any artifacts which aren't used by any running or waiting steps.
+   *
+   * This releases artifacts as soon as they're no longer used. This can be
+   * particularly relevant when a build uses multiple hosts since it returns
+   * hosts to the pool more quickly.
+   *
+   * @param list<HarbormasterBuildTarget> Targets in the build.
+   * @param list<HarbormasterBuildStep> List of running and waiting steps.
+   * @return void
+   */
+  private function releaseUnusedArtifacts(array $targets, array $steps) {
+    assert_instances_of($targets, 'HarbormasterBuildTarget');
+    assert_instances_of($steps, 'HarbormasterBuildStep');
+
+    if (!$targets || !$steps) {
+      return;
+    }
+
+    $target_phids = mpull($targets, 'getPHID');
+
+    $artifacts = id(new HarbormasterBuildArtifactQuery())
+      ->setViewer($this->getViewer())
+      ->withBuildTargetPHIDs($target_phids)
+      ->withIsReleased(false)
+      ->execute();
+    if (!$artifacts) {
+      return;
+    }
+
+    // Collect all the artifacts that remaining build steps accept as inputs.
+    $must_keep = array();
+    foreach ($steps as $step) {
+      $inputs = $step->getStepImplementation()->getArtifactInputs();
+      foreach ($inputs as $input) {
+        $artifact_key = $input['key'];
+        $must_keep[$artifact_key] = true;
+      }
+    }
+
+    // Queue unreleased artifacts which no remaining step uses for immediate
+    // release.
+    foreach ($artifacts as $artifact) {
+      $key = $artifact->getArtifactKey();
+      if (isset($must_keep[$key])) {
+        continue;
+      }
+
+      $this->artifactReleaseQueue[] = $artifact;
+    }
+  }
+
+
   /**
    * Process messages which were sent to these targets, kicking applicable
    * targets out of "Waiting" and into either "Passed" or "Failed".
@@ -488,12 +551,18 @@ final class HarbormasterBuildEngine extends Phobject {
     $artifacts = id(new HarbormasterBuildArtifactQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
       ->withBuildTargetPHIDs($target_phids)
+      ->withIsReleased(false)
       ->execute();
-
     foreach ($artifacts as $artifact) {
       $artifact->releaseArtifact();
     }
+  }
 
+  private function releaseQueuedArtifacts() {
+    foreach ($this->artifactReleaseQueue as $key => $artifact) {
+      $artifact->releaseArtifact();
+      unset($this->artifactReleaseQueue[$key]);
+    }
   }
 
 }

src/applications/harbormaster/query/HarbormasterBuildArtifactQuery.php

@@ -9,6 +9,7 @@ final class HarbormasterBuildArtifactQuery
   private $artifactIndexes;
   private $keyBuildPHID;
   private $keyBuildGeneration;
+  private $isReleased;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -30,6 +31,11 @@ final class HarbormasterBuildArtifactQuery
     return $this;
   }
 
+  public function withIsReleased($released) {
+    $this->isReleased = $released;
+    return $this;
+  }
+
   public function newResultObject() {
     return new HarbormasterBuildArtifact();
   }
@@ -94,6 +100,13 @@ final class HarbormasterBuildArtifactQuery
         $this->artifactIndexes);
     }
 
+    if ($this->isReleased !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'isReleased = %d',
+        (int)$this->isReleased);
+    }
+
     return $where;
   }
 

src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php

@@ -103,11 +103,6 @@ abstract class HarbormasterBuildStepImplementation extends Phobject {
   /**
    * Return the name of artifacts produced by this command.
    *
-   * Something like:
-   *
-   *   return array(
-   *     'some_name_input_by_user' => 'host');
-   *
    * Future steps will calculate all available artifact mappings
    * before them and filter on the type.
    *

src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php

@@ -8,6 +8,7 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO
   protected $artifactIndex;
   protected $artifactKey;
   protected $artifactData = array();
+  protected $isReleased = 0;
 
   private $buildTarget = self::ATTACHABLE;
   private $artifactImplementation;
@@ -29,6 +30,7 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO
         'artifactType' => 'text32',
         'artifactIndex' => 'bytes12',
         'artifactKey' => 'text255',
+        'isReleased' => 'bool',
       ),
       self::CONFIG_KEY_SCHEMA => array(
         'key_artifact' => array(
@@ -83,13 +85,18 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO
   }
 
   public function releaseArtifact() {
-    $impl = $this->getArtifactImplementation();
+    if ($this->getIsReleased()) {
+      return $this;
+    }
 
+    $impl = $this->getArtifactImplementation();
     if ($impl) {
       $impl->releaseArtifact(PhabricatorUser::getOmnipotentUser());
     }
 
-    return null;
+    return $this
+      ->setIsReleased(1)
+      ->save();
   }
 
   public function getArtifactImplementation() {