From 6deb09efcdb4d5a743cfba06ec7e4191e1b16714 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Oct 2018 06:05:36 -0700 Subject: [PATCH 01/10] When leasing a Working Copy in Drydock, just "git reset --hard" so empty repositories work Summary: Ref T13210. We currently "git reset --hard HEAD" during working copy leasing, mostly by convention/familiarity. However, this command does not work in an empty repository, because there is no HEAD yet. The command "git reset --hard" appears to have the same meaning and effect in all cases, except that it also works correctly in an empty repository. The manual suggests that omitting HEAD should be the same as specifying HEAD, too: > The / defaults to HEAD in all forms. Test Plan: Successfully leased a working copy for an empty repository using Drydock. Reviewers: amckinley Reviewed By: amckinley Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19750 --- .../blueprint/DrydockWorkingCopyBlueprintImplementation.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index f45c4f2adb..c1672f99fb 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -256,8 +256,9 @@ final class DrydockWorkingCopyBlueprintImplementation $ref = idx($spec, 'ref'); // Reset things first, in case previous builds left anything staged or - // dirty. - $cmd[] = 'git reset --hard HEAD'; + // dirty. Note that we don't reset to "HEAD" because that does not work + // in empty repositories. + $cmd[] = 'git reset --hard'; if ($commit !== null) { $cmd[] = 'git checkout %s --'; From 1f6869a7654a8215ddf9afc8567fa6b44cae364b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Oct 2018 17:39:47 -0700 Subject: [PATCH 02/10] In "bin/drydock lease", take a JSON "--attributes" so we can accept complex values Summary: Depends on D19750. See T13210. The `bin/drydock lease` command makes it easier to request ad-hoc leases, but currently takes lease attributes in the form `--attributes x=y,a=b`. This was okay for all leases at the time, but doesn't really work for modern WorkingCopy resources since they take a `repositories.map` which has a dictionary as a value. You can't specify that with `repositories.map=...`. Instead, point `--attributes` at a JSON file or use `--attributes -` to read from stdin. Test Plan: Used `--attributes` with a file and stdin to allocate working copy leases with repositories. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D19751 --- .../DrydockManagementLeaseWorkflow.php | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php index 75636dab09..7371232620 100644 --- a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php +++ b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php @@ -20,9 +20,11 @@ final class DrydockManagementLeaseWorkflow 'help' => pht('Set lease expiration time.'), ), array( - 'name' => 'attributes', - 'param' => 'name=value,...', - 'help' => pht('Resource specification.'), + 'name' => 'attributes', + 'param' => 'file', + 'help' => pht( + 'JSON file with lease attributes. Use "-" to read attributes '. + 'from stdin.'), ), )); } @@ -49,11 +51,20 @@ final class DrydockManagementLeaseWorkflow } } - $attributes = $args->getArg('attributes'); - if ($attributes) { - $options = new PhutilSimpleOptions(); - $options->setCaseSensitive(true); - $attributes = $options->parse($attributes); + $attributes_file = $args->getArg('attributes'); + if (strlen($attributes_file)) { + if ($attributes_file == '-') { + echo tsprintf( + "%s\n", + 'Reading JSON attributes from stdin...'); + $data = file_get_contents('php://stdin'); + } else { + $data = Filesystem::readFile($attributes_file); + } + + $attributes = phutil_json_decode($data); + } else { + $attributes = array(); } $lease = id(new DrydockLease()) From e9309fdd6ada13e3e1dcfaac7dedb8c9e387239a Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Oct 2018 07:32:19 -0700 Subject: [PATCH 03/10] When a Drydock lease schedules a resource to be reclaimed, awaken the lease update task when the reclaim completes Summary: Depends on D19751. Ref T13210. When Drydock needs to reclaim an existing unused resource in order to build a new resource to satisfy a lease, the lease which triggered the reclaim currently gets thrown back into the pool with a 15-second yield. If the queue is pretty empty and the reclaim is quick, this can mean that we spend up to 15 extra seconds just waiting for the lease update task to get another shot at building a resource (the resource reclaim may complete in a second or two, but nothing else happens until the yield ends). Instead, when a lease triggers a reclaim, have the reclaim reawaken the lease task when it completes. In the best case, this saves us 15 seconds of waiting. In other cases (the task already completed some other way, the resource gets claimed before the lease gets to it), it's harmless. Test Plan: - Allocated A, A, A working copies with limit 3. Leased a B working copy. - Before patch: allocation took ~32 seconds. - After patch: allocation takes ~17 seconds (i.e., about 15 seconds less). Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19752 --- .../autopatches/20181024.drydock.01.commandprops.sql | 2 ++ .../20181024.drydock.02.commanddefaults.sql | 2 ++ src/applications/drydock/storage/DrydockCommand.php | 12 ++++++++++++ .../drydock/worker/DrydockResourceUpdateWorker.php | 7 +++++++ src/applications/drydock/worker/DrydockWorker.php | 5 +++++ 5 files changed, 28 insertions(+) create mode 100644 resources/sql/autopatches/20181024.drydock.01.commandprops.sql create mode 100644 resources/sql/autopatches/20181024.drydock.02.commanddefaults.sql diff --git a/resources/sql/autopatches/20181024.drydock.01.commandprops.sql b/resources/sql/autopatches/20181024.drydock.01.commandprops.sql new file mode 100644 index 0000000000..e808146b02 --- /dev/null +++ b/resources/sql/autopatches/20181024.drydock.01.commandprops.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_drydock.drydock_command + ADD properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20181024.drydock.02.commanddefaults.sql b/resources/sql/autopatches/20181024.drydock.02.commanddefaults.sql new file mode 100644 index 0000000000..2c336dc40e --- /dev/null +++ b/resources/sql/autopatches/20181024.drydock.02.commanddefaults.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_drydock.drydock_command + SET properties = '{}' WHERE properties = ''; diff --git a/src/applications/drydock/storage/DrydockCommand.php b/src/applications/drydock/storage/DrydockCommand.php index e7d003bdd6..0f7f253217 100644 --- a/src/applications/drydock/storage/DrydockCommand.php +++ b/src/applications/drydock/storage/DrydockCommand.php @@ -11,6 +11,7 @@ final class DrydockCommand protected $targetPHID; protected $command; protected $isConsumed; + protected $properties = array(); private $commandTarget = self::ATTACHABLE; @@ -22,6 +23,9 @@ final class DrydockCommand protected function getConfiguration() { return array( + self::CONFIG_SERIALIZATION => array( + 'properties' => self::SERIALIZATION_JSON, + ), self::CONFIG_COLUMN_SCHEMA => array( 'command' => 'text32', 'isConsumed' => 'bool', @@ -43,6 +47,14 @@ final class DrydockCommand return $this->assertAttached($this->commandTarget); } + public function setProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + + public function getProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php index baa80f90d8..817fbaddee 100644 --- a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php @@ -150,6 +150,13 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { $this->releaseResource($resource, $reclaimer_phid); break; } + + // If the command specifies that other worker tasks should be awakened + // after it executes, awaken them now. + $awaken_ids = $command->getProperty('awakenTaskIDs'); + if (is_array($awaken_ids) && $awaken_ids) { + PhabricatorWorker::awakenTaskIDs($awaken_ids); + } } diff --git a/src/applications/drydock/worker/DrydockWorker.php b/src/applications/drydock/worker/DrydockWorker.php index 443780680d..8a81da09b4 100644 --- a/src/applications/drydock/worker/DrydockWorker.php +++ b/src/applications/drydock/worker/DrydockWorker.php @@ -241,10 +241,15 @@ abstract class DrydockWorker extends PhabricatorWorker { DrydockLease $lease) { $viewer = $this->getViewer(); + // When the resource releases, we we want to reawaken this task since it + // should be able to start building a new resource right away. + $worker_task_id = $this->getCurrentWorkerTaskID(); + $command = DrydockCommand::initializeNewCommand($viewer) ->setTargetPHID($resource->getPHID()) ->setAuthorPHID($lease->getPHID()) ->setCommand(DrydockCommand::COMMAND_RECLAIM) + ->setProperty('awakenTaskIDs', array($worker_task_id)) ->save(); $resource->scheduleUpdate(); From 78ab675bd8aa7c2a497651bc7681f5286c1cc8b3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Oct 2018 08:07:37 -0700 Subject: [PATCH 04/10] After a Drydock lease triggers a resource to be reclaimed, stop it from triggering another reclaim until the first one completes Summary: Depends on D19752. Ref T13210. If resources take a long time to reclaim/destroy (normally, more than 15 seconds) a single new lease may update several times during the reclaim/destroy process and end up reclaiming multiple resources. Instead: after a lease triggers a reclaim, prevent it from triggering another reclaim as long as the resource it is reclaiming hasn't finished its reclaim/destroy cycle. Basically, each lease only gets to destroy one resource at a time. Test Plan: - Added a `sleep(120)` to `destroyResource()` to simulate a long reclaim/destroy cycle. - Allocated A, A, A working copies. Leased a B working copy. - Before patch: saw "B" lease destroy all three "A" working copies after ~0, ~15, and ~30 seconds, then build a new "B" resource after ~120 seconds (when the first reclaim/destroy finished). - After patch: saw "B" lease destroy one "A" working copy after ~0 seconds, then wait patiently until it finished up, then build a new "B" resource. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19753 --- .../worker/DrydockLeaseUpdateWorker.php | 20 +++++++++++++++++++ .../drydock/worker/DrydockWorker.php | 15 +++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php index 4a0cba8de2..5506e3cb9f 100644 --- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php @@ -666,6 +666,26 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { DrydockLease $lease) { $viewer = $this->getViewer(); + // If this lease is marked as already in the process of reclaiming a + // resource, don't let it reclaim another one until the first reclaim + // completes. This stops one lease from reclaiming a large number of + // resources if the reclaims take a while to complete. + $reclaiming_phid = $lease->getAttribute('drydock.reclaimingPHID'); + if ($reclaiming_phid) { + $reclaiming_resource = id(new DrydockResourceQuery()) + ->setViewer($viewer) + ->withPHIDs(array($reclaiming_phid)) + ->withStatuses( + array( + DrydockResourceStatus::STATUS_ACTIVE, + DrydockResourceStatus::STATUS_RELEASED, + )) + ->executeOne(); + if ($reclaiming_resource) { + return null; + } + } + $resources = id(new DrydockResourceQuery()) ->setViewer($viewer) ->withBlueprintPHIDs(array($blueprint->getPHID())) diff --git a/src/applications/drydock/worker/DrydockWorker.php b/src/applications/drydock/worker/DrydockWorker.php index 8a81da09b4..fcb6876cfe 100644 --- a/src/applications/drydock/worker/DrydockWorker.php +++ b/src/applications/drydock/worker/DrydockWorker.php @@ -241,16 +241,25 @@ abstract class DrydockWorker extends PhabricatorWorker { DrydockLease $lease) { $viewer = $this->getViewer(); + // Mark the lease as reclaiming this resource. It won't be allowed to start + // another reclaim as long as this resource is still in the process of + // being reclaimed. + $lease->setAttribute('drydock.reclaimingPHID', $resource->getPHID()); + // When the resource releases, we we want to reawaken this task since it - // should be able to start building a new resource right away. + // should (usually) be able to start building a new resource right away. $worker_task_id = $this->getCurrentWorkerTaskID(); $command = DrydockCommand::initializeNewCommand($viewer) ->setTargetPHID($resource->getPHID()) ->setAuthorPHID($lease->getPHID()) ->setCommand(DrydockCommand::COMMAND_RECLAIM) - ->setProperty('awakenTaskIDs', array($worker_task_id)) - ->save(); + ->setProperty('awakenTaskIDs', array($worker_task_id)); + + $lease->openTransaction(); + $lease->save(); + $command->save(); + $lease->saveTransaction(); $resource->scheduleUpdate(); From f6122547d77444a027119c3c2b7f2602a5acde53 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Oct 2018 09:26:24 -0700 Subject: [PATCH 05/10] When a lease triggers a resource allocation for a resource which must activate, awaken the lease task after the resource activates Summary: Depends on D19753. Ref T13210. This is a small optimization that saves us from waiting up to 15 seconds for a yield. When there are no Working Copy resources and a new lease comes in, we'll allocate one and yield until it activates. If activating it (SSH'ing and running `git clone`) takes less than 15 seconds, the resource will activate (say, at T+4) but the lease won't update again for a while (say, until T+15). This leaves us with a pointless wait (in this example, we're sitting around for 9 seconds when we could move forward). To improve this a little bit, let resources wake up the lease update tasks that triggered allocation after they activate. In the best case, that task runs ~15 seconds sooner. In the worst case, the awaken is just a no-op. With a more-full queue, this has a smaller effect (it's likely something else will run and be able to use the resource in those 9 seconds). With already-activated resources, this has no effect (when resources are already activated, we can lease immediately). Test Plan: - Cleaned up all working copy resources. - Requested a new "A" working copy. - Before patch: got a working copy after 17-18 seconds, most of which was spent yielded. - After patch: got a working copy after 3-4 seconds. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19754 --- .../drydock/worker/DrydockLeaseUpdateWorker.php | 7 +++++++ .../drydock/worker/DrydockResourceUpdateWorker.php | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php index 5506e3cb9f..3ff9900239 100644 --- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php @@ -599,6 +599,13 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { 'DrydockResourceUpdateWorker', array( 'resourcePHID' => $resource->getPHID(), + + // This task will generally yield while the resource activates, so + // wake it back up once the resource comes online. Most of the time, + // we'll be able to lease the newly activated resource. + 'awakenOnActivation' => array( + $this->getCurrentWorkerTaskID(), + ), ), array( 'objectPHID' => $resource->getPHID(), diff --git a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php index 817fbaddee..14ef8e4936 100644 --- a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php @@ -170,6 +170,11 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { $blueprint = $resource->getBlueprint(); $blueprint->activateResource($resource); $this->validateActivatedResource($blueprint, $resource); + + $awaken_ids = $this->getTaskDataValue('awakenOnActivation'); + if (is_array($awaken_ids) && $awaken_ids) { + PhabricatorWorker::awakenTaskIDs($awaken_ids); + } } From 5f3a7cb41b1743fad421351f0047035b1adfbb94 Mon Sep 17 00:00:00 2001 From: Mike Riley Date: Thu, 25 Oct 2018 04:39:32 -0700 Subject: [PATCH 06/10] Expose Drydock leases via Conduit Summary: See T13212 for some context and discussion on this being revived. See T11694 for original context. Add a query constraint for lease owners and implement the conduit search method for Drydock leases. Ref T11694. Fixes T13212. Test Plan: - Called the API method from conduit and browsed lease queries from the UI. - Used the new "ownerPHIDs" constraint via API console. {F5963044} Reviewers: yelirekim, amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam, epriestley Maniphest Tasks: T11694, T13212 Differential Revision: https://secure.phabricator.com/D16594 --- src/__phutil_library_map__.php | 3 + .../DrydockLeaseSearchConduitAPIMethod.php | 18 +++++ .../drydock/query/DrydockLeaseQuery.php | 13 ++++ .../query/DrydockLeaseSearchEngine.php | 9 +++ .../drydock/storage/DrydockLease.php | 69 ++++++++++++++++++- 5 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 src/applications/drydock/conduit/DrydockLeaseSearchConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8bcf3d6120..a472b66411 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1146,6 +1146,7 @@ phutil_register_library_map(array( 'DrydockLeaseReclaimLogType' => 'applications/drydock/logtype/DrydockLeaseReclaimLogType.php', 'DrydockLeaseReleaseController' => 'applications/drydock/controller/DrydockLeaseReleaseController.php', 'DrydockLeaseReleasedLogType' => 'applications/drydock/logtype/DrydockLeaseReleasedLogType.php', + 'DrydockLeaseSearchConduitAPIMethod' => 'applications/drydock/conduit/DrydockLeaseSearchConduitAPIMethod.php', 'DrydockLeaseSearchEngine' => 'applications/drydock/query/DrydockLeaseSearchEngine.php', 'DrydockLeaseStatus' => 'applications/drydock/constants/DrydockLeaseStatus.php', 'DrydockLeaseUpdateWorker' => 'applications/drydock/worker/DrydockLeaseUpdateWorker.php', @@ -6532,6 +6533,7 @@ phutil_register_library_map(array( 'DrydockLease' => array( 'DrydockDAO', 'PhabricatorPolicyInterface', + 'PhabricatorConduitResultInterface', ), 'DrydockLeaseAcquiredLogType' => 'DrydockLogType', 'DrydockLeaseActivatedLogType' => 'DrydockLogType', @@ -6552,6 +6554,7 @@ phutil_register_library_map(array( 'DrydockLeaseReclaimLogType' => 'DrydockLogType', 'DrydockLeaseReleaseController' => 'DrydockLeaseController', 'DrydockLeaseReleasedLogType' => 'DrydockLogType', + 'DrydockLeaseSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'DrydockLeaseSearchEngine' => 'PhabricatorApplicationSearchEngine', 'DrydockLeaseStatus' => 'PhabricatorObjectStatus', 'DrydockLeaseUpdateWorker' => 'DrydockWorker', diff --git a/src/applications/drydock/conduit/DrydockLeaseSearchConduitAPIMethod.php b/src/applications/drydock/conduit/DrydockLeaseSearchConduitAPIMethod.php new file mode 100644 index 0000000000..6d75ed1718 --- /dev/null +++ b/src/applications/drydock/conduit/DrydockLeaseSearchConduitAPIMethod.php @@ -0,0 +1,18 @@ +ownerPHIDs = $phids; + return $this; + } + public function withStatuses(array $statuses) { $this->statuses = $statuses; return $this; @@ -105,6 +111,13 @@ final class DrydockLeaseQuery extends DrydockQuery { $this->resourcePHIDs); } + if ($this->ownerPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'ownerPHID IN (%Ls)', + $this->ownerPHIDs); + } + if ($this->ids !== null) { $where[] = qsprintf( $conn, diff --git a/src/applications/drydock/query/DrydockLeaseSearchEngine.php b/src/applications/drydock/query/DrydockLeaseSearchEngine.php index 3b551023b2..39c91e66bf 100644 --- a/src/applications/drydock/query/DrydockLeaseSearchEngine.php +++ b/src/applications/drydock/query/DrydockLeaseSearchEngine.php @@ -40,6 +40,10 @@ final class DrydockLeaseSearchEngine $query->withStatuses($map['statuses']); } + if ($map['ownerPHIDs']) { + $query->withOwnerPHIDs($map['ownerPHIDs']); + } + return $query; } @@ -49,6 +53,11 @@ final class DrydockLeaseSearchEngine ->setLabel(pht('Statuses')) ->setKey('statuses') ->setOptions(DrydockLeaseStatus::getStatusMap()), + id(new PhabricatorPHIDsSearchField()) + ->setLabel(pht('Owners')) + ->setKey('ownerPHIDs') + ->setAliases(array('owner', 'owners', 'ownerPHID')) + ->setDescription(pht('Search leases by owner.')), ); } diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php index 866bb21b37..2b77146495 100644 --- a/src/applications/drydock/storage/DrydockLease.php +++ b/src/applications/drydock/storage/DrydockLease.php @@ -1,7 +1,9 @@ array( 'columns' => array('status'), ), + 'key_owner' => array( + 'columns' => array('ownerPHID'), + ), ), ) + parent::getConfiguration(); } @@ -535,4 +540,66 @@ final class DrydockLease extends DrydockDAO return pht('Leases inherit policies from the resources they lease.'); } + +/* -( PhabricatorConduitResultInterface )---------------------------------- */ + + + public function getFieldSpecificationsForConduit() { + return array( + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('resourcePHID') + ->setType('phid?') + ->setDescription(pht('PHID of the leased resource, if any.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('resourceType') + ->setType('string') + ->setDescription(pht('Type of resource being leased.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('until') + ->setType('int?') + ->setDescription(pht('Epoch at which this lease expires, if any.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('ownerPHID') + ->setType('phid?') + ->setDescription(pht('The PHID of the object that owns this lease.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('authorizingPHID') + ->setType('phid') + ->setDescription(pht( + 'The PHID of the object that authorized this lease.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('status') + ->setType('map') + ->setDescription(pht( + "The string constant and name of this lease's status.")), + ); + } + + public function getFieldValuesForConduit() { + $status = $this->getStatus(); + + $until = $this->getUntil(); + if ($until) { + $until = (int)$until; + } else { + $until = null; + } + + return array( + 'resourcePHID' => $this->getResourcePHID(), + 'resourceType' => $this->getResourceType(), + 'until' => $until, + 'ownerPHID' => $this->getOwnerPHID(), + 'authorizingPHID' => $this->getAuthorizingPHID(), + 'status' => array( + 'value' => $status, + 'name' => DrydockLeaseStatus::getNameForStatus($status), + ), + ); + } + + public function getConduitSearchAttachments() { + return array(); + } + } From a7c008708d04cb0e3d010ec7919db0804d3af77c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Oct 2018 09:38:29 -0700 Subject: [PATCH 07/10] Correct a mangled translation string in "bin/phd log --id X" Summary: Ref T13210. See PHI930. This translation is wrong: the parameter is a comma-separated list as a string, but the USEnglish translation provides alternatives. We can't select among alternatives based on a random string (it isn't a plurality value to let us select "chair" vs "chairs", and isn't a gender value to let us select "his profile" vs "her profile") so we get an error. But the string itself is also misleading, since "bin/phd log --id A --id B --id C" will say "none of these are valid" if //any// of them are invalid. Instead, just tell the user explicitly about the first problem. Test Plan: - Ran `bin/phd log --id` with good (got logs) and bad IDs (got sensible error). - Ran `bin/phd log` with any logs (got logs) and (simluated) without any logs (got error). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19755 --- ...PhabricatorDaemonManagementLogWorkflow.php | 19 ++++++++++++------- .../PhabricatorUSEnglishTranslation.php | 4 ---- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementLogWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementLogWorkflow.php index 394b4735a5..8fca624b44 100644 --- a/src/applications/daemon/management/PhabricatorDaemonManagementLogWorkflow.php +++ b/src/applications/daemon/management/PhabricatorDaemonManagementLogWorkflow.php @@ -40,15 +40,20 @@ final class PhabricatorDaemonManagementLogWorkflow $query->withIDs($ids); } $daemons = $query->execute(); + $daemons = mpull($daemons, null, 'getID'); - if (!$daemons) { - if ($ids) { - throw new PhutilArgumentUsageException( - pht('No daemon(s) with id(s) "%s" exist!', implode(', ', $ids))); - } else { - throw new PhutilArgumentUsageException( - pht('No daemons are running.')); + if ($ids) { + foreach ($ids as $id) { + if (!isset($daemons[$id])) { + throw new PhutilArgumentUsageException( + pht( + 'No log record exists for a daemon with ID "%s".', + $id)); + } } + } else if (!$daemons) { + throw new PhutilArgumentUsageException( + pht('No log records exist for any daemons.')); } $console = PhutilConsole::getConsole(); diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index c2241995e7..5169c08ffd 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php @@ -9,10 +9,6 @@ final class PhabricatorUSEnglishTranslation protected function getTranslations() { return array( - 'No daemon(s) with id(s) "%s" exist!' => array( - 'No daemon with id %s exists!', - 'No daemons with ids %s exist!', - ), 'These %d configuration value(s) are related:' => array( 'This configuration value is related:', 'These configuration values are related:', From 61ec434208086c5027e9b06477632ee11826ab88 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 25 Oct 2018 04:51:37 -0700 Subject: [PATCH 08/10] Remove unicode marks for "Accept/Raise Concern" in Audit Summary: Ref T13210. The comment action dropdown for audits has a heavy checkmark next to "Accept" and a heavy "X" next to "Raise Concern". We previously removed similar marks in Differential in D19405 and that seems to have gone fine. For consistency, remove these too. Test Plan: Viewed the comment action dropdown, no longer saw checkmark and X-mark. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19759 --- .../diffusion/xaction/DiffusionCommitAcceptTransaction.php | 2 +- .../diffusion/xaction/DiffusionCommitConcernTransaction.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php index 2e0dab2c35..f7b919be8e 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php @@ -7,7 +7,7 @@ final class DiffusionCommitAcceptTransaction const ACTIONKEY = 'accept'; protected function getCommitActionLabel() { - return pht("Accept Commit \xE2\x9C\x94"); + return pht('Accept Commit'); } protected function getCommitActionDescription() { diff --git a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php index 1f6ae72621..94590def9e 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php @@ -7,7 +7,7 @@ final class DiffusionCommitConcernTransaction const ACTIONKEY = 'concern'; protected function getCommitActionLabel() { - return pht("Raise Concern \xE2\x9C\x98"); + return pht('Raise Concern'); } protected function getCommitActionDescription() { From 65e953658a32c09c55f0df531e89327ad82fd217 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 25 Oct 2018 04:58:06 -0700 Subject: [PATCH 09/10] Expose Audit actions for "transaction.search" in a basic way Summary: Ref T13210. See PHI841. This mirrors D19509 for Differential. Test Plan: Called `transaction.search` on a commit with a bunch of audit activity, got appropriate labels in the results. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19760 --- .../xaction/DiffusionCommitAcceptTransaction.php | 8 ++++++++ .../xaction/DiffusionCommitConcernTransaction.php | 8 ++++++++ .../xaction/DiffusionCommitResignTransaction.php | 8 ++++++++ .../xaction/DiffusionCommitVerifyTransaction.php | 8 ++++++++ 4 files changed, 32 insertions(+) diff --git a/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php index f7b919be8e..5ade7f3513 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php @@ -70,4 +70,12 @@ final class DiffusionCommitAcceptTransaction $this->renderObject()); } + public function getTransactionTypeForConduit($xaction) { + return 'accept'; + } + + public function getFieldValuesForConduit($object, $data) { + return array(); + } + } diff --git a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php index 94590def9e..ffd084412e 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php @@ -76,4 +76,12 @@ final class DiffusionCommitConcernTransaction $this->renderObject()); } + public function getTransactionTypeForConduit($xaction) { + return 'concern'; + } + + public function getFieldValuesForConduit($object, $data) { + return array(); + } + } diff --git a/src/applications/diffusion/xaction/DiffusionCommitResignTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitResignTransaction.php index 103d0fabfe..8adc8346dc 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitResignTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitResignTransaction.php @@ -63,4 +63,12 @@ final class DiffusionCommitResignTransaction $this->renderObject()); } + public function getTransactionTypeForConduit($xaction) { + return 'resign'; + } + + public function getFieldValuesForConduit($object, $data) { + return array(); + } + } diff --git a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php index f1ec5834b1..f9f19f4290 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php @@ -68,4 +68,12 @@ final class DiffusionCommitVerifyTransaction $this->renderObject()); } + public function getTransactionTypeForConduit($xaction) { + return 'request-verification'; + } + + public function getFieldValuesForConduit($object, $data) { + return array(); + } + } From 57b4b5981947d5ba6d601bfb399b8763141f94e0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 25 Oct 2018 05:44:34 -0700 Subject: [PATCH 10/10] When a Drydock host based on an Almanac blueprint has its binding disabled, stop handing out leases Summary: Ref T13210. Ref T12145. The "Almanac Host" blueprint currently hands out new leases on a given host even if the binding has been disabled. Although there are some more complicated cases here (e.g., involving cleanup of the existing resource and existing leases), this one seems clear cut: if the binding has been disabled, we should stop handing out new leases on it. Test Plan: - Created a service with two hosts. - Requested a lease, got host A. - Requested more leases, always got host A (we never build a new host when we don't have to, and we currently never have to). - Disabled the binding to host A. - Requested a lease. - Before patch: got host A. - After patch: got host B. - Also disabled the other binding to host B, requested a lease, got an indefinite wait for resources (which is expected and reasonable). Reviewers: amckinley Reviewed By: amckinley Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13210, T12145 Differential Revision: https://secure.phabricator.com/D19761 --- ...anacServiceHostBlueprintImplementation.php | 54 +++++++++++++------ 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php index ef904e1019..4724a2c807 100644 --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php @@ -113,6 +113,14 @@ final class DrydockAlmanacServiceHostBlueprintImplementation DrydockBlueprint $blueprint, DrydockResource $resource, DrydockLease $lease) { + + // Require the binding to a given host be active before we'll hand out more + // leases on the corresponding resource. + $binding = $this->loadBindingForResource($resource); + if ($binding->getIsDisabled()) { + return false; + } + return true; } @@ -154,24 +162,10 @@ final class DrydockAlmanacServiceHostBlueprintImplementation DrydockLease $lease, $type) { - $viewer = PhabricatorUser::getOmnipotentUser(); - switch ($type) { case DrydockCommandInterface::INTERFACE_TYPE: $credential_phid = $blueprint->getFieldValue('credentialPHID'); - $binding_phid = $resource->getAttribute('almanacBindingPHID'); - - $binding = id(new AlmanacBindingQuery()) - ->setViewer($viewer) - ->withPHIDs(array($binding_phid)) - ->executeOne(); - if (!$binding) { - throw new Exception( - pht( - 'Unable to load binding "%s" to create command interface.', - $binding_phid)); - } - + $binding = $this->loadBindingForResource($resource); $interface = $binding->getInterface(); return id(new DrydockSSHCommandInterface()) @@ -213,7 +207,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation $blueprint->getBlueprintName())); } - $viewer = PhabricatorUser::getOmnipotentUser(); + $viewer = $this->getViewer(); $services = id(new AlmanacServiceQuery()) ->setViewer($viewer) ->withPHIDs($service_phids) @@ -246,7 +240,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation private function loadFreeBindings(DrydockBlueprint $blueprint) { if ($this->freeBindings === null) { - $viewer = PhabricatorUser::getOmnipotentUser(); + $viewer = $this->getViewer(); $pool = id(new DrydockResourceQuery()) ->setViewer($viewer) @@ -293,5 +287,31 @@ final class DrydockAlmanacServiceHostBlueprintImplementation ); } + private function loadBindingForResource(DrydockResource $resource) { + $binding_phid = $resource->getAttribute('almanacBindingPHID'); + if (!$binding_phid) { + throw new Exception( + pht( + 'Drydock resource ("%s") has no Almanac binding PHID, so its '. + 'binding can not be loaded.', + $resource->getPHID())); + } + + $viewer = $this->getViewer(); + + $binding = id(new AlmanacBindingQuery()) + ->setViewer($viewer) + ->withPHIDs(array($binding_phid)) + ->executeOne(); + if (!$binding) { + throw new Exception( + pht( + 'Unable to load Almanac binding ("%s") for resource ("%s").', + $binding_phid, + $resource->getPHID())); + } + + return $binding; + } }