From ccb18a04ecaa41d60101f41bc28039700bba9d69 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Fri, 28 Jan 2022 15:15:44 +0100 Subject: [PATCH] Checkable: Add test for state notifications after a suppression ends --- lib/icinga/checkable-notification.cpp | 46 +++--- lib/icinga/checkable.cpp | 2 +- lib/icinga/checkable.hpp | 4 +- lib/icinga/downtime.hpp | 3 +- lib/icinga/downtime.ti | 4 +- lib/icinga/host.hpp | 3 +- lib/icinga/service.hpp | 3 +- test/CMakeLists.txt | 1 + test/icinga-checkresult.cpp | 220 ++++++++++++++++++++++++++ 9 files changed, 256 insertions(+), 30 deletions(-) diff --git a/lib/icinga/checkable-notification.cpp b/lib/icinga/checkable-notification.cpp index 7978d4076..21bcc399f 100644 --- a/lib/icinga/checkable-notification.cpp +++ b/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 wasLastParentRecoveryRecent ([&checkable]() { - auto cr (checkable->GetLastCheckResult()); + LazyInit wasLastParentRecoveryRecent ([this]() { + 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(); - NotificationType type = cr && checkable->IsStateOK(cr->GetState()) ? NotificationRecovery : NotificationProblem; - bool state_suppressed = checkable->NotificationReasonSuppressed(NotificationProblem) || checkable->NotificationReasonSuppressed(NotificationRecovery); + CheckResult::Ptr cr = GetLastCheckResult(); + NotificationType type = cr && IsStateOK(cr->GetState()) ? NotificationRecovery : NotificationProblem; + 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 (checkable->NotificationReasonApplies(type)) { - Checkable::OnNotificationsRequested(checkable, type, cr, "", "", nullptr); + if (!state_suppressed && GetStateType() == StateTypeHard && !IsLikelyToBeCheckedSoon() && !wasLastParentRecoveryRecent.Get()) { + if (NotificationReasonApplies(type)) { + 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()) { - Checkable::OnNotificationsRequested(checkable, type, checkable->GetLastCheckResult(), "", "", nullptr); + if (!NotificationReasonSuppressed(type) && !IsLikelyToBeCheckedSoon() && !wasLastParentRecoveryRecent.Get()) { + 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()) { - ::FireSuppressedNotifications(host.get()); + host->FireSuppressedNotifications(); } for (auto& service : ConfigType::GetObjectsByType()) { - ::FireSuppressedNotifications(service.get()); + service->FireSuppressedNotifications(); } } diff --git a/lib/icinga/checkable.cpp b/lib/icinga/checkable.cpp index 6ec7f6f92..b212389b3 100644 --- a/lib/icinga/checkable.cpp +++ b/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(); diff --git a/lib/icinga/checkable.hpp b/lib/icinga/checkable.hpp index 292dac281..bb8e567b4 100644 --- a/lib/icinga/checkable.hpp +++ b/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 */ diff --git a/lib/icinga/downtime.hpp b/lib/icinga/downtime.hpp index 38d4e41e4..5f0c4937b 100644 --- a/lib/icinga/downtime.hpp +++ b/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; diff --git a/lib/icinga/downtime.ti b/lib/icinga/downtime.ti index d21f0ebf5..9fe182392 100644 --- a/lib/icinga/downtime.ti +++ b/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); diff --git a/lib/icinga/host.hpp b/lib/icinga/host.hpp index 87f560af5..d0d6c1aa4 100644 --- a/lib/icinga/host.hpp +++ b/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: diff --git a/lib/icinga/service.hpp b/lib/icinga/service.hpp index 493f261e2..74237018b 100644 --- a/lib/icinga/service.hpp +++ b/lib/icinga/service.hpp @@ -44,10 +44,11 @@ public: static void EvaluateApplyRules(const Host::Ptr& host); + void OnAllConfigLoaded() override; + static boost::signals2::signal OnHostProblemChanged; protected: - void OnAllConfigLoaded() override; void CreateChildObjects(const Type::Ptr& childType) override; private: diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 530dd1cc9..8b800d914 100644 --- a/test/CMakeLists.txt +++ b/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 diff --git a/test/icinga-checkresult.cpp b/test/icinga-checkresult.cpp index 33bbc509e..fdc78915e 100644 --- a/test/icinga-checkresult.cpp +++ b/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 #include +#include +#include +#include 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> GetAndClear() { + std::lock_guard lock (mutex); + + std::vector> ret; + std::swap(ret, log); + return ret; + } + + void Add(std::pair notification) { + std::lock_guard lock (mutex); + + log.emplace_back(notification); + } + + private: + std::mutex mutex; + std::vector> log; + }; + + const std::vector 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 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(); + + 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>& expected, + const std::string& extraMessage + ) { + // Pretty-printer for the vectors of requested and expected notifications. + auto pretty = [](const std::vector>& 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()