Optimise alertmanager config loading #3898

pracucci · 2021-03-03T09:18:08Z

What this PR does:
There are two bottlenecks in the alertmanager configs loading from the object storage:

Configs are loaded sequentially
When alertmanager sharding is enabled, we load configs for all users, regardless they're owned by the alertmanager replica

This PR addresses both issues. To do it I had to refactor the AlertStore interface to split between the listing of users and loading of configs for the owned users:

Removed ListAlertConfigs
Added ListAllUsers and GetAlertConfigs (reason why I've added GetAlertConfigs instead of calling GetAlertConfig for every user is because with GetAlertConfigs the configdb implementation is optimised)
Added concurrent fetching to GetAlertConfigs in the object store client implementations

Which issue(s) this PR fixes:
N/A

Checklist

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

Signed-off-by: Marco Pracucci <marco@pracucci.com>

…tenantAlertmanager Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci · 2021-03-03T09:21:24Z

pkg/alertmanager/api_test.go

@@ -125,7 +123,7 @@ template_files:
 	}

 	am := &MultitenantAlertmanager{
-		store:  noopAlertStore{},
+		store:  prepareFilesystemAlertStore(t),


Better to use an empty real storage than a mocked one.

Wouldn't it be simpler to use bucketclient.NewBucketAlertStore on in-memory bucket? No need to cleanup anything.

We currently don't allow to configure the in-memory bucket via the bucket.Config. Instead of doing changes to use it, I just used the filesystem-based one which is few lines more of code because of the temporarily directory.

We don't need to use bucket.Config, do we?

func prepareFilesystemAlertStore(t *testing.T) alertstore.AlertStore { obj := objstore.NewInMemBucket() return bucketclient.NewBucketAlertStore(obj, nil, log.NewNopLogger()) }

Yeah, done. I was smoking some good weed when I first replied you.

pracucci · 2021-03-03T09:22:23Z

pkg/alertmanager/multitenant.go


-	alertmanagers, err := am.ring.Get(ringHasher.Sum32(), RingOp, nil, nil, nil)
+	alertmanagers, err := am.ring.Get(shardByUser(userID), RingOp, nil, nil, nil)


We have shardByUser(): let's use it.

pracucci · 2021-03-03T09:23:04Z

pkg/alertmanager/multitenant.go

-	ringHasher := fnv.New32a()
-	// Hasher never returns err.
-	_, _ = ringHasher.Write([]byte(userID))
+func (am *MultitenantAlertmanager) isUserOwned(userID string) bool {


it's easier to use if it can't return error and the error case is handled internally.

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pstibrany

Change looks good, but I have doubts about using concurrency.ForEachUser in its current form.

pkg/alertmanager/alertstore/store.go

pstibrany · 2021-03-03T10:56:29Z

pkg/alertmanager/api_test.go

@@ -125,7 +123,7 @@ template_files:
 	}

 	am := &MultitenantAlertmanager{
-		store:  noopAlertStore{},
+		store:  prepareFilesystemAlertStore(t),


Wouldn't it be simpler to use bucketclient.NewBucketAlertStore on in-memory bucket? No need to cleanup anything.

pstibrany · 2021-03-03T10:59:02Z

pkg/alertmanager/alertstore/bucketclient/bucket_client.go

+		cfgs   = make(map[string]alertspb.AlertConfigDesc, len(userIDs))
+	)
+
+	err := concurrency.ForEachUser(ctx, userIDs, fetchConcurrency, func(ctx context.Context, userID string) error {


concurrency.ForEachUser continues fetching configs even if some user returns error. But this method returns nil in case of errors. There seems to be a mismatch.

I ~~don't~~ think using approach based on errgroup would be better.

Good point. I've created a generic utility concurrency.ForEach() which works with []interface{} and used it. What do you think?

pkg/alertmanager/alertstore/bucketclient/bucket_client.go

pstibrany · 2021-03-03T11:01:33Z

pkg/alertmanager/alertstore/objectclient/store.go

+		cfgs   = make(map[string]alertspb.AlertConfigDesc, len(userIDs))
+	)
+
+	err := concurrency.ForEachUser(ctx, userIDs, fetchConcurrency, func(ctx context.Context, userID string) error {


Same comment about using concurrency.ForEachUser vs returning nil applies here.

CHANGELOG.md

gotjosh

LGTM! All my comments are nits and completely optional.

pkg/alertmanager/multitenant.go

pkg/alertmanager/alertstore/store_test.go

CHANGELOG.md

pkg/alertmanager/alertstore/local/store_test.go

Signed-off-by: Marco Pracucci <marco@pracucci.com>

… used it Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pstibrany

Thanks for addressing my feedback. Nice work.

* Added ListAllUsers() to AlertStore Signed-off-by: Marco Pracucci <marco@pracucci.com> * Added local store unit tests Signed-off-by: Marco Pracucci <marco@pracucci.com> * Added GetAlertConfigs to AlertStore Signed-off-by: Marco Pracucci <marco@pracucci.com> * Replace ListAlertConfigs with ListAllUsers + GetAlertConfigs in MultitenantAlertmanager Signed-off-by: Marco Pracucci <marco@pracucci.com> * Removed unused ListAlertConfigs Signed-off-by: Marco Pracucci <marco@pracucci.com> * Replace noopAlertStore with a the filesystem-based storage Signed-off-by: Marco Pracucci <marco@pracucci.com> * Concurrently load alertmanager configs from object storage Signed-off-by: Marco Pracucci <marco@pracucci.com> * Added CHANGELOG entry Signed-off-by: Marco Pracucci <marco@pracucci.com> * Fixed PR number in CHANGELOG entry Signed-off-by: Marco Pracucci <marco@pracucci.com> * Fixed linter Signed-off-by: Marco Pracucci <marco@pracucci.com> * Addressed nits in reviews Signed-off-by: Marco Pracucci <marco@pracucci.com> * Improved unit tests Signed-off-by: Marco Pracucci <marco@pracucci.com> * Created concurrency.ForEach() utility which breaks on first error and used it Signed-off-by: Marco Pracucci <marco@pracucci.com> * Simplify alert store used in unit tests Signed-off-by: Marco Pracucci <marco@pracucci.com> * Simplified ForEachUser() and ForEach() utilities Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci added 8 commits March 3, 2021 09:07

Added ListAllUsers() to AlertStore

7f70480

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Added local store unit tests

80a8272

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Added GetAlertConfigs to AlertStore

b4a8f98

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Replace ListAlertConfigs with ListAllUsers + GetAlertConfigs in Multi…

2e7af9d

…tenantAlertmanager Signed-off-by: Marco Pracucci <marco@pracucci.com>

Removed unused ListAlertConfigs

ef4d715

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Replace noopAlertStore with a the filesystem-based storage

f660c9f

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Concurrently load alertmanager configs from object storage

4045175

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Added CHANGELOG entry

00e4ac3

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci requested review from pstibrany and jtlisi March 3, 2021 09:18

pull-request-size bot added the size/XL label Mar 3, 2021

Fixed PR number in CHANGELOG entry

d403870

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci commented Mar 3, 2021

View reviewed changes

Fixed linter

8fcbd51

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pstibrany reviewed Mar 3, 2021

View reviewed changes

gotjosh approved these changes Mar 3, 2021

View reviewed changes

pracucci added 2 commits March 3, 2021 16:32

Addressed nits in reviews

69d68a5

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Improved unit tests

e5375f1

Signed-off-by: Marco Pracucci <marco@pracucci.com>

gotjosh approved these changes Mar 3, 2021

View reviewed changes

pracucci added 3 commits March 3, 2021 17:05

Created concurrency.ForEach() utility which breaks on first error and…

1b303fe

… used it Signed-off-by: Marco Pracucci <marco@pracucci.com>

Simplify alert store used in unit tests

73b3eee

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Simplified ForEachUser() and ForEach() utilities

193731c

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pstibrany approved these changes Mar 4, 2021

View reviewed changes

pracucci merged commit 2b94533 into cortexproject:master Mar 4, 2021

pracucci deleted the optimise-alertmanager-config-loading branch March 4, 2021 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimise alertmanager config loading #3898

Optimise alertmanager config loading #3898

pracucci commented Mar 3, 2021

pracucci Mar 3, 2021

pstibrany Mar 3, 2021

pracucci Mar 3, 2021

pstibrany Mar 3, 2021

pstibrany Mar 3, 2021

pracucci Mar 3, 2021

pracucci Mar 3, 2021

pracucci Mar 3, 2021

pstibrany left a comment

pstibrany Mar 3, 2021

pstibrany Mar 3, 2021 •

edited

pracucci Mar 3, 2021

pstibrany Mar 3, 2021

gotjosh left a comment

pstibrany left a comment


		alertmanagers, err := am.ring.Get(ringHasher.Sum32(), RingOp, nil, nil, nil)
		alertmanagers, err := am.ring.Get(shardByUser(userID), RingOp, nil, nil, nil)

Optimise alertmanager config loading #3898

Optimise alertmanager config loading #3898

Conversation

pracucci commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany Mar 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany Mar 3, 2021 •

edited