fix: retry times error && add comments for clarity

Signed-off-by: Cheng-Yeh Chung <[email protected]>

Author: kenchung285

apiserversdk/proxy.go

@@ -34,7 +34,7 @@ func NewMux(config MuxConfig) (*http.ServeMux, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failed to get transport for config: %w", err)
 	}
-	proxy.Transport = newRetryRoundTripper(baseTransport, HTTPClientDefaultMaxRetry)
+	proxy.Transport = newRetryRoundTripper(baseTransport)
 	var handler http.Handler = proxy
 	if config.Middleware != nil {
 		handler = config.Middleware(proxy)
@@ -95,21 +95,26 @@ func requireKubeRayService(handler http.Handler, k8sClient *kubernetes.Clientset
 // retryRoundTripper is a custom implementation of http.RoundTripper that retries HTTP requests.
 // It verifies retryable HTTP status codes and retries using exponential backoff.
 type retryRoundTripper struct {
-	base    http.RoundTripper
-	retries int
+	base http.RoundTripper
+
+	// Num of retries after the initial attempt
+	maxRetries int
 }
 
-func newRetryRoundTripper(base http.RoundTripper, retries int) http.RoundTripper {
-	return &retryRoundTripper{base: base, retries: retries}
+func newRetryRoundTripper(base http.RoundTripper) http.RoundTripper {
+	return &retryRoundTripper{base: base, maxRetries: HTTPClientDefaultMaxRetry}
 }
 
 func (rrt *retryRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 	ctx := req.Context()
 
 	var resp *http.Response
 	var err error
-	for attempt := 0; attempt < rrt.retries; attempt++ {
+	for attempt := 0; attempt <= rrt.maxRetries; attempt++ {
+		/* Try up to (rrt.maxRetries + 1) times: initial attempt + retries */
+
 		if attempt == 0 && req.Body != nil && req.GetBody == nil {
+			/* Reuse request body in each attempt */
 			bodyBytes, err := io.ReadAll(req.Body)
 			if err != nil {
 				return nil, fmt.Errorf("failed to read request body for retry support: %w", err)
@@ -146,7 +151,8 @@ func (rrt *retryRoundTripper) RoundTrip(req *http.Request) (*http.Response, erro
 			return resp, nil
 		}
 
-		if attempt < rrt.retries-1 && resp.Body != nil {
+		if attempt < rrt.maxRetries && resp.Body != nil {
+			/* If not last attempt, drain response body */
 			if _, err = io.Copy(io.Discard, resp.Body); err != nil {
 				return nil, fmt.Errorf("retryRoundTripper internal failure to drain response body: %w", err)
 			}

apiserversdk/proxy_test.go

@@ -329,8 +329,6 @@ var _ = Describe("kuberay service", Ordered, func() {
 })
 
 var _ = Describe("retryRoundTripper", func() {
-	const maxAttemps = 5
-
 	It("should not retry on successful status OK", func() {
 		var attempts int32
 		mock := &mockRoundTripper{
@@ -342,7 +340,7 @@ var _ = Describe("retryRoundTripper", func() {
 				}, nil
 			},
 		}
-		retrier := newRetryRoundTripper(mock, maxAttemps /*retries*/)
+		retrier := newRetryRoundTripper(mock)
 		req, err := http.NewRequest(http.MethodGet, "http://test", nil)
 		Expect(err).ToNot(HaveOccurred())
 		resp, err := retrier.RoundTrip(req)
@@ -352,12 +350,12 @@ var _ = Describe("retryRoundTripper", func() {
 	})
 
 	It("should retry failed requests and eventually succeed", func() {
-		const maxFailure = 3
+		const maxFailure = 2
 		var attempts int32
 		mock := &mockRoundTripper{
 			fn: func(_ *http.Request) (*http.Response, error) {
 				count := atomic.AddInt32(&attempts, 1)
-				if count < maxFailure {
+				if count <= maxFailure {
 					return &http.Response{
 						StatusCode: http.StatusInternalServerError,
 						Body:       io.NopCloser(strings.NewReader("internal error")),
@@ -369,13 +367,13 @@ var _ = Describe("retryRoundTripper", func() {
 				}, nil
 			},
 		}
-		retrier := newRetryRoundTripper(mock, maxAttemps /*retries*/)
+		retrier := newRetryRoundTripper(mock)
 		req, err := http.NewRequest(http.MethodGet, "http://test", nil)
 		Expect(err).ToNot(HaveOccurred())
 		resp, err := retrier.RoundTrip(req)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(resp.StatusCode).To(Equal(http.StatusOK))
-		Expect(attempts).To(Equal(int32(maxFailure)))
+		Expect(attempts).To(Equal(int32(maxFailure + 1)))
 	})
 
 	It("Retries exceed maximum retry counts", func() {
@@ -389,13 +387,13 @@ var _ = Describe("retryRoundTripper", func() {
 				}, nil
 			},
 		}
-		retrier := newRetryRoundTripper(mock, maxAttemps /*retries*/)
+		retrier := newRetryRoundTripper(mock)
 		req, err := http.NewRequest(http.MethodGet, "http://test", nil)
 		Expect(err).ToNot(HaveOccurred())
 		resp, err := retrier.RoundTrip(req)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(resp.StatusCode).To(Equal(http.StatusInternalServerError))
-		Expect(attempts).To(Equal(int32(maxAttemps)))
+		Expect(attempts).To(Equal(int32(HTTPClientDefaultMaxRetry + 1)))
 	})
 
 	It("should not retry on non-retriable status", func() {
@@ -409,7 +407,7 @@ var _ = Describe("retryRoundTripper", func() {
 				}, nil
 			},
 		}
-		retrier := newRetryRoundTripper(mock, maxAttemps /*retries*/)
+		retrier := newRetryRoundTripper(mock)
 		req, err := http.NewRequest(http.MethodGet, "http://test", nil)
 		Expect(err).ToNot(HaveOccurred())
 		resp, err := retrier.RoundTrip(req)
@@ -428,7 +426,7 @@ var _ = Describe("retryRoundTripper", func() {
 				}, nil
 			},
 		}
-		retrier := newRetryRoundTripper(mock, maxAttemps /*retries*/)
+		retrier := newRetryRoundTripper(mock)
 		ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
 		defer cancel()
 		req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://test", nil)