From 322cd8d2c1ac0574aae9534473e58620ea3666e2 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 11 Nov 2026 00:08:07 +0800 Subject: [PATCH] fix(decom): count lifecycle removals as scan progress Reuse the source-version lookup to count versions removed by ILM before migration cleanup so nested keys are discovered on the next pass. Keep delete-marker-only expiry from reporting physical progress, and cover both the decommission regression and the removal-count boundary. Signed-off-by: Feng Ruohang --- cmd/erasure-server-pool-consistency.go | 13 +++++- ...asure-server-pool-decom-keys-below_test.go | 38 +++++++++++++++++- cmd/erasure-server-pool-migration-ilm_test.go | 40 +++++++++++++++++++ 3 files changed, 88 insertions(+), 3 deletions(-) diff --git a/cmd/erasure-server-pool-consistency.go b/cmd/erasure-server-pool-consistency.go index 79ba0f01c..1d1414023 100644 --- a/cmd/erasure-server-pool-consistency.go +++ b/cmd/erasure-server-pool-consistency.go @@ -272,7 +272,7 @@ func (z *erasureServerPools) applyMigrationExpiry(ctx context.Context, bucket, o // owns replication state and tier cleanup, and it re-evaluates the current // lifecycle rule. A migration cleanup must never turn an asynchronous ILM // decision into an unconditional physical purge. It returns how many source -// versions it deleted. +// versions were removed, including versions deleted by ILM before cleanup. func (z *erasureServerPools) removeMovedVersions(ctx context.Context, bucket, object string, source int, versions []FileInfo, expired []migrationExpiredVersion) (removed int, err error) { if err := z.applyMigrationExpiry(ctx, bucket, object, source, expired); err != nil { return 0, err @@ -290,8 +290,14 @@ func (z *erasureServerPools) removeMovedVersions(ctx context.Context, bucket, ob if vid == "" { vid = nullVersionID } + expiredVersion := isExpiredVersion(expired, version) copies, err := z.objectPoolInfos(ctx, bucket, object, ObjectOptions{VersionID: vid, NoAuditLog: true}) if isErrObjectNotFound(err) || isErrVersionNotFound(err) { + // ILM can remove the parent key before cleanup. Count that as + // progress so the next walk discovers keys below it. + if expiredVersion { + removed++ + } continue } if err != nil { @@ -307,12 +313,15 @@ func (z *erasureServerPools) removeMovedVersions(ctx context.Context, bucket, ob } } if src == nil { + if expiredVersion { + removed++ + } continue } if !src.ModTime.Equal(version.ModTime) || src.DeleteMarker != version.Deleted { return removed, PreConditionFailed{} } - if isExpiredVersion(expired, version) { + if expiredVersion { // The logical expiry path owns this version. Keeping the source is // also the retry point if that path failed or the pool check races it. continue diff --git a/cmd/erasure-server-pool-decom-keys-below_test.go b/cmd/erasure-server-pool-decom-keys-below_test.go index 7435bd6a4..1538511a0 100644 --- a/cmd/erasure-server-pool-decom-keys-below_test.go +++ b/cmd/erasure-server-pool-decom-keys-below_test.go @@ -17,7 +17,10 @@ package cmd -import "testing" +import ( + "testing" + "time" +) // A key below another key ("k/b" under "k") is a directory inside the // directory of that key. The listing treats the directory of an object as a @@ -64,3 +67,36 @@ func TestDecommissionMovesKeysBelowAKey(t *testing.T) { } } } + +// An ILM expiry can remove the parent key during a decommission walk. That +// removal must count as progress, or the next key below the parent remains +// hidden from the listing and the source pool is left non-empty. +func TestDecommissionContinuesAfterLifecycleRemovesParent(t *testing.T) { + z, _ := migrationTestPools(t) + const bucket = "decommission-ilm-keys-below" + if err := z.MakeBucket(t.Context(), bucket, MakeBucketOptions{}); err != nil { + t.Fatal(err) + } + meta, err := globalBucketMetadataSys.Get(bucket) + if err != nil { + t.Fatal(err) + } + meta.LifecycleConfigXML = []byte(`expire-oldEnabled1`) + if err := meta.Save(t.Context(), z); err != nil { + t.Fatal(err) + } + globalBucketMetadataSys.Set(bucket, meta) + + putConsistencyObject(t, z, bucket, "k", 0, "parent", ObjectOptions{MTime: UTCNow().Add(-72 * time.Hour)}) + putConsistencyObject(t, z, bucket, "k/b", 0, "child", ObjectOptions{MTime: UTCNow()}) + + if info := keysBelowDecommission(t, z); !info.Complete || info.Failed { + t.Fatalf("decommission: complete %v, failed %v; want complete", info.Complete, info.Failed) + } + if _, err := z.serverPools[0].GetObjectInfo(t.Context(), bucket, "k/b", ObjectOptions{}); !isErrObjectNotFound(err) { + t.Fatalf("child still in source pool: %v", err) + } + if body := migrationTestBody(t, z.GetObjectNInfo, bucket, "k/b", ""); body != "child" { + t.Fatalf("child reads %q", body) + } +} diff --git a/cmd/erasure-server-pool-migration-ilm_test.go b/cmd/erasure-server-pool-migration-ilm_test.go index da14d2df9..ddd049611 100644 --- a/cmd/erasure-server-pool-migration-ilm_test.go +++ b/cmd/erasure-server-pool-migration-ilm_test.go @@ -414,6 +414,46 @@ func testMigrationLifecycleExpiry(t *testing.T, kind string) { }) } +// Physical expiry advances the walk; creating a delete marker does not remove +// the addressed version and must not make a walk repeat without progress. +func TestPoolsMigrationLifecycleRemovalProgress(t *testing.T) { + for _, versioned := range []bool{false, true} { + status := versioning.State("") + name, wantRemoved := "physical-delete", 1 + if versioned { + status = versioning.Enabled + name, wantRemoved = "delete-marker-only", 0 + } + t.Run(name, func(t *testing.T) { + m := newMigrationILM(t, status, false, migrationILMExpireCurrent()) + oi := m.put("obj-01", "v1", ObjectOptions{Versioned: versioned, MTime: UTCNow().Add(-72 * time.Hour)}) + _, updatedAt, err := globalBucketMetadataSys.GetLifecycleConfig(m.bucket) + if err != nil { + t.Fatal(err) + } + version := FileInfo{Name: oi.Name, VersionID: oi.VersionID, ModTime: oi.ModTime} + removed, err := m.z.removeMovedVersions(t.Context(), m.bucket, oi.Name, 0, []FileInfo{version}, []migrationExpiredVersion{{ + version: version, + event: lifecycle.Event{Action: lifecycle.DeleteAction}, + lifecycleUpdatedAt: updatedAt, + source: lcEventSrc_Decom, + }}) + if err != nil || removed != wantRemoved { + t.Fatalf("removed %d versions, err %v; want %d, nil", removed, err, wantRemoved) + } + if versioned { + if body := migrationTestBody(t, m.z.GetObjectNInfo, m.bucket, oi.Name, oi.VersionID); body != "obj-01 v1" { + t.Fatalf("noncurrent version reads %q", body) + } + } + current, err := m.z.GetObjectInfo(t.Context(), m.bucket, oi.Name, ObjectOptions{}) + if !isErrObjectNotFound(err) || current.DeleteMarker != versioned { + t.Fatalf("after expiry: delete marker %t, err %v", current.DeleteMarker, err) + } + }) + } +} + // A lifecycle action found during the migration walk must not be applied // after the bucket's lifecycle configuration changes. The source copy stays // available for a fresh scan under the new policy.