Repository navigation
fix(decom): repeat the walk of a bucket while it removes source versions - #335
Merged
Merged
Conversation
Signed-off-by: Denis Babchuk <babchukdenis@icloud.com>
Signed-off-by: Denis Babchuk <babchukdenis@icloud.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution Licensing (no CLA, inbound=outbound, DCO required)
This pull request contributes to PGSTY SILO (
pgsty/silo). Code contributionsare accepted under AGPL-3.0-or-later, the same license as the server.
This project does not use a CLA or require a separate Apache-2.0 license grant.
By submitting this pull request I represent that I have the right to contribute
the code changes under this repository's
GNU Affero General Public License v3.0 or later
and retain copyright in my original work. Existing copyright and license
notices remain intact; separately licensed material keeps its applicable terms.
Every commit must carry a DCO
Signed-off-bytrailer(
git commit -s) certifying theDeveloper Certificate of Origin — see
CONTRIBUTING.md.
Description
Fixes #334.
A key below another key (
k/bunderk) is a directory inside the directory of that key, and the walk does not descend into the directory of an object. A decommission therefore sawk/bonly afterkhad left the pool. By then the walk was over, so the check after the bucket foundk/band failed the decommission. Each level of keys below a key took one more run.Two commits:
test(decom): cover keys below a key that the walk does not see
cmd/erasure-server-pool-decom-keys-below_test.go,TestDecommissionMovesKeysBelowAKey: two pools, a versioned bucket,k,k/bandk/b/cin pool 0. One decommission of pool 0 must complete, leave none of the keys in pool 0, and read each one with its content.fix(decom): repeat the walk of a bucket while it removes source versions
cmd/erasure-server-pool-consistency.go,removeMovedVersionsalso returns how many source versions it deleted. A version that was already gone, or that is left to the expiry path, is not counted.cmd/erasure-server-pool-decom.go: the walk becomesdecommissionPoolWalk, which returns the number of keys whose source versions it removed.decommissionPoolrepeats it while that number is above zero, the walk returns no error and the context is not canceled.cmd/erasure-server-pool-rebalance.goand three tests ignore the new count. Rebalance is unchanged: a key it does not see stays in its pool, which is not removed.Motivation and Context
The details are in #334. Found while porting the fix for #297 to MinIO
RELEASE.2025-07-23T15-54-02Z. There the source copy is removed with a prefix delete, which loses the keys below the moved key. Our port repeats the walk the same way.How to test this PR?
On main, with the test file of this PR:
With the fix it passes.
repro.shfrom #334 (one local server, two pools of 4 drives):On main
d13899698:With this PR:
Run locally on Go 1.27.3 (darwin/arm64). The new test and
repro.shran on maind13899698. The checks below ran on main8f3657dc5, which has the same Go code:d13899698adds only documentation.make verifiers(golangci-lint: 0 issues, check-gen, rebrand checks),gofmt,go vet ./cmd,make buildgo test ./cmd/ -timeout 40m(534 s):ok; theconsistency_strictcompile; the CI race sets^Test(PoolsLifecycle|PoolsMigrationLifecycle|ExpireRestored|NativeRestore)and^Test(Pools(Delete|BulkDelete|RetiringCopyPreservesSharedTierObject)|BulkDeleteQuorumEvidence):okgo test -race ./cmd -run '^Test(DecommissionMovesKeysBelowAKey|PoolsDecommission|Decommission|PoolMetaDecommission|PoolsMigration)' -count=3:okrepro.shabove on binaries of main and of this branch (CGO_ENABLED=0 go build -tags kqueue -trimpath). After the run with this PR, noxl.metaof the bucket is left on the drives of the first pool.Compatibility impact
No API, configuration, storage metadata, metric, route or dependency changes.
Types of changes
Checklist:
git commit -s) per the DCOcommit-idorPR #here): no, the walk is the same in MinIORELEASE.2025-07-23T15-54-02Zmake verifierspassesgo test ./cmd, the decommission, migration and lifecycle race sets andmake buildpasspgsty/silo.pgsty.com, if needed (not needed)