Alertmanager Distributor by codesome · Pull Request #3671 · cortexproject/cortex

codesome · 2021-01-08T15:17:39Z

What this PR does:

Adds sharding capabilities to alertmanager for the /alerts API and introduces a new Distributor component in it based on the design here.

Checklist

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

gotjosh · 2021-01-08T19:20:05Z

Depends on #3664

pracucci · 2021-01-15T12:13:34Z

Let's get #3664 merged and then we'll review this PR, otherwise it's getting quite complicated to follow. Merging of #3664 should happen relatively soon (looks in a good shape to me).

gotjosh · 2021-01-19T18:18:18Z

#3664 is now merged, feel free to rebase.

pracucci · 2021-01-19T18:19:47Z

@codesome Josh's PR #3664 has been merged. Could you rebase this PR, please?

codesome · 2021-01-20T07:55:41Z

I have rebased, but I need to fix some TODOs, add some metrics and more tests. I will tell when it is ready for review.

codesome · 2021-01-20T13:31:16Z

It is ready for a review. I still have to fix the metrics part of unit test and have to test running the distributor and alertmanager in a single binary to see if routing is happening properly (with no conflicting route registrations).

codesome · 2021-01-28T10:55:29Z

PR is ready for review. I am still testing some bits locally to make sure everything is working fine. I will be adding more tests to alertmanager.

The current idea is to handle sharding only for /alerts API and incrementally add support for others. cc @gotjosh

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

pracucci

Thanks a lot for addressing all comments. Change LGTM and most of the left comments are last nit, but there are a couple of serious ones reason why I haven't marked as approved yet. Thanks!

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

pracucci

Good job, LGTM. Thanks! (just left few minor nits)

ranton256 · 2021-02-11T18:15:43Z

+
+	sp, ctx := opentracing.StartSpanFromContext(r.Context(), "Distribute.doRead")
+	defer sp.Finish()
+	// Until we have a mechanism to combine the results from multiple alertmanagers,


What would combining mean in terms of behavior?

Combining would deduplicate the results from those alertmanagers, basically a union

ranton256

LGTM

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

gotjosh · 2021-02-11T14:26:46Z

+
+// DistributeRequest shards the writes and returns as soon as the quorum is satisfied.
+// In case of reads, it proxies the request to one of the alertmanagers.
+// DistributeRequest assumes that the caller has verified IsPathSupported return


Suggested change

// DistributeRequest assumes that the caller has verified IsPathSupported return

// DistributeRequest assumes that the caller has verified IsPathSupported returns

pstibrany

Great job, this looks good! I've left some last comments.

pstibrany · 2021-02-16T13:15:33Z

+	defer sp.Finish()
+	// Until we have a mechanism to combine the results from multiple alertmanagers,
+	// we forward the request to only only of the alertmanagers.
+	amDesc := replicationSet.Ingesters[rand.Intn(len(replicationSet.Ingesters))]


To make sure we don't panic in rand.Intn, we should verify that len(replicationSet) > 0.

The strategy used for AM ring makes sure there is at least 1 AM, so do we need the check here?

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

pracucci

Still LGTM. Time to merge it!

pull-request-size Bot added the size/L label Jan 8, 2021

codesome force-pushed the am-distributors branch 2 times, most recently from de1bc94 to 5f303ca Compare January 12, 2021 11:04

pull-request-size Bot added size/XXL and removed size/L labels Jan 12, 2021

codesome force-pushed the am-distributors branch 2 times, most recently from 44306db to 9852490 Compare January 14, 2021 15:04

codesome force-pushed the am-distributors branch from 9852490 to f85ca18 Compare January 20, 2021 07:54

pull-request-size Bot added size/XL and removed size/XXL labels Jan 20, 2021

codesome force-pushed the am-distributors branch from f85ca18 to 85d1f5f Compare January 20, 2021 13:29

codesome marked this pull request as ready for review January 20, 2021 13:29

codesome force-pushed the am-distributors branch from 85d1f5f to dc746c9 Compare January 20, 2021 13:43

codesome marked this pull request as draft January 27, 2021 15:31

codesome force-pushed the am-distributors branch from 4f0bf13 to e5cd2b3 Compare January 27, 2021 15:53

codesome marked this pull request as ready for review January 28, 2021 10:53

codesome force-pushed the am-distributors branch from 4088bc7 to c2eaed2 Compare January 28, 2021 13:42

ranton256 reviewed Jan 28, 2021

View reviewed changes

Comment thread pkg/alertmanager/multitenant.go Outdated

codesome force-pushed the am-distributors branch from c2eaed2 to bbbedab Compare January 28, 2021 15:17

Ganesh Vernekar and others added 4 commits January 28, 2021 22:48

Alertmanager Distributor

e342d8e

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Fix lint

2865984

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Fix tests

fd9dc8f

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

WIP: HTTP over GRPC for alertmanager Distributors

1b832b2

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Fix most of Peter's comments

7ae2d0b

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the am-distributors branch 2 times, most recently from 1bb7349 to 2159ef5 Compare February 5, 2021 14:18

Fix remaining comments

f414266

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the am-distributors branch from 2159ef5 to f414266 Compare February 8, 2021 12:51

Fix integration tests

7727e1b

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

pracucci reviewed Feb 10, 2021

View reviewed changes

Fix review comments

bfe3857

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the am-distributors branch 2 times, most recently from 27ad663 to 0ae3b27 Compare February 11, 2021 12:53

Merge remote-tracking branch 'upstream/master' into am-distributors

1b4ee91

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the am-distributors branch from 0ae3b27 to 1b4ee91 Compare February 11, 2021 13:12

pracucci approved these changes Feb 11, 2021

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread pkg/alertmanager/distributor.go Outdated

Comment thread pkg/alertmanager/distributor.go Outdated

Comment thread pkg/alertmanager/distributor.go Outdated

Comment thread pkg/alertmanager/multitenant.go Outdated

ranton256 reviewed Feb 11, 2021

View reviewed changes

ranton256 approved these changes Feb 11, 2021

View reviewed changes

codesome force-pushed the am-distributors branch from 7ce76cf to e43d915 Compare February 12, 2021 12:26

Fix Marco's comments

92657d3

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the am-distributors branch from e43d915 to 92657d3 Compare February 12, 2021 13:04

gotjosh approved these changes Feb 16, 2021

View reviewed changes

pstibrany reviewed Feb 16, 2021

View reviewed changes

codesome force-pushed the am-distributors branch 4 times, most recently from 44bbf73 to 2f4e05c Compare February 17, 2021 12:02

Fix Peter's and Josh's comments

6a39e95

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

codesome force-pushed the am-distributors branch from 2f4e05c to 6a39e95 Compare February 17, 2021 12:55

pracucci approved these changes Feb 18, 2021

View reviewed changes

gouthamve approved these changes Feb 18, 2021

View reviewed changes

Comment thread CHANGELOG.md

Comment thread CHANGELOG.md

pracucci merged commit 6c0bebf into cortexproject:master Feb 18, 2021

gotjosh mentioned this pull request Feb 18, 2021

Alertmanager: Replicate state using the Ring #3839

Merged

3 tasks

	// DistributeRequest assumes that the caller has verified IsPathSupported return
	// DistributeRequest assumes that the caller has verified IsPathSupported returns

Conversation

codesome commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gotjosh commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci commented Jan 15, 2021

Uh oh!

gotjosh commented Jan 19, 2021

Uh oh!

pracucci commented Jan 19, 2021

Uh oh!

codesome commented Jan 20, 2021

Uh oh!

codesome commented Jan 20, 2021

Uh oh!

codesome commented Jan 28, 2021

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ranton256 Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

codesome Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

ranton256 left a comment

Choose a reason for hiding this comment

Uh oh!

gotjosh Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pstibrany Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

codesome Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codesome commented Jan 8, 2021 •

edited

Loading

gotjosh commented Jan 8, 2021 •

edited

Loading