Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compactor: add per tenant compaction delete enabled flag #6410

Merged

Conversation

MichelHollands
Copy link
Contributor

@MichelHollands MichelHollands commented Jun 16, 2022

Signed-off-by: Michel Hollands michel.hollands@grafana.com

What this PR does / why we need it:

This PR adds a per tenant compaction delete enabled flag called compactor_deletion_enabled. A unit test was added for the http middleware that checks access.

This was also manually tested. The following snippet was added in the Loki config file:

limits_config:
  compactor_deletion_enabled: false

The runtime config contained the following:

overrides:
  tenant1:
    compactor_deletion_enabled: true
  tenant2:
    compactor_deletion_enabled: false

Running curl commands against the API gives the expected results:

% curl  http://localhost:3100/loki/api/v1/delete
no org id
% curl -H "X-Scope-OrgID: tenant1"  http://localhost:3100/loki/api/v1/delete
[]
% curl -H "X-Scope-OrgID: tenant2"  http://localhost:3100/loki/api/v1/delete
access denied
% curl -H "X-Scope-OrgID: tenant3"  http://localhost:3100/loki/api/v1/delete
[]

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/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%

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/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.3%
+            querier	0%
- querier/queryrange	-0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@MasslessParticle
Copy link
Contributor

MasslessParticle commented Jun 16, 2022

Would it be cleaner to just make a middleware to wrap our handlers?

If we took that approach, we wouldn't have to implement this same check in every handler.

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 17, 2022
@MichelHollands
Copy link
Contributor Author

Would it be cleaner to just make a middleware to wrap our handlers?

If we took that approach, we wouldn't have to implement this same check in every handler.

Yes, it's been added. It even has unit tests :)

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/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%

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
@MichelHollands MichelHollands marked this pull request as ready for review June 17, 2022 13:38
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/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.3%
+            querier	0%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@MasslessParticle
Copy link
Contributor

This looks good! Were we going remove WholeStreamDelete in this PR or another one?

@MichelHollands
Copy link
Contributor Author

This looks good! Were we going remove WholeStreamDelete in this PR or another one?

Due to the size of this one I was going to do it in a separate PR.

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Added two usability nits, otherwise looks good

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @MichelHollands 👍
The docs changes here are fairly minor @KMiller-Grafana so I'm going to merge, I hope you don't mind?

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/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.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@dannykopping dannykopping merged commit b4e6c59 into grafana:main Jun 20, 2022
@grafanabot
Copy link
Collaborator

The backport to k102 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-6410-to-k102 origin/k102
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x b4e6c599368278b8da20852a9acc4722f70af603
# Push it to GitHub
git push --set-upstream origin backport-6410-to-k102
git switch main
# Remove the local backport branch
git branch -D backport-6410-to-k102

Then, create a pull request where the base branch is k102 and the compare/head branch is backport-6410-to-k102.

MichelHollands added a commit to MichelHollands/loki that referenced this pull request Jun 23, 2022
* Add per tenant compaction delete enabled flag

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Remove changes in wrong place

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add compactor deletion enabled field

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Use limit in compactor

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Use http middleware and add test

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Fix lint issue

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add changelog

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Revert to default setting if no override

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add default value command line option

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Update the docs

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Enable access to deletion API for integration test

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Rename flag to allow_deletes

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Update per review comments

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
(cherry picked from commit b4e6c59)
MichelHollands added a commit that referenced this pull request Jun 23, 2022
* Compactor: add per tenant compaction delete enabled flag (#6410)

* Add per tenant compaction delete enabled flag

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Remove changes in wrong place

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add compactor deletion enabled field

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Use limit in compactor

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Use http middleware and add test

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Fix lint issue

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add changelog

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Revert to default setting if no override

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add default value command line option

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Update the docs

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Enable access to deletion API for integration test

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Rename flag to allow_deletes

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Update per review comments

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
(cherry picked from commit b4e6c59)

* Fix changelog

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
grafanabot pushed a commit that referenced this pull request Jun 24, 2022
* Compactor: add per tenant compaction delete enabled flag (#6410)

* Add per tenant compaction delete enabled flag

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Remove changes in wrong place

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add compactor deletion enabled field

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Use limit in compactor

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Use http middleware and add test

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Fix lint issue

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add changelog

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Revert to default setting if no override

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add default value command line option

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Update the docs

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Enable access to deletion API for integration test

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Rename flag to allow_deletes

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Update per review comments

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
(cherry picked from commit b4e6c59)

* Fix changelog

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
(cherry picked from commit df48f93)
vlad-diachenko pushed a commit that referenced this pull request Jun 24, 2022
…) (#6496)

* Compactor: add per tenant compaction delete enabled flag (#6410)

* Add per tenant compaction delete enabled flag

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Remove changes in wrong place

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add compactor deletion enabled field

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Use limit in compactor

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Use http middleware and add test

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Fix lint issue

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add changelog

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Revert to default setting if no override

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Add default value command line option

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Update the docs

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Enable access to deletion API for integration test

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Rename flag to allow_deletes

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

* Update per review comments

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
(cherry picked from commit b4e6c59)

* Fix changelog

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
(cherry picked from commit df48f93)

Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com>
@osg-grafana osg-grafana added type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories and removed area/docs labels Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants