Push/pull errors improvement and cleanup

Several improvements to error handling:

- Introduce ImageConfigPullError type, wrapping errors related to
  downloading the image configuration blob in schema2. This allows for a
  more descriptive error message to be seen by the end user.

- Change some logrus.Debugf calls that display errors to logrus.Errorf.
  Add log lines in the push/pull fallback cases to make sure the errors
  leading to the fallback are shown.

- Move error-related types and functions which are only used by the
  distribution package out of the registry package.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

distribution/errors.go

@@ -0,0 +1,102 @@
+package distribution
+
+import (
+	"net/url"
+	"strings"
+	"syscall"
+
+	"github.com/docker/distribution/registry/api/errcode"
+	"github.com/docker/distribution/registry/api/v2"
+	"github.com/docker/distribution/registry/client"
+	"github.com/docker/docker/distribution/xfer"
+)
+
+// ErrNoSupport is an error type used for errors indicating that an operation
+// is not supported. It encapsulates a more specific error.
+type ErrNoSupport struct{ Err error }
+
+func (e ErrNoSupport) Error() string {
+	if e.Err == nil {
+		return "not supported"
+	}
+	return e.Err.Error()
+}
+
+// fallbackError wraps an error that can possibly allow fallback to a different
+// endpoint.
+type fallbackError struct {
+	// err is the error being wrapped.
+	err error
+	// confirmedV2 is set to true if it was confirmed that the registry
+	// supports the v2 protocol. This is used to limit fallbacks to the v1
+	// protocol.
+	confirmedV2 bool
+}
+
+// Error renders the FallbackError as a string.
+func (f fallbackError) Error() string {
+	return f.err.Error()
+}
+
+// shouldV2Fallback returns true if this error is a reason to fall back to v1.
+func shouldV2Fallback(err errcode.Error) bool {
+	switch err.Code {
+	case errcode.ErrorCodeUnauthorized, v2.ErrorCodeManifestUnknown, v2.ErrorCodeNameUnknown:
+		return true
+	}
+	return false
+}
+
+// continueOnError returns true if we should fallback to the next endpoint
+// as a result of this error.
+func continueOnError(err error) bool {
+	switch v := err.(type) {
+	case errcode.Errors:
+		if len(v) == 0 {
+			return true
+		}
+		return continueOnError(v[0])
+	case ErrNoSupport:
+		return continueOnError(v.Err)
+	case errcode.Error:
+		return shouldV2Fallback(v)
+	case *client.UnexpectedHTTPResponseError:
+		return true
+	case ImageConfigPullError:
+		return false
+	case error:
+		return !strings.Contains(err.Error(), strings.ToLower(syscall.ENOSPC.Error()))
+	}
+	// let's be nice and fallback if the error is a completely
+	// unexpected one.
+	// If new errors have to be handled in some way, please
+	// add them to the switch above.
+	return true
+}
+
+// retryOnError wraps the error in xfer.DoNotRetry if we should not retry the
+// operation after this error.
+func retryOnError(err error) error {
+	switch v := err.(type) {
+	case errcode.Errors:
+		return retryOnError(v[0])
+	case errcode.Error:
+		switch v.Code {
+		case errcode.ErrorCodeUnauthorized, errcode.ErrorCodeUnsupported, errcode.ErrorCodeDenied:
+			return xfer.DoNotRetry{Err: err}
+		}
+	case *url.Error:
+		return retryOnError(v.Err)
+	case *client.UnexpectedHTTPResponseError:
+		return xfer.DoNotRetry{Err: err}
+	case error:
+		if strings.Contains(err.Error(), strings.ToLower(syscall.ENOSPC.Error())) {
+			return xfer.DoNotRetry{Err: err}
+		}
+	}
+	// let's be nice and fallback if the error is a completely
+	// unexpected one.
+	// If new errors have to be handled in some way, please
+	// add them to the switch above.
+	return err
+}

distribution/pull.go

@@ -136,7 +136,7 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo
 				}
 			}
 			if fallback {
-				if _, ok := err.(registry.ErrNoSupport); !ok {
+				if _, ok := err.(ErrNoSupport); !ok {
 					// Because we found an error that's not ErrNoSupport, discard all subsequent ErrNoSupport errors.
 					discardNoSupportErrors = true
 					// append subsequent errors
@@ -147,9 +147,10 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo
 					// append subsequent errors
 					lastErr = err
 				}
+				logrus.Errorf("Attempting next endpoint for pull after error: %v", err)
 				continue
 			}
-			logrus.Debugf("Not continuing with error: %v", err)
+			logrus.Errorf("Not continuing with pull after error: %v", err)
 			return err
 		}
 

distribution/pull_v1.go

