Page MenuHomePhabricator

Drydock may reclaim recently-used resources
Closed, ResolvedPublic


See PHI2177. An install reports that WorkingCopy resources are being reclaimed by Drydock immediately after all leases release.

The expected behavior is that resources have a 3-minute immunity window after releases release during which they can not be reclaimed (in DrydockWorker->canReclaimResource()).

Likely, nothing is actually updating dateModified. A probable fix is one of:

  1. bump dateModified before we release a lease; or
  2. add a field like lastActiveEpoch and bump that; or
  3. in addition to looking for active leases, also look for recently destroyed leases.

The advantage of (1) is that it's simple; the advantage of (2) is that it's explicit; the advantage of (3) is that LeaseUpdateWorker won't have to mutate resources. This is probably the most desirable advantage.

Event Timeline

epriestley triaged this task as Normal priority.May 3 2022, 5:35 PM
epriestley created this task.

Here's a fairly simple way to reproduce this:

  • Launch an EC2 instance with SSH on a public IP.
  • Add that host to Almanac.
  • Create an Almanac service with the host.
  • Create an Almanac Host blueprint in Drydock.
  • (My system only) Recompile PHP 8.1 with mysqli and pcntl support after previously overwriting it while building an embedded binary.
  • Start a taskmaster in foreground mode:
$ ./bin/phd debug task
  • Uhh, fix all the PHP 8.1 strlen(null) warnings (see T13588).

Grab a test lease on the host with:

$ ./bin/drydock lease --type host


$ ./bin/drydock command --lease ... -- ls -alh
total 28K
drwxr-x--- 4 ubuntu ubuntu 4.0K May  3 18:08 .
drwxr-xr-x 3 root   root   4.0K May  3 17:40 ..
-rw-r--r-- 1 ubuntu ubuntu  220 Jan  6 16:23 .bash_logout
-rw-r--r-- 1 ubuntu ubuntu 3.7K Jan  6 16:23 .bashrc
drwx------ 2 ubuntu ubuntu 4.0K May  3 18:08 .cache
-rw-r--r-- 1 ubuntu ubuntu  807 Jan  6 16:23 .profile
drwx------ 2 ubuntu ubuntu 4.0K May  3 17:40 .ssh

Looks good.

  • Release that lease (bin/drydock release-lease --id ...).

Now, we want to create working copy pressure on the host.

  • Create a working copy blueprint with a low limit (e.g. 2).
  • Authorize it on the host blueprint.

To create pressure without actually doing builds, we'd like to use bin/drydock lease --type working-copy so we don't have to create a Harbormaster build plan. This is a bit tricky to use because you have to specify which working copies you want with an internal undocumented attribute, and omitting it fatals. I'm not going to make this elegant here, but we can do better than a fatal:

RuntimeException: foreach() argument must be of type array|object, null given in...

(one orthogonal bug I found is that bin/drydock lease discards any blueprints provided in an attributes JSON)

(one orthogonal bug I found is that bin/drydock lease discards any blueprints provided in an attributes JSON)

Hmm, give me an example of what you're passing in and what you're expecting it to do?

I haven't touched any of this stuff in a while, but I think (?) there's currently no way to tell a lease which blueprint(s) you want it to use, although this is a generally sensible sort of thing to want to do, but maybe you're doing something which is expected to work and it's failing for some other reason. I've got the code open so I'm happy to take a look if you want to fill in the details a bit.

After D21796:

Exception: Working copy lease is missing required attribute "".

That's at least an improvement. Here's what it wants, which you can only figure out by digging in the source:

