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

Fixes a number of bugs in silences #8525

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

grobinson-grafana
Copy link
Contributor

@grobinson-grafana grobinson-grafana commented Jun 26, 2024

What this PR does

This commit fixes the following bugs in silences:

which could cause an existing silence to be deleted/expired when updating the silence failed. This can happen when the replacing silence exceeded limits or was invalid.

Which issue(s) this PR fixes or relates to

Here are some screenshots showing the broken behavior, where an existing silence is expired because the limit was reached:

Screenshot 2024-06-26 at 17 05 02 Screenshot 2024-06-26 at 17 06 07 Screenshot 2024-06-26 at 17 06 18

And here is the same update performed with the fix:

Screenshot 2024-06-26 at 17 11 10 Screenshot 2024-06-26 at 17 11 18 Screenshot 2024-06-26 at 17 11 25

You can see the original silence is kept. This has also been tested for invalid silences and silences which are too large (exceed maximum size limit):

Screenshot 2024-06-26 at 17 24 12 Screenshot 2024-06-26 at 17 24 16

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@@ -63,7 +63,7 @@ require (
github.com/google/go-github/v57 v57.0.0
github.com/google/uuid v1.6.0
github.com/grafana-tools/sdk v0.0.0-20220919052116-6562121319fc
github.com/grafana/alerting v0.0.0-20240607182251-835aff588914
github.com/grafana/alerting v0.0.0-20240626080128-8299cb22b8df
Copy link
Contributor Author

@grobinson-grafana grobinson-grafana Jun 26, 2024

Choose a reason for hiding this comment

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

This comes from here. It is a branch that has two commits on top of 835aff5 to avoid bringing in the changes from #8515 while it is still in review.

The changes in this branch are cherry-picked from main, and were reviewed by @yuri-tceretian:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@santihernandezc asked me why do this instead of using main from grafana/alerting. My reasoning for this was so I don't have to bring in changes from #8515 for the bug fix as I don't know if and when that PR will be approved. If you consider this to be unnecessary I will happily update the PR to use main.

This commit fixes the following bugs in silences:

- prometheus/alertmanager#3877
- prometheus/alertmanager#3898
- prometheus/alertmanager#3897

which could cause an existing silence to be deleted/expired
when updating the silence failed. This could be because
the replacing silence exceeded limits or was invalid.

additional tests in upstream.
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 🎉

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.

Approving since this touches integration/ which requires a maintainer approval. I am relying on alerting team members to review the rest of the changes.

@grobinson-grafana grobinson-grafana merged commit 1cfb657 into main Jun 26, 2024
29 checks passed
@grobinson-grafana grobinson-grafana deleted the grobinson/fix-silences branch June 26, 2024 19:06
@dimitarvdimitrov dimitarvdimitrov mentioned this pull request Jul 2, 2024
66 tasks
@grafanabot
Copy link
Contributor

The backport to release-2.13 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-8525-to-release-2.13 origin/release-2.13
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 1cfb65777a4dbf21c85b1467e67b925e32e3042e
# Push it to GitHub
git push --set-upstream origin backport-8525-to-release-2.13
git switch main
# Remove the local backport branch
git branch -D backport-8525-to-release-2.13

Then, create a pull request where the base branch is release-2.13 and the compare/head branch is backport-8525-to-release-2.13.

dimitarvdimitrov pushed a commit that referenced this pull request Jul 2, 2024
This commit fixes the following bugs in silences:

- prometheus/alertmanager#3877
- prometheus/alertmanager#3898
- prometheus/alertmanager#3897

which could cause an existing silence to be deleted/expired
when updating the silence failed. This could be because
the replacing silence exceeded limits or was invalid.

(cherry picked from commit 1cfb657)
dimitarvdimitrov added a commit that referenced this pull request Jul 2, 2024
* Fixes a number of bugs in silences (#8525)

This commit fixes the following bugs in silences:

- prometheus/alertmanager#3877
- prometheus/alertmanager#3898
- prometheus/alertmanager#3897

which could cause an existing silence to be deleted/expired
when updating the silence failed. This could be because
the replacing silence exceeded limits or was invalid.

(cherry picked from commit 1cfb657)

* Update CHANGELOG.md (#8526)

(cherry picked from commit 36f7af3)

---------

Co-authored-by: George Robinson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants