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.

Sign up for GitHub

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

Jump to bottom

Memberlist related jsonnet #6253

Merged
merged 3 commits into from
May 30, 2022
Merged

Memberlist related jsonnet #6253

merged 3 commits into from
May 30, 2022

Conversation

cstyan
Copy link
Contributor

@cstyan cstyan commented May 25, 2022

Adds memberlist.libsonnet and modify loki services so we can set the port when using memberlist for the ring.

based on the memberlist jsonnet code written by @pstibrany for mimir, the main change is how the memberlist config values are set for services; mimir team uses cli flags while we use the config file

cc @JordanRushing @vlad-diachenko

Signed-off-by: Callum Styan callumstyan@gmail.com

@cstyan
Add memberlist.libsonnet and modify loki services so we can set the port
94677fe 
when using memberlist for the ring.

Signed-off-by: Callum Styan <callumstyan@gmail.com>
@cstyan cstyan requested a review from a team as a code owner May 25, 2022 21:55
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/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%

DylanGuedes
DylanGuedes approved these changes May 26, 2022
View reviewed changes
Copy link
Contributor

@DylanGuedes DylanGuedes left a comment

Choose a reason for hiding this comment

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

few boring questions, but lgtm!

production/ksonnet/loki/memberlist.libsonnet Show resolved Hide resolved
production/ksonnet/loki/memberlist.libsonnet Show resolved Hide resolved
trevorwhitney
trevorwhitney approved these changes May 26, 2022
View reviewed changes
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.

LGTM, with the one small concern about defining specific ring sections for each component

production/ksonnet/loki/memberlist.libsonnet Show resolved Hide resolved
@cstyan
Missed the + for the ruler ring config here.
1141383 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/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%

@cstyan
Fix linting.
150ec4b 
Signed-off-by: Callum Styan <callumstyan@gmail.com>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

vlad-diachenko
vlad-diachenko approved these changes May 27, 2022
View reviewed changes
Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

just one small comment that is not a blocker

production/ksonnet/loki/memberlist.libsonnet Show resolved Hide resolved
kavirajk
kavirajk approved these changes May 30, 2022
View reviewed changes
Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

LGTM. 👍
should we have separate migration doc? (not a blocker, can be a separate PR)

@kavirajk kavirajk merged commit 0cb3994 into grafana:main May 30, 2022
owen-d pushed a commit that referenced this pull request Jun 2, 2022
@cstyan
K100 backport memberlist jsonnet (#6296)
c381c81 
* Memberlist related jsonnet (#6253)

* Add memberlist.libsonnet and modify loki services so we can set the port
when using memberlist for the ring.

Signed-off-by: Callum Styan <callumstyan@gmail.com>

* Missed the + for the ruler ring config here.

Signed-off-by: Callum Styan <callumstyan@gmail.com>

* Fix linting.

Signed-off-by: Callum Styan <callumstyan@gmail.com>

* Import memberlist.libsonnet in loki.libsonnet. (#6294)

Signed-off-by: Callum Styan <callumstyan@gmail.com>
This was referenced Jun 28, 2022
Closed
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlad-diachenko vlad-diachenko vlad-diachenko approved these changes

@trevorwhitney trevorwhitney trevorwhitney approved these changes

@DylanGuedes DylanGuedes DylanGuedes approved these changes

@kavirajk kavirajk kavirajk approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cstyan @grafanabot @trevorwhitney @kavirajk @DylanGuedes @vlad-diachenko