From 06ef849daa69128c5b703a933cc533d8e0256b16 Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Mon, 15 Apr 2024 00:21:01 +0000 Subject: [PATCH 1/3] fix(storage): wrap error when MaxAttempts is hit Wrap the error when retries are cut off by hitting the configured value for MaxAttempts. This makes it easier to verify that retries occurred. Also adds emulator tests verifying that wrapping occurs as expected for timeout errors and MaxAttempts. Updates #9720 --- storage/client_test.go | 100 +++++++++++++++++++++++++++++++++-------- storage/invoke.go | 2 +- 2 files changed, 82 insertions(+), 20 deletions(-) diff --git a/storage/client_test.go b/storage/client_test.go index af8fc4c32e9f..d17148e2b747 100644 --- a/storage/client_test.go +++ b/storage/client_test.go @@ -28,6 +28,7 @@ import ( "cloud.google.com/go/iam/apiv1/iampb" "github.com/google/go-cmp/cmp" + "github.com/googleapis/gax-go/v2" "github.com/googleapis/gax-go/v2/apierror" "github.com/googleapis/gax-go/v2/callctx" "google.golang.org/api/iterator" @@ -1351,31 +1352,57 @@ func TestObjectConditionsEmulated(t *testing.T) { func TestRetryNeverEmulated(t *testing.T) { transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) { ctx := context.Background() + instructions := map[string][]string{"storage.buckets.get": {"return-503"}} + testID := createRetryTest(t, project, bucket, client, instructions) + ctx = callctx.SetHeaders(ctx, "x-retry-test-id", testID) + _, err := client.GetBucket(ctx, bucket, nil, withRetryConfig(&retryConfig{policy: RetryNever})) - attrs, err := client.CreateBucket(ctx, project, bucket, &BucketAttrs{}, nil) - if err != nil { - t.Fatalf("creating bucket: %v", err) + var ae *apierror.APIError + if errors.As(err, &ae) { + // We espect a 503/UNAVAILABLE error. For anything else including a nil + // error, the test should fail. + if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 { + t.Errorf("GetBucket: got unexpected error %v; want 503", err) + } } + }) +} - // Need the HTTP hostname to set up a retry test, as well as knowledge of - // underlying transport to specify instructions. - host := os.Getenv("STORAGE_EMULATOR_HOST") - endpoint, err := url.Parse(host) - if err != nil { - t.Fatalf("parsing endpoint: %v", err) +// Test that errors are wrapped correctly if retry happens until a timeout. +func TestRetryTimeoutEmulated(t *testing.T) { + transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) { + ctx := context.Background() + instructions := map[string][]string{"storage.buckets.get": {"return-503", "return-503", "return-503", "return-503", "return-503"}} + testID := createRetryTest(t, project, bucket, client, instructions) + ctx = callctx.SetHeaders(ctx, "x-retry-test-id", testID) + ctx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) + defer cancel() + _, err := client.GetBucket(ctx, bucket, nil, idempotent(true)) + + var ae *apierror.APIError + if errors.As(err, &ae) { + // We espect a 503/UNAVAILABLE error. For anything else including a nil + // error, the test should fail. + if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 { + t.Errorf("GetBucket: got unexpected error: %v; want 503", err) + } } - var transport string - if _, ok := client.(*httpStorageClient); ok { - transport = "http" - } else { - transport = "grpc" + // Error should be wrapped so it's also equivalent to a context timeout. + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("GetBucket: got unexpected error %v, want to match DeadlineExceeded.", err) } + }) +} - et := emulatorTest{T: t, name: "testRetryNever", resources: resources{}, - host: endpoint} - et.create(map[string][]string{"storage.buckets.get": {"return-503"}}, transport) - ctx = callctx.SetHeaders(ctx, "x-retry-test-id", et.id) - _, err = client.GetBucket(ctx, attrs.Name, nil, withRetryConfig(&retryConfig{policy: RetryNever})) +// Test that errors are wrapped correctly if retry happens until max attempts. +func TestRetryMaxAttemptsEmulated(t *testing.T) { + transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) { + ctx := context.Background() + instructions := map[string][]string{"storage.buckets.get": {"return-503", "return-503", "return-503", "return-503", "return-503"}} + testID := createRetryTest(t, project, bucket, client, instructions) + ctx = callctx.SetHeaders(ctx, "x-retry-test-id", testID) + config := &retryConfig{maxAttempts: expectedAttempts(3), backoff: &gax.Backoff{Initial: 10 * time.Millisecond}} + _, err := client.GetBucket(ctx, bucket, nil, idempotent(true), withRetryConfig(config)) var ae *apierror.APIError if errors.As(err, &ae) { @@ -1385,9 +1412,44 @@ func TestRetryNeverEmulated(t *testing.T) { t.Errorf("GetBucket: got unexpected error %v; want 503", err) } } + // Error should be wrapped so it indicates that MaxAttempts has been reached. + if got, want := err.Error(), "retry failed after 3 attempts"; !strings.Contains(got, want) { + t.Errorf("got error: %q, want to contain: %q", got, want) + } }) } +// createRetryTest creates a bucket in the emulator and sets up a test using the +// Retry Test API for the given instructions. This is intended for emulator tests +// of retry behavior that are not covered by conformance tests. +func createRetryTest(t *testing.T, project, bucket string, client storageClient, instructions map[string][]string) string { + t.Helper() + ctx := context.Background() + + _, err := client.CreateBucket(ctx, project, bucket, &BucketAttrs{}, nil) + if err != nil { + t.Fatalf("creating bucket: %v", err) + } + + // Need the HTTP hostname to set up a retry test, as well as knowledge of + // underlying transport to specify instructions. + host := os.Getenv("STORAGE_EMULATOR_HOST") + endpoint, err := url.Parse(host) + if err != nil { + t.Fatalf("parsing endpoint: %v", err) + } + var transport string + if _, ok := client.(*httpStorageClient); ok { + transport = "http" + } else { + transport = "grpc" + } + + et := emulatorTest{T: t, name: t.Name(), resources: resources{}, host: endpoint} + et.create(instructions, transport) + return et.id +} + // createObject creates an object in the emulator and returns its name, generation, and // metageneration. func createObject(ctx context.Context, bucket string) (string, int64, int64, error) { diff --git a/storage/invoke.go b/storage/invoke.go index 1b52eb5d2c65..88f3c38e48ba 100644 --- a/storage/invoke.go +++ b/storage/invoke.go @@ -71,7 +71,7 @@ func run(ctx context.Context, call func(ctx context.Context) error, retry *retry ctxWithHeaders := setInvocationHeaders(ctx, invocationID, attempts) err = call(ctxWithHeaders) if retry.maxAttempts != nil && attempts >= *retry.maxAttempts { - return true, err + return true, fmt.Errorf("storage: retry failed after %v attempts; last error: %w", *retry.maxAttempts, err) } attempts++ return !errorFunc(err), err From 5c3944c49766000b4d91b6b0eb00651017d8df44 Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Mon, 15 Apr 2024 19:25:12 +0000 Subject: [PATCH 2/3] fix wrapping on last attempt and invoke test --- storage/invoke.go | 2 +- storage/invoke_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/storage/invoke.go b/storage/invoke.go index 88f3c38e48ba..376be1adf1a4 100644 --- a/storage/invoke.go +++ b/storage/invoke.go @@ -70,7 +70,7 @@ func run(ctx context.Context, call func(ctx context.Context) error, retry *retry return internal.Retry(ctx, bo, func() (stop bool, err error) { ctxWithHeaders := setInvocationHeaders(ctx, invocationID, attempts) err = call(ctxWithHeaders) - if retry.maxAttempts != nil && attempts >= *retry.maxAttempts { + if err != nil && retry.maxAttempts != nil && attempts >= *retry.maxAttempts { return true, fmt.Errorf("storage: retry failed after %v attempts; last error: %w", *retry.maxAttempts, err) } attempts++ diff --git a/storage/invoke_test.go b/storage/invoke_test.go index a89695a86734..d3549b235bb8 100644 --- a/storage/invoke_test.go +++ b/storage/invoke_test.go @@ -251,9 +251,9 @@ func TestInvoke(t *testing.T) { return test.finalErr } got := run(ctx, call, test.retry, test.isIdempotentValue) - if test.expectFinalErr && got != test.finalErr { + if test.expectFinalErr && !errors.Is(got, test.finalErr) { s.Errorf("got %v, want %v", got, test.finalErr) - } else if !test.expectFinalErr && got != test.initialErr { + } else if !test.expectFinalErr && !errors.Is(got, test.initialErr) { s.Errorf("got %v, want %v", got, test.initialErr) } wantAttempts := 1 + test.count From 885f188ef33758d78f7da5691e25ab77f3efb3a1 Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Mon, 15 Apr 2024 19:54:07 +0000 Subject: [PATCH 3/3] test cleanup, fix typos --- storage/client_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/storage/client_test.go b/storage/client_test.go index d17148e2b747..d54686c2b3dc 100644 --- a/storage/client_test.go +++ b/storage/client_test.go @@ -1359,7 +1359,7 @@ func TestRetryNeverEmulated(t *testing.T) { var ae *apierror.APIError if errors.As(err, &ae) { - // We espect a 503/UNAVAILABLE error. For anything else including a nil + // We expect a 503/UNAVAILABLE error. For anything else including a nil // error, the test should fail. if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 { t.Errorf("GetBucket: got unexpected error %v; want 503", err) @@ -1381,7 +1381,7 @@ func TestRetryTimeoutEmulated(t *testing.T) { var ae *apierror.APIError if errors.As(err, &ae) { - // We espect a 503/UNAVAILABLE error. For anything else including a nil + // We expect a 503/UNAVAILABLE error. For anything else including a nil // error, the test should fail. if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 { t.Errorf("GetBucket: got unexpected error: %v; want 503", err) @@ -1406,7 +1406,7 @@ func TestRetryMaxAttemptsEmulated(t *testing.T) { var ae *apierror.APIError if errors.As(err, &ae) { - // We espect a 503/UNAVAILABLE error. For anything else including a nil + // We expect a 503/UNAVAILABLE error. For anything else including a nil // error, the test should fail. if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 { t.Errorf("GetBucket: got unexpected error %v; want 503", err) @@ -1447,6 +1447,9 @@ func createRetryTest(t *testing.T, project, bucket string, client storageClient, et := emulatorTest{T: t, name: t.Name(), resources: resources{}, host: endpoint} et.create(instructions, transport) + t.Cleanup(func() { + et.delete() + }) return et.id }