From 8c1473a618f89fda3b89462c69c2a3c1a320d4c6 Mon Sep 17 00:00:00 2001 From: Denis Babchuk Date: Mon, 2 Nov 2026 19:59:13 +0300 Subject: [PATCH] fix(pools): move versions whose ILM expiry does not hold with the versions in other pools Signed-off-by: Denis Babchuk --- cmd/erasure-server-pool-decom.go | 4 + cmd/erasure-server-pool-lifecycle.go | 29 ++++++++ cmd/erasure-server-pool-lifecycle_test.go | 89 ++++++++++++++++++++++- cmd/erasure-server-pool-rebalance.go | 4 + 4 files changed, 124 insertions(+), 2 deletions(-) diff --git a/cmd/erasure-server-pool-decom.go b/cmd/erasure-server-pool-decom.go index bc5e1c514..19e6a01e8 100644 --- a/cmd/erasure-server-pool-decom.go +++ b/cmd/erasure-server-pool-decom.go @@ -819,6 +819,10 @@ func (z *erasureServerPools) decommissionPool(ctx context.Context, idx int, pool // a noncurrent version and has to be moved like any other. The // scanner applies the expiry once it lives in another pool. return lifecycle.NoneAction + case evt.Action.Delete() && !z.migrationExpiryApplies(ctx, objInfo, evt): + // Another pool holds versions with which the rule does not + // apply. The version moves like any other. + return lifecycle.NoneAction case evt.Action.Delete(): return evt.Action default: diff --git a/cmd/erasure-server-pool-lifecycle.go b/cmd/erasure-server-pool-lifecycle.go index 800b39d92..16079e534 100644 --- a/cmd/erasure-server-pool-lifecycle.go +++ b/cmd/erasure-server-pool-lifecycle.go @@ -250,3 +250,32 @@ func (z *erasureServerPools) expiryNeedsAllPools(opts ObjectOptions) bool { } return opts.DeletePrefix || opts.VersionID != "" || opts.Versioned || opts.VersionSuspended } + +// migrationExpiryApplies reports whether the ILM expiry evt, which a +// decommission or rebalance decided for oi on the versions of the pool it +// walks, applies with the versions of the object in all pools too. Only then +// may the walk leave the version to the expiry instead of moving it. A +// version that is the latest in the walked pool can be a noncurrent version +// of the object, and a delete marker alone in that pool can hide versions in +// another one. Such a version is moved like any other, and the scanner +// applies the rule in the pool that holds the object then. +func (z *erasureServerPools) migrationExpiryApplies(ctx context.Context, oi ObjectInfo, evt lifecycle.Event) bool { + // The options the expiry deletes with, see + // applyExpiryOnNonTransitionedObjects and expireTransitionedObject. + opts := ObjectOptions{ + Expiration: ExpirationOptions{Expire: true}, + Versioned: globalBucketVersioningSys.PrefixEnabled(oi.Bucket, oi.Name), + VersionSuspended: globalBucketVersioningSys.PrefixSuspended(oi.Bucket, oi.Name), + } + if evt.Action.DeleteVersioned() { + opts.VersionID = oi.VersionID + } + if evt.Action.DeleteAll() { + opts.DeletePrefix = true + } + opts.Transition.ExpireRestored = evt.Action.DeleteRestored() + if !z.expiryNeedsAllPools(opts) { + return true + } + return z.checkExpiryAllPools(ctx, oi.Bucket, encodeDirObject(oi.Name), opts) == nil +} diff --git a/cmd/erasure-server-pool-lifecycle_test.go b/cmd/erasure-server-pool-lifecycle_test.go index 0e346e33f..ea39d2819 100644 --- a/cmd/erasure-server-pool-lifecycle_test.go +++ b/cmd/erasure-server-pool-lifecycle_test.go @@ -29,8 +29,8 @@ import ( "github.com/minio/minio/internal/bucket/versioning" ) -// The scanner, the drive heal and a GET or HEAD of a version evaluate ILM on -// the versions of a key in one pool: IsLatest, NumVersions and SuccessorModTime +// The scanner and the walks of decommission and rebalance evaluate ILM on the +// versions of a key in one pool: IsLatest, NumVersions and SuccessorModTime // count only these. A key can have versions in several pools: two concurrent // first writes, overwrites that went to another pool. The expiry they decide // runs in every pool. @@ -414,3 +414,88 @@ func TestPoolsLifecycleSplitObject(t *testing.T) { } }) } + +// walk moves pool 0 out with a decommission or a rebalance pass, as move +// does, but reports a decommission that did not complete instead of stopping +// the test, so the reads are still checked. +func (s *splitILM) walk(kind string) { + s.t.Helper() + ctx := s.t.Context() + if kind == "rebalance" { + migrationTestWalk(s.t, s.z, s.bucket, 0, kind) + s.z.rebalMu.Lock() + s.z.rebalMeta.PoolStats[0].Info.Status = rebalCompleted + s.z.rebalMu.Unlock() + return + } + if err := s.z.StartDecommission(ctx, 0); err != nil { + s.t.Fatal(err) + } + s.z.doDecommissionInRoutine(ctx, 0) + s.z.poolMetaMutex.RLock() + info := *s.z.poolMeta.Pools[0].Decommission + s.z.poolMetaMutex.RUnlock() + if !info.Complete || info.Failed || info.ItemsDecommissionFailed > 0 { + s.t.Errorf("decommission did not complete: complete=%t failed=%t itemsFailed=%d; pool 0 holds %v", + info.Complete, info.Failed, info.ItemsDecommissionFailed, s.view(0)) + } +} + +// Rules that expire a whole object (DelMarkerExpiration, +// ExpiredObjectAllVersions) or a delete marker without noncurrent versions +// (ExpiredObjectDeleteMarker) depend on every version of the object. A walk +// sees the versions in its own pool only. +func TestPoolsMigrationLifecycleWholeObject(t *testing.T) { + recent := 1.0 / 24 + objects := []struct { + name string + versions []splitVersion // oldest first + before string // what a GET reads before the walk + after string // what a GET reads once ILM caught up + want []string // versions in pool 1 once ILM caught up, newest first + }{ + // The marker expires and takes v1 with it. + {"dme-one-pool", []splitVersion{splitAt("v1", 5, 0), splitMarker("dm", 3, 0)}, "-", "-", nil}, + {"dme-older-in-pool-1", []splitVersion{splitAt("v1", 5, 1), splitMarker("dm", 3, 0)}, "-", "-", nil}, + // v2 is the current version, the marker does not expire. + {"dme-newer-in-pool-1", []splitVersion{splitMarker("dm", 3, 0), splitAt("v2", recent, 1)}, "v2", "v2", []string{"v2", "dm"}}, + // v2 expires and takes v1 with it. + {"eav-one-pool", []splitVersion{splitAt("v1", 5, 0), splitAt("v2", 4, 0)}, "v2", "-", nil}, + {"eav-older-in-pool-1", []splitVersion{splitAt("v1", 5, 1), splitAt("v2", 4, 0)}, "v2", "-", nil}, + // v2 is the current version and does not expire. + {"eav-newer-in-pool-1", []splitVersion{splitAt("v1", 5, 0), splitAt("v2", recent, 1)}, "v2", "v2", []string{"v2", "v1"}}, + // The marker hides v1, it is not a marker without noncurrent versions. + {"edm-older-in-pool-1", []splitVersion{splitAt("v1", 5, 1), splitMarker("dm", 3, 0)}, "-", "-", []string{"dm", "v1"}}, + {"keep-marker", []splitVersion{splitAt("v1", 5, 0), splitMarker("dm", 3, 0)}, "-", "-", []string{"dm", "v1"}}, + {"keep-versions", []splitVersion{splitAt("v1", 5, 0), splitAt("v2", 4, 0)}, "v2", "v2", []string{"v2", "v1"}}, + } + for _, kind := range []string{"decommission", "rebalance"} { + t.Run(kind, func(t *testing.T) { + s := newSplitILM(t, versioning.Enabled, splitILMRules) + for _, o := range objects { + for _, v := range o.versions { + s.write(o.name, v) + } + if got := s.get(o.name); got != o.before { + t.Fatalf("%s: fixture reads %s, want %s", o.name, got, o.before) + } + } + s.walk(kind) + for _, o := range objects { + if got := s.get(o.name); got != o.before && got != o.after { + t.Errorf("%s: reads %s after the %s, before ILM caught up; want %s or %s", o.name, got, kind, o.before, o.after) + } + } + s.runQueue() + s.scan(1) + want := map[string][]string{} + for _, o := range objects { + want[o.name] = o.want + if got := s.get(o.name); got != o.after { + t.Errorf("%s: reads %s once ILM caught up, want %s", o.name, got, o.after) + } + } + s.check(want) + }) + } +} diff --git a/cmd/erasure-server-pool-rebalance.go b/cmd/erasure-server-pool-rebalance.go index a23a9eb3d..10a40bd86 100644 --- a/cmd/erasure-server-pool-rebalance.go +++ b/cmd/erasure-server-pool-rebalance.go @@ -605,6 +605,10 @@ func (z *erasureServerPools) rebalanceBucket(ctx context.Context, bucket string, // a noncurrent version and has to be moved like any other. The // scanner applies the expiry once it lives in another pool. return lifecycle.NoneAction + case evt.Action.Delete() && !z.migrationExpiryApplies(ctx, objInfo, evt): + // Another pool holds versions with which the rule does not + // apply. The version moves like any other. + return lifecycle.NoneAction case evt.Action.Delete(): return evt.Action }