Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add retry on image push 5xx errors #2043

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Add retry on image push 5xx errors #2043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion util/contentutil/copy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing nil to retryhandler.New would cause panics if the retry handler tried to log something.

}

if err := images.Dispatch(ctx, images.Handlers(handlers...), nil, desc); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion util/imageutil/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func Config(ctx context.Context, str string, resolver remotes.Resolver, cache Co
children := childrenConfigHandler(cache, platform)

handlers := []images.Handler{
retryhandler.New(remotes.FetchHandler(cache, fetcher), nil),
retryhandler.New(remotes.FetchHandler(cache, fetcher), func(_ []byte) {}),
children,
}
if err := images.Dispatch(ctx, images.Handlers(handlers...), nil, desc); err != nil {
Expand Down
1 change: 1 addition & 0 deletions util/progress/logs/logs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pw.Write(identity.NewID(), client.VertexLog{
Stream: stderr,
Data: []byte(dt),
Expand Down
9 changes: 9 additions & 0 deletions util/resolver/retryhandler/retry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/containerd/containerd/images"
remoteserrors "github.com/containerd/containerd/remotes/errors"
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
)
Expand Down Expand Up @@ -48,6 +49,14 @@ func New(f images.HandlerFunc, logger func([]byte)) images.HandlerFunc {
}

func retryError(err error) bool {
// Retry on 5xx errors
var errUnexpectedStatus remoteserrors.ErrUnexpectedStatus
if errors.As(err, &errUnexpectedStatus) &&
errUnexpectedStatus.StatusCode >= 500 &&
errUnexpectedStatus.StatusCode <= 599 {
return true
}

if errors.Is(err, io.EOF) || errors.Is(err, syscall.ECONNRESET) || errors.Is(err, syscall.EPIPE) {
return true
}
Expand Down