deletion: fix log deletion with line filters #8151

sandeepsukhani · 2023-01-16T08:07:42Z

What this PR does / why we need it:
When a delete request is issued with a line filter, the current deletion code always assumes that the chunk would be re-written with a new id and marks the existing chunk for deletion. However, if no lines match the line filter, then the chunk is not changed. This causes us to delete the whole chunk unexpectedly.

This PR takes care of the issue by detecting if there was any data deleted from the chunk. If there is no change in the chunk then it should be a no-op.
Additionally, I have refactored the code to simplify partial chunk deletion by accepting time.Time in filter.Func. This way we do not have to calculate the chunk remainders beforehand and use this filter.Func to decide whether the log line is supposed to be deleted or not while iterating through the logs in the chunk. This should not add much overhead since the chunk slicer anyways iterates through the log lines for building a new chunk.

I have also improved the integration tests to cover all the deletion modes and test the deletion with/without query time filtering.

Checklist

Tests updated
CHANGELOG.md updated

…en no lines match

grafanabot · 2023-01-16T09:02:11Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

integration/loki_micro_services_delete_test.go

MichelHollands · 2023-01-17T17:30:23Z

pkg/storage/stores/indexshipper/compactor/retention/retention.go

@@ -332,75 +339,85 @@ func newChunkRewriter(chunkClient client.Client, tableName string, chunkIndexer
 	}
 }

-func (c *chunkRewriter) rewriteChunk(ctx context.Context, ce ChunkEntry, tableInterval model.Interval, intervalFilters []IntervalFilter) (bool, error) {
+func (c *chunkRewriter) rewriteChunk(ctx context.Context, ce ChunkEntry, tableInterval model.Interval, filterFunc filter.Func) (bool, bool, error) {


Can you add a comment that explains the return values? It's only at the end of this long function that you can see what they mean.

MasslessParticle

This is way easier to follow. LGTM with a couple of comments

MasslessParticle · 2023-01-17T22:55:27Z

pkg/storage/stores/indexshipper/compactor/deletion/delete_request.go

-				},
-				Filter: ff,
-			})
+	// delete request is without a line filter so initialize it to always return true to simplify the code below for partial chunk deletion.


This is maybe stylistic:

Because everything is derived from a DeleteRequest, I'd expect everything from lines 146 -> 168 to be par of d.FilterFunction. It would definitely simplify this function and keep everything associated with creating a "correct" filter function together.

wdyt?

MasslessParticle · 2023-01-17T23:06:13Z

pkg/storage/stores/indexshipper/compactor/retention/retention.go

@@ -332,75 +339,85 @@ func newChunkRewriter(chunkClient client.Client, tableName string, chunkIndexer
 	}
 }

-func (c *chunkRewriter) rewriteChunk(ctx context.Context, ce ChunkEntry, tableInterval model.Interval, intervalFilters []IntervalFilter) (bool, error) {
+func (c *chunkRewriter) rewriteChunk(ctx context.Context, ce ChunkEntry, tableInterval model.Interval, filterFunc filter.Func) (bool, bool, error) {


grafanabot · 2023-01-30T06:08:16Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2023-01-30T06:23:04Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2023-01-30T06:35:08Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2023-01-30T07:20:44Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

MichelHollands

LGTM

grafanabot · 2023-02-23T19:36:55Z

The backport to release-2.7.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-8151-to-release-2.7.x origin/release-2.7.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x b2d0481cbeee17355a4305ba48b7517c0cdea080
# Push it to GitHub
git push --set-upstream origin backport-8151-to-release-2.7.x
git switch main
# Remove the local backport branch
git branch -D backport-8151-to-release-2.7.x

Then, create a pull request where the base branch is release-2.7.x and the compare/head branch is backport-8151-to-release-2.7.x.

(cherry picked from commit b2d0481)

sandeepsukhani added 2 commits January 16, 2023 13:18

fix handling of processing whole chunks for deletion with a filter wh…

f34661e

…en no lines match

improve integration tests for deletion

7a2a8fb

sandeepsukhani requested a review from a team as a code owner January 16, 2023 08:07

sandeepsukhani assigned MasslessParticle and MichelHollands Jan 16, 2023

pull-request-size bot added the size/XXL label Jan 16, 2023

sandeepsukhani added 2 commits January 16, 2023 14:18

lint

649ef2e

lint

f5f7c4b

sandeepsukhani unassigned MasslessParticle and MichelHollands Jan 16, 2023

sandeepsukhani requested review from MasslessParticle and MichelHollands January 16, 2023 08:57

MichelHollands reviewed Jan 17, 2023

View reviewed changes

MasslessParticle approved these changes Jan 17, 2023

View reviewed changes

MichelHollands approved these changes Jan 18, 2023

View reviewed changes

changes suggested from PR review

d543511

Merge branch 'main' into fix-chunk-deletion-with-filters

269f0e3

sandeepsukhani changed the title ~~fix chunk deletion with filters~~ deletion: fix log deletion with line filters Jan 30, 2023

update changelog

9c3f421

sandeepsukhani requested review from MasslessParticle and MichelHollands January 30, 2023 08:37

MichelHollands approved these changes Jan 30, 2023

View reviewed changes

MasslessParticle approved these changes Jan 30, 2023

View reviewed changes

MasslessParticle merged commit b2d0481 into grafana:main Jan 30, 2023

DylanGuedes added the type/bug label Feb 23, 2023

DylanGuedes added the backport release-2.7.x label Feb 23, 2023

grafanabot added the backport-failed label Feb 23, 2023

DylanGuedes pushed a commit that referenced this pull request Feb 23, 2023

deletion: fix log deletion with line filters (#8151)

3ef99f2

(cherry picked from commit b2d0481)

DylanGuedes mentioned this pull request Feb 23, 2023

[release-2.7.x] deletion: fix log deletion with line filters #8611

Closed

DylanGuedes mentioned this pull request Feb 23, 2023

[release-2.7.x] deletion: fix log deletion with line filters (#8151) #8612

Closed

DylanGuedes pushed a commit that referenced this pull request Feb 23, 2023

deletion: fix log deletion with line filters (#8151)

64ef980

(cherry picked from commit b2d0481)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deletion: fix log deletion with line filters #8151

deletion: fix log deletion with line filters #8151

sandeepsukhani commented Jan 16, 2023 •

edited

Loading

grafanabot commented Jan 16, 2023

MichelHollands Jan 17, 2023

MasslessParticle Jan 17, 2023

MasslessParticle left a comment

MasslessParticle Jan 17, 2023

MasslessParticle Jan 17, 2023

grafanabot commented Jan 30, 2023

grafanabot commented Jan 30, 2023

grafanabot commented Jan 30, 2023

grafanabot commented Jan 30, 2023

MichelHollands left a comment

grafanabot commented Feb 23, 2023

deletion: fix log deletion with line filters #8151

deletion: fix log deletion with line filters #8151

Conversation

sandeepsukhani commented Jan 16, 2023 • edited Loading

grafanabot commented Jan 16, 2023

MichelHollands Jan 17, 2023

Choose a reason for hiding this comment

MasslessParticle Jan 17, 2023

Choose a reason for hiding this comment

MasslessParticle left a comment

Choose a reason for hiding this comment

MasslessParticle Jan 17, 2023

Choose a reason for hiding this comment

MasslessParticle Jan 17, 2023

Choose a reason for hiding this comment

grafanabot commented Jan 30, 2023

grafanabot commented Jan 30, 2023

grafanabot commented Jan 30, 2023

grafanabot commented Jan 30, 2023

MichelHollands left a comment

Choose a reason for hiding this comment

grafanabot commented Feb 23, 2023

sandeepsukhani commented Jan 16, 2023 •

edited

Loading