Handle RequestTimeout and TooManyRequests

knm3000 · knm3000 · commit 3d3dbc8fbfac · 2019-07-01T19:19:40.000+03:00
Retry 5 times in case of StatusRequestTimeout StatusTooManyRequests This fixes the issue #2680 "Make content fetch retry more robust" Signed-off-by: Konstantin Maksimov <kmaksimov@gmail.com>
diff --git a/remotes/docker/fetcher_test.go b/remotes/docker/fetcher_test.go
@@ -109,6 +109,7 @@ func TestDockerFetcherOpen(t *testing.T) {
 		wantErr                bool
 		wantServerMessageError bool
 		wantPlainError         bool
+		retries                int
 	}{
 		{
 			name:         "should return status and error.message if it exists if the registry request fails",
@@ -128,12 +129,31 @@ func TestDockerFetcherOpen(t *testing.T) {
 			want:           nil,
 			wantErr:        true,
 			wantPlainError: true,
+		}, {
+			name:           "should return StatusRequestTimeout after 5 retries",
+			mockedStatus:   http.StatusRequestTimeout,
+			mockedErr:      fmt.Errorf(http.StatusText(http.StatusRequestTimeout)),
+			want:           nil,
+			wantErr:        true,
+			wantPlainError: true,
+			retries:        5,
+		}, {
+			name:           "should return StatusTooManyRequests after 5 retries",
+			mockedStatus:   http.StatusTooManyRequests,
+			mockedErr:      fmt.Errorf(http.StatusText(http.StatusTooManyRequests)),
+			want:           nil,
+			wantErr:        true,
+			wantPlainError: true,
+			retries:        5,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 
 			s := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+				if tt.retries > 0 {
+					tt.retries--
+				}
 				rw.WriteHeader(tt.mockedStatus)
 				bytes, _ := json.Marshal(tt.mockedErr)
 				rw.Write(bytes)
@@ -147,6 +167,7 @@ func TestDockerFetcherOpen(t *testing.T) {
 			got, err := r.open(context.TODO(), s.URL, "", 0)
 			assert.Equal(t, tt.wantErr, (err != nil))
 			assert.Equal(t, tt.want, got)
+			assert.Equal(t, tt.retries, 0)
 			if tt.wantErr {
 				var expectedError error
 				if tt.wantServerMessageError {
diff --git a/remotes/docker/resolver.go b/remotes/docker/resolver.go
@@ -455,7 +455,8 @@ func (r *dockerBase) retryRequest(ctx context.Context, req *http.Request, respon
 		return nil, nil
 	}
 	last := responses[len(responses)-1]
-	if last.StatusCode == http.StatusUnauthorized {
+	switch last.StatusCode {
+	case http.StatusUnauthorized:
 		log.G(ctx).WithField("header", last.Header.Get("WWW-Authenticate")).Debug("Unauthorized")
 		if r.auth != nil {
 			if err := r.auth.AddResponses(ctx, responses); err == nil {
@@ -464,16 +465,17 @@ func (r *dockerBase) retryRequest(ctx context.Context, req *http.Request, respon
 				return nil, err
 			}
 		}
-
 		return nil, nil
-	} else if last.StatusCode == http.StatusMethodNotAllowed && req.Method == http.MethodHead {
+	case http.StatusMethodNotAllowed:
 		// Support registries which have not properly implemented the HEAD method for
 		// manifests endpoint
-		if strings.Contains(req.URL.Path, "/manifests/") {
+		if req.Method == http.MethodHead && strings.Contains(req.URL.Path, "/manifests/") {
 			// TODO: copy request?
 			req.Method = http.MethodGet
 			return copyRequest(req)
 		}
+	case http.StatusRequestTimeout, http.StatusTooManyRequests:
+		return copyRequest(req)
 	}
 
 	// TODO: Handle 50x errors accounting for attempt history

Original file line number	Diff line number	Diff line change
`@@ -455,7 +455,8 @@ func (r dockerBase) retryRequest(ctx context.Context, req http.Request, respon`
`455`	`455`	`return nil, nil`
`456`	`456`	`}`
`457`	`457`	`last := responses[len(responses)-1]`
`458`		`- if last.StatusCode == http.StatusUnauthorized {`
	`458`	`+ switch last.StatusCode {`
	`459`	`+ case http.StatusUnauthorized:`
`459`	`460`	`log.G(ctx).WithField("header", last.Header.Get("WWW-Authenticate")).Debug("Unauthorized")`
`460`	`461`	`if r.auth != nil {`
`461`	`462`	`if err := r.auth.AddResponses(ctx, responses); err == nil {`
`@@ -464,16 +465,17 @@ func (r dockerBase) retryRequest(ctx context.Context, req http.Request, respon`
`464`	`465`	`return nil, err`
`465`	`466`	`}`
`466`	`467`	`}`
`467`		`-`
`468`	`468`	`return nil, nil`
`469`		`- } else if last.StatusCode == http.StatusMethodNotAllowed && req.Method == http.MethodHead {`
	`469`	`+ case http.StatusMethodNotAllowed:`
`470`	`470`	`// Support registries which have not properly implemented the HEAD method for`
`471`	`471`	`// manifests endpoint`
`472`		`- if strings.Contains(req.URL.Path, "/manifests/") {`
	`472`	`+ if req.Method == http.MethodHead && strings.Contains(req.URL.Path, "/manifests/") {`
`473`	`473`	`// TODO: copy request?`
`474`	`474`	`req.Method = http.MethodGet`
`475`	`475`	`return copyRequest(req)`
`476`	`476`	`}`
	`477`	`+ case http.StatusRequestTimeout, http.StatusTooManyRequests:`
	`478`	`+ return copyRequest(req)`
`477`	`479`	`}`
`478`	`480`
`479`	`481`	`// TODO: Handle 50x errors accounting for attempt history`