$ cat working-copy-attributes.json 
  "" : {
    "philter": {
      "phid": "PHID-REPO-pqcfafdedcwwrjwcosig"

Now this works:

$ ./bin/drydock lease --type working-copy --attributes working-copy-attributes.json
Queued lease for activation:

Waiting for daemons to activate lease...
<Lease Queued> Lease queued for acquisition.
<Waiting for Activation> Waiting 15 seconds for lease to activate.
<Lease Acquired> Lease acquired.
<Lease Activated> Lease activated.
Activation complete. This lease is permanent until manually released with:

        $ ./bin/drydock release-lease --id 13

Lease activated in 3,051ms.

...and we can verify that it built a working copy:

$ ./bin/drydock command --lease 13 -- pwd    
$ ./bin/drydock command --lease 13 -- ls -alh
total 144K
drwxrwxr-x 3 ubuntu ubuntu 4.0K May  3 18:49 .
drwxrwxr-x 3 ubuntu ubuntu 4.0K May  3 18:49 ..
drwxrwxr-x 8 ubuntu ubuntu 4.0K May  3 18:49 .git
-rw-rw-r-- 1 ubuntu ubuntu 4.4K May  3 18:49 FilterRule.php
-rw-rw-r-- 1 ubuntu ubuntu 105K May  3 18:49 bases.json
-rw-rw-r-- 1 ubuntu ubuntu  11K May  3 18:49 index.php
-rw-rw-r-- 1 ubuntu ubuntu   39 May  3 18:49 util.php

I'm going to unload a couple more strlen(null) changes and then work on applying resource pressure to trigger the reclaim behavior.

fill in the details a bit.

Sure. Our Drydock infrastructure organizes a few almanac devices into a drydock pool dedicated to a single repository's working copy (via respective blueprints).

When a working copy lease is created in the database, the attributes field contains an internal.blueprintPHIDs parameter.

The "expected" behavior would be that I could take the attributes JSON from the database, purge internal.awakenTaskIDs, and pass it into bin/drydock lease in order to obtain a lease on a specific blueprint's working copy.

This is not the case, however. The actual behavior is that a lease is created with a long list of blueprint PHIDs. I don't know what the material impacts of that are. However, when adjusting the code to accept my blueprint PHID and discard the others via the patch below, I get the behavior that I want (a lease allocated/activated for the specified blueprint's active, or soon-active resources.)

diff --git a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php
index 08f33c6b5f..8f58e8a5b6 100644
--- a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php
+++ b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php
@@ -89,7 +89,10 @@ final class DrydockManagementLeaseWorkflow
           'No blueprints exist which can plausibly allocate resources to '.
           'satisfy the requested lease.'));
-    $lease->setAllowedBlueprintPHIDs($allowed_phids);
+    if (!$lease->getAllowedBlueprintPHIDS()) {
+      $lease->setAllowedBlueprintPHIDs($allowed_phids);
+    }

     if ($until) {

That patch is reasonable, and shouldn't break anything as long as the list you provide is a subset of the possible list.

Requesting a lease with a specific set and/or order of blueprints, and even with a specific set and/or order of resources is a reasonable thing to do. There's currently no first-class support for this stuff, but from the CLI it would look approximately like your patch but probably with a little more error checking (e.g., allow the union of the provided list and the actually-usable list). I'll see if I can get a patch in, since I think there's nothing tricky about it (at least for blueprints).

The blueprint thing was on the way toward creating allocation pressure, so D21802 allows you to select a blueprint (or a set of possible blueprints) with --blueprint. You can specify an ID or PHID:

$ ./bin/drydock lease --type host --blueprint 1 [--blueprint ...]

The specified blueprints must be plausible blueprints that match the coarse parameters of the lease. After D21801, only plausible blueprints are selected internally. This mostly just prevents silly requests where you ask for a lease on a "host" resource built using a "working-copy" blueprint or whatever and it doesn't work because the parameters don't make sense.

(bin/drydock lease could now guess --type if --blueprint is provided, but I didn't make it do that.)

To create resource pressure, I'm now going to try this -- I guess I don't really need the --count flag, but it does make the terminal juggling slightly easier:

  • With "working-copy" blueprint limit 2, lease two copies of repository A.
    • Expect: both allocate and lease.
  • Queue leases for a copy of repository B.
    • Expect: lease waits for resources.
  • Release one of the A leases.
    • If the bug reproduces: instant reclaim on A.
    • Desired behavior: reclaim on A after 3 minutes.

This all worked as expected and the bug reproduced immediately. At least for the moment, I'm going to apply (3) as a fix because it's the least likely change to cause any kind of race anywhere down the line, since it keeps "LeaseUpdateWorker" from updating Resource objects.

Before, instant reclaim after lease destruction:

Screen Shot 2022-05-03 at 3.54.54 PM.png (81×835 px, 21 KB)

After, three minute delay:

Screen Shot 2022-05-03 at 3.54.37 PM.png (78×853 px, 22 KB)

Perhaps a philosophical question here is: do we care about which repositories are checked out in a working copy resource?

That is, a working copy resource is a directory (like /var/drydock/workingcopy-123/) and it may have multiple repository directories inside it (since the upstream needed that for phabricator/ and arcanist/). But, right now, the only way to get multiple working copies is to create the resource from a Harbormaster build plan that has "Also Clone" specified. In particular:

  • A lease that wants repository "X" can't lease a working copy that has repository "A" and then clone "X".
  • And a lease that wants repositories "X" and "Y" can't lease a working copy that has repository "X" and then clone just "Y".

In this sense, the "Limit" on a "Working Copy" resource acts mostly like a disk space limit: it roughly means "at most N repository directories on disk at once", with some wiggle room because of "Also Clone".

But probably no one cares about this, and a more sensible interpretation would be "at most N builds running at once", where N copies of every repository on disk is perfectly fine.

(There could be some "maximum age" on resources to eventually clean them up so you don't end up with tons of old copies of long-dead repositories.)

I'm not going to build this yet, but I think this is a reasonable improvement if further work on Drydock is motivated.

In any case, I think this is fixed, so I'm calling this one resolved until evidence to the contrary arises.