From f0fdcf1a516c9475362acb465c066ff32bfd9ccd Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 Nov 2012 11:30:42 -0700 Subject: [PATCH] Undumb the Drydock resource allocator pipeline Summary: This was the major goal of D3859/D3855, and to a lesser degree D3854/D3852. As Drydock is allocating a resource, it may need to allocate other resources first. For example, if it's allocating a working copy, it may need to allocate a host first. Currently, we have the process basically queue up the allocation (insert a task into the queue) and sleep() until it finishes. This is problematic for a bunch of reasons, but the major one is that if allocation takes more resources (host, port, machine, DNS) than you have daemons, they could all end up sleeping and waiting for some other daemon to do their work. This is really stupid. Even if you only take up some of them, you're spending slots sleeping when you could be doing useful work. To partially get around this and make the CLI experience less dumb, there's this goofy `synchronous` flag that gets passed around everywhere and pushes the workflow through a pile of special cases. Basically the `synchronous` flag causes us to do everything in-process. But this is dumb too because we'd rather do things in parallel if we can, and we have to have a lot of special case code to make it work at all. Get rid of all of this. Instead of sleep()ing, try to work on the tasks that need to be worked on. If another daemon grabbed them already that's fine, but in the worst case we just gracefully degrade and do everything in process. So we get the best of both worlds: if we have parallelizable tasks and free daemons, things will execute in parallel. If we have nonparallelizable tasks or no free daemons, things will execute in process. Test Plan: Ran `drydock_control.php --trace` and saw it perform cascading allocations without sleeping or special casing. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2015 Differential Revision: https://secure.phabricator.com/D3861 --- resources/sql/patches/drydocktaskid.sql | 1 + scripts/drydock/drydock_control.php | 9 ++-- .../drydock/allocator/DrydockAllocator.php | 15 ++---- .../allocator/DrydockAllocatorWorker.php | 5 -- .../drydock/blueprint/DrydockBlueprint.php | 8 ---- .../drydock/storage/DrydockLease.php | 47 +++++++++++++------ .../daemon/workers/PhabricatorWorker.php | 43 +++++++++++++++++ .../query/PhabricatorWorkerLeaseQuery.php | 18 +++++-- .../patch/PhabricatorBuiltinPatchList.php | 4 ++ 9 files changed, 102 insertions(+), 48 deletions(-) create mode 100644 resources/sql/patches/drydocktaskid.sql diff --git a/resources/sql/patches/drydocktaskid.sql b/resources/sql/patches/drydocktaskid.sql new file mode 100644 index 0000000000..7c3869d885 --- /dev/null +++ b/resources/sql/patches/drydocktaskid.sql @@ -0,0 +1 @@ +ALTER TABLE {$NAMESPACE}_drydock.drydock_lease ADD taskID INT UNSIGNED; diff --git a/scripts/drydock/drydock_control.php b/scripts/drydock/drydock_control.php index e3a7612e8d..4bb361d2b7 100755 --- a/scripts/drydock/drydock_control.php +++ b/scripts/drydock/drydock_control.php @@ -20,10 +20,11 @@ $root = dirname(dirname(dirname(__FILE__))); require_once $root.'/scripts/__init_script__.php'; -PhutilServiceProfiler::installEchoListener(); +$args = new PhutilArgumentParser($argv); +$args->parseStandardArguments(); +$args->parse(array()); $allocator = new DrydockAllocator(); -$allocator->makeSynchronous(); $allocator->setResourceType('webroot'); $lease = $allocator->allocate(); @@ -46,5 +47,5 @@ echo $stdout; $lease->release(); -//$i_http = $lease->getInterface('httpd'); -//echo $i_http->getURI('/index.html')."\n"; +// $i_http = $lease->getInterface('httpd'); +// echo $i_http->getURI('/index.html')."\n"; diff --git a/src/applications/drydock/allocator/DrydockAllocator.php b/src/applications/drydock/allocator/DrydockAllocator.php index 1df11bcb09..5e7ce6d8a3 100644 --- a/src/applications/drydock/allocator/DrydockAllocator.php +++ b/src/applications/drydock/allocator/DrydockAllocator.php @@ -20,7 +20,6 @@ final class DrydockAllocator { private $resourceType; private $lease; - private $synchronous; public function setResourceType($resource_type) { $this->resourceType = $resource_type; @@ -31,10 +30,6 @@ final class DrydockAllocator { return $this->resourceType; } - public function makeSynchronous() { - $this->synchronous = true; - } - public function getPendingLease() { if (!$this->lease) { $lease = new DrydockLease(); @@ -54,13 +49,9 @@ final class DrydockAllocator { 'lease' => $lease->getID(), ); - if ($this->synchronous) { - $data['synchronous'] = true; - $worker = new DrydockAllocatorWorker($data); - $worker->executeTask(); - } else { - PhabricatorWorker::scheduleTask('DrydockAllocatorWorker', $data); - } + $task = PhabricatorWorker::scheduleTask('DrydockAllocatorWorker', $data); + + $lease->setTaskID($task->getID()); return $lease; } diff --git a/src/applications/drydock/allocator/DrydockAllocatorWorker.php b/src/applications/drydock/allocator/DrydockAllocatorWorker.php index 1ad03b3778..bf2e4c7564 100644 --- a/src/applications/drydock/allocator/DrydockAllocatorWorker.php +++ b/src/applications/drydock/allocator/DrydockAllocatorWorker.php @@ -64,11 +64,6 @@ final class DrydockAllocatorWorker extends PhabricatorWorker { shuffle($blueprints); $blueprint = head($blueprints); - - if (isset($data['synchronous'])) { - $blueprint->makeSynchronous(); - } - $resource = $blueprint->allocateResource(); } diff --git a/src/applications/drydock/blueprint/DrydockBlueprint.php b/src/applications/drydock/blueprint/DrydockBlueprint.php index d043d3b5d2..58ec9fdad9 100644 --- a/src/applications/drydock/blueprint/DrydockBlueprint.php +++ b/src/applications/drydock/blueprint/DrydockBlueprint.php @@ -20,11 +20,6 @@ abstract class DrydockBlueprint { private $activeLease; private $activeResource; - private $synchronous; - - final public function makeSynchronous() { - $this->synchronous = true; - } abstract public function getType(); abstract public function getInterface( @@ -40,9 +35,6 @@ abstract class DrydockBlueprint { protected function getAllocator($type) { $allocator = new DrydockAllocator(); - if ($this->synchronous) { - $allocator->makeSynchronous(); - } $allocator->setResourceType($type); return $allocator; diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php index c4978b1893..b0e960afb1 100644 --- a/src/applications/drydock/storage/DrydockLease.php +++ b/src/applications/drydock/storage/DrydockLease.php @@ -24,6 +24,7 @@ final class DrydockLease extends DrydockDAO { protected $ownerPHID; protected $attributes = array(); protected $status; + protected $taskID; private $resource; @@ -95,25 +96,43 @@ final class DrydockLease extends DrydockDAO { } } - public function waitUntilActive() { - $this->reload(); + public static function waitForLeases(array $leases) { + assert_instances_of($leases, 'DrydockLease'); + + $task_ids = array_filter(mpull($leases, 'getTaskID')); + PhabricatorWorker::waitForTasks($task_ids); + + $unresolved = $leases; while (true) { - switch ($this->status) { - case DrydockLeaseStatus::STATUS_ACTIVE: - break 2; - case DrydockLeaseStatus::STATUS_RELEASED: - case DrydockLeaseStatus::STATUS_EXPIRED: - case DrydockLeaseStatus::STATUS_BROKEN: - throw new Exception("Lease will never become active!"); - case DrydockLeaseStatus::STATUS_PENDING: - break; + foreach ($unresolved as $key => $lease) { + $lease->reload(); + switch ($lease->getStatus()) { + case DrydockLeaseStatus::STATUS_ACTIVE: + unset($unresolved[$key]); + break; + case DrydockLeaseStatus::STATUS_RELEASED: + case DrydockLeaseStatus::STATUS_EXPIRED: + case DrydockLeaseStatus::STATUS_BROKEN: + throw new Exception("Lease will never become active!"); + case DrydockLeaseStatus::STATUS_PENDING: + break; + } + } + + if ($unresolved) { + sleep(1); + } else { + break; } - sleep(2); - $this->reload(); } - $this->attachResource($this->loadResource()); + foreach ($leases as $lease) { + $lease->attachResource($lease->loadResource()); + } + } + public function waitUntilActive() { + self::waitForLeases(array($this)); return $this; } diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php index 703fae925d..23e339cfef 100644 --- a/src/infrastructure/daemon/workers/PhabricatorWorker.php +++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php @@ -107,4 +107,47 @@ abstract class PhabricatorWorker { ->save(); } + + /** + * Wait for tasks to complete. If tasks are not leased by other workers, they + * will be executed in this process while waiting. + * + * @param list List of queued task IDs to wait for. + * @return void + */ + final public static function waitForTasks(array $task_ids) { + $task_table = new PhabricatorWorkerActiveTask(); + + $waiting = array_combine($task_ids, $task_ids); + while ($waiting) { + $conn_w = $task_table->establishConnection('w'); + + // Check if any of the tasks we're waiting on are still queued. If they + // are not, we're done waiting. + $row = queryfx_one( + $conn_w, + 'SELECT COUNT(*) N FROM %T WHERE id IN (%Ld)', + $task_table->getTableName(), + $waiting); + if (!$row['N']) { + // Nothing is queued anymore. Stop waiting. + 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(); + } + } + } diff --git a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php index 961aba1295..8929658e36 100644 --- a/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php +++ b/src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php @@ -25,6 +25,7 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { const PHASE_UNLEASED = 'unleased'; const PHASE_EXPIRED = 'expired'; + const PHASE_SELECT = 'select'; const DEFAULT_LEASE_DURATION = 60; // Seconds @@ -66,7 +67,8 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { foreach ($phases as $phase) { queryfx( $conn_w, - 'UPDATE %T SET leaseOwner = %s, leaseExpires = UNIX_TIMESTAMP() + %d + 'UPDATE %T task + SET leaseOwner = %s, leaseExpires = UNIX_TIMESTAMP() + %d %Q %Q %Q', $task_table->getTableName(), $lease_ownership_name, @@ -90,11 +92,10 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { 'SELECT task.*, taskdata.data _taskData, UNIX_TIMESTAMP() _serverTime FROM %T task LEFT JOIN %T taskdata ON taskdata.id = task.dataID - WHERE leaseOwner = %s AND leaseExpires > UNIX_TIMESTAMP() - %Q %Q', + %Q %Q %Q', $task_table->getTableName(), $taskdata_table->getTableName(), - $lease_ownership_name, + $this->buildWhereClause($conn_w, self::PHASE_SELECT), $this->buildOrderClause($conn_w), $this->buildLimitClause($conn_w, $limit)); @@ -124,6 +125,13 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { case self::PHASE_EXPIRED: $where[] = 'leaseExpires < UNIX_TIMESTAMP()'; break; + case self::PHASE_SELECT: + $where[] = qsprintf( + $conn_w, + 'leaseOwner = %s', + $this->getLeaseOwnershipName()); + $where[] = 'leaseExpires > UNIX_TIMESTAMP()'; + break; default: throw new Exception("Unknown phase '{$phase}'!"); } @@ -131,7 +139,7 @@ final class PhabricatorWorkerLeaseQuery extends PhabricatorQuery { if ($this->ids) { $where[] = qsprintf( $conn_w, - 'id IN (%Ld)', + 'task.id IN (%Ld)', $this->ids); } diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 7824b41178..7bcd08a90b 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1020,6 +1020,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('daemontaskarchive.sql'), ), + 'drydocktaskid.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('drydocktaskid.sql'), + ), ); }