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

Add switch to allow enabling overrides-exporter ring #3995

Merged
merged 6 commits into from
Jan 20, 2023

Conversation

flxbk
Copy link
Contributor

@flxbk flxbk commented Jan 18, 2023

What this PR does

This adds jsonnet config for the ring for overrides-exporters and enables it for the read-write deployment mode.

Do changes like this need a changelog entry?

Which issue(s) this PR fixes or relates to

#3806

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@flxbk flxbk changed the title Add switch to allow enabling overrides exporter ring Add switch to allow enabling overrides-exporter ring Jan 18, 2023
@flxbk flxbk marked this pull request as ready for review January 18, 2023 12:24
@flxbk flxbk requested a review from a team as a code owner January 18, 2023 12:24
@flxbk flxbk self-assigned this Jan 18, 2023
@@ -24,11 +25,20 @@
local containerPort = $.core.v1.containerPort,
overrides_exporter_port:: containerPort.newNamed(name='http-metrics', containerPort=$._config.server_http_port),

local ringConfig = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our jsonnet support two ring backends: consul (default) and memberlist. Look for querySchedulerRingClientConfig and try to come up with something similar. Since in this case both lifecycler and ring client is only used by overrides exporter, you don't need two separate configs for the lifecycler and ring client, but a single overridesExporterRingConfig is enough. Make overridesExporterRingConfig not local, so that it's overrideable (like querySchedulerRingClientConfig).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then I would enable it by default with deployment mode is read-write, so we also see the difference in the jsonnet tests (run make jsonnet-tests to update the tests output).

@flxbk flxbk marked this pull request as draft January 18, 2023 14:49
@flxbk flxbk force-pushed the felix/backend-exporter-ring branch from f4f007c to 8219634 Compare January 18, 2023 15:46
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Very good! I just left a comment to fix the memberlist override, we shouldn't happen if the overrides-exporter ring is disabled.

Comment on lines 509 to 511
- -memberlist.bind-port=7946
- -memberlist.join=dns+gossip-ring.default.svc.cluster.local:7946
- -overrides-exporter.ring.store=memberlist
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct. Shouldn't be set if the overrides-exporter ring is disabled.

operations/mimir/memberlist.libsonnet Outdated Show resolved Hide resolved
@flxbk flxbk force-pushed the felix/backend-exporter-ring branch from d736ff3 to 5b6ff22 Compare January 20, 2023 13:24
Copy link
Collaborator

@pracucci pracucci 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!

@flxbk flxbk marked this pull request as ready for review January 20, 2023 14:05
@pracucci pracucci enabled auto-merge (squash) January 20, 2023 14:12
@pracucci pracucci merged commit efd72bb into main Jan 20, 2023
@pracucci pracucci deleted the felix/backend-exporter-ring branch January 20, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants