Skip to content

Commit e5852cc

Browse files
committed
Evaluate dependency group state only for a specific child
Previously the dependency state was evaluated by picking the first dependency object from the batched members. However, since the dependency `disable_{checks,notifications` attributes aren't taken into account when batching the members, the evaluated state may yield a wrong result for some Checkables due to some random dependency from other Checkable of that group that has the `disable_{checks,notifications` attrs set. This commit forces the callers to always provide the child Checkable the state is evaluated for and picks only the dependency objects of that child Checkable.
1 parent 70797a9 commit e5852cc

File tree

6 files changed

+32
-16
lines changed

6 files changed

+32
-16
lines changed

lib/icinga/checkable-dependency.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ bool Checkable::IsReachable(DependencyType dt, int rstack) const
204204
}
205205

206206
for (auto& dependencyGroup : GetDependencyGroups()) {
207-
if (auto state(dependencyGroup->GetState(dt, rstack + 1)); !state.Reachable || !state.OK) {
207+
if (auto state(dependencyGroup->GetState(this, dt, rstack + 1)); !state.Reachable || !state.OK) {
208208
Log(LogDebug, "Checkable")
209209
<< "Dependency group '" << dependencyGroup->GetRedundancyGroupName() << "' have failed for checkable '"
210210
<< GetName() << "': Marking as unreachable.";

lib/icinga/dependency-group.cpp

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -324,9 +324,13 @@ String DependencyGroup::GetCompositeKey()
324324
* a dependency group may still be marked as failed even when it has reachable parent Checkables, but an unreachable
325325
* group has always a failed state.
326326
*
327+
* @param child The child Checkable to evaluate the state for.
328+
* @param dt The dependency type to evaluate the state for, defaults to DependencyState.
329+
* @param rstack The recursion stack level to prevent infinite recursion, defaults to 0.
330+
*
327331
* @return - Returns the state of the current dependency group.
328332
*/
329-
DependencyGroup::State DependencyGroup::GetState(DependencyType dt, int rstack) const
333+
DependencyGroup::State DependencyGroup::GetState(const Checkable* child, DependencyType dt, int rstack) const
330334
{
331335
MembersMap members;
332336
{
@@ -340,15 +344,27 @@ DependencyGroup::State DependencyGroup::GetState(DependencyType dt, int rstack)
340344
for (auto& [_, dependencies] : members) {
341345
ASSERT(!dependencies.empty());
342346

343-
// Dependencies are grouped by parent and all config attributes affecting their availability. Hence, only
344-
// one dependency from the map entry has to be considered, all others will share the same state.
345-
if (auto& [_, dependency] = *dependencies.begin(); dependency->GetParent()->IsReachable(dt, rstack)) {
346-
reachable++;
347+
auto [begin, end] = dependencies.equal_range(child);
348+
for (auto it(begin); it != end; ++it) {
349+
Dependency::Ptr dependency(it->second);
350+
if (dependency->GetParent()->IsReachable(dt, rstack)) {
351+
reachable++;
352+
353+
// Only reachable parents are considered for availability. If they are unreachable and checks are disabled,
354+
// they could be incorrectly treated as available otherwise.
355+
if (dependency->IsAvailable(dt)) {
356+
available++;
357+
}
358+
}
347359

348-
// Only reachable parents are considered for availability. If they are unreachable and checks are disabled,
349-
// they could be incorrectly treated as available otherwise.
350-
if (dependency->IsAvailable(dt)) {
351-
available++;
360+
if (dt == DependencyState) {
361+
// Dependencies are grouped by parent and all config attributes affecting their availability.
362+
// However, their disable_{checks,notifications} attrs aren't taken into account when grouping them,
363+
// thus the map may contain multiple dependencies for that child with different config of these attrs.
364+
// Therefore, if this state evaluation is for DependencyState, we can break here as we only need to
365+
// consider only a single dependency as they all share the same state, otherwise we need to evaluate
366+
// all of them.
367+
break;
352368
}
353369
}
354370
}

lib/icinga/dependency.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ class DependencyGroup final : public SharedObject
163163
bool OK; // Whether the dependency group is reachable and OK.
164164
};
165165

166-
State GetState(DependencyType dt = DependencyState, int rstack = 0) const;
166+
State GetState(const Checkable* child, DependencyType dt = DependencyState, int rstack = 0) const;
167167

168168
static boost::signals2::signal<void(const Checkable::Ptr&, const DependencyGroup::Ptr&)> OnChildRegistered;
169169
static boost::signals2::signal<void(const DependencyGroup::Ptr&, const std::vector<Dependency::Ptr>&)> OnChildRemoved;

lib/icingadb/icingadb-objects.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,7 +1275,7 @@ void IcingaDB::InsertCheckableDependencies(
12751275
AddObjectDataToRuntimeUpdates(*runtimeUpdates, redundancyGroupId, m_PrefixConfigObject + "dependency:node", nodeData);
12761276
}
12771277

1278-
auto stateAttrs(SerializeRedundancyGroupState(dependencyGroup));
1278+
auto stateAttrs(SerializeRedundancyGroupState(checkable, dependencyGroup));
12791279
hmsetRedundancyGroupsStates.emplace_back(redundancyGroupId);
12801280
hmsetRedundancyGroupsStates.emplace_back(JsonEncode(stateAttrs));
12811281

@@ -1471,7 +1471,7 @@ void IcingaDB::UpdateDependenciesState(const Checkable::Ptr& checkable) const
14711471
}
14721472

14731473
if (isRedundancyGroup) {
1474-
Dictionary::Ptr stateAttrs(SerializeRedundancyGroupState(dependencyGroup));
1474+
Dictionary::Ptr stateAttrs(SerializeRedundancyGroupState(checkable, dependencyGroup));
14751475

14761476
Dictionary::Ptr sharedGroupState(stateAttrs->ShallowClone());
14771477
sharedGroupState->Remove("redundancy_group_id");

lib/icingadb/icingadb-utility.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -202,9 +202,9 @@ Dictionary::Ptr IcingaDB::SerializeDependencyEdgeState(const DependencyGroup::Pt
202202
*
203203
* @return A dictionary with the serialized redundancy group state.
204204
*/
205-
Dictionary::Ptr IcingaDB::SerializeRedundancyGroupState(const DependencyGroup::Ptr& redundancyGroup)
205+
Dictionary::Ptr IcingaDB::SerializeRedundancyGroupState(const Checkable::Ptr& child, const DependencyGroup::Ptr& redundancyGroup)
206206
{
207-
auto state(redundancyGroup->GetState());
207+
auto state(redundancyGroup->GetState(child.get()));
208208
return new Dictionary{
209209
{"id", redundancyGroup->GetIcingaDBIdentifier()},
210210
{"environment_id", m_EnvironmentId},

lib/icingadb/icingadb.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ class IcingaDB : public ObjectImpl<IcingaDB>
164164
static const char* GetNotificationTypeByEnum(NotificationType type);
165165
static Dictionary::Ptr SerializeVars(const Dictionary::Ptr& vars);
166166
static Dictionary::Ptr SerializeDependencyEdgeState(const DependencyGroup::Ptr& dependencyGroup, const Dependency::Ptr& dep);
167-
static Dictionary::Ptr SerializeRedundancyGroupState(const DependencyGroup::Ptr& redundancyGroup);
167+
static Dictionary::Ptr SerializeRedundancyGroupState(const Checkable::Ptr& child, const DependencyGroup::Ptr& redundancyGroup);
168168

169169
static String HashValue(const Value& value);
170170
static String HashValue(const Value& value, const std::set<String>& propertiesBlacklist, bool propertiesWhitelist = false);

0 commit comments

Comments
 (0)