Add Alertmanager Integration Tests and Static File Backend #1686

jtlisi · 2019-09-20T14:32:32Z

Overview

Abstract the ConfigDB client behind the alert store interface
Add Integration tests for the Alertmanager that ensures the proper functionality of the v1 api
Add a static file config backend for the Alertmanager

Important Detail

pkg/alertmanager/multitenant_test.go is run with !race tag because of an underlying mild race condition in the Alertmanager. https://github.com/prometheus/alertmanager/issues/2182

First time I'm seeing the multi-tenant alertmanager and config DB, so I have have miss something. I left some comments. All in all, looks doing it job. I left several comments to polish it up a bit (especially from the user perspective), plus I think we're not correctly handling the case an user config is removed (see comment in multitenant.go).

pracucci · 2020-01-22T17:38:02Z

pkg/alertmanager/storage.go

+// AlertStoreConfig configures the alertmanager backend
+type AlertStoreConfig struct {
+	Type     string `yaml:"type"`
+	ConfigDB client.Config


Some thoughts:

I would suggest to make the YAML field name explict. Given the type could be file or configdb, we may call these fields with the same name (to keep it specular), so yaml:"configdb" and yaml:"file". However, file is a very bad name because very generic.

I've seen you've also used "local" sometimes in the code: maybe it's a better name for file?

Is really a value naming the DB type configdb? Isn't just a db the backend storage?

What's your take?

I think local is a better fit. It's what we use to designate the boltdb backend for chunks

pracucci · 2020-01-22T17:44:55Z

pkg/alertmanager/storage.go

+	cfg.File.RegisterFlags(f)
+	cfg.ConfigDB.RegisterFlagsWithPrefix("alertmanager.", f)
+	f.StringVar(&cfg.Type, "alertmanager.storage.type", "configdb", "Method to use for backend rule storage (configdb, file)")


The CLI flags should have all the same prefix, then:

.type

.<type>.<option> (ie. .file.path)

Right now there's a bit of inconsistency:

-alertmanager.storage.type string -alertmanager.storage.local.path string -alertmanager.configs.auto-webhook-root string -alertmanager.configs.client-timeout duration -alertmanager.configs.fallback string -alertmanager.configs.poll-interval duration -alertmanager.configs.url value

I think everything related to backend config storage should have a alertmanager.storage.. However, I wanted to avoid making a breaking change in this PR and updating the prefix for alertmanager.configs to alertmanager.storage.configs would force users to change their flags. I can add a TODO to make that change in the future and go throough a change/deprecation process in a future PR?

pracucci · 2020-01-22T17:46:09Z

pkg/alertmanager/storage.go

+	case "file":
+		return local.NewFileAlertStore(cfg.File)
+	default:
+		return nil, fmt.Errorf("Unrecognized rule storage mode %v, choose one of: configdb, file", cfg.Type)


mode > type given we call like this in the config

rule > replacing it with alertmanager is more clear to the final user

Good catch, a lot of this code is based on the Ruler.

pracucci · 2020-01-22T17:47:35Z

pkg/alertmanager/local/alert_client.go

@@ -0,0 +1,71 @@
+package local


I would suggest to name the package and file based on the content. For example, local/alert_client.go makes me think about "local" storage (while it's named file) and "client" is actually code "store" in the code.

pracucci · 2020-01-22T17:49:57Z

pkg/alertmanager/local/alert_client.go

+
+// RegisterFlags registers flags related to the alertmanager file store
+func (cfg *FileAlertStoreConfig) RegisterFlags(f *flag.FlagSet) {
+	f.StringVar(&cfg.Path, "alertmanager.storage.local.path", "/etc/cortex/alertmanager_configs/", "Path at which alertmanager configurations are stored.")


Is /etc/cortex/alertmanager_configs/ a good default? We don't have any default filepath in that location. Looks more inherited by our cluster setup, than picked to be a good default to me.

pkg/alertmanager/multitenant.go

pracucci · 2020-01-22T18:24:09Z

pkg/alertmanager/alerts/compat.go

+package alerts
+
+// ToProto con
+func ToProto(cfg string, templates map[string]string, user string) (*AlertConfigDesc, error) {


Who uses it?

Not needed in this PR, I'll remove this file

pracucci · 2020-01-22T18:24:12Z

pkg/alertmanager/alerts/compat.go

+}
+
+// ParseTemplates returns a alertmanager config object
+func ParseTemplates(cfg *AlertConfigDesc) map[string]string {


Who uses it?

Not needed in this PR, I'll remove this file

pkg/alertmanager/alerts/alerts.proto

jtlisi · 2020-01-29T06:36:34Z

@pracucci this should be good for a second look

pracucci

Good job @jtlisi! I think it's in a pretty good shape. I've left few nits, which should be quick to address. I haven't manually tested it, please don't forget to test all features with the latest version of the code 🙏

pracucci · 2020-01-30T10:44:05Z

docs/configuration/config-file-reference.md

-# The configstore_config configures the config database storing rules and
-# alerts, and is used by the Cortex alertmanager.
-# The CLI flags prefix for this block config is: alertmanager
-[config_store: <configstore_config>]


Please add an entry to CHANGELOG.md explaning all changes (this is a config breaking change).

pracucci · 2020-01-30T10:46:34Z

pkg/cortex/modules.go

@@ -446,7 +446,7 @@ func (t *Cortex) stopConfigs() error {
 }

 func (t *Cortex) initAlertmanager(cfg *Config) (err error) {
-	t.alertmanager, err = alertmanager.NewMultitenantAlertmanager(&cfg.Alertmanager, cfg.ConfigStore)
+	t.alertmanager, err = alertmanager.NewMultitenantAlertmanager(&cfg.Alertmanager)
 	if err != nil {
 		return


return err

We specify the err variable in the function signature and utilize a naked return pattern throughout the modules.go file. I personally like to explicitly return the error so I'll update it.

pracucci · 2020-01-30T10:50:58Z

pkg/alertmanager/multitenant.go

 	if err != nil {
-		level.Warn(util.Logger).Log("msg", "MultitenantAlertmanager: configs server poll failed", "err", err)
+		level.Warn(util.Logger).Log("msg", "MultitenantAlertmanager: configs alert store poll failed", "err", err)


This MultitenantAlertmanager: prefix repeated across all logs looks a bit annoying. If you want to clean it up, you may pick the logger in input in NewMultitenantAlertmanager(), wrap it with log.With(util.Logger, "component", "multiteant-alertmanager") and then always use that internally.

Up to you.

I'll use an embedded logger with a component field.

pracucci · 2020-01-30T10:53:38Z

pkg/alertmanager/multitenant.go

 }

-func (am *MultitenantAlertmanager) addNewConfigs(cfgs map[string]configs.View) {
+func (am *MultitenantAlertmanager) syncConfigs(cfgs map[string]alerts.AlertConfigDesc) {
 	// TODO: instrument how many configs we have, both valid & invalid.


What's this TODO about? Doable in this PR too?

I broke the totalConfigs metric up to have a status label

pracucci · 2020-01-30T10:59:02Z

pkg/alertmanager/multitenant.go

 		am.alertmanagersMtx.Unlock()
-	} else if am.cfgs[userID].AlertmanagerConfig != config.AlertmanagerConfig || hasTemplateChanges {
+	} else if am.cfgs[cfg.User].RawConfig != cfg.RawConfig || hasTemplateChanges {
+		level.Debug(util.Logger).Log("msg", "MultitenantAlertmanager: updating new alertmanager tenant", "user", cfg.User)


This may be an Info

pracucci · 2020-01-30T11:19:44Z

pkg/alertmanager/alerts/store.go

@@ -0,0 +1,64 @@
+package alerts


Being the configdb store, may make sense to move it to pkg/alertmanager/alerts/configdb/store.go and just move the AlertStore interface back into pkg/alertmanager/alerts?

Makes sense, I will also move the pkg/alertmananger/local package to be pkg/alertmanager/alerts/local

pracucci · 2020-01-30T11:20:33Z

pkg/alertmanager/alerts/store.go

+}
+
+// ConfigAlertStore is a concrete implementation of RuleStore that sources rules from the config service
+type ConfigAlertStore struct {


ConfigDBAlertStore would be more appropriate? Just asking, up to you.

pracucci · 2020-01-30T11:23:21Z

pkg/alertmanager/alerts/store.go

+type ConfigAlertStore struct {
+	configClient client.Client
+	since        configs.ID
+	alertConfigs map[string]AlertConfigDesc


Why do we need to retain it? Looking at ListAlertConfigs() implementation looks not useful to me.

configs, err := c.configClient.GetAlerts(ctx, c.since)

Will only return configs that have IDs greater than c.since. This avoids the configdb having to do a full pull of it's table.

pracucci · 2020-01-30T11:24:20Z

pkg/alertmanager/local/store.go

+				return err
+			}
+
+			user := strings.TrimSuffix(info.Name(), ext)


As previously mentioned, I would add a comment to explain that the filename must match the user ID.

Sorry I didn't rectify this previously

pracucci · 2020-01-30T11:25:36Z

pkg/alertmanager/local/store.go

+
+		ext := filepath.Ext(info.Name())
+
+		if !info.IsDir() && (ext == ".yml" || ext == ".yaml") {


As previously mentioned, I would reverse this into a guard. Looks easier to read the code, if we return nil in case it's not a file or not a YAML extension.

I would also suggest to log with a Warn any skipped entry, because we wouldn't expect any spurious file/directory within f.cfg.Path, right?

Sorry I didn't rectify this previously

pracucci

Bash-based integration tests are no more a thing. I see two options:

Strip away integration tests from this PR and eventually open a new PR for them
Move these tests to the new Go integration tests

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

jtlisi · 2020-02-12T22:09:36Z

Closing in favor of #2125

jtlisi force-pushed the 20190916_file_backed_alertmanager branch 2 times, most recently from eaa9211 to b32dbb3 Compare October 11, 2019 02:36

jtlisi force-pushed the 20190916_file_backed_alertmanager branch 2 times, most recently from d079fa6 to e574088 Compare January 15, 2020 16:45

pracucci reviewed Jan 22, 2020

View reviewed changes

jtlisi force-pushed the 20190916_file_backed_alertmanager branch from 4f81964 to 65b264b Compare January 28, 2020 22:42

pracucci reviewed Jan 30, 2020

View reviewed changes

jtlisi mentioned this pull request Feb 11, 2020

Fix panic when alertmanager applies configs #2114

Merged

1 task

jtlisi force-pushed the 20190916_file_backed_alertmanager branch from 712b5e2 to 545835c Compare February 11, 2020 18:42

pracucci reviewed Feb 12, 2020

View reviewed changes

jtlisi added 17 commits February 12, 2020 13:16

refactor alertmanager storage and add static file alert store

27c77c7

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

add integration test for alertmananger

858d1ec

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

revert client

6d3405b

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

delete unneeded files

056bc51

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

refactor to match rebase

5497b46

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

fix struct import

71b73ed

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

fix interface change

0b04e5b

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

remove unused function

c4935b2

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

remove unused config

ffcc546

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

instantiate map

0ac7460

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

address linting issues around file/local nomenclature

4409f94

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

ensure user alertmanagers are deleted

caf9db7

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

rename addNewConfigs to syncConfigs

f2e7846

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

rename server to store

5315819

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

fix comments to use store language

513c1ad

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

remove unused file

7a0ddb0

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

refactor logic when setting map

26de24d

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

jtlisi added 11 commits February 12, 2020 13:16

add unit tests for syncing alertmanager configs

1c036ee

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

goformat test file

2086938

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

fix mild race condition

c6e8c3a

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

update docs

2932615

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

remove mutex and stop linearly

4eaefac

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

revert change to dispatcher

33dc8c5

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

revert alermanager to original file and remove delete test case

fae9c12

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

fix alertmanager test

d461ee6

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

refactor per PR comments

765581b

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

add Go based integration test

bf4b4d2

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

rebase alertmanager integration test

cbd8c18

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

jtlisi force-pushed the 20190916_file_backed_alertmanager branch from 545835c to cbd8c18 Compare February 12, 2020 18:20

jtlisi added 6 commits February 12, 2020 13:25

check errors

d96c619

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

update vendored deps

ef86964

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

revert alertmanager upgrade

5002d5b

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

fix goimports lint

4c1214a

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

revert debugging vendor change

aeca3b5

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

update docs

e7eb89f

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

jtlisi mentioned this pull request Feb 12, 2020

Add Alertmanager Integration Tests and Static File Backend #2125

Merged

jtlisi closed this Feb 12, 2020


		ext := filepath.Ext(info.Name())

		if !info.IsDir() && (ext == ".yml" \|\| ext == ".yaml") {

Add Alertmanager Integration Tests and Static File Backend #1686

Add Alertmanager Integration Tests and Static File Backend #1686

Uh oh!

Conversation

jtlisi commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Important Detail

Related

Uh oh!

bboreham commented Sep 23, 2019

Uh oh!

jtlisi commented Sep 23, 2019

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtlisi commented Jan 29, 2020

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jtlisi commented Sep 20, 2019 •

edited

Loading