Skip to content

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)

@@ -1,8 +1,8 @@
# Changelog

## 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
* `--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 <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
@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