From 4d5e8a149a7da724532c4b499bc41d0c03a7bf51 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Jan 2014 12:32:10 -0800 Subject: [PATCH] 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 | 8 +- .../engine/HarbormasterBuildEngine.php | 223 ++++++++++++++++++ .../worker/HarbormasterBuildWorker.php | 78 +----- .../worker/HarbormasterTargetWorker.php | 59 +++++ .../worker/HarbormasterWorker.php | 9 + 5 files changed, 306 insertions(+), 71 deletions(-) create mode 100644 src/applications/harbormaster/engine/HarbormasterBuildEngine.php create mode 100644 src/applications/harbormaster/worker/HarbormasterTargetWorker.php create mode 100644 src/applications/harbormaster/worker/HarbormasterWorker.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 706932bff7..dd611e9bde 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php new file mode 100644 index 0000000000..bdc2e105fa --- /dev/null +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -0,0 +1,223 @@ +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); + } + } + +} diff --git a/src/applications/harbormaster/worker/HarbormasterBuildWorker.php b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php index d89500406b..31f37b2f2b 100644 --- a/src/applications/harbormaster/worker/HarbormasterBuildWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php @@ -1,22 +1,17 @@ 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(); } } diff --git a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php new file mode 100644 index 0000000000..00b4afb582 --- /dev/null +++ b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php @@ -0,0 +1,59 @@ +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(); + } + +} diff --git a/src/applications/harbormaster/worker/HarbormasterWorker.php b/src/applications/harbormaster/worker/HarbormasterWorker.php new file mode 100644 index 0000000000..00cc8812a2 --- /dev/null +++ b/src/applications/harbormaster/worker/HarbormasterWorker.php @@ -0,0 +1,9 @@ +