AlertStore: implementation using thanos objstore bucket client #3888

pracucci · 2021-02-26T17:12:57Z

What this PR does:
Similarly to #3805, in this PR I'm introducing the Thanos object store client support to the alertmanager backend clients. The config structure follows the same pattern used for the ruler in #3805.

Important: the BucketAlertStore is not using UserBucketClient yet (so it's not supporting S3 SSE per-tenant overrides) because it will need some changes, due to the structure of the alertmanager config in the bucket. I prefer to do it in a separate PR to keep the diff easier to review.

The ListAlertConfigs is not optimised (configs are loaded 1-by-1). I plan to optimise it in a follow up PR. The problem is that the caller is not optimised (we load all tenants configs and then we filter by shard's ownership). I will fix this in a separate PR.

I suggest to review changes with "Hide whitespace changes" enabled.

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]

stevesg · 2021-03-01T12:07:32Z

pkg/alertmanager/alertstore/bucketclient/bucket_client.go

+
+		cfg, err := s.getAlertConfig(ctx, userID)
+		if err != nil {
+			return errors.Wrapf(nil, "failed to fetch alertmanager config for user %s", userID)


I think Wrapf(nil, ...) will always return nil here - https://pkg.go.dev/github.com/pkg/errors#Wrapf

Did you mean Errorf() here?

Damn, good job spotting it! I meant Wrapf(err, ...).

stevesg · 2021-03-01T12:20:23Z

pkg/alertmanager/alertstore/config.go

+
+// NewLegacyAlertStore returns a new alertmanager storage backend poller and store
+func NewLegacyAlertStore(cfg LegacyConfig, logger log.Logger) (AlertStore, error) {
+	if cfg.Type == "configdb" {


Use the ConfigDB constant here?

stevesg · 2021-03-01T12:24:05Z

pkg/alertmanager/alertstore/config.go

-// Config configures the alertmanager backend
-type Config struct {
+const (
+	ConfigDB = "configdb"


Does this need to be exposed?

No, it doesn't.

stevesg · 2021-03-01T12:27:14Z

pkg/alertmanager/alertstore/config.go

-	return objectclient.NewAlertStore(client), nil
+
+	store := bucketclient.NewBucketAlertStore(bucketClient, cfgProvider, logger)
+	if err != nil {


NewBucketAlertStore doesn't return an error - no need to check err.

Can just return bucketclient.NewBucketAlertStore(...), nil

pracucci · 2021-03-01T14:23:21Z

pkg/alertmanager/multitenant_test.go

@@ -43,38 +47,6 @@ receivers:
  - name: dummy`
 )

-// basic easily configurable mock
-type mockAlertStore struct {


Removed in favour of using a real implementation (the one backed by filesystem) or the bucket.ClientMock when we need to mock the backend (eg. simulate a failure).

…Config Signed-off-by: Marco Pracucci <[email protected]>

Signed-off-by: Marco Pracucci <[email protected]>

stevesg

LGTM!

jtlisi

LGTM, great work

jtlisi · 2021-03-01T23:04:15Z

pkg/alertmanager/multitenant_test.go

@@ -43,38 +47,6 @@ receivers:
  - name: dummy`
 )

-// basic easily configurable mock
-type mockAlertStore struct {


jtlisi · 2021-03-01T23:09:23Z

pkg/alertmanager/alertstore/store.go

+	if cfg.Backend == local.Name {
+		return local.NewStore(cfg.Local)
+	}


thought: When I updated the ruler client, I didn't port over the local implementation, just the configdb. My main thought was that it is readonly and mainly used for testing and the new client will already have a filesystem/inmem implementation out of the box. I figured I'd leave it in the legacy config for the time being to avoid confusions with the filesystem backend. However, I'm fine keeping it since there are some desirable properties to a statically configured read-only Alertmanager/RuleStorage backend. Up to you if you think it's worth porting over to the config style or if we should deprecate it when we remove the old config in the future.

If we add it here, we should also port it over to the new ruler config backend as well.

Good point. My point was to deprecate the old config as soon as we complete the rollout in prod (and we confirm the new config works fine). Unless we want to completely remove the local support, then I think the new config should support local too so that we can completely remove the old config in 2 releases.

pstibrany

Very nice, thanks!

pracucci requested review from pstibrany and jtlisi February 26, 2021 17:12

pull-request-size bot added the size/XL label Feb 26, 2021

pracucci marked this pull request as draft February 26, 2021 17:18

stevesg reviewed Mar 1, 2021

View reviewed changes

pracucci force-pushed the add-thanos-bucket-client-support-to-alertmanager branch from 61fcd78 to 67ea9f7 Compare March 1, 2021 13:39

pracucci commented Mar 1, 2021

View reviewed changes

pull-request-size bot added size/XXL and removed size/XL labels Mar 1, 2021

pracucci added 15 commits March 1, 2021 15:40

Renamed NewAlertStore to NewLegacyAlertStore and its config to Legacy…

ea4d3b3

…Config Signed-off-by: Marco Pracucci <[email protected]>

Added BucketAlertStore implementation + unit tests

ad38838

Signed-off-by: Marco Pracucci <[email protected]>

Added new config to Cortex one

eb34f9a

Signed-off-by: Marco Pracucci <[email protected]>

Mounted NewAlertStore() in initAlertManager()

559f4e3

Signed-off-by: Marco Pracucci <[email protected]>

Ensure no global logger in pkg/alertmanager/alertstore/

fecbfd4

Signed-off-by: Marco Pracucci <[email protected]>

Updated development/ config to test alertmanager with new storage client

702865a

Signed-off-by: Marco Pracucci <[email protected]>

Fixed BucketAlertStore

a602fe4

Signed-off-by: Marco Pracucci <[email protected]>

Added CHANGELOG entry

441fe8e

Signed-off-by: Marco Pracucci <[email protected]>

Updated CHANGELOG entry

975e757

Signed-off-by: Marco Pracucci <[email protected]>

Fixed code comment

a5f5d1e

Signed-off-by: Marco Pracucci <[email protected]>

Addressed review comments

f04721f

Signed-off-by: Marco Pracucci <[email protected]>

Added 'local' support to new config and simplified unit tests

1ad18a5

Signed-off-by: Marco Pracucci <[email protected]>

Moved NewAlertStore and NewLegacyAlertStore from config.go to store.go

3be2313

Signed-off-by: Marco Pracucci <[email protected]>

Do not use mockAlertStore in unit tests

175564e

Signed-off-by: Marco Pracucci <[email protected]>

Updated integration tests

c0e225f

Signed-off-by: Marco Pracucci <[email protected]>

pracucci force-pushed the add-thanos-bucket-client-support-to-alertmanager branch from 4a47c3f to c0e225f Compare March 1, 2021 14:41

pracucci marked this pull request as ready for review March 1, 2021 14:41

stevesg approved these changes Mar 1, 2021

View reviewed changes

jtlisi approved these changes Mar 1, 2021

View reviewed changes

pstibrany approved these changes Mar 2, 2021

View reviewed changes

pracucci merged commit 003eb33 into cortexproject:master Mar 2, 2021

pracucci deleted the add-thanos-bucket-client-support-to-alertmanager branch March 2, 2021 16:50

pracucci mentioned this pull request Mar 24, 2021

Deprecated old alertmanager storage config #4002

Merged

3 tasks

This was referenced May 20, 2021

Alertmanager fails to read fallback config #3395

Closed

Document how to drive Ruler and AlertManager config #4206

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AlertStore: implementation using thanos objstore bucket client #3888

AlertStore: implementation using thanos objstore bucket client #3888

pracucci commented Feb 26, 2021 •

edited

Loading

stevesg Mar 1, 2021

pracucci Mar 1, 2021

stevesg Mar 1, 2021

stevesg Mar 1, 2021

pracucci Mar 1, 2021

stevesg Mar 1, 2021

pracucci Mar 1, 2021

jtlisi Mar 1, 2021

stevesg left a comment

jtlisi left a comment

jtlisi Mar 1, 2021

jtlisi Mar 1, 2021

pracucci Mar 2, 2021

jtlisi Mar 2, 2021

pstibrany left a comment

AlertStore: implementation using thanos objstore bucket client #3888

AlertStore: implementation using thanos objstore bucket client #3888

Conversation

pracucci commented Feb 26, 2021 • edited Loading

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

stevesg left a comment

Choose a reason for hiding this comment

jtlisi left a 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

pracucci commented Feb 26, 2021 •

edited

Loading