From e589d152310a0b3727ee9454d2ca772ae5694261 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 Oct 2015 08:12:51 -0700 Subject: [PATCH] Improve error and exception handling for Drydock resources Summary: Ref T9252. Currently, error handling behavior isn't great and a lot of errors aren't dealt with properly. Try to improve this by making default behaviors better: - Yields, slot lock exceptions, and aggregate or proxy exceptions containing an excpetion of these types turn into yields. - All other exceptions are considered permanent failures. They break the resource and This feels a little bit "magical" but I want to try to get the default behaviors to align reasonably well with expectations so that blueprints mostly don't need to have a ton of error handling. This will probably need at least some refinement down the road, but it's a reasonable rule for all exception/error conditions we currently have. Test Plan: I did a clean build, but haven't vetted this super thoroughly. Next diff will do the same thing to leases, then I'll work on stabilizing this code better. Reviewers: chad, hach-que Reviewed By: hach-que Maniphest Tasks: T9252 Differential Revision: https://secure.phabricator.com/D14211 --- ...anacServiceHostBlueprintImplementation.php | 1 - ...dockWorkingCopyBlueprintImplementation.php | 17 ++- .../worker/DrydockResourceUpdateWorker.php | 109 ++++++++++++++++-- .../drydock/worker/DrydockWorker.php | 41 +++++++ 4 files changed, 152 insertions(+), 16 deletions(-) diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php index 4ae64b39be..9a71474624 100644 --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php @@ -163,7 +163,6 @@ final class DrydockAlmanacServiceHostBlueprintImplementation ->withPHIDs(array($binding_phid)) ->executeOne(); if (!$binding) { - // TODO: This is probably a permanent failure, destroy this resource? throw new Exception( pht( 'Unable to load binding "%s" to create command interface.', diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index f9a8794391..a61af96ebf 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -297,7 +297,6 @@ final class DrydockWorkingCopyBlueprintImplementation foreach ($phids as $phid) { if (empty($repositories[$phid])) { - // TODO: Permanent failure. throw new Exception( pht( 'Repository PHID "%s" does not exist.', @@ -306,12 +305,16 @@ final class DrydockWorkingCopyBlueprintImplementation } foreach ($repositories as $repository) { - switch ($repository->getVersionControlSystem()) { + $repository_vcs = $repository->getVersionControlSystem(); + switch ($repository_vcs) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: break; default: - // TODO: Permanent failure. - throw new Exception(pht('Unsupported VCS!')); + throw new Exception( + pht( + 'Repository ("%s") has unsupported VCS ("%s").', + $repository->getPHID(), + $repository_vcs)); } } @@ -328,8 +331,10 @@ final class DrydockWorkingCopyBlueprintImplementation ->withPHIDs(array($lease_phid)) ->executeOne(); if (!$lease) { - // TODO: Permanent failure. - throw new Exception(pht('Unable to load lease "%s".', $lease_phid)); + throw new Exception( + pht( + 'Unable to load lease ("%s").', + $lease_phid)); } return $lease; diff --git a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php index 9027829b9c..e0deabdb1b 100644 --- a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php @@ -1,9 +1,11 @@ loadResource($resource_phid); - $this->updateResource($resource); + $this->handleUpdate($resource); } catch (Exception $ex) { $lock->unlock(); throw $ex; @@ -28,6 +30,37 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { $lock->unlock(); } + +/* -( Updating Resources )------------------------------------------------- */ + + + /** + * Update a resource, handling exceptions thrown during the update. + * + * @param DrydockReosource Resource to update. + * @return void + * @task update + */ + private function handleUpdate(DrydockResource $resource) { + try { + $this->updateResource($resource); + } catch (Exception $ex) { + if ($this->isTemporaryException($ex)) { + $this->yieldResource($resource, $ex); + } else { + $this->breakResource($resource, $ex); + } + } + } + + + /** + * Update a resource. + * + * @param DrydockResource Resource to update. + * @return void + * @task update + */ private function updateResource(DrydockResource $resource) { $this->processResourceCommands($resource); @@ -52,6 +85,26 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { } + /** + * Convert a temporary exception into a yield. + * + * @param DrydockResource Resource to yield. + * @param Exception Temporary exception worker encountered. + * @task update + */ + private function yieldResource(DrydockResource $resource, Exception $ex) { + $duration = $this->getYieldDurationFromException($ex); + + $resource->logEvent( + DrydockResourceActivationYieldLogType::LOGCONST, + array( + 'duration' => $duration, + )); + + throw new PhabricatorWorkerYieldException($duration); + } + + /* -( Processing Commands )------------------------------------------------ */ @@ -138,14 +191,9 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { $viewer = $this->getViewer(); $drydock_phid = id(new PhabricatorDrydockApplication())->getPHID(); - $resource->openTransaction(); - $resource - ->setStatus(DrydockResourceStatus::STATUS_RELEASED) - ->save(); - - // TODO: Hold slot locks until destruction? - DrydockSlotLock::releaseLocks($resource->getPHID()); - $resource->saveTransaction(); + $resource + ->setStatus(DrydockResourceStatus::STATUS_RELEASED) + ->save(); $statuses = array( DrydockLeaseStatus::STATUS_PENDING, @@ -173,6 +221,47 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { } +/* -( Breaking Resources )------------------------------------------------- */ + + + /** + * @task break + */ + private function breakResource(DrydockResource $resource, Exception $ex) { + switch ($resource->getStatus()) { + case DrydockResourceStatus::STATUS_BROKEN: + case DrydockResourceStatus::STATUS_RELEASED: + case DrydockResourceStatus::STATUS_DESTROYED: + // If the resource was already broken, just throw a normal exception. + // This will retry the task eventually. + throw new PhutilProxyException( + pht( + 'Unexpected failure while destroying resource ("%s").', + $resource->getPHID()), + $ex); + } + + $resource + ->setStatus(DrydockResourceStatus::STATUS_BROKEN) + ->save(); + + $resource->scheduleUpdate(); + + $resource->logEvent( + DrydockResourceActivationFailureLogType::LOGCONST, + array( + 'class' => get_class($ex), + 'message' => $ex->getMessage(), + )); + + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Permanent failure while activating resource ("%s"): %s', + $resource->getPHID(), + $ex->getMessage())); + } + + /* -( Destroying Resources )----------------------------------------------- */ @@ -183,6 +272,8 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { $blueprint = $resource->getBlueprint(); $blueprint->destroyResource($resource); + DrydockSlotLock::releaseLocks($resource->getPHID()); + $resource ->setStatus(DrydockResourceStatus::STATUS_DESTROYED) ->save(); diff --git a/src/applications/drydock/worker/DrydockWorker.php b/src/applications/drydock/worker/DrydockWorker.php index fb2666f735..d2dc1ca399 100644 --- a/src/applications/drydock/worker/DrydockWorker.php +++ b/src/applications/drydock/worker/DrydockWorker.php @@ -114,4 +114,45 @@ abstract class DrydockWorker extends PhabricatorWorker { throw new PhabricatorWorkerYieldException($expires - $now); } + protected function isTemporaryException(Exception $ex) { + if ($ex instanceof PhabricatorWorkerYieldException) { + return true; + } + + if ($ex instanceof DrydockSlotLockException) { + return true; + } + + if ($ex instanceof PhutilAggregateException) { + $any_temporary = false; + foreach ($ex->getExceptions() as $sub) { + if ($this->isTemporaryException($sub)) { + $any_temporary = true; + break; + } + } + if ($any_temporary) { + return true; + } + } + + if ($ex instanceof PhutilProxyException) { + return $this->isTemporaryException($ex->getPreviousException()); + } + + return false; + } + + protected function getYieldDurationFromException(Exception $ex) { + if ($ex instanceof PhabricatorWorkerYieldException) { + return $ex->getDuration(); + } + + if ($ex instanceof DrydockSlotLockException) { + return 5; + } + + return 15; + } + }