@@ -38,7 +38,7 @@ type v1Puller struct {
 func (p *v1Puller) Pull(ctx context.Context, ref reference.Named) error {
 	if _, isCanonical := ref.(reference.Canonical); isCanonical {
 		// Allowing fallback, because HTTPS v1 is before HTTP v2
-		return fallbackError{err: registry.ErrNoSupport{Err: errors.New("Cannot pull by digest with v1 registry")}}
+		return fallbackError{err: ErrNoSupport{Err: errors.New("Cannot pull by digest with v1 registry")}}
 	}
 
 	tlsConfig, err := p.config.RegistryService.TLSConfig(p.repoInfo.Index.Name)

distribution/pull_v2.go

@@ -35,6 +35,17 @@ import (
 
 var errRootFSMismatch = errors.New("layers from manifest don't match image configuration")
 
+// ImageConfigPullError is an error pulling the image config blob
+// (only applies to schema2).
+type ImageConfigPullError struct {
+	Err error
+}
+
+// Error returns the error string for ImageConfigPullError.
+func (e ImageConfigPullError) Error() string {
+	return "error pulling image configuration: " + e.Err.Error()
+}
+
 type v2Puller struct {
 	V2MetadataService *metadata.V2MetadataService
 	endpoint          registry.APIEndpoint
@@ -58,8 +69,8 @@ func (p *v2Puller) Pull(ctx context.Context, ref reference.Named) (err error) {
 		if _, ok := err.(fallbackError); ok {
 			return err
 		}
-		if registry.ContinueOnError(err) {
-			logrus.Debugf("Error trying v2 registry: %v", err)
+		if continueOnError(err) {
+			logrus.Errorf("Error trying v2 registry: %v", err)
 			return fallbackError{err: err, confirmedV2: p.confirmedV2}
 		}
 	}
@@ -170,7 +181,7 @@ func (ld *v2LayerDescriptor) Download(ctx context.Context, progressOutput progre
 
 	layerDownload, err := blobs.Open(ctx, ld.digest)
 	if err != nil {
-		logrus.Debugf("Error initiating layer download: %v", err)
+		logrus.Errorf("Error initiating layer download: %v", err)
 		if err == distribution.ErrBlobUnknown {
 			return nil, 0, xfer.DoNotRetry{Err: err}
 		}
@@ -280,12 +291,12 @@ func (ld *v2LayerDescriptor) truncateDownloadFile() error {
 	ld.verifier = nil
 
 	if _, err := ld.tmpFile.Seek(0, os.SEEK_SET); err != nil {
-		logrus.Debugf("error seeking to beginning of download file: %v", err)
+		logrus.Errorf("error seeking to beginning of download file: %v", err)
 		return err
 	}
 
 	if err := ld.tmpFile.Truncate(0); err != nil {
-		logrus.Debugf("error truncating download file: %v", err)
+		logrus.Errorf("error truncating download file: %v", err)
 		return err
 	}
 
@@ -484,7 +495,7 @@ func (p *v2Puller) pullSchema2(ctx context.Context, ref reference.Named, mfst *s
 	go func() {
 		configJSON, err := p.pullSchema2ImageConfig(ctx, target.Digest)
 		if err != nil {
-			errChan <- err
+			errChan <- ImageConfigPullError{Err: err}
 			cancel()
 			return
 		}
@@ -704,12 +715,12 @@ func allowV1Fallback(err error) error {
 	switch v := err.(type) {
 	case errcode.Errors:
 		if len(v) != 0 {
-			if v0, ok := v[0].(errcode.Error); ok && registry.ShouldV2Fallback(v0) {
+			if v0, ok := v[0].(errcode.Error); ok && shouldV2Fallback(v0) {
 				return fallbackError{err: err, confirmedV2: false}
 			}
 		}
 	case errcode.Error:
-		if registry.ShouldV2Fallback(v) {
+		if shouldV2Fallback(v) {
 			return fallbackError{err: err, confirmedV2: false}
 		}
 	case *url.Error:

distribution/push.go

@@ -144,11 +144,12 @@ func Push(ctx context.Context, ref reference.Named, imagePushConfig *ImagePushCo
 					confirmedV2 = confirmedV2 || fallbackErr.confirmedV2
 					err = fallbackErr.err
 					lastErr = err
+					logrus.Errorf("Attempting next endpoint for push after error: %v", err)
 					continue
 				}
 			}
 
-			logrus.Debugf("Not continuing with error: %v", err)
+			logrus.Errorf("Not continuing with push after error: %v", err)
 			return err
 		}
 

distribution/push_v2.go

@@ -68,7 +68,7 @@ func (p *v2Pusher) Push(ctx context.Context) (err error) {
 	}
 
 	if err = p.pushV2Repository(ctx); err != nil {
-		if registry.ContinueOnError(err) {
+		if continueOnError(err) {
 			return fallbackError{err: err, confirmedV2: p.pushState.confirmedV2}
 		}
 	}

distribution/registry.go

@@ -6,38 +6,19 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
-	"syscall"
 	"time"
 
 	"github.com/docker/distribution"
 	distreference "github.com/docker/distribution/reference"
-	"github.com/docker/distribution/registry/api/errcode"
 	"github.com/docker/distribution/registry/client"
 	"github.com/docker/distribution/registry/client/auth"
 	"github.com/docker/distribution/registry/client/transport"
-	"github.com/docker/docker/distribution/xfer"
 	"github.com/docker/docker/dockerversion"
 	"github.com/docker/docker/registry"
 	"github.com/docker/engine-api/types"
 	"golang.org/x/net/context"
 )
 
-// fallbackError wraps an error that can possibly allow fallback to a different
-// endpoint.
-type fallbackError struct {
-	// err is the error being wrapped.
-	err error
-	// confirmedV2 is set to true if it was confirmed that the registry
-	// supports the v2 protocol. This is used to limit fallbacks to the v1
-	// protocol.
-	confirmedV2 bool
-}
-
-// Error renders the FallbackError as a string.
-func (f fallbackError) Error() string {
-	return f.err.Error()
-}
-
 type dumbCredentialStore struct {
 	auth *types.AuthConfig
 }
@@ -141,30 +122,3 @@ func (th *existingTokenHandler) AuthorizeRequest(req *http.Request, params map[s
 	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", th.token))
 	return nil
 }
-
-// retryOnError wraps the error in xfer.DoNotRetry if we should not retry the
-// operation after this error.
-func retryOnError(err error) error {
-	switch v := err.(type) {
-	case errcode.Errors:
-		return retryOnError(v[0])
-	case errcode.Error:
-		switch v.Code {
-		case errcode.ErrorCodeUnauthorized, errcode.ErrorCodeUnsupported, errcode.ErrorCodeDenied:
-			return xfer.DoNotRetry{Err: err}
-		}
-	case *url.Error:
-		return retryOnError(v.Err)
-	case *client.UnexpectedHTTPResponseError:
-		return xfer.DoNotRetry{Err: err}
-	case error:
-		if strings.Contains(err.Error(), strings.ToLower(syscall.ENOSPC.Error())) {
-			return xfer.DoNotRetry{Err: err}
-		}
-	}
-	// let's be nice and fallback if the error is a completely
-	// unexpected one.
-	// If new errors have to be handled in some way, please
-	// add them to the switch above.
-	return err
-}

registry/registry.go

@@ -13,13 +13,9 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
-	"syscall"
 	"time"
 
 	"github.com/Sirupsen/logrus"
-	"github.com/docker/distribution/registry/api/errcode"
-	"github.com/docker/distribution/registry/api/v2"
-	"github.com/docker/distribution/registry/client"
 	"github.com/docker/distribution/registry/client/transport"
 	"github.com/docker/go-connections/tlsconfig"
 )
@@ -169,51 +165,6 @@ func addRequiredHeadersToRedirectedRequests(req *http.Request, via []*http.Reque
 	return nil
 }
 
-// ShouldV2Fallback returns true if this error is a reason to fall back to v1.
-func ShouldV2Fallback(err errcode.Error) bool {
-	switch err.Code {
-	case errcode.ErrorCodeUnauthorized, v2.ErrorCodeManifestUnknown, v2.ErrorCodeNameUnknown:
-		return true
-	}
-	return false
-}
-
-// ErrNoSupport is an error type used for errors indicating that an operation
-// is not supported. It encapsulates a more specific error.
-type ErrNoSupport struct{ Err error }
-
-func (e ErrNoSupport) Error() string {
-	if e.Err == nil {
-		return "not supported"
-	}
-	return e.Err.Error()
-}
-
-// ContinueOnError returns true if we should fallback to the next endpoint
-// as a result of this error.
-func ContinueOnError(err error) bool {
-	switch v := err.(type) {
-	case errcode.Errors:
-		if len(v) == 0 {
-			return true
-		}
-		return ContinueOnError(v[0])
-	case ErrNoSupport:
-		return ContinueOnError(v.Err)
-	case errcode.Error:
-		return ShouldV2Fallback(v)
-	case *client.UnexpectedHTTPResponseError:
-		return true
-	case error:
-		return !strings.Contains(err.Error(), strings.ToLower(syscall.ENOSPC.Error()))
-	}
-	// let's be nice and fallback if the error is a completely
-	// unexpected one.
-	// If new errors have to be handled in some way, please
-	// add them to the switch above.
-	return true
-}
-
 // NewTransport returns a new HTTP transport. If tlsConfig is nil, it uses the
 // default TLS configuration.
 func NewTransport(tlsConfig *tls.Config) *http.Transport {