fixing race

Signed-off-by: Alan Protasio <[email protected]>

Author: alanprot

pkg/storegateway/bucket_stores.go

@@ -65,7 +65,11 @@ type BucketStores struct {
 
 	// Keeps a bucket store for each tenant.
 	storesMu sync.RWMutex
-	stores   map[string]*BucketStoreWithLastError
+	stores   map[string]*store.BucketStore
+
+	// Keeps the last sync error for the  bucket store for each tenant.
+	storesErrorsMu sync.RWMutex
+	storesErrors   map[string]error
 
 	// Metrics.
 	syncTimes         prometheus.Histogram
@@ -100,7 +104,8 @@ func NewBucketStores(cfg tsdb.BlocksStorageConfig, shardingStrategy ShardingStra
 		limits:             limits,
 		bucket:             cachingBucket,
 		shardingStrategy:   shardingStrategy,
-		stores:             map[string]*BucketStoreWithLastError{},
+		stores:             map[string]*store.BucketStore{},
+		storesErrors: 		map[string]error{},
 		logLevel:           logLevel,
 		bucketStoreMetrics: NewBucketStoreMetrics(),
 		metaFetcherMetrics: NewMetadataFetcherMetrics(),
@@ -198,7 +203,7 @@ func (u *BucketStores) syncUsersBlocks(ctx context.Context, f func(context.Conte
 
 	type job struct {
 		userID string
-		store  *BucketStoreWithLastError
+		store  *store.BucketStore
 	}
 
 	wg := &sync.WaitGroup{}
@@ -231,16 +236,20 @@ func (u *BucketStores) syncUsersBlocks(ctx context.Context, f func(context.Conte
 			defer wg.Done()
 
 			for job := range jobs {
-				if err := f(ctx, job.store.BucketStore); err != nil {
+				if err := f(ctx, job.store); err != nil {
 					if errors.Is(err, bucket.ErrCustomerManagedKeyError) {
-						job.store.err = err
+						u.storesErrorsMu.Lock()
+						u.storesErrors[job.userID] = err
+						u.storesErrorsMu.Unlock()
 					} else {
 						errsMx.Lock()
 						errs.Add(errors.Wrapf(err, "failed to synchronize TSDB blocks for user %s", job.userID))
 						errsMx.Unlock()
 					}
 				} else {
-					job.store.err = nil
+					u.storesErrorsMu.Lock()
+					delete(u.storesErrors, job.userID)
+					u.storesErrorsMu.Unlock()
 				}
 			}
 		}()
@@ -298,11 +307,13 @@ func (u *BucketStores) Series(req *storepb.SeriesRequest, srv storepb.Store_Seri
 		return nil
 	}
 
-	if store.err != nil && errors.Is(store.err, bucket.ErrCustomerManagedKeyError) {
-		return httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", store.err)
+	err := u.getStoreError(userID)
+
+	if err != nil && errors.Is(err, bucket.ErrCustomerManagedKeyError) {
+		return httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
 	}
 
-	err := store.Series(req, spanSeriesServer{
+	err = store.Series(req, spanSeriesServer{
 		Store_SeriesServer: srv,
 		ctx:                spanCtx,
 	})
@@ -366,12 +377,18 @@ func (u *BucketStores) scanUsers(ctx context.Context) ([]string, error) {
 	return users, err
 }
 
-func (u *BucketStores) getStore(userID string) *BucketStoreWithLastError {
+func (u *BucketStores) getStore(userID string) *store.BucketStore {
 	u.storesMu.RLock()
 	defer u.storesMu.RUnlock()
 	return u.stores[userID]
 }
 
+func (u *BucketStores) getStoreError(userID string) error {
+	u.storesErrorsMu.RLock()
+	defer u.storesErrorsMu.RUnlock()
+	return u.storesErrors[userID]
+}
+
 var (
 	errBucketStoreNotEmpty = errors.New("bucket store not empty")
 	errBucketStoreNotFound = errors.New("bucket store not found")
@@ -409,7 +426,7 @@ func (u *BucketStores) closeEmptyBucketStore(userID string) error {
 	return bs.Close()
 }
 
-func isEmptyBucketStore(bs *BucketStoreWithLastError) bool {
+func isEmptyBucketStore(bs *store.BucketStore) bool {
 	min, max := bs.TimeRange()
 	return min == math.MaxInt64 && max == math.MinInt64
 }
@@ -418,7 +435,7 @@ func (u *BucketStores) syncDirForUser(userID string) string {
 	return filepath.Join(u.cfg.BucketStore.SyncDir, userID)
 }
 
-func (u *BucketStores) getOrCreateStore(userID string) (*BucketStoreWithLastError, error) {
+func (u *BucketStores) getOrCreateStore(userID string) (*store.BucketStore, error) {
 	// Check if the store already exists.
 	bs := u.getStore(userID)
 	if bs != nil {
@@ -522,7 +539,7 @@ func (u *BucketStores) getOrCreateStore(userID string) (*BucketStoreWithLastErro
 		bucketStoreOpts = append(bucketStoreOpts, store.WithDebugLogging())
 	}
 
-	s, err := store.NewBucketStore(
+	bs, err := store.NewBucketStore(
 		userBkt,
 		fetcher,
 		u.syncDirForUser(userID),
@@ -542,10 +559,6 @@ func (u *BucketStores) getOrCreateStore(userID string) (*BucketStoreWithLastErro
 		return nil, err
 	}
 
-	bs = &BucketStoreWithLastError{
-		BucketStore: s,
-	}
-
 	u.stores[userID] = bs
 	u.metaFetcherMetrics.AddUserRegistry(userID, fetcherReg)
 	u.bucketStoreMetrics.AddUserRegistry(userID, bucketStoreReg)

pkg/storegateway/bucket_stores_test.go

@@ -29,8 +29,10 @@ import (
 	"github.com/thanos-io/thanos/pkg/store"
 	"github.com/thanos-io/thanos/pkg/store/labelpb"
 	"github.com/thanos-io/thanos/pkg/store/storepb"
+	"github.com/weaveworks/common/httpgrpc"
 	"github.com/weaveworks/common/logging"
 	"go.uber.org/atomic"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
 
 	cortex_testutil "github.com/cortexproject/cortex/pkg/storage/tsdb/testutil"
@@ -74,17 +76,26 @@ func TestBucketStores_CustomerKeyError(t *testing.T) {
 
 	// Should set the error on user-1
 	require.NoError(t, stores.InitialSync(ctx))
-	require.ErrorIs(t, stores.stores["user-1"].err, bucket.ErrCustomerManagedKeyError)
-	require.ErrorIs(t, stores.stores["user-2"].err, nil)
+	require.ErrorIs(t, stores.storesErrors["user-1"], bucket.ErrCustomerManagedKeyError)
+	require.ErrorIs(t, stores.storesErrors["user-2"], nil)
 	require.NoError(t, stores.SyncBlocks(context.Background()))
-	require.ErrorIs(t, stores.stores["user-1"].err, bucket.ErrCustomerManagedKeyError)
-	require.ErrorIs(t, stores.stores["user-2"].err, nil)
+	require.ErrorIs(t, stores.storesErrors["user-1"], bucket.ErrCustomerManagedKeyError)
+	require.ErrorIs(t, stores.storesErrors["user-2"], nil)
+
+	_, _, err = querySeries(stores, "user-1", "anything", 0, 100)
+	require.Equal(t, err, httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", bucket.ErrCustomerManagedKeyError))
+	_, _, err = querySeries(stores, "user-2", "anything", 0, 100)
+	require.NoError(t, err)
 
 	// Cleaning the error
 	mBucket.GetFailures = map[string]error{}
 	require.NoError(t, stores.SyncBlocks(context.Background()))
-	require.ErrorIs(t, stores.stores["user-1"].err, nil)
-	require.ErrorIs(t, stores.stores["user-2"].err, nil)
+	require.ErrorIs(t, stores.storesErrors["user-1"], nil)
+	require.ErrorIs(t, stores.storesErrors["user-2"], nil)
+	_, _, err = querySeries(stores, "user-1", "anything", 0, 100)
+	require.NoError(t, err)
+	_, _, err = querySeries(stores, "user-2", "anything", 0, 100)
+	require.NoError(t, err)
 }
 
 func TestBucketStores_InitialSync(t *testing.T) {