Arvados

Adding nearly-full jitter to the exponential backoff timing, and supporting Retry-After: {datetime} response headers:

20540-crunch-run-retry @ 54e9c47a8340c5952e8e4c0e96e33eb47c3cb963 -- developer-run-tests: #3678

Tom Clegg wrote in #note-4:

Adding nearly-full jitter to the exponential backoff timing, and supporting Retry-After: {datetime} response headers:

20540-crunch-run-retry @ 54e9c47a8340c5952e8e4c0e96e33eb47c3cb963 -- developer-run-tests: #3678

Could you confirm that the value of "min" provided to exponentialBackoff can never be 0?

Peter Amstutz wrote in #note-5:

Could you confirm that the value of "min" provided to exponentialBackoff can never be 0?

That's a good point, version in #note-3 didn't handle small/zero values of min very well. When min=0, it would always use delay=0 between attempts.

With this commit, min<1s means delay exactly min (even if zero) after the first attempt, but after subsequent attempts, behave as if min was 1s (i.e., delay between 1 and 2^n seconds).

20540-crunch-run-retry @ f4a37bb12147851402fd585e6e5987f227591028 -- developer-run-tests: #3680

This looks good, my only remaining comment is that either the default should be a bit longer (8 or 10 minutes instead of 5 minutes) or that system services like crunch-run specifically should use a higher limit.

This is based on the experience of seeing the API server get saturated for several minutes, I want crunch-run in particular to try very hard to make sure it doesn't abandon containers at the finalization stage (we talked about having the retry time even being proportional to the container runtime, it is rational to wait up to the length of time the container ran to register a successful result.)

I was reluctant to change the default total-sleep-between-retries too drastically, 6s to 3m already seems like a lot.

Crunch-run's existing code had Retries=8, which used to mean 8x 2-second delays, but now means up to 8 minutes of exponential-backoff. Raised that to 10m:

20540-crunch-run-retry @ 0afb0b057d2572487cc00aa1dc2631cb99713ea7 -- developer-run-tests: #3685

Raising the limit further as the container runs sounds reasonable, although we might still want an upper limit. If a container ran for 4 days but then controller has since been unreachable for 3 straight days, is it really sensible to wait one more day?

Should I merge this and leave the issue open to do the "retry time proportional to runtime" thing? Or should we make that a separate issue and schedule it separately?

Tom Clegg wrote in #note-11:

I was reluctant to change the default total-sleep-between-retries too drastically, 6s to 3m already seems like a lot.

Crunch-run's existing code had Retries=8, which used to mean 8x 2-second delays, but now means up to 8 minutes of exponential-backoff. Raised that to 10m:

20540-crunch-run-retry @ 0afb0b057d2572487cc00aa1dc2631cb99713ea7 -- developer-run-tests: #3685

Ah, I overlooked that the original setting of Retries=8 translated into 8 minutes already, I was just looking at the new default of 5 minutes. 10 minutes is good.

Raising the limit further as the container runs sounds reasonable, although we might still want an upper limit. If a container ran for 4 days but then controller has since been unreachable for 3 straight days, is it really sensible to wait one more day?

Right, a reasonable upper limit is probably only an hour or two, which puts it more into the realm of "surviving sustained downtime" not "surviving busy spikes and trouble-free system reboots".

Should I merge this and leave the issue open to do the "retry time proportional to runtime" thing? Or should we make that a separate issue and schedule it separately?

Yes, please merge with the 10 minute timeout. Make a separate issue for the backlog, we can remind ourselves if it becomes an issue in the future.