1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-26 08:42:41 +01:00

Make Drydock lease infrastructure more nimble

Summary:
Ref T9252. Currently, Harbormaster does this when trying to acquire a working copy:

  - Ask for a working copy.
  - Yield for 15 seconds.
  - Check if we have a working copy yet.

That's OK, but Drydock takes ~1s to acquire a working copy lease if a resource is already available, so we end up doing this:

  - T+0: Ask for a working copy.
  - T+0: Yield for 15 seconds.
  - T+1: Working copy lease activates.
  - T+15: Working copy lease is used.
  - T+16: Build finishes.

So we end up spending about 2 seconds doing work and 14 seconds sleeping.

One way to fix this would be to fiddle with the yield duration, so we yield for 1, 2, 4, ... seconds or something. This probably isn't a bad idea for longer leases (i.e., wait for 15, 30, 45 ... seconds or similar) but it implies a lot of churn for short leases.

Instead, let tasks "awaken" other tasks when they complete. The "awaken" operation means: if a task is in a yielded state (no failures, no owner, explicitly yielded, future expires time), pretend it only yielded until right now instead of whenever it really yielded to.

Basically, this rewrites history so that even though Harbormaster did a `yield(15)`, we pretend it did a `yield(4)` after we activate the lease if lease activation took 4 seconds.

If this misses, it's fine: we fall back to the normal yield behavior and things move forward normally a few seconds later.

If it hits, we get a more nimble process pretty cleanly.

Test Plan:
  - Restarted a build plan (lease working copy + run `ls`) with this patch no-op'd, took about 16 seconds.
  - Restarted a build plan with this patch active, took about 1 second.

Reviewers: hach-que, chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14178
This commit is contained in:
epriestley 2015-09-28 09:35:40 -07:00
parent cd2dd2a08f
commit 9b29d46e60
7 changed files with 106 additions and 2 deletions

View file

@ -334,10 +334,15 @@ final class DrydockLease extends DrydockDAO
), ),
array( array(
'objectPHID' => $this->getPHID(), 'objectPHID' => $this->getPHID(),
'delayUntil' => $epoch, 'delayUntil' => ($epoch ? (int)$epoch : null),
)); ));
} }
public function setAwakenTaskIDs(array $ids) {
$this->setAttribute('internal.awakenTaskIDs', $ids);
return $this;
}
private function didActivate() { private function didActivate() {
$viewer = PhabricatorUser::getOmnipotentUser(); $viewer = PhabricatorUser::getOmnipotentUser();
$need_update = false; $need_update = false;
@ -359,6 +364,11 @@ final class DrydockLease extends DrydockDAO
if ($expires) { if ($expires) {
$this->scheduleUpdate($expires); $this->scheduleUpdate($expires);
} }
$awaken_ids = $this->getAttribute('internal.awakenTaskIDs');
if (is_array($awaken_ids) && $awaken_ids) {
PhabricatorWorker::awakenTaskIDs($awaken_ids);
}
} }

View file

@ -218,7 +218,7 @@ final class DrydockResource extends DrydockDAO
), ),
array( array(
'objectPHID' => $this->getPHID(), 'objectPHID' => $this->getPHID(),
'delayUntil' => $epoch, 'delayUntil' => ($epoch ? (int)$epoch : null),
)); ));
} }

View file

