move shuffle to refreshOwnedUsers

CHANGELOG.md

@@ -35,7 +35,7 @@
   * `cortex_ingester_tsdb_block_postings_for_matchers_cache_misses_total`
   * `cortex_ingester_tsdb_block_postings_for_matchers_cache_requests_total`
   * `cortex_ingester_tsdb_block_postings_for_matchers_cache_skips_total`
-* [ENHANCEMENT] Compactor: Shuffle order of users in `UsersScanner` before submitting to `BlocksCleaner`. #10513
+* [ENHANCEMENT] Compactor: Shuffle users' order in `BlocksCleaner`. Prevents bucket indexes from going an extended period without cleanup during compactor restarts. #10513
 * [BUGFIX] Distributor: Use a boolean to track changes while merging the ReplicaDesc components, rather than comparing the objects directly. #10185
 * [BUGFIX] Querier: fix timeout responding to query-frontend when response size is very close to `-querier.frontend-client.grpc-max-send-msg-size`. #10154
 * [BUGFIX] Query-frontend and querier: show warning/info annotations in some cases where they were missing (if a lazy querier was used). #10277

pkg/compactor/blocks_cleaner.go

@@ -8,6 +8,7 @@ package compactor
 import (
 	"context"
 	"fmt"
+	"math/rand"
 	"strconv"
 	"strings"
 	"sync"
@@ -232,6 +233,10 @@ func (c *BlocksCleaner) refreshOwnedUsers(ctx context.Context) ([]string, map[st
 		return nil, nil, errors.Wrap(err, "failed to discover users from bucket")
 	}
 
+	rand.Shuffle(len(users), func(i, j int) {
+		users[i], users[j] = users[j], users[i]
+	})
+
 	isActive := util.StringsMap(users)
 	isDeleted := util.StringsMap(deleted)
 	allUsers := append(users, deleted...)

pkg/storage/tsdb/users_scanner.go

@@ -7,7 +7,6 @@ package tsdb
 
 import (
 	"context"
-	"math/rand"
 	"strings"
 
 	"github.com/go-kit/log"
@@ -47,10 +46,6 @@ func (s *UsersScanner) ScanUsers(ctx context.Context) (users, markedForDeletion
 		return nil, nil, err
 	}
 
-	rand.Shuffle(len(users), func(i, j int) {
-		users[i], users[j] = users[j], users[i]
-	})
-
 	// Check users for being owned by instance, and split users into non-deleted and deleted.
 	// We do these checks after listing all users, to improve cacheability of Iter (result is only cached at the end of Iter call).
 	for ix := 0; ix < len(users); {

pkg/storage/tsdb/users_scanner_test.go

@@ -8,7 +8,6 @@ package tsdb
 import (
 	"context"
 	"errors"
-	"fmt"
 	"path"
 	"testing"
 
@@ -37,10 +36,10 @@ func TestUsersScanner_ScanUsers_ShouldReturnedOwnedUsersOnly(t *testing.T) {
 }
 
 func TestUsersScanner_ScanUsers_ShouldReturnUsersForWhichOwnerCheckOrTenantDeletionCheckFailed(t *testing.T) {
-	users := []string{"user-1", "user-2"}
+	expected := []string{"user-1", "user-2"}
 
 	bucketClient := &bucket.ClientMock{}
-	bucketClient.MockIter("", users, nil)
+	bucketClient.MockIter("", expected, nil)
 	bucketClient.MockExists(path.Join("user-1", TenantDeletionMarkPath), false, nil)
 	bucketClient.MockExists(path.Join("user-2", TenantDeletionMarkPath), false, errors.New("fail"))
 
@@ -51,12 +50,11 @@ func TestUsersScanner_ScanUsers_ShouldReturnUsersForWhichOwnerCheckOrTenantDelet
 	s := NewUsersScanner(bucketClient, isOwned, log.NewNopLogger())
 	actual, deleted, err := s.ScanUsers(context.Background())
 	require.NoError(t, err)
-	assert.ElementsMatch(t, actual, users)
+	assert.Equal(t, expected, actual)
 	assert.Empty(t, deleted)
 }
 
 func TestUsersScanner_ScanUsers_ShouldNotReturnPrefixedUsedByMimirInternals(t *testing.T) {
-	users := []string{"user-1", "user-2"}
 	bucketClient := &bucket.ClientMock{}
 	bucketClient.MockIter("", []string{"user-1", "user-2", bucket.MimirInternalsPrefix}, nil)
 	bucketClient.MockExists(path.Join("user-1", TenantDeletionMarkPath), false, nil)
@@ -65,28 +63,5 @@ func TestUsersScanner_ScanUsers_ShouldNotReturnPrefixedUsedByMimirInternals(t *t
 	s := NewUsersScanner(bucketClient, AllUsers, log.NewNopLogger())
 	actual, _, err := s.ScanUsers(context.Background())
 	require.NoError(t, err)
-	assert.ElementsMatch(t, actual, users)
-}
-
-func TestUsersScanner_ScanUsers_ShouldReturnRandomizedOrder(t *testing.T) {
-	var users = make([]string, 20)
-	for i := 0; i < len(users); i++ {
-		users[i] = fmt.Sprintf("user-%d", i)
-	}
-	bucketClient := &bucket.ClientMock{}
-	bucketClient.MockIter("", users, nil)
-	for i := 0; i < len(users); i++ {
-		bucketClient.MockExists(path.Join(users[i], TenantDeletionMarkPath), false, nil)
-	}
-
-	isOwned := func(_ string) (bool, error) {
-		return true, nil
-	}
-
-	s := NewUsersScanner(bucketClient, isOwned, log.NewNopLogger())
-	actual, deleted, err := s.ScanUsers(context.Background())
-	require.NoError(t, err)
-	assert.ElementsMatch(t, actual, users)
-	assert.Empty(t, deleted)
-	assert.NotEqual(t, actual, users)
+	assert.Equal(t, []string{"user-1", "user-2"}, actual)
 }