host-ctr: Implement exponential backoff for image pulls #433

etungsten · 2019-10-17T22:42:11Z

Issue #, if available: Implements #431

Description of changes:

Implements exponential backoff for image pulls.
- Current configuration is maximum 5 attempts, retry interval multiplier of 2.
- Random jitter of 2-6 seconds is added to each retry interval
Moves signal notification channel set up to after image pull since that is mostly relevant to only when we're starting a container task.
Ran go fmt
Minor modification to an info log message

Testing done:
Locally testing host-ctr with bad/invalid image:

$ sudo ./host-ctr -source registry.hub.docker.com/library/busybox:asdasd -pull-image-only -containerd-socket /run/containerd/containerd.sock
WARN[0000] failed to pull image. Waiting 4.291s before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox:asdasd\": registry.hub.docker.com/library/busybox:asdasd not found"
WARN[0005] failed to pull image. Waiting 7.232s before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox:asdasd\": registry.hub.docker.com/library/busybox:asdasd not found"
WARN[0012] failed to pull image. Waiting 8.961s before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox:asdasd\": registry.hub.docker.com/library/busybox:asdasd not found"
WARN[0022] failed to pull image. Waiting 10.513s before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox:asdasd\": registry.hub.docker.com/library/busybox:asdasd not found"
WARN[0032] failed to pull image. Waiting 18.934s before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox:asdasd\": registry.hub.docker.com/library/busybox:asdasd not found"
ERRO[0052] retries exhausted: failed to resolve reference "registry.hub.docker.com/library/busybox:asdasd": registry.hub.docker.com/library/busybox:asdasd not found  ref="registry.hub.docker.com/library/busybox:asdasd"

With good image:

✦ $ sudo ./host-ctr -source registry.hub.docker.com/library/busybox:latest -pull-image-only -containerd-socket /run/containerd/containerd.sock
INFO[0000] Pulled successfully                           img="registry.hub.docker.com/library/busybox:latest"
INFO[0000] Unpacking...                                  img="registry.hub.docker.com/library/busybox:latest"

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

workspaces/host-ctr/cmd/host-ctr/main.go

jahkeup · 2019-10-17T23:35:04Z

workspaces/host-ctr/cmd/host-ctr/main.go

-		containerd.WithSchema1Conversion)
-	if err != nil {
-		return nil, errors.Wrap(err, "Failed to pull ctr image")
+	// Retry with exponential backoff when failures occur, with 10 attempts


Most folks would expect to see a fairly common implementation reused such as https://github.com/cenkalti/backoff, but this looks small enough to me to stand alone perfectly well in this use case.

workspaces/host-ctr/cmd/host-ctr/main.go

zmrow

🎃

etungsten · 2019-10-18T16:32:51Z

Addresses @jahkeup 's comments.
Replaces time.Sleep with select{ } on channel returned by time.NewTimer and ctx.Done()

jahkeup

I think we might want to handle the contexts throughout main to respond to signals thoroughly.

workspaces/host-ctr/cmd/host-ctr/main.go

etungsten · 2019-10-18T18:06:47Z

Addresses @jahkeup 's comment regarding signal channel.
Interrupting during image pull retries:

✦ $ sudo ./host-ctr -source registry.hub.docker.com/library/busybox -pull-image-only -containerd-socket /run/containerd/containerd.sock
WARN[0000] Failed to pull image. Waiting 1 seconds before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox\": object required"
WARN[0001] Failed to pull image. Waiting 2 seconds before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox\": object required"
WARN[0003] Failed to pull image. Waiting 4 seconds before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox\": object required"
WARN[0007] Failed to pull image. Waiting 8 seconds before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox\": object required"
WARN[0015] Failed to pull image. Waiting 16 seconds before retrying...  error="failed to resolve reference \"registry.hub.docker.com/library/busybox\": object required"
^CINFO[0015] Received signal: interrupt                   
ERRO[0015] context ended while retrying: failed to resolve reference "registry.hub.docker.com/library/busybox": object required  ref=registry.hub.docker.com/library/busybox

etungsten · 2019-10-18T18:20:42Z

Updates maximum retry interval from 32 seconds to 30 seconds.

etungsten · 2019-10-18T18:30:21Z

Sorry, I misunderstood the comment about the duration. Updated to guarantee maximum retry duration won't exceed 31 seconds (1+2+4+8+16).

workspaces/host-ctr/cmd/host-ctr/main.go

etungsten · 2019-10-18T22:59:08Z

Addresses @jahkeup 's comments

Ran go fmt
Use empty contexts for defer statements that's concerned with clean up
Create new cleanCtx for logging and deriving contexts for sending signals to container task.

jahkeup

Couple nits, but otherwise looks great!! 🐻

workspaces/host-ctr/cmd/host-ctr/main.go

etungsten · 2019-10-18T23:47:13Z

Addresses @jahkeup comments.

bcressey · 2019-10-19T16:18:51Z

Some thoughts:

we should add some jitter to the retry attempts, so we don't have a large number of nodes retrying exactly in lockstep
we should make sure that the system converges on a good state even in the face of a multi-day outage for the upstream registry

It's useful for host-ctr to periodically error out and restart from the top rather than retrying indefinitely; that can help if the underlying cause was transient (e.g. bad DNS resolvers) and would be fixed by an application restart. In that case the service needs to be configured to always restart.

In the context of a prolonged outage, a retry pattern like 1 -> 2 -> 4 -> 8 ... <restart> ... 1 -> 2 -> 4 -> 8 ... isn't ideal since we don't need the rapid bursts of retries, and that drives load on an upstream that may be struggling. Maybe a single try at start, then a random delay for 45-75 seconds between attempts. The service should also delay between restarts so we don't end up with two requests with no intervening delay (last request -> exit -> first request).

etungsten · 2019-10-21T17:48:14Z

Rebase develop.

jahkeup

@bcressey I think we should lift some of the handling into the unit, we can modify the RestartSec directive to specify a larger period of time to allow for retries. @etungsten we can also use a well-known returnable error (with var retriesError = ... or similar) to detect a retry error and sleep before exiting to add that jitter.

Either way, I think to make this reliable we'll also want to further designate errors to be handled to determine transient failures (where possible) from permanent failures (eg: user specified a bad image). This can be a fast follow item IMO. I think this already gets us to a better standing in terms of transient errors.

workspaces/host-ctr/cmd/host-ctr/main.go

etungsten · 2019-10-21T20:30:09Z

It seems to me that there isn't a good way to match on types of errors to determine if a failure was transient or not.
I tried reading the source for Client.Pull to see what error message suffix or prefix I should match on. I'm not sure if suffix and prefix matching is something we'd like to do, it doesn't seem reliable since the error messages could change depending how the underlying Resolver is implemented.
Please let me know if there's a preferred way of doing this.
For now, I've created an issue to track this #443

etungsten · 2019-10-21T21:15:34Z

@bcressey @jahkeup RestartSec is already set to 10 in host-containers@.service:
https://github.com/amazonlinux/PRIVATE-thar/blob/d0b543de747e76d0df0cd4154932d917c2a108b6/packages/workspaces/host-containers%40.service#L10-L11

jahkeup · 2019-10-21T21:18:25Z

RestartSec is already set to 10 in host-containers@.service:

Yeah, but I think to address @bcressey's concerns, we'd want to increase that and/or add some delay to the to-be-errored exit to make host-ctr spread out its restarts (maybe with a flag to control the behavior).

etungsten · 2019-10-21T21:25:29Z

Adds a random jitter of 2-6 to each retry attempt to prevent multiple clients from trying to request in synchronization.

Configuration allows for minimum total retry duration of (2*5+31)=41 and maximum retry duration of (6*5 + 31)=61 seconds

Addresses @jahkeup 's comment

etungsten · 2019-10-21T21:50:37Z

go fmt and stop the jitter from accumulating

etungsten · 2019-10-22T20:24:38Z

Increases RestartSec from 10 seconds to 45 seconds.

jahkeup

I think the current code paths are okay even with the small comments I have, if you have a chance to make these changes now please do. If we need to move on, please track em to fix! 👍

workspaces/host-ctr/cmd/host-ctr/main.go

Implements exponential backoff for image pulls. Also move signal handling channel set up to after image pull happens since its mostly relevant to only when we're starting a container task.

etungsten · 2019-10-23T20:52:35Z

Addresses @jahkeup 's comments

jahkeup

⛵ 💾 👍

jahkeup · 2019-10-23T21:53:41Z

workspaces/host-ctr/cmd/host-ctr/main.go

+			return nil, errors.Wrap(err, "retries exhausted")
+		}
+		// Add a random jitter between 2 - 6 seconds to the retry interval
+		retryIntervalWithJitter := retryInterval + time.Duration(rand.Int31n(jitterPeakAmplitude))*time.Millisecond + jitterLowerBound*time.Millisecond


nit: this should use types in the const where possible, but is small enough in scope that this isn't needed at the moment. Let's carry on, let's get this on a host!

etungsten requested review from iliana and jahkeup October 17, 2019 22:42

jahkeup reviewed Oct 17, 2019

View reviewed changes

zmrow approved these changes Oct 18, 2019

View reviewed changes

etungsten force-pushed the host-ctr-retry-pull branch from 62265a1 to 61c3cc9 Compare October 18, 2019 16:31

jahkeup suggested changes Oct 18, 2019

View reviewed changes

etungsten force-pushed the host-ctr-retry-pull branch from 61c3cc9 to 24633c1 Compare October 18, 2019 18:04

etungsten force-pushed the host-ctr-retry-pull branch from 24633c1 to 6d9636e Compare October 18, 2019 18:20

etungsten force-pushed the host-ctr-retry-pull branch from 6d9636e to 4f7e5cf Compare October 18, 2019 18:29

jahkeup suggested changes Oct 18, 2019

View reviewed changes

workspaces/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

workspaces/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

workspaces/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

etungsten force-pushed the host-ctr-retry-pull branch from 4f7e5cf to 0b08876 Compare October 18, 2019 22:56

jahkeup approved these changes Oct 18, 2019

View reviewed changes

workspaces/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

workspaces/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

etungsten force-pushed the host-ctr-retry-pull branch from 0b08876 to 788182e Compare October 18, 2019 23:47

etungsten mentioned this pull request Oct 21, 2019

host-ctr: fix references for non-ECR images #439

Merged

etungsten force-pushed the host-ctr-retry-pull branch from 788182e to 8624071 Compare October 21, 2019 17:47

jahkeup approved these changes Oct 21, 2019

View reviewed changes

workspaces/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

etungsten mentioned this pull request Oct 21, 2019

host-ctr image pull: differentiate transient and permanent errors and handle them differently #443

Open

etungsten force-pushed the host-ctr-retry-pull branch from 8624071 to cef01ec Compare October 21, 2019 21:18

etungsten force-pushed the host-ctr-retry-pull branch 2 times, most recently from b352492 to 03eb767 Compare October 21, 2019 21:49

etungsten force-pushed the host-ctr-retry-pull branch from 03eb767 to 7e80672 Compare October 22, 2019 20:24

tjkirch requested a review from bcressey October 23, 2019 17:27

jahkeup approved these changes Oct 23, 2019

View reviewed changes

workspaces/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

workspaces/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

workspaces/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

host-ctr: Implement exponential backoff for image pulls

c52ba69

Implements exponential backoff for image pulls. Also move signal handling channel set up to after image pull happens since its mostly relevant to only when we're starting a container task.

etungsten force-pushed the host-ctr-retry-pull branch from 7e80672 to c52ba69 Compare October 23, 2019 20:51

jahkeup approved these changes Oct 23, 2019

View reviewed changes

bcressey approved these changes Oct 24, 2019

View reviewed changes

etungsten merged commit 07ebe30 into develop Oct 24, 2019

etungsten deleted the host-ctr-retry-pull branch October 24, 2019 21:30

iliana added this to the v0.2.0 milestone Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

host-ctr: Implement exponential backoff for image pulls #433

host-ctr: Implement exponential backoff for image pulls #433

etungsten commented Oct 17, 2019 •

edited

Loading

jahkeup Oct 17, 2019

zmrow left a comment

etungsten commented Oct 18, 2019 •

edited

Loading

jahkeup left a comment

etungsten commented Oct 18, 2019

etungsten commented Oct 18, 2019

etungsten commented Oct 18, 2019

etungsten commented Oct 18, 2019

jahkeup left a comment

etungsten commented Oct 18, 2019

bcressey commented Oct 19, 2019 •

edited

Loading

etungsten commented Oct 21, 2019

jahkeup left a comment

etungsten commented Oct 21, 2019 •

edited

Loading

etungsten commented Oct 21, 2019

jahkeup commented Oct 21, 2019

etungsten commented Oct 21, 2019 •

edited

Loading

etungsten commented Oct 21, 2019

etungsten commented Oct 22, 2019

jahkeup left a comment

etungsten commented Oct 23, 2019

jahkeup left a comment

jahkeup Oct 23, 2019

host-ctr: Implement exponential backoff for image pulls #433

host-ctr: Implement exponential backoff for image pulls #433

Conversation

etungsten commented Oct 17, 2019 • edited Loading

jahkeup Oct 17, 2019

Choose a reason for hiding this comment

zmrow left a comment

Choose a reason for hiding this comment

etungsten commented Oct 18, 2019 • edited Loading

jahkeup left a comment

Choose a reason for hiding this comment

etungsten commented Oct 18, 2019

etungsten commented Oct 18, 2019

etungsten commented Oct 18, 2019

etungsten commented Oct 18, 2019

jahkeup left a comment

Choose a reason for hiding this comment

etungsten commented Oct 18, 2019

bcressey commented Oct 19, 2019 • edited Loading

etungsten commented Oct 21, 2019

jahkeup left a comment

Choose a reason for hiding this comment

etungsten commented Oct 21, 2019 • edited Loading

etungsten commented Oct 21, 2019

jahkeup commented Oct 21, 2019

etungsten commented Oct 21, 2019 • edited Loading

etungsten commented Oct 21, 2019

etungsten commented Oct 22, 2019

jahkeup left a comment

Choose a reason for hiding this comment

etungsten commented Oct 23, 2019

jahkeup left a comment

Choose a reason for hiding this comment

jahkeup Oct 23, 2019

Choose a reason for hiding this comment

etungsten commented Oct 17, 2019 •

edited

Loading

etungsten commented Oct 18, 2019 •

edited

Loading

bcressey commented Oct 19, 2019 •

edited

Loading

etungsten commented Oct 21, 2019 •

edited

Loading

etungsten commented Oct 21, 2019 •

edited

Loading