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

Change a few default config values and improve application of common storage config #4543

Merged
merged 6 commits into from
Oct 28, 2021

Conversation

trevorwhitney
Copy link
Collaborator

@trevorwhitney trevorwhitney commented Oct 25, 2021

What this PR does / why we need it:

This PR represents a bunch of small changes that I uncovered when preparing my Obscon demo for the Simple Scalable Deployment architecture. One goal of this project has been simplifying configs so they are closer to how we run Loki.

  • default ruler api to on
  • register defaults for storage configs in the common config (ie.
    signature version for S3)
  • enable ingester wal by default
  • disable chunk retries by default
  • allow common path prefix to include or omit trailing slash
  • allow partial storage config to be defined for overriding things like
    bucket names per component

Checklist

  • Documentation added
  • Tests updated

@trevorwhitney trevorwhitney requested a review from a team as a code owner October 25, 2021 20:45
@trevorwhitney trevorwhitney changed the title Final ssd configs Change a few default config values and improve application of common storage config Oct 25, 2021
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.

Looks great! Will wait for another maintainer to review who's more familiar with the problem at hand

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

I'm a little torn on whether we should make the WAL a default or not. On one hand, it's much safer to run with a WAL as it greatly alleviates durability concerns, but it also adds an explicit requirement on disk, which is a tricky dependency from an operator's perspective. There's also a secondary question: Will this break existing users upgrading to 2.4?

pkg/loki/common/common.go Outdated Show resolved Hide resolved
@trevorwhitney
Copy link
Collaborator Author

We can take the WAL bit out and maybe wait for a breaking release (3.0 anyone?), but I do think long term this is a change worth making. The advantage this brings re: durability is big, and in terms of breaking existing installations, I would be curious how common a practice it is to run loki without a WAL? I couldn't find any environment, dev or prod, at GL that was running loki in this way, which makes me think most production environments are running loki with a WAL.

trevorwhitney and others added 6 commits October 27, 2021 14:14
* default ruler api to on
* register defaults for storage configs in the common config (ie.
  signature version for S3)
* enable ingester wal by default
* disable chunk retries by default
* allow common path prefix to include or omit trailing slash
* allow partial storage config to be defined for overriding things like
  bucket names per component
Co-authored-by: Owen Diehl <ow.diehl@gmail.com>
@slim-bean slim-bean merged commit ed025e0 into main Oct 28, 2021
@slim-bean slim-bean deleted the final-ssd-configs branch October 28, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants