From 89b37f0357da3813274e2c05a2718879b83f4895 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 Nov 2012 16:53:17 -0700 Subject: [PATCH] Make various Drydock improvements Summary: Tightens up a bunch of stuff: - In `drydock lease`, pull and print logs so the user can see what's happening. - Remove `DrydockAllocator`, which was a dumb class that did nothing. Move the tiny amount of logic it held directly to `DrydockLease`. - Move `resourceType` from worker task metadata directly to `DrydockLease`. Other things (like the web UI) can be more informative with this information available. - Pass leases to `allocateResource()`. We always allocate in response to a lease activation request, and the lease often has vital information. This also allows us to associate logs with leases correctly. Test Plan: Ran `drydock lease --type host` and saw it perform a host allocation in EC2. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2015 Differential Revision: https://secure.phabricator.com/D3870 --- resources/sql/patches/drydockresourcetype.sql | 2 + src/__phutil_library_map__.php | 3 +- .../drydock/allocator/DrydockAllocator.php | 59 ------------------- .../drydock/blueprint/DrydockBlueprint.php | 18 +++--- .../blueprint/DrydockEC2HostBlueprint.php | 4 +- ...DrydockPhabricatorApplicationBlueprint.php | 7 ++- .../webroot/DrydockApacheWebrootBlueprint.php | 2 +- .../DrydockManagementLeaseWorkflow.php | 34 +++++++---- .../drydock/query/DrydockLogQuery.php | 45 ++++++++++---- .../drydock/storage/DrydockLease.php | 36 +++++++++-- .../DrydockAllocatorWorker.php | 12 ++-- .../patch/PhabricatorBuiltinPatchList.php | 4 ++ 12 files changed, 116 insertions(+), 110 deletions(-) create mode 100644 resources/sql/patches/drydockresourcetype.sql delete mode 100644 src/applications/drydock/allocator/DrydockAllocator.php rename src/applications/drydock/{allocator => worker}/DrydockAllocatorWorker.php (89%) diff --git a/resources/sql/patches/drydockresourcetype.sql b/resources/sql/patches/drydockresourcetype.sql new file mode 100644 index 0000000000..f6a794d095 --- /dev/null +++ b/resources/sql/patches/drydockresourcetype.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_drydock.drydock_lease + ADD resourceType VARCHAR(128) NOT NULL COLLATE utf8_bin; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 472e3d39db..d2e9c42053 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -411,8 +411,7 @@ phutil_register_library_map(array( 'DiffusionURITestCase' => 'applications/diffusion/request/__tests__/DiffusionURITestCase.php', 'DiffusionView' => 'applications/diffusion/view/DiffusionView.php', 'DivinerListController' => 'applications/diviner/controller/DivinerListController.php', - 'DrydockAllocator' => 'applications/drydock/allocator/DrydockAllocator.php', - 'DrydockAllocatorWorker' => 'applications/drydock/allocator/DrydockAllocatorWorker.php', + 'DrydockAllocatorWorker' => 'applications/drydock/worker/DrydockAllocatorWorker.php', 'DrydockApacheWebrootBlueprint' => 'applications/drydock/blueprint/webroot/DrydockApacheWebrootBlueprint.php', 'DrydockApacheWebrootInterface' => 'applications/drydock/interface/webroot/DrydockApacheWebrootInterface.php', 'DrydockBlueprint' => 'applications/drydock/blueprint/DrydockBlueprint.php', diff --git a/src/applications/drydock/allocator/DrydockAllocator.php b/src/applications/drydock/allocator/DrydockAllocator.php deleted file mode 100644 index 5e7ce6d8a3..0000000000 --- a/src/applications/drydock/allocator/DrydockAllocator.php +++ /dev/null @@ -1,59 +0,0 @@ -resourceType = $resource_type; - return $this; - } - - public function getResourceType() { - return $this->resourceType; - } - - public function getPendingLease() { - if (!$this->lease) { - $lease = new DrydockLease(); - $lease->setStatus(DrydockLeaseStatus::STATUS_PENDING); - $lease->save(); - - $this->lease = $lease; - } - return $lease; - } - - public function allocate() { - $lease = $this->getPendingLease(); - - $data = array( - 'type' => $this->getResourceType(), - 'lease' => $lease->getID(), - ); - - $task = PhabricatorWorker::scheduleTask('DrydockAllocatorWorker', $data); - - $lease->setTaskID($task->getID()); - - return $lease; - } - -} diff --git a/src/applications/drydock/blueprint/DrydockBlueprint.php b/src/applications/drydock/blueprint/DrydockBlueprint.php index 58ec9fdad9..bcfef77d16 100644 --- a/src/applications/drydock/blueprint/DrydockBlueprint.php +++ b/src/applications/drydock/blueprint/DrydockBlueprint.php @@ -33,13 +33,6 @@ abstract class DrydockBlueprint { return; } - protected function getAllocator($type) { - $allocator = new DrydockAllocator(); - $allocator->setResourceType($type); - - return $allocator; - } - final public function acquireLease( DrydockResource $resource, DrydockLease $lease) { @@ -101,13 +94,18 @@ abstract class DrydockBlueprint { return false; } - protected function executeAllocateResource() { + protected function executeAllocateResource(DrydockLease $lease) { throw new Exception("This blueprint can not allocate resources!"); } - final public function allocateResource() { + final public function allocateResource(DrydockLease $lease) { + $this->activeLease = $lease; + $this->activeResource = null; + + $this->log('Allocating Resource'); + try { - $resource = $this->executeAllocateResource(); + $resource = $this->executeAllocateResource($lease); } catch (Exception $ex) { $this->logException($ex); $this->activeResource = null; diff --git a/src/applications/drydock/blueprint/DrydockEC2HostBlueprint.php b/src/applications/drydock/blueprint/DrydockEC2HostBlueprint.php index b25757d1c4..03183f1b6d 100644 --- a/src/applications/drydock/blueprint/DrydockEC2HostBlueprint.php +++ b/src/applications/drydock/blueprint/DrydockEC2HostBlueprint.php @@ -22,7 +22,7 @@ final class DrydockEC2HostBlueprint extends DrydockRemoteHostBlueprint { return true; } - public function executeAllocateResource() { + public function executeAllocateResource(DrydockLease $lease) { $resource = $this->newResourceTemplate('EC2 Host'); $resource->setStatus(DrydockResourceStatus::STATUS_ALLOCATING); @@ -41,7 +41,7 @@ final class DrydockEC2HostBlueprint extends DrydockRemoteHostBlueprint { $instance_id = (string)$xml->instancesSet[0]->item[0]->instanceId[0]; - $this->log('Started Instance: {$instance_id}'); + $this->log("Started Instance: {$instance_id}"); $resource->setAttribute('instance.id', $instance_id); $resource->save(); diff --git a/src/applications/drydock/blueprint/application/DrydockPhabricatorApplicationBlueprint.php b/src/applications/drydock/blueprint/application/DrydockPhabricatorApplicationBlueprint.php index 44f029b1a7..4d293460e2 100644 --- a/src/applications/drydock/blueprint/application/DrydockPhabricatorApplicationBlueprint.php +++ b/src/applications/drydock/blueprint/application/DrydockPhabricatorApplicationBlueprint.php @@ -27,15 +27,16 @@ abstract class DrydockPhabricatorApplicationBlueprint return true; } - public function executeAllocateResource() { + public function executeAllocateResource(DrydockLease $lease) { $resource = $this->newResourceTemplate('Phabricator'); $resource->setStatus(DrydockResourceStatus::STATUS_ALLOCATING); $resource->save(); - $allocator = $this->getAllocator('host'); - $host = $allocator->allocate(); + $host = id(new DrydockLease()) + ->setResourceType('host') + ->queueForActivation(); $cmd = $host->waitUntilActive()->getInterface('command'); diff --git a/src/applications/drydock/blueprint/webroot/DrydockApacheWebrootBlueprint.php b/src/applications/drydock/blueprint/webroot/DrydockApacheWebrootBlueprint.php index 641b974271..b4e5d94073 100644 --- a/src/applications/drydock/blueprint/webroot/DrydockApacheWebrootBlueprint.php +++ b/src/applications/drydock/blueprint/webroot/DrydockApacheWebrootBlueprint.php @@ -76,7 +76,7 @@ EOSETUP $lease->save(); } - public function executeAllocateResource() { + public function executeAllocateResource(DrydockLease $lease) { $resource = $this->newResourceTemplate('Apache'); diff --git a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php index 60beddae1e..649ea87245 100644 --- a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php +++ b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php @@ -31,7 +31,7 @@ final class DrydockManagementLeaseWorkflow 'help' => 'Resource type.', ), array( - 'name' => 'spec', + 'name' => 'attributes', 'param' => 'name=value,...', 'help' => 'Resource specficiation.', ), @@ -47,19 +47,18 @@ final class DrydockManagementLeaseWorkflow "Specify a resource type with `--type`."); } - $spec = $args->getArg('spec'); - if ($spec) { + $attributes = $args->getArg('attributes'); + if ($attributes) { $options = new PhutilSimpleOptions(); - $spec = $options->parse($spec); + $attributes = $options->parse($attributes); } - $allocator = new DrydockAllocator(); - $allocator->setResourceType($resource_type); - if ($spec) { - // TODO: Shove this in there. + $lease = new DrydockLease(); + $lease->setResourceType($resource_type); + if ($attributes) { + $lease->setAttributes($attributes); } - - $lease = $allocator->allocate(); + $lease->queueForActivation(); $root = dirname(phutil_get_library_root('phabricator')); $wait = new ExecFuture( @@ -67,14 +66,25 @@ final class DrydockManagementLeaseWorkflow $root.'/scripts/drydock/drydock_control.php', $lease->getID()); + $cursor = 0; foreach (Futures(array($wait))->setUpdateInterval(1) as $key => $future) { if ($future) { $future->resolvex(); break; } - // TODO: Pull logs. - $console->writeErr("Working...\n"); + $logs = id(new DrydockLogQuery()) + ->withLeaseIDs(array($lease->getID())) + ->withAfterID($cursor) + ->setOrder(DrydockLogQuery::ORDER_ID) + ->execute(); + + if ($logs) { + foreach ($logs as $log) { + $console->writeErr("%s\n", $log->getMessage()); + } + $cursor = max(mpull($logs, 'getID')); + } } $console->writeOut("Acquired Lease %s\n", $lease->getID()); diff --git a/src/applications/drydock/query/DrydockLogQuery.php b/src/applications/drydock/query/DrydockLogQuery.php index c4940026b1..f7c904dc91 100644 --- a/src/applications/drydock/query/DrydockLogQuery.php +++ b/src/applications/drydock/query/DrydockLogQuery.php @@ -18,8 +18,13 @@ final class DrydockLogQuery extends PhabricatorOffsetPagedQuery { + const ORDER_EPOCH = 'order-epoch'; + const ORDER_ID = 'order-id'; + private $resourceIDs; private $leaseIDs; + private $afterID; + private $order = self::ORDER_EPOCH; public function withResourceIDs(array $ids) { $this->resourceIDs = $ids; @@ -31,26 +36,32 @@ final class DrydockLogQuery extends PhabricatorOffsetPagedQuery { return $this; } + public function setOrder($order) { + $this->order = $order; + return $this; + } + + public function withAfterID($id) { + $this->afterID = $id; + return $this; + } + public function execute() { $table = new DrydockLog(); $conn_r = $table->establishConnection('r'); - $where = $this->buildWhereClause($conn_r); - $order = $this->buildOrderClause($conn_r); - $limit = $this->buildLimitClause($conn_r); - $data = queryfx_all( $conn_r, 'SELECT log.* FROM %T log %Q %Q %Q', $table->getTableName(), - $where, - $order, - $limit); + $this->buildWhereClause($conn_r), + $this->buildOrderClause($conn_r), + $this->buildLimitClause($conn_r)); return $table->loadAllFromArray($data); } - private function buildWhereClause($conn_r) { + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); if ($this->resourceIDs) { @@ -67,11 +78,25 @@ final class DrydockLogQuery extends PhabricatorOffsetPagedQuery { $this->leaseIDs); } + if ($this->afterID) { + $where[] = qsprintf( + $conn_r, + 'id > %d', + $this->afterID); + } + return $this->formatWhereClause($where); } - private function buildOrderClause($conn_r) { - return 'ORDER BY log.epoch DESC'; + private function buildOrderClause(AphrontDatabaseConnection $conn_r) { + switch ($this->order) { + case self::ORDER_EPOCH: + return 'ORDER BY log.epoch DESC'; + case self::ORDER_ID: + return 'ORDER BY id ASC'; + default: + throw new Exception("Unknown order '{$this->order}'!"); + } } } diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php index b0e960afb1..b18faefe4c 100644 --- a/src/applications/drydock/storage/DrydockLease.php +++ b/src/applications/drydock/storage/DrydockLease.php @@ -20,10 +20,11 @@ final class DrydockLease extends DrydockDAO { protected $phid; protected $resourceID; + protected $resourceType; protected $until; protected $ownerPHID; protected $attributes = array(); - protected $status; + protected $status = DrydockLeaseStatus::STATUS_PENDING; protected $taskID; private $resource; @@ -76,10 +77,37 @@ final class DrydockLease extends DrydockDAO { $this->getResourceID()); } + public function queueForActivation() { + if ($this->getID()) { + throw new Exception( + "Only new leases may be queued for activation!"); + } + + $this->setStatus(DrydockLeaseStatus::STATUS_PENDING); + $this->save(); + + // NOTE: Prevent a race where some eager worker quickly grabs the task + // before we can save the Task ID. + + $this->openTransaction(); + $this->beginReadLocking(); + + $this->reload(); + + $task = PhabricatorWorker::scheduleTask( + 'DrydockAllocatorWorker', + $this->getID()); + + $this->setTaskID($task->getID()); + $this->save(); + + $this->endReadLocking(); + $this->saveTransaction(); + + return $this; + } + public function release() { - - // TODO: Insert a cleanup task into the taskmaster queue. - $this->setStatus(DrydockLeaseStatus::STATUS_RELEASED); $this->save(); diff --git a/src/applications/drydock/allocator/DrydockAllocatorWorker.php b/src/applications/drydock/worker/DrydockAllocatorWorker.php similarity index 89% rename from src/applications/drydock/allocator/DrydockAllocatorWorker.php rename to src/applications/drydock/worker/DrydockAllocatorWorker.php index bf2e4c7564..46123c41cc 100644 --- a/src/applications/drydock/allocator/DrydockAllocatorWorker.php +++ b/src/applications/drydock/worker/DrydockAllocatorWorker.php @@ -19,20 +19,18 @@ final class DrydockAllocatorWorker extends PhabricatorWorker { protected function doWork() { - $data = $this->getTaskData(); + $lease_id = $this->getTaskData(); - $lease = id(new DrydockLease())->loadOneWhere( - 'id = %d', - $data['lease']); + $lease = id(new DrydockLease())->load($lease_id); if (!$lease) { return; } - $type = $data['type']; + $type = $lease->getResourceType(); $candidates = id(new DrydockResource())->loadAllWhere( 'type = %s AND status = %s', - $type, + $lease->getResourceType(), DrydockResourceStatus::STATUS_OPEN); if ($candidates) { @@ -64,7 +62,7 @@ final class DrydockAllocatorWorker extends PhabricatorWorker { shuffle($blueprints); $blueprint = head($blueprints); - $resource = $blueprint->allocateResource(); + $resource = $blueprint->allocateResource($lease); } $blueprint = $resource->getBlueprint(); diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 7bcd08a90b..11531c8555 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1024,6 +1024,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('drydocktaskid.sql'), ), + 'drydockresoucetype.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('drydockresourcetype.sql'), + ), ); }