From 51b34c0544291711eb49fd5a8deafa82eca5d421 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 26 Aug 2014 20:46:23 +1000 Subject: [PATCH] Abort previous build targets when a build is restarted Summary: Ref T5936. This implements build implementations aborting early when the build has since been restarted. Build steps now periodically poll to see if the build's current generation does not match their generation, and they throw a `HarbormasterBuildAbortedException` if that is the case. Test Plan: Tested locally on my machine with the sleep build step. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley, Korvin Maniphest Tasks: T5936 Differential Revision: https://secure.phabricator.com/D10322 --- src/__phutil_library_map__.php | 2 + .../engine/HarbormasterBuildEngine.php | 4 +- .../HarbormasterBuildAbortedException.php | 5 ++ .../HarbormasterBuildStepImplementation.php | 25 +++++++++ ...ormasterCommandBuildStepImplementation.php | 55 +++++++++++++------ ...sterHTTPRequestBuildStepImplementation.php | 5 +- ...rbormasterSleepBuildStepImplementation.php | 21 ++++++- .../storage/build/HarbormasterBuildTarget.php | 7 +++ .../worker/HarbormasterTargetWorker.php | 9 +++ 9 files changed, 113 insertions(+), 20 deletions(-) create mode 100644 src/applications/harbormaster/exception/HarbormasterBuildAbortedException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0adb3af9db..7006beff68 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -654,6 +654,7 @@ phutil_register_library_map(array( 'FlagEditConduitAPIMethod' => 'applications/flag/conduit/FlagEditConduitAPIMethod.php', 'FlagQueryConduitAPIMethod' => 'applications/flag/conduit/FlagQueryConduitAPIMethod.php', 'HarbormasterBuild' => 'applications/harbormaster/storage/build/HarbormasterBuild.php', + 'HarbormasterBuildAbortedException' => 'applications/harbormaster/exception/HarbormasterBuildAbortedException.php', 'HarbormasterBuildActionController' => 'applications/harbormaster/controller/HarbormasterBuildActionController.php', 'HarbormasterBuildArtifact' => 'applications/harbormaster/storage/build/HarbormasterBuildArtifact.php', 'HarbormasterBuildArtifactQuery' => 'applications/harbormaster/query/HarbormasterBuildArtifactQuery.php', @@ -3419,6 +3420,7 @@ phutil_register_library_map(array( 'HarbormasterDAO', 'PhabricatorPolicyInterface', ), + 'HarbormasterBuildAbortedException' => 'Exception', 'HarbormasterBuildActionController' => 'HarbormasterController', 'HarbormasterBuildArtifact' => array( 'HarbormasterDAO', diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php index a3faf45314..98e67635f3 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -130,9 +130,9 @@ final class HarbormasterBuildEngine extends Phobject { // Increment the build generation counter on the build. $build->setBuildGeneration($build->getBuildGeneration() + 1); - // TODO: Currently running targets should periodically check their build + // Currently running targets should periodically check their build // generation (which won't have changed) against the build's generation. - // If it is different, they should automatically stop what they're doing + // If it is different, they will automatically stop what they're doing // and abort. // Previously we used to delete targets, logs and artifacts here. Instead diff --git a/src/applications/harbormaster/exception/HarbormasterBuildAbortedException.php b/src/applications/harbormaster/exception/HarbormasterBuildAbortedException.php new file mode 100644 index 0000000000..7cbb8cf44b --- /dev/null +++ b/src/applications/harbormaster/exception/HarbormasterBuildAbortedException.php @@ -0,0 +1,5 @@ +getDetail('builtin.wait-for-message'); } + protected function shouldAbort( + HarbormasterBuild $build, + HarbormasterBuildTarget $target) { + + return $build->getBuildGeneration() !== $target->getBuildGeneration(); + } + + protected function resolveFuture( + HarbormasterBuild $build, + HarbormasterBuildTarget $target, + Future $future) { + + $futures = Futures(array($future)); + foreach ($futures->setUpdateInterval(5) as $key => $future) { + if ($future === null) { + $build->reload(); + if ($this->shouldAbort($build, $target)) { + throw new HarbormasterBuildAbortedException(); + } + } else { + return $future->resolve(); + } + } + } + } diff --git a/src/applications/harbormaster/step/HarbormasterCommandBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterCommandBuildStepImplementation.php index d16f11fcfc..806e311083 100644 --- a/src/applications/harbormaster/step/HarbormasterCommandBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterCommandBuildStepImplementation.php @@ -62,26 +62,49 @@ final class HarbormasterCommandBuildStepImplementation $start_stdout = $log_stdout->start(); $start_stderr = $log_stderr->start(); + $build_update = 5; + // Read the next amount of available output every second. - while (!$future->isReady()) { - list($stdout, $stderr) = $future->read(); - $log_stdout->append($stdout); - $log_stderr->append($stderr); - $future->discardBuffers(); + $futures = Futures(array($future)); + foreach ($futures->setUpdateInterval(1) as $key => $future_iter) { + if ($future_iter === null) { - // Wait one second before querying for more data. - sleep(1); + // Check to see if we should abort. + if ($build_update <= 0) { + $build->reload(); + if ($this->shouldAbort($build, $build_target)) { + $future->resolveKill(); + throw new HarbormasterBuildAbortedException(); + } else { + $build_update = 5; + } + } else { + $build_update -= 1; + } + + // Command is still executing. + + // Read more data as it is available. + list($stdout, $stderr) = $future->read(); + $log_stdout->append($stdout); + $log_stderr->append($stderr); + $future->discardBuffers(); + } else { + // Command execution is complete. + + // Get the return value so we can log that as well. + list($err) = $future->resolve(); + + // Retrieve the last few bits of information. + list($stdout, $stderr) = $future->read(); + $log_stdout->append($stdout); + $log_stderr->append($stderr); + $future->discardBuffers(); + + break; + } } - // Get the return value so we can log that as well. - list($err) = $future->resolve(); - - // Retrieve the last few bits of information. - list($stdout, $stderr) = $future->read(); - $log_stdout->append($stdout); - $log_stderr->append($stderr); - $future->discardBuffers(); - $log_stdout->finalize($start_stdout); $log_stderr->finalize($start_stderr); diff --git a/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php index 10df86689f..dfc29cedc7 100644 --- a/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php @@ -66,7 +66,10 @@ final class HarbormasterHTTPRequestBuildStepImplementation $key->getPasswordEnvelope()); } - list($status, $body, $headers) = $future->resolve(); + list($status, $body, $headers) = $this->resolveFuture( + $build, + $build_target, + $future); $log_body->append($body); $log_body->finalize($start); diff --git a/src/applications/harbormaster/step/HarbormasterSleepBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterSleepBuildStepImplementation.php index bca222c17a..968c5b99cb 100644 --- a/src/applications/harbormaster/step/HarbormasterSleepBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterSleepBuildStepImplementation.php @@ -23,7 +23,26 @@ final class HarbormasterSleepBuildStepImplementation $settings = $this->getSettings(); - sleep($settings['seconds']); + $target = time() + $settings['seconds']; + + // Use $build_update so that we only reload every 5 seconds, but + // the sleep mechanism remains accurate. + $build_update = 5; + + while (time() < $target) { + sleep(1); + + if ($build_update <= 0) { + $build->reload(); + $build_update = 5; + + if ($this->shouldAbort($build, $build_target)) { + throw new HarbormasterBuildAbortedException(); + } + } else { + $build_update -= 1; + } + } } public function getFieldSpecifications() { diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php index 3271903d70..1a142dc5cb 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php @@ -19,6 +19,7 @@ final class HarbormasterBuildTarget extends HarbormasterDAO const STATUS_WAITING = 'target/waiting'; const STATUS_PASSED = 'target/passed'; const STATUS_FAILED = 'target/failed'; + const STATUS_ABORTED = 'target/aborted'; private $build = self::ATTACHABLE; private $buildStep = self::ATTACHABLE; @@ -36,6 +37,8 @@ final class HarbormasterBuildTarget extends HarbormasterDAO return pht('Passed'); case self::STATUS_FAILED: return pht('Failed'); + case self::STATUS_ABORTED: + return pht('Aborted'); default: return pht('Unknown'); } @@ -52,6 +55,8 @@ final class HarbormasterBuildTarget extends HarbormasterDAO return PHUIStatusItemView::ICON_ACCEPT; case self::STATUS_FAILED: return PHUIStatusItemView::ICON_REJECT; + case self::STATUS_ABORTED: + return PHUIStatusItemView::ICON_MINUS; default: return PHUIStatusItemView::ICON_QUESTION; } @@ -66,6 +71,7 @@ final class HarbormasterBuildTarget extends HarbormasterDAO case self::STATUS_PASSED: return 'green'; case self::STATUS_FAILED: + case self::STATUS_ABORTED: return 'red'; default: return 'bluegrey'; @@ -179,6 +185,7 @@ final class HarbormasterBuildTarget extends HarbormasterDAO switch ($this->getTargetStatus()) { case self::STATUS_PASSED: case self::STATUS_FAILED: + case self::STATUS_ABORTED: return true; } diff --git a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php index f76108f934..180f19907a 100644 --- a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php @@ -38,6 +38,10 @@ final class HarbormasterTargetWorker extends HarbormasterWorker { $target->setDateStarted(time()); try { + if ($target->getBuildGeneration() !== $build->getBuildGeneration()) { + throw new HarbormasterBuildAbortedException(); + } + $status_pending = HarbormasterBuildTarget::STATUS_PENDING; if ($target->getTargetStatus() == $status_pending) { $target->setTargetStatus(HarbormasterBuildTarget::STATUS_BUILDING); @@ -68,6 +72,11 @@ final class HarbormasterTargetWorker extends HarbormasterWorker { $target->setTargetStatus(HarbormasterBuildTarget::STATUS_FAILED); $target->setDateCompleted(time()); $target->save(); + } catch (HarbormasterBuildAbortedException $ex) { + // A build step is aborting because the build has been restarted. + $target->setTargetStatus(HarbormasterBuildTarget::STATUS_ABORTED); + $target->setDateCompleted(time()); + $target->save(); } catch (Exception $ex) { phlog($ex);