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

[helm] Single binary object storage #8284

Merged

Conversation

trevorwhitney
Copy link
Collaborator

@trevorwhitney trevorwhitney commented Jan 25, 2023

What this PR does / why we need it:

The PR allows an operator to install Loki in Single Binary mode in an HA fashion, meaning more than 1 Single Binary replica. In order for this to work, it is not possible to specify an object storage backend when running in Single Binary mode. The gateway is also now always enabled, for both Single Binary and Scalable modes.

Which issue(s) this PR fixes:
Fixes #8269 #8185

Special notes for your reviewer:

This PR builds off #8261, will rebase after that is merged and diff will be smaller.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@grafanabot
Copy link
Collaborator

./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%

1 similar comment
@grafanabot
Copy link
Collaborator

./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%

@JStickler
Copy link
Contributor

@trevorwhitney, you tagged me on this, but I don't see much in the way of documentation changes.

Not sure if we need to update this topic, which mentions using a shared object store? https://grafana.com/docs/loki/latest/fundamentals/architecture/deployment-modes/

@trevorwhitney
Copy link
Collaborator Author

@JStickler sorry the bot tagged you when I accidentally was removing changes to a docs file due to a botched rebase. No docs need on this one 😄

@grafanabot
Copy link
Collaborator

./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%

1 similar comment
@grafanabot
Copy link
Collaborator

./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%

Copy link
Contributor

@jeschkies jeschkies left a comment

Choose a reason for hiding this comment

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

Could you add some documentation on how this could be used?

@trevorwhitney trevorwhitney force-pushed the single-binary-object-storage branch 2 times, most recently from 7f7045f to 17c7eb3 Compare February 2, 2023 22:49
Copy link
Contributor

@JStickler JStickler left a comment

Choose a reason for hiding this comment

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

[Docs squad] One small suggestion for rewording to make it clearer that there are now two options here.

docs/sources/installation/helm/install-monolithic/index.md Outdated Show resolved Hide resolved
Copy link
Contributor

@sapslaj sapslaj left a comment

Choose a reason for hiding this comment

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

This is great @trevorwhitney! I just had that one thing about the replica counts come up in my testing, otherwise it looks great.

{{- fail "Cannot run more than 1 Single Binary replica without an object storage backend."}}
{{- end }}

{{- if and (eq (include "loki.isUsingObjectStorage" .) "false") (or (gt (int .Values.backend.replicas) 0) (gt (int .Values.read.replicas) 0) (gt (int .Values.write.replicas) 0)) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

One (potentially negative?) side effect of this check is that you have to explicitly scale down the backend, read, and write replicas if you're using single binary with filesystem storage. If you don't, then this check will fail.

I wonder if we need to also and in something like (eq (int .Values.singleBinary.replicas) 0) in here, like we do when defining loki.deployment.isScalable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, added that extra check to the condition.

* Allow object storage use with single binary
* Always enable the gateway
* Enable tokengen for both enterprise modes
Co-authored-by: J Stickler <julie.stickler@grafana.com>
@trevorwhitney trevorwhitney merged commit ef8b382 into grafana:main Feb 6, 2023
@trevorwhitney trevorwhitney deleted the single-binary-object-storage branch February 6, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm 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.

[Helm] Enable the gateway for single binary mode
5 participants