Cleanup obsolete local files for alertmanager. #3910

pstibrany · 2021-03-04T14:48:45Z

What this PR does: This PR implements cleanup of local files on alertmanager when AM is no longer running for given user.

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci

Good job! I'm a bit scared about the alertmanagersMtx locking complexity in MultitenantAlertmanager. I'm wondering if it's time to properly address it (with a per-tenant alertmanager lock and a state to CAS on it, so we can safely handle all cases). I'm also open to work on such refactoring.

pkg/alertmanager/multitenant.go

pkg/alertmanager/alertmanager.go

CHANGELOG.md

pkg/alertmanager/multitenant.go

CHANGELOG.md

pstibrany · 2021-03-08T11:23:51Z

I've pushed next version of PR that implemens following changes:

per-tenant state is now kept in per-tenant directory, under cfg.DataDir
cleanup for the user means deleting entire directory
there is a migration procedure, that moves existing nflog:<tenantID>, silences:<tenantID> and templates into per-tenant directory on startup.

I'd be interested to hear opinions on these design decisions. (Tests are not yet updated to cover this, so please ignore tests for now)

One con of this approach is that reverting back to previous version of AM will ignore existing snapshot files in per-tenant directory. Hopefully moving from new to previous AM version is rare.

/cc @gotjosh @pracucci @stevesg

pracucci

Good job! Changes LGTM. I've left a couple of nits and a comment about testing.

CHANGELOG.md

pracucci · 2021-03-08T11:55:14Z

CHANGELOG.md

@@ -2,6 +2,8 @@

 ## master / unreleased

+* [CHANGE] Alertmanager: clean obsolete local files after Alertmanager is no longer running for removed or resharded user. #3910


I'm not sure I understand this CHANGELOG entry. Also remember to mention that the path where each tenant alertmanager data is stored has changed.

pkg/alertmanager/multitenant.go

stevesg

Only a quick pass over. I had a question though:

there is a migration procedure, that moves existing nflog:, silences: and > templates into per-tenant directory on startup.

I'm wondering what the benefit is from having the migration - as there is some complexity here we could avoid. Is listing out the files for a particular user difficult in some way? Or perhaps are we worried about the number of files in a directory?

Not saying one way or the other, just curious on the reasoning.

pkg/alertmanager/multitenant.go

pstibrany · 2021-03-08T14:59:08Z

I'm wondering what the benefit is from having the migration - as there is some complexity here we could avoid. Is listing out the files for a particular user difficult in some way? Or perhaps are we worried about the number of files in a directory?

Not saying one way or the other, just curious on the reasoning.

Rationale behind single directory per tenant is to make it easy to delete tenant files when needed. Previous version of this PR only deleted silence and nflog file, but today I've found that I've missed template files – which then led to idea of using per-tenant directory.

Reasoning behind migrating is to avoid losing notifications and silences state. Even though this state is propagated by other alertmanagers via gossip, it takes some time and in the meantime missing silences could cause spurious notifications. Note that this problem still exists when doing downgrade to previous AM, which doesn't understand new structure.

PR has changed since it was approved, please take a look again.

pstibrany · 2021-03-08T20:27:31Z

PR is now ready for review. Since original version, it has also added change to how files are stored locally. I've also extended unit tests to cover use of templates, that were missing.

Signed-off-by: Peter Štibraný <[email protected]>

Add test for migration. Fix test for deletion of unused dirs. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

pracucci

Good job, LGTM! I left few nits just to be a bit more clear in log messages (some log messages are generic and the final user may be lost reading them).

pkg/alertmanager/multitenant.go

pracucci · 2021-03-10T08:17:02Z

pkg/alertmanager/multitenant.go

 func (am *MultitenantAlertmanager) newAlertmanager(userID string, amConfig *amconfig.Config, rawCfg string) (*Alertmanager, error) {
 	reg := prometheus.NewRegistry()

+	tenantDir := am.getTenantDirectory(userID)
+	err := os.MkdirAll(tenantDir, 0777)


Suggested change

err := os.MkdirAll(tenantDir, 0777)

err := os.MkdirAll(tenantDir, os.ModePerm)

pracucci · 2021-03-10T08:17:14Z

pkg/alertmanager/multitenant.go

+
+	for userID, files := range st {
+		tenantDir := am.getTenantDirectory(userID)
+		err := os.MkdirAll(tenantDir, 0777)


Suggested change

err := os.MkdirAll(tenantDir, 0777)

err := os.MkdirAll(tenantDir, os.ModePerm)

While it is the same value, the intent is not the same. os.ModePerm is simply all permission-bits set and defined next to other Mode* constants for higher-bits. Note that there is no such constant for files (0666, without exec-bit).

After the fact since this is merged, but why do we need 0777 instead of 0755? Every one of these causes some pain for anyone using source scanning tools for :( I know we have some other similar permissions, so there may be a good reason, but want to understand if so.

The reason for using wide permissions is mostly for consistency with vendored Prometheus code, which already uses very wide permissions (see prometheus/prometheus#7782). Cortex users that want reduced permissions need to use umask to do so. (I’m not quite sure how to do that in Kubernetes though).

pkg/alertmanager/multitenant.go

stevesg

LGTM, just a unit test nit.

pkg/alertmanager/multitenant_test.go

Co-authored-by: Marco Pracucci <[email protected]> Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

* Cleanup obsolete local files for alertmanager. Signed-off-by: Peter Štibraný <[email protected]> * CHANGELOG.md Signed-off-by: Peter Štibraný <[email protected]> * Comment. Signed-off-by: Peter Štibraný <[email protected]> * Don't ignore directories. Log error when deletion fails instead. Signed-off-by: Peter Štibraný <[email protected]> * Address review feedback. Signed-off-by: Peter Štibraný <[email protected]> * Move per-tenant state into tenant directory to simplify cleanup. Signed-off-by: Peter Štibraný <[email protected]> * Move migration to separate function. Add test for migration. Fix test for deletion of unused dirs. Signed-off-by: Peter Štibraný <[email protected]> * Store templates to correct place. Signed-off-by: Peter Štibraný <[email protected]> * CHANGELOG.md Signed-off-by: Peter Štibraný <[email protected]> * Verify that templates are stored properly into correct location. Signed-off-by: Peter Štibraný <[email protected]> * Comments. Signed-off-by: Peter Štibraný <[email protected]> * Comments. Signed-off-by: Peter Štibraný <[email protected]> * Apply suggestions from code review Co-authored-by: Marco Pracucci <[email protected]> Signed-off-by: Peter Štibraný <[email protected]> * Review feedback. Signed-off-by: Peter Štibraný <[email protected]> Co-authored-by: Marco Pracucci <[email protected]>

pull-request-size bot added the size/L label Mar 4, 2021

pstibrany requested a review from pracucci March 4, 2021 14:48

pracucci reviewed Mar 4, 2021

View reviewed changes

pkg/alertmanager/multitenant.go Outdated Show resolved Hide resolved

pkg/alertmanager/multitenant.go Show resolved Hide resolved

gotjosh reviewed Mar 4, 2021

View reviewed changes

pstibrany commented Mar 8, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pstibrany force-pushed the alertmanager-cleanup-local-files branch from b3d987d to abecf4c Compare March 8, 2021 08:19

pracucci previously approved these changes Mar 8, 2021

View reviewed changes

stevesg reviewed Mar 8, 2021

View reviewed changes

pkg/alertmanager/multitenant.go Outdated Show resolved Hide resolved

pstibrany force-pushed the alertmanager-cleanup-local-files branch from 69a9383 to e432d82 Compare March 8, 2021 17:16

pstibrany requested a review from pracucci March 8, 2021 20:24

pstibrany added 12 commits March 8, 2021 22:09

Cleanup obsolete local files for alertmanager.

aabf7c6

Signed-off-by: Peter Štibraný <[email protected]>

CHANGELOG.md

8eb1cf1

Signed-off-by: Peter Štibraný <[email protected]>

Comment.

a39b3ad

Signed-off-by: Peter Štibraný <[email protected]>

Don't ignore directories. Log error when deletion fails instead.

27d19cb

Signed-off-by: Peter Štibraný <[email protected]>

Address review feedback.

2d85dc6

Signed-off-by: Peter Štibraný <[email protected]>

Move per-tenant state into tenant directory to simplify cleanup.

0f5c25a

Signed-off-by: Peter Štibraný <[email protected]>

Move migration to separate function.

88fb03c

Add test for migration. Fix test for deletion of unused dirs. Signed-off-by: Peter Štibraný <[email protected]>

Store templates to correct place.

1fb06ca

Signed-off-by: Peter Štibraný <[email protected]>

CHANGELOG.md

27efd7f

Signed-off-by: Peter Štibraný <[email protected]>

Verify that templates are stored properly into correct location.

3ca9ac9

Signed-off-by: Peter Štibraný <[email protected]>

Comments.

31ae118

Signed-off-by: Peter Štibraný <[email protected]>

Comments.

de67abb

Signed-off-by: Peter Štibraný <[email protected]>

pstibrany force-pushed the alertmanager-cleanup-local-files branch from 5f6bdc5 to de67abb Compare March 8, 2021 21:09

pracucci approved these changes Mar 10, 2021

View reviewed changes

stevesg approved these changes Mar 10, 2021

View reviewed changes

pkg/alertmanager/multitenant_test.go Outdated Show resolved Hide resolved

Apply suggestions from code review

8e944e0

Co-authored-by: Marco Pracucci <[email protected]> Signed-off-by: Peter Štibraný <[email protected]>

Review feedback.

6746059

Signed-off-by: Peter Štibraný <[email protected]>

pstibrany force-pushed the alertmanager-cleanup-local-files branch from bcf060a to 6746059 Compare March 10, 2021 16:43

pstibrany merged commit f485bc8 into cortexproject:master Mar 11, 2021

stevesg mentioned this pull request Mar 29, 2021

Extend alertstore to support read/write of clusterpb.FullState. #4020

Merged

3 tasks

pstibrany mentioned this pull request Jun 28, 2022

Removed migration of alertmanager local state files from old hierarchy (Cortex 1.8 and earlier) grafana/mimir#2253

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup obsolete local files for alertmanager. #3910

Cleanup obsolete local files for alertmanager. #3910

pstibrany commented Mar 4, 2021

pracucci left a comment

pstibrany commented Mar 8, 2021 •

edited

Loading

pracucci left a comment

pracucci Mar 8, 2021

stevesg left a comment

pstibrany commented Mar 8, 2021

pstibrany commented Mar 8, 2021

pracucci left a comment

pracucci Mar 10, 2021

pracucci Mar 10, 2021

pstibrany Mar 10, 2021

ranton256 Mar 11, 2021

pstibrany Mar 11, 2021

stevesg left a comment

		@@ -2,6 +2,8 @@

		## master / unreleased

		* [CHANGE] Alertmanager: clean obsolete local files after Alertmanager is no longer running for removed or resharded user. #3910

	err := os.MkdirAll(tenantDir, 0777)
	err := os.MkdirAll(tenantDir, os.ModePerm)

Cleanup obsolete local files for alertmanager. #3910

Cleanup obsolete local files for alertmanager. #3910

Conversation

pstibrany commented Mar 4, 2021

pracucci left a comment

Choose a reason for hiding this comment

pstibrany commented Mar 8, 2021 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

pracucci Mar 8, 2021

Choose a reason for hiding this comment

stevesg left a comment

Choose a reason for hiding this comment

pstibrany commented Mar 8, 2021

pstibrany commented Mar 8, 2021

pracucci left a comment

Choose a reason for hiding this comment

pracucci Mar 10, 2021

Choose a reason for hiding this comment

pracucci Mar 10, 2021

Choose a reason for hiding this comment

pstibrany Mar 10, 2021

Choose a reason for hiding this comment

ranton256 Mar 11, 2021

Choose a reason for hiding this comment

pstibrany Mar 11, 2021

Choose a reason for hiding this comment

stevesg left a comment

Choose a reason for hiding this comment

pstibrany commented Mar 8, 2021 •

edited

Loading