Add retry on image push 5xx errors #2043
Merged
Conversation
This file contains 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
Some registries can be flaky and return intermittent 5xx errors. This change allows those errors to be retried, similarly to network-level errors. Note that this needs the upstream containerd fix containerd/containerd#5276 to work reliably. This was tested with a registry that was modified to return 504 on every other manifest PUT. Without the change, exports to the registry fail every other attempt. With the change and the related containerd change, exports to the registry always succeed. Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
aaronlehmann
commented
Mar 27, 2021
| @@ -65,7 +65,7 @@ func CopyChain(ctx context.Context, ingester content.Ingester, provider content. | |||
| handlers := []images.Handler{ | |||
| images.ChildrenHandler(provider), | |||
| filterHandler, | |||
| retryhandler.New(remotes.FetchHandler(ingester, &localFetcher{provider}), nil), | |||
| retryhandler.New(remotes.FetchHandler(ingester, &localFetcher{provider}), func(_ []byte) {}), | |||
There was a problem hiding this comment.
Passing nil to retryhandler.New would cause panics if the retry handler tried to log something.
aaronlehmann
commented
Mar 27, 2021
| @@ -133,6 +133,7 @@ func (sw *streamWriter) Close() error { | |||
| func LoggerFromContext(ctx context.Context) func([]byte) { | |||
| return func(dt []byte) { | |||
| pw, _, _ := progress.FromContext(ctx) | |||
| defer pw.Close() | |||
There was a problem hiding this comment.
I found that the client was getting "context cancelled" errors reading status if and only if the retry handler logged anything. It was a bit hard to track down the cause, but it looks like this missing Close kept the status stream open after the build was complete and led to a timeout.
tonistiigi
approved these changes
Mar 29, 2021
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Some registries can be flaky and return intermittent 5xx errors. This
change allows those errors to be retried, similarly to network-level
errors.
Note that this needs the upstream containerd fix
containerd/containerd#5276 to work reliably.
This was tested with a registry that was modified to return 504 on every
other manifest PUT. Without the change, exports to the registry fail
every other attempt. With the change and the related containerd change,
exports to the registry always succeed.
Signed-off-by: Aaron Lehmann alehmann@netflix.com