From ccb676e60cb7441ee65ff7c35f3b7828979101fd Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Fri, 11 Sep 2026 20:23:09 +0800 Subject: [PATCH] fix(storage): preserve shared tier references during pool cleanup Signed-off-by: Feng Ruohang --- cmd/erasure-server-pool-consistency.go | 39 ++++++- cmd/erasure-server-pool-consistency_test.go | 107 ++++++++++++++++++ cmd/object-api-interface.go | 4 +- .../release-readiness-20260911.md | 17 ++- 4 files changed, 160 insertions(+), 7 deletions(-) diff --git a/cmd/erasure-server-pool-consistency.go b/cmd/erasure-server-pool-consistency.go index aa374f775..415f1fa19 100644 --- a/cmd/erasure-server-pool-consistency.go +++ b/cmd/erasure-server-pool-consistency.go @@ -25,6 +25,7 @@ import ( "time" madmin "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/bucket/lifecycle" xhttp "github.com/minio/minio/internal/http" ) @@ -232,6 +233,22 @@ func reconcileStoredObjectTags(metadata, stored map[string]string) { } } +// A restored version still owns its tier reference even while IsRemote is +// false. Only the last copy of a reference may schedule its contents for GC. +func sharesTierObject(oi ObjectInfo, copies []PoolObjInfo) bool { + ref := oi.TransitionedObject + if ref.Status != lifecycle.TransitionComplete { + return false + } + for _, copy := range copies { + other := copy.ObjInfo.TransitionedObject + if other.Status == lifecycle.TransitionComplete && ref.Tier == other.Tier && ref.Name == other.Name && ref.VersionID == other.VersionID { + return true + } + } + return false +} + // retireReplicaCopies runs only after committing a replacement. Failures are // returned to the caller, so a stale copy cannot be hidden behind a successful // response. Data movement owns its source cleanup and does not use this helper. @@ -244,12 +261,25 @@ func (z *erasureServerPools) retireReplicaCopies(ctx context.Context, bucket, ob if err != nil { return err } + var retained []PoolObjInfo for _, copy := range copies { + if copy.Index == keep { + retained = []PoolObjInfo{copy} + break + } + } + if len(retained) == 0 { + return VersionNotFound{Bucket: bucket, Object: decodeDirObject(object), VersionID: versionID} + } + for i, copy := range copies { if copy.Index == keep { continue } _, err := z.serverPools[copy.Index].DeleteObject(ctx, bucket, object, - ObjectOptions{VersionID: versionID, NoLock: true, NoAuditLog: true}) + ObjectOptions{ + VersionID: versionID, NoLock: true, NoAuditLog: true, + SkipFreeVersion: sharesTierObject(copy.ObjInfo, retained) || sharesTierObject(copy.ObjInfo, copies[i+1:]), + }) if err != nil && !isErrObjectNotFound(err) && !isErrVersionNotFound(err) { return err } @@ -304,8 +334,11 @@ func (z *erasureServerPools) deleteObjectConditional(ctx context.Context, bucket // Retire non-authoritative copies first. If cleanup fails, retain the // authoritative version and report the error instead of acknowledging a // deletion that would expose an older copy. - for _, copy := range copies[1:] { - _, err := z.serverPools[copy.Index].DeleteObject(ctx, bucket, object, opts) + for i := 1; i < len(copies); i++ { + candidate := copies[i] + deleteOpts := opts + deleteOpts.SkipFreeVersion = opts.SkipFreeVersion || sharesTierObject(candidate.ObjInfo, copies[:1]) || sharesTierObject(candidate.ObjInfo, copies[i+1:]) + _, err := z.serverPools[candidate.Index].DeleteObject(ctx, bucket, object, deleteOpts) if err != nil && !isErrObjectNotFound(err) && !isErrVersionNotFound(err) { return ObjectInfo{}, err } diff --git a/cmd/erasure-server-pool-consistency_test.go b/cmd/erasure-server-pool-consistency_test.go index 99c117628..5eec0f63a 100644 --- a/cmd/erasure-server-pool-consistency_test.go +++ b/cmd/erasure-server-pool-consistency_test.go @@ -20,6 +20,7 @@ package cmd import ( "bytes" "context" + "errors" "fmt" "io" "maps" @@ -573,3 +574,109 @@ func TestPoolsReplicaCleanupFailureCanRetry(t *testing.T) { t.Errorf("retry left the competing version: %v", err) } } + +func TestPoolsRetiringCopyPreservesSharedTierObject(t *testing.T) { + for _, test := range []struct { + name string + deleting bool + failPrimaryDelete bool + differentRemote bool + restored bool + }{ + {name: "metadata-copy"}, + {name: "restored-metadata-copy", restored: true}, + {name: "metadata-copy-distinct-reference", differentRemote: true}, + {name: "failed-primary-delete", deleting: true, failPrimaryDelete: true}, + {name: "failed-primary-delete-distinct-reference", deleting: true, failPrimaryDelete: true, differentRemote: true}, + {name: "successful-delete", deleting: true}, + } { + t.Run(test.name, func(t *testing.T) { + z, bucket := consistencyPools(t) + const object = "shared-tier-object" + metadata := map[string]string{ + ReservedMetadataPrefixLower + TransitionStatus: "complete", + ReservedMetadataPrefixLower + TransitionTier: "TEST-TIER", + ReservedMetadataPrefixLower + TransitionedObjectName: "shared-remote-object", + ReservedMetadataPrefixLower + TransitionedVersionID: "shared-remote-version", + } + 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}) + 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, + }) + current, err := z.GetObjectInfo(t.Context(), bucket, object, ObjectOptions{VersionID: oi.VersionID}) + if err != nil || current.TransitionedObject.Status != "complete" || current.IsRemote() == test.restored { + t.Fatalf("fixture did not persist the tier reference: %+v, %v", current.TransitionedObject, err) + } + if test.failPrimaryDelete { + // The authoritative copy remains readable if its deletion fails. + // Retiring a secondary copy must not schedule its shared remote + // contents for garbage collection in that case. + set := z.serverPools[0].getHashedSet(object) + getDisks := set.getDisks + faulty := append([]StorageAPI(nil), getDisks()...) + for i := range faulty { + faulty[i] = accessMoveDeleteFaultDisk{StorageAPI: faulty[i], bucket: bucket, object: object, version: oi.VersionID} + } + set.getDisks = func() []StorageAPI { return faulty } + defer func() { set.getDisks = getDisks }() + } + if test.deleting { + _, err := z.DeleteObject(t.Context(), bucket, object, ObjectOptions{ + Versioned: true, VersionID: oi.VersionID, + CheckPrecondFn: func(info ObjectInfo) bool { return info.ETag != current.ETag }, + }) + if (err != nil) != test.failPrimaryDelete { + t.Fatalf("unexpected authoritative delete result: %v", err) + } + } else { + current.metadataOnly = true + current.UserDefined["metadata-update"] = "new" + _, err := z.CopyObject(t.Context(), bucket, object, bucket, object, current, + ObjectOptions{VersionID: oi.VersionID}, ObjectOptions{ + Versioned: true, VersionID: oi.VersionID, MTime: oi.ModTime, ReplicaLockReconcile: true, + }) + if err != nil { + t.Fatal(err) + } + } + _, err = z.serverPools[0].GetObjectInfo(t.Context(), bucket, object, ObjectOptions{VersionID: oi.VersionID}) + primaryDeleted := test.deleting && !test.failPrimaryDelete + if primaryDeleted { + if !isErrVersionNotFound(err) { + t.Fatalf("authoritative copy survived successful delete: %v", err) + } + } else if err != nil { + t.Fatalf("lost retained authoritative copy: %v", err) + } + for pool, wantFree := range []bool{primaryDeleted, test.differentRemote} { + for _, disk := range z.serverPools[pool].getHashedSet(object).getDisks() { + data, err := disk.ReadAll(t.Context(), bucket, pathJoin(object, xlStorageFormatFile)) + if errors.Is(err, errFileNotFound) && !wantFree { + continue + } + if err != nil { + t.Fatal(err) + } + versions, err := getFileInfoVersions(data, bucket, object, false) + if err != nil { + t.Fatal(err) + } + wantCount := 0 + if wantFree { + wantCount = 1 + } + if len(versions.FreeVersions) != wantCount { + t.Fatalf("pool %d has %d tier GC markers, want %d", pool, len(versions.FreeVersions), wantCount) + } + } + } + }) + } +} diff --git a/cmd/object-api-interface.go b/cmd/object-api-interface.go index 681024c00..51b52647f 100644 --- a/cmd/object-api-interface.go +++ b/cmd/object-api-interface.go @@ -139,8 +139,8 @@ type ObjectOptions struct { // when looking up a version by fi.VersionID InclFreeVersions bool // SkipFreeVersion skips adding a free version when a tiered version is - // being 'replaced' - // Note: Used only when a tiered object is being expired. + // being replaced. Used when expiring tiered content or retiring a copy + // whose tier reference is still owned by another copy. SkipFreeVersion bool MetadataChg bool // is true if it is a metadata update operation. diff --git a/docs/investigations/release-readiness-20260911.md b/docs/investigations/release-readiness-20260911.md index 70bee061d..b5fa759c6 100644 --- a/docs/investigations/release-readiness-20260911.md +++ b/docs/investigations/release-readiness-20260911.md @@ -28,6 +28,12 @@ Cleanup failures propagate; a replacement may already have committed when cleanup fails, and a retry can finish cleanup. Data movement keeps ownership of its source cleanup. +Retiring copies preserves any remote-tier reference still held by a surviving +copy, including temporarily restored objects. Only the last copy of that tier +reference schedules remote contents for garbage collection. This also protects +the authoritative copy if the final conditional deletion fails; unrelated tier +contents remain eligible for cleanup. + Conditional DELETE evaluates its precondition against the logical latest or explicitly addressed version. It checks all pools before mutation, removes secondary copies before the authoritative one, and returns cleanup errors. @@ -40,17 +46,24 @@ The deterministic two-pool, 32-drive fixtures cover version selection, duplicate removal, delete failure propagation, PUT/DELETE and completion/DELETE interleavings, independent lock winners, draining/rebalancing owners, null versions, metadata COPY, metadata/healing serialization and cleanup retry. +Tiered-copy tests inspect the persisted garbage-collection markers after +metadata COPY, restored COPY, successful deletion and failed primary deletion, +with distinct remote references as cleanup controls. The original branch reproduced the wrong-version lookup, surviving duplicate, suppressed delete error, PUT/DELETE race, and PUT/completion lock-state failures before the fixes were applied. -Validation completed locally: the full `cmd` suite (255.720 s), all `internal` -tests, `go vet ./...`, generated-file checks and the branding/entrypoint checks. +Before the final tier-reference guard, local validation passed the full `cmd` +suite (255.720 s), all `internal` tests, `go vet ./...`, generated-file checks +and the branding/entrypoint checks. Focused race checks cover the pooled interleavings, SSE-C lock regressions, conditional deletion, access-tier movement and TLS defaults. The two-pool and related replica/delete/movement tests also passed as a Linux/arm64 test binary in an isolated container with an 8 GiB `/tmp` tmpfs. Its initial 1 GiB tmpfs was insufficient for the existing single-drive test fixtures' free-space guard. +The final tier-reference guard passed all six new cases in that Linux fixture; +the retained restart binary below predates this guard and has no remote tier +configured. ## Linux restart/readback (#116)