Checkable: Add test for state notifications after a suppression ends

lib/icinga/checkable-notification.cpp

@@ -129,26 +129,26 @@ void Checkable::UnregisterNotification(const Notification::Ptr& notification)
 	m_Notifications.erase(notification);
 }
 
-static void FireSuppressedNotifications(Checkable* checkable)
+void Checkable::FireSuppressedNotifications()
 {
-	if (!checkable->IsActive())
+	if (!IsActive())
 		return;
 
-	if (checkable->IsPaused())
+	if (IsPaused())
 		return;
 
-	if (!checkable->GetEnableNotifications())
+	if (!GetEnableNotifications())
 		return;
 
-	int suppressed_types (checkable->GetSuppressedNotifications());
+	int suppressed_types (GetSuppressedNotifications());
 	if (!suppressed_types)
 		return;
 
 	int subtract = 0;
 
 	{
-		LazyInit<bool> wasLastParentRecoveryRecent ([&checkable]() {
+		LazyInit<bool> wasLastParentRecoveryRecent ([this]() {
-			auto cr (checkable->GetLastCheckResult());
+			auto cr (GetLastCheckResult());
 
 			if (!cr) {
 				return true;
@@ -156,7 +156,7 @@ static void FireSuppressedNotifications(Checkable* checkable)
 
 			auto threshold (cr->GetExecutionStart());
 
-			for (auto& dep : checkable->GetDependencies()) {
+			for (auto& dep : GetDependencies()) {
 				auto parent (dep->GetParent());
 				ObjectLock oLock (parent);
 
@@ -169,9 +169,9 @@ static void FireSuppressedNotifications(Checkable* checkable)
 		});
 
 		if (suppressed_types & (NotificationProblem|NotificationRecovery)) {
-			CheckResult::Ptr cr = checkable->GetLastCheckResult();
+			CheckResult::Ptr cr = GetLastCheckResult();
-			NotificationType type = cr && checkable->IsStateOK(cr->GetState()) ? NotificationRecovery : NotificationProblem;
+			NotificationType type = cr && IsStateOK(cr->GetState()) ? NotificationRecovery : NotificationProblem;
-			bool state_suppressed = checkable->NotificationReasonSuppressed(NotificationProblem) || checkable->NotificationReasonSuppressed(NotificationRecovery);
+			bool state_suppressed = NotificationReasonSuppressed(NotificationProblem) || NotificationReasonSuppressed(NotificationRecovery);
 
 			/* Only process (i.e. send or dismiss) suppressed state notifications if the following conditions are met:
 			 *
@@ -191,9 +191,9 @@ static void FireSuppressedNotifications(Checkable* checkable)
 			 *
 			 * If any of these conditions is not met, processing the suppressed notification is further delayed.
 			 */
-			if (!state_suppressed && checkable->GetStateType() == StateTypeHard && !checkable->IsLikelyToBeCheckedSoon() && !wasLastParentRecoveryRecent.Get()) {
+			if (!state_suppressed && GetStateType() == StateTypeHard && !IsLikelyToBeCheckedSoon() && !wasLastParentRecoveryRecent.Get()) {
-				if (checkable->NotificationReasonApplies(type)) {
+				if (NotificationReasonApplies(type)) {
-					Checkable::OnNotificationsRequested(checkable, type, cr, "", "", nullptr);
+					Checkable::OnNotificationsRequested(this, type, cr, "", "", nullptr);
 				}
 				subtract |= NotificationRecovery|NotificationProblem;
 			}
@@ -201,11 +201,11 @@ static void FireSuppressedNotifications(Checkable* checkable)
 
 		for (auto type : {NotificationFlappingStart, NotificationFlappingEnd}) {
 			if (suppressed_types & type) {
-				bool still_applies = checkable->NotificationReasonApplies(type);
+				bool still_applies = NotificationReasonApplies(type);
 
 				if (still_applies) {
-					if (!checkable->NotificationReasonSuppressed(type) && !checkable->IsLikelyToBeCheckedSoon() && !wasLastParentRecoveryRecent.Get()) {
+					if (!NotificationReasonSuppressed(type) && !IsLikelyToBeCheckedSoon() && !wasLastParentRecoveryRecent.Get()) {
-						Checkable::OnNotificationsRequested(checkable, type, checkable->GetLastCheckResult(), "", "", nullptr);
+						Checkable::OnNotificationsRequested(this, type, GetLastCheckResult(), "", "", nullptr);
 
 						subtract |= type;
 					}
@@ -217,13 +217,13 @@ static void FireSuppressedNotifications(Checkable* checkable)
 	}
 
 	if (subtract) {
-		ObjectLock olock (checkable);
+		ObjectLock olock (this);
 
-		int suppressed_types_before (checkable->GetSuppressedNotifications());
+		int suppressed_types_before (GetSuppressedNotifications());
 		int suppressed_types_after (suppressed_types_before & ~subtract);
 
 		if (suppressed_types_after != suppressed_types_before) {
-			checkable->SetSuppressedNotifications(suppressed_types_after);
+			SetSuppressedNotifications(suppressed_types_after);
 		}
 	}
 }
@@ -231,14 +231,14 @@ static void FireSuppressedNotifications(Checkable* checkable)
 /**
  * Re-sends all notifications previously suppressed by e.g. downtimes if the notification reason still applies.
  */
-void Checkable::FireSuppressedNotifications(const Timer * const&)
+void Checkable::FireSuppressedNotificationsTimer(const Timer * const&)
 {
 	for (auto& host : ConfigType::GetObjectsByType<Host>()) {
-		::FireSuppressedNotifications(host.get());
+		host->FireSuppressedNotifications();
 	}
 
 	for (auto& service : ConfigType::GetObjectsByType<Service>()) {
-		::FireSuppressedNotifications(service.get());
+		service->FireSuppressedNotifications();
 	}
 }
 

lib/icinga/checkable.cpp

@@ -103,7 +103,7 @@ void Checkable::Start(bool runtimeCreated)
 	boost::call_once(once, []() {
 		l_CheckablesFireSuppressedNotifications = new Timer();
 		l_CheckablesFireSuppressedNotifications->SetInterval(5);
-		l_CheckablesFireSuppressedNotifications->OnTimerExpired.connect(&Checkable::FireSuppressedNotifications);
+		l_CheckablesFireSuppressedNotifications->OnTimerExpired.connect(&Checkable::FireSuppressedNotificationsTimer);
 		l_CheckablesFireSuppressedNotifications->Start();
 
 		l_CleanDeadlinedExecutions = new Timer();

lib/icinga/checkable.hpp

@@ -191,6 +191,8 @@ public:
 	bool NotificationReasonSuppressed(NotificationType type);
 	bool IsLikelyToBeCheckedSoon();
 
+	void FireSuppressedNotifications();
+
 	static void IncreasePendingChecks();
 	static void DecreasePendingChecks();
 	static int GetPendingChecks();
@@ -222,7 +224,7 @@ private:
 
 	static void NotifyDowntimeEnd(const Downtime::Ptr& downtime);
 
-	static void FireSuppressedNotifications(const Timer * const&);
+	static void FireSuppressedNotificationsTimer(const Timer * const&);
 	static void CleanDeadlinedExecutions(const Timer * const&);
 
 	/* Comments */

lib/icinga/downtime.hpp

@@ -62,12 +62,13 @@ public:
 	void TriggerDowntime(double triggerTime);
 	void SetRemovalInfo(const String& removedBy, double removeTime, const MessageOrigin::Ptr& origin = nullptr);
 
+	void OnAllConfigLoaded() override;
+
 	static String GetDowntimeIDFromLegacyID(int id);
 
 	static DowntimeChildOptions ChildOptionsFromValue(const Value& options);
 
 protected:
-	void OnAllConfigLoaded() override;
 	void Start(bool runtimeCreated) override;
 	void Stop(bool runtimeRemoved) override;
 

lib/icinga/downtime.ti

@@ -25,12 +25,12 @@ class Downtime : ConfigObject < DowntimeNameComposer
 	load_after Host;
 	load_after Service;
 
-	[config, protected, required, navigation(host)] name(Host) host_name {
+	[config, required, navigation(host)] name(Host) host_name {
 		navigate {{{
 			return Host::GetByName(GetHostName());
 		}}}
 	};
-	[config, protected, navigation(service)] String service_name {
+	[config, navigation(service)] String service_name {
 		track {{{
 			if (!oldValue.IsEmpty()) {
 				Service::Ptr service = Service::GetByNamePair(GetHostName(), oldValue);

lib/icinga/host.hpp

@@ -50,10 +50,11 @@ public:
 
 	bool ResolveMacro(const String& macro, const CheckResult::Ptr& cr, Value *result) const override;
 
+	void OnAllConfigLoaded() override;
+
 protected:
 	void Stop(bool runtimeRemoved) override;
 
-	void OnAllConfigLoaded() override;
 	void CreateChildObjects(const Type::Ptr& childType) override;
 
 private:

lib/icinga/service.hpp

@@ -44,10 +44,11 @@ public:
 
 	static void EvaluateApplyRules(const Host::Ptr& host);
 
+	void OnAllConfigLoaded() override;
+
 	static boost::signals2::signal<void (const Service::Ptr&, const CheckResult::Ptr&, const MessageOrigin::Ptr&)> OnHostProblemChanged;
 
 protected:
-	void OnAllConfigLoaded() override;
 	void CreateChildObjects(const Type::Ptr& childType) override;
 
 private:

test/CMakeLists.txt

@@ -133,6 +133,7 @@ add_boost_test(base
     icinga_checkresult/service_3attempts
     icinga_checkresult/host_flapping_notification
     icinga_checkresult/service_flapping_notification
+    icinga_checkresult/suppressed_notification
     icinga_dependencies/multi_parent
     icinga_notification/strings
     icinga_notification/state_filter

test/icinga-checkresult.cpp

@@ -1,8 +1,13 @@
 /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */
 
+#include "icinga/downtime.hpp"
 #include "icinga/host.hpp"
+#include "icinga/service.hpp"
 #include <BoostTestTargetConfig.h>
 #include <iostream>
+#include <sstream>
+#include <utility>
+#include <vector>
 
 using namespace icinga;
 
@@ -809,4 +814,219 @@ BOOST_AUTO_TEST_CASE(service_flapping_ok_over_bad_into_ok)
 
 #endif /* I2_DEBUG */
 }
+
+BOOST_AUTO_TEST_CASE(suppressed_notification)
+{
+	/* Tests that suppressed notifications on a Checkable are sent after the suppression ends if and only if the first
+	 * hard state after the suppression is different from the last hard state before the suppression. The test works
+	 * by bringing a service in a defined hard state, creating a downtime, performing some state changes, removing the
+	 * downtime, bringing the service into another defined hard state (if not already) and checking the requested
+	 * notifications.
+	 */
+
+	struct NotificationLog {
+		std::vector<std::pair<NotificationType, ServiceState>> GetAndClear() {
+			std::lock_guard<std::mutex> lock (mutex);
+
+			std::vector<std::pair<NotificationType, ServiceState>> ret;
+			std::swap(ret, log);
+			return ret;
+		}
+
+		void Add(std::pair<NotificationType, ServiceState> notification) {
+			std::lock_guard<std::mutex> lock (mutex);
+
+			log.emplace_back(notification);
+		}
+
+	private:
+		std::mutex mutex;
+		std::vector<std::pair<NotificationType, ServiceState>> log;
+	};
+
+	const std::vector<ServiceState> states {ServiceOK, ServiceWarning, ServiceCritical, ServiceUnknown};
+
+	for (bool isVolatile : {false, true}) {
+		for (int checkAttempts : {1, 2}) {
+			for (ServiceState initialState : states) {
+				for (ServiceState s1 : states)
+				for (ServiceState s2 : states)
+				for (ServiceState s3 : states) {
+					const std::vector<ServiceState> sequence {s1, s2, s3};
+
+					std::string testcase;
+
+					{
+						std::ostringstream buf;
+						buf << "volatile=" << isVolatile
+							<< " checkAttempts=" << checkAttempts
+							<< " sequence={" << Service::StateToString(initialState);
+
+						for (ServiceState s : sequence) {
+							buf << " " << Service::StateToString(s);
+						}
+
+						buf << "}";
+						testcase = buf.str();
+					}
+
+					std::cout << "Test case: " << testcase << std::endl;
+
+					// Create host and service for the test.
+					Host::Ptr host = new Host();
+					host->SetName("suppressed_notifications");
+					host->Register();
+
+					Service::Ptr service = new Service();
+					service->SetHostName(host->GetName());
+					service->SetName("service");
+					service->SetActive(true);
+					service->SetVolatile(isVolatile);
+					service->SetMaxCheckAttempts(checkAttempts);
+					service->Activate();
+					service->SetAuthority(true);
+					service->Register();
+
+					host->OnAllConfigLoaded();
+					service->OnAllConfigLoaded();
+
+					// Bring service into the initial hard state.
+					for (int i = 0; i < checkAttempts; i++) {
+						std::cout << "  ProcessCheckResult("
+							<< Service::StateToString(initialState) << ")" << std::endl;
+						service->ProcessCheckResult(MakeCheckResult(initialState));
+					}
+
+					BOOST_CHECK(service->GetState() == initialState);
+					BOOST_CHECK(service->GetStateType() == StateTypeHard);
+
+					/* Keep track of all notifications requested from now on.
+					 *
+					 * Boost.Signal2 handler may still be executing from another thread after they were disconnected.
+					 * Make the structures accessed by the handlers shared pointers so that they remain valid as long
+					 * as they may be accessed from one of these handlers.
+					 */
+					auto notificationLog = std::make_shared<NotificationLog>();
+
+					boost::signals2::scoped_connection c (Checkable::OnNotificationsRequested.connect(
+						[notificationLog,service](
+							const Checkable::Ptr& checkable, NotificationType type,	const CheckResult::Ptr& cr,
+							const String&, const String&, const MessageOrigin::Ptr&
+						) {
+							BOOST_CHECK_EQUAL(checkable, service);
+							std::cout << "  -> OnNotificationsRequested(" << Notification::NotificationTypeToString(type)
+								<< ", " << Service::StateToString(cr->GetState()) << ")" << std::endl;
+
+							notificationLog->Add({type, cr->GetState()});
+						}
+					));
+
+					// Helper to assert which notifications were requested. Implicitly clears the stored notifications.
+					auto assertNotifications = [notificationLog](
+						const std::vector<std::pair<NotificationType, ServiceState>>& expected,
+						const std::string& extraMessage
+					) {
+						// Pretty-printer for the vectors of requested and expected notifications.
+						auto pretty = [](const std::vector<std::pair<NotificationType, ServiceState>>& vec) {
+							std::ostringstream s;
+
+							s << "{";
+							bool first = true;
+							for (const auto &v : vec) {
+								if (first) {
+									first = false;
+								} else {
+									s << ", ";
+								}
+								s << Notification::NotificationTypeToString(v.first)
+								  << "/" << Service::StateToString(v.second);
+							}
+							s << "}";
+
+							return s.str();
+						};
+
+						auto got (notificationLog->GetAndClear());
+
+						BOOST_CHECK_MESSAGE(got == expected, "expected=" << pretty(expected)
+							<< " got=" << pretty(got)
+							<< (extraMessage.empty() ? "" : " ") << extraMessage);
+					};
+
+					// Start a downtime for the service.
+					std::cout << "  Downtime Start" << std::endl;
+					Downtime::Ptr downtime = new Downtime();
+					downtime->SetHostName(host->GetName());
+					downtime->SetServiceName(service->GetName());
+					downtime->SetName("downtime");
+					downtime->SetFixed(true);
+					downtime->SetStartTime(Utility::GetTime() - 3600);
+					downtime->SetEndTime(Utility::GetTime() + 3600);
+					service->RegisterDowntime(downtime);
+					downtime->Register();
+					downtime->OnAllConfigLoaded();
+					downtime->TriggerDowntime(Utility::GetTime());
+
+					BOOST_CHECK(service->IsInDowntime());
+
+					// Process check results for the state sequence.
+					for (ServiceState s : sequence) {
+						std::cout << "  ProcessCheckResult(" << Service::StateToString(s) << ")" << std::endl;
+						service->ProcessCheckResult(MakeCheckResult(s));
+						BOOST_CHECK(service->GetState() == s);
+						if (checkAttempts == 1) {
+							BOOST_CHECK(service->GetStateType() == StateTypeHard);
+						}
+					}
+
+					assertNotifications({}, "(no notifications in downtime)");
+
+					if (service->GetSuppressedNotifications()) {
+						BOOST_CHECK_EQUAL(service->GetStateBeforeSuppression(), initialState);
+					}
+
+					// Remove the downtime.
+					std::cout << "  Downtime End" << std::endl;
+					service->UnregisterDowntime(downtime);
+					downtime->Unregister();
+					BOOST_CHECK(!service->IsInDowntime());
+
+					if (service->GetStateType() == icinga::StateTypeSoft) {
+						// When the current state is a soft state, no notification should be sent just yet.
+						std::cout << "  FireSuppressedNotifications()" << std::endl;
+						service->FireSuppressedNotifications();
+
+						assertNotifications({}, testcase + " (should not fire in soft state)");
+
+						// Repeat the last check result until reaching a hard state.
+						for (int i = 0; i < checkAttempts && service->GetStateType() == StateTypeSoft; i++) {
+							std::cout << "  ProcessCheckResult(" << Service::StateToString(sequence.back()) << ")"
+									  << std::endl;
+							service->ProcessCheckResult(MakeCheckResult(sequence.back()));
+							BOOST_CHECK(service->GetState() == sequence.back());
+						}
+					}
+
+					// The service should be in a hard state now and notifications should now be sent if applicable.
+					BOOST_CHECK(service->GetStateType() == StateTypeHard);
+
+					std::cout << "  FireSuppressedNotifications()" << std::endl;
+					service->FireSuppressedNotifications();
+
+					if (initialState != sequence.back()) {
+						NotificationType t = sequence.back() == ServiceOK ? NotificationRecovery : NotificationProblem;
+						assertNotifications({{t, sequence.back()}}, testcase);
+					} else {
+						assertNotifications({}, testcase);
+					}
+
+					// Remove host and service.
+					service->Unregister();
+					host->Unregister();
+				}
+			}
+		}
+	}
+}
+
 BOOST_AUTO_TEST_SUITE_END()