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

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.',

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;

src/applications/drydock/worker/DrydockResourceUpdateWorker.php

@@ -1,9 +1,11 @@
 <?php
 
 /**
+ * @task update Updating Resources
  * @task command Processing Commands
  * @task activate Activating Resources
  * @task release Releasing Resources
+ * @task break Breaking Resources
  * @task destroy Destroying Resources
  */
 final class DrydockResourceUpdateWorker extends DrydockWorker {
@@ -19,7 +21,7 @@ final class DrydockResourceUpdateWorker extends DrydockWorker {
 
     try {
       $resource = $this->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,15 +191,10 @@ 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();
-
     $statuses = array(
       DrydockLeaseStatus::STATUS_PENDING,
       DrydockLeaseStatus::STATUS_ACQUIRED,
@@ -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();

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;
+  }
+
 }