Merge pull request #1681 from p0lyn0mial/downstream-etcd-retry-shim-m…

…ore-tests OCPBUGS-18149: UPSTREAM: <carry>: retry etcd Unavailable errors
openshift · Aug 30, 2023 · 2c83a9f · 2c83a9f
2 parents 533bc29 + 01e0384
commit 2c83a9f
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 2 deletions.
diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/etcd3retry/retry_etcdclient.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/etcd3retry/retry_etcdclient.go
@@ -151,11 +151,11 @@ func onError(ctx context.Context, backoff wait.Backoff, retriable func(error) (s
 	var lastErr error
 	var lastErrLabel string
 	var retry bool
+	var retryCounter int
 	err := backoffWithRequestContext(ctx, backoff, func() (bool, error) {
 		err := fn()
 		if retry {
-			// TODO: reduce verbosity once debugging in ci (upgrade tests) is done.
-			klog.V(1).Infof("etcd retry - lastErrLabel: %s error:%v", lastErrLabel, err)
+			klog.V(1).Infof("etcd retry - counter: %v, lastErrLabel: %s lastError: %v, error: %v", retryCounter, lastErrLabel, lastErr, err)
 			metrics.UpdateEtcdRequestRetry(lastErrLabel)
 		}
 		if err == nil {
@@ -165,6 +165,7 @@ func onError(ctx context.Context, backoff wait.Backoff, retriable func(error) (s
 		lastErrLabel, retry = retriable(err)
 		if retry {
 			lastErr = err
+			retryCounter++
 			return false, nil
 		}
 

diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/etcd3retry/retry_etcdclient_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/etcd3retry/retry_etcdclient_test.go
@@ -1,6 +1,7 @@
 package etcd3retry
 
 import (
+	"context"
 	"fmt"
 	"net"
 	"net/url"
@@ -12,6 +13,64 @@ import (
 	"k8s.io/apiserver/pkg/storage"
 )
 
+func TestOnError(t *testing.T) {
+	tests := []struct {
+		name               string
+		returnedFnError    func(retryCounter int) error
+		expectedRetries    int
+		expectedFinalError error
+	}{
+		{
+			name:               "retry ErrLeaderChanged",
+			returnedFnError:    func(_ int) error { return etcdrpc.ErrLeaderChanged },
+			expectedRetries:    5,
+			expectedFinalError: etcdrpc.ErrLeaderChanged,
+		},
+		{
+			name: "retry ErrLeaderChanged a few times",
+			returnedFnError: func(retryCounter int) error {
+				if retryCounter == 3 {
+					return nil
+				}
+				return etcdrpc.ErrLeaderChanged
+			},
+			expectedRetries: 3,
+		},
+		{
+			name:            "no retries",
+			returnedFnError: func(_ int) error { return nil },
+		},
+		{
+			name:               "no retries for a random error",
+			returnedFnError:    func(_ int) error { return fmt.Errorf("random error") },
+			expectedFinalError: fmt.Errorf("random error"),
+		},
+	}
+
+	for _, scenario := range tests {
+		t.Run(scenario.name, func(t *testing.T) {
+			ctx := context.TODO()
+			// we set it to -1 to indicate that the first
+			// execution is not a retry
+			actualRetries := -1
+			err := onError(ctx, defaultRetry, isRetriableEtcdError, func() error {
+				actualRetries++
+				return scenario.returnedFnError(actualRetries)
+			})
+
+			if actualRetries != scenario.expectedRetries {
+				t.Errorf("Unexpected number of retries %v, expected %v", actualRetries, scenario.expectedRetries)
+			}
+			if (err == nil && scenario.expectedFinalError != nil) || (err != nil && scenario.expectedFinalError == nil) {
+				t.Errorf("Expected error %v, got %v", scenario.expectedFinalError, err)
+			}
+			if err != nil && scenario.expectedFinalError != nil && err.Error() != scenario.expectedFinalError.Error() {
+				t.Errorf("Expected error %v, got %v", scenario.expectedFinalError, err)
+			}
+		})
+	}
+}
+
 func TestIsRetriableEtcdError(t *testing.T) {
 	tests := []struct {
 		name               string