Add Alertmanager Integration Tests and Static File Backend

[jtlisi]

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

Related

#1647
#1244
#1513
#619

[bboreham]

Where does the new protobuf come in?

[jtlisi]

I chose to use a protobuf for the polling interface boundary to provide a easy to use storage format that wasn't yaml based and easy to version. This would make any additional backend based on cloud provider object stores much easier to add, while not reducing any of the current functionality.

[pracucci]

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]

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?

[jtlisi]

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

[pracucci]

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

[jtlisi]

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]

• mode > type given we call like this in the config
• rule > replacing it with alertmanager is more clear to the final user

[jtlisi]

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

[pracucci]

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]

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.

[pracucci]

Who uses it?

[jtlisi]

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

[pracucci]

Who uses it?

[jtlisi]

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

[jtlisi]

@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]

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

[pracucci]

return err

[jtlisi]

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]

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.

[jtlisi]

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

[pracucci]

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

[jtlisi]

I broke the totalConfigs metric up to have a status label

[pracucci]

This may be an Info

[jtlisi]

Agreed.

[pracucci]

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?

[jtlisi]

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

[pracucci]

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

[jtlisi]

SGTM

[pracucci]

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

[jtlisi]

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]

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

[jtlisi]

Sorry I didn't rectify this previously

[pracucci]

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?

[jtlisi]

Sorry I didn't rectify this previously

[pracucci]

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

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

[jtlisi]

Closing in favor of #2125