Only notify users on recovery who have been notified on not-OK before

Also ensure that type NotificationRecovery always
passes the state filter (missing `OK` is totally fine).

Also fix that notification delays set the correct
next notification time to the begin time window.

fixes #7579
fixes #7623
fixes #6547

doc/4-monitoring-basics.md

@@ -610,8 +610,6 @@ to the defined notifications. That way you'll save duplicated attributes in each
 
 The time period `24x7` is shipped as example configuration with Icinga 2.
 
-
-
 Use the `apply` keyword to create `Notification` objects for your services:
 
     apply Notification "notify-cust-xy-mysql" to Service {
@@ -628,6 +626,11 @@ Instead of assigning users to notifications, you can also add the `user_groups`
 attribute with a list of user groups to the `Notification` object. Icinga 2 will
 send notifications to all group members.
 
+> **Note**
+>
+> Only users who have been notified of a problem before  (`Warning`, `Critical`, `Unknown`
+> states for services, `Down` for hosts) will receive `Recovery` notifications.
+
 ### <a id="notification-escalations"></a> Notification Escalations
 
 When a problem notification is sent and a problem still exists at the time of re-notification

lib/icinga/notification.cpp

@@ -245,6 +245,14 @@ void Notification::BeginExecuteNotification(NotificationType type, const CheckRe
 			if (times && times->Contains("begin") && now < checkable->GetLastHardStateChange() + times->Get("begin")) {
 				Log(LogNotice, "Notification")
 				    << "Not sending notifications for notification object '" << GetName() << "': before escalation range";
+
+				/* we need to adjust the next notification time
+ 				 * to now + begin delaying the first notification
+ 				 */
+				double nextProposedNotification = now + times->Get("begin") + 1.0;
+				if (GetNextNotification() > nextProposedNotification)
+					SetNextNotification(nextProposedNotification);
+
 				return;
 			}
 
@@ -262,25 +270,29 @@ void Notification::BeginExecuteNotification(NotificationType type, const CheckRe
 
 		if (!(ftype & GetTypeFilter())) {
 			Log(LogNotice, "Notification")
-			    << "Not sending notifications for notification object '" << GetName() << "': type filter does not match";
+			    << "Not sending notifications for notification object '" << GetName() << "': type filter does not match '"
+			    << NotificationTypeToString(type) << "'";
 			return;
 		}
 
-		Host::Ptr host;
-		Service::Ptr service;
-		tie(host, service) = GetHostService(checkable);
+		/* ensure that recovery notifications are always sent, no state filter checks necessary */
+		if (type != NotificationRecovery) {
+			Host::Ptr host;
+			Service::Ptr service;
+			tie(host, service) = GetHostService(checkable);
 
-		unsigned long fstate;
+			unsigned long fstate;
 
-		if (service)
-			fstate = ServiceStateToFilter(service->GetState());
-		else
-			fstate = HostStateToFilter(host->GetState());
+			if (service)
+				fstate = ServiceStateToFilter(service->GetState());
+			else
+				fstate = HostStateToFilter(host->GetState());
 
-		if (!(fstate & GetStateFilter())) {
-			Log(LogNotice, "Notification")
-			    << "Not sending notifications for notification object '" << GetName() << "': state filter does not match";
-			return;
+			if (!(fstate & GetStateFilter())) {
+				Log(LogNotice, "Notification")
+				    << "Not sending notifications for notification object '" << GetName() << "': state filter does not match";
+				return;
+			}
 		}
 	}
 
@@ -307,19 +319,47 @@ void Notification::BeginExecuteNotification(NotificationType type, const CheckRe
 	Service::OnNotificationSendStart(this, checkable, allUsers, type, cr, author, text);
 
 	std::set<User::Ptr> allNotifiedUsers;
+
 	BOOST_FOREACH(const User::Ptr& user, allUsers) {
-		if (!user->GetEnableNotifications() || !CheckNotificationUserFilters(type, user, force))
+		String userName = user->GetName();
+
+		if (!user->GetEnableNotifications()) {
+			Log(LogNotice, "Notification")
+			    << "Disabled notifications for user '" << userName << "'. Not sending notification.";
 			continue;
+		}
+
+		if (!CheckNotificationUserFilters(type, user, force)) {
+			Log(LogNotice, "Notification")
+			    << "Notification filters for user '" << userName << "' not matched. Not sending notification.";
+			continue;
+		}
+
+		/* on recovery, check if user was notified before */
+		if (type == NotificationRecovery) {
+			if (m_NotifiedUsers.find(userName) == m_NotifiedUsers.end()) {
+				Log(LogNotice, "Notification")
+				    << "We did not notify user '" << userName << "' before. Not sending recovery notification.";
+				continue;
+			}
+		}
 
 		Log(LogInformation, "Notification")
-		    << "Sending notification for user '" << user->GetName() << "'";
+		    << "Sending notification for user '" << userName << "'";
 
 		Utility::QueueAsyncCallback(boost::bind(&Notification::ExecuteNotificationHelper, this, type, user, cr, force, author, text));
 
 		/* collect all notified users */
 		allNotifiedUsers.insert(user);
+
+		/* store all notified users for later recovery checks */
+		m_NotifiedUsers.insert(userName);
 	}
 
+	/* if this was a recovery notification, reset all notified users */
+	if (type == NotificationRecovery)
+		ResetNotifiedUsers();
+
 	/* used in db_ido for notification history */
 	Service::OnNotificationSentToAllUsers(this, checkable, allNotifiedUsers, type, cr, author, text);
 }
@@ -347,23 +387,26 @@ bool Notification::CheckNotificationUserFilters(NotificationType type, const Use
 			return false;
 		}
 
-		Checkable::Ptr checkable = GetCheckable();
-		Host::Ptr host;
-		Service::Ptr service;
-		tie(host, service) = GetHostService(checkable);
+		/* check state filters it this is not a recovery notification */
+		if (type != NotificationRecovery) {
+			Checkable::Ptr checkable = GetCheckable();
+			Host::Ptr host;
+			Service::Ptr service;
+			tie(host, service) = GetHostService(checkable);
 
-		unsigned long fstate;
+			unsigned long fstate;
 
-		if (service)
-				fstate = ServiceStateToFilter(service->GetState());
-		else
-				fstate = HostStateToFilter(host->GetState());
+			if (service)
+					fstate = ServiceStateToFilter(service->GetState());
+			else
+					fstate = HostStateToFilter(host->GetState());
 
-		if (!(fstate & user->GetStateFilter())) {
-			Log(LogNotice, "Notification")
-			    << "Not sending notifications for notification object '"
-			    << GetName() << " and user '" << user->GetName() << "': state filter does not match";
-			return false;
+			if (!(fstate & user->GetStateFilter())) {
+				Log(LogNotice, "Notification")
+				    << "Not sending notifications for notification object '"
+				    << GetName() << " and user '" << user->GetName() << "': state filter does not match";
+				return false;
+			}
 		}
 	}
 
@@ -403,6 +446,11 @@ void Notification::ExecuteNotificationHelper(NotificationType type, const User::
 	}
 }
 
+void Notification::ResetNotifiedUsers(void)
+{
+	m_NotifiedUsers.clear();
+}
+
 int icinga::ServiceStateToFilter(ServiceState state)
 {
 	switch (state) {
@@ -471,4 +519,3 @@ void Notification::ValidateFilters(const String& location, const Dictionary::Ptr
 		    location + ": Type filter is invalid.");
 	}
 }
-

lib/icinga/notification.hpp

@@ -97,6 +97,8 @@ public:
 
 	bool CheckNotificationUserFilters(NotificationType type, const User::Ptr& user, bool force);
 
+	void ResetNotifiedUsers(void);
+
 	static String NotificationTypeToString(NotificationType type);
 
 	static boost::signals2::signal<void (const Notification::Ptr&, double, const MessageOrigin&)> OnNextNotificationChanged;
@@ -111,6 +113,8 @@ protected:
 	virtual void Stop(void);
 
 private:
+	std::set<String> m_NotifiedUsers;
+
 	void ExecuteNotificationHelper(NotificationType type, const User::Ptr& user, const CheckResult::Ptr& cr, bool force, const String& author = "", const String& text = "");
 
 	static void EvaluateApplyRuleOneInstance(const intrusive_ptr<Checkable>& checkable, const String& name, const Dictionary::Ptr& locals, const ApplyRule& rule);