Shuffle User Order in UsersScanner

CHANGELOG.md

@@ -35,6 +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
 * [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/storage/bucket/client_mock.go

@@ -127,7 +127,7 @@ func (m *ClientMock) MockGetAndLastModified(name, content string, lastModified t
 	} else {
 		m.On("Exists", mock.Anything, name).Return(false, err)
 		m.On("Get", mock.Anything, name).Return(nil, ErrObjectDoesNotExist)
-		m.On("Attributes", mock.Anything, name).Return(nil, ErrObjectDoesNotExist)
+		m.On("Attributes", mock.Anything, name).Return(objstore.ObjectAttributes{}, ErrObjectDoesNotExist)
 	}
 }
 

pkg/storage/tsdb/users_scanner.go

@@ -7,6 +7,7 @@ package tsdb
 
 import (
 	"context"
+	"math/rand"
 	"strings"
 
 	"github.com/go-kit/log"
@@ -46,6 +47,10 @@ 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,6 +8,7 @@ package tsdb
 import (
 	"context"
 	"errors"
+	"fmt"
 	"path"
 	"testing"
 
@@ -36,10 +37,10 @@ func TestUsersScanner_ScanUsers_ShouldReturnedOwnedUsersOnly(t *testing.T) {
 }
 
 func TestUsersScanner_ScanUsers_ShouldReturnUsersForWhichOwnerCheckOrTenantDeletionCheckFailed(t *testing.T) {
-	expected := []string{"user-1", "user-2"}
+	users := []string{"user-1", "user-2"}
 
 	bucketClient := &bucket.ClientMock{}
-	bucketClient.MockIter("", expected, nil)
+	bucketClient.MockIter("", users, nil)
 	bucketClient.MockExists(path.Join("user-1", TenantDeletionMarkPath), false, nil)
 	bucketClient.MockExists(path.Join("user-2", TenantDeletionMarkPath), false, errors.New("fail"))
 
@@ -50,11 +51,14 @@ func TestUsersScanner_ScanUsers_ShouldReturnUsersForWhichOwnerCheckOrTenantDelet
 	s := NewUsersScanner(bucketClient, isOwned, log.NewNopLogger())
 	actual, deleted, err := s.ScanUsers(context.Background())
 	require.NoError(t, err)
-	assert.Equal(t, expected, actual)
+	for _, userID := range users {
+		assert.Contains(t, actual, userID)
+	}
 	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)
@@ -63,5 +67,32 @@ func TestUsersScanner_ScanUsers_ShouldNotReturnPrefixedUsedByMimirInternals(t *t
 	s := NewUsersScanner(bucketClient, AllUsers, log.NewNopLogger())
 	actual, _, err := s.ScanUsers(context.Background())
 	require.NoError(t, err)
-	assert.Equal(t, []string{"user-1", "user-2"}, actual)
+	for _, userID := range users {
+		assert.Contains(t, actual, userID)
+	}
+}
+
+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)
+	for _, userID := range users {
+		assert.Contains(t, actual, userID)
+	}
+	assert.Empty(t, deleted)
+	assert.NotEqual(t, actual, users)
 }