Skip to content
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

Deprecated old alertmanager storage config #4002

Merged

Conversation

pracucci
Copy link
Contributor

What this PR does:
In #3888 we introduced the new alertmanager storage config. In this PR I'm proposing to deprecate the old storage config, so that it will be officially marked as deprecated in Cortex 1.9 and then removed in 1.11.

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]

@pracucci pracucci requested a review from jtlisi March 24, 2021 09:37
@@ -4,7 +4,8 @@

* [CHANGE] Alertmanager now removes local files after Alertmanager is no longer running for removed or resharded user. #3910
* [CHANGE] Alertmanager now stores local files in per-tenant folders. Files stored by Alertmanager previously are migrated to new hierarchy. Support for this migration will be removed in Cortex 1.11. #3910
* [CHANGE] Ruler: deprecated `-ruler.storage.*` CLI flags (and their respective YAML config options) in favour of `-ruler-storage.*`. The deprecated config will be removed in Cortex 1.11. #3944
* [CHANGE] Ruler: deprecated `-ruler.storage.*` CLI flags (and their respective YAML config options) in favour of `-ruler-storage.*`. The deprecated config will be removed in Cortex 1.11. #3945
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed the PR number here. It was wrong.

@pracucci pracucci merged commit 23c32c2 into cortexproject:master Mar 25, 2021
@pracucci pracucci deleted the deprecate-old-alertmanager-storage branch March 25, 2021 08:40
@bboreham
Copy link
Contributor

I'm confused: is this about storage or about config? The former is for things like silences and the latter is for things like routing rules.

For example -alertmanager.storage.path was used for silences.

@@ -4,7 +4,8 @@

* [CHANGE] Alertmanager now removes local files after Alertmanager is no longer running for removed or resharded user. #3910
* [CHANGE] Alertmanager now stores local files in per-tenant folders. Files stored by Alertmanager previously are migrated to new hierarchy. Support for this migration will be removed in Cortex 1.11. #3910
* [CHANGE] Ruler: deprecated `-ruler.storage.*` CLI flags (and their respective YAML config options) in favour of `-ruler-storage.*`. The deprecated config will be removed in Cortex 1.11. #3944
* [CHANGE] Ruler: deprecated `-ruler.storage.*` CLI flags (and their respective YAML config options) in favour of `-ruler-storage.*`. The deprecated config will be removed in Cortex 1.11. #3945
* [CHANGE] Alertmanager: deprecated `-alertmanager.storage.*` CLI flags (and their respective YAML config options) in favour of `-alertmanager-storage.*`. The deprecated config will be removed in Cortex 1.11. #4002
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can see this is not true for these two fields:

f.StringVar(&cfg.DataDir, "alertmanager.storage.path", "data/", "Base path for data storage.")
f.DurationVar(&cfg.Retention, "alertmanager.storage.retention", 5*24*time.Hour, "How long to keep data for.")

@jtlisi
Copy link
Contributor

jtlisi commented May 11, 2021

@bboreham

I'm confused: is this about storage or about config? The former is for things like silences and the latter is for things like routing rules.

This is about deprecating the object storage backend for storing each tenants alertmanager config.

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

Successfully merging this pull request may close these issues.

5 participants