From c059149eb98edb56cacc54ab3cc27b0b2acce8fd Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 26 Oct 2015 12:39:47 -0700 Subject: [PATCH] Remove Drydock host resource limits and give working copies simple limits Summary: Ref T9252. Right now, we have very strict limits on Drydock: one lease per host, and one working copy per working copy blueprint. These are silly and getting in the way of using "Land Revision" more widely, since we need at least one working copy for each landable repository. For now, just remove the host limit and put a simple limit on working copies. This might need to be fancier some day (e.g., limit working copies per-host) but it is generally reasonable for the use cases of today. Also add a `--background` flag to make testing a little easier. (Limits are also less important nowadays than they were in the past, because pools expand slowly now and we seem to have stamped out all the "runaway train" bugs where allocators go crazy and allocate a million things.) Test Plan: - With a limit of 5, ran 10 concurrent builds and saw them finish after allocating 5 total resources. - Removed limit, raised taskmaster concurrency to 128, ran thousands of builds in blocks of 128 or 256. - Saw Drydock gradually expand the pool, allocating a few more working copies at first and a lot of working copies later. - Got ~256 builds in ~140 seconds, which isn't a breakneck pace or anything but isn't too bad. - This stuff seems to be mostly bottlenecked on `sbuild` throttling inbound SSH connections. I haven't tweaked it. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9252 Differential Revision: https://secure.phabricator.com/D14334 --- ...anacServiceHostBlueprintImplementation.php | 15 +-- .../DrydockBlueprintImplementation.php | 109 +++++++++++++++++- ...dockWorkingCopyBlueprintImplementation.php | 32 ++--- .../HarbormasterManagementBuildWorkflow.php | 11 +- 4 files changed, 135 insertions(+), 32 deletions(-) diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php index e62c7b7558..19e476c4ba 100644 --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php @@ -109,11 +109,6 @@ final class DrydockAlmanacServiceHostBlueprintImplementation DrydockBlueprint $blueprint, DrydockResource $resource, DrydockLease $lease) { - - if (!DrydockSlotLock::isLockFree($this->getLeaseSlotLock($resource))) { - return false; - } - return true; } @@ -124,7 +119,6 @@ final class DrydockAlmanacServiceHostBlueprintImplementation $lease ->setActivateWhenAcquired(true) - ->needSlotLock($this->getLeaseSlotLock($resource)) ->acquireOnResource($resource); } @@ -146,11 +140,6 @@ final class DrydockAlmanacServiceHostBlueprintImplementation return; } - private function getLeaseSlotLock(DrydockResource $resource) { - $resource_phid = $resource->getPHID(); - return "almanac.host.lease({$resource_phid})"; - } - public function getType() { return 'host'; } @@ -188,7 +177,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation } } - public function getFieldSpecifications() { + protected function getCustomFieldSpecifications() { return array( 'almanacServicePHIDs' => array( 'name' => pht('Almanac Services'), @@ -207,7 +196,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation 'credential.type' => PassphraseSSHPrivateKeyTextCredentialType::CREDENTIAL_TYPE, ), - ) + parent::getFieldSpecifications(); + ); } private function loadServices(DrydockBlueprint $blueprint) { diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php index 0c53b19fcf..01c2067280 100644 --- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php @@ -16,6 +16,26 @@ abstract class DrydockBlueprintImplementation extends Phobject { abstract public function getDescription(); public function getFieldSpecifications() { + $fields = array(); + + $fields += $this->getCustomFieldSpecifications(); + + if ($this->shouldUseConcurrentResourceLimit()) { + $fields += array( + 'allocator.limit' => array( + 'name' => pht('Limit'), + 'caption' => pht( + 'Maximum number of resources this blueprint can have active '. + 'concurrently.'), + 'type' => 'int', + ), + ); + } + + return $fields; + } + + protected function getCustomFieldSpecifications() { return array(); } @@ -316,6 +336,85 @@ abstract class DrydockBlueprintImplementation extends Phobject { } + /** + * Does this implementation use concurrent resource limits? + * + * Implementations can override this method to opt into standard limit + * behavior, which provides a simple concurrent resource limit. + * + * @return bool True to use limits. + */ + protected function shouldUseConcurrentResourceLimit() { + return false; + } + + + /** + * Get the effective concurrent resource limit for this blueprint. + * + * @param DrydockBlueprint Blueprint to get the limit for. + * @return int|null Limit, or `null` for no limit. + */ + protected function getConcurrentResourceLimit(DrydockBlueprint $blueprint) { + if ($this->shouldUseConcurrentResourceLimit()) { + $limit = $blueprint->getFieldValue('allocator.limit'); + $limit = (int)$limit; + if ($limit > 0) { + return $limit; + } else { + return null; + } + } + + return null; + } + + + protected function getConcurrentResourceLimitSlotLock( + DrydockBlueprint $blueprint) { + + $limit = $this->getConcurrentResourceLimit($blueprint); + if ($limit === null) { + return; + } + + $blueprint_phid = $blueprint->getPHID(); + + // TODO: This logic shouldn't do anything awful, but is a little silly. It + // would be nice to unify the "huge limit" and "small limit" cases + // eventually but it's a little tricky. + + // If the limit is huge, just pick a random slot. This is just stopping + // us from exploding if someone types a billion zillion into the box. + if ($limit > 1024) { + $slot = mt_rand(0, $limit - 1); + return "allocator({$blueprint_phid}).limit({$slot})"; + } + + // For reasonable limits, actually check for an available slot. + $locks = DrydockSlotLock::loadLocks($blueprint_phid); + $locks = mpull($locks, null, 'getLockKey'); + + $slots = range(0, $limit - 1); + shuffle($slots); + + foreach ($slots as $slot) { + $slot_lock = "allocator({$blueprint_phid}).limit({$slot})"; + if (empty($locks[$slot_lock])) { + return $slot_lock; + } + } + + // If we found no free slot, just return whatever we checked last (which + // is just a random slot). There's a small chance we'll get lucky and the + // lock will be free by the time we try to take it, but usually we'll just + // fail to grab the lock, throw an appropriate lock exception, and get back + // on the right path to retry later. + return $slot_lock; + } + + + /** * Apply standard limits on resource allocation rate. * @@ -329,7 +428,7 @@ abstract class DrydockBlueprintImplementation extends Phobject { // configurable by the blueprint implementation. // Limit on total number of active resources. - $total_limit = 1; + $total_limit = $this->getConcurrentResourceLimit($blueprint); // Always allow at least this many allocations to be in flight at once. $min_allowed = 1; @@ -358,9 +457,11 @@ abstract class DrydockBlueprintImplementation extends Phobject { // If we're at the limit on total active resources, limit additional // allocations. - $n_total = ($n_alloc + $n_active + $n_broken + $n_released); - if ($n_total >= $total_limit) { - return true; + if ($total_limit !== null) { + $n_total = ($n_alloc + $n_active + $n_broken + $n_released); + if ($n_total >= $total_limit) { + return true; + } } // If the number of in-flight allocations is fewer than the minimum number diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index 1d7776af14..f4b33adb8b 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -125,15 +125,7 @@ final class DrydockWorkingCopyBlueprintImplementation ->setOwnerPHID($resource_phid) ->setAttribute('workingcopy.resourcePHID', $resource_phid) ->setAllowedBlueprintPHIDs($blueprint_phids); - - $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? + $resource->setAttribute('host.leasePHID', $host_lease->getPHID()); $map = $lease->getAttribute('repositories.map'); foreach ($map as $key => $value) { @@ -143,10 +135,18 @@ final class DrydockWorkingCopyBlueprintImplementation 'phid', )); } + $resource->setAttribute('repositories.map', $map); + + $slot_lock = $this->getConcurrentResourceLimitSlotLock($blueprint); + if ($slot_lock !== null) { + $resource->needSlotLock($slot_lock); + } - return $resource - ->setAttribute('repositories.map', $map) - ->allocateResource(); + $resource->allocateResource(); + + $host_lease->queueForActivation(); + + return $resource; } public function activateResource( @@ -393,14 +393,18 @@ final class DrydockWorkingCopyBlueprintImplementation return $lease; } - public function getFieldSpecifications() { + protected function getCustomFieldSpecifications() { return array( 'blueprintPHIDs' => array( 'name' => pht('Use Blueprints'), 'type' => 'blueprints', 'required' => true, ), - ) + parent::getFieldSpecifications(); + ); + } + + protected function shouldUseConcurrentResourceLimit() { + return true; } diff --git a/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php index bc6a52018c..7110d98d6e 100644 --- a/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php +++ b/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php @@ -15,6 +15,12 @@ final class HarbormasterManagementBuildWorkflow 'param' => 'id', 'help' => pht('ID of build plan to run.'), ), + array( + 'name' => 'background', + 'help' => pht( + 'Submit builds into the build queue normally instead of '. + 'running them in the foreground.'), + ), array( 'name' => 'buildable', 'wildcard' => true, @@ -88,7 +94,10 @@ final class HarbormasterManagementBuildWorkflow "\n %s\n\n", PhabricatorEnv::getProductionURI('/B'.$buildable->getID())); - PhabricatorWorker::setRunAllTasksInProcess(true); + if (!$args->getArg('background')) { + PhabricatorWorker::setRunAllTasksInProcess(true); + } + $buildable->applyPlan($plan, array()); $console->writeOut("%s\n", pht('Done.')); -- 2.51.2