From 91e5ca0ee28ca0e0424608bcf20f4cae3a420bb2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 Oct 2015 08:10:40 -0700 Subject: [PATCH] Merge the DrydockResource workers into a single worker Summary: Ref T9252. Currently, Drydock Leases and Resources have several workers: - Resources: ResourceWorker, ResourceUpdateWorker, ResourceDestroyWorker - Leases: AllocatorWorker, LeaseWorker, LeaseUpdateWorker, LeaseDestroyWorker This is kind of a lot of stuff, and it creates some problems. In particular, leases and resources in early lifecycle phases (pending/allocating/acquiring) can't process commands yet, because that code is only in the "UpdateWorker" classes. If they aren't able to move forward because of a bug, they also can't be released because they can't react to the release command until later in their lifecycle. This creates a soft hang where I have to go wipe stuff out of the database since there's no other way to get rid of it. Instead, I want leases and resources to be releasable from any (pre-release / pre-destroy) phase of their lifecycle. To support this, all the workers before the "UpdateWorker" need to be able to process commands. A second, similar issue is that logging and exception handling behaviors are underpowered right now. Elsewhere I began improving this, but ran into issues where all of the workers needed to share very similar exception code. Merging them will make this future change simpler. This diff fixes this for resources: it merges the Worker, UpdateWorker and DestroyWorker logic into UpdateWorker and throws away the other two workers. Test Plan: Nothing substantive yet, see next diff. I'll do the same thing for Leases, then test both more thoroughly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9252 Differential Revision: https://secure.phabricator.com/D14201 --- src/__phutil_library_map__.php | 4 - .../drydock/storage/DrydockResource.php | 10 +- .../drydock/worker/DrydockAllocatorWorker.php | 2 +- .../worker/DrydockResourceDestroyWorker.php | 35 ------ .../worker/DrydockResourceUpdateWorker.php | 118 +++++++++++++++--- .../drydock/worker/DrydockResourceWorker.php | 45 ------- .../drydock/worker/DrydockWorker.php | 2 +- 7 files changed, 107 insertions(+), 109 deletions(-) delete mode 100644 src/applications/drydock/worker/DrydockResourceDestroyWorker.php delete mode 100644 src/applications/drydock/worker/DrydockResourceWorker.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f07f29299e..d4bcc2b6a0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -867,7 +867,6 @@ phutil_register_library_map(array( 'DrydockResource' => 'applications/drydock/storage/DrydockResource.php', 'DrydockResourceController' => 'applications/drydock/controller/DrydockResourceController.php', 'DrydockResourceDatasource' => 'applications/drydock/typeahead/DrydockResourceDatasource.php', - 'DrydockResourceDestroyWorker' => 'applications/drydock/worker/DrydockResourceDestroyWorker.php', 'DrydockResourceListController' => 'applications/drydock/controller/DrydockResourceListController.php', 'DrydockResourceListView' => 'applications/drydock/view/DrydockResourceListView.php', 'DrydockResourcePHIDType' => 'applications/drydock/phid/DrydockResourcePHIDType.php', @@ -877,7 +876,6 @@ phutil_register_library_map(array( 'DrydockResourceStatus' => 'applications/drydock/constants/DrydockResourceStatus.php', 'DrydockResourceUpdateWorker' => 'applications/drydock/worker/DrydockResourceUpdateWorker.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', @@ -4618,7 +4616,6 @@ phutil_register_library_map(array( ), 'DrydockResourceController' => 'DrydockController', 'DrydockResourceDatasource' => 'PhabricatorTypeaheadDatasource', - 'DrydockResourceDestroyWorker' => 'DrydockWorker', 'DrydockResourceListController' => 'DrydockResourceController', 'DrydockResourceListView' => 'AphrontView', 'DrydockResourcePHIDType' => 'PhabricatorPHIDType', @@ -4628,7 +4625,6 @@ phutil_register_library_map(array( 'DrydockResourceStatus' => 'DrydockConstants', 'DrydockResourceUpdateWorker' => 'DrydockWorker', 'DrydockResourceViewController' => 'DrydockResourceController', - 'DrydockResourceWorker' => 'DrydockWorker', 'DrydockSFTPFilesystemInterface' => 'DrydockFilesystemInterface', 'DrydockSSHCommandInterface' => 'DrydockCommandInterface', 'DrydockSlotLock' => 'DrydockDAO', diff --git a/src/applications/drydock/storage/DrydockResource.php b/src/applications/drydock/storage/DrydockResource.php index ab2230ce32..2070cf8b33 100644 --- a/src/applications/drydock/storage/DrydockResource.php +++ b/src/applications/drydock/storage/DrydockResource.php @@ -246,12 +246,14 @@ final class DrydockResource extends DrydockDAO } } - public function canUpdate() { + public function canReceiveCommands() { switch ($this->getStatus()) { - case DrydockResourceStatus::STATUS_ACTIVE: - return true; - default: + case DrydockResourceStatus::STATUS_RELEASED: + case DrydockResourceStatus::STATUS_BROKEN: + case DrydockResourceStatus::STATUS_DESTROYED: return false; + default: + return true; } } diff --git a/src/applications/drydock/worker/DrydockAllocatorWorker.php b/src/applications/drydock/worker/DrydockAllocatorWorker.php index a67f242c01..08a702b589 100644 --- a/src/applications/drydock/worker/DrydockAllocatorWorker.php +++ b/src/applications/drydock/worker/DrydockAllocatorWorker.php @@ -333,7 +333,7 @@ final class DrydockAllocatorWorker extends DrydockWorker { // activate it. if ($resource->getStatus() == DrydockResourceStatus::STATUS_PENDING) { PhabricatorWorker::scheduleTask( - 'DrydockResourceWorker', + 'DrydockResourceUpdateWorker', array( 'resourcePHID' => $resource->getPHID(), ), diff --git a/src/applications/drydock/worker/DrydockResourceDestroyWorker.php b/src/applications/drydock/worker/DrydockResourceDestroyWorker.php deleted file mode 100644 index af00ebeb2a..0000000000 --- a/src/applications/drydock/worker/DrydockResourceDestroyWorker.php +++ /dev/null @@ -1,35 +0,0 @@ -getTaskDataValue('resourcePHID'); - $resource = $this->loadResource($resource_phid); - $this->destroyResource($resource); - } - - private function destroyResource(DrydockResource $resource) { - $status = $resource->getStatus(); - - switch ($status) { - case DrydockResourceStatus::STATUS_RELEASED: - case DrydockResourceStatus::STATUS_BROKEN: - break; - default: - throw new PhabricatorWorkerPermanentFailureException( - pht( - 'Unable to destroy resource ("%s"), resource has the wrong '. - 'status ("%s").', - $resource->getPHID(), - $status)); - } - - $blueprint = $resource->getBlueprint(); - $blueprint->destroyResource($resource); - - $resource - ->setStatus(DrydockResourceStatus::STATUS_DESTROYED) - ->save(); - } - -} diff --git a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php index 681741b6f1..9027829b9c 100644 --- a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php @@ -1,5 +1,11 @@ canUpdate()) { + $this->processResourceCommands($resource); + + $resource_status = $resource->getStatus(); + switch ($resource_status) { + case DrydockResourceStatus::STATUS_PENDING: + $this->activateResource($resource); + break; + case DrydockResourceStatus::STATUS_ACTIVE: + // Nothing to do. + break; + case DrydockResourceStatus::STATUS_RELEASED: + case DrydockResourceStatus::STATUS_BROKEN: + $this->destroyResource($resource); + break; + case DrydockResourceStatus::STATUS_DESTROYED: + // Nothing to do. + break; + } + + $this->yieldIfExpiringResource($resource); + } + + +/* -( Processing Commands )------------------------------------------------ */ + + + /** + * @task command + */ + private function processResourceCommands(DrydockResource $resource) { + if (!$resource->canReceiveCommands()) { return; } @@ -31,21 +67,23 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { $commands = $this->loadCommands($resource->getPHID()); foreach ($commands as $command) { - if (!$resource->canUpdate()) { + if (!$resource->canReceiveCommands()) { break; } - $this->processCommand($resource, $command); + $this->processResourceCommand($resource, $command); $command ->setIsConsumed(true) ->save(); } - - $this->yieldIfExpiringResource($resource); } - private function processCommand( + + /** + * @task command + */ + private function processResourceCommand( DrydockResource $resource, DrydockCommand $command) { @@ -56,13 +94,47 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { } } - private function releaseResource(DrydockResource $resource) { - if ($resource->getStatus() != DrydockResourceStatus::STATUS_ACTIVE) { - // If we had multiple release commands - // This command is only meaningful to resources in the "Open" state. - return; + +/* -( Activating Resources )----------------------------------------------- */ + + + /** + * @task activate + */ + private function activateResource(DrydockResource $resource) { + $blueprint = $resource->getBlueprint(); + $blueprint->activateResource($resource); + $this->validateActivatedResource($blueprint, $resource); + } + + + /** + * @task activate + */ + 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()')); } + } + + +/* -( Releasing Resources )------------------------------------------------ */ + + + /** + * @task release + */ + private function releaseResource(DrydockResource $resource) { $viewer = $this->getViewer(); $drydock_phid = id(new PhabricatorDrydockApplication())->getPHID(); @@ -97,14 +169,22 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { $lease->scheduleUpdate(); } - PhabricatorWorker::scheduleTask( - 'DrydockResourceDestroyWorker', - array( - 'resourcePHID' => $resource->getPHID(), - ), - array( - 'objectPHID' => $resource->getPHID(), - )); + $this->destroyResource($resource); } + +/* -( Destroying Resources )----------------------------------------------- */ + + + /** + * @task destroy + */ + private function destroyResource(DrydockResource $resource) { + $blueprint = $resource->getBlueprint(); + $blueprint->destroyResource($resource); + + $resource + ->setStatus(DrydockResourceStatus::STATUS_DESTROYED) + ->save(); + } } diff --git a/src/applications/drydock/worker/DrydockResourceWorker.php b/src/applications/drydock/worker/DrydockResourceWorker.php deleted file mode 100644 index 45d63029f7..0000000000 --- a/src/applications/drydock/worker/DrydockResourceWorker.php +++ /dev/null @@ -1,45 +0,0 @@ -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 index e64cfdafd7..a7273b2f9d 100644 --- a/src/applications/drydock/worker/DrydockWorker.php +++ b/src/applications/drydock/worker/DrydockWorker.php @@ -94,7 +94,7 @@ abstract class DrydockWorker extends PhabricatorWorker { } protected function yieldIfExpiringResource(DrydockResource $resource) { - if (!$resource->canUpdate()) { + if (!$resource->canReceiveCommands()) { return; }