From 996930da2ae0773b78ec336819f1ca1660b60482 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Jan 2014 12:21:49 -0800 Subject: [PATCH] Improve several exception behaviors for Harbormaster workers Summary: Ref T2015. Several fixes: - `checkForCancellation()` no longer exists, and isn't relevant for resumable stops. Throw it away for now. - Fix an issue where a build could pass even if the final step failed. - `phlog()` exceptions so they show up in `bin/harbormaster` and the daemon logs. - Write an exception log if a step fails. - Add a "throw an exception" step to debug this stuff more easily. Test Plan: - Grepped for `checkForCancellation()`. - Ran a failing build where the final step caused the failure. - Observed `phlog()` in `bin/harbormaster` output. - Observed log in web UI: {F101168} Reviewers: btrahan, hach-que Reviewed By: btrahan CC: aran Maniphest Tasks: T2015 Differential Revision: https://secure.phabricator.com/D7935 --- src/__phutil_library_map__.php | 2 ++ .../engine/HarbormasterBuildEngine.php | 14 +++++------ .../step/CommandBuildStepImplementation.php | 12 +-------- .../HarbormasterThrowExceptionBuildStep.php | 25 +++++++++++++++++++ ...WaitForPreviousBuildStepImplementation.php | 17 ++----------- .../worker/HarbormasterTargetWorker.php | 11 ++++++++ 6 files changed, 48 insertions(+), 33 deletions(-) create mode 100644 src/applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a10670934b..623adccae7 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -767,6 +767,7 @@ phutil_register_library_map(array( 'HarbormasterStepDeleteController' => 'applications/harbormaster/controller/HarbormasterStepDeleteController.php', 'HarbormasterStepEditController' => 'applications/harbormaster/controller/HarbormasterStepEditController.php', 'HarbormasterTargetWorker' => 'applications/harbormaster/worker/HarbormasterTargetWorker.php', + 'HarbormasterThrowExceptionBuildStep' => 'applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php', 'HarbormasterUIEventListener' => 'applications/harbormaster/event/HarbormasterUIEventListener.php', 'HarbormasterWorker' => 'applications/harbormaster/worker/HarbormasterWorker.php', 'HeraldAction' => 'applications/herald/storage/HeraldAction.php', @@ -3260,6 +3261,7 @@ phutil_register_library_map(array( 'HarbormasterStepDeleteController' => 'HarbormasterController', 'HarbormasterStepEditController' => 'HarbormasterController', 'HarbormasterTargetWorker' => 'HarbormasterWorker', + 'HarbormasterThrowExceptionBuildStep' => 'BuildStepImplementation', 'HarbormasterUIEventListener' => 'PhabricatorEventListener', 'HarbormasterWorker' => 'PhabricatorWorker', 'HeraldAction' => 'HeraldDAO', diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php index 9c531e1419..fc0ae9bcdc 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -163,17 +163,17 @@ final class HarbormasterBuildEngine extends Phobject { } } - // 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); + // If any step failed, fail the whole build, then bail. + if (count($failed)) { + $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED); $build->save(); return; } - // If any step failed, fail the whole build, then bail. - if (count($failed)) { - $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED); + // 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; } diff --git a/src/applications/harbormaster/step/CommandBuildStepImplementation.php b/src/applications/harbormaster/step/CommandBuildStepImplementation.php index 6e1cd49549..16312edcfd 100644 --- a/src/applications/harbormaster/step/CommandBuildStepImplementation.php +++ b/src/applications/harbormaster/step/CommandBuildStepImplementation.php @@ -53,16 +53,6 @@ final class CommandBuildStepImplementation $log_stderr->append($stderr); $future->discardBuffers(); - // Check to see if we have moved from a "Building" status. This - // can occur if the user cancels the build, in which case we want - // to terminate whatever we're doing and return as quickly as possible. - if ($build->checkForCancellation()) { - $log_stdout->finalize($start_stdout); - $log_stderr->finalize($start_stderr); - $future->resolveKill(); - return; - } - // Wait one second before querying for more data. sleep(1); } @@ -80,7 +70,7 @@ final class CommandBuildStepImplementation $log_stderr->finalize($start_stderr); if ($err) { - $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED); + throw new Exception(pht('Command failed with error %d.', $err)); } } diff --git a/src/applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php b/src/applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php new file mode 100644 index 0000000000..3a2adb8d41 --- /dev/null +++ b/src/applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php @@ -0,0 +1,25 @@ +setBuildStatus(HarbormasterBuild::STATUS_WAITING); - $build->save(); - // Block until all previous builds of the same build plan have // finished. $plan = $build->getBuildPlan(); @@ -42,13 +38,6 @@ final class WaitForPreviousBuildStepImplementation $log_start = null; $blockers = $this->getBlockers($object, $plan, $build); while (count($blockers) > 0) { - if ($build->checkForCancellation()) { - if ($log !== null) { - $log->finalize($log_start); - } - return; - } - if ($log === null) { $log = $build->createLog($build_target, "waiting", "blockers"); $log_start = $log->start(); @@ -56,16 +45,14 @@ final class WaitForPreviousBuildStepImplementation $log->append("Blocked by: ".implode(",", $blockers)."\n"); + // TODO: This should fail temporarily instead after setting the target to + // waiting, and thereby push the build into a waiting status. sleep(1); $blockers = $this->getBlockers($object, $plan, $build); } if ($log !== null) { $log->finalize($log_start); } - - // Move back into building status. - $build->setBuildStatus(HarbormasterBuild::STATUS_BUILDING); - $build->save(); } private function getBlockers( diff --git a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php index afc6b3f770..33f23afe62 100644 --- a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php @@ -46,6 +46,17 @@ final class HarbormasterTargetWorker extends HarbormasterWorker { $target->save(); } } catch (Exception $ex) { + phlog($ex); + + try { + $log = $build->createLog($target, 'core', 'exception'); + $start = $log->start(); + $log->append((string)$ex); + $log->finalize($start); + } catch (Exception $log_ex) { + phlog($log_ex); + } + $target->setTargetStatus(HarbormasterBuildTarget::STATUS_FAILED); $target->save(); }