Page MenuHomePhabricator
Files F15416986

D10470.id25181.diff
No OneTemporary
Actions

D10470.id25181.diff
View Options

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
@@ -240,6 +240,31 @@
->setStatus(DrydockResourceStatus::STATUS_ALLOCATING)
->save();
+ // Pre-emptively allocate the lease on the resource inside the lock,
+ // to ensure that other allocators don't cause this worker to lose
+ // an allocation race. If we fail to allocate a lease here, then the
+ // blueprint is allocating resources it can't lease against.
+ //
+ // NOTE; shouldAllocateLease is specified to only check resource
+ // constraints, which means that it shouldn't be checking compatibility
+ // of details on resources or leases. If there are any
+ // shouldAllocateLease that use details on the resources or leases to
+ // complete their work, then we might have to change this to:
+ //
+ // $lease->setStatus(DrydockLeaseStatus::STATUS_ACQUIRING);
+ // $lease->setResourceID($resource->getID());
+ // $lease->attachResource($resource);
+ // $lease->save();
+ //
+ // and bypass the resource quota logic entirely (and just assume that
+ // a resource allocated by a blueprint can have the lease allocated
+ // against it).
+ //
+ if (!$blueprint->allocateLease($resource, $lease)) {
+ throw new Exception(
+ 'Blueprint allocated a resource, but can\'t lease against it.');
+ }
+
$lock->unlock();
} catch (Exception $ex) {
$lock->unlock();
@@ -253,23 +278,25 @@
throw $ex;
}
- if (!$blueprint->allocateLease($resource, $lease)) {
- // TODO: This "should" happen only if we lost a race with another lease,
- // which happened to acquire this resource immediately after we
- // allocated it. In this case, the right behavior is to retry
- // immediately. However, other things like a blueprint allocating a
- // resource it can't actually allocate the lease on might be happening
- // too, in which case we'd just allocate infinite resources. Probably
- // what we should do is test for an active or allocated lease and retry
- // if we find one (although it might have already been released by now)
- // and fail really hard ("your configuration is a huge broken mess")
- // otherwise. But just throw for now since this stuff is all edge-casey.
- // Alternatively we could bring resources up in a "BESPOKE" status
- // and then switch them to "OPEN" only after the allocating lease gets
- // its grubby mitts on the resource. This might make more sense but
- // is a bit messy.
- throw new Exception('Lost an allocation race?');
- }
+ // We do not need to call allocateLease here, because we have already
+ // performed this check inside the lock. If the logic at the end of the
+ // lock changes to bypass allocateLease, then we probably need to do some
+ // logic like (where STATUS_RESERVED does not count towards allocation
+ // limits):
+ //
+ // $lock->lock(10000);
+ // $lease->setStatus(DrydockLeaseStatus::STATUS_RESERVED);
+ // $lease->save();
+ // try {
+ // if (!$blueprint->allocateLease($resource, $lease)) {
+ // throw new Exception('Lost an allocation race?');
+ // }
+ // } catch (Exception $ex) {
+ // $lock->unlock();
+ // throw $ex;
+ // }
+ // $lock->unlock();
+ //
}
$blueprint = $resource->getBlueprint();

File Metadata

Mime Type
text/plain
Expires
Mar 21 2025, 2:40 PM (4 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7712840
Default Alt Text
D10470.id25181.diff (3 KB)

Event Timeline

Phabricator · Built by Phacility