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

Fix lost recovery notifications after recovery outside of notification time period #10241

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

Al2Klimov
Copy link
Member

This fixes an issue where recovery notifications get lost if they happen outside of a notification time period.

Not all calls to Checkable::NotificationReasonApplies() need GetStateBeforeSuppression() to be checked. In fact, for one caller, FireSuppressedNotifications() in
lib/notification/notificationcomponent.cpp, the state before suppression may not even be initialized properly, so that the default value of OK is used which can lead to incorrect return values. Note the difference between suppressions happening on the level of the Checkable object level and the Notification object level. Only the first sets the state before suppression in the Checkable object, but so far, also the latter used that value incorrectly.

This commit moves the check of GetStateBeforeSuppression() from Checkable::NotificationReasonApplies() to the one place where it's actually relevant: Checkable::FireSuppressedNotifications(). This made the existing call to NotificationReasonApplies() unneccessary as it would always return true: the type argument is computed based on the current check result, so there's no need to check it against the current check result.

This fixes an issue where recovery notifications get lost if they happen
outside of a notification time period.

Not all calls to `Checkable::NotificationReasonApplies()` need
`GetStateBeforeSuppression()` to be checked. In fact, for one caller,
`FireSuppressedNotifications()` in
`lib/notification/notificationcomponent.cpp`, the state before suppression may
not even be initialized properly, so that the default value of OK is used which
can lead to incorrect return values. Note the difference between suppressions
happening on the level of the `Checkable` object level and the `Notification`
object level. Only the first sets the state before suppression in the
`Checkable` object, but so far, also the latter used that value incorrectly.

This commit moves the check of `GetStateBeforeSuppression()` from
`Checkable::NotificationReasonApplies()` to the one place where it's actually
relevant: `Checkable::FireSuppressedNotifications()`. This made the existing
call to `NotificationReasonApplies()` unneccessary as it would always return
true: the `type` argument is computed based on the current check result, so
there's no need to check it against the current check result.
@cla-bot cla-bot bot added the cla/signed label Nov 14, 2024
@icinga-probot icinga-probot bot added area/notifications Notification events bug Something isn't working ref/IP labels Nov 14, 2024
@icinga-probot icinga-probot bot added this to the 2.13.11 milestone Nov 14, 2024
@yhabteab yhabteab changed the title Use Checkable::GetStateBeforeSuppression() only where relevant Fix lost recovery notifications after recovery outside of notification time period Nov 14, 2024
@yhabteab yhabteab merged commit 9c9af8e into support/2.13 Nov 14, 2024
27 checks passed
@yhabteab yhabteab deleted the state-before-suppression213 branch November 14, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/notifications Notification events bug Something isn't working cla/signed ref/IP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants