From ce1ed8556c8a7c8ad693c197c3665cc5a2fbc99b Mon Sep 17 00:00:00 2001
From: Julian Brost <julian.brost@icinga.com>
Date: Wed, 12 Feb 2025 11:18:12 +0100
Subject: [PATCH] Simplify DependencyGroup::GetState() implementation

The new implementation just counts reachable and available parents and
determines the overall result by comparing numbers, see inline comments for
more information.

This also fixes an issue in the previous implementation: if it didn't return
early from the loop, it would just return the state of the last parent
considered which may not actually represent the group state accurately.
---
 lib/icinga/dependency-group.cpp | 42 ++++++++++++++++++++-------------
 lib/icinga/dependency.hpp       |  6 ++++-
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/lib/icinga/dependency-group.cpp b/lib/icinga/dependency-group.cpp
index 01520467e..043e8f514 100644
--- a/lib/icinga/dependency-group.cpp
+++ b/lib/icinga/dependency-group.cpp
@@ -322,26 +322,34 @@ DependencyGroup::State DependencyGroup::GetState(DependencyType dt, int rstack)
 		members = m_Members;
 	}
 
-	State state{false /* Reachable */, false /* OK */};
-	for (auto& [_, children] : members) {
-		for (auto& [checkable, dependency] : children) {
-			state.Reachable = dependency->GetParent()->IsReachable(dt, rstack);
-			if (!state.Reachable && !IsRedundancyGroup()) {
-				return state;
-			}
+	size_t reachable = 0, available = 0;
 
-			if (state.Reachable) {
-				state.OK = dependency->IsAvailable(dt);
-				// If this is a redundancy group, and we have found one functional path, that's enough and we can return.
-				// Likewise, if this is a non-redundant dependency group, and we have found one non-functional path,
-				// we have to mark the group as failed and return.
-				if (state.OK == IsRedundancyGroup()) { // OK && IsRedundancyGroup() || !OK && !IsRedundancyGroup()
-					return state;
-				}
+	for (auto& [_, dependencies] : members) {
+		ASSERT(!dependencies.empty());
+
+		// Dependencies are grouped by parent and all config attributes affecting their availability. Hence, only
+		// one dependency from the map entry has to be considered, all others will share the same state.
+		if (auto& [_, dependency] = *dependencies.begin(); dependency->GetParent()->IsReachable(dt, rstack)) {
+			reachable++;
+
+			// Only reachable parents are considered for availability. If they are unreachable and checks are disabled,
+			// they could be incorrectly treated as available otherwise.
+			if (dependency->IsAvailable(dt)) {
+				available++;
 			}
-			break; // Move on to the next batch of group members (next composite key).
 		}
 	}
 
-	return state;
+	if (IsRedundancyGroup()) {
+		// The state of a redundancy group is determined by the best state of any parent. If any parent ist reachable,
+		// the redundancy group is reachable, analogously for availability. Note that an unreachable group cannot be
+		// available as reachable = 0 implies available = 0.
+		return {reachable > 0, available > 0};
+	} else {
+		// For dependencies without a redundancy group, members.size() will be 1 in almost all cases. It will only
+		// contain more elements if there are duplicate dependency config objects between two checkables. In this case,
+		// all of them have to be reachable or available as they don't provide redundancy. Note that unreachable implies
+		// unavailable here as well as only reachable parents count towards the number of available parents.
+		return {reachable >= members.size(), available == members.size()};
+	}
 }
diff --git a/lib/icinga/dependency.hpp b/lib/icinga/dependency.hpp
index fe005dbb2..49fb93989 100644
--- a/lib/icinga/dependency.hpp
+++ b/lib/icinga/dependency.hpp
@@ -182,7 +182,7 @@ private:
 	{
 		size_t operator()(const DependencyGroup::Ptr& dependencyGroup) const
 		{
-			size_t hash = 0;
+			size_t hash = std::hash<String>{}(dependencyGroup->GetRedundancyGroupName());
 			for (const auto& [key, group] : dependencyGroup->m_Members) {
 				boost::hash_combine(hash, key);
 			}
@@ -194,6 +194,10 @@ private:
 	{
 		bool operator()(const DependencyGroup::Ptr& lhs, const DependencyGroup::Ptr& rhs) const
 		{
+			if (lhs->GetRedundancyGroupName() != rhs->GetRedundancyGroupName()) {
+				return false;
+			}
+
 			return std::equal(
 				lhs->m_Members.begin(), lhs->m_Members.end(),
 				rhs->m_Members.begin(), rhs->m_Members.end(),