Repository navigation
fix(ilm): expire a restored copy by the version it belongs to - #282
Open
botdeniska wants to merge 1 commit into
Open
botdeniska wants to merge 1 commit into
botdeniska wants to merge 1 commit into
Conversation
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 #281.
When the restore of the current version of a transitioned object expires,
Lifecycle.EvalreturnsDeleteRestoredAction, andexpireTransitionedObjectcalledDeleteObjectfor it without a version ID. The delete then applied to the object, not to the restored copy: a versioned bucket got a delete marker that hid the object, and with versioning suspended the delete went to thenullversion, removing its data if there was one and failing withVersionNotFoundif not.DeleteRestoredVersionAction, for noncurrent versions, already passed the version ID and was not affected.One commit:
fix(ilm): expire a restored copy by the version it belongs to
expireTransitionedObject(cmd/bucket-lifecycle.go) passes the version ID of the restored version in itsDeleteRestored()branch for both actions. An empty version ID stands for thenullversion and is sent asnull: the ListObjectsmax-keys=1shortcut reads the latest version without the bucket's versioning, so anullversion reaches the expiry with an empty ID even in a versioned bucket. With a version ID,DeleteObjecttakes the path the noncurrent case already takes: the restore headers of that version are dropped and its data directory is removed. Nothing else in the branch changes.cmd/bucket-lifecycle-restore_test.go,TestExpireRestoredCopy. It builds the state through the object layer: it writes the versions, transitions one to an in-memory tier withTransitionObject, restores it withRestoreTransitionedObject, and moves the expiry date inx-amz-restoreone day into the past. Then it runs the expiry along each path that applies ILM: the scanner (applyActions, then an expiry worker), a HEAD (applyExpiryRule), ListObjects withmax-keys=1plus ListObjectVersions, and the synchronous call of decommission and drive heal (applyExpiryOnTransitionedObject). Seven layouts (versioning enabled, suspended or off; restored current, noncurrent ornullversion; suspended with and without anullversion under the current one), on one pool and on two. It checks that the versions in each pool are unchanged and there are no free versions, that a GET of the object returns the same version, that the restored data directory is gone from every drive, that the version is still transitioned withoutx-amz-restore, that a GET of it reads the tier, and that every other version still reads its own data.The test restores through
z.serverPools[0]. With more than one pool,erasureServerPools.RestoreTransitionedObjectholds the object lock, and thePutObjectof the restored copy insideerasureObjects.restoreTransitionedObjectwaits for that same lock (putRestoreOptsdoes not setNoLock). That is a separate problem and not addressed here.Motivation and Context
A restore gives a transitioned object a local copy for a while. Once it expired, the object looked deleted: GET and HEAD returned 404
NoSuchKey, and the delete marker stayed. A plain GET or HEAD after the expiry date was enough, since both queue the expiry and still serve the object once. With versioning suspended, an oldernullversion could lose its data, or the restored copy stayed on the drives for good.This does not overlap #240: that change leaves the expiry of restored copies local and touches only the branch for transitioned objects below it.
How to test this PR?
On main, with this test file (
git checkout <this branch> -- cmd/bucket-lifecycle-restore_test.go), 32 of 56 cases fail, for every path and both pool counts: versioning enabled with a restored current version ornullversion, and versioning suspended with a restored current version, with and without anullversion under it. For example:The cases that pass on main too (a restored noncurrent version, a restored
nullcurrent version with versioning suspended, an unversioned bucket) guard the paths that already worked. With the fix all 56 pass. Without thenullmapping onlyversioned-null/listfails, on both pool counts.Run locally on Go 1.27.3 (darwin/arm64), on main
0fc5a0b52:make verifiers(golangci-lint v2.15.0: 0 issues, check-gen, rebrand checks)go vet ./cmd,go test ./internal/bucket/lifecycle -count=1./cmd:go test ./cmd -count=1 -run '^Test(.*Lifecycle.*|.*Restore.*|.*Transition.*|.*Tier.*|.*Scanner.*|DataUsage.*|Apply.*|EvalAction.*|.*ILM.*|.*Expir.*|.*FreeVersion.*|.*DelMarker.*|Pools.*|Migration.*|ObjectIsRemote)$':ok. I did not run the whole./cmdsuite.make buildNo live reproduction: the server sets a restore to expire at UTC midnight at least a day ahead, and the API cannot move that date back, so the test moves it instead.
Compatibility impact
No API, configuration, storage metadata, metric, route or dependency changes.
DELETE ?versionId=.nullversion when the current version has an ID. Restored copies that stayed because the expiry failed are removed on the next pass.Types of changes
Checklist:
git commit -s) per the DCOcommit-idorPR #here): no. The branch has never passed a version ID forDeleteRestoredAction; the code is the same in MinIORELEASE.2025-07-23T15-54-02Zand minio/minio mastermake verifierspassesmake buildpasspgsty/silo.pgsty.com, if needed (not needed)