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

WIP: Implement a common ring config #4610

Closed

Conversation

DylanGuedes
Copy link
Contributor

@DylanGuedes DylanGuedes commented Nov 1, 2021

What this PR does / why we need it:

General Idea

This PR creates a new subsection in the common config named ring, that will be reused internally to configure other rings.
The previous behavior was the following:
For every ring, check if an explicit config is given. If it is, use it. If it isn't, reuse whatever is set for the Ingester ring.

The new behavior, implemented by this PR, is the following:
For every ring, check if an explicit config is given:

  • If it is given, use it
  • If it isn't given, check if a common ring (ring under common section) is given. If it is, use it
  • If a common ring isn't given, just reuse whatever is set for the Ingester ring.

Technical Decisions

  • Since the LifecyclerConfig (from Cortex) is the ring config with the most configurations, I decided to use it in the reuseConfig... function instead of others. If you think this looks weird just let me know
  • I'm applying the token paths in a separate function after all this new behavior to isolate things
  • There's a caveat in reusing the common config and appending a loopback interface when appropriate: if we call appendLoopbackInterface before time, the check reflect.DeepEqual(config.Ingester.LifecyclerConfig, defaults.ingester.LifecyclerConfig) will fail when it shouldn't. Because of this, I have to first check if the Ingester config was set or not, and only then I can invoke appendLoopbackInterface.

Which issue(s) this PR fixes:
Fixes # (I'll create an issue later)

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@DylanGuedes DylanGuedes force-pushed the add-ingestor-ring-config-copying branch 2 times, most recently from e6b0cf6 to fa0ce3f Compare November 1, 2021 21:14
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

I think this approach looks good!


f, err := tokensFile(r, "ingester.tokens")
func reuseCommonConfig(r, defaults *ConfigWrapper) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could use a better name, as it's not really "reusing", and we should call out it applies specifically to the ring

@DylanGuedes DylanGuedes force-pushed the add-ingestor-ring-config-copying branch from fa0ce3f to 717812e Compare November 1, 2021 21:30
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.

Nice work, this is a really good pass at this code. It's definitely getting pretty complex, so I wonder if we can do a few things to simplify it further. I've added a few thoughts to that end.

Of these steps, I'd use the following:

If it is given, use it
If it isn't given, check if a common ring (ring under common section) is given. If it is, use it
If a common ring isn't given, just reuse whatever is set for the Ingester ring. (remove this step, now superseded by the common section

We can also make the loopback address included by default on these configurations rather than need the indirection we currently use with appendLoopback. We may need to do a little bit of struct embedding and flagext.RegisterFlags implementation on top, but that seems a nicer approach to me. In the medium term, we can PR into dskit's ring and then revendor into Loki (post 2.4 sometime).

// If the ingester ring has its interface names sets to a value equal to the default (["eth0", en0"]), it will try to append
// the loopback interface at the end of it.
func applyConfigToRings(r, defaults *ConfigWrapper, reuse ring.LifecyclerConfig) {
shouldOverrideIngester := reflect.DeepEqual(r.Ingester.LifecyclerConfig, defaults.Ingester.LifecyclerConfig)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should apply this conditional to every ring section.

// TODO(dylanguedes): Make sure no important config is missed.
return ring.LifecyclerConfig{
RingConfig: ring.Config{KVStore: cfg.KVStore},
NumTokens: 128, // default ingester num tokens
Copy link
Member

Choose a reason for hiding this comment

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

Note (for a different PR): We tend to use 512 for the ingesters internally. It's probably worth exposing the superset of ring configs in the common section and then being able to slim it down via a function similar to this one. This would allow us to specify things like NumTokens in the common section.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Took a different approach in the new PR, but when forced to guess still using 128. Maybe 512 is better?

@trevorwhitney trevorwhitney mentioned this pull request Nov 2, 2021
3 tasks
@trevorwhitney
Copy link
Collaborator

trevorwhitney commented Nov 2, 2021

@DylanGuedes I picked this up while you were off to hopefully push it over the finish line in time for 2.4. Since I can't push to your fork, I created a new branch/PR: #4617. I'm going to close this in favor of that one, lets move discussion over there.

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.

3 participants