diff --git a/CHANGELOG.md b/CHANGELOG.md index 40c730ec6ee..dea8b40d8c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## master / unreleased +* [BUGFIX] Compactor: Fix the final cleanup of a tenant marked for deletion being reported as failed on object stores that return an error when deleting a missing object (GCS, Azure, Swift, OCI). #7861 ## 1.22.0 in progress * [CHANGE] Ruler: Remove the deprecated `-ruler.evaluation-delay-duration` flag and its `ruler_evaluation_delay_duration` per-tenant limit. Use `-ruler.query-offset` / `ruler_query_offset`, which no longer takes the higher of the two values. Cortex decodes the runtime config strictly, so a leftover `ruler_evaluation_delay_duration` override makes the runtime config fail to load: Cortex **exits at startup** (`module failed`, `module=runtime-config`), and on an already-running process every reload fails, pinning the last good overrides and dropping `cortex_runtime_config_last_reload_successful` to 0. Run `grep -r ruler_evaluation_delay_duration` over your runtime configs before upgrading. #7792 diff --git a/pkg/compactor/blocks_cleaner_test.go b/pkg/compactor/blocks_cleaner_test.go index a30a1e665e8..db9e88989eb 100644 --- a/pkg/compactor/blocks_cleaner_test.go +++ b/pkg/compactor/blocks_cleaner_test.go @@ -133,7 +133,9 @@ func TestBlockCleaner_KeyPermissionDenied(t *testing.T) { } func testBlocksCleanerWithOptions(t *testing.T, options testBlocksCleanerOptions) { - bucketClient, _ := cortex_testutil.PrepareFilesystemBucket(t) + // Use an in-memory bucket: the filesystem bucket's Delete also removes emptied parent + // directories, which races with the visit marker heartbeat writing under the same tenant. + bucketClient := objstore.WithNoopInstr(objstore.NewInMemBucket()) // If the markers migration is enabled, then we create the fixture blocks without // writing the deletion marks in the global location, because they will be migrated @@ -224,6 +226,14 @@ func testBlocksCleanerWithOptions(t *testing.T, options testBlocksCleanerOptions require.NoError(t, services.StartAndAwaitRunning(ctx, cleaner)) defer services.StopAndAwaitTerminated(ctx, cleaner) //nolint:errcheck + // The cleanup of each tenant waits for the visit marker heartbeat to delete the cleaner + // visit marker before returning, so none is left once the initial cleanup has completed. + for _, userID := range []string{"user-1", "user-2", "user-3", "user-4", "user-5", "user-6"} { + exists, err := bucketClient.Exists(ctx, path.Join(userID, bucketindex.MarkersPathname, CleanerVisitMarkerName)) + require.NoError(t, err) + assert.False(t, exists, userID) + } + for _, tc := range []struct { path string expectedExists bool @@ -278,6 +288,9 @@ func testBlocksCleanerWithOptions(t *testing.T, options testBlocksCleanerOptions assert.Equal(t, float64(1), prom_testutil.ToFloat64(cleaner.runsStarted.WithLabelValues(activeStatus))) assert.Equal(t, float64(1), prom_testutil.ToFloat64(cleaner.runsCompleted.WithLabelValues(activeStatus))) assert.Equal(t, float64(0), prom_testutil.ToFloat64(cleaner.runsFailed.WithLabelValues(activeStatus))) + assert.Equal(t, float64(1), prom_testutil.ToFloat64(cleaner.runsStarted.WithLabelValues(deletedStatus))) + assert.Equal(t, float64(1), prom_testutil.ToFloat64(cleaner.runsCompleted.WithLabelValues(deletedStatus))) + assert.Equal(t, float64(0), prom_testutil.ToFloat64(cleaner.runsFailed.WithLabelValues(deletedStatus))) assert.Equal(t, float64(7), prom_testutil.ToFloat64(cleaner.blocksCleanedTotal)) assert.Equal(t, float64(0), prom_testutil.ToFloat64(cleaner.blocksFailedTotal)) diff --git a/pkg/util/users/tenant_deletion_mark.go b/pkg/util/users/tenant_deletion_mark.go index 622f1e8bdb1..5c7b1e4970e 100644 --- a/pkg/util/users/tenant_deletion_mark.go +++ b/pkg/util/users/tenant_deletion_mark.go @@ -59,12 +59,13 @@ func ReadTenantDeletionMark(ctx context.Context, bkt objstore.InstrumentedBucket return read(ctx, bkt.WithExpectedErrs(bkt.IsObjNotFoundErr), markerFile, logger) } -// Deletes the tenant deletion mark for given user if it exists. +// Deletes the tenant deletion mark for given user from both the global and the local location, +// if it exists. Not-found errors are ignored for both locations. func DeleteTenantDeletionMark(ctx context.Context, bkt objstore.Bucket, userID string) error { - if err := bkt.Delete(ctx, GetGlobalDeletionMarkPath(userID)); err != nil { + if err := bkt.Delete(ctx, GetGlobalDeletionMarkPath(userID)); err != nil && !bkt.IsObjNotFoundErr(err) { return err } - if err := bkt.Delete(ctx, GetLocalDeletionMarkPath(userID)); err != nil { + if err := bkt.Delete(ctx, GetLocalDeletionMarkPath(userID)); err != nil && !bkt.IsObjNotFoundErr(err) { return err } return nil diff --git a/pkg/util/users/tenant_deletion_mark_test.go b/pkg/util/users/tenant_deletion_mark_test.go index 5da53554260..9c5b0f2ab77 100644 --- a/pkg/util/users/tenant_deletion_mark_test.go +++ b/pkg/util/users/tenant_deletion_mark_test.go @@ -7,6 +7,8 @@ import ( "github.com/stretchr/testify/require" "github.com/thanos-io/objstore" + + "github.com/cortexproject/cortex/pkg/util/testutil" ) func TestTenantDeletionMarkExists(t *testing.T) { @@ -68,3 +70,55 @@ func TestTenantDeletionMarkExists(t *testing.T) { }) } } + +func TestDeleteTenantDeletionMark(t *testing.T) { + const username = "user" + + for name, tc := range map[string]struct { + objects []string + deleteFailures []string + expectedErr string + }{ + "only global mark exists": { + objects: []string{GetGlobalDeletionMarkPath(username)}, + }, + "only local mark exists": { + objects: []string{GetLocalDeletionMarkPath(username)}, + }, + "both marks exist": { + objects: []string{GetGlobalDeletionMarkPath(username), GetLocalDeletionMarkPath(username)}, + }, + "no mark exists": { + objects: nil, + }, + "failure deleting global mark": { + objects: []string{GetGlobalDeletionMarkPath(username)}, + deleteFailures: []string{GetGlobalDeletionMarkPath(username)}, + expectedErr: "mocked delete failure", + }, + "failure deleting local mark": { + objects: []string{GetGlobalDeletionMarkPath(username), GetLocalDeletionMarkPath(username)}, + deleteFailures: []string{GetLocalDeletionMarkPath(username)}, + expectedErr: "mocked delete failure", + }, + } { + t.Run(name, func(t *testing.T) { + // Like GCS, Azure, Swift and OCI, the in-memory bucket returns an error when deleting a missing object. + bkt := objstore.NewInMemBucket() + for _, objName := range tc.objects { + require.NoError(t, bkt.Upload(context.Background(), objName, bytes.NewReader([]byte("data")))) + } + + err := DeleteTenantDeletionMark(context.Background(), &testutil.MockBucketFailure{Bucket: bkt, DeleteFailures: tc.deleteFailures}, username) + if tc.expectedErr != "" { + require.ErrorContains(t, err, tc.expectedErr) + return + } + require.NoError(t, err) + + exists, err := TenantDeletionMarkExists(context.Background(), bkt, username) + require.NoError(t, err) + require.False(t, exists) + }) + } +}