client-go: refactor retry logic for backoff, rate limiter and metric

Kubernetes-commit: cecc563d3b9a9438cd3e6ae1576baa0a36f2d843

rest/request_test.go

@@ -2584,6 +2584,34 @@ func TestRequestWatchRetryWithRateLimiterBackoffAndMetrics(t *testing.T) {
 	})
 }
 
+func TestRequestDoWithRetryInvokeOrder(t *testing.T) {
+	// both request.Do and request.DoRaw have the same behavior and expectations
+	testWithRetryInvokeOrder(t, "Do", func(ctx context.Context, r *Request) {
+		r.DoRaw(ctx)
+	})
+}
+
+func TestRequestStreamWithRetryInvokeOrder(t *testing.T) {
+	testWithRetryInvokeOrder(t, "Stream", func(ctx context.Context, r *Request) {
+		r.Stream(ctx)
+	})
+}
+
+func TestRequestWatchWithRetryInvokeOrder(t *testing.T) {
+	testWithRetryInvokeOrder(t, "Watch", func(ctx context.Context, r *Request) {
+		w, err := r.Watch(ctx)
+		if err == nil {
+			// in this test the the response body returned by the server is always empty,
+			// this will cause StreamWatcher.receive() to:
+			// - return an io.EOF to indicate that the watch closed normally and
+			// - then close the io.Reader
+			// since we assert on the number of times 'Close' has been called on the
+			// body of the response object, we need to wait here to avoid race condition.
+			<-w.ResultChan()
+		}
+	})
+}
+
 func testRequestWithRetry(t *testing.T, key string, doFunc func(ctx context.Context, r *Request)) {
 	type expected struct {
 		attempts  int
@@ -2968,6 +2996,134 @@ func testRetryWithRateLimiterBackoffAndMetrics(t *testing.T, key string, doFunc
 	}
 }
 
+type retryInterceptor struct {
+	WithRetry
+	invokeOrderGot []string
+}
+
+func (ri *retryInterceptor) IsNextRetry(ctx context.Context, restReq *Request, httpReq *http.Request, resp *http.Response, err error, f IsRetryableErrorFunc) bool {
+	ri.invokeOrderGot = append(ri.invokeOrderGot, "WithRetry.IsNextRetry")
+	return ri.WithRetry.IsNextRetry(ctx, restReq, httpReq, resp, err, f)
+}
+
+func (ri *retryInterceptor) Before(ctx context.Context, request *Request) error {
+	ri.invokeOrderGot = append(ri.invokeOrderGot, "WithRetry.Before")
+	return ri.WithRetry.Before(ctx, request)
+}
+
+func (ri *retryInterceptor) After(ctx context.Context, request *Request, resp *http.Response, err error) {
+	ri.invokeOrderGot = append(ri.invokeOrderGot, "WithRetry.After")
+	ri.WithRetry.After(ctx, request, resp, err)
+}
+
+func (ri *retryInterceptor) Do() {
+	ri.invokeOrderGot = append(ri.invokeOrderGot, "Client.Do")
+}
+
+func testWithRetryInvokeOrder(t *testing.T, key string, doFunc func(ctx context.Context, r *Request)) {
+	// we define the expected order of how the client
+	// should invoke the retry interface
+	// scenario:
+	//  - A: original request fails with a retryable response: (500, 'Retry-After: 1')
+	//  - B: retry 1: successful with a status code 200
+	// so we have a total of 2 attempts
+	defaultInvokeOrderWant := []string{
+		// first attempt (A)
+		"WithRetry.Before",
+		"Client.Do",
+		"WithRetry.After",
+		// server returns a retryable response: (500, 'Retry-After: 1')
+		// IsNextRetry is expected to return true
+		"WithRetry.IsNextRetry",
+
+		// second attempt (B) - retry 1: successful with a status code 200
+		"WithRetry.Before",
+		"Client.Do",
+		"WithRetry.After",
+		// success: IsNextRetry is expected to return false
+		// Watch and Stream are an exception, they return as soon as the
+		// server sends a status code of success.
+		"WithRetry.IsNextRetry",
+	}
+
+	tests := []struct {
+		name          string
+		maxRetries    int
+		serverReturns []responseErr
+		// expectations differ based on whether it is 'Watch', 'Stream' or 'Do'
+		expectations map[string][]string
+	}{
+		{
+			name:       "success after one retry",
+			maxRetries: 1,
+			serverReturns: []responseErr{
+				{response: retryAfterResponse(), err: nil},
+				{response: &http.Response{StatusCode: http.StatusOK}, err: nil},
+			},
+			expectations: map[string][]string{
+				"Do": defaultInvokeOrderWant,
+				// Watch and Stream skip the final 'IsNextRetry' by returning
+				// as soon as they see a success from the server.
+				"Watch":  defaultInvokeOrderWant[0 : len(defaultInvokeOrderWant)-1],
+				"Stream": defaultInvokeOrderWant[0 : len(defaultInvokeOrderWant)-1],
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			interceptor := &retryInterceptor{
+				WithRetry: &withRetry{maxRetries: test.maxRetries},
+			}
+
+			var attempts int
+			client := clientForFunc(func(req *http.Request) (*http.Response, error) {
+				defer func() {
+					attempts++
+				}()
+
+				interceptor.Do()
+				resp := test.serverReturns[attempts].response
+				if resp != nil {
+					resp.Body = ioutil.NopCloser(bytes.NewReader([]byte{}))
+				}
+				return resp, test.serverReturns[attempts].err
+			})
+
+			base, err := url.Parse("http://foo.bar")
+			if err != nil {
+				t.Fatalf("Wrong test setup - did not find expected for: %s", key)
+			}
+			req := &Request{
+				verb: "GET",
+				body: bytes.NewReader([]byte{}),
+				c: &RESTClient{
+					base:    base,
+					content: defaultContentConfig(),
+					Client:  client,
+				},
+				pathPrefix:  "/api/v1",
+				rateLimiter: flowcontrol.NewFakeAlwaysRateLimiter(),
+				backoff:     &NoBackoff{},
+				retry:       interceptor,
+			}
+
+			doFunc(context.Background(), req)
+
+			if attempts != 2 {
+				t.Errorf("%s: Expected attempts: %d, but got: %d", key, 2, attempts)
+			}
+			invokeOrderWant, ok := test.expectations[key]
+			if !ok {
+				t.Fatalf("Wrong test setup - did not find expected for: %s", key)
+			}
+			if !cmp.Equal(invokeOrderWant, interceptor.invokeOrderGot) {
+				t.Errorf("%s: Expected invoke order to match, diff: %s", key, cmp.Diff(invokeOrderWant, interceptor.invokeOrderGot))
+			}
+		})
+	}
+}
+
 func TestReuseRequest(t *testing.T) {
 	var tests = []struct {
 		name        string