@ -6,6 +6,16 @@
abstract class HarbormasterBuildStepImplementation extends Phobject { abstract class HarbormasterBuildStepImplementation extends Phobject {
private $settings; private $settings;
private $currentWorkerTaskID;
public function setCurrentWorkerTaskID($id) {
$this->currentWorkerTaskID = $id;
return $this;
}
public function getCurrentWorkerTaskID() {
return $this->currentWorkerTaskID;
}
public static function getImplementations() { public static function getImplementations() {
return id(new PhutilClassMapQuery()) return id(new PhutilClassMapQuery())

View file

@ -54,6 +54,11 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation
->setAttribute('repositoryPHID', $repository_phid) ->setAttribute('repositoryPHID', $repository_phid)
->setAttribute('commit', $commit); ->setAttribute('commit', $commit);
$task_id = $this->getCurrentWorkerTaskID();
if ($task_id) {
$lease->setAwakenTaskIDs(array($task_id));
}
$lease->queueForActivation(); $lease->queueForActivation();
$build_target $build_target

View file

@ -59,6 +59,7 @@ final class HarbormasterTargetWorker extends HarbormasterWorker {
} }
$implementation = $target->getImplementation(); $implementation = $target->getImplementation();
$implementation->setCurrentWorkerTaskID($this->getCurrentWorkerTaskID());
$implementation->execute($build, $target); $implementation->execute($build, $target);
$next_status = HarbormasterBuildTarget::STATUS_PASSED; $next_status = HarbormasterBuildTarget::STATUS_PASSED;

View file

@ -8,6 +8,7 @@ abstract class PhabricatorWorker extends Phobject {
private $data; private $data;
private static $runAllTasksInProcess = false; private static $runAllTasksInProcess = false;
private $queuedTasks = array(); private $queuedTasks = array();
private $currentWorkerTask;
// NOTE: Lower priority numbers execute first. The priority numbers have to // NOTE: Lower priority numbers execute first. The priority numbers have to
// have the same ordering that IDs do (lowest first) so MySQL can use a // have the same ordering that IDs do (lowest first) so MySQL can use a
@ -18,6 +19,10 @@ abstract class PhabricatorWorker extends Phobject {
const PRIORITY_BULK = 3000; const PRIORITY_BULK = 3000;
const PRIORITY_IMPORT = 4000; const PRIORITY_IMPORT = 4000;
/**
* Special owner indicating that the task has yielded.
*/
const YIELD_OWNER = '(yield)';
/* -( Configuring Retries and Failures )----------------------------------- */ /* -( Configuring Retries and Failures )----------------------------------- */
@ -77,6 +82,23 @@ abstract class PhabricatorWorker extends Phobject {
return null; return null;
} }
public function setCurrentWorkerTask(PhabricatorWorkerTask $task) {
$this->currentWorkerTask = $task;
return $this;
}
public function getCurrentWorkerTask() {
return $this->currentWorkerTask;
}
public function getCurrentWorkerTaskID() {
$task = $this->getCurrentWorkerTask();
if (!$task) {
return null;
}
return $task->getID();
}
abstract protected function doWork(); abstract protected function doWork();
final public function __construct($data) { final public function __construct($data) {
@ -105,6 +127,14 @@ abstract class PhabricatorWorker extends Phobject {
$data, $data,
$options = array()) { $options = array()) {
PhutilTypeSpec::checkMap(
$options,
array(
'priority' => 'optional int|null',
'objectPHID' => 'optional string|null',
'delayUntil' => 'optional int|null',
));
$priority = idx($options, 'priority'); $priority = idx($options, 'priority');
if ($priority === null) { if ($priority === null) {
$priority = self::PRIORITY_DEFAULT; $priority = self::PRIORITY_DEFAULT;
@ -208,4 +238,49 @@ abstract class PhabricatorWorker extends Phobject {
return $this->queuedTasks; return $this->queuedTasks;
} }
/**
* Awaken tasks that have yielded.
*
* Reschedules the specified tasks if they are currently queued in a yielded,
* unleased, unretried state so they'll execute sooner. This can let the
* queue avoid unnecessary waits.
*
* This method does not provide any assurances about when these tasks will
* execute, or even guarantee that it will have any effect at all.
*
* @param list<id> List of task IDs to try to awaken.
* @return void
*/
final public static function awakenTaskIDs(array $ids) {
if (!$ids) {
return;
}
$table = new PhabricatorWorkerActiveTask();
$conn_w = $table->establishConnection('w');
// NOTE: At least for now, we're keeping these tasks yielded, just
// pretending that they threw a shorter yield than they really did.
// Overlap the windows here to handle minor client/server time differences
// and because it's likely correct to push these tasks to the head of their
// respective priorities. There is a good chance they are ready to execute.
$window = phutil_units('1 hour in seconds');
$epoch_ago = (PhabricatorTime::getNow() - $window);
queryfx(
$conn_w,
'UPDATE %T SET leaseExpires = %d
WHERE id IN (%Ld)
AND leaseOwner = %s
AND leaseExpires > %d
AND failureCount = 0',
$table->getTableName(),
$epoch_ago,
$ids,
self::YIELD_OWNER,
$epoch_ago);
}
} }

View file

@ -141,6 +141,7 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask {
$worker = null; $worker = null;
try { try {
$worker = $this->getWorkerInstance(); $worker = $this->getWorkerInstance();
$worker->setCurrentWorkerTask($this);
$maximum_failures = $worker->getMaximumRetryCount(); $maximum_failures = $worker->getMaximumRetryCount();
if ($maximum_failures !== null) { if ($maximum_failures !== null) {
@ -175,6 +176,8 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask {
} catch (PhabricatorWorkerYieldException $ex) { } catch (PhabricatorWorkerYieldException $ex) {
$this->setExecutionException($ex); $this->setExecutionException($ex);
$this->setLeaseOwner(PhabricatorWorker::YIELD_OWNER);
$retry = $ex->getDuration(); $retry = $ex->getDuration();
$retry = max($retry, 5); $retry = max($retry, 5);