Added 'git-retry' bootstrap

Added 'git-retry' bootstrap

Added the 'git-retry' bootstrap command. This can be used to wrap other 'git'
commands around a fault-tolerant retry wrapper.

BUG=295109
TEST=localtest
R=iannucci@chromium.org, petermayo@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=285939

[dnj, 2014-07-17 20:21:54]

PTAL

[dnj, 2014-07-17 22:27:40]

PTAL

[dnj, 2014-07-22 17:37:21]

Ping? This is ready for review!

[Peter Mayo, 2014-07-22 22:34:36]

https://codereview.chromium.org/401673003/diff/20001/git_retry.py
File git_retry.py (right):

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode26
git_retry.py:26: def run(self):
why not chunks = [] and replace self.chunks with chunks below?

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode52
git_retry.py:52: stderr[start:end])
why not just 
self.logger.info("Encountered known transient error: [%s]",
  stderr[m.start():m.end()])
?

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode58
git_retry.py:58: args,
This potentially desynchronizes stdout & stderr.

> foo
! bang
> bar

may now come out as 
> foo
> bar
! bang

Actually seems OK, but I don't know the git discipline.

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode72
git_retry.py:72: return proc.returncode, None, stderr_tee.data
returning stdout as None because we left it on sys.stdout seems wierd

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode92
git_retry.py:92: # Delay (if requested)
- Doesn't make sense to delay after last (failed) attempt.
- I would prefer the delay, and the evolution of delay be at least as separated
as execute.
- I would prefer an exponential delay be the default
- I would prefer factors other than 2 be supported.

suggest adding line 78.5:
delay = self.compute_delay(i)
if delay > 0:
  self.logger.debug("Delaying for %s until next retry", delay)
  time.sleep(delay.total_seconds())

[dnj, 2014-07-22 23:16:20]

https://codereview.chromium.org/401673003/diff/20001/git_retry.py
File git_retry.py (right):

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode26
git_retry.py:26: def run(self):
On 2014/07/22 22:34:36, Peter Mayo wrote:
> why not chunks = [] and replace self.chunks with chunks below?

Done.

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode52
git_retry.py:52: stderr[start:end])
On 2014/07/22 22:34:36, Peter Mayo wrote:
> why not just 
> self.logger.info("Encountered known transient error: [%s]",
>   stderr[m.start():m.end()])
> ?

I liked one call vs. 2, but it does look better. Done.

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode58
git_retry.py:58: args,
On 2014/07/22 22:34:36, Peter Mayo wrote:
> This potentially desynchronizes stdout & stderr.
> 
> > foo
> ! bang
> > bar
> 
> may now come out as 
> > foo
> > bar
> ! bang
> 
> Actually seems OK, but I don't know the git discipline.
> 

You're completely correct. However, AFAIK any assumptions about STDOUT and
STDERR being synchronized are bad assumptions. The two serve different purposes
and carry independent data streams.

As things stand, all of the transient errors that we're looking for are
currently in STDERR, so I'm taking the lighter-weight approach and only tee-ing
it. If we need to ever examine STDOUT, we can add it later pretty painlessly.

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode72
git_retry.py:72: return proc.returncode, None, stderr_tee.data
On 2014/07/22 22:34:36, Peter Mayo wrote:
> returning stdout as None because we left it on sys.stdout seems wierd

Just leaving room in case we need to filter on STDOUT later. It feels more
Pythonic to return them as a tuple, just like 'proc.communicate()' does.

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode92
git_retry.py:92: # Delay (if requested)
On 2014/07/22 22:34:36, Peter Mayo wrote:
> - Doesn't make sense to delay after last (failed) attempt.
> - I would prefer the delay, and the evolution of delay be at least as
separated
> as execute.
> - I would prefer an exponential delay be the default
> - I would prefer factors other than 2 be supported.
> 
> suggest adding line 78.5:
> delay = self.compute_delay(i)
> if delay > 0:
>   self.logger.debug("Delaying for %s until next retry", delay)
>   time.sleep(delay.total_seconds())

Done.

[Peter Mayo, 2014-07-22 23:39:55]

lgtm

https://codereview.chromium.org/401673003/diff/20001/git_retry.py
File git_retry.py (right):

https://codereview.chromium.org/401673003/diff/20001/git_retry.py#newcode52
git_retry.py:52: stderr[start:end])
On 2014/07/22 23:16:20, dnj wrote:
> On 2014/07/22 22:34:36, Peter Mayo wrote:
> > why not just 
> > self.logger.info("Encountered known transient error: [%s]",
> >   stderr[m.start():m.end()])
> > ?
> 
> I liked one call vs. 2, but it does look better. Done.

The "if" can disappear now too. Taking the side-effect variance of turning up
logging with it.

https://codereview.chromium.org/401673003/diff/40001/git_retry.py
File git_retry.py (right):

https://codereview.chromium.org/401673003/diff/40001/git_retry.py#newcode87
git_retry.py:87: return iteration * self.delay
won't this be negative in the usual case?

https://codereview.chromium.org/401673003/diff/40001/git_retry.py#newcode89
git_retry.py:89: return (self.delay_factor ** (iteration - 1)) * self.delay
Consider dividing and using (count - iteration) instead (delay becomes maximum
instead of minimum delay)

or perhaps choosing this when delay < 0

[dnj, 2014-07-23 00:04:52]

https://codereview.chromium.org/401673003/diff/40001/git_retry.py
File git_retry.py (right):

https://codereview.chromium.org/401673003/diff/40001/git_retry.py#newcode87
git_retry.py:87: return iteration * self.delay
On 2014/07/22 23:39:55, Peter Mayo wrote:
> won't this be negative in the usual case?

The negative was more of a catch-all; the usual case here is '0'. I'm going to
just constrain this value to a positive (>=0) integer in the constructor.

https://codereview.chromium.org/401673003/diff/40001/git_retry.py#newcode89
git_retry.py:89: return (self.delay_factor ** (iteration - 1)) * self.delay
On 2014/07/22 23:39:55, Peter Mayo wrote:
> Consider dividing and using (count - iteration) instead (delay becomes maximum
> instead of minimum delay)
> 
> or perhaps choosing this when delay < 0

I don't want to make this 'factor' number too complicated. It already chooses
between linear and non-linear delays. I'd like the default to be a good common
option choice. Right now, the default goes: 0, 3, 6, 12, 24.

Chromite uses a linear scaling (iteration * factor), which is why that was
default in the previous patch set. However, I think the above numbers ride a
nice line between giving ample retry time and failing bots semi-fast. Thoughts?

[dnj, 2014-07-28 17:36:37]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2014-07-28 17:36:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnj@chromium.org/401673003/60001

[commit-bot: I haz the power, 2014-07-28 17:39:10]

Change committed as 285939