Browse code

Revert reporting of multiple pull errors

Revert the portions of #17617 that report all errors when a pull
falls back, and go back to just reporting the last error. This was nice
to have, but causes some UX issues because nonexistent images show
additional "unauthorized" errors.

Keep the part of the PR that handled ENOSPC, as this appears to work
even without tracking multiple errors.

Fixes #19419

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

Aaron Lehmann authored on 2016/01/19 07:17:39
Showing 3 changed files
... ...
@@ -3,7 +3,6 @@ package distribution
3 3
 import (
4 4
 	"fmt"
5 5
 	"os"
6
-	"strings"
7 6
 
8 7
 	"github.com/Sirupsen/logrus"
9 8
 	"github.com/docker/docker/api"
... ...
@@ -97,13 +96,12 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo
97 97
 	}
98 98
 
99 99
 	var (
100
-		// use a slice to append the error strings and return a joined string to caller
101
-		errors []string
100
+		lastErr error
102 101
 
103 102
 		// discardNoSupportErrors is used to track whether an endpoint encountered an error of type registry.ErrNoSupport
104
-		// By default it is false, which means that if a ErrNoSupport error is encountered, it will be saved in errors.
103
+		// By default it is false, which means that if a ErrNoSupport error is encountered, it will be saved in lastErr.
105 104
 		// As soon as another kind of error is encountered, discardNoSupportErrors is set to true, avoiding the saving of
106
-		// any subsequent ErrNoSupport errors in errors.
105
+		// any subsequent ErrNoSupport errors in lastErr.
107 106
 		// It's needed for pull-by-digest on v1 endpoints: if there are only v1 endpoints configured, the error should be
108 107
 		// returned and displayed, but if there was a v2 endpoint which supports pull-by-digest, then the last relevant
109 108
 		// error is the ones from v2 endpoints not v1.
... ...
@@ -123,7 +121,7 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo
123 123
 
124 124
 		puller, err := newPuller(endpoint, repoInfo, imagePullConfig)
125 125
 		if err != nil {
126
-			errors = append(errors, err.Error())
126
+			lastErr = err
127 127
 			continue
128 128
 		}
129 129
 		if err := puller.Pull(ctx, ref); err != nil {
... ...
@@ -144,34 +142,28 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo
144 144
 					// Because we found an error that's not ErrNoSupport, discard all subsequent ErrNoSupport errors.
145 145
 					discardNoSupportErrors = true
146 146
 					// append subsequent errors
147
-					errors = append(errors, err.Error())
147
+					lastErr = err
148 148
 				} else if !discardNoSupportErrors {
149 149
 					// Save the ErrNoSupport error, because it's either the first error or all encountered errors
150 150
 					// were also ErrNoSupport errors.
151 151
 					// append subsequent errors
152
-					errors = append(errors, err.Error())
152
+					lastErr = err
153 153
 				}
154 154
 				continue
155 155
 			}
156
-			errors = append(errors, err.Error())
157
-			logrus.Debugf("Not continuing with error: %v", fmt.Errorf(strings.Join(errors, "\n")))
158
-			if len(errors) > 0 {
159
-				return fmt.Errorf(strings.Join(errors, "\n"))
160
-			}
156
+			logrus.Debugf("Not continuing with error: %v", err)
157
+			return err
161 158
 		}
162 159
 
163 160
 		imagePullConfig.ImageEventLogger(ref.String(), repoInfo.Name(), "pull")
164 161
 		return nil
165 162
 	}
166 163
 
167
-	if len(errors) == 0 {
168
-		return fmt.Errorf("no endpoints found for %s", ref.String())
164
+	if lastErr == nil {
165
+		lastErr = fmt.Errorf("no endpoints found for %s", ref.String())
169 166
 	}
170 167
 
171
-	if len(errors) > 0 {
172
-		return fmt.Errorf(strings.Join(errors, "\n"))
173
-	}
174
-	return nil
168
+	return lastErr
175 169
 }
176 170
 
177 171
 // writeStatus writes a status message to out. If layersDownloaded is true, the
... ...
@@ -279,18 +279,6 @@ func (s *DockerSchema1RegistrySuite) TestPullIDStability(c *check.C) {
279 279
 	testPullIDStability(c)
280 280
 }
281 281
 
282
-// TestPullFallbackOn404 tries to pull a nonexistent manifest and confirms that
283
-// the pull falls back to the v1 protocol.
284
-//
285
-// Ref: docker/docker#18832
286
-func (s *DockerRegistrySuite) TestPullFallbackOn404(c *check.C) {
287
-	repoName := fmt.Sprintf("%v/does/not/exist", privateRegistryURL)
288
-
289
-	out, _, _ := dockerCmdWithError("pull", repoName)
290
-
291
-	c.Assert(out, checker.Contains, "v1 ping attempt")
292
-}
293
-
294 282
 func (s *DockerRegistrySuite) TestPullManifestList(c *check.C) {
295 283
 	pushDigest, err := setupImage(c)
296 284
 	c.Assert(err, checker.IsNil, check.Commentf("error setting up image"))
... ...
@@ -62,6 +62,7 @@ func (s *DockerHubPullSuite) TestPullNonExistingImage(c *check.C) {
62 62
 			out, err := s.CmdWithError("pull", "-a", e.Alias)
63 63
 			c.Assert(err, checker.NotNil, check.Commentf("expected non-zero exit status when pulling non-existing image: %s", out))
64 64
 			c.Assert(out, checker.Contains, fmt.Sprintf("Error: image %s not found", e.Repo), check.Commentf("expected image not found error messages"))
65
+			c.Assert(out, checker.Not(checker.Contains), "unauthorized", check.Commentf(`message should not contain "unauthorized"`))
65 66
 		}
66 67
 	}
67 68