From 78bf266bde4acc5f640a0a9b6089e5aa576c5b01 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 16 Apr 2014 13:01:46 -0700 Subject: [PATCH] Allow Harbormaster build targets to wait for messages Summary: This hooks up all the pieces of the build pipeline so `harbormaster.sendmessage` actually works. Particularly: - Candidate build steps (i.e., those which interact with external systems) can now "Wait for Message". This pauses them indefinitely when they complete, until something calls `harbormaster.sendmessage`. - After processing a target, we check if we should move it to PASSED or WAITING. - Before updating a build, we move WAITING targets with pending messages to either PASSED or FAILED. - I added an explicit "Building" state, which doesn't affect workflows but communicates more information to human users. A big part of this is avoiding races. I believe we get the correct behavior no matter which order events occur in: - We update builds after targets complete and after we receive messages, so we're guaranteed to update once both these conditions are true. This means messages can't be lost (even if they arrive before a build completes). - The minor changes to the build engine logic mean that firing additional build updates is always safe, no matter what the current state of the build is. - The build itself is protected by a lock in the build engine. - The target is not covered by an explicit lock, but for all states only the engine (waiting) //or// the worker (all other states) can interact with it. All of the interactions also move the target state forward to the same destination and have no other side effects. - Messages are only consumed inside the engine lock, so they don't need an explicit lock. Test Plan: - Made an HTTP request wait after completion, then ran a pile of builds through it using `bin/harbormaster build` and the web UI. - Passed and failed message-awaiting builds with `harbormaster.sendmessage`. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley, zeeg Differential Revision: https://secure.phabricator.com/D8788 --- ...uitAPI_harbormaster_sendmessage_Method.php | 7 ++ .../HarbormasterBuildableViewController.php | 14 ++- .../HarbormasterBuildStepCoreCustomField.php | 36 +++++- .../engine/HarbormasterBuildEngine.php | 109 ++++++++++++++++-- .../query/HarbormasterBuildMessageQuery.php | 2 +- .../HarbormasterBuildStepImplementation.php | 12 ++ ...sterHTTPRequestBuildStepImplementation.php | 4 + .../storage/build/HarbormasterBuildTarget.php | 22 ++++ .../worker/HarbormasterTargetWorker.php | 14 ++- 9 files changed, 204 insertions(+), 16 deletions(-) diff --git a/src/applications/harbormaster/conduit/ConduitAPI_harbormaster_sendmessage_Method.php b/src/applications/harbormaster/conduit/ConduitAPI_harbormaster_sendmessage_Method.php index 8b6d601e4b..913367cf4e 100644 --- a/src/applications/harbormaster/conduit/ConduitAPI_harbormaster_sendmessage_Method.php +++ b/src/applications/harbormaster/conduit/ConduitAPI_harbormaster_sendmessage_Method.php @@ -43,6 +43,13 @@ final class ConduitAPI_harbormaster_sendmessage_Method ->setType($message_type) ->save(); + // If the build has completely paused because all steps are blocked on + // waiting targets, this will resume it. + id(new HarbormasterBuildEngine()) + ->setViewer($viewer) + ->setBuild($build_target->getBuild()) + ->continueBuild(); + return null; } diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php index 5ed96d8c40..7b2b868f1b 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php @@ -251,15 +251,27 @@ final class HarbormasterBuildableViewController switch ($target->getTargetStatus()) { case HarbormasterBuildTarget::STATUS_PENDING: $icon = 'time-green'; + $status_name = pht('Pending'); + break; + case HarbormasterBuildTarget::STATUS_BUILDING: + $icon = 'right-green'; + $status_name = pht('Building'); + break; + case HarbormasterBuildTarget::STATUS_WAITING: + $icon = 'time-orange'; + $status_name = pht('Waiting'); break; case HarbormasterBuildTarget::STATUS_PASSED: $icon = 'accept-green'; + $status_name = pht('Passed'); break; case HarbormasterBuildTarget::STATUS_FAILED: $icon = 'reject-red'; + $status_name = pht('Failed'); break; default: $icon = 'question'; + $status_name = pht('Unknown'); break; } @@ -272,7 +284,7 @@ final class HarbormasterBuildableViewController $target_list->addItem( id(new PHUIStatusItemView()) - ->setIcon($icon) + ->setIcon($icon, $status_name) ->setTarget(pht('Target %d', $target->getID())) ->setNote($name)); } diff --git a/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php b/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php index 84b67974d7..03703a48f4 100644 --- a/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php +++ b/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php @@ -9,7 +9,41 @@ final class HarbormasterBuildStepCoreCustomField } public function createFields($object) { - $specs = $object->getStepImplementation()->getFieldSpecifications(); + $impl = $object->getStepImplementation(); + $specs = $impl->getFieldSpecifications(); + + if ($impl->supportsWaitForMessage()) { + $specs['builtin.next-steps-header'] = array( + 'type' => 'header', + 'name' => pht('Next Steps'), + ); + + $specs['builtin.wait-for-message'] = array( + 'type' => 'select', + 'name' => pht('When Complete'), + 'instructions' => pht( + 'After completing this build step Harbormaster can continue the '. + 'build normally, or it can pause the build and wait for a message. '. + 'If you are using this build step to trigger some work in an '. + 'external system, you may want to have Phabricator wait for that '. + 'system to perform the work and report results back.'. + "\n\n". + 'If you select **Continue Build Normally**, the build plan will '. + 'proceed once this step finishes.'. + "\n\n". + 'If you select **Wait For Message**, the build plan will pause '. + 'indefinitely once this step finishes. To resume the build, an '. + 'external system must call `harbormaster.sendmessage` with the '. + 'build target PHID, and either `"pass"` or `"fail"` to indicate '. + 'the result for this step. After the result is recorded, the build '. + 'plan will resume.'), + 'options' => array( + '' => pht('Continue Build Normally'), + 'wait' => pht('Wait For Message'), + ), + ); + } + return PhabricatorStandardCustomField::buildStandardFields($this, $specs); } diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php index af2057b990..389ca11a99 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -127,6 +127,9 @@ final class HarbormasterBuildEngine extends Phobject { ->setViewer($this->getViewer()) ->withBuildPHIDs(array($build->getPHID())) ->execute(); + + $this->updateWaitingTargets($targets); + $targets = mgroup($targets, 'getBuildStepPHID'); $steps = id(new HarbormasterBuildStepQuery()) @@ -134,15 +137,35 @@ final class HarbormasterBuildEngine extends Phobject { ->withBuildPlanPHIDs(array($build->getBuildPlan()->getPHID())) ->execute(); - // Identify steps which are complete. + // Identify steps which are in various states. + $queued = array(); + $underway = array(); + $waiting = array(); $complete = array(); $failed = array(); - $waiting = array(); foreach ($steps as $step) { $step_targets = idx($targets, $step->getPHID(), array()); if ($step_targets) { + $is_queued = false; + + $is_underway = false; + foreach ($step_targets as $target) { + if ($target->isUnderway()) { + $is_underway = true; + break; + } + } + + $is_waiting = false; + foreach ($step_targets as $target) { + if ($target->isWaiting()) { + $is_waiting = true; + break; + } + } + $is_complete = true; foreach ($step_targets as $target) { if (!$target->isComplete()) { @@ -158,12 +181,24 @@ final class HarbormasterBuildEngine extends Phobject { break; } } - - $is_waiting = false; } else { + $is_queued = true; + $is_underway = false; + $is_waiting = false; $is_complete = false; $is_failed = false; - $is_waiting = true; + } + + if ($is_queued) { + $queued[$step->getPHID()] = true; + } + + if ($is_underway) { + $underway[$step->getPHID()] = true; + } + + if ($is_waiting) { + $waiting[$step->getPHID()] = true; } if ($is_complete) { @@ -173,10 +208,6 @@ final class HarbormasterBuildEngine extends Phobject { if ($is_failed) { $failed[$step->getPHID()] = true; } - - if ($is_waiting) { - $waiting[$step->getPHID()] = true; - } } // If any step failed, fail the whole build, then bail. @@ -209,7 +240,7 @@ final class HarbormasterBuildEngine extends Phobject { $dependencies = array(); } - if (isset($waiting[$step->getPHID()])) { + if (isset($queued[$step->getPHID()])) { $can_run = true; foreach ($dependencies as $dependency) { if (empty($complete[$dependency->getPHID()])) { @@ -226,9 +257,9 @@ final class HarbormasterBuildEngine extends Phobject { $previous_step = $step; } - if (!$runnable) { + if (!$runnable && !$waiting && !$underway) { // TODO: This means the build is deadlocked, probably? It should not - // normally be possible, but we should communicate it more clearly. + // normally be possible yet, but we should communicate it more clearly. $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED); $build->save(); return; @@ -243,6 +274,60 @@ final class HarbormasterBuildEngine extends Phobject { $this->queueNewBuildTarget($target); } + + } + + + /** + * Process messages which were sent to these targets, kicking applicable + * targets out of "Waiting" and into either "Passed" or "Failed". + * + * @param list List of targets to process. + * @return void + */ + private function updateWaitingTargets(array $targets) { + assert_instances_of($targets, 'HarbormasterBuildTarget'); + + // We only care about messages for targets which are actually in a waiting + // state. + $waiting_targets = array(); + foreach ($targets as $target) { + if ($target->isWaiting()) { + $waiting_targets[$target->getPHID()] = $target; + } + } + + if (!$waiting_targets) { + return; + } + + $messages = id(new HarbormasterBuildMessageQuery()) + ->setViewer($this->getViewer()) + ->withBuildTargetPHIDs(array_keys($waiting_targets)) + ->withConsumed(false) + ->execute(); + + foreach ($messages as $message) { + $target = $waiting_targets[$message->getBuildTargetPHID()]; + + $new_status = null; + switch ($message->getType()) { + case 'pass': + $new_status = HarbormasterBuildTarget::STATUS_PASSED; + break; + case 'fail': + $new_status = HarbormasterBuildTarget::STATUS_FAILED; + break; + } + + if ($new_status !== null) { + $message->setIsConsumed(true); + $message->save(); + + $target->setTargetStatus($new_status); + $target->save(); + } + } } } diff --git a/src/applications/harbormaster/query/HarbormasterBuildMessageQuery.php b/src/applications/harbormaster/query/HarbormasterBuildMessageQuery.php index 071ef03793..b792b9d06b 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildMessageQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildMessageQuery.php @@ -83,7 +83,7 @@ final class HarbormasterBuildMessageQuery $where[] = qsprintf( $conn_r, 'isConsumed = %d', - (int)$this->isConsumed); + (int)$this->consumed); } $where[] = $this->buildPagingClause($conn_r); diff --git a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php index 4ba72c09b7..830965442b 100644 --- a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php @@ -196,4 +196,16 @@ abstract class HarbormasterBuildStepImplementation { } } + public function supportsWaitForMessage() { + return false; + } + + public function shouldWaitForMessage(HarbormasterBuildTarget $target) { + if (!$this->supportsWaitForMessage()) { + return false; + } + + return (bool)$target->getDetail('builtin.wait-for-message'); + } + } diff --git a/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php index ec4bad34a2..10df86689f 100644 --- a/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php @@ -99,4 +99,8 @@ final class HarbormasterHTTPRequestBuildStepImplementation ); } + public function supportsWaitForMessage() { + return true; + } + } diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php index 2b17bc8cad..c545bcaf03 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php @@ -11,6 +11,8 @@ final class HarbormasterBuildTarget extends HarbormasterDAO protected $targetStatus; const STATUS_PENDING = 'target/pending'; + const STATUS_BUILDING = 'target/building'; + const STATUS_WAITING = 'target/waiting'; const STATUS_PASSED = 'target/passed'; const STATUS_FAILED = 'target/failed'; @@ -128,6 +130,26 @@ final class HarbormasterBuildTarget extends HarbormasterDAO } + public function isWaiting() { + switch ($this->getTargetStatus()) { + case self::STATUS_WAITING: + return true; + } + + return false; + } + + public function isUnderway() { + switch ($this->getTargetStatus()) { + case self::STATUS_PENDING: + case self::STATUS_BUILDING: + return true; + } + + return false; + } + + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php index 7fb13a54d2..9dbf2136ad 100644 --- a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php @@ -36,9 +36,21 @@ final class HarbormasterTargetWorker extends HarbormasterWorker { $viewer = $this->getViewer(); try { + $status_pending = HarbormasterBuildTarget::STATUS_PENDING; + if ($target->getTargetStatus() == $status_pending) { + $target->setTargetStatus(HarbormasterBuildTarget::STATUS_BUILDING); + $target->save(); + } + $implementation = $target->getImplementation(); $implementation->execute($build, $target); - $target->setTargetStatus(HarbormasterBuildTarget::STATUS_PASSED); + + $next_status = HarbormasterBuildTarget::STATUS_PASSED; + if ($implementation->shouldWaitForMessage($target)) { + $next_status = HarbormasterBuildTarget::STATUS_WAITING; + } + + $target->setTargetStatus($next_status); $target->save(); } catch (Exception $ex) { phlog($ex);