Don't set Notification#no_more_notifications on custom notifications by Al2Klimov · Pull Request #7818 · Icinga/icinga2

Al2Klimov · 2020-02-06T15:46:05Z

... as it has nothing to do with reminder notifications.

Edit

Imagine you get a problem notification. The above if branch does SetNoMoreNotifications(true), so that (the only) GetNoMoreNotifications() in NotificationComponent prevent reminders if none configured.

Now, if you send a custom notification, w/o my change the else branch does SetNoMoreNotifications(false), so that the NotificationComponent sends a reminder next time despite none configured.

Al2Klimov · 2021-08-04T12:23:04Z

@cla-bot check

Al2Klimov · 2023-12-04T14:35:33Z

What unit test cases would you like to prove this didn't break anything?

yhabteab

Firstly the referenced issues are very different ones. The OP of #7758 is reporting that the no_more_notifications is still set even after the Host has recovered while in Downtime and the Downtime has ended (either cancelled manually or ended automatically). This can even be found in the debug logs he provided.

Host goes down:

[2020-01-22 16:45:10 +0100] notice/ClusterEvents: Discarding 'send custom notification' message for checkable 'test-host-notification-bug-3' from 'xyz-mon-sat1.xyz.de': Unauthorized access.
[2020-01-22 16:45:10 +0100] information/Checkable: Checkable 'test-host-notification-bug-3' has 1 notification(s). Checking filters for type 'Problem', sends will be logged.
[2020-01-22 16:45:10 +0100] notice/Notification: Attempting to send notifications of type 'Problem' for notification object 'test-host-notification-bug-3!test-host-notification-bug-delay'.
[2020-01-22 16:45:10 +0100] notice/Notification: Not sending notifications for notification object 'test-host-notification-bug-3!test-host-notification-bug-delay': before specified begin time (10 seconds)

A moment later the notification is send and sets no_more_notifications to true.

[2020-01-22 16:45:25 +0100] notice/NotificationComponent: Attempting to send reminder notification 'test-host-notification-bug-3!test-host-notification-bug-delay'.
[2020-01-22 16:45:25 +0100] notice/Notification: Attempting to send reminder notifications of type 'Problem' for notification object 'test-host-notification-bug-3!test-host-notification-bug-delay'.
[2020-01-22 16:45:25 +0100] debug/Notification: User 'test-user-notification-bug' notification 'test-host-notification-bug-3!test-host-notification-bug-delay', Type 'Problem', TypeFilter: Problem (FType=32, TypeFilter=104)
[2020-01-22 16:45:25 +0100] debug/Notification: User 'test-user-notification-bug' notification 'test-host-notification-bug-3!test-host-notification-bug-delay', State 'Down', StateFilter: Critical and Down (FState=32, StateFilter=36)
[2020-01-22 16:45:25 +0100] information/Notification: Sending reminder 'Problem' notification 'test-host-notification-bug-3!test-host-notification-bug-delay' for user 'test-user-notification-bug'

During the downtime, the host recovers and the Recovery notification is suppressed. Afterwards, the downtime is ended/canceled and no notification is set due to filter types and ends up here and leaves no_more_notifications unchanged.

icinga2/lib/icinga/notification.cpp

Lines 329 to 346 in 2be08aa

    
           		if (!(ftype & GetTypeFilter())) { 
        
           			Log(LogNotice, "Notification") 
        
           				<< "Not sending " << (reminder ? "reminder " : "") << "notifications for notification object '" 
        
           				<< notificationName << "': type '" 
        
           				<< NotificationTypeToString(type) << "' does not match type filter: " 
        
           				<< NotificationFilterToString(GetTypeFilter(), GetTypeFilterMap()) << "."; 
        
           			/* Ensure to reset no_more_notifications on Recovery notifications, 
        
           			 * even if the admin did not configure them in the filter. 
        
           			 */ 
        
           			{ 
        
           				ObjectLock olock(this); 
        
           				if (type == NotificationRecovery && GetInterval() <= 0) 
        
           					SetNoMoreNotifications(false); 
        
           			} 
        
           			return; 
        
           		}

2020-01-22 16:53:05 +0100] notice/Notification: Attempting to send notifications of type 'DowntimeEnd' for notification object 'test-host-notification-bug-3!test-host-notification-bug-delay'.
[2020-01-22 16:53:05 +0100] notice/Notification: Not sending notifications for notification object 'test-host-notification-bug-3!test-host-notification-bug-delay': type 'DowntimeEnd' does not match type filter: Custom, Problem and Recovery.

So, how is your PR supposed to solve the problem? That would make it even worse and wouldn't reset it if the filter types would contain DowntimeEnd.

The OP of #9046, on the other hand, reports that sending custom notifications implicitly resets the "no_more_notifications" flag and causes problem notifications to be sent again while interval is set to 0.

Al2Klimov · 2024-03-14T16:13:33Z

#9046 test

This PR

sends the custom notification and that's it.

master

sends another problem one afterwards.

yhabteab

~~fixes~~ doesn't fix #7758

Please remove this!

Al2Klimov · 2024-04-04T08:27:50Z

