Skip to content

fix(compactor): Delete chunks marked for deletion when compactor leadership changes - #23547

Open
locker95 wants to merge 1 commit into
grafana:mainfrom
locker95:fix/retention-markers-object-storage-11119
Open

fix(compactor): Delete chunks marked for deletion when compactor leadership changes#23547
locker95 wants to merge 1 commit into
grafana:mainfrom
locker95:fix/retention-markers-object-storage-11119

Conversation

@locker95

Copy link
Copy Markdown

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.start is called only from the leader branch of Compactor.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. newMarkerReader creates one context per processor and Stop cancels it, so once an instance has lost the leader election a later Start returns 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:

  1. marker files should go to object storage
  2. all compactors could run sweepers to make sure they cleanup any marker files they have even if they aren't the elected leader

Option 1 already landed in #19689: -compactor.deletion-marker-object-store-prefix stores 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, where backend runs 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_prefix is 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_delay semantics are unchanged: eligibility is still decided by markerProcessor.availableFiles from 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:

  • No configuration change, no new option, no default value change.
  • Marker file format and on-disk layout are untouched, so a downgrade is safe: markers written before or after this change are read identically by either version.
  • Markers already sitting on the local disk of a former leader when the new version starts are picked up on startup. That backlog is exactly what the fix drains, so no migration step is needed. The pre-existing retention.CopyMarkers migration from the legacy retention/markers and retention/<store>/markers directories into the period-specific directories is left as is.
  • Deployments already using deletion_marker_object_store_prefix keep their current behaviour, minus the restart bug.
  • No upgrade guide entry, since no operator action is required. Happy to add one if you would rather call the behaviour change out explicitly.
  • One consequence worth flagging: instances that are not the leader now emit the existing no marks file found sweeper 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 runs Compactor.loop with 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=1 passes, as does -race on ./pkg/compactor/ ./pkg/compactor/retention/; go build ./cmd/loki and gofmt are clean.

The docs change mentions -compactor.deletion-marker-object-store-prefix on the retention page next to the existing persistent-disk recommendation, since that option was not referenced there at all.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md — n/a, no operator action required
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory — n/a
…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>
@locker95
locker95 requested a review from a team as a code owner July 25, 2026 19:37

@JStickler JStickler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[docs team] Docs LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants