Split Harbormaster workers apart so build steps can run in parallel

Summary:
Ref T1049. Currently, the Harbormaster worker looks like this:

  foreach (step) {
    run_step(step);
  }

This means steps can't ever be run in parallel. Instead, split it into two workers. The "Build" worker starts things off, and basically does:

  update_build();

(We could theoretically do this in the original process because it should never take very long, but since there's a lock and it's a little bit complex it seemed cleaner to separate it.)

The "Target" worker runs an individual target (like a command, or an HTTP request, or whatever), then updates the build:

  run_one_step(step);
  update_build();

The new `update_build()` mechanism in `HarbormasterBuildEngine` does this, roughly:

  figure_out_overall_status_of_all_steps();
  if (build is done) { done(); }
  if (build is fail) { fail(); }
  foreach (step that is ready to run) {
    queue_target_worker_for_step(step);
  }

So, overall:

  - The part of the code that updates Builds is completely separated from the part of the code that updates Targets.
  - Targets can run in parallel.

Test Plan:
  - Ran a bunch of builds via `bin/harbormaster build`.
  - Ran a bunch of builds via web UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1049

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

src/__phutil_library_map__.php

@@ -707,6 +707,7 @@ phutil_register_library_map(array(
     'HarbormasterBuildArtifact' => 'applications/harbormaster/storage/build/HarbormasterBuildArtifact.php',
     'HarbormasterBuildArtifactQuery' => 'applications/harbormaster/query/HarbormasterBuildArtifactQuery.php',
     'HarbormasterBuildCancelController' => 'applications/harbormaster/controller/HarbormasterBuildCancelController.php',
+    'HarbormasterBuildEngine' => 'applications/harbormaster/engine/HarbormasterBuildEngine.php',
     'HarbormasterBuildItem' => 'applications/harbormaster/storage/build/HarbormasterBuildItem.php',
     'HarbormasterBuildItemQuery' => 'applications/harbormaster/query/HarbormasterBuildItemQuery.php',
     'HarbormasterBuildLog' => 'applications/harbormaster/storage/build/HarbormasterBuildLog.php',
@@ -757,7 +758,9 @@ phutil_register_library_map(array(
     'HarbormasterStepAddController' => 'applications/harbormaster/controller/HarbormasterStepAddController.php',
     'HarbormasterStepDeleteController' => 'applications/harbormaster/controller/HarbormasterStepDeleteController.php',
     'HarbormasterStepEditController' => 'applications/harbormaster/controller/HarbormasterStepEditController.php',
+    'HarbormasterTargetWorker' => 'applications/harbormaster/worker/HarbormasterTargetWorker.php',
     'HarbormasterUIEventListener' => 'applications/harbormaster/event/HarbormasterUIEventListener.php',
+    'HarbormasterWorker' => 'applications/harbormaster/worker/HarbormasterWorker.php',
     'HeraldAction' => 'applications/herald/storage/HeraldAction.php',
     'HeraldAdapter' => 'applications/herald/adapter/HeraldAdapter.php',
     'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php',
@@ -3159,6 +3162,7 @@ phutil_register_library_map(array(
     ),
     'HarbormasterBuildArtifactQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'HarbormasterBuildCancelController' => 'HarbormasterController',
+    'HarbormasterBuildEngine' => 'Phobject',
     'HarbormasterBuildItem' => 'HarbormasterDAO',
     'HarbormasterBuildItemQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'HarbormasterBuildLog' =>
@@ -3193,7 +3197,7 @@ phutil_register_library_map(array(
     ),
     'HarbormasterBuildTargetQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'HarbormasterBuildViewController' => 'HarbormasterController',
-    'HarbormasterBuildWorker' => 'PhabricatorWorker',
+    'HarbormasterBuildWorker' => 'HarbormasterWorker',
     'HarbormasterBuildable' =>
     array(
       0 => 'HarbormasterDAO',
@@ -3238,7 +3242,9 @@ phutil_register_library_map(array(
     'HarbormasterStepAddController' => 'HarbormasterController',
     'HarbormasterStepDeleteController' => 'HarbormasterController',
     'HarbormasterStepEditController' => 'HarbormasterController',
+    'HarbormasterTargetWorker' => 'HarbormasterWorker',
     'HarbormasterUIEventListener' => 'PhabricatorEventListener',
+    'HarbormasterWorker' => 'PhabricatorWorker',
     'HeraldAction' => 'HeraldDAO',
     'HeraldApplyTranscript' => 'HeraldDAO',
     'HeraldCapabilityManageGlobalRules' => 'PhabricatorPolicyCapability',

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

@@ -0,0 +1,223 @@
+<?php
+
+/**
+ * Moves a build forward by queuing build tasks, canceling or restarting the
+ * build, or failing it in response to task failures.
+ */
+final class HarbormasterBuildEngine extends Phobject {
+
+  private $build;
+  private $viewer;
+  private $newBuildTargets = array();
+
+  public function queueNewBuildTarget(HarbormasterBuildTarget $target) {
+    $this->newBuildTargets[] = $target;
+    return $this;
+  }
+
+  public function getNewBuildTargets() {
+    return $this->newBuildTargets;
+  }
+
+  public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  public function getViewer() {
+    return $this->viewer;
+  }
+
+  public function setBuild(HarbormasterBuild $build) {
+    $this->build = $build;
+    return $this;
+  }
+
+  public function getBuild() {
+    return $this->build;
+  }
+
+  public function continueBuild() {
+    $build = $this->getBuild();
+
+    $lock_key = 'harbormaster.build:'.$build->getID();
+    $lock = PhabricatorGlobalLock::newLock($lock_key)->lock(15);
+
+    $build->reload();
+
+    try {
+      $this->updateBuild($build);
+    } catch (Exception $ex) {
+      // If any exception is raised, the build is marked as a failure and the
+      // exception is re-thrown (this ensures we don't leave builds in an
+      // inconsistent state).
+      $build->setBuildStatus(HarbormasterBuild::STATUS_ERROR);
+      $build->save();
+
+      $lock->unlock();
+      throw $ex;
+    }
+
+    $lock->unlock();
+
+    // NOTE: We queue new targets after releasing the lock so that in-process
+    // execution via `bin/harbormaster` does not reenter the locked region.
+    foreach ($this->getNewBuildTargets() as $target) {
+      $task = PhabricatorWorker::scheduleTask(
+        'HarbormasterTargetWorker',
+        array(
+          'targetID' => $target->getID(),
+        ));
+    }
+  }
+
+  private function updateBuild(HarbormasterBuild $build) {
+    // TODO: Handle cancellation and restarts.
+
+    if ($build->getBuildStatus() == HarbormasterBuild::STATUS_PENDING) {
+      $this->destroyBuildTargets($build);
+      $build->setBuildStatus(HarbormasterBuild::STATUS_BUILDING);
+      $build->save();
+    }
+
+    if ($build->getBuildStatus() == HarbormasterBuild::STATUS_BUILDING) {
+      return $this->updateBuildSteps($build);
+    }
+  }
+
+  private function destroyBuildTargets(HarbormasterBuild $build) {
+    $targets = id(new HarbormasterBuildTargetQuery())
+      ->setViewer($this->getViewer())
+      ->withBuildPHIDs(array($build->getPHID()))
+      ->execute();
+    foreach ($targets as $target) {
+      $target->delete();
+    }
+  }
+
+  private function updateBuildSteps(HarbormasterBuild $build) {
+    $targets = id(new HarbormasterBuildTargetQuery())
+      ->setViewer($this->getViewer())
+      ->withBuildPHIDs(array($build->getPHID()))
+      ->execute();
+    $targets = mgroup($targets, 'getBuildStepPHID');
+
+    $steps = id(new HarbormasterBuildStepQuery())
+      ->setViewer($this->getViewer())
+      ->withBuildPlanPHIDs(array($build->getBuildPlan()->getPHID()))
+      ->execute();
+
+    // Identify steps which are complete.
+
+    $complete = array();
+    $failed = array();
+    $waiting = array();
+    foreach ($steps as $step) {
+      $step_targets = idx($targets, $step->getPHID(), array());
+
+      if ($step_targets) {
+        $is_complete = true;
+        foreach ($step_targets as $target) {
+          // TODO: Move this to a top-level "status" field on BuildTarget.
+          if (!$target->getDetail('__done__')) {
+            $is_complete = false;
+            break;
+          }
+        }
+
+        $is_failed = false;
+        foreach ($step_targets as $target) {
+          // TODO: Move this to a top-level "status" field on BuildTarget.
+          if ($target->getDetail('__failed__')) {
+            $is_failed = true;
+            break;
+          }
+        }
+
+        $is_waiting = false;
+      } else {
+        $is_complete = false;
+        $is_failed = false;
+        $is_waiting = true;
+      }
+
+      if ($is_complete) {
+        $complete[$step->getPHID()] = true;
+      }
+
+      if ($is_failed) {
+        $failed[$step->getPHID()] = true;
+      }
+
+      if ($is_waiting) {
+        $waiting[$step->getPHID()] = true;
+      }
+    }
+
+    // If every step is complete, we're done with this build. Mark it passed
+    // and bail.
+    if (count($complete) == count($steps)) {
+      $build->setBuildStatus(HarbormasterBuild::STATUS_PASSED);
+      $build->save();
+      return;
+    }
+
+    // If any step failed, fail the whole build, then bail.
+    if (count($failed)) {
+      $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED);
+      $build->save();
+      return;
+    }
+
+    // Identify all the steps which are ready to run (because all their
+    // depdendencies are complete).
+
+    $previous_step = null;
+    $runnable = array();
+    foreach ($steps as $step) {
+      // TODO: For now, we're hard coding sequential dependencies into build
+      // steps. In the future, we can be smart about this instead.
+
+      if ($previous_step) {
+        $dependencies = array($previous_step);
+      } else {
+        $dependencies = array();
+      }
+
+      if (isset($waiting[$step->getPHID()])) {
+        $can_run = true;
+        foreach ($dependencies as $dependency) {
+          if (empty($complete[$dependency->getPHID()])) {
+            $can_run = false;
+            break;
+          }
+        }
+
+        if ($can_run) {
+          $runnable[] = $step;
+        }
+      }
+
+      $previous_step = $step;
+    }
+
+    if (!$runnable) {
+      // TODO: This means the build is deadlocked, probably? It should not
+      // normally be possible, but we should communicate it more clearly.
+      $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED);
+      $build->save();
+      return;
+    }
+
+    foreach ($runnable as $runnable_step) {
+      $target = HarbormasterBuildTarget::initializeNewBuildTarget(
+        $build,
+        $step,
+        $build->retrieveVariablesFromBuild());
+      $target->save();
+
+      $this->queueNewBuildTarget($target);
+    }
+  }
+
+}

src/applications/harbormaster/worker/HarbormasterBuildWorker.php

@@ -1,22 +1,17 @@
 <?php
 
 /**
- * Run builds
+ * Start a build.
  */
-final class HarbormasterBuildWorker extends PhabricatorWorker {
-
-  public function getRequiredLeaseTime() {
-    return 60 * 60 * 24;
-  }
+final class HarbormasterBuildWorker extends HarbormasterWorker {
 
   public function doWork() {
     $data = $this->getTaskData();
     $id = idx($data, 'buildID');
+    $viewer = $this->getViewer();
 
-    // Get a reference to the build.
     $build = id(new HarbormasterBuildQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withBuildStatuses(array(HarbormasterBuild::STATUS_PENDING))
+      ->setViewer($viewer)
       ->withIDs(array($id))
       ->executeOne();
     if (!$build) {
@@ -24,67 +19,10 @@ final class HarbormasterBuildWorker extends PhabricatorWorker {
         pht('Invalid build ID "%s".', $id));
     }
 
-    // It's possible for the user to request cancellation before
-    // a worker picks up a build.  We check to see if the build
-    // is already cancelled, and return if it is.
-    if ($build->checkForCancellation()) {
-      return;
-    }
-
-    try {
-      $build->setBuildStatus(HarbormasterBuild::STATUS_BUILDING);
-      $build->save();
-
-      $buildable = $build->getBuildable();
-      $plan = $build->getBuildPlan();
-
-      $steps = $plan->loadOrderedBuildSteps();
-
-      // Perform the build.
-      foreach ($steps as $step) {
-
-        // Create the target at this step.
-        // TODO: Support variable artifacts.
-        $target = HarbormasterBuildTarget::initializeNewBuildTarget(
-          $build,
-          $step,
-          $build->retrieveVariablesFromBuild());
-        $target->save();
-
-        $implementation = $target->getImplementation();
-        if (!$implementation->validateSettings()) {
-          $build->setBuildStatus(HarbormasterBuild::STATUS_ERROR);
-          break;
-        }
-        $implementation->execute($build, $target);
-        if ($build->getBuildStatus() !== HarbormasterBuild::STATUS_BUILDING) {
-          break;
-        }
-        if ($build->checkForCancellation()) {
-          break;
-        }
-      }
-
-      // Check to see if the user requested cancellation.  If they did and
-      // we get to here, they might have either cancelled too late, or the
-      // step isn't cancellation aware.  In either case we ignore the result
-      // and move to a cancelled state.
-      $build->checkForCancellation();
-
-      // If we get to here, then the build has finished.  Set it to passed
-      // if no build step explicitly set the status.
-      if ($build->getBuildStatus() === HarbormasterBuild::STATUS_BUILDING) {
-        $build->setBuildStatus(HarbormasterBuild::STATUS_PASSED);
-      }
-      $build->save();
-    } catch (Exception $e) {
-      // If any exception is raised, the build is marked as a failure and
-      // the exception is re-thrown (this ensures we don't leave builds
-      // in an inconsistent state).
-      $build->setBuildStatus(HarbormasterBuild::STATUS_ERROR);
-      $build->save();
-      throw $e;
-    }
+    id(new HarbormasterBuildEngine())
+      ->setViewer($viewer)
+      ->setBuild($build)
+      ->continueBuild();
   }
 
 }

src/applications/harbormaster/worker/HarbormasterTargetWorker.php

@@ -0,0 +1,59 @@
+<?php
+
+/**
+ * Execute a build target.
+ */
+final class HarbormasterTargetWorker extends HarbormasterWorker {
+
+  public function getRequiredLeaseTime() {
+    // This worker performs actual build work, which may involve a long wait
+    // on external systems.
+    return 60 * 60 * 24;
+  }
+
+  private function loadBuildTarget() {
+    $data = $this->getTaskData();
+    $id = idx($data, 'targetID');
+
+    $target = id(new HarbormasterBuildTargetQuery())
+      ->withIDs(array($id))
+      ->setViewer($this->getViewer())
+      ->executeOne();
+
+    if (!$target) {
+      throw new PhabricatorWorkerPermanentFailureException(
+        pht(
+          'Bad build target ID "%d".',
+          $id));
+    }
+
+    return $target;
+  }
+
+  public function doWork() {
+    $target = $this->loadBuildTarget();
+    $build = $target->getBuild();
+    $viewer = $this->getViewer();
+
+    try {
+      $implementation = $target->getImplementation();
+      if (!$implementation->validateSettings()) {
+        $target->setDetail('__failed__', true);
+        $target->save();
+      } else {
+        $implementation->execute($build, $target);
+        $target->setDetail('__done__', true);
+        $target->save();
+      }
+    } catch (Exception $ex) {
+      $target->setDetail('__failed__', true);
+      $target->save();
+    }
+
+    id(new HarbormasterBuildEngine())
+      ->setViewer($viewer)
+      ->setBuild($build)
+      ->continueBuild();
+  }
+
+}

src/applications/harbormaster/worker/HarbormasterWorker.php

@@ -0,0 +1,9 @@
+<?php
+
+abstract class HarbormasterWorker extends PhabricatorWorker {
+
+  public function getViewer() {
+    return PhabricatorUser::getOmnipotentUser();
+  }
+
+}