Skip to content

fix: differentiate alarm task types - #1408

Draft
beck-8 wants to merge 1 commit into
mainfrom
fix/pdp-taskfailure-scope
Draft

fix: differentiate alarm task types#1408
beck-8 wants to merge 1 commit into
mainfrom
fix/pdp-taskfailure-scope

Conversation

@beck-8

@beck-8 beck-8 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

It turns out that there is no distinction between types for the same alarm. It will occur that after the problem has been solved, the alarm will continue to be issued. This is because TaskFailures looks back at errors within 30 minutes.

Now differentiate them.
image

@beck-8
beck-8 requested review from a team and ZenGround0 as code owners August 7, 2026 14:29

@LexLuthr LexLuthr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It doesn't seem to be solving the issue you are describing in the summary. Another problem is that the current behaviour is expected. Any failures should be looked at by SPs. Why did proving failed. In your case, you already knew but what if proving failed and SP finds out because of the alert?

Comment thread alertmanager/alerts.go
func taskFailureCheck(al *alerts) {
taskFailureCheckWith(al, Name_TaskFailures, FullAlertInterval, sealingTasks)
taskFailureCheckWith(al, Name_TaskFailures, FullAlertInterval, sealingTasks, func(taskName string) bool {
return !slices.Contains(pdpTasks, taskName)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This will basically ensure that any important task not part of the array will be ignored. That's not a good choice.

Comment thread alertmanager/alerts.go
func pdpTaskFailureCheck(al *alerts) {
taskFailureCheckWith(al, Name_PDPTaskFailures, AlertManagerInterval, pdpTasks)
taskFailureCheckWith(al, Name_PDPTaskFailures, AlertManagerInterval, pdpTasks, func(taskName string) bool {
return slices.Contains(pdpTasks, taskName)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This will basically ensure that any important task not part of the array will be ignored. That's not a good choice.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There may be an inaccuracy here. But the tasks in the list all take a 5-minute route, and the rest that are not here still follow the previous 30-minute route?

@beck-8

beck-8 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Here is a duplicate alert, and after the problem has been resolved. He also continues to alert in TaskFailures.
The normal expectation is that the first three messages are only PDPTaskFailures, and the next three messages should not occur because they have been resolved at this time.
He continued to alert for 20 minutes(This part is redundant)

@ZenGround0
ZenGround0 requested a review from LexLuthr August 10, 2026 19:11
@LexLuthr

Copy link
Copy Markdown
Collaborator

Here is a duplicate alert, and after the problem has been resolved. He also continues to alert in TaskFailures. The normal expectation is that the first three messages are only PDPTaskFailures, and the next three messages should not occur because they have been resolved at this time. He continued to alert for 20 minutes(This part is redundant)

I think this change is based on an incorrect interpretation of the alert.

TaskFailures reports finalized task failures that occurred within its lookback window. It does not claim that the underlying problem is still present. If 101 PDPv0_ProvPeriod tasks failed, continuing to report those failures for 30 minutes is expected, even if later tasks succeed. A later recovery does not make those failures irrelevant.

This PR makes the alert disappear sooner by excluding selected PDP tasks from the general 30-minute failure check. That suppresses valid failure information; it does not fix either the failures or the alert logic.

It also incorrectly treats pdpTasks, which is a sensitivity list, as the authoritative classification of all PDP tasks. The list is incomplete, so some PDP failures will remain visible for 30 minutes while others disappear after 5 minutes, with no clear reason for the difference.

If duplicate presentation between TaskFailures and PDPTaskFailures is the problem, then we should fix that explicitly while preserving the intended visibility of finalized failures. We should not redefine a recent-failure alert as an indication of current health and then remove valid failures to make it appear resolved.

@beck-8

beck-8 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Ah, my expectation is that when the error stops, the alarm should stop (the alarm has been going on for a period of time when the problem occurs). Otherwise, after solving the problem, the user will be confused when he finds out why there are still alarms.

This is the original reason why I paid attention to this issue in the first place

@beck-8
beck-8 marked this pull request as draft August 11, 2026 13:21
@LexLuthr

Copy link
Copy Markdown
Collaborator

Your double alert observation is on point. The solution you implemented needs some fine tuning.

@beck-8

beck-8 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

I don't have a good idea about repetition for the time being. Maybe I just add some words to make it clearer.

Task: PDPv0_ProvPeriod, Failures: 101 (last 30m). Machine: 127.0.0.1:skiff, Failures: 103 (last 30m).
Task: PDPv0_ProvPeriod, Failures: 20 (last 5m). Machine: 127.0.0.1:skiff, Failures: 20 (last 5m).

I'll take a look at it tomorrow. Maybe this PR is just to differentiate between duplicate alarms and make them clearer.

For issues that have been resolved but still trigger alerts, I keep my opinion (preferably, if the alarm stops, then stop). <-- I don’t know how to implement this elegantly.

@LexLuthr

Copy link
Copy Markdown
Collaborator

@beck-8 There is no hurry. Take your time. Think about an approach which keeps as much of current semantics and general intent and then do a small surgical change which removes the duplication. What that entails, I leave up to you.

If you need help, you can ping me on Slack and we can discuss this there in detail.

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.

2 participants