From f1119ffcf51d592568281cadb7e31eed9e6f0ad7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 21 Sep 2015 04:46:24 -0700 Subject: [PATCH] Support working copies and separate allocate + activate steps for resources/leases in Drydock Summary: Ref T9253. For resources and leases that need to do something which takes a lot of time or requires waiting, allow them to allocate/acquire first and then activate later. When we allocate a resource or acquire a lease, the blueprint can either activate it immediately (if all the work can happen quickly/inline) or activate it later. If the blueprint activates it later, we queue a worker to handle activating it. Rebuild the "working copy" blueprint to work with this model: it allocates/acquires and activates in a separate step, once it is able to acquire a host. Test Plan: With some power of imagination, brought up a bunch of working copies with `bin/drydock lease --type working-copy ...` Reviewers: hach-que, chad Reviewed By: hach-que, chad Maniphest Tasks: T9253 Differential Revision: https://secure.phabricator.com/D14127 --- src/__phutil_library_map__.php | 10 +- ...anacServiceHostBlueprintImplementation.php | 19 +- .../DrydockBlueprintImplementation.php | 33 +++ ...dockWorkingCopyBlueprintImplementation.php | 233 +++++++++++++----- .../exception/DrydockSlotLockException.php | 25 ++ .../DrydockManagementLeaseWorkflow.php | 2 - .../drydock/storage/DrydockBlueprint.php | 22 ++ .../drydock/storage/DrydockLease.php | 53 +++- .../drydock/storage/DrydockResource.php | 41 ++- .../drydock/storage/DrydockSlotLock.php | 86 ++++++- .../drydock/worker/DrydockAllocatorWorker.php | 56 +++-- .../drydock/worker/DrydockLeaseWorker.php | 81 ++++++ .../drydock/worker/DrydockResourceWorker.php | 45 ++++ .../drydock/worker/DrydockWorker.php | 39 +++ .../daemon/workers/PhabricatorWorker.php | 33 +-- 15 files changed, 639 insertions(+), 139 deletions(-) create mode 100644 src/applications/drydock/exception/DrydockSlotLockException.php create mode 100644 src/applications/drydock/worker/DrydockLeaseWorker.php create mode 100644 src/applications/drydock/worker/DrydockResourceWorker.php create mode 100644 src/applications/drydock/worker/DrydockWorker.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a9e43d215e..747e085023 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -838,6 +838,7 @@ phutil_register_library_map(array( 'DrydockLeaseSearchEngine' => 'applications/drydock/query/DrydockLeaseSearchEngine.php', 'DrydockLeaseStatus' => 'applications/drydock/constants/DrydockLeaseStatus.php', 'DrydockLeaseViewController' => 'applications/drydock/controller/DrydockLeaseViewController.php', + 'DrydockLeaseWorker' => 'applications/drydock/worker/DrydockLeaseWorker.php', 'DrydockLog' => 'applications/drydock/storage/DrydockLog.php', 'DrydockLogController' => 'applications/drydock/controller/DrydockLogController.php', 'DrydockLogListController' => 'applications/drydock/controller/DrydockLogListController.php', @@ -861,10 +862,13 @@ phutil_register_library_map(array( 'DrydockResourceSearchEngine' => 'applications/drydock/query/DrydockResourceSearchEngine.php', 'DrydockResourceStatus' => 'applications/drydock/constants/DrydockResourceStatus.php', 'DrydockResourceViewController' => 'applications/drydock/controller/DrydockResourceViewController.php', + 'DrydockResourceWorker' => 'applications/drydock/worker/DrydockResourceWorker.php', 'DrydockSFTPFilesystemInterface' => 'applications/drydock/interface/filesystem/DrydockSFTPFilesystemInterface.php', 'DrydockSSHCommandInterface' => 'applications/drydock/interface/command/DrydockSSHCommandInterface.php', 'DrydockSlotLock' => 'applications/drydock/storage/DrydockSlotLock.php', + 'DrydockSlotLockException' => 'applications/drydock/exception/DrydockSlotLockException.php', 'DrydockWebrootInterface' => 'applications/drydock/interface/webroot/DrydockWebrootInterface.php', + 'DrydockWorker' => 'applications/drydock/worker/DrydockWorker.php', 'DrydockWorkingCopyBlueprintImplementation' => 'applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php', 'FeedConduitAPIMethod' => 'applications/feed/conduit/FeedConduitAPIMethod.php', 'FeedPublishConduitAPIMethod' => 'applications/feed/conduit/FeedPublishConduitAPIMethod.php', @@ -4502,7 +4506,7 @@ phutil_register_library_map(array( 'DoorkeeperSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'DoorkeeperTagView' => 'AphrontView', 'DoorkeeperTagsController' => 'PhabricatorController', - 'DrydockAllocatorWorker' => 'PhabricatorWorker', + 'DrydockAllocatorWorker' => 'DrydockWorker', 'DrydockAlmanacServiceHostBlueprintImplementation' => 'DrydockBlueprintImplementation', 'DrydockApacheWebrootInterface' => 'DrydockWebrootInterface', 'DrydockBlueprint' => array( @@ -4555,6 +4559,7 @@ phutil_register_library_map(array( 'DrydockLeaseSearchEngine' => 'PhabricatorApplicationSearchEngine', 'DrydockLeaseStatus' => 'DrydockConstants', 'DrydockLeaseViewController' => 'DrydockLeaseController', + 'DrydockLeaseWorker' => 'DrydockWorker', 'DrydockLog' => array( 'DrydockDAO', 'PhabricatorPolicyInterface', @@ -4584,10 +4589,13 @@ phutil_register_library_map(array( 'DrydockResourceSearchEngine' => 'PhabricatorApplicationSearchEngine', 'DrydockResourceStatus' => 'DrydockConstants', 'DrydockResourceViewController' => 'DrydockResourceController', + 'DrydockResourceWorker' => 'DrydockWorker', 'DrydockSFTPFilesystemInterface' => 'DrydockFilesystemInterface', 'DrydockSSHCommandInterface' => 'DrydockCommandInterface', 'DrydockSlotLock' => 'DrydockDAO', + 'DrydockSlotLockException' => 'Exception', 'DrydockWebrootInterface' => 'DrydockInterface', + 'DrydockWorker' => 'PhabricatorWorker', 'DrydockWorkingCopyBlueprintImplementation' => 'DrydockBlueprintImplementation', 'FeedConduitAPIMethod' => 'ConduitAPIMethod', 'FeedPublishConduitAPIMethod' => 'FeedConduitAPIMethod', diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php index 37fe18efab..64c002bdb0 100644 --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php @@ -76,7 +76,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation ->needSlotLock("almanac.host.binding({$binding_phid})"); try { - return $resource->allocateResource(DrydockResourceStatus::STATUS_OPEN); + return $resource->allocateResource(); } catch (Exception $ex) { $exceptions[] = $ex; } @@ -92,11 +92,9 @@ final class DrydockAlmanacServiceHostBlueprintImplementation DrydockResource $resource, DrydockLease $lease) { - // TODO: The current rule is one lease per resource, and there's no way to - // make that cheaper here than by just trying to acquire the lease below, - // so don't do any special checks for now. When we eventually permit - // multiple leases per host, we'll need to load leases anyway, so we can - // reject fully leased hosts cheaply here. + if (!DrydockSlotLock::isLockFree($this->getLeaseSlotLock($resource))) { + return false; + } return true; } @@ -106,14 +104,17 @@ final class DrydockAlmanacServiceHostBlueprintImplementation DrydockResource $resource, DrydockLease $lease) { - $resource_phid = $resource->getPHID(); - $lease ->setActivateWhenAcquired(true) - ->needSlotLock("almanac.host.lease({$resource_phid})") + ->needSlotLock($this->getLeaseSlotLock($resource)) ->acquireOnResource($resource); } + private function getLeaseSlotLock(DrydockResource $resource) { + $resource_phid = $resource->getPHID(); + return "almanac.host.lease({$resource_phid})"; + } + public function getType() { return 'host'; } diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php index b3983d250d..d5085002eb 100644 --- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php @@ -67,6 +67,12 @@ abstract class DrydockBlueprintImplementation extends Phobject { DrydockResource $resource, DrydockLease $lease); + public function activateLease( + DrydockBlueprint $blueprint, + DrydockResource $resource, + DrydockLease $lease) { + throw new PhutilMethodNotImplementedException(); + } final public function releaseLease( DrydockBlueprint $blueprint, @@ -198,6 +204,11 @@ abstract class DrydockBlueprintImplementation extends Phobject { DrydockBlueprint $blueprint, DrydockLease $lease); + public function activateResource( + DrydockBlueprint $blueprint, + DrydockResource $resource) { + throw new PhutilMethodNotImplementedException(); + } /* -( Resource Interfaces )------------------------------------------------ */ @@ -276,6 +287,9 @@ abstract class DrydockBlueprintImplementation extends Phobject { ->setStatus(DrydockResourceStatus::STATUS_PENDING) ->setName($name); + // Pre-allocate the resource PHID. + $resource->setPHID($resource->generatePHID()); + $this->activeResource = $resource; $this->log( @@ -286,6 +300,25 @@ abstract class DrydockBlueprintImplementation extends Phobject { return $resource; } + protected function newLease(DrydockBlueprint $blueprint) { + return id(new DrydockLease()); + } + + protected function requireActiveLease(DrydockLease $lease) { + $lease_status = $lease->getStatus(); + + switch ($lease_status) { + case DrydockLeaseStatus::STATUS_ACQUIRED: + // TODO: Temporary failure. + throw new Exception(pht('Lease still activating.')); + case DrydockLeaseStatus::STATUS_ACTIVE: + return; + default: + // TODO: Permanent failure. + throw new Exception(pht('Lease in bad state.')); + } + } + private function pushActiveScope( DrydockResource $resource = null, DrydockLease $lease = null) { diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index 34593bba3a..6fdc787274 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -17,21 +17,18 @@ final class DrydockWorkingCopyBlueprintImplementation public function canAnyBlueprintEverAllocateResourceForLease( DrydockLease $lease) { - // TODO: These checks are out of date. return true; } public function canEverAllocateResourceForLease( DrydockBlueprint $blueprint, DrydockLease $lease) { - // TODO: These checks are out of date. return true; } public function canAllocateResourceForLease( DrydockBlueprint $blueprint, DrydockLease $lease) { - // TODO: These checks are out of date. return true; } @@ -39,82 +36,130 @@ final class DrydockWorkingCopyBlueprintImplementation DrydockBlueprint $blueprint, DrydockResource $resource, DrydockLease $lease) { - // TODO: These checks are out of date. - $resource_repo = $resource->getAttribute('repositoryID'); - $lease_repo = $lease->getAttribute('repositoryID'); + $have_phid = $resource->getAttribute('repositoryPHID'); + $need_phid = $lease->getAttribute('repositoryPHID'); - return ($resource_repo && $lease_repo && ($resource_repo == $lease_repo)); - } - - public function allocateResource( - DrydockBlueprint $blueprint, - DrydockLease $lease) { - - $repository_id = $lease->getAttribute('repositoryID'); - if (!$repository_id) { - throw new Exception( - pht( - "Lease is missing required '%s' attribute.", - 'repositoryID')); + if ($need_phid !== $have_phid) { + return false; } - $repository = id(new PhabricatorRepositoryQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withIDs(array($repository_id)) - ->executeOne(); - - if (!$repository) { - throw new Exception( - pht( - "Repository '%s' does not exist!", - $repository_id)); + if (!DrydockSlotLock::isLockFree($this->getLeaseSlotLock($resource))) { + return false; } - switch ($repository->getVersionControlSystem()) { - case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - break; - default: - throw new Exception(pht('Unsupported VCS!')); - } - - // TODO: Policy stuff here too. - $host_lease = id(new DrydockLease()) - ->setResourceType('host') - ->waitUntilActive(); - - $path = $host_lease->getAttribute('path').$repository->getCallsign(); - - $this->log( - pht('Cloning %s into %s....', $repository->getCallsign(), $path)); - - $cmd = $host_lease->getInterface('command'); - $cmd->execx( - 'git clone --origin origin %P %s', - $repository->getRemoteURIEnvelope(), - $path); - - $this->log(pht('Complete.')); - - $resource = $this->newResourceTemplate( - $blueprint, - pht( - 'Working Copy (%s)', - $repository->getCallsign())); - $resource->setStatus(DrydockResourceStatus::STATUS_OPEN); - $resource->setAttribute('lease.host', $host_lease->getID()); - $resource->setAttribute('path', $path); - $resource->setAttribute('repositoryID', $repository->getID()); - $resource->save(); - - return $resource; + return true; } public function acquireLease( DrydockBlueprint $blueprint, DrydockResource $resource, DrydockLease $lease) { - return; + + $lease + ->needSlotLock($this->getLeaseSlotLock($resource)) + ->acquireOnResource($resource); + } + + private function getLeaseSlotLock(DrydockResource $resource) { + $resource_phid = $resource->getPHID(); + return "workingcopy.lease({$resource_phid})"; + } + + public function allocateResource( + DrydockBlueprint $blueprint, + DrydockLease $lease) { + + $repository_phid = $lease->getAttribute('repositoryPHID'); + $repository = $this->loadRepository($repository_phid); + + $resource = $this->newResourceTemplate( + $blueprint, + pht( + 'Working Copy (%s)', + $repository->getCallsign())); + + $resource_phid = $resource->getPHID(); + + $host_lease = $this->newLease($blueprint) + ->setResourceType('host') + ->setOwnerPHID($resource_phid) + ->setAttribute('workingcopy.resourcePHID', $resource_phid) + ->queueForActivation(); + + // TODO: Add some limits to the number of working copies we can have at + // once? + + return $resource + ->setAttribute('repositoryPHID', $repository->getPHID()) + ->setAttribute('host.leasePHID', $host_lease->getPHID()) + ->allocateResource(); + } + + public function activateResource( + DrydockBlueprint $blueprint, + DrydockResource $resource) { + + $lease = $this->loadHostLease($resource); + $this->requireActiveLease($lease); + + $repository_phid = $resource->getAttribute('repositoryPHID'); + $repository = $this->loadRepository($repository_phid); + $repository_id = $repository->getID(); + + $command_type = DrydockCommandInterface::INTERFACE_TYPE; + $interface = $lease->getInterface($command_type); + + // TODO: Make this configurable. + $resource_id = $resource->getID(); + $root = "/var/drydock/workingcopy-{$resource_id}"; + $path = "{$root}/repo/{$repository_id}/"; + + $interface->execx( + 'git clone -- %s %s', + (string)$repository->getCloneURIObject(), + $path); + + $resource + ->setAttribute('workingcopy.root', $root) + ->setAttribute('workingcopy.path', $path) + ->activateResource(); + } + + public function activateLease( + DrydockBlueprint $blueprint, + DrydockResource $resource, + DrydockLease $lease) { + + $command_type = DrydockCommandInterface::INTERFACE_TYPE; + $interface = $lease->getInterface($command_type); + + $cmd = array(); + $arg = array(); + + $cmd[] = 'git clean -d --force'; + $cmd[] = 'git reset --hard HEAD'; + $cmd[] = 'git fetch'; + + $commit = $lease->getAttribute('commit'); + $branch = $lease->getAttribute('branch'); + + if ($commit !== null) { + $cmd[] = 'git reset --hard %s'; + $arg[] = $commit; + } else if ($branch !== null) { + $cmd[] = 'git reset --hard %s'; + $arg[] = $branch; + } + + $cmd = implode(' && ', $cmd); + $argv = array_merge(array($cmd), $arg); + + $result = call_user_func_array( + array($interface, 'execx'), + $argv); + + $lease->activateOnResource($resource); } public function getType() { @@ -126,7 +171,59 @@ final class DrydockWorkingCopyBlueprintImplementation DrydockResource $resource, DrydockLease $lease, $type) { - // TODO: This blueprint doesn't work at all. + + switch ($type) { + case DrydockCommandInterface::INTERFACE_TYPE: + $host_lease = $this->loadHostLease($resource); + $command_interface = $host_lease->getInterface($type); + + $path = $resource->getAttribute('workingcopy.path'); + $command_interface->setWorkingDirectory($path); + + return $command_interface; + } } + private function loadRepository($repository_phid) { + $repository = id(new PhabricatorRepositoryQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs(array($repository_phid)) + ->executeOne(); + if (!$repository) { + // TODO: Permanent failure. + throw new Exception( + pht( + 'Repository PHID "%s" does not exist.', + $repository_phid)); + } + + switch ($repository->getVersionControlSystem()) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + break; + default: + // TODO: Permanent failure. + throw new Exception(pht('Unsupported VCS!')); + } + + return $repository; + } + + private function loadHostLease(DrydockResource $resource) { + $viewer = PhabricatorUser::getOmnipotentUser(); + + $lease_phid = $resource->getAttribute('host.leasePHID'); + + $lease = id(new DrydockLeaseQuery()) + ->setViewer($viewer) + ->withPHIDs(array($lease_phid)) + ->executeOne(); + if (!$lease) { + // TODO: Permanent failure. + throw new Exception(pht('Unable to load lease "%s".', $lease_phid)); + } + + return $lease; + } + + } diff --git a/src/applications/drydock/exception/DrydockSlotLockException.php b/src/applications/drydock/exception/DrydockSlotLockException.php new file mode 100644 index 0000000000..2bcf5f8cd7 --- /dev/null +++ b/src/applications/drydock/exception/DrydockSlotLockException.php @@ -0,0 +1,25 @@ +lockMap = $locks; + + if ($locks) { + $lock_list = array(); + foreach ($locks as $lock => $owner_phid) { + $lock_list[] = pht('"%s" (owned by "%s")', $lock, $owner_phid); + } + $message = pht( + 'Unable to acquire slot locks: %s.', + implode(', ', $lock_list)); + } else { + $message = pht('Unable to acquire slot locks.'); + } + + parent::__construct($message); + } + +} diff --git a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php index 238177ae62..f3617f9b44 100644 --- a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php +++ b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php @@ -40,8 +40,6 @@ final class DrydockManagementLeaseWorkflow $attributes = $options->parse($attributes); } - PhabricatorWorker::setRunAllTasksInProcess(true); - $lease = id(new DrydockLease()) ->setResourceType($resource_type); if ($attributes) { diff --git a/src/applications/drydock/storage/DrydockBlueprint.php b/src/applications/drydock/storage/DrydockBlueprint.php index 9e09ee744c..0907948d14 100644 --- a/src/applications/drydock/storage/DrydockBlueprint.php +++ b/src/applications/drydock/storage/DrydockBlueprint.php @@ -134,6 +134,15 @@ final class DrydockBlueprint extends DrydockDAO } + /** + * @task resource + */ + public function activateResource(DrydockResource $resource) { + return $this->getImplementation()->activateResource( + $this, + $resource); + } + /* -( Acquiring Leases )--------------------------------------------------- */ @@ -163,6 +172,19 @@ final class DrydockBlueprint extends DrydockDAO } + /** + * @task lease + */ + public function activateLease( + DrydockResource $resource, + DrydockLease $lease) { + return $this->getImplementation()->activateLease( + $this, + $resource, + $lease); + } + + /** * @task lease */ diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php index 1b301d93eb..475d209ea0 100644 --- a/src/applications/drydock/storage/DrydockLease.php +++ b/src/applications/drydock/storage/DrydockLease.php @@ -14,6 +14,7 @@ final class DrydockLease extends DrydockDAO private $resource = self::ATTACHABLE; private $releaseOnDestruction; private $isAcquired = false; + private $isActivated = false; private $activateWhenAcquired = false; private $slotLocks = array(); @@ -111,7 +112,12 @@ final class DrydockLease extends DrydockDAO $task = PhabricatorWorker::scheduleTask( 'DrydockAllocatorWorker', - $this->getID()); + array( + 'leasePHID' => $this->getPHID(), + ), + array( + 'objectPHID' => $this->getPHID(), + )); // NOTE: Scheduling the task might execute it in-process, if we're running // from a CLI script. Reload the lease to make sure we have the most @@ -229,11 +235,11 @@ final class DrydockLease extends DrydockDAO if ($this->activateWhenAcquired) { $new_status = DrydockLeaseStatus::STATUS_ACTIVE; } else { - $new_status = DrydockLeaseStatus::STATUS_PENDING; + $new_status = DrydockLeaseStatus::STATUS_ACQUIRED; } - if ($new_status === DrydockLeaseStatus::STATUS_ACTIVE) { - if ($resource->getStatus() === DrydockResourceStatus::STATUS_PENDING) { + if ($new_status == DrydockLeaseStatus::STATUS_ACTIVE) { + if ($resource->getStatus() == DrydockResourceStatus::STATUS_PENDING) { throw new Exception( pht( 'Trying to acquire an active lease on a pending resource. '. @@ -263,6 +269,45 @@ final class DrydockLease extends DrydockDAO return $this->isAcquired; } + public function activateOnResource(DrydockResource $resource) { + $expect_status = DrydockLeaseStatus::STATUS_ACQUIRED; + $actual_status = $this->getStatus(); + if ($actual_status != $expect_status) { + throw new Exception( + pht( + 'Trying to activate a lease which has the wrong status: status '. + 'must be "%s", actually "%s".', + $expect_status, + $actual_status)); + } + + if ($resource->getStatus() == DrydockResourceStatus::STATUS_PENDING) { + // TODO: Be stricter about this? + throw new Exception( + pht( + 'Trying to activate a lease on a pending resource.')); + } + + $this->openTransaction(); + + $this + ->setStatus(DrydockLeaseStatus::STATUS_ACTIVE) + ->save(); + + DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks); + $this->slotLocks = array(); + + $this->saveTransaction(); + + $this->isActivated = true; + + return $this; + } + + public function isActivatedLease() { + return $this->isActivated; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/drydock/storage/DrydockResource.php b/src/applications/drydock/storage/DrydockResource.php index 4f6f8b83ac..6affb7863c 100644 --- a/src/applications/drydock/storage/DrydockResource.php +++ b/src/applications/drydock/storage/DrydockResource.php @@ -16,6 +16,7 @@ final class DrydockResource extends DrydockDAO private $blueprint = self::ATTACHABLE; private $isAllocated = false; + private $isActivated = false; private $activateWhenAllocated = false; private $slotLocks = array(); @@ -86,7 +87,7 @@ final class DrydockResource extends DrydockDAO return $this; } - public function allocateResource($status) { + public function allocateResource() { if ($this->getID()) { throw new Exception( pht( @@ -131,6 +132,44 @@ final class DrydockResource extends DrydockDAO return $this->isAllocated; } + public function activateResource() { + if (!$this->getID()) { + throw new Exception( + pht( + 'Trying to activate a resource which has not yet been persisted.')); + } + + $expect_status = DrydockResourceStatus::STATUS_PENDING; + $actual_status = $this->getStatus(); + if ($actual_status != $expect_status) { + throw new Exception( + pht( + 'Trying to activate a resource from the wrong status. Status must '. + 'be "%s", actually "%s".', + $expect_status, + $actual_status)); + } + + $this->openTransaction(); + + $this + ->setStatus(DrydockResourceStatus::STATUS_OPEN) + ->save(); + + DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks); + $this->slotLocks = array(); + + $this->saveTransaction(); + + $this->isActivated = true; + + return $this; + } + + public function isActivatedResource() { + return $this->isActivated; + } + public function closeResource() { // TODO: This is super broken and will race other lease writers! diff --git a/src/applications/drydock/storage/DrydockSlotLock.php b/src/applications/drydock/storage/DrydockSlotLock.php index 2d52a81f1e..2f980660aa 100644 --- a/src/applications/drydock/storage/DrydockSlotLock.php +++ b/src/applications/drydock/storage/DrydockSlotLock.php @@ -8,6 +8,7 @@ * machine. These optimistic "slot locks" provide a flexible way to do this * sort of simple locking. * + * @task info Getting Lock Information * @task lock Acquiring and Releasing Locks */ final class DrydockSlotLock extends DrydockDAO { @@ -35,6 +36,17 @@ final class DrydockSlotLock extends DrydockDAO { ) + parent::getConfiguration(); } + +/* -( Getting Lock Information )------------------------------------------- */ + + + /** + * Load all locks held by a particular owner. + * + * @param phid Owner PHID. + * @return list All held locks. + * @task info + */ public static function loadLocks($owner_phid) { return id(new DrydockSlotLock())->loadAllWhere( 'ownerPHID = %s', @@ -42,6 +54,57 @@ final class DrydockSlotLock extends DrydockDAO { } + /** + * Test if a lock is currently free. + * + * @param string Lock key to test. + * @return bool True if the lock is currently free. + * @task info + */ + public static function isLockFree($lock) { + return self::areLocksFree(array($lock)); + } + + + /** + * Test if a list of locks are all currently free. + * + * @param list List of lock keys to test. + * @return bool True if all locks are currently free. + * @task info + */ + public static function areLocksFree(array $locks) { + $lock_map = self::loadHeldLocks($locks); + return !$lock_map; + } + + + /** + * Load named locks. + * + * @param list List of lock keys to load. + * @return list List of held locks. + * @task info + */ + public static function loadHeldLocks(array $locks) { + if (!$locks) { + return array(); + } + + $table = new DrydockSlotLock(); + $conn_r = $table->establishConnection('r'); + + $indexes = array(); + foreach ($locks as $lock) { + $indexes[] = PhabricatorHash::digestForIndex($lock); + } + + return id(new DrydockSlotLock())->loadAllWhere( + 'lockIndex IN (%Ls)', + $indexes); + } + + /* -( Acquiring and Releasing Locks )-------------------------------------- */ @@ -74,15 +137,20 @@ final class DrydockSlotLock extends DrydockDAO { $lock); } - // TODO: These exceptions are pretty tricky to read. It would be good to - // figure out which locks could not be acquired and try to improve the - // exception to make debugging easier. - - queryfx( - $conn_w, - 'INSERT INTO %T (ownerPHID, lockIndex, lockKey) VALUES %Q', - $table->getTableName(), - implode(', ', $sql)); + try { + queryfx( + $conn_w, + 'INSERT INTO %T (ownerPHID, lockIndex, lockKey) VALUES %Q', + $table->getTableName(), + implode(', ', $sql)); + } catch (AphrontDuplicateKeyQueryException $ex) { + // Try to improve the readability of the exception. We might miss on + // this query if the lock has already been released, but most of the + // 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/drydock/worker/DrydockAllocatorWorker.php b/src/applications/drydock/worker/DrydockAllocatorWorker.php index 407910ae1e..4288662434 100644 --- a/src/applications/drydock/worker/DrydockAllocatorWorker.php +++ b/src/applications/drydock/worker/DrydockAllocatorWorker.php @@ -5,33 +5,12 @@ * @task resource Managing Resources * @task lease Managing Leases */ -final class DrydockAllocatorWorker extends PhabricatorWorker { - - private function getViewer() { - return PhabricatorUser::getOmnipotentUser(); - } - - private function loadLease() { - $viewer = $this->getViewer(); - - // TODO: Make the task data a dictionary like every other worker, and - // probably make this a PHID. - $lease_id = $this->getTaskData(); - - $lease = id(new DrydockLeaseQuery()) - ->setViewer($viewer) - ->withIDs(array($lease_id)) - ->executeOne(); - if (!$lease) { - throw new PhabricatorWorkerPermanentFailureException( - pht('No such lease "%s"!', $lease_id)); - } - - return $lease; - } +final class DrydockAllocatorWorker extends DrydockWorker { protected function doWork() { - $lease = $this->loadLease(); + $lease_phid = $this->getTaskDataValue('leasePHID'); + $lease = $this->loadLease($lease_phid); + $this->allocateAndAcquireLease($lease); } @@ -351,6 +330,20 @@ final class DrydockAllocatorWorker extends PhabricatorWorker { DrydockLease $lease) { $resource = $blueprint->allocateResource($lease); $this->validateAllocatedResource($blueprint, $resource, $lease); + + // If this resource was allocated as a pending resource, queue a task to + // activate it. + if ($resource->getStatus() == DrydockResourceStatus::STATUS_PENDING) { + PhabricatorWorker::scheduleTask( + 'DrydockResourceWorker', + array( + 'resourcePHID' => $resource->getPHID(), + ), + array( + 'objectPHID' => $resource->getPHID(), + )); + } + return $resource; } @@ -429,6 +422,19 @@ final class DrydockAllocatorWorker extends PhabricatorWorker { $blueprint->acquireLease($resource, $lease); $this->validateAcquiredLease($blueprint, $resource, $lease); + + // If this lease has been acquired but not activated, queue a task to + // activate it. + if ($lease->getStatus() == DrydockLeaseStatus::STATUS_ACQUIRED) { + PhabricatorWorker::scheduleTask( + 'DrydockLeaseWorker', + array( + 'leasePHID' => $lease->getPHID(), + ), + array( + 'objectPHID' => $lease->getPHID(), + )); + } } diff --git a/src/applications/drydock/worker/DrydockLeaseWorker.php b/src/applications/drydock/worker/DrydockLeaseWorker.php new file mode 100644 index 0000000000..2143a4e4cf --- /dev/null +++ b/src/applications/drydock/worker/DrydockLeaseWorker.php @@ -0,0 +1,81 @@ +getTaskDataValue('leasePHID'); + $lease = $this->loadLease($lease_phid); + + $this->activateLease($lease); + } + + + private function activateLease(DrydockLease $lease) { + $actual_status = $lease->getStatus(); + + if ($actual_status != DrydockLeaseStatus::STATUS_ACQUIRED) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Trying to activate lease from wrong status ("%s").', + $actual_status)); + } + + $resource_id = $lease->getResourceID(); + + $resource = id(new DrydockResourceQuery()) + ->setViewer($this->getViewer()) + ->withIDs(array($resource_id)) + ->executeOne(); + if (!$resource) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Trying to activate lease on invalid resource ("%s").', + $resource_id)); + } + + $resource_status = $resource->getStatus(); + + if ($resource_status == DrydockResourceStatus::STATUS_PENDING) { + // TODO: This is explicitly a temporary failure -- we are waiting for + // the resource to come up. + throw new Exception(pht('Resource still activating.')); + } + + if ($resource_status != DrydockResourceStatus::STATUS_OPEN) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Trying to activate lease on a dead resource (in status "%s").', + $resource_status)); + } + + // NOTE: We can race resource destruction here. Between the time we + // performed the read above and now, the resource might have closed, so + // we may activate leases on dead resources. At least for now, this seems + // fine: a resource dying right before we activate a lease on it should not + // be distinguisahble from a resource dying right after we activate a lease + // on it. We end up with an active lease on a dead resource either way, and + // can not prevent resources dying from lightning strikes. + + $blueprint = $resource->getBlueprint(); + $blueprint->activateLease($resource, $lease); + $this->validateActivatedLease($blueprint, $resource, $lease); + } + + private function validateActivatedLease( + DrydockBlueprint $blueprint, + DrydockResource $resource, + DrydockLease $lease) { + + if (!$lease->isActivatedLease()) { + throw new Exception( + pht( + 'Blueprint "%s" (of type "%s") is not properly implemented: it '. + 'returned from "%s" without activating a lease.', + $blueprint->getBlueprintName(), + $blueprint->getClassName(), + 'acquireLease()')); + } + + } + +} diff --git a/src/applications/drydock/worker/DrydockResourceWorker.php b/src/applications/drydock/worker/DrydockResourceWorker.php new file mode 100644 index 0000000000..45d63029f7 --- /dev/null +++ b/src/applications/drydock/worker/DrydockResourceWorker.php @@ -0,0 +1,45 @@ +getTaskDataValue('resourcePHID'); + $resource = $this->loadResource($resource_phid); + + $this->activateResource($resource); + } + + + private function activateResource(DrydockResource $resource) { + $resource_status = $resource->getStatus(); + + if ($resource_status != DrydockResourceStatus::STATUS_PENDING) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Trying to activate resource from wrong status ("%s").', + $resource_status)); + } + + $blueprint = $resource->getBlueprint(); + $blueprint->activateResource($resource); + $this->validateActivatedResource($blueprint, $resource); + } + + + private function validateActivatedResource( + DrydockBlueprint $blueprint, + DrydockResource $resource) { + + if (!$resource->isActivatedResource()) { + throw new Exception( + pht( + 'Blueprint "%s" (of type "%s") is not properly implemented: %s '. + 'must actually allocate the resource it returns.', + $blueprint->getBlueprintName(), + $blueprint->getClassName(), + 'allocateResource()')); + } + + } + +} diff --git a/src/applications/drydock/worker/DrydockWorker.php b/src/applications/drydock/worker/DrydockWorker.php new file mode 100644 index 0000000000..abff7bcd39 --- /dev/null +++ b/src/applications/drydock/worker/DrydockWorker.php @@ -0,0 +1,39 @@ +getViewer(); + + $lease = id(new DrydockLeaseQuery()) + ->setViewer($viewer) + ->withPHIDs(array($lease_phid)) + ->executeOne(); + if (!$lease) { + throw new PhabricatorWorkerPermanentFailureException( + pht('No such lease "%s"!', $lease_phid)); + } + + return $lease; + } + + protected function loadResource($resource_phid) { + $viewer = $this->getViewer(); + + $resource = id(new DrydockResourceQuery()) + ->setViewer($viewer) + ->withPHIDs(array($resource_phid)) + ->executeOne(); + if (!$resource) { + throw new PhabricatorWorkerPermanentFailureException( + pht('No such resource "%s"!', $resource_phid)); + } + + return $resource; + } + +} diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php index 629e688b90..9d79ed5ddc 100644 --- a/src/infrastructure/daemon/workers/PhabricatorWorker.php +++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php @@ -87,6 +87,15 @@ abstract class PhabricatorWorker extends Phobject { return $this->data; } + final protected function getTaskDataValue($key, $default = null) { + $data = $this->getTaskData(); + if (!is_array($data)) { + throw new PhabricatorWorkerPermanentFailureException( + pht('Expected task data to be a dictionary.')); + } + return idx($data, $key, $default); + } + final public function executeTask() { $this->doWork(); } @@ -149,8 +158,7 @@ abstract class PhabricatorWorker extends Phobject { /** - * Wait for tasks to complete. If tasks are not leased by other workers, they - * will be executed in this process while waiting. + * Wait for tasks to complete. * * @param list List of queued task IDs to wait for. * @return void @@ -178,24 +186,9 @@ abstract class PhabricatorWorker extends Phobject { break; } - $tasks = id(new PhabricatorWorkerLeaseQuery()) - ->withIDs($waiting) - ->setLimit(1) - ->execute(); - - if (!$tasks) { - // We were not successful in leasing anything. Sleep for a bit and - // see if we have better luck later. - sleep(1); - continue; - } - - $task = head($tasks)->executeTask(); - - $ex = $task->getExecutionException(); - if ($ex) { - throw $ex; - } + // We were not successful in leasing anything. Sleep for a bit and + // see if we have better luck later. + sleep(1); } $tasks = id(new PhabricatorWorkerArchiveTaskQuery())