Zone aware ingesters #7923

manohar-koukuntla · 2022-12-13T09:32:00Z

What this PR does / why we need it:

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

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide
Documentation added
Tests updated
CHANGELOG.md updated
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

CLAassistant · 2022-12-13T09:32:06Z

All committers have signed the CLA.

grafanabot · 2022-12-19T16:40:38Z

./tools/diff_coverage.sh ../loki-target-branch/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.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2023-01-03T21:45:53Z

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

manohar-koukuntla · 2023-01-03T21:49:20Z

Friendly bump.

manohar-koukuntla · 2023-01-04T15:13:18Z

@kavirajk

kavirajk · 2023-01-06T12:43:57Z

Thanks @manohar-koukuntla for the PR.

We have something very similar (also forked from Mimir's public jsonnet lib) internally that we were using it for our zone aware ingesters setup(with some minor tweaks.)

@cstyan knows more about it. @cstyan can we help @manohar-koukuntla tweak the PR a little and merge it?

manohar-koukuntla · 2023-01-09T16:42:27Z

Thanks @manohar-koukuntla for the PR.

We have something very similar (also forked from Mimir's public jsonnet lib) internally that we were using it for our zone aware ingesters setup(with some minor tweaks.)

@cstyan knows more about it. @cstyan can we help @manohar-koukuntla tweak the PR a little and merge it?

@kavirajk Thank you. I am using the same concept we are using in mimir. Since mimir does extra tweaks on top of standard cortex zone awareness, I am using default cortex setting for loki and tempo(grafana/tempo#1936).

grafanabot · 2023-01-09T16:52:33Z

./tools/diff_coverage.sh ../loki-target-branch/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.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2023-01-10T11:21:59Z

./tools/diff_coverage.sh ../loki-target-branch/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

LGTM for the most part, just a few comments on things that I know caused issues for us internally, and I also left one question.

We have some more jsonnet, mostly related to ease of use + migrating to zone aware ingesters that I can port/update after your PR.

cstyan · 2023-01-12T23:03:34Z

production/ksonnet/loki/multi-zone.libsonnet

+    container.withArgs($.util.mapToFlags(
+      $.ingester_args + zone_args
+    )) +
+    container.withEnvMixin([{ name: 'AVAILABILITY_ZONE', value: zone_name }]),


is this from mimir or did you add it yourself? if the latter, do you mind leaving a comment as to what the intended use is for the env var?

I am using the same concept we are using in mimir. Since mimir does extra tweaks on top of standard cortex zone awareness, I am using default cortex setting for loki and tempo(grafana/tempo#1936).

Sorry if I was unclear, I am asking about the environment variable itself. I don't see a reason why we're adding it.

production/ksonnet/loki/multi-zone.libsonnet

grafanabot · 2023-01-19T16:21:51Z

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

grafanabot · 2023-01-19T16:34:43Z

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

grafanabot · 2023-01-19T19:34:45Z

./tools/diff_coverage.sh ../loki-target-branch/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

Approving, I'll open a separate PR to upstream additional changes we had to make to get things to work internally.

manohar-koukuntla requested a review from a team as a code owner December 13, 2022 09:32

pull-request-size bot added the size/L label Dec 13, 2022

manohar-koukuntla force-pushed the zone_aware_ingesters branch 2 times, most recently from 5ab5cab to a23ca9a Compare December 13, 2022 09:38

manohar-koukuntla force-pushed the zone_aware_ingesters branch from 21db603 to 67acdd9 Compare December 19, 2022 16:48

manohar-koukuntla force-pushed the zone_aware_ingesters branch from 9cd4bbd to b8fedf3 Compare January 9, 2023 16:47

cstyan reviewed Jan 12, 2023

View reviewed changes

manohar-koukuntla added 2 commits January 19, 2023 17:15

zone aware ingesters

d433d7d

update change log

fa393e0

manohar-koukuntla force-pushed the zone_aware_ingesters branch from a90bad1 to fa393e0 Compare January 19, 2023 16:15

manohar-koukuntla added 2 commits January 19, 2023 17:16

remove prof files

9a917a4

remove prof files

635d327

manohar-koukuntla marked this pull request as draft January 19, 2023 16:17

manohar-koukuntla added 2 commits January 19, 2023 17:23

jsonnet fmt

c7a04d1

replace null with {}

9e05317

manohar-koukuntla marked this pull request as ready for review January 19, 2023 16:28

replace null with {}

10d43a0

Merge branch 'grafana:main' into zone_aware_ingesters

37f7335

cstyan approved these changes Jan 30, 2023

View reviewed changes

cstyan merged commit 37d6502 into grafana:main Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zone aware ingesters #7923

Zone aware ingesters #7923

manohar-koukuntla commented Dec 13, 2022 •

edited

CLAassistant commented Dec 13, 2022 •

edited

grafanabot commented Dec 19, 2022

grafanabot commented Jan 3, 2023

manohar-koukuntla commented Jan 3, 2023 •

edited

manohar-koukuntla commented Jan 4, 2023

kavirajk commented Jan 6, 2023

manohar-koukuntla commented Jan 9, 2023

grafanabot commented Jan 9, 2023

grafanabot commented Jan 10, 2023

cstyan left a comment

cstyan Jan 12, 2023

manohar-koukuntla Jan 19, 2023

cstyan Jan 21, 2023

grafanabot commented Jan 19, 2023

grafanabot commented Jan 19, 2023

grafanabot commented Jan 19, 2023

cstyan left a comment

Zone aware ingesters #7923

Zone aware ingesters #7923

Conversation

manohar-koukuntla commented Dec 13, 2022 • edited

CLAassistant commented Dec 13, 2022 • edited

grafanabot commented Dec 19, 2022

grafanabot commented Jan 3, 2023

manohar-koukuntla commented Jan 3, 2023 • edited

manohar-koukuntla commented Jan 4, 2023

kavirajk commented Jan 6, 2023

manohar-koukuntla commented Jan 9, 2023

grafanabot commented Jan 9, 2023

grafanabot commented Jan 10, 2023

cstyan left a comment

Choose a reason for hiding this comment

cstyan Jan 12, 2023

Choose a reason for hiding this comment

manohar-koukuntla Jan 19, 2023

Choose a reason for hiding this comment

cstyan Jan 21, 2023

Choose a reason for hiding this comment

grafanabot commented Jan 19, 2023

grafanabot commented Jan 19, 2023

grafanabot commented Jan 19, 2023

cstyan left a comment

Choose a reason for hiding this comment

manohar-koukuntla commented Dec 13, 2022 •

edited

CLAassistant commented Dec 13, 2022 •

edited

manohar-koukuntla commented Jan 3, 2023 •

edited