adding gs

Author: alanprot

pkg/compactor/blocks_cleaner.go

@@ -326,17 +326,40 @@ func (c *BlocksCleaner) cleanUser(ctx context.Context, userID string, firstRun b
 		}
 	}
 
+	// Reading bucket index sync stats
+	idxs, err := bucketindex.ReadSyncStatus(ctx, c.bucketClient, userID, userLogger)
+
+	if err != nil {
+		level.Warn(userLogger).Log("msg", "error reading the bucket index status", "err", err)
+		idxs = bucketindex.Status{Version: bucketindex.SyncStatusFileVersion, NonQueryableReason: bucketindex.Unknown}
+	}
+
+	idxs.Status = bucketindex.Ok
+	idxs.SyncTime = time.Now().Unix()
+
 	// Read the bucket index.
 	idx, err := bucketindex.ReadIndex(ctx, c.bucketClient, userID, c.cfgProvider, c.logger)
+
+	defer func() {
+		bucketindex.WriteSyncStatus(ctx, c.bucketClient, userID, idxs, userLogger)
+	}()
+
 	if errors.Is(err, bucketindex.ErrIndexCorrupted) {
 		level.Warn(userLogger).Log("msg", "found a corrupted bucket index, recreating it")
 	} else if errors.Is(err, bucket.ErrCustomerManagedKeyAccessDenied) {
 		// Give up cleaning if we get access denied
 		level.Warn(userLogger).Log("msg", "customer manager key access denied", "err", err)
-		bucketindex.WriteSyncStatus(ctx, c.bucketClient, userID, bucketindex.CustomerManagedKeyError, userLogger)
+		idxs.Status = bucketindex.CustomerManagedKeyError
+		// Making the tenant non queryable until 2x the cleanup interval to give time to compactors and storegateways
+		// to reload the bucket index in case the key access is re-granted
+		idxs.NonQueryableUntil = time.Now().Add(2 * c.cfg.CleanupInterval).Unix()
+		idxs.NonQueryableReason = bucketindex.CustomerManagedKeyError
+
+		// Update the bucket index update time
+		c.tenantBucketIndexLastUpdate.WithLabelValues(userID).SetToCurrentTime()
 		return nil
 	} else if err != nil && !errors.Is(err, bucketindex.ErrIndexNotFound) {
-		bucketindex.WriteSyncStatus(ctx, c.bucketClient, userID, bucketindex.GenericError, userLogger)
+		idxs.Status = bucketindex.GenericError
 		return err
 	}
 
@@ -355,7 +378,7 @@ func (c *BlocksCleaner) cleanUser(ctx context.Context, userID string, firstRun b
 	w := bucketindex.NewUpdater(c.bucketClient, userID, c.cfgProvider, c.logger)
 	idx, partials, totalBlocksBlocksMarkedForNoCompaction, err := w.UpdateIndex(ctx, idx)
 	if err != nil {
-		bucketindex.WriteSyncStatus(ctx, c.bucketClient, userID, bucketindex.GenericError, userLogger)
+		idxs.Status = bucketindex.GenericError
 		return err
 	}
 
@@ -406,7 +429,6 @@ func (c *BlocksCleaner) cleanUser(ctx context.Context, userID string, firstRun b
 	c.tenantBlocksMarkedForNoCompaction.WithLabelValues(userID).Set(float64(totalBlocksBlocksMarkedForNoCompaction))
 	c.tenantBucketIndexLastUpdate.WithLabelValues(userID).SetToCurrentTime()
 	c.tenantPartialBlocks.WithLabelValues(userID).Set(float64(len(partials)))
-	bucketindex.WriteSyncStatus(ctx, c.bucketClient, userID, bucketindex.Ok, userLogger)
 	return nil
 }
 

pkg/compactor/blocks_cleaner_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"crypto/rand"
 	"fmt"
-	"os"
 	"path"
 	"strings"
 	"testing"
@@ -58,14 +57,14 @@ func TestBlocksCleaner(t *testing.T) {
 func TestBlockCleaner_KeyPermissionDenied(t *testing.T) {
 	const userID = "user-1"
 
-	bucketClient, dir := cortex_testutil.PrepareFilesystemBucket(t)
-	bucketClient = bucketindex.BucketWithGlobalMarkers(bucketClient)
+	bkt, _ := cortex_testutil.PrepareFilesystemBucket(t)
+	bkt = bucketindex.BucketWithGlobalMarkers(bkt)
 
 	// Create blocks.
 	ctx := context.Background()
 	deletionDelay := 12 * time.Hour
-	bucketClient = &cortex_testutil.MockBucketFailure{
-		Bucket: bucketClient,
+	mbucket := &cortex_testutil.MockBucketFailure{
+		Bucket: bkt,
 		GetFailures: map[string]error{
 			path.Join(userID, "bucket-index.json.gz"): cortex_testutil.ErrKeyAccessDeniedError,
 		},
@@ -78,18 +77,37 @@ func TestBlockCleaner_KeyPermissionDenied(t *testing.T) {
 	}
 
 	logger := log.NewNopLogger()
-	scanner := tsdb.NewUsersScanner(bucketClient, tsdb.AllUsers, logger)
+	scanner := tsdb.NewUsersScanner(mbucket, tsdb.AllUsers, logger)
 	cfgProvider := newMockConfigProvider()
 
-	cleaner := NewBlocksCleaner(cfg, bucketClient, scanner, cfgProvider, logger, nil)
-	err := cleaner.cleanUser(ctx, userID, true)
+	cleaner := NewBlocksCleaner(cfg, mbucket, scanner, cfgProvider, logger, nil)
+
+	// Clean User with no error
+	cleaner.bucketClient = bkt
+	err := cleaner.cleanUser(ctx, userID, false)
 	require.NoError(t, err)
-	stat, err := os.Stat(path.Join(dir, userID, bucketindex.SyncStatusFile))
+	s, err := bucketindex.ReadSyncStatus(ctx, bkt, userID, logger)
 	require.NoError(t, err)
-	require.True(t, stat.Size() > 0)
-	s, err := bucketindex.ReadSyncStatus(ctx, bucketClient, userID, logger)
+	require.Equal(t, bucketindex.Ok, s.Status)
+	require.Equal(t, int64(0), s.NonQueryableUntil)
+
+	// Clean with cmk error
+	cleaner.bucketClient = mbucket
+	err = cleaner.cleanUser(ctx, userID, false)
+	require.NoError(t, err)
+	s, err = bucketindex.ReadSyncStatus(ctx, bkt, userID, logger)
+	require.NoError(t, err)
+	require.Equal(t, bucketindex.CustomerManagedKeyError, s.Status)
+	require.Less(t, int64(0), s.NonQueryableUntil)
+
+	// Re grant access to the key
+	cleaner.bucketClient = bkt
+	err = cleaner.cleanUser(ctx, userID, false)
+	require.NoError(t, err)
+	s, err = bucketindex.ReadSyncStatus(ctx, bkt, userID, logger)
 	require.NoError(t, err)
-	require.Equal(t, bucketindex.CustomerManagedKeyError, s)
+	require.Equal(t, bucketindex.Ok, s.Status)
+	require.Less(t, int64(0), s.NonQueryableUntil)
 }
 
 func testBlocksCleanerWithOptions(t *testing.T, options testBlocksCleanerOptions) {
@@ -241,7 +259,7 @@ func testBlocksCleanerWithOptions(t *testing.T, options testBlocksCleanerOptions
 		assert.ElementsMatch(t, tc.expectedMarks, idx.BlockDeletionMarks.GetULIDs())
 		s, err := bucketindex.ReadSyncStatus(ctx, bucketClient, tc.userID, logger)
 		require.NoError(t, err)
-		require.Equal(t, bucketindex.Ok, s)
+		require.Equal(t, bucketindex.Ok, s.Status)
 	}
 
 	assert.NoError(t, prom_testutil.GatherAndCompare(reg, strings.NewReader(`
@@ -397,7 +415,7 @@ func TestBlocksCleaner_ShouldRebuildBucketIndexOnCorruptedOne(t *testing.T) {
 	assert.ElementsMatch(t, []ulid.ULID{block3}, idx.BlockDeletionMarks.GetULIDs())
 	s, err := bucketindex.ReadSyncStatus(ctx, bucketClient, userID, logger)
 	require.NoError(t, err)
-	require.Equal(t, bucketindex.Ok, s)
+	require.Equal(t, bucketindex.Ok, s.Status)
 }
 
 func TestBlocksCleaner_ShouldRemoveMetricsForTenantsNotBelongingAnymoreToTheShard(t *testing.T) {

pkg/compactor/compactor.go

@@ -675,6 +675,14 @@ func (c *Compactor) compactUsers(ctx context.Context) {
 			continue
 		}
 
+		if idxs, err := bucketindex.ReadSyncStatus(ctx, c.bucketClient, userID, util_log.WithUserID(userID, c.logger)); err == nil {
+			if idxs.Status == bucketindex.CustomerManagedKeyError {
+				c.compactionRunSkippedTenants.Inc()
+				level.Info(c.logger).Log("msg", "skipping compactUser due CustomerManagedKeyError", "user", userID)
+				continue
+			}
+		}
+
 		ownedUsers[userID] = struct{}{}
 
 		if markedForDeletion, err := cortex_tsdb.TenantDeletionMarkExists(ctx, c.bucketClient, userID); err != nil {

pkg/compactor/compactor_test.go

@@ -151,6 +151,38 @@ func TestConfig_Validate(t *testing.T) {
 	}
 }
 
+func TestCompactor_SkipCompactionWhenCmkError(t *testing.T) {
+	t.Parallel()
+	userID := "user-1"
+
+	ss := bucketindex.Status{Status: bucketindex.CustomerManagedKeyError, Version: bucketindex.SyncStatusFileVersion}
+	content, err := json.Marshal(ss)
+	require.NoError(t, err)
+
+	// No user blocks stored in the bucket.
+	bucketClient := &bucket.ClientMock{}
+	bucketClient.MockIter("", []string{userID}, nil)
+	bucketClient.MockIter(userID+"/", []string{}, nil)
+	bucketClient.MockIter(userID+"/markers/", nil, nil)
+	bucketClient.MockGet(userID+"/bucket-index-sync-status.json", string(content), nil)
+	bucketClient.MockGet(userID+"/bucket-index.json.gz", "", nil)
+	bucketClient.MockUpload(userID+"/bucket-index-sync-status.json", nil)
+	bucketClient.MockUpload(userID+"/bucket-index.json.gz", nil)
+	bucketClient.MockExists(path.Join(userID, cortex_tsdb.TenantDeletionMarkPath), false, nil)
+
+	cfg := prepareConfig()
+	c, _, _, logs, _ := prepare(t, cfg, bucketClient, nil)
+	require.NoError(t, services.StartAndAwaitRunning(context.Background(), c))
+
+	// Wait until a run has completed.
+	cortex_testutil.Poll(t, time.Second, 1.0, func() interface{} {
+		return prom_testutil.ToFloat64(c.compactionRunsCompleted)
+	})
+
+	require.NoError(t, services.StopAndAwaitTerminated(context.Background(), c))
+	assert.Contains(t, strings.Split(strings.TrimSpace(logs.String()), "\n"), `level=info component=compactor msg="skipping compactUser due CustomerManagedKeyError" user=user-1`)
+}
+
 func TestCompactor_ShouldDoNothingOnNoUserBlocks(t *testing.T) {
 	t.Parallel()
 
@@ -535,6 +567,8 @@ func TestCompactor_ShouldIterateOverUsersAndRunCompaction(t *testing.T) {
 	bucketClient.MockGet("user-2/01FN3V83ABR9992RF8WRJZ76ZQ/visit-mark.json", "", nil)
 	bucketClient.MockGet("user-1/bucket-index.json.gz", "", nil)
 	bucketClient.MockGet("user-2/bucket-index.json.gz", "", nil)
+	bucketClient.MockGet("user-1/bucket-index-sync-status.json", "", nil)
+	bucketClient.MockGet("user-2/bucket-index-sync-status.json", "", nil)
 	bucketClient.MockIter("user-1/markers/", nil, nil)
 	bucketClient.MockIter("user-2/markers/", nil, nil)
 	bucketClient.MockUpload("user-1/bucket-index.json.gz", nil)
@@ -676,6 +710,7 @@ func TestCompactor_ShouldNotCompactBlocksMarkedForDeletion(t *testing.T) {
 	bucketClient.MockDelete("user-1/markers/01DTW0ZCPDDNV4BV83Q2SV4QAZ-deletion-mark.json", nil)
 	bucketClient.MockDelete("user-1/01DTW0ZCPDDNV4BV83Q2SV4QAZ", nil)
 	bucketClient.MockGet("user-1/bucket-index.json.gz", "", nil)
+	bucketClient.MockGet("user-1/bucket-index-sync-status.json", "", nil)
 	bucketClient.MockUpload("user-1/bucket-index.json.gz", nil)
 	bucketClient.MockUpload("user-1/bucket-index-sync-status.json", nil)
 
@@ -799,6 +834,8 @@ func TestCompactor_ShouldNotCompactBlocksMarkedForSkipCompact(t *testing.T) {
 
 	bucketClient.MockGet("user-1/bucket-index.json.gz", "", nil)
 	bucketClient.MockGet("user-2/bucket-index.json.gz", "", nil)
+	bucketClient.MockGet("user-1/bucket-index-sync-status.json", "", nil)
+	bucketClient.MockGet("user-2/bucket-index-sync-status.json", "", nil)
 	bucketClient.MockIter("user-1/markers/", nil, nil)
 	bucketClient.MockIter("user-2/markers/", nil, nil)
 	bucketClient.MockUpload("user-1/bucket-index.json.gz", nil)
@@ -1031,6 +1068,8 @@ func TestCompactor_ShouldCompactAllUsersOnShardingEnabledButOnlyOneInstanceRunni
 	bucketClient.MockUpload("user-2/01FN3V83ABR9992RF8WRJZ76ZQ/visit-mark.json", nil)
 	bucketClient.MockGet("user-1/bucket-index.json.gz", "", nil)
 	bucketClient.MockGet("user-2/bucket-index.json.gz", "", nil)
+	bucketClient.MockGet("user-1/bucket-index-sync-status.json", "", nil)
+	bucketClient.MockGet("user-2/bucket-index-sync-status.json", "", nil)
 	bucketClient.MockUpload("user-1/bucket-index.json.gz", nil)
 	bucketClient.MockUpload("user-2/bucket-index.json.gz", nil)
 	bucketClient.MockUpload("user-1/bucket-index-sync-status.json", nil)

pkg/ingester/ingester.go

@@ -16,6 +16,8 @@ import (
 	"github.com/prometheus/prometheus/config"
 	"github.com/prometheus/prometheus/tsdb/chunks"
 
+	"github.com/cortexproject/cortex/pkg/storage/tsdb/bucketindex"
+
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
 	"github.com/gogo/status"
@@ -2289,6 +2291,13 @@ func (i *Ingester) shipBlocks(ctx context.Context, allowed *util.AllowedTenants)
 		}
 		defer userDB.casState(activeShipping, active)
 
+		if idxs, err := bucketindex.ReadSyncStatus(ctx, i.TSDBState.bucket, userID, logutil.WithContext(ctx, i.logger)); err == nil {
+			if idxs.Status == bucketindex.CustomerManagedKeyError {
+				level.Info(logutil.WithContext(ctx, i.logger)).Log("msg", "skipping shipping blocks due CustomerManagedKeyError", "user", userID)
+				return nil
+			}
+		}
+
 		uploaded, err := userDB.shipper.Sync(ctx)
 		if err != nil {
 			level.Warn(logutil.WithContext(ctx, i.logger)).Log("msg", "shipper failed to synchronize TSDB blocks with the storage", "user", userID, "uploaded", uploaded, "err", err)

pkg/ingester/ingester_test.go

@@ -40,6 +40,8 @@ import (
 	"golang.org/x/sync/errgroup"
 	"google.golang.org/grpc"
 
+	"github.com/cortexproject/cortex/pkg/storage/tsdb/bucketindex"
+
 	"github.com/cortexproject/cortex/pkg/chunk"
 	"github.com/cortexproject/cortex/pkg/chunk/encoding"
 	"github.com/cortexproject/cortex/pkg/cortexpb"
@@ -2642,40 +2644,60 @@ func TestIngester_OpenExistingTSDBOnStartup(t *testing.T) {
 }
 
 func TestIngester_shipBlocks(t *testing.T) {
-	cfg := defaultIngesterTestConfig(t)
-	cfg.LifecyclerConfig.JoinAfter = 0
-	cfg.BlocksStorageConfig.TSDB.ShipConcurrency = 2
+	testCases := map[string]struct {
+		ss                   bucketindex.Status
+		expectetNumberOfCall int
+	}{
+		"should ship blocks if status ok": {
+			ss:                   bucketindex.Status{Version: bucketindex.IndexVersion1, Status: bucketindex.Ok},
+			expectetNumberOfCall: 1,
+		},
+		"should not ship on cmk errors": {
+			ss:                   bucketindex.Status{Version: bucketindex.IndexVersion1, Status: bucketindex.CustomerManagedKeyError},
+			expectetNumberOfCall: 0,
+		},
+	}
 
-	// Create ingester
-	i, err := prepareIngesterWithBlocksStorage(t, cfg, nil)
-	require.NoError(t, err)
-	require.NoError(t, services.StartAndAwaitRunning(context.Background(), i))
-	defer services.StopAndAwaitTerminated(context.Background(), i) //nolint:errcheck
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
 
-	// Wait until it's ACTIVE
-	test.Poll(t, 1*time.Second, ring.ACTIVE, func() interface{} {
-		return i.lifecycler.GetState()
-	})
+			cfg := defaultIngesterTestConfig(t)
+			cfg.LifecyclerConfig.JoinAfter = 0
+			cfg.BlocksStorageConfig.TSDB.ShipConcurrency = 2
 
-	// Create the TSDB for 3 users and then replace the shipper with the mocked one
-	mocks := []*shipperMock{}
-	for _, userID := range []string{"user-1", "user-2", "user-3"} {
-		userDB, err := i.getOrCreateTSDB(userID, false)
-		require.NoError(t, err)
-		require.NotNil(t, userDB)
+			// Create ingester
+			i, err := prepareIngesterWithBlocksStorage(t, cfg, nil)
+			require.NoError(t, err)
+			require.NoError(t, services.StartAndAwaitRunning(context.Background(), i))
+			defer services.StopAndAwaitTerminated(context.Background(), i) //nolint:errcheck
 
-		m := &shipperMock{}
-		m.On("Sync", mock.Anything).Return(0, nil)
-		mocks = append(mocks, m)
+			// Wait until it's ACTIVE
+			test.Poll(t, 1*time.Second, ring.ACTIVE, func() interface{} {
+				return i.lifecycler.GetState()
+			})
 
-		userDB.shipper = m
-	}
+			// Create the TSDB for 3 users and then replace the shipper with the mocked one
+			mocks := []*shipperMock{}
+			for _, userID := range []string{"user-1", "user-2", "user-3"} {
+				bucketindex.WriteSyncStatus(context.Background(), i.TSDBState.bucket, userID, tc.ss, log.NewNopLogger())
+				userDB, err := i.getOrCreateTSDB(userID, false)
+				require.NoError(t, err)
+				require.NotNil(t, userDB)
 
-	// Ship blocks and assert on the mocked shipper
-	i.shipBlocks(context.Background(), nil)
+				m := &shipperMock{}
+				m.On("Sync", mock.Anything).Return(0, nil)
+				mocks = append(mocks, m)
 
-	for _, m := range mocks {
-		m.AssertNumberOfCalls(t, "Sync", 1)
+				userDB.shipper = m
+			}
+
+			// Ship blocks and assert on the mocked shipper
+			i.shipBlocks(context.Background(), nil)
+
+			for _, m := range mocks {
+				m.AssertNumberOfCalls(t, "Sync", tc.expectetNumberOfCall)
+			}
+		})
 	}
 }
 

pkg/querier/blocks_finder_bucket_index.go

@@ -63,13 +63,12 @@ func (f *BucketIndexBlocksFinder) GetBlocks(ctx context.Context, userID string,
 		// This is a legit edge case, happening when a new tenant has not shipped blocks to the storage yet
 		// so the bucket index hasn't been created yet.
 		return nil, nil, nil
-	}
-
-	if errors.Is(err, bucket.ErrCustomerManagedKeyAccessDenied) {
+	} else if errors.Is(err, bucket.ErrCustomerManagedKeyAccessDenied) {
 		return nil, nil, validation.AccessDeniedError(err.Error())
 	}
 
-	if ss == bucketindex.CustomerManagedKeyError {
+	// Short circuit when bucket failed to be updated due CMK errors recently
+	if time.Since(ss.GetNonQueryableUntil()) < 0 && ss.NonQueryableReason == bucketindex.CustomerManagedKeyError {
 		return nil, nil, validation.AccessDeniedError(bucket.ErrCustomerManagedKeyAccessDenied.Error())
 	}