Because it says nothing (could be every other not fixed issue number) or because it fixes the mentioned issue?

yhabteab · 2024-04-04T08:30:28Z

Because it says nothing (could be every other not fixed issue number)

Exactly! It has nothing to do with PR at all!

Al2Klimov requested a review from dnsmichi February 6, 2020 15:53

Al2Klimov marked this pull request as ready for review February 6, 2020 15:53

Al2Klimov added this to the 2.13.0 milestone Mar 17, 2020

Al2Klimov mentioned this pull request Mar 17, 2020

Reminder notifications get send out even with interval=0 when forcing CUSTOM notification via webinterface #6333

Closed

N-o-X requested review from N-o-X and removed request for dnsmichi September 3, 2020 06:33

Al2Klimov modified the milestones: 2.13.0, 2.14.0 Jun 2, 2021

cla-bot bot added the cla/signed label Aug 4, 2021

Al2Klimov added the bug Something isn't working label Aug 10, 2021

Al2Klimov mentioned this pull request Feb 15, 2022

Custom notifications on downed hosts also send another down notification #9046

Closed

Al2Klimov mentioned this pull request Mar 22, 2022

Duplicate notifications while replay log is processed #9265

Open

julianbrost removed the request for review from N-o-X January 12, 2023 12:42

julianbrost removed this from the 2.14.0 milestone Jan 23, 2023

Al2Klimov added the area/notifications Notification events label May 17, 2023

Al2Klimov requested a review from julianbrost December 4, 2023 14:35

yhabteab reviewed Feb 14, 2024

View reviewed changes

Al2Klimov force-pushed the bugfix/no_more_notifications-7758 branch from 9cc81a1 to 3cfa9bd Compare March 14, 2024 15:32

Al2Klimov requested a review from yhabteab March 14, 2024 16:13

yhabteab approved these changes Apr 4, 2024

View reviewed changes

Al2Klimov added this to the 2.15.0 milestone Apr 4, 2024

Al2Klimov changed the title ~~Don't set Notification#no_more_notifications on others than problem/recovery~~ Don't set Notification#no_more_notifications on custom notifications Apr 4, 2024

Don't set Notification#no_more_notifications on custom notifications

35a7057

Al2Klimov force-pushed the bugfix/no_more_notifications-7758 branch from 3cfa9bd to 35a7057 Compare November 15, 2024 12:03

yhabteab removed the request for review from julianbrost November 15, 2024 12:07

yhabteab enabled auto-merge November 15, 2024 12:07

yhabteab merged commit 2931aea into master Nov 15, 2024

yhabteab deleted the bugfix/no_more_notifications-7758 branch November 15, 2024 13:43

Al2Klimov added the consider backporting Should be considered for inclusion in a bugfix release label Feb 4, 2025

Al2Klimov mentioned this pull request Feb 4, 2025

Release v2.14.5 #10341

Merged

yhabteab mentioned this pull request Mar 6, 2025

Notification: Reset internal states on (missed)recovery #10361

Merged

yhabteab removed the consider backporting Should be considered for inclusion in a bugfix release label Jun 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't set Notification#no_more_notifications on custom notifications#7818

Don't set Notification#no_more_notifications on custom notifications#7818
yhabteab merged 1 commit intomasterfrom
bugfix/no_more_notifications-7758

Al2Klimov commented Feb 6, 2020 •

edited by yhabteab

Loading

Uh oh!

Al2Klimov commented Aug 4, 2021

Uh oh!

Al2Klimov commented Dec 4, 2023

Uh oh!

yhabteab left a comment

Uh oh!

Al2Klimov commented Mar 14, 2024

Uh oh!

yhabteab left a comment

Uh oh!

Al2Klimov commented Apr 4, 2024

Uh oh!

yhabteab commented Apr 4, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (!(ftype & GetTypeFilter())) {
	Log(LogNotice, "Notification")
	<< "Not sending " << (reminder ? "reminder " : "") << "notifications for notification object '"
	<< notificationName << "': type '"
	<< NotificationTypeToString(type) << "' does not match type filter: "
	<< NotificationFilterToString(GetTypeFilter(), GetTypeFilterMap()) << ".";

	/* Ensure to reset no_more_notifications on Recovery notifications,
	* even if the admin did not configure them in the filter.
	*/
	{
	ObjectLock olock(this);
	if (type == NotificationRecovery && GetInterval() <= 0)
	SetNoMoreNotifications(false);
	}

	return;
	}

Conversation

Al2Klimov commented Feb 6, 2020 • edited by yhabteab Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Edit

Uh oh!

Al2Klimov commented Aug 4, 2021

Uh oh!

Al2Klimov commented Dec 4, 2023

Uh oh!

yhabteab left a comment

Choose a reason for hiding this comment

Uh oh!

Al2Klimov commented Mar 14, 2024

#9046 test

This PR

master

Uh oh!

yhabteab left a comment

Choose a reason for hiding this comment

Uh oh!

Al2Klimov commented Apr 4, 2024

Uh oh!

yhabteab commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Al2Klimov commented Feb 6, 2020 •

edited by yhabteab

Loading

yhabteab commented Apr 4, 2024 •

edited

Loading