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

Loki: Apply the ingester ring config to all other rings (distributor, ruler, query-scheduler) #4546

Merged
merged 2 commits into from
Oct 26, 2021

Conversation

slim-bean
Copy link
Collaborator

What this PR does / why we need it:

Problem: Loki fails to start when using the default config (specifically any config which doesn't properly set the query-scheduler ring config)

Conceptually it makes sense to me to apply the ring config used in the ingesters to any other rings used by Loki. The likelyhood of someone using multiple ring stores between different components is extremly low.

It seems resonable then to use the ingester ring config for all rings.

Unfortunately the implementation of this is a fairly brittle mapping exercise because each ring uses a different struct for config (even though they are mostly similar and largely the same)

I know this being broken is blocking several things/people so this is my suggested fix.

I have an idea for a test, although it's crude but I don't have any better ideas:

msgf := "%s has changed, this is a crude attempt to catch mapping errors missed in config_wrapper.applyIngesterRingConfig when a ring config changes. Please add a new mapping and update the expected value in this test."

assert.Equal(t, 8,  reflect.TypeOf(distributor.RingConfig{}).NumField(), fmt.Sprintf(msgf, reflect.TypeOf(distributor.RingConfig{}).String()))
// Do this for all the other rings

seems better than nothing....

We can merge this PR as is and I'll follow up with the test, I know we are blocked a few places on this so we can merge this first else I'll push more to it tomorrow.

Which issue(s) this PR fixes:
Fixes #4533

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@slim-bean slim-bean requested a review from a team as a code owner October 26, 2021 01:31
…e step to apply the common memberlist config when join_members is defined.
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.

Thanks for this, @slim-bean!
It seems to me that we need to move the ring config to its own central definition and reuse it everywhere; I agree with you that using different ring configs is pretty unlikely.

Copy link
Contributor

@MichelHollands MichelHollands left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Loki failing to start using latest dockerhub image.
3 participants