Skip to content

Add Alertmanager Integration Tests and Static File Backend #2125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 17, 2020
Merged

Add Alertmanager Integration Tests and Static File Backend #2125

merged 3 commits into from
Feb 17, 2020

Conversation

jtlisi
Copy link
Contributor

@jtlisi jtlisi commented Feb 12, 2020

Carried over from #1686 due to CircleCI configs for branches based in the grafana repo affecting integration tests.

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

@jtlisi
Copy link
Contributor Author

jtlisi commented Feb 13, 2020

@pracucci I updated this PR to utilize Go Integration tests. I also re-opened the PR from my github account due to some configs on the Grafana CircleCI account affecting the integration tests. Specifically the NOQUAY config.

@jtlisi jtlisi mentioned this pull request Feb 13, 2020
2 tasks
@jtlisi jtlisi closed this Feb 13, 2020
@jtlisi jtlisi reopened this Feb 13, 2020
@jtlisi jtlisi changed the title 20190916 file backed alertmanager Add Alertmanager Integration Tests and Static File Backend Feb 13, 2020
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job @jtlisi! I left few minor comments but the overall changes LGTM.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing job @jtlisi ! LGTM (with a last minute nit)

@@ -2,7 +2,7 @@

## master / unreleased
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized we didn't mention the introduction of the local storage in the changelog. I think it's worth to mention it.

CHANGELOG.md Outdated
@@ -13,6 +14,7 @@
* `--experimental.distributor.user-subring-size`
* [FEATURE] Added flag `-experimental.ruler.enable-api` to enable the ruler api which implements the Prometheus API `/api/v1/rules` and `/api/v1/alerts` endpoints under the configured `-http.prefix`. #1999
* [FEATURE] Added sharding support to compactor when using the experimental TSDB blocks storage. #2113
* [ENHANCEMENT] Add `status` label to `cortex_alertmanager_configs` metric to guage the number of valid and invalid configs. #2125
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guage > gauge

@pracucci
Copy link
Contributor

The lint issue is unrelated from this PR, but if you rebase master it should be fixed now.

… integration test for alertmananger

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
@jtlisi jtlisi merged commit 4ad61d2 into cortexproject:master Feb 17, 2020
@jtlisi jtlisi deleted the 20190916_file_backed_alertmanager branch February 17, 2020 19:34
@bboreham
Copy link
Contributor

Should #1647 be closed by this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants