fix(compactor): Delete chunks marked for deletion when compactor leadership changes - #23547
Open
locker95 wants to merge 1 commit into
Open
fix(compactor): Delete chunks marked for deletion when compactor leadership changes#23547locker95 wants to merge 1 commit into
locker95 wants to merge 1 commit into
Conversation
…ership changes Retention and delete requests are applied in two phases: the compaction removes the chunk references from the index and records them in a marker file, and a sweeper deletes those chunks from the object store once retention_delete_delay (2 hours by default) has elapsed. Marker files are written to the local disk of whichever instance currently wins the compactor ring leader election, and only the elected instance runs the sweeper. In SSD mode the leadership can move to another instance inside the delay window, and because markers on local disk are not visible to the other instances, the chunks they reference are never deleted from the object store. Besides the wasted storage this makes delete requests silently ineffective even though the delete API reported success. Every instance now runs the chunk sweeper for the markers on its own disk, regardless of whether it currently owns the compaction, which is the second solution listed in the issue. Markers stored in the object store through compactor.deletion-marker-object-store-prefix keep being swept by the elected instance only, since they are already visible to every instance and sweeping them everywhere would duplicate the deletes. This also fixes the marker processor so that it can be restarted: Stop cancelled a context created once per processor, so an instance that lost and then regained the leader election never resumed sweeping. Signed-off-by: Dean Chen <862469039@qq.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.
What this PR does / why we need it:
Retention and delete requests are applied in two phases. The compaction removes the chunk references from the index and records them in a marker file; a sweeper then deletes those chunks from the object store once
retention_delete_delay(2 hours by default) has elapsed.Marker files are written to the local disk of whichever instance currently wins the compactor ring leader election, and only the elected instance runs the sweeper (
tablesManager.startis called only from the leader branch ofCompactor.loop). In SSD mode the leadership can move to another instance inside the delay window, and markers on local disk are not visible to the other instances, so the chunks they reference are never deleted from the object store.As @slim-bean notes in #11119, this is not only wasted object storage. Chunks marked because of a delete request are dropped from the index but never removed from storage, so a delete request submitted through the delete API silently fails to delete the data even though the API reported success — which matters for anyone relying on the API to satisfy a data deletion obligation. @Perdjesk's comment on the issue reports the same failure mode reachable from the default Helm SSD values.
While digging into this I also found a second, sharper failure: the marker processor cannot be restarted.
newMarkerReadercreates one context per processor andStopcancels it, so once an instance has lost the leader election a laterStartreturns immediately (r.ctx.Err() != nil) and that instance never sweeps again for the rest of the process lifetime — not even while it is the elected leader. This makes the "unless that node becomes the leader elected compactor again" escape hatch mentioned in the issue ineffective, and it fits @duj4's observation on the issue of a compactor service starting again after a stop.Which issue(s) this PR fixes:
Fixes #11119
Solution and trade-offs:
The issue lists two solutions:
Option 1 already landed in #19689:
-compactor.deletion-marker-object-store-prefixstores markers under an object storage prefix, and existing local markers are copied over on startup. It is opt-in and empty by default, so the default configuration — including the default Helm SSD values, wherebackendruns the compactor with 3 replicas — is still affected.This PR implements option 2 for that default. Each instance now runs the chunk sweeper for the markers on its own disk regardless of whether it currently owns the compaction, so a former leader finishes deleting the chunks it marked.
Deliberately, this applies only when markers are on local disk. When
deletion_marker_object_store_prefixis set the markers live in one shared prefix that every instance can already see, so sweeping continues to run on the elected instance only. Sweeping a shared prefix from every instance would duplicate the object storage deletes and let two instances race over the same marker file, whereas local markers are private to their instance by construction, so there is no contention.I did not change the default of
deletion_marker_object_store_prefix. Flipping it would change the storage layout and the object storage request profile of every existing deployment, which seems like a maintainer call rather than something to slip into a bug fix. Happy to follow up separately if you would like that instead.retention_delete_delaysemantics are unchanged: eligibility is still decided bymarkerProcessor.availableFilesfrom the marker file timestamp, so the window that lets index gateways refresh their index copies before chunks disappear is preserved. The sweep just happens on the instance holding the marker instead of nowhere.Backwards compatibility / upgrade:
retention.CopyMarkersmigration from the legacyretention/markersandretention/<store>/markersdirectories into the period-specific directories is left as is.deletion_marker_object_store_prefixkeep their current behaviour, minus the restart bug.no marks file foundsweeper log line once a minute per period. It is a local directory listing, so the cost is negligible, but the line was previously logged only by the leader. Let me know if you would prefer it downgraded to debug.Special notes for your reviewer:
Two tests, both verified to fail before the change:
Test_markerProcessor_Restart— processes a mark, stops the processor the way a lost leader election does, writes another mark and starts it again. Before the fix the second mark is never processed (Condition never satisfied).TestCompactor_SweepsOwnMarkersWithoutLeadership— writes a chunk and a marker for it, then runsCompactor.loopwith a ring that never elects this instance, and asserts the chunk is removed from the object store. Before the fix the chunk survives (chunk marked for deletion was not swept).Verification:
go test ./pkg/compactor/... -count=1passes, as does-raceon./pkg/compactor/ ./pkg/compactor/retention/;go build ./cmd/lokiandgofmtare clean.The docs change mentions
-compactor.deletion-marker-object-store-prefixon the retention page next to the existing persistent-disk recommendation, since that option was not referenced there at all.Checklist
CONTRIBUTING.mdguide (required)docs/sources/setup/upgrade/_index.md— n/a, no operator action requireddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory — n/a