In Harbormaster, make sure artifacts are destroyed even if a build is aborted

Summary:
Ref T9252. Currently, Harbormaster and Drydock work like this in some cases:

  # Queue a lease for activation.
  # Then, a little later, save the lease PHID somewhere.
  # When the target/resource is destroyed, destroy the lease.

However, something can happen between (1) and (2). In Drydock this window is very short and the "something" would have to be a lighting strike or something similar, but in Harbormaster we wait until the resource activates to do (2) so the window can be many minutes long. In particular, a user can use "Abort Build" during those many minutes.

If they do, the target is destroyed but it doesn't yet have a record of the artifact, so the artifact isn't cleaned up.

Make these things work like this instead:

  # Create a new lease and pre-generate a PHID for it.
  # Save that PHID as something that needs to be cleaned up.
  # Queue the lease for activation.
  # When the target/resource is destroyed, destroy the lease if it exists.

This makes sure there's no step in the process where we might lose track of a lease/resource.

Also, clean up and standardize some other stuff I hit.

Test Plan:
  - Stopped daemons.
  - Restarted a build in Harbormaster.
  - Stepped through the build one stage at a time using `bin/worker execute ...`.
  - After the lease was queued, but before it activated, aborted the build.
  - Processed the Harbormaster side of things only.
  - Saw the lease get destroyed properly.

Reviewers: chad, hach-que

Reviewed By: hach-que

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14234

src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php

@@ -262,13 +262,14 @@ final class DrydockAlmanacServiceHostBlueprintImplementation
           array(
             DrydockResourceStatus::STATUS_PENDING,
             DrydockResourceStatus::STATUS_ACTIVE,
+            DrydockResourceStatus::STATUS_BROKEN,
             DrydockResourceStatus::STATUS_RELEASED,
           ))
         ->execute();
 
       $allocated_phids = array();
       foreach ($pool as $resource) {
-        $allocated_phids[] = $resource->getAttribute('almanacDevicePHID');
+        $allocated_phids[] = $resource->getAttribute('almanacBindingPHID');
       }
       $allocated_phids = array_fuse($allocated_phids);
 

src/applications/drydock/blueprint/DrydockBlueprintImplementation.php

@@ -288,7 +288,7 @@ abstract class DrydockBlueprintImplementation extends Phobject {
   }
 
   protected function newLease(DrydockBlueprint $blueprint) {
-    return id(new DrydockLease());
+    return DrydockLease::initializeNewLease();
   }
 
   protected function requireActiveLease(DrydockLease $lease) {

src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php

@@ -104,8 +104,13 @@ final class DrydockWorkingCopyBlueprintImplementation
     $host_lease = $this->newLease($blueprint)
       ->setResourceType('host')
       ->setOwnerPHID($resource_phid)
-      ->setAttribute('workingcopy.resourcePHID', $resource_phid)
+      ->setAttribute('workingcopy.resourcePHID', $resource_phid);
-      ->queueForActivation();
+
+    $resource
+      ->setAttribute('host.leasePHID', $host_lease->getPHID())
+      ->save();
+
+    $host_lease->queueForActivation();
 
     // TODO: Add some limits to the number of working copies we can have at
     // once?
@@ -121,7 +126,6 @@ final class DrydockWorkingCopyBlueprintImplementation
 
     return $resource
       ->setAttribute('repositories.map', $map)
-      ->setAttribute('host.leasePHID', $host_lease->getPHID())
       ->allocateResource();
   }
 
@@ -165,7 +169,13 @@ final class DrydockWorkingCopyBlueprintImplementation
     DrydockBlueprint $blueprint,
     DrydockResource $resource) {
 
-    $lease = $this->loadHostLease($resource);
+    try {
+      $lease = $this->loadHostLease($resource);
+    } catch (Exception $ex) {
+      // If we can't load the lease, assume we don't need to take any actions
+      // to destroy it.
+      return;
+    }
 
     // Destroy the lease on the host.
     $lease->releaseOnDestruction();

src/applications/drydock/logtype/DrydockLeaseWaitingForResourcesLogType.php

@@ -19,7 +19,7 @@ final class DrydockLeaseWaitingForResourcesLogType extends DrydockLogType {
 
     return pht(
       'Waiting for available resources from: %s.',
-      $viewer->renderHandleList($blueprint_phids));
+      $viewer->renderHandleList($blueprint_phids)->render());
   }
 
 }

src/applications/drydock/storage/DrydockLease.php

@@ -19,6 +19,16 @@ final class DrydockLease extends DrydockDAO
   private $activateWhenAcquired = false;
   private $slotLocks = array();
 
+  public static function initializeNewLease() {
+    $lease = new DrydockLease();
+
+    // Pregenerate a PHID so that the caller can set something up to release
+    // this lease before queueing it for activation.
+    $lease->setPHID($lease->generatePHID());
+
+    return $lease;
+  }
+
   /**
    * Flag this lease to be released when its destructor is called. This is
    * mostly useful if you have a script which acquires, uses, and then releases
@@ -232,6 +242,7 @@ final class DrydockLease extends DrydockDAO
     }
 
     $this->openTransaction();
+
     try {
       DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks);
       $this->slotLocks = array();
@@ -247,16 +258,12 @@ final class DrydockLease extends DrydockDAO
       throw $ex;
     }
 
-    try {
+    $this
-      $this
+      ->setResourcePHID($resource->getPHID())
-        ->setResourcePHID($resource->getPHID())
+      ->attachResource($resource)
-        ->attachResource($resource)
+      ->setStatus($new_status)
-        ->setStatus($new_status)
+      ->save();
-        ->save();
+
-    } catch (Exception $ex) {
-      $this->killTransaction();
-      throw $ex;
-    }
     $this->saveTransaction();
 
     $this->isAcquired = true;
@@ -295,12 +302,24 @@ final class DrydockLease extends DrydockDAO
 
     $this->openTransaction();
 
-      $this
+    try {
-        ->setStatus(DrydockLeaseStatus::STATUS_ACTIVE)
-        ->save();
-
       DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks);
       $this->slotLocks = array();
+    } catch (DrydockSlotLockException $ex) {
+      $this->killTransaction();
+
+      $this->logEvent(
+        DrydockSlotLockFailureLogType::LOGCONST,
+        array(
+          'locks' => $ex->getLockMap(),
+        ));
+
+      throw $ex;
+    }
+
+    $this
+      ->setStatus(DrydockLeaseStatus::STATUS_ACTIVE)
+      ->save();
 
     $this->saveTransaction();
 

src/applications/drydock/storage/DrydockResource.php

@@ -113,13 +113,6 @@ final class DrydockResource extends DrydockDAO
   }
 
   public function allocateResource() {
-    if ($this->getID()) {
-      throw new Exception(
-        pht(
-          'Trying to allocate a resource which has already been persisted. '.
-          'Only new resources may be allocated.'));
-    }
-
     // We expect resources to have a pregenerated PHID, as they should have
     // been created by a call to DrydockBlueprint->newResourceTemplate().
     if (!$this->getPHID()) {
@@ -155,9 +148,14 @@ final class DrydockResource extends DrydockDAO
     } catch (DrydockSlotLockException $ex) {
       $this->killTransaction();
 
-      // NOTE: We have to log this on the blueprint, as the resource is not
+      if ($this->getID()) {
-      // going to be saved so the PHID will vanish.
+        $log_target = $this;
-      $this->getBlueprint()->logEvent(
+      } else {
+        // If we don't have an ID, we have to log this on the blueprint, as the
+        // resource is not going to be saved so the PHID will vanish.
+        $log_target = $this->getBlueprint();
+      }
+      $log_target->logEvent(
         DrydockSlotLockFailureLogType::LOGCONST,
         array(
           'locks' => $ex->getLockMap(),
@@ -166,14 +164,9 @@ final class DrydockResource extends DrydockDAO
       throw $ex;
     }
 
-    try {
+    $this
-      $this
+      ->setStatus($new_status)
-        ->setStatus($new_status)
+      ->save();
-        ->save();
-    } catch (Exception $ex) {
-      $this->killTransaction();
-      throw $ex;
-    }
 
     $this->saveTransaction();
 
@@ -210,12 +203,24 @@ final class DrydockResource extends DrydockDAO
 
     $this->openTransaction();
 
-      $this
+    try {
-        ->setStatus(DrydockResourceStatus::STATUS_ACTIVE)
-        ->save();
-
       DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks);
       $this->slotLocks = array();
+    } catch (DrydockSlotLockException $ex) {
+      $this->killTransaction();
+
+      $this->logEvent(
+        DrydockSlotLockFailureLogType::LOGCONST,
+        array(
+          'locks' => $ex->getLockMap(),
+        ));
+
+      throw $ex;
+    }
+
+    $this
+      ->setStatus(DrydockResourceStatus::STATUS_ACTIVE)
+      ->save();
 
     $this->saveTransaction();
 

src/applications/drydock/storage/DrydockSlotLock.php

@@ -149,6 +149,7 @@ final class DrydockSlotLock extends DrydockDAO {
       // time we should be able to figure out which locks are already held.
       $held = self::loadHeldLocks($locks);
       $held = mpull($held, 'getOwnerPHID', 'getLockKey');
+
       throw new DrydockSlotLockException($held);
     }
   }

src/applications/harbormaster/artifact/HarbormasterDrydockLeaseArtifact.php

@@ -29,7 +29,9 @@ abstract class HarbormasterDrydockLeaseArtifact
   }
 
   public function willCreateArtifact(PhabricatorUser $actor) {
-    $this->loadArtifactLease($actor);
+    // We don't load the lease here because it's expected that artifacts are
+    // created before leases actually exist. This guarantees that the leases
+    // will be cleaned up.
   }
 
   public function loadArtifactLease(PhabricatorUser $viewer) {
@@ -51,7 +53,15 @@ abstract class HarbormasterDrydockLeaseArtifact
   }
 
   public function releaseArtifact(PhabricatorUser $actor) {
-    $lease = $this->loadArtifactLease($actor);
+    try {
+      $lease = $this->loadArtifactLease($actor);
+    } catch (Exception $ex) {
+      // If we can't load the lease, treat it as already released. Artifacts
+      // are generated before leases are queued, so it's possible to arrive
+      // here under normal conditions.
+      return;
+    }
+
     if (!$lease->canRelease()) {
       return;
     }

src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php

@@ -41,7 +41,7 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation
       $working_copy_type = id(new DrydockWorkingCopyBlueprintImplementation())
         ->getType();
 
-      $lease = id(new DrydockLease())
+      $lease = DrydockLease::initializeNewLease()
         ->setResourceType($working_copy_type)
         ->setOwnerPHID($build_target->getPHID());
 
@@ -54,6 +54,18 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation
         $lease->setAwakenTaskIDs(array($task_id));
       }
 
+      // TODO: Maybe add a method to mark artifacts like this as pending?
+
+      // Create the artifact now so that the lease is always disposed of, even
+      // if this target is aborted.
+      $build_target->createArtifact(
+        $viewer,
+        $settings['name'],
+        HarbormasterWorkingCopyArtifact::ARTIFACTCONST,
+        array(
+          'drydockLeasePHID' => $lease->getPHID(),
+        ));
+
       $lease->queueForActivation();
 
       $build_target
@@ -73,14 +85,6 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation
           'Lease "%s" never activated.',
           $lease->getPHID()));
     }
-
-    $artifact = $build_target->createArtifact(
-      $viewer,
-      $settings['name'],
-      HarbormasterWorkingCopyArtifact::ARTIFACTCONST,
-      array(
-        'drydockLeasePHID' => $lease->getPHID(),
-      ));
   }
 
   public function getArtifactOutputs() {

src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementExecuteWorkflow.php

@@ -42,7 +42,8 @@ final class PhabricatorWorkerManagementExecuteWorkflow
         $task->getDataID());
       $task->setData($task_data->getData());
 
-      $console->writeOut(
+      echo tsprintf(
+        "%s\n",
         pht(
           'Executing task %d (%s)...',
           $task->getID(),

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php

@@ -1398,6 +1398,11 @@ final class PhabricatorUSEnglishTranslation
         'Setting retention policy for "%s" to %s days.',
       ),
 
+      'Waiting %s second(s) for lease to activate.' => array(
+        'Waiting a second for lease to activate.',
+        'Waiting %s seconds for lease to activate.',
+      ),
+
     );
   }