Page MenuHomePhabricator

Unify intracluster sync and Drydock working copy construction timeouts as a repository "copy time limit"

Authored by epriestley on Nov 16 2018, 5:19 PM.



Depends on D19814. Ref T13216. See PHI885. For various eldritch reasons, git fetch can hang. Although we'd probably like to fix this with git fetch --require-sustained-network-transfer-rate=512KB/5s or similar, that flag doesn't exist and we don't have a reasonable way to build it.

Short of that, move toward formalizing a repository "copy time limit": the longest amount of time anything may spend trying to make a copy of this repository.

This grows out of the existing intracluster sync limit, which is effectively the same thing. Here, apply it to git clone and git fetch in Drydock working copy construction, too. A future change may make it configurable.

Test Plan
  • Set the limit to 0.001.
  • Tried to build and lease working copies, got sensible timeout errors (see D19815).
<Activation Failed> Lease activation failed: [CommandException] Command killed by timeout after running for more than 0.001 seconds.
ssh '-o' 'LogLevel=quiet' '-o' 'StrictHostKeyChecking=no' '-o' 'UserKnownHostsFile=/dev/null' '-o' 'BatchMode=yes' -l '********' -p '2222' -i '********' '' -- '(cd '\''/var/drydock/workingcopy-163/repo/spellbook/'\'' && git clean -d --force && git fetch && git reset --hard)'

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Nov 16 2018, 5:19 PM
Owners added a subscriber: Restricted Owners Package.Nov 16 2018, 5:19 PM
epriestley requested review of this revision.Nov 16 2018, 5:20 PM
amckinley accepted this revision.Nov 16 2018, 8:13 PM
amckinley added inline comments.

Maybe make this "See T13108 and PHI885."?


Could this result in a collision? Or are these directories already guaranteed to be unique?


We don't want to also do these in parallel like activateResource() now does?

33–34 ↗(On Diff #47324)

Just checking that this was intentional, and not a temporary logging change.


Presumably this will eventually become configurable on a per-repository basis, right?

This revision is now accepted and ready to land.Nov 16 2018, 8:13 PM
epriestley marked 4 inline comments as done.Nov 16 2018, 8:49 PM
epriestley added inline comments.

The directories are "sort of guaranteed unique for our purposes" if we get this far since they're already keys in $map.

They're originally derived elsewhere from $repository->getCloneName(), which is not always guaranteed to be unique, but I think that's more of an issue for a higher-level layer to care about. We aren't creating any new problems by doing this, at least.


It would probably be slightly preferable, but since the two commands have to go in sequence it would be a bigger rework than the clone rework above. In practice, although we use this stuff (multiple working copies: libphutil/ + arcanist/ + phabricator/) other projects normally don't so the benefit is likely minimal. I reworked above but not here just out of guesses about patch size and comfort with how thoroughly I was testing.

I think this stuff is likely to get more significant work in some future iteration, e.g. see T9492.

33–34 ↗(On Diff #47324)

This got backed out in master already, see T13179.


Yeah, see T13216#242393 for my current thinking.

This revision was automatically updated to reflect the committed changes.