recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Fix an issue where newly created Drydock resources could be improperly acquired

Summary:
Ref T9252. This is mostly a fix for an edge case from D14236. Here's the setup:

- There are no resources.
- A request for a new resource arrives.
- We build a new resource.

Now, if we were leasing an existing resource, we'd call `canAcquireLeaseOnResource()` before acquiring a lease on the new resource.

However, for new resources we don't do that: we just acquire a lease immediately. This is wrong, because we now allow and expect some resources to be unleasable when created.

In a more complex workflow, this can also produce the wrong result and leave the lease acquired sub-optimally (and, today, deadlocked).

Make the "can we acquire?" pathway consistent for new and existing resources, so we always do the same set of checks.

Test Plan:
- Started daemons.
- Deleted all working copy resources.
- Ran two working-copy-using build plans at the same time.
- Before this change, one would often [1] acquire a lease on a pending resource which never allocated, then deadlock.
- After this change, the same thing happens except that the lease remains pending and the work completes.

[1] Although the race this implies is allowed (resource pool limits are soft/advisory, and it is expected that we may occasionally run over them), it's MUCH easier to hit right now than I would expect it to be, so I think there's probably at least one more small bug here somewhere. I'll see if I can root it out after this change.

Reviewers: chad, hach-que

Reviewed By: hach-que

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14272

epriestley 083a321d 4169d7bf

+27 -2
+27 -2
src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
··· 211 211 $exceptions); 212 212 } 213 213 214 + $resources = $this->removeUnacquirableResources($resources, $lease); 215 + if (!$resources) { 216 + // If we make it here, we just built a resource but aren't allowed 217 + // to acquire it. We expect this during routine operation if the 218 + // resource prevents acquisition until it activates. Yield and wait 219 + // for activation. 220 + throw new PhabricatorWorkerYieldException(15); 221 + } 222 + 214 223 // NOTE: We have not acquired the lease yet, so it is possible that the 215 224 // resource we just built will be snatched up by some other lease before 216 - // we can. This is not problematic: we'll retry a little later and should 217 - // suceed eventually. 225 + // we can acquire it. This is not problematic: we'll retry a little later 226 + // and should suceed eventually. 218 227 } 219 228 220 229 $resources = $this->rankResources($resources, $lease); ··· 382 391 )) 383 392 ->execute(); 384 393 394 + return $this->removeUnacquirableResources($resources, $lease); 395 + } 396 + 397 + 398 + /** 399 + * Remove resources which can not be acquired by a given lease from a list. 400 + * 401 + * @param list<DrydockResource> Candidate resources. 402 + * @param DrydockLease Acquiring lease. 403 + * @return list<DrydockResource> Resources which the lease may be able to 404 + * acquire. 405 + * @task allocator 406 + */ 407 + private function removeUnacquirableResources( 408 + array $resources, 409 + DrydockLease $lease) { 385 410 $keep = array(); 386 411 foreach ($resources as $key => $resource) { 387 412 $blueprint = $resource->getBlueprint();