gcs: retry Delete on transient 5xx

CHANGELOG.md

@@ -15,6 +15,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 - [#157](https://github.com/thanos-io/objstore/pull/157) Azure: Add `az_tenant_id`, `client_id` and `client_secret` configs.
 
 ### Fixed
+- [#261](https://github.com/thanos-io/objstore/pull/261) GCS: retry Delete on transient 5xx, and cap all operations at MaxRetries (default 3 when unset).
 - [#196](https://github.com/thanos-io/objstore/pull/196) GCS: fix error check in Exists method when object does not exist.
 - [#153](https://github.com/thanos-io/objstore/pull/153) Metrics: Fix `objstore_bucket_operation_duration_seconds_*` for `get` and `get_range` operations.
 - [#141](https://github.com/thanos-io/objstore/pull/142) S3: Fix missing encryption configuration for `Bucket.Exists()` and `Bucket.Attributes()` calls.

providers/gcs/gcs.go

@@ -35,6 +35,7 @@ const DirDelim = "/"
 
 var DefaultConfig = Config{
 	HTTPConfig: exthttp.DefaultHTTPConfig,
+	MaxRetries: 3,
 }
 
 // Config stores the configuration for gcs bucket.
@@ -55,9 +56,8 @@ type Config struct {
 	ChunkSizeBytes int  `yaml:"chunk_size_bytes"`
 	noAuth         bool `yaml:"no_auth"`
 
-	// MaxRetries controls the number of retries for idempotent operations.
-	// Overrides the default gcs storage client behavior if this value is greater than 0.
-	// Set this to 1 to disable retries.
+	// MaxRetries controls the number of attempts for retryable operations.
+	// Defaults to 3 when unset (see DefaultConfig). Set this to 1 to disable retries.
 	MaxRetries int `yaml:"max_retries"`
 }
 
@@ -179,9 +179,14 @@ func newBucket(ctx context.Context, logger log.Logger, gc Config, opts []option.
 		chunkSize: gc.ChunkSizeBytes,
 	}
 
-	if gc.MaxRetries > 0 {
-		bkt.bkt = bkt.bkt.Retryer(storage.WithMaxAttempts(gc.MaxRetries))
+	// Cap retries on transient errors. See
+	// https://docs.cloud.google.com/storage/docs/retry-strategy for what
+	// counts as transient and how the SDK handles backoff.
+	maxAttempts := gc.MaxRetries
+	if maxAttempts == 0 {
+		maxAttempts = DefaultConfig.MaxRetries
 	}
+	bkt.bkt = bkt.bkt.Retryer(storage.WithMaxAttempts(maxAttempts))
 
 	return bkt, nil
 }
@@ -349,9 +354,11 @@ func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader, opts ...o
 	return w.Close()
 }
 
-// Delete removes the object with the given name.
+// Delete removes the object with the given name. RetryAlways overrides the
+// default RetryIdempotent policy, which would otherwise exclude Delete because
+// we don't pass IfGenerationMatch.
 func (b *Bucket) Delete(ctx context.Context, name string) error {
-	return b.bkt.Object(name).Delete(ctx)
+	return b.bkt.Object(name).Retryer(storage.WithPolicy(storage.RetryAlways)).Delete(ctx)
 }
 
 // IsObjNotFoundErr returns true if error means that object is not found. Relevant to Get operations.

providers/gcs/gcs_test.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -17,6 +18,7 @@ import (
 	"github.com/go-kit/log"
 	"github.com/prometheus/common/model"
 	"github.com/thanos-io/objstore/errutil"
+	"go.uber.org/atomic"
 	"google.golang.org/api/option"
 )
 
@@ -178,3 +180,30 @@ func TestNewBucketWithErrorRoundTripper(t *testing.T) {
 	testutil.NotOk(t, err)
 	testutil.Assert(t, errutil.IsMockedError(err), "Expected RoundTripper error, got: %v", err)
 }
+
+func TestBucket_Delete_RetriesOnTransient5xx(t *testing.T) {
+	var deleteAttempts atomic.Int32
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "/o/") {
+			n := deleteAttempts.Add(1)
+			if n < 3 {
+				w.WriteHeader(http.StatusServiceUnavailable)
+				_, _ = w.Write([]byte(`{"error":{"code":503,"message":"backendError"}}`))
+				return
+			}
+			w.WriteHeader(http.StatusNoContent)
+			return
+		}
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer srv.Close()
+
+	t.Setenv("STORAGE_EMULATOR_HOST", srv.Listener.Addr().String())
+
+	bkt, err := newBucket(context.Background(), log.NewNopLogger(), Config{Bucket: "test-bucket"}, []option.ClientOption{})
+	testutil.Ok(t, err)
+
+	testutil.Ok(t, bkt.Delete(context.Background(), "test-object"))
+	testutil.Equals(t, int32(3), deleteAttempts.Load())
+}