diff --git a/resources/sql/autopatches/20150916.drydock.slotlocks.1.sql b/resources/sql/autopatches/20150916.drydock.slotlocks.1.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20150916.drydock.slotlocks.1.sql @@ -0,0 +1,8 @@ +CREATE TABLE {$NAMESPACE}_drydock.drydock_slotlock ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + ownerPHID VARBINARY(64) NOT NULL, + lockIndex BINARY(12) NOT NULL, + lockKey LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + UNIQUE KEY `key_lock` (lockIndex), + KEY `key_owner` (ownerPHID) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -863,6 +863,7 @@ 'DrydockResourceViewController' => 'applications/drydock/controller/DrydockResourceViewController.php', 'DrydockSFTPFilesystemInterface' => 'applications/drydock/interface/filesystem/DrydockSFTPFilesystemInterface.php', 'DrydockSSHCommandInterface' => 'applications/drydock/interface/command/DrydockSSHCommandInterface.php', + 'DrydockSlotLock' => 'applications/drydock/storage/DrydockSlotLock.php', 'DrydockWebrootInterface' => 'applications/drydock/interface/webroot/DrydockWebrootInterface.php', 'DrydockWorkingCopyBlueprintImplementation' => 'applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php', 'FeedConduitAPIMethod' => 'applications/feed/conduit/FeedConduitAPIMethod.php', @@ -4585,6 +4586,7 @@ 'DrydockResourceViewController' => 'DrydockResourceController', 'DrydockSFTPFilesystemInterface' => 'DrydockFilesystemInterface', 'DrydockSSHCommandInterface' => 'DrydockCommandInterface', + 'DrydockSlotLock' => 'DrydockDAO', 'DrydockWebrootInterface' => 'DrydockInterface', 'DrydockWorkingCopyBlueprintImplementation' => 'DrydockBlueprintImplementation', 'FeedConduitAPIMethod' => 'ConduitAPIMethod', diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php @@ -67,14 +67,13 @@ $device = $binding->getDevice(); $device_name = $device->getName(); + $binding_phid = $binding->getPHID(); + $resource = $this->newResourceTemplate($blueprint, $device_name) ->setActivateWhenAllocated(true) ->setAttribute('almanacServicePHID', $binding->getServicePHID()) - ->setAttribute('almanacBindingPHID', $binding->getPHID()); - - // TODO: This algorithm can race, and the "free" binding may not be - // free by the time we acquire it. Do slot-locking here if that works - // out, or some other kind of locking if it does not. + ->setAttribute('almanacBindingPHID', $binding_phid) + ->needSlotLock("almanac.host.binding({$binding_phid})"); try { return $resource->allocateResource(DrydockResourceStatus::STATUS_OPEN); @@ -93,8 +92,11 @@ DrydockResource $resource, DrydockLease $lease) { - // TODO: We'll currently lease each resource an unlimited number of times, - // but should stop doing that. + // TODO: The current rule is one lease per resource, and there's no way to + // make that cheaper here than by just trying to acquire the lease below, + // so don't do any special checks for now. When we eventually permit + // multiple leases per host, we'll need to load leases anyway, so we can + // reject fully leased hosts cheaply here. return true; } @@ -104,11 +106,11 @@ DrydockResource $resource, DrydockLease $lease) { - // TODO: Once we have limit rules, we should perform slot locking (or other - // kinds of locking) here. + $resource_phid = $resource->getPHID(); $lease ->setActivateWhenAcquired(true) + ->needSlotLock("almanac.host.lease({$resource_phid})") ->acquireOnResource($resource); } diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php --- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php @@ -106,8 +106,12 @@ DrydockLease $lease); final public function releaseLease( + DrydockBlueprint $blueprint, DrydockResource $resource, DrydockLease $lease) { + + // TODO: This is all broken nonsense. + $scope = $this->pushActiveScope(null, $lease); $released = false; @@ -117,6 +121,7 @@ $lease->reload(); if ($lease->getStatus() == DrydockLeaseStatus::STATUS_ACTIVE) { + $lease->release(); $lease->setStatus(DrydockLeaseStatus::STATUS_RELEASED); $lease->save(); $released = true; @@ -293,6 +298,7 @@ $resource = id(new DrydockResource()) ->setBlueprintPHID($blueprint->getPHID()) + ->attachBlueprint($blueprint) ->setType($this->getType()) ->setStatus(DrydockResourceStatus::STATUS_PENDING) ->setName($name); diff --git a/src/applications/drydock/storage/DrydockBlueprint.php b/src/applications/drydock/storage/DrydockBlueprint.php --- a/src/applications/drydock/storage/DrydockBlueprint.php +++ b/src/applications/drydock/storage/DrydockBlueprint.php @@ -163,6 +163,17 @@ } + /** + * @task lease + */ + public function releaseLease( + DrydockResource $resource, + DrydockLease $lease) { + $this->getImplementation()->releaseLease($this, $resource, $lease); + return $this; + } + + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php --- a/src/applications/drydock/storage/DrydockLease.php +++ b/src/applications/drydock/storage/DrydockLease.php @@ -15,6 +15,7 @@ private $releaseOnDestruction; private $isAcquired = false; private $activateWhenAcquired = false; + private $slotLocks = array(); /** * Flag this lease to be released when its destructor is called. This is @@ -128,6 +129,8 @@ $this->setStatus(DrydockLeaseStatus::STATUS_RELEASED); $this->save(); + DrydockSlotLock::releaseLocks($this->getPHID()); + $this->resource = null; return $this; @@ -206,6 +209,11 @@ return $this; } + public function needSlotLock($key) { + $this->slotLocks[] = $key; + return $this; + } + public function acquireOnResource(DrydockResource $resource) { $expect_status = DrydockLeaseStatus::STATUS_PENDING; $actual_status = $this->getStatus(); @@ -234,10 +242,17 @@ } } - $this - ->setResourceID($resource->getID()) - ->setStatus($new_status) - ->save(); + $this->openTransaction(); + + $this + ->setResourceID($resource->getID()) + ->setStatus($new_status) + ->save(); + + DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks); + $this->slotLocks = array(); + + $this->saveTransaction(); $this->isAcquired = true; diff --git a/src/applications/drydock/storage/DrydockResource.php b/src/applications/drydock/storage/DrydockResource.php --- a/src/applications/drydock/storage/DrydockResource.php +++ b/src/applications/drydock/storage/DrydockResource.php @@ -17,6 +17,7 @@ private $blueprint = self::ATTACHABLE; private $isAllocated = false; private $activateWhenAllocated = false; + private $slotLocks = array(); protected function getConfiguration() { return array( @@ -80,6 +81,11 @@ return $this; } + public function needSlotLock($key) { + $this->slotLocks[] = $key; + return $this; + } + public function allocateResource($status) { if ($this->getID()) { throw new Exception( @@ -105,11 +111,18 @@ $new_status = DrydockResourceStatus::STATUS_PENDING; } - $this - ->setStatus($new_status) - ->save(); + $this->openTransaction(); + + $this + ->setStatus($new_status) + ->save(); - $this->didAllocate = true; + DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks); + $this->slotLocks = array(); + + $this->saveTransaction(); + + $this->isAllocated = true; return $this; } @@ -151,6 +164,9 @@ $this->setStatus(DrydockResourceStatus::STATUS_CLOSED); $this->save(); + + DrydockSlotLock::releaseLocks($this->getPHID()); + $this->saveTransaction(); } diff --git a/src/applications/drydock/storage/DrydockSlotLock.php b/src/applications/drydock/storage/DrydockSlotLock.php new file mode 100644 --- /dev/null +++ b/src/applications/drydock/storage/DrydockSlotLock.php @@ -0,0 +1,107 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array( + 'lockIndex' => 'bytes12', + 'lockKey' => 'text', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_lock' => array( + 'columns' => array('lockIndex'), + 'unique' => true, + ), + 'key_owner' => array( + 'columns' => array('ownerPHID'), + ), + ), + ) + parent::getConfiguration(); + } + + public static function loadLocks($owner_phid) { + return id(new DrydockSlotLock())->loadAllWhere( + 'ownerPHID = %s', + $owner_phid); + } + + +/* -( Acquiring and Releasing Locks )-------------------------------------- */ + + + /** + * Acquire a set of slot locks. + * + * This method either acquires all the locks or throws an exception (usually + * because one or more locks are held). + * + * @param phid Lock owner PHID. + * @param list List of locks to acquire. + * @return void + * @task locks + */ + public static function acquireLocks($owner_phid, array $locks) { + if (!$locks) { + return; + } + + $table = new DrydockSlotLock(); + $conn_w = $table->establishConnection('w'); + + $sql = array(); + foreach ($locks as $lock) { + $sql[] = qsprintf( + $conn_w, + '(%s, %s, %s)', + $owner_phid, + PhabricatorHash::digestForIndex($lock), + $lock); + } + + // TODO: These exceptions are pretty tricky to read. It would be good to + // figure out which locks could not be acquired and try to improve the + // exception to make debugging easier. + + queryfx( + $conn_w, + 'INSERT INTO %T (ownerPHID, lockIndex, lockKey) VALUES %Q', + $table->getTableName(), + implode(', ', $sql)); + } + + + /** + * Release all locks held by an owner. + * + * @param phid Lock owner PHID. + * @return void + * @task locks + */ + public static function releaseLocks($owner_phid) { + $table = new DrydockSlotLock(); + $conn_w = $table->establishConnection('w'); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE ownerPHID = %s', + $table->getTableName(), + $owner_phid); + } + +} diff --git a/src/applications/drydock/worker/DrydockAllocatorWorker.php b/src/applications/drydock/worker/DrydockAllocatorWorker.php --- a/src/applications/drydock/worker/DrydockAllocatorWorker.php +++ b/src/applications/drydock/worker/DrydockAllocatorWorker.php @@ -350,7 +350,7 @@ DrydockBlueprint $blueprint, DrydockLease $lease) { $resource = $blueprint->allocateResource($lease); - $this->validateAllocatedResource($resource); + $this->validateAllocatedResource($blueprint, $resource, $lease); return $resource; } @@ -369,7 +369,6 @@ DrydockBlueprint $blueprint, $resource, DrydockLease $lease) { - $blueprint = $this->getBlueprintClass(); if (!($resource instanceof DrydockResource)) { throw new Exception(