From b950f877c50cb6130c4b4aaf22cb5cfa1b1fb7c2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 25 Oct 2018 06:28:26 -0700 Subject: [PATCH 1/4] Allow Drydock Blueprints to control "supplemental allocation" behavior so all hosts in an Almanac pool get used Summary: Fixes T12145. Ref T13210. See PHI570. See PHI536. Currently, when you give Drydock an Almanac host pool with more than one host, it never voluntarily builds a second host resource: there is no way to say "maximum X working copies per host" (only "maximum X global working copies") to make the first host overflow, and the allocator tries to pack resources as tightly as possible. If you can force it to allocate the 2nd..Nth host, things will work reasonably well from there (it will spread working copies across the hosts randomly), but tricking it is very hard, especially before D19761. To deal with this, give blueprints a new behavior around "supplemental allocations". The idea here is that a blueprint may decide that it would prefer to allocate a fresh new resource instead of allowing an otherwise valid acquisition to occur. These supplemental allocations follow all the normal allocation rules (they can't exceed limits or actually replace existing resources), so they can only happen if there's free space in the resource pool. But a blueprint can elect for a supplemental allocation to provide a "grow the pool" hint. The only useful policies here are probably "true" (immediately use all resources, like Almanac) or "false" (pack resources as efficiently as possible) but some other policies //might// be useful (perhaps "start growing the pool when we're getting a bit full even if we aren't at the limit yet, since our workload is bursty"). Then, give Almanac host resources a "true" policy (always allocate supplemental resources) so they use all hosts once a similar number of concurrent jobs arrive. One aspect of this approach is that we only do supplemental resources if the normal allocation algorithm already decided that the best resource to acquire was part of the same blueprint. I started with an approach like "look at all the blueprints and see if any of them want to be greedy", but then a not-very-desirable blueprint would end up filling up its whole pool before we skipped the supplemental allocation part and ended up picking a different resource. That felt a bit silly and this feels a little cleaner and more focused. Test Plan: - Without changing the Almanac blueprint policy, allocated hosts. Got A, A, A, A, ... (second host never used). - Changed the Almanac policy. - Allocated hosts, got A, B, random mix of A and B. - Destroyed B. Destroyed all leases on A. Allocated. Got A. This tests the "don't build a supplemental resource if there are no leases on the natural resource". Reviewers: amckinley Reviewed By: amckinley Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13210, T12145 Differential Revision: https://secure.phabricator.com/D19762 --- ...anacServiceHostBlueprintImplementation.php | 10 +++ .../DrydockBlueprintImplementation.php | 32 +++++++++ .../drydock/storage/DrydockBlueprint.php | 9 +++ .../worker/DrydockLeaseUpdateWorker.php | 72 +++++++++++++++++++ 4 files changed, 123 insertions(+) diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php index 4724a2c807..12ccf8a343 100644 --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php @@ -45,6 +45,16 @@ final class DrydockAlmanacServiceHostBlueprintImplementation return true; } + public function shouldAllocateSupplementalResource( + DrydockBlueprint $blueprint, + DrydockResource $resource, + DrydockLease $lease) { + // We want to use every host in an Almanac service, since the amount of + // hardware is fixed and there's normally no value in packing leases onto a + // subset of it. Always build a new supplemental resource if we can. + return true; + } + public function canAllocateResourceForLease( DrydockBlueprint $blueprint, DrydockLease $lease) { diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php index 5acf63bf6b..88bc4d935a 100644 --- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php @@ -139,6 +139,38 @@ abstract class DrydockBlueprintImplementation extends Phobject { DrydockResource $resource, DrydockLease $lease); + /** + * Return true to try to allocate a new resource and expand the resource + * pool instead of permitting an otherwise valid acquisition on an existing + * resource. + * + * This allows the blueprint to provide a soft hint about when the resource + * pool should grow. + * + * Returning "true" in all cases generally makes sense when a blueprint + * controls a fixed pool of resources, like a particular number of physical + * hosts: you want to put all the hosts in service, so whenever it is + * possible to allocate a new host you want to do this. + * + * Returning "false" in all cases generally make sense when a blueprint + * has a flexible pool of expensive resources and you want to pack leases + * onto them as tightly as possible. + * + * @param DrydockBlueprint The blueprint for an existing resource being + * acquired. + * @param DrydockResource The resource being acquired, which we may want to + * build a supplemental resource for. + * @param DrydockLease The current lease performing acquisition. + * @return bool True to prefer allocating a supplemental resource. + * + * @task lease + */ + public function shouldAllocateSupplementalResource( + DrydockBlueprint $blueprint, + DrydockResource $resource, + DrydockLease $lease) { + return false; + } /* -( Resource Allocation )------------------------------------------------ */ diff --git a/src/applications/drydock/storage/DrydockBlueprint.php b/src/applications/drydock/storage/DrydockBlueprint.php index f7654a8b31..61e2dbfcfa 100644 --- a/src/applications/drydock/storage/DrydockBlueprint.php +++ b/src/applications/drydock/storage/DrydockBlueprint.php @@ -278,6 +278,15 @@ final class DrydockBlueprint extends DrydockDAO return $interface; } + public function shouldAllocateSupplementalResource( + DrydockResource $resource, + DrydockLease $lease) { + return $this->getImplementation()->shouldAllocateSupplementalResource( + $this, + $resource, + $lease); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php index 3ff9900239..b83022f720 100644 --- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php @@ -306,6 +306,7 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { $allocated = false; foreach ($resources as $resource) { try { + $resource = $this->newResourceForAcquisition($resource, $lease); $this->acquireLease($resource, $lease); $allocated = true; break; @@ -319,6 +320,10 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { // got around to acquiring it, we just got unlucky. We can yield and // try again later. $yields[] = $ex; + } catch (PhabricatorWorkerYieldException $ex) { + // We can be told to yield, particularly by the supplemental allocator + // trying to give us a supplemental resource. + $yields[] = $ex; } catch (Exception $ex) { $exceptions[] = $ex; } @@ -791,6 +796,73 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { } } + private function newResourceForAcquisition( + DrydockResource $resource, + DrydockLease $lease) { + + // If the resource has no leases against it, never build a new one. This is + // likely already a new resource that just activated. + $viewer = $this->getViewer(); + + $statuses = array( + DrydockLeaseStatus::STATUS_PENDING, + DrydockLeaseStatus::STATUS_ACQUIRED, + DrydockLeaseStatus::STATUS_ACTIVE, + ); + + $leases = id(new DrydockLeaseQuery()) + ->setViewer($viewer) + ->withResourcePHIDs(array($resource->getPHID())) + ->withStatuses($statuses) + ->setLimit(1) + ->execute(); + if (!$leases) { + return $resource; + } + + // If we're about to get a lease on a resource, check if the blueprint + // wants to allocate a supplemental resource. If it does, try to perform a + // new allocation instead. + $blueprint = $resource->getBlueprint(); + if (!$blueprint->shouldAllocateSupplementalResource($resource, $lease)) { + return $resource; + } + + // If the blueprint is already overallocated, we can't allocate a new + // resource. Just return the existing resource. + $remaining = $this->removeOverallocatedBlueprints( + array($blueprint), + $lease); + if (!$remaining) { + return $resource; + } + + // Try to build a new resource. + try { + $new_resource = $this->allocateResource($blueprint, $lease); + } catch (Exception $ex) { + $blueprint->logEvent( + DrydockResourceAllocationFailureLogType::LOGCONST, + array( + 'class' => get_class($ex), + 'message' => $ex->getMessage(), + )); + + return $resource; + } + + // If we can't actually acquire the new resource yet, just yield. + // (We could try to move forward with the original resource instead.) + $acquirable = $this->removeUnacquirableResources( + array($new_resource), + $lease); + if (!$acquirable) { + throw new PhabricatorWorkerYieldException(15); + } + + return $new_resource; + } + /* -( Activating Leases )-------------------------------------------------- */ From 5d4970d6b24ac98f50990cddd5151291c10855fa Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 31 Oct 2018 17:14:02 -0700 Subject: [PATCH 2/4] Fix a bug where "View as Query" could replace a saved query row by ID, causing workboard 404s Summary: Fixes T13208. See that task for details. The `clone $query` line is safe if `$query` is a builtin query (like "open"). However, if it's a saved query we clone not only the query parameters but the ID, too. Then when we `save()` the query later, we overwrite the original query. So this would happen in the database. First, you run a query and save it as the workboard default (query key "abc123"): | 123 | abc123 | {"...xxx..."} | Then we `clone` it and change the parameters, and `save()` it. But that causes an `UPDATE ... WHERE id = 123` and the table now looks like this: | 123 | def456 | {"...yyy..."} | What we want is to create a new query instead, with an `INSERT ...`: | 123 | abc123 | {"...xxx..."} | | 124 | def456 | {"...yyy..."} | Test Plan: - Followed reproduction steps from above. - With just the new `save()` guard, hit the guard error. - With the `newCopy()`, got a new copy of the query and "View as Query" remained functional without overwriting the original query row. - Ran migration, saw an affected board get fixed. Reviewers: amckinley, joshuaspence Reviewed By: joshuaspence Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13208 Differential Revision: https://secure.phabricator.com/D19768 --- .../20181031.board.01.queryreset.php | 50 +++++++++++++++++++ .../PhabricatorProjectBoardViewController.php | 2 +- .../PhabricatorApplicationSearchEngine.php | 8 +++ .../search/storage/PhabricatorSavedQuery.php | 7 +++ 4 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 resources/sql/autopatches/20181031.board.01.queryreset.php diff --git a/resources/sql/autopatches/20181031.board.01.queryreset.php b/resources/sql/autopatches/20181031.board.01.queryreset.php new file mode 100644 index 0000000000..781cf456ce --- /dev/null +++ b/resources/sql/autopatches/20181031.board.01.queryreset.php @@ -0,0 +1,50 @@ +establishConnection('w'); + +$iterator = new LiskMigrationIterator($table); +$search_engine = id(new ManiphestTaskSearchEngine()) + ->setViewer($viewer); + +foreach ($iterator as $project) { + $default_filter = $project->getDefaultWorkboardFilter(); + if (!strlen($default_filter)) { + continue; + } + + if ($search_engine->isBuiltinQuery($default_filter)) { + continue; + } + + $saved = id(new PhabricatorSavedQueryQuery()) + ->setViewer($viewer) + ->withQueryKeys(array($default_filter)) + ->executeOne(); + if ($saved) { + continue; + } + + $properties = $project->getProperties(); + unset($properties['workboard.filter.default']); + + queryfx( + $conn, + 'UPDATE %T SET properties = %s WHERE id = %d', + $table->getTableName(), + phutil_json_encode($properties), + $project->getID()); + + echo tsprintf( + "%s\n", + pht( + 'Project ("%s") had an invalid query saved as a default workboard '. + 'query. The query has been reset. See T13208.', + $project->getDisplayName())); +} diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index bb048f22ea..15e0c5d075 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -203,7 +203,7 @@ final class PhabricatorProjectBoardViewController // with the column filter. If the user currently has constraints on the // board, we want to add a new column or project constraint, not // completely replace the constraints. - $saved_query = clone $saved; + $saved_query = $saved->newCopy(); if ($query_column->getProxyPHID()) { $project_phids = $saved_query->getParameter('projectPHIDs'); diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index b808291a52..a89a017e85 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -103,6 +103,14 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { } public function saveQuery(PhabricatorSavedQuery $query) { + if ($query->getID()) { + throw new Exception( + pht( + 'Query (with ID "%s") has already been saved. Queries are '. + 'immutable once saved.', + $query->getID())); + } + $query->setEngineClassName(get_class($this)); $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); diff --git a/src/applications/search/storage/PhabricatorSavedQuery.php b/src/applications/search/storage/PhabricatorSavedQuery.php index 3f75027888..837f4fde42 100644 --- a/src/applications/search/storage/PhabricatorSavedQuery.php +++ b/src/applications/search/storage/PhabricatorSavedQuery.php @@ -63,6 +63,13 @@ final class PhabricatorSavedQuery extends PhabricatorSearchDAO return $this->assertAttachedKey($this->parameterMap, $key); } + public function newCopy() { + return id(new self()) + ->setParameters($this->getParameters()) + ->setQueryKey(null) + ->setEngineClassName($this->getEngineClassName()); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ From 9bea00c159b6a2bc98a29bcec5cce89b0e4a3442 Mon Sep 17 00:00:00 2001 From: Tim Hirsh Date: Fri, 2 Nov 2018 02:56:34 +0000 Subject: [PATCH 3/4] Add harbormaster.buildplan.search api method Summary: This revision adds a conduit search method for build plans. Other api methods (eg: `harbormaster.build.search`) support build plan phid's as a constraint, but they weren't exposed anywhere, so this provides a way to fetch them. Test Plan: Used the api console to run some searches. Output: ``` { "data": [ { "id": 1, "type": "HMCP", "phid": "PHID-HMCP-q2c25wvegzdkxs7gzor6", "fields": { "name": "my build plan", "planStatus": "active", "dateCreated": 1538085249, "dateModified": 1538085249, "policy": { "view": "users", "edit": "admin" } }, { "id": 1, "type": "HMCP", "phid": "PHID-HMCP-q2c25wvegzdkxs7gzor6", "fields": { "name": "my build plan", "status": { "value": "active" }, "dateCreated": 1538085249, "dateModified": 1538085249, "policy": { "view": "users", "edit": "admin" } }, "attachments": {} }, ... ], "maps": {}, "query": { "queryKey": null }, "cursor": { "limit": 100, "after": null, "before": null, "order": null } } ``` Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, yelirekim Differential Revision: https://secure.phabricator.com/D19769 --- src/__phutil_library_map__.php | 3 ++ .../HarbormasterBuildPlanSearchAPIMethod.php | 18 +++++++++++ .../configuration/HarbormasterBuildPlan.php | 31 +++++++++++++++++++ 3 files changed, 52 insertions(+) create mode 100644 src/applications/harbormaster/conduit/HarbormasterBuildPlanSearchAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a472b66411..7a9ea1df70 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1326,6 +1326,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanNameNgrams' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlanNameNgrams.php', 'HarbormasterBuildPlanPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPlanPHIDType.php', 'HarbormasterBuildPlanQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanQuery.php', + 'HarbormasterBuildPlanSearchAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildPlanSearchAPIMethod.php', 'HarbormasterBuildPlanSearchEngine' => 'applications/harbormaster/query/HarbormasterBuildPlanSearchEngine.php', 'HarbormasterBuildPlanTransaction' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlanTransaction.php', 'HarbormasterBuildPlanTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanTransactionQuery.php', @@ -6775,6 +6776,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyInterface', 'PhabricatorSubscribableInterface', 'PhabricatorNgramsInterface', + 'PhabricatorConduitResultInterface', 'PhabricatorProjectInterface', ), 'HarbormasterBuildPlanDatasource' => 'PhabricatorTypeaheadDatasource', @@ -6785,6 +6787,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanNameNgrams' => 'PhabricatorSearchNgrams', 'HarbormasterBuildPlanPHIDType' => 'PhabricatorPHIDType', 'HarbormasterBuildPlanQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildPlanSearchAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'HarbormasterBuildPlanSearchEngine' => 'PhabricatorApplicationSearchEngine', 'HarbormasterBuildPlanTransaction' => 'PhabricatorApplicationTransaction', 'HarbormasterBuildPlanTransactionQuery' => 'PhabricatorApplicationTransactionQuery', diff --git a/src/applications/harbormaster/conduit/HarbormasterBuildPlanSearchAPIMethod.php b/src/applications/harbormaster/conduit/HarbormasterBuildPlanSearchAPIMethod.php new file mode 100644 index 0000000000..b5e03b3071 --- /dev/null +++ b/src/applications/harbormaster/conduit/HarbormasterBuildPlanSearchAPIMethod.php @@ -0,0 +1,18 @@ +setKey('name') + ->setType('string') + ->setDescription(pht('The name of this build plan.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('status') + ->setType('map') + ->setDescription(pht('The current status of this build plan.')), + ); + } + + public function getFieldValuesForConduit() { + return array( + 'name' => $this->getName(), + 'status' => array( + 'value' => $this->getPlanStatus(), + ), + ); + } + + public function getConduitSearchAttachments() { + return array(); + } + } From 24a061f844958cc22d6f4874535b57574a6c13c3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 Nov 2018 19:55:03 -0700 Subject: [PATCH 4/4] Correct an ambiguous regexp in DiffusionRequest Summary: See . The intent of `[\d-,]` is "digits, hyphen, and comma" but `[x-y]` means "character range x-y". Specify `[\d,-]` instead to disambiguate the hyphen as "literal hyphen", not a character range marker. Test Plan: I can't reproduce the original error as reported, but browsed around Diffusion for a bit. Reviewers: amckinley, avivey Reviewed By: avivey Differential Revision: https://secure.phabricator.com/D19770 --- src/applications/diffusion/request/DiffusionRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php index 1a46d43adf..52a09d12ae 100644 --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -489,7 +489,7 @@ abstract class DiffusionRequest extends Phobject { // Consume the back part of the URI, up to the first "$". Use a negative // lookbehind to prevent matching '$$'. We double the '$' symbol when // encoding so that files with names like "money/$100" will survive. - $pattern = '@(?:(?:^|[^$])(?:[$][$])*)[$]([\d-,]+)$@'; + $pattern = '@(?:(?:^|[^$])(?:[$][$])*)[$]([\d,-]+)$@'; if (preg_match($pattern, $blob, $matches)) { $result['line'] = $matches[1]; $blob = substr($blob, 0, -(strlen($matches[1]) + 1));