fix: retry docker pull network errors instead of failing deployments - #1836
Open
aws-kevinrickard wants to merge 8 commits into
Open
fix: retry docker pull network errors instead of failing deployments#1836aws-kevinrickard wants to merge 8 commits into
aws-kevinrickard wants to merge 8 commits into
Conversation
…tely A failed docker pull is classified by substring-matching the docker CLI's stderr against a list of seven known network-error strings. Anything that matches becomes ConnectionException and is retried; anything that does not becomes DockerPullException, which appears in no retry config's retryableExceptions list and so fails the deployment on the first attempt. A device whose network fails over mid-pull reports: read tcp 172.28.1.230:52044->34.204.60.241:443: read: connection reset by peer That matches none of the seven. It is read tcp rather than dial tcp, and read: connection reset by peer rather than read: connection timed out. The same failover happening a moment earlier, before the connection is established, produces dial tcp and does retry, which is why the failure looks intermittent and why redeploying succeeds. Invert the default for this classification. Known network errors keep mapping to ConnectionException and its indefinite retry, and a new explicit list of errors a retry cannot recover from (manifest unknown, no matching manifest, invalid reference format, no space left on device, authentication required, access denied) keeps mapping to DockerPullException and failing fast. Every one of those already failed fast before this change, so their behavior is unchanged. What changes is the remainder: an error matching neither list is now UnknownDockerPullException, retried up to 5 times at 10s to 2m before the deployment fails. Bounding those retries rather than routing them to the indefinite tier keeps the failure mode for a genuinely permanent error that the non-retryable list does not yet name to a bounded delay before the same failure surfaces, rather than a deployment that never completes. UnknownDockerPullException extends DockerPullException so the deployment error code reported once retries are exhausted is still DOCKER_PULL_ERROR.
The indefinite retry tier for docker registry network errors backs off exponentially from 1 minute to a 64 minute ceiling, so after roughly seven consecutive failures a device waits an hour between attempts. On a device whose connectivity recovers in between, recovery is gated on that interval rather than on the network, and the deployment looks stalled. The other artifact downloaders retry network errors indefinitely at a flat 1 minute (S3Downloader, GreengrassRepositoryDownloader), so this tier is the outlier. Cap it at 5 minutes: exponential backoff is retained, so a sustained outage still costs roughly a twelfth of the requests a flat 1 minute retry would make, while worst-case recovery latency drops from an hour to 5 minutes.
MikeDombo
reviewed
Aug 17, 2026
| * @return true if the error is a known network error | ||
| */ | ||
| static boolean isConnectionError(String err) { | ||
| String lowerCaseErr = err.toLowerCase(); |
Member
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
toLowerCase
The parameterless versions of String.toLowerCase() and String.toUpperCase() use the default locale of the JVM when transforming strings. This can have unintended results, including difficult-to-debug transient failures because case mapping differs based on locale.
Even if you aren’t writing code for a user experience that’s displayed to a customer, system internal code often uses case folding to normalize strings for comparison.
Not doing so can missing locale lead to errors when running on systems that are configured to use a different system locale. This can include developer desktops or build servers (not just your fleet hosts): many hours have been spent debugging errors that only occur on a specific machine due to its locale configuration.
Recommended solutions:
Always pass in a Locale.
Language or locale-dependent processing:
Get the locale from the platform, use the language of the content, or configure it in your code and pass that to the method.
String firstName = "Ichabod";
Locale locale = // Where you obtain the locale from is platform dependent
String lowerCaseFirstName = firstName.toLowerCase(locale);
Internal processing that isn’t locale-dependent:
Example: S3 bucket name
Always specify the language/country-neutral locale Locale.ROOT.
String imgTag = "IMG";
if ("img".equals(imgTag.toLowerCase(Locale.ROOT)) {
// do something
}
| * @return true if the error is known to be non-retryable | ||
| */ | ||
| static boolean isNonRetryableError(String err) { | ||
| String lowerCaseErr = err.toLowerCase(); |
Member
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
toLowerCase
The parameterless versions of String.toLowerCase() and String.toUpperCase() use the default locale of the JVM when transforming strings. This can have unintended results, including difficult-to-debug transient failures because case mapping differs based on locale.
Even if you aren’t writing code for a user experience that’s displayed to a customer, system internal code often uses case folding to normalize strings for comparison.
Not doing so can missing locale lead to errors when running on systems that are configured to use a different system locale. This can include developer desktops or build servers (not just your fleet hosts): many hours have been spent debugging errors that only occur on a specific machine due to its locale configuration.
Recommended solutions:
Always pass in a Locale.
Language or locale-dependent processing:
Get the locale from the platform, use the language of the content, or configure it in your code and pass that to the method.
String firstName = "Ichabod";
Locale locale = // Where you obtain the locale from is platform dependent
String lowerCaseFirstName = firstName.toLowerCase(locale);
Internal processing that isn’t locale-dependent:
Example: S3 bucket name
Always specify the language/country-neutral locale Locale.ROOT.
String imgTag = "IMG";
if ("img".equals(imgTag.toLowerCase(Locale.ROOT)) {
// do something
}
A docker pull that fails at the network transport layer is recoverable once connectivity is restored, so it belongs in the indefinite retry tier alongside the errors already classified as ConnectionException. The classification only recognized transport failures that happened before the connection was established, matching "dial tcp" but nothing for a connection that dies mid-transfer, so the error a device reports when its network fails over during a pull was not recognized as a network error at all: read tcp 172.28.1.230:52044->34.204.60.241:443: read: connection reset by peer Match on the Go net.OpError op prefix. Go renders that error as "<op> <net> <source>-><addr>: <cause>", so "read tcp" and "write tcp" classify a failure at the TCP layer regardless of which cause follows, including causes docker has not been observed emitting. This is what makes the classification robust rather than exhaustive: previously each new cause string needed to be enumerated, and the customer-reported error is precisely a cause that was not. Also match transport-level causes on their own, since docker and containerd surface some of them without an op prefix when wrapping an error raised while copying an image layer: connection reset by peer, connection refused, connection aborted, broken pipe, network is unreachable, network is down, host is unreachable, no route to host, i/o timeout, EOF and unexpected EOF, context deadline exceeded, and server misbehaving. The previously matched strings are all retained, so no error that was already retried indefinitely stops being. Two of them were matched case sensitively and are now matched case insensitively like the rest, which only widens what they match. connection refused and no route to host are listed explicitly but change nothing on their own, as Go always prefixes them with dial tcp. Collapse both classifications into lists rather than boolean chains, which keeps the matching in one place as the number of entries grows. Unrecognized errors continue to route to UnknownDockerPullException and its bounded retry, which remains the safety net for a genuinely novel error. This change narrows what reaches it to errors that are not identifiable as transport failures at all.
Matching the Go net.OpError op prefix covers any transport failure that reaches us as a net.OpError, whatever cause follows it. It does not cover transport failures raised at a layer that does not produce one, which arrive with no op prefix to match: - crypto/tls errors are not OpErrors, so a connection corrupted or torn down mid-stream reports "remote error: tls: bad record MAC", "tls: use of closed connection", or "remote error: tls: internal error" - HTTP/2 connection and stream failures report "http2: server sent GOAWAY", "http2: client connection lost", or "stream error: stream ID N; INTERNAL_ERROR" - containerd discards the OpError when wrapping a failure while copying an image layer, leaving "read |0: file already closed" Add these, plus "context canceled" alongside the "request canceled" already matched. http2 is matched on its package prefix for the same reason net/http already is; a stream error carries no such prefix and is matched separately. Only wire-level TLS failures are listed. An error reporting that the peer rejected our certificate or that we do not trust theirs is a configuration problem a retry cannot fix, so "x509:", "bad certificate" and "handshake failure" are deliberately absent, and are noted as such so they are not added later by pattern-matching on the others. Registry 5xx responses are also deliberately absent. A gateway error is retryable but is not a transport failure, and a registry returning it persistently is a real fault that should surface rather than retry forever. Those already reach the bounded retry tier, and reach the indefinite tier when the device is offline. This does not make the classification exhaustive and cannot: the strings are an undocumented contract spanning the docker CLI, dockerd, containerd and the Go standard library, and Go itself deprecated net.Error.Temporary() on the grounds that transient versus permanent is not well defined. These are the transport failures known to be missed today.
…s offline Classifying a docker pull failure depends on substring-matching docker's stderr, which is an undocumented contract spanning the docker CLI, dockerd, containerd and the Go standard library. No list of strings can be exhaustive, so a transport failure worded in a way not matched still lands in the unrecognized bucket and gives up after a bounded number of attempts, even though it would have recovered once connectivity returned. Use the device's own connectivity as a signal that does not depend on the error text at all. runWithConnectionErrorCheck already infers this for DockerServiceUnavailableException, which is likewise ambiguous about whether a retry can help; extend the same inference to UnknownDockerPullException. An unrecognized pull failure on a device whose MQTT connection is down is far more likely to be that outage than anything the registry reported, so it becomes a ConnectionException and is retried until connectivity returns. The bound still applies when the device is online, which is what keeps this from reintroducing the risk the bounded tier exists to avoid: an error that is genuinely permanent surfaces after a bounded delay on a healthy device rather than hanging a deployment that has no timeout. Two limits on how much this can be relied on, neither of which makes it worse than the status quo: - MQTT reachability does not prove registry reachability. They are different endpoints and can differ under a proxy or split routing, so an online device can still fail a pull for connectivity reasons and get only the bounded retry. - Detection lags the network. Keep-alive defaults to 60s with a 30s ping timeout, so getMqttOnline can report online for up to about 90s after connectivity is lost. A pull failing inside that window is not attributed to the outage. This is therefore a second line of defence behind the string matching rather than a replacement for it.
|
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against db427a5 |
|
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against db427a5 |
runWithRetry(DifferentiatedRetryConfig) tracked one retryInterval shared by every config in the list, and slept on it before clamping to the matched config's ceiling. A config with a high ceiling could therefore ramp the interval and leave a config with a much lower ceiling sleeping far past its own. Both existing callers hid this. DeploymentDocumentDownloader pairs two configs that are both flat at 1 minute, and the single-config runWithRetry overload delegates through fromRetryConfig, where one config cannot interfere with another. Pairing configs whose ceilings differ, as the docker downloader now does with 32 minutes and 2 minutes, makes the behaviour reachable: several DockerServiceUnavailableException failures ramp the interval toward 32 minutes, and an UnknownDockerPullException arriving afterwards sleeps for up to that long despite its own 2 minute ceiling. Track the interval per config, initialized from each config's own initialRetryInterval. This also removes the lazy zero-initialization, which previously meant a config's initialRetryInterval only applied when its exception happened to be the first failure. Behaviour is unchanged for a single config and for two configs with equal intervals, so neither existing caller is affected. The new test pairs a config ramping 200ms to 30s with one fixed at 10ms and asserts total elapsed time. It fails at 6178ms against the shared interval and passes at 2275ms with the fix.
…tures Two DockerImageDownloaderTest cases built an UnknownDockerPullException whose message was "connection reset by peer". That string is in CONNECTION_ERRORS, so DefaultDockerClient can never produce an UnknownDockerPullException carrying it, and the fixture implied a combination that cannot occur. Use the same unrecognized message the third case already used. Add classification cases for the connection error strings that had none: network is down, host is unreachable, connection aborted, context deadline exceeded, and server misbehaving.
The test asserted elapsed time under 3100ms, against a maximum of 3010ms for the fixed code, leaving 90ms of slack across five Thread.sleep calls. Thread.sleep guarantees only a minimum, so overshoot on a loaded machine could fail the assertion spuriously. The gap between the two cases works out to half the initial interval, because the leaked sleep is at least the ramped interval's half while the noise is the jitter spread of the sleeps preceding it. Sleeps late in a ramp dominate that spread, so one sleep on a large interval separates the cases far better than several on small ones. Ramp with a single 2s interval instead of four from 200ms. The fixed code now takes at most 2010ms and the shared interval at least 3000ms, so a 2500ms threshold leaves roughly 490ms either side. The test also runs faster, measuring 1932ms rather than 2275ms, and still fails against the shared interval, measuring 5641ms.
aws-kevinrickard
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DefaultDockerClient.pullImageclassifies a faileddocker pullby substring-matching docker's stderr against seven network-error strings:A match becomes
ConnectionException, which retries indefinitely. Everything else becomesDockerPullException, which no retry config lists as retryable, andRetryUtilsrethrows when nothing matches. So an unmatched error fails the deployment on the first attempt, with no backoff and no second try.A device whose connection fails over mid-pull reports:
That matches none of the seven: it says
read tcprather thandial tcp, andread: connection reset by peerrather thanread: connection timed out.dial tcpdoes match, which is why the failure looks intermittent. A failover landing before the connection opens retries indefinitely and stays invisible; one landing mid-transfer producesread tcpand fails outright. Pulls run long enough to make mid-transfer failover the likely case.The gap is structural. Each incident has extended the allowlist by one string (#1369, then #1507 for
dial tcp), so any transport error nobody anticipated fails a deployment with zero retries. Every other downloader classifies by exception type instead —S3DownloaderandGreengrassRepositoryDownloaderretryIOException, so a connection reset during an S3 download already retries today.One more string will not fix this
docker exposes no machine-readable signal. On docker 25.0.14, a DNS failure, a
manifest unknownand an invalid reference all exit1, anddocker pulloffers no--format. The Engine API reports failures as{"errorDetail":{"message":"..."}}inside a 200 response — the same string wearing JSON.Four layers wrap that text: the docker CLI, dockerd, containerd, and the Go standard library. Go once exposed the predicate this code needs and has since deprecated it:
The layer producing these errors gave up classifying them. This change therefore layers imperfect signals rather than chasing an exhaustive list.
Fix
Six separately reviewable commits described below in logical rather than commit order, plus one test-only commit.
1. Classify by the Go
net.OpErrorshape. Go renders anOpErroras"<op> <net> <source>-><addr>: <cause>". Matching the op prefix —read tcpandwrite tcp, joiningdial tcp— catches any TCP-layer failure whatever cause follows, including causes docker has never emitted. Doing so moves the dependency onto the stable half of the string: the standard library generates the prefix and has not changed it since Go 1.0, whereas the cause is a syscall errno string that varies by platform and kernel. The old allowlist enumerated the varying half.Transport causes still match on their own, because docker and containerd sometimes drop the op prefix when wrapping an error raised while copying a layer:
connection reset by peer,connection refused,connection aborted,broken pipe,network is unreachable,network is down,host is unreachable,no route to host,i/o timeout,EOF,unexpected EOF,context deadline exceeded,server misbehaving.All seven original strings remain, so nothing that already retried indefinitely stops. Two now match case-insensitively, which only widens them.
2. Match transport failures raised above
net. Some layers never build anOpError.crypto/tlsreportsremote error: tls: bad record MAC,tls: use of closed connectionandremote error: tls: internal error; HTTP/2 reportshttp2: server sent GOAWAY,http2: client connection lostandstream error: stream ID N; INTERNAL_ERROR; containerd discards theOpErrorwhile copying a layer, leavingread |0: file already closed. These now match, along withcontext canceled.Only wire-level TLS failures qualify. When the peer rejects our certificate or we distrust theirs, no retry helps, so
x509:,bad certificateandhandshake failurestay out and the code says why. Registry 5xx stays out too: retryable, but not transport, and a registry returning one persistently is a fault that should surface.3. Fall back on device connectivity.
runWithConnectionErrorCheckalready infers connectivity forDockerServiceUnavailableException; the same inference now coversUnknownDockerPullException. When MQTT is down, an unrecognized pull failure more likely reflects that outage than anything the registry said, so it retries until connectivity returns. An online device still gets the bound, which preserves the no-hang property.Two limits. MQTT reachability does not prove registry reachability, since the endpoints differ and can diverge under a proxy. And detection lags: keep-alive defaults to 60s with a 30s ping timeout, so
getMqttOnlinecan report online for up to roughly 90s after connectivity drops. A pull failing inside that window escapes the inference, which is why the string matching still carries the reported case. This signal backs up the strings rather than replacing them.4. Retry the remainder instead of failing immediately. Six unrecoverable errors —
manifest unknown,no matching manifest for,invalid reference format,no space left on device,authentication required,requested access to the resource is denied— now map explicitly toDockerPullExceptionand fail fast. All six already failed fast, so they become non-retryable by intent rather than by omission. Anything matching neither list becomes the newUnknownDockerPullExceptionand retries 5 times at 10s→2m, roughly 75–150 seconds with jitter. Only the currently broken bucket changes behavior.This tier stays bounded because it catches errors nothing is known about. Nucleus imposes no deployment timeout by design —
DefaultDeploymentTaskandDeploymentServiceboth block without one and delegate detection downstream — so routing unknowns to the indefinite tier would hang a deployment forever on a permanent error the list does not yet name, recoverable only by cancelling it. Guessing wrong should cost minutes, not a stuck device. That also rules out reusingfiniteAttemptsRetryConfig, whose 30 attempts at 10s→32m take hours.UnknownDockerPullException extends DockerPullException, so exhausting the retries still reportsDOCKER_PULL_ERRORwith the same text. BecauseRetryUtilsmatches withisInstanceand only the subclass appears in a retry config, a plainDockerPullExceptionstill fails fast.5. Back off each differentiated retry config independently.
runWithRetry(DifferentiatedRetryConfig)tracked oneretryIntervalshared by every config in the list, and slept on it before clamping to the matched config's ceiling — so a config with a high ceiling could ramp the interval and leave a config with a lower ceiling sleeping far past its own. Both existing callers hid this:DeploymentDocumentDownloaderpairs two configs both flat at 1 minute, and the single-config overload cannot interfere with itself. Pairing 32-minute and 2-minute ceilings, as item 4 does, makes it reachable — severalDockerServiceUnavailableExceptionfailures ramp the interval toward 32 minutes, and anUnknownDockerPullExceptionarriving afterwards sleeps that long despite its own ceiling. Tracking the interval per config fixes it and leaves both existing callers unaffected. Without this, item 4's bounded tier does not hold to the 75–150s it claims.6. Lower the indefinite tier's ceiling from 64 to 5 minutes. Droppable without affecting the rest. At 64 minutes, a device seven failures in waits an hour between attempts, so backoff gates recovery even after the network returns. The other downloaders retry at a flat 1 minute. Five minutes keeps exponential backoff — a 225s average sleep against flat-1-minute's 45s, so roughly a fifth of the requests during a sustained outage — while bounding recovery latency.
Note the scope:
infiniteAttemptsRetryConfigalso covers the ECR auth-token fetch and docker login, so this raises their steady-state retry rate too. That looks desirable for the same reason, but it is wider than the pull path.Testing
DefaultDockerClientTest(new) covers the two extracted predicates with 35 cases across network, known non-retryable and unrecognized errors. Each case also asserts the two classifications agree, sinceisConnectionErrorruns first. The network group quotes the reported string verbatim and fails against the previous code, and includes aread tcperror whose cause is deliberately a string docker does not emit, pinning the op prefix as the mechanism rather than the cause list.DockerImageDownloaderTestgains four cases: an unknown error retried to the bound and then surfacing asPackageDownloadException, asserting the attempt count so it fails against the old single-attempt behavior; one clearing on the second attempt; one on an offline device succeeding on the fifth attempt, which exceeds the bounded limit and so passes only via the offline inference — reverting that inference makes it fail, confirming it discriminates; and a plainDockerPullExceptionstill failing in one attempt.RetryUtilsTestgains a case pairing a config that ramps a 2s interval to 4s with one fixed at 10ms, asserting total elapsed time. It fails at 5641ms against the shared interval and passes at 1932ms with the per-config fix. A single large interval rather than a ramp over several small ones puts roughly 490ms either side of the 2500ms threshold, so no plausibleThread.sleepovershoot can flip the result.The pre-existing
GIVEN_network_error_WHEN_download_docker_image_THEN_retry_download_image_until_succeedpasses unchanged and guards fail-fast against regression. All 63 tests across the two docker classes pass, as doRetryUtilsTestandDeploymentDocumentDownloaderTest(the otherDifferentiatedRetryConfigcaller), andcheckstyle:check,pmd:checkandspotbugs:checkrun clean.Notes
Left for separate changes:
DOCKER_PULL_TIMEOUTis the bare string"timeout", andisConnectionErrorruns beforeisNonRetryableError, so a non-retryable error containingtimeoutreaches the indefinite tier. This predates the change and stays untouched, so the claim that no previously matched string stops matching holds exactly.OpErrorshape and the connectivity fallback narrow the exposure without removing it.DockerImageDownloader.run()nests the indefinite tier inside the finite one, so aConnectionExceptionnever reaches the outer tier.