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

Alertmanager: Add state size limit #9475

Merged
merged 8 commits into from
Oct 3, 2024

Conversation

titolins
Copy link
Contributor

@titolins titolins commented Sep 30, 2024

@titolins titolins requested review from a team and tacole02 as code owners September 30, 2024 07:28
Copy link
Contributor

@santihernandezc santihernandezc left a comment

Choose a reason for hiding this comment

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

Should we add an entry to the changelog as you did in your last PR?

Copy link
Contributor

@santihernandezc santihernandezc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@santihernandezc
Copy link
Contributor

@titolins is OoO, I'm fixing some issues (mostly conflicts with main in CHANGELOG.md) before merging this PR.

Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Couple of spurious changes seem to have crept in.

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/alertmanager/api.go Outdated Show resolved Hide resolved
@@ -169,8 +169,24 @@ func (am *MultitenantAlertmanager) SetUserGrafanaState(w http.ResponseWriter, r
return
}

payload, err := io.ReadAll(r.Body)
var input io.Reader
Copy link
Contributor

@56quarters 56quarters Oct 1, 2024

Choose a reason for hiding this comment

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

We use the globalerror package for formatting errors returned to users in response to hitting limits. This gives each error a unique name with an associated entry in a runbook along with a message that describes how the limit can be adjusted. Can you please make use of that for this limit and the previously added limit for grafana config size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's nice - tks for pointing it out. Adjusted now 👍

@titolins titolins force-pushed the titolins/implement_config_size_limit branch from b74a1d7 to c66df9e Compare October 2, 2024 15:42
@titolins
Copy link
Contributor Author

titolins commented Oct 2, 2024

squashing the commits to make rebasing easier since CHANGELOG changes are relatively common

@titolins titolins force-pushed the titolins/implement_config_size_limit branch 2 times, most recently from 1fa4dfa to 81ee011 Compare October 2, 2024 17:19
@titolins titolins force-pushed the titolins/implement_config_size_limit branch from 81ee011 to 5b006c9 Compare October 2, 2024 17:21
@titolins
Copy link
Contributor Author

titolins commented Oct 2, 2024

Should be good for another round. Squashed the previous review state in the first commit, all others are new 👍

Copy link
Contributor

@santihernandezc santihernandezc left a comment

Choose a reason for hiding this comment

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

LGTM! Please take a look at my comments.

Comment on lines +3654 to +3655
# Maximum size of the Grafana Alertmanager configuration for a tenant. 0 = no
# limit.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
# Maximum size of the Grafana Alertmanager configuration for a tenant. 0 = no
# limit.
# Maximum size of the Grafana Alertmanager configuration for a tenant. 0 = no limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is auto-generate (make doc)

@@ -3661,6 +3661,10 @@ The `limits` block configures default and per-tenant limits imposed by component
# CLI flag: -alertmanager.max-config-size-bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

Just two lines above this one, the old description for the config size limit should be fixed.

Copy link
Contributor Author

@titolins titolins Oct 2, 2024

Choose a reason for hiding this comment

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

This is the description of the remote AM configuration limit. I had copied the grafana config limit from this, that's why it's almost the same 😅
I've adjusted it now to keep the same wording for both

docs/sources/mimir/manage/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
docs/sources/mimir/manage/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
titolins and others added 4 commits October 2, 2024 20:24
Co-authored-by: Santiago <santiagohernandez.1997@gmail.com>
Co-authored-by: Santiago <santiagohernandez.1997@gmail.com>
Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

Thanks for making that change!

@santihernandezc santihernandezc merged commit 4ba903c into main Oct 3, 2024
29 checks passed
@santihernandezc santihernandezc deleted the titolins/implement_config_size_limit branch October 3, 2024 16:07
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.

5 participants