Fix overdue state doesn't account for timeperiods in HA cluster

lib/checker/checkercomponent.cpp

@@ -55,6 +55,9 @@ void CheckerComponent::OnConfigLoaded()
 	Checkable::OnNextCheckChanged.connect([this](const Checkable::Ptr& checkable, const Value&) {
 		NextCheckChangedHandler(checkable);
 	});
+	Checkable::OnRescheduleCheck.connect([this](const Checkable::Ptr& checkable, double nextCheck) {
+		NextCheckChangedHandler(checkable, nextCheck);
+	});
 }
 
 void CheckerComponent::Start(bool runtimeCreated)
@@ -68,7 +71,7 @@ void CheckerComponent::Start(bool runtimeCreated)
 	m_Thread = std::thread([this]() { CheckThreadProc(); });
 
 	m_ResultTimer = Timer::Create();
-	m_ResultTimer->SetInterval(5);
+	m_ResultTimer->SetInterval(m_ResultTimerInterval);
 	m_ResultTimer->OnTimerExpired.connect([this](const Timer * const&) { ResultTimerHandler(); });
 	m_ResultTimer->Start();
 }
@@ -135,7 +138,6 @@ void CheckerComponent::CheckThreadProc()
 
 		bool forced = checkable->GetForceNextCheck();
 		bool check = true;
-		bool notifyNextCheck = false;
 		double nextCheck = -1;
 
 		if (!forced) {
@@ -144,7 +146,6 @@ void CheckerComponent::CheckThreadProc()
 					<< "Skipping check for object '" << checkable->GetName() << "': Dependency failed.";
 
 				check = false;
-				notifyNextCheck = true;
 			}
 
 			Host::Ptr host;
@@ -181,7 +182,6 @@ void CheckerComponent::CheckThreadProc()
 						<< Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", nextCheck);
 
 					check = false;
-					notifyNextCheck = true;
 				}
 			}
 		}
@@ -200,11 +200,6 @@ void CheckerComponent::CheckThreadProc()
 				checkable->UpdateNextCheck();
 			}
 
-			if (notifyNextCheck) {
-				// Trigger update event for Icinga DB
-				Checkable::OnNextCheckUpdated(checkable);
-			}
-
 			lock.lock();
 
 			continue;
@@ -341,7 +336,7 @@ CheckableScheduleInfo CheckerComponent::GetCheckableScheduleInfo(const Checkable
 	return csi;
 }
 
-void CheckerComponent::NextCheckChangedHandler(const Checkable::Ptr& checkable)
+void CheckerComponent::NextCheckChangedHandler(const Checkable::Ptr& checkable, double nextCheck)
 {
 	std::unique_lock<std::mutex> lock(m_Mutex);
 
@@ -356,7 +351,13 @@ void CheckerComponent::NextCheckChangedHandler(const Checkable::Ptr& checkable)
 
 	idx.erase(checkable);
 
-	CheckableScheduleInfo csi = GetCheckableScheduleInfo(checkable);
+	CheckableScheduleInfo csi;
+	if (nextCheck < 0) {
+		csi = GetCheckableScheduleInfo(checkable);
+	} else {
+		csi.NextCheck = nextCheck;
+		csi.Object = checkable;
+	}
 	idx.insert(csi);
 
 	m_CV.notify_all();
@@ -375,3 +376,18 @@ unsigned long CheckerComponent::GetPendingCheckables()
 
 	return m_PendingCheckables.size();
 }
+
+/**
+ * Sets the interval in seconds for the result timer.
+ *
+ * The result timer periodically logs the number of pending and idle checkables
+ * as well as the checks per second rate. The default interval is 5 seconds.
+ *
+ * Note, this method must be called before the component is started to have an effect on the timer.
+ *
+ * @param interval Interval in seconds for the result timer.
+ */
+void CheckerComponent::SetResultTimerInterval(double interval)
+{
+	m_ResultTimerInterval = interval;
+}

lib/checker/checkercomponent.hpp

@@ -26,21 +26,25 @@ struct CheckableScheduleInfo
 {
 	Checkable::Ptr Object;
 	double NextCheck;
-};
-
-/**
- * @ingroup checker
- */
-struct CheckableNextCheckExtractor
-{
-	typedef double result_type;
 
 	/**
-	 * @threadsafety Always.
+	 * Get the index value for ordering in the multi-index container.
+	 *
+	 * This function returns a very large value for checkables that have a running check, effectively pushing
+	 * them to the end of the ordering. This ensures that checkables with running checks are not prioritized
+	 * for scheduling ahead of others. Rescheduling of such checkables is unnecessary because the checkable
+	 * is going to reject this anyway if it notices that a check is already running, so avoiding unnecessary
+	 * CPU load. Once the running check is finished, the checkable will be re-inserted into the set with its
+	 * actual next check time as the index value.
+	 *
+	 * @return The index value for ordering in the multi-index container.
 	 */
-	double operator()(const CheckableScheduleInfo& csi)
+	double Index() const
 	{
-		return csi.NextCheck;
+		if (Object->HasRunningCheck()) {
+			return std::numeric_limits<double>::max();
+		}
+		return NextCheck;
 	}
 };
 
@@ -57,7 +61,7 @@ class CheckerComponent final : public ObjectImpl<CheckerComponent>
 		CheckableScheduleInfo,
 		boost::multi_index::indexed_by<
 			boost::multi_index::ordered_unique<boost::multi_index::member<CheckableScheduleInfo, Checkable::Ptr, &CheckableScheduleInfo::Object> >,
-			boost::multi_index::ordered_non_unique<CheckableNextCheckExtractor>
+			boost::multi_index::ordered_non_unique<boost::multi_index::const_mem_fun<CheckableScheduleInfo, double, &CheckableScheduleInfo::Index>>
 		>
 	> CheckableSet;
 
@@ -69,12 +73,16 @@ class CheckerComponent final : public ObjectImpl<CheckerComponent>
 	unsigned long GetIdleCheckables();
 	unsigned long GetPendingCheckables();
 
+	void SetResultTimerInterval(double interval);
+
 private:
 	std::mutex m_Mutex;
 	std::condition_variable m_CV;
 	bool m_Stopped{false};
 	std::thread m_Thread;
 
+	double m_ResultTimerInterval{5.0}; // Interval in seconds for the result timer.
+
 	CheckableSet m_IdleCheckables;
 	CheckableSet m_PendingCheckables;
 
@@ -86,12 +94,8 @@ class CheckerComponent final : public ObjectImpl<CheckerComponent>
 
 	void ExecuteCheckHelper(const Checkable::Ptr& checkable);
 
-	void AdjustCheckTimer();
-
 	void ObjectHandler(const ConfigObject::Ptr& object);
-	void NextCheckChangedHandler(const Checkable::Ptr& checkable);
-
-	void RescheduleCheckTimer();
+	void NextCheckChangedHandler(const Checkable::Ptr& checkable, double nextCheck = -1);
 
 	static CheckableScheduleInfo GetCheckableScheduleInfo(const Checkable::Ptr& checkable);
 };

lib/db_ido/dbevents.cpp

@@ -40,7 +40,7 @@ void DbEvents::StaticInitialize()
 		DbEvents::RemoveAcknowledgement(checkable);
 	});
 
-	Checkable::OnNextCheckUpdated.connect([](const Checkable::Ptr& checkable) { NextCheckUpdatedHandler(checkable); });
+	Checkable::OnNextCheckChanged.connect([](const Checkable::Ptr& checkable, const Value&) { NextCheckUpdatedHandler(checkable); });
 	Checkable::OnFlappingChanged.connect([](const Checkable::Ptr& checkable, const Value&) { FlappingChangedHandler(checkable); });
 	Checkable::OnNotificationSentToAllUsers.connect([](const Notification::Ptr& notification, const Checkable::Ptr& checkable,
 		const std::set<User::Ptr>&, const NotificationType&, const CheckResult::Ptr&, const String&, const String&,

lib/icinga/apiactions.cpp

@@ -159,9 +159,6 @@ Dictionary::Ptr ApiActions::RescheduleCheck(const ConfigObject::Ptr& object,
 
 	checkable->SetNextCheck(nextCheck);
 
-	/* trigger update event for DB IDO */
-	Checkable::OnNextCheckUpdated(checkable);
-
 	return ApiActions::CreateResult(200, "Successfully rescheduled check for object '" + checkable->GetName() + "'.");
 }
 

lib/icinga/checkable-check.cpp

@@ -22,7 +22,7 @@ boost::signals2::signal<void (const Checkable::Ptr&, const CheckResult::Ptr&, co
 boost::signals2::signal<void (const Checkable::Ptr&, const CheckResult::Ptr&, StateType, const MessageOrigin::Ptr&)> Checkable::OnStateChange;
 boost::signals2::signal<void (const Checkable::Ptr&, const CheckResult::Ptr&, std::set<Checkable::Ptr>, const MessageOrigin::Ptr&)> Checkable::OnReachabilityChanged;
 boost::signals2::signal<void (const Checkable::Ptr&, NotificationType, const CheckResult::Ptr&, const String&, const String&, const MessageOrigin::Ptr&)> Checkable::OnNotificationsRequested;
-boost::signals2::signal<void (const Checkable::Ptr&)> Checkable::OnNextCheckUpdated;
+boost::signals2::signal<void (const Checkable::Ptr&, double)> Checkable::OnRescheduleCheck;
 
 Atomic<uint_fast64_t> Checkable::CurrentConcurrentChecks (0);
 
@@ -45,12 +45,22 @@ void Checkable::SetSchedulingOffset(long offset)
 	m_SchedulingOffset = offset;
 }
 
-long Checkable::GetSchedulingOffset()
+long Checkable::GetSchedulingOffset() const
 {
 	return m_SchedulingOffset;
 }
 
-void Checkable::UpdateNextCheck(const MessageOrigin::Ptr& origin)
+/**
+ * Update the next check time of this checkable based on its check interval and last check time.
+ *
+ * If onlyReschedule is true, the next check time is not actually updated, but the @c Checkable::OnRescheduleCheck
+ * signal is emitted with the new calculated next check time. Otherwise, the next check time is updated
+ * and the @c Checkable::OnNextCheckChanged signal is emitted accordingly.
+ *
+ * @param origin The origin of the message triggering this update, can be nullptr.
+ * @param onlyReschedule If true, only emit @c OnRescheduleCheck without updating the next check time.
+ */
+void Checkable::UpdateNextCheck(const MessageOrigin::Ptr& origin, bool onlyReschedule)
 {
 	double interval;
 
@@ -78,14 +88,26 @@ void Checkable::UpdateNextCheck(const MessageOrigin::Ptr& origin)
 		<< " (" << lastCheck << ") to next check time at "
 		<< Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", nextCheck) << " (" << nextCheck << ").";
 
-	SetNextCheck(nextCheck, false, origin);
+	if (onlyReschedule) {
+		// Someone requested to only reschedule the next check without actually changing it.
+		// So, just tell the checker about this new timestamp and return.
+		OnRescheduleCheck(this, nextCheck);
+	} else {
+		// Otherwise, set the next check to the newly calculated timestamp and inform all its listeners.
+		SetNextCheck(nextCheck, false, origin);
+	}
 }
 
 bool Checkable::HasBeenChecked() const
 {
 	return GetLastCheckResult() != nullptr;
 }
 
+bool Checkable::HasRunningCheck() const
+{
+	return m_CheckRunning;
+}
+
 double Checkable::GetLastCheck() const
 {
 	CheckResult::Ptr cr = GetLastCheckResult();
@@ -105,7 +127,7 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr
 	VERIFY(producer);
 
 	ObjectLock olock(this);
-	m_CheckRunning = false;
+	m_CheckRunning.store(false);
 
 	double now = Utility::GetTime();
 
@@ -513,12 +535,15 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr
 	if (recovery) {
 		for (auto& child : children) {
 			if (child->GetProblem() && child->GetEnableActiveChecks()) {
-				auto nextCheck (now + Utility::Random() % 60);
-
-				ObjectLock oLock (child);
-
-				if (nextCheck < child->GetNextCheck()) {
-					child->SetNextCheck(nextCheck);
+				if (auto nextCheck (now + Utility::Random() % 60); nextCheck < child->GetNextCheck()) {
+					/**
+					 * We only want to enforce the checker to pick this up sooner, and no need to actually change
+					 * the timesatmp. Plus, no other listeners should be informed about this other than the checker,
+					 * so we emit the OnRescheduleCheck signal directly. In case our checker isn't responsible for
+					 * this child object, we've already broadcasted the `CheckResult` event which will cause on the
+					 * responsible node to enter this exact branch and do the rescheduling for its own checker.
+					 */
+					OnRescheduleCheck(child, nextCheck);
 				}
 			}
 		}
@@ -534,8 +559,8 @@ Checkable::ProcessingResult Checkable::ProcessCheckResult(const CheckResult::Ptr
 				continue;
 
 			if (parent->GetNextCheck() >= now + parent->GetRetryInterval()) {
-				ObjectLock olock(parent);
-				parent->SetNextCheck(now);
+				// See comment above for children. We want to just enforce an immediate check by our checker.
+				OnRescheduleCheck(parent, now);
 			}
 		}
 	}
@@ -561,29 +586,21 @@ void Checkable::ExecuteCheck(const WaitGroup::Ptr& producer)
 {
 	CONTEXT("Executing check for object '" << GetName() << "'");
 
+	/* don't run another check if there is one pending */
+	if (m_CheckRunning.exchange(true))
+		return; // Should never happen as the checker already takes care of this.
+
 	/* keep track of scheduling info in case the check type doesn't provide its own information */
 	double scheduled_start = GetNextCheck();
 	double before_check = Utility::GetTime();
 
 	SetLastCheckStarted(Utility::GetTime());
 
-	/* This calls SetNextCheck() which updates the CheckerComponent's idle/pending
-	 * queues and ensures that checks are not fired multiple times. ProcessCheckResult()
-	 * is called too late. See #6421.
-	 */
-	UpdateNextCheck();
-
 	bool reachable = IsReachable();
 
 	{
 		ObjectLock olock(this);
 
-		/* don't run another check if there is one pending */
-		if (m_CheckRunning)
-			return;
-
-		m_CheckRunning = true;
-
 		SetLastStateRaw(GetStateRaw());
 		SetLastStateType(GetLastStateType());
 		SetLastReachable(reachable);
@@ -640,11 +657,16 @@ void Checkable::ExecuteCheck(const WaitGroup::Ptr& producer)
 			if (listener)
 				listener->SyncSendMessage(endpoint, message);
 
-			/* Re-schedule the check so we don't run it again until after we've received
-			 * a check result from the remote instance. The check will be re-scheduled
-			 * using the proper check interval once we've received a check result.
+			/*
+			 * Let the checker use a dummy next check time until we actually receive the check result from the
+			 * remote endpoint. This should be sufficiently far in the future to avoid excessive CPU load by
+			 * constantly re-running the check, but not too far in the future to avoid that the check is not
+			 * re-run for too long in case the remote endpoint never responds. We add a small grace period
+			 * to the check command timeout to account for network latency and processing time on the remote
+			 * endpoint. So, we only need to silently update this without notifying any listeners, and once
+			 * this function returns, the checker is going access it via GetNextCheck() again.
 			 */
-			SetNextCheck(Utility::GetTime() + checkTimeout + 30);
+			SetNextCheck(Utility::GetTime() + checkTimeout + 30, true);
 
 		/*
 		 * Let the user know that there was a problem with the check if
@@ -667,12 +689,22 @@ void Checkable::ExecuteCheck(const WaitGroup::Ptr& producer)
 			cr->SetOutput(output);
 
 			ProcessCheckResult(cr, producer);
+		} else {
+			/**
+			 * The endpoint is currently either syncing its state or not connected yet and we are within
+			 * the magical 5min cold startup window. In both cases, we just don't do anything and wait for
+			 * the next check interval to re-try the check again. So, this check is effectively skipped.
+			 */
+			UpdateNextCheck();
 		}
 
-		{
-			ObjectLock olock(this);
-			m_CheckRunning = false;
-		}
+		/**
+		 * If this is a remote check, we don't know when the check result will be received and processed.
+		 * Therefore, we must mark the check as no longer running here, otherwise, no further checks
+		 * would be executed for this checkable as it would always appear as having a running check
+		 * (see the check at the start of this function).
+		 */
+		m_CheckRunning.store(false);
 	}
 }