From a2073d25142ced87b9bde8afa38aef2d0383768e Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sat, 24 Oct 2026 15:47:51 +0800 Subject: [PATCH] fix(pools): preserve shared tier references during failed deletes Reuse deleteObjectReconciled for tiered bulk entries and unversioned single deletes. Its reference-aware cleanup avoids scheduling remote contents for garbage collection while another copy still needs them, including restored copies. Keep the normal local-data bulk path unchanged. Extend existing tier GC regressions to bulk and unversioned deletes and include them in the multi-pool race CI step. Signed-off-by: Feng Ruohang --- .github/workflows/go.yml | 2 +- cmd/erasure-server-pool-consistency.go | 27 +++++++++++++++++- cmd/erasure-server-pool-consistency_test.go | 31 +++++++++++++++++---- cmd/erasure-server-pool.go | 6 ++++ 4 files changed, 59 insertions(+), 7 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 733cfa762..1c5f5295d 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -94,7 +94,7 @@ jobs: run: go test -race ./cmd -run '^Test(PoolsConditionalPut|SinglePoolConditionalPutHTTP)' -count=1 -timeout=5m - name: Run multi-pool delete consistency tests under race detector - run: go test -race ./cmd -run '^Test(Pools(Delete|BulkDelete)|BulkDeleteQuorumEvidence)' -count=1 -timeout=5m + run: go test -race ./cmd -run '^Test(Pools(Delete|BulkDelete|RetiringCopyPreservesSharedTierObject)|BulkDeleteQuorumEvidence)' -count=1 -timeout=5m - name: Run multipart listing, cancellation and cleanup tests under race detector env: diff --git a/cmd/erasure-server-pool-consistency.go b/cmd/erasure-server-pool-consistency.go index 322eeae69..776e1e4cc 100644 --- a/cmd/erasure-server-pool-consistency.go +++ b/cmd/erasure-server-pool-consistency.go @@ -542,6 +542,7 @@ func (z *erasureServerPools) deleteObjectsReconciled(ctx context.Context, bucket deleted := make([]DeletedObject, len(objects)) errs := make([]error, len(objects)) primary := make([]int, len(objects)) + tiered := make([]bool, len(objects)) lookup := errgroup.WithNErrs(len(objects)).WithConcurrency(32) for i, obj := range objects { // Preserve the full request identity even if lookup or cleanup fails. @@ -556,6 +557,9 @@ func (z *erasureServerPools) deleteObjectsReconciled(ctx context.Context, bucket copies, err := z.objectPoolInfos(ctx, bucket, obj.ObjectName, ObjectOptions{VersionID: obj.VersionID, NoAuditLog: true}) if err == nil { primary[i] = copies[0].Index + for _, copy := range copies { + tiered[i] = tiered[i] || copy.ObjInfo.TransitionedObject.Status == lifecycle.TransitionComplete + } } else if !isErrObjectNotFound(err) && !isErrVersionNotFound(err) { if isErrReadQuorum(err) { err = InsufficientWriteQuorum{Bucket: bucket, Object: decodeDirObject(obj.ObjectName)} @@ -576,7 +580,7 @@ func (z *erasureServerPools) deleteObjectsReconciled(ctx context.Context, bucket for phase := 0; phase < 2; phase++ { batches := make([]poolBatch, len(z.serverPools)) for i, obj := range objects { - if errs[i] != nil { + if errs[i] != nil || tiered[i] { continue } for p := range z.serverPools { @@ -614,5 +618,26 @@ func (z *erasureServerPools) deleteObjectsReconciled(ctx context.Context, bucket } } } + // Tiered copies can share one remote object, including restored copies. + // Reuse the existing reconciliation which suppresses premature tier GC. + // Serializing this uncommon path also keeps duplicate key/version entries + // in one request from racing their metadata updates under the shared lock. + for i, obj := range objects { + if !tiered[i] || errs[i] != nil { + continue + } + deleteOpts := opts + deleteOpts.VersionID, deleteOpts.NoLock = obj.VersionID, true + deleteOpts.DeleteReplication = obj.ReplicationState() + if opts.PrefixEnabledFn != nil { + deleteOpts.Versioned = opts.PrefixEnabledFn(obj.ObjectName) + } + _, err := z.deleteObjectReconciled(ctx, bucket, obj.ObjectName, deleteOpts) + if err != nil && !isErrObjectNotFound(err) && !isErrVersionNotFound(err) { + errs[i] = err + } else { + deleted[i].found = true + } + } return deleted, errs } diff --git a/cmd/erasure-server-pool-consistency_test.go b/cmd/erasure-server-pool-consistency_test.go index 8b21dbc9e..0dec71fd5 100644 --- a/cmd/erasure-server-pool-consistency_test.go +++ b/cmd/erasure-server-pool-consistency_test.go @@ -1071,6 +1071,8 @@ func TestPoolsRetiringCopyPreservesSharedTierObject(t *testing.T) { restored bool unconditional bool skipFreeVersion bool + bulk bool + unversioned bool }{ {name: "metadata-copy"}, {name: "restored-metadata-copy", restored: true}, @@ -1081,6 +1083,14 @@ func TestPoolsRetiringCopyPreservesSharedTierObject(t *testing.T) { {name: "ordinary-failed-primary-delete", deleting: true, unconditional: true, failPrimaryDelete: true}, {name: "ordinary-successful-delete", deleting: true, unconditional: true}, {name: "ordinary-skip-free-version", deleting: true, unconditional: true, skipFreeVersion: true}, + {name: "bulk-failed-primary-delete", deleting: true, bulk: true, failPrimaryDelete: true}, + {name: "bulk-restored-failed-primary-delete", deleting: true, bulk: true, restored: true, failPrimaryDelete: true}, + {name: "bulk-successful-delete", deleting: true, bulk: true}, + {name: "bulk-distinct-reference", deleting: true, bulk: true, differentRemote: true}, + {name: "bulk-skip-free-version", deleting: true, bulk: true, skipFreeVersion: true}, + {name: "unversioned-failed-primary-delete", deleting: true, unconditional: true, unversioned: true, failPrimaryDelete: true}, + {name: "unversioned-successful-delete", deleting: true, unconditional: true, unversioned: true}, + {name: "bulk-unversioned-failed-primary-delete", deleting: true, bulk: true, unversioned: true, failPrimaryDelete: true}, } { t.Run(test.name, func(t *testing.T) { z, bucket := consistencyPools(t) @@ -1094,13 +1104,13 @@ func TestPoolsRetiringCopyPreservesSharedTierObject(t *testing.T) { if test.restored { metadata[xhttp.AmzRestore] = completedRestoreObj(time.Now().Add(time.Hour)).String() } - oi := putConsistencyObject(t, z, bucket, object, 0, "data", ObjectOptions{Versioned: true, UserDefined: metadata}) + oi := putConsistencyObject(t, z, bucket, object, 0, "data", ObjectOptions{Versioned: !test.unversioned, UserDefined: metadata}) secondaryMetadata := maps.Clone(metadata) if test.differentRemote { secondaryMetadata[ReservedMetadataPrefixLower+TransitionedObjectName] = "other-remote-object" } putConsistencyObject(t, z, bucket, object, 1, "data", ObjectOptions{ - Versioned: true, VersionID: oi.VersionID, MTime: oi.ModTime, UserDefined: secondaryMetadata, + Versioned: !test.unversioned, VersionID: oi.VersionID, MTime: oi.ModTime, UserDefined: secondaryMetadata, }) current, err := z.GetObjectInfo(t.Context(), bucket, object, ObjectOptions{VersionID: oi.VersionID}) if err != nil || current.TransitionedObject.Status != "complete" || current.IsRemote() == test.restored { @@ -1113,7 +1123,11 @@ func TestPoolsRetiringCopyPreservesSharedTierObject(t *testing.T) { set := z.serverPools[0].getHashedSet(object) getDisks := set.getDisks faulty := append([]StorageAPI(nil), getDisks()...) + var failed, attempted atomic.Int32 for i := range faulty { + if test.bulk { + faulty[i] = bulkDeleteFaultDisk{StorageAPI: faulty[i], bucket: bucket, object: object, mode: "eio", fail: true, failed: &failed, attempted: &attempted} + } faulty[i] = consistencyDeleteFaultDisk{StorageAPI: faulty[i], bucket: bucket, object: object, version: oi.VersionID} } set.getDisks = func() []StorageAPI { return faulty } @@ -1121,13 +1135,20 @@ func TestPoolsRetiringCopyPreservesSharedTierObject(t *testing.T) { } if test.deleting { opts := ObjectOptions{ - Versioned: true, VersionID: oi.VersionID, SkipFreeVersion: test.skipFreeVersion, + Versioned: !test.unversioned, VersionID: oi.VersionID, SkipFreeVersion: test.skipFreeVersion, CheckPrecondFn: func(info ObjectInfo) bool { return info.ETag != current.ETag }, } if test.unconditional { opts.CheckPrecondFn = nil } - _, err := z.DeleteObject(t.Context(), bucket, object, opts) + var err error + if test.bulk { + opts.CheckPrecondFn = nil + _, errs := z.DeleteObjects(t.Context(), bucket, []ObjectToDelete{{ObjectV: ObjectV{ObjectName: object, VersionID: oi.VersionID}}}, opts) + err = errs[0] + } else { + _, err = z.DeleteObject(t.Context(), bucket, object, opts) + } if (err != nil) != test.failPrimaryDelete { t.Fatalf("unexpected authoritative delete result: %v", err) } @@ -1145,7 +1166,7 @@ func TestPoolsRetiringCopyPreservesSharedTierObject(t *testing.T) { _, err = z.serverPools[0].GetObjectInfo(t.Context(), bucket, object, ObjectOptions{VersionID: oi.VersionID}) primaryDeleted := test.deleting && !test.failPrimaryDelete if primaryDeleted { - if !isErrVersionNotFound(err) { + if !isErrVersionNotFound(err) && (!test.unversioned || !isErrObjectNotFound(err)) { t.Fatalf("authoritative copy survived successful delete: %v", err) } } else if err != nil { diff --git a/cmd/erasure-server-pool.go b/cmd/erasure-server-pool.go index 671f6fd47..a75af7339 100644 --- a/cmd/erasure-server-pool.go +++ b/cmd/erasure-server-pool.go @@ -39,6 +39,7 @@ import ( "github.com/minio/minio-go/v7/pkg/set" "github.com/minio/minio-go/v7/pkg/tags" "github.com/minio/minio/internal/bpool" + "github.com/minio/minio/internal/bucket/lifecycle" "github.com/minio/minio/internal/cachevalue" "github.com/minio/minio/internal/config/storageclass" xioutil "github.com/minio/minio/internal/ioutil" @@ -1357,6 +1358,11 @@ func (z *erasureServerPools) DeleteObject(ctx context.Context, bucket string, ob // Retire older copies before the newest copy for unversioned objects. if len(noReadQuorumPools) > 0 && !opts.Versioned && !opts.VersionSuspended { + if pinfo.ObjInfo.TransitionedObject.Status == lifecycle.TransitionComplete { + // Restored and remote copies can share a tier object. Reuse the + // reconciliation which keeps that reference out of premature GC. + return z.deleteObjectReconciled(ctx, bucket, object, opts) + } return z.deleteObjectFromAllPools(ctx, bucket, object, opts, noReadQuorumPools) }