From 4cf1270ecdd8570f904d48533795aa66b7e39d30 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 5 Oct 2015 05:58:53 -0700 Subject: [PATCH] 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 --- ...anacServiceHostBlueprintImplementation.php | 3 +- .../DrydockBlueprintImplementation.php | 2 +- ...dockWorkingCopyBlueprintImplementation.php | 18 +++++-- ...DrydockLeaseWaitingForResourcesLogType.php | 2 +- .../drydock/storage/DrydockLease.php | 47 ++++++++++++------ .../drydock/storage/DrydockResource.php | 49 ++++++++++--------- .../drydock/storage/DrydockSlotLock.php | 1 + .../HarbormasterDrydockLeaseArtifact.php | 14 +++++- ...easeWorkingCopyBuildStepImplementation.php | 22 +++++---- ...ricatorWorkerManagementExecuteWorkflow.php | 3 +- .../PhabricatorUSEnglishTranslation.php | 5 ++ 11 files changed, 111 insertions(+), 55 deletions(-) diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php index e458020600..e62c7b7558 100644 --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/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); diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php index 513d7b37f9..e3cdff34c5 100644 --- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php +++ b/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) { diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index 7a34e2d3bc..e086294c4d 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/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) - ->queueForActivation(); + ->setAttribute('workingcopy.resourcePHID', $resource_phid); + + $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(); diff --git a/src/applications/drydock/logtype/DrydockLeaseWaitingForResourcesLogType.php b/src/applications/drydock/logtype/DrydockLeaseWaitingForResourcesLogType.php index 1faf762ae8..46ab965b36 100644 --- a/src/applications/drydock/logtype/DrydockLeaseWaitingForResourcesLogType.php +++ b/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()); } } diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php index aa2ea753f0..01ccb0a5c4 100644 --- a/src/applications/drydock/storage/DrydockLease.php +++ b/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 - ->setResourcePHID($resource->getPHID()) - ->attachResource($resource) - ->setStatus($new_status) - ->save(); - } catch (Exception $ex) { - $this->killTransaction(); - throw $ex; - } + $this + ->setResourcePHID($resource->getPHID()) + ->attachResource($resource) + ->setStatus($new_status) + ->save(); + $this->saveTransaction(); $this->isAcquired = true; @@ -295,12 +302,24 @@ final class DrydockLease extends DrydockDAO $this->openTransaction(); - $this - ->setStatus(DrydockLeaseStatus::STATUS_ACTIVE) - ->save(); - + try { 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(); diff --git a/src/applications/drydock/storage/DrydockResource.php b/src/applications/drydock/storage/DrydockResource.php index da97a089d9..1ee663fa3c 100644 --- a/src/applications/drydock/storage/DrydockResource.php +++ b/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 - // going to be saved so the PHID will vanish. - $this->getBlueprint()->logEvent( + if ($this->getID()) { + $log_target = $this; + } 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 - ->setStatus($new_status) - ->save(); - } catch (Exception $ex) { - $this->killTransaction(); - throw $ex; - } + $this + ->setStatus($new_status) + ->save(); $this->saveTransaction(); @@ -210,12 +203,24 @@ final class DrydockResource extends DrydockDAO $this->openTransaction(); - $this - ->setStatus(DrydockResourceStatus::STATUS_ACTIVE) - ->save(); - + try { 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(); diff --git a/src/applications/drydock/storage/DrydockSlotLock.php b/src/applications/drydock/storage/DrydockSlotLock.php index 2f980660aa..7e4e320946 100644 --- a/src/applications/drydock/storage/DrydockSlotLock.php +++ b/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); } } diff --git a/src/applications/harbormaster/artifact/HarbormasterDrydockLeaseArtifact.php b/src/applications/harbormaster/artifact/HarbormasterDrydockLeaseArtifact.php index bd3868e7db..e7a2ac41e6 100644 --- a/src/applications/harbormaster/artifact/HarbormasterDrydockLeaseArtifact.php +++ b/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; } diff --git a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php index 001a981a60..91c6eb0517 100644 --- a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php +++ b/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() { diff --git a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementExecuteWorkflow.php b/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementExecuteWorkflow.php index 3826075276..2acc8452ea 100644 --- a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementExecuteWorkflow.php +++ b/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(), diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index 055b85f0df..beda39c21c 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/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.', + ), + ); }