fix(storage): disable gax retries for gRPC (#9747)

tritone · web-flow · commit bbfc0acc272f · 2024-04-11T20:02:16.000Z
All gRPC ops are retried as needed at the handwritten layer. Disable gax retries.
diff --git a/storage/client_test.go b/storage/client_test.go
@@ -16,8 +16,10 @@ package storage
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log"
+	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -26,7 +28,10 @@ import (
 
 	"cloud.google.com/go/iam/apiv1/iampb"
 	"github.com/google/go-cmp/cmp"
+	"github.com/googleapis/gax-go/v2/apierror"
+	"github.com/googleapis/gax-go/v2/callctx"
 	"google.golang.org/api/iterator"
+	"google.golang.org/grpc/codes"
 )
 
 var emulatorClients map[string]storageClient
@@ -1342,6 +1347,47 @@ func TestObjectConditionsEmulated(t *testing.T) {
 	})
 }
 
+// Test that RetryNever prevents any retries from happening in both transports.
+func TestRetryNeverEmulated(t *testing.T) {
+	transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) {
+		ctx := context.Background()
+
+		attrs, 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: "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}))
+
+		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)
+			}
+		}
+	})
+}
+
 // 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/grpc_client.go b/storage/grpc_client.go
@@ -116,6 +116,8 @@ type grpcStorageClient struct {
 func newGRPCStorageClient(ctx context.Context, opts ...storageOption) (storageClient, error) {
 	s := initSettings(opts...)
 	s.clientOption = append(defaultGRPCOptions(), s.clientOption...)
+	// Disable all gax-level retries in favor of retry logic in the veneer client.
+	s.gax = append(s.gax, gax.WithRetry(nil))
 
 	config := newStorageConfig(s.clientOption...)
 	if config.readAPIWasSet {
diff --git a/storage/retry_conformance_test.go b/storage/retry_conformance_test.go
@@ -576,6 +576,9 @@ var methods = map[string][]retryFunc{
 }
 
 func TestRetryConformance(t *testing.T) {
+	// This endpoint is used only to call the testbench retry test API, which is HTTP
+	// based. The endpoint called by the client library is determined inside of the
+	// client constructor and will differ depending on the transport.
 	host := os.Getenv("STORAGE_EMULATOR_HOST")
 	if host == "" {
 		t.Skip("This test must use the testbench emulator; set STORAGE_EMULATOR_HOST to run.")
@@ -759,7 +762,7 @@ func (et *emulatorTest) create(instructions map[string][]string, transport strin
 
 	et.host.Path = "retry_test"
 	resp, err := c.Post(et.host.String(), "application/json", buf)
-	if resp.StatusCode == 501 {
+	if resp != nil && resp.StatusCode == 501 {
 		et.T.Skip("This retry test case is not yet supported in the testbench.")
 	}
 	if err != nil || resp.StatusCode != 200 {
