diff --git a/lib/icinga/checkable-dependency.cpp b/lib/icinga/checkable-dependency.cpp index 921b36a7e..27ea5c0ae 100644 --- a/lib/icinga/checkable-dependency.cpp +++ b/lib/icinga/checkable-dependency.cpp @@ -26,15 +26,12 @@ static constexpr int l_MaxDependencyRecursionLevel(256); void Checkable::PushDependencyGroupsToRegistry() { std::lock_guard lock(m_DependencyMutex); - if (!m_DependencyGroupsPushedToRegistry) { - m_DependencyGroupsPushedToRegistry = true; - - decltype(m_DependencyGroups) dependencyGroups; - m_DependencyGroups.swap(dependencyGroups); - - for (auto& [dependencyGroupKey, dependencyGroup] : dependencyGroups) { - m_DependencyGroups.emplace(dependencyGroupKey, DependencyGroup::Register(dependencyGroup)); + if (m_PendingDependencies != nullptr) { + for (const auto& [key, dependencies] : *m_PendingDependencies) { + String redundancyGroup = std::holds_alternative(key) ? std::get(key) : ""; + m_DependencyGroups.emplace(key, DependencyGroup::Register(new DependencyGroup(redundancyGroup, dependencies))); } + m_PendingDependencies.reset(); } } @@ -78,12 +75,8 @@ void Checkable::AddDependency(const Dependency::Ptr& dependency) std::unique_lock lock(m_DependencyMutex); auto dependencyGroupKey(GetDependencyGroupKey(dependency)); - if (!m_DependencyGroupsPushedToRegistry) { - auto& dependencyGroup = m_DependencyGroups[dependencyGroupKey]; - if (!dependencyGroup) { - dependencyGroup = new DependencyGroup(dependency->GetRedundancyGroup()); - } - dependencyGroup->AddDependency(dependency); + if (m_PendingDependencies != nullptr) { + (*m_PendingDependencies)[dependencyGroupKey].emplace(dependency); return; } @@ -151,10 +144,17 @@ void Checkable::RemoveDependency(const Dependency::Ptr& dependency, bool runtime } } -std::vector Checkable::GetDependencies() const +std::vector Checkable::GetDependencies(bool includePending) const { std::unique_lock lock(m_DependencyMutex); std::vector dependencies; + + if (includePending && m_PendingDependencies != nullptr) { + for (const auto& [group, groupDeps] : *m_PendingDependencies) { + dependencies.insert(dependencies.end(), groupDeps.begin(), groupDeps.end()); + } + } + for (const auto& [_, dependencyGroup] : m_DependencyGroups) { auto tmpDependencies(dependencyGroup->GetDependenciesForChild(this)); dependencies.insert(dependencies.end(), tmpDependencies.begin(), tmpDependencies.end()); diff --git a/lib/icinga/checkable.hpp b/lib/icinga/checkable.hpp index 53db79d72..98c015ed6 100644 --- a/lib/icinga/checkable.hpp +++ b/lib/icinga/checkable.hpp @@ -190,7 +190,7 @@ public: std::vector> GetDependencyGroups() const; void AddDependency(const intrusive_ptr& dependency); void RemoveDependency(const intrusive_ptr& dependency, bool runtimeRemoved = false); - std::vector > GetDependencies() const; + std::vector > GetDependencies(bool includePending = false) const; bool HasAnyDependencies() const; void AddReverseDependency(const intrusive_ptr& dep); @@ -251,9 +251,19 @@ private: /* Dependencies */ mutable std::mutex m_DependencyMutex; - bool m_DependencyGroupsPushedToRegistry{false}; std::map, intrusive_ptr> m_DependencyGroups; std::set > m_ReverseDependencies; + /** + * Registering a checkable to its parent DependencyGroups is delayed during config loading until all dependencies + * were registered on the checkable. m_PendingDependencies is used to temporarily store the dependencies until then. + * It is a pointer type for two reasons: + * 1. The field is no longer needed after the DependencyGroups were registered, having it as a pointer reduces the + * overhead from sizeof(std::map<>) to sizeof(std::map<>*). + * 2. It allows the field to also be used as a flag: the delayed group registration is only done until it is reset + * to nullptr. + */ + std::unique_ptr, std::set>>> + m_PendingDependencies {std::make_unique()}; void GetAllChildrenInternal(std::set& seenChildren, int level = 0) const; diff --git a/lib/icinga/dependency-group.cpp b/lib/icinga/dependency-group.cpp index fdf58b5e2..29d628832 100644 --- a/lib/icinga/dependency-group.cpp +++ b/lib/icinga/dependency-group.cpp @@ -46,17 +46,18 @@ std::pair, bool> DependencyGroup::Unregister(const Dep { std::lock_guard lock(m_RegistryMutex); if (auto it(m_Registry.find(dependencyGroup)); it != m_Registry.end()) { - auto existingGroup(*it); + auto& existingGroup(*it); auto dependencies(existingGroup->GetDependenciesForChild(child.get())); for (const auto& dependency : dependencies) { existingGroup->RemoveDependency(dependency); } - if (existingGroup->IsEmpty()) { + bool remove = !existingGroup->HasChildren(); + if (remove) { m_Registry.erase(it); } - return {{dependencies.begin(), dependencies.end()}, existingGroup->IsEmpty()}; + return {{dependencies.begin(), dependencies.end()}, remove}; } return {{}, false}; } @@ -72,14 +73,11 @@ size_t DependencyGroup::GetRegistrySize() return m_Registry.size(); } -DependencyGroup::DependencyGroup(String name): m_RedundancyGroupName(std::move(name)) -{ -} - -DependencyGroup::DependencyGroup(String name, const std::set& dependencies): m_RedundancyGroupName(std::move(name)) +DependencyGroup::DependencyGroup(String name, const std::set& dependencies) + : m_RedundancyGroupName(std::move(name)) { for (const auto& dependency : dependencies) { - AddDependency(dependency); + m_Members[MakeCompositeKeyFor(dependency)].emplace(dependency->GetChild().get(), dependency.get()); } } @@ -103,14 +101,14 @@ DependencyGroup::CompositeKeyType DependencyGroup::MakeCompositeKeyFor(const Dep } /** - * Check if the current dependency group is empty. + * Check if the current dependency has any children. * - * @return bool - Returns true if the current dependency group has no members, otherwise false. + * @return bool - Returns true if the current dependency group has children, otherwise false. */ -bool DependencyGroup::IsEmpty() const +bool DependencyGroup::HasChildren() const { std::lock_guard lock(m_Mutex); - return m_Members.empty(); + return std::any_of(m_Members.begin(), m_Members.end(), [](const auto& pair) { return !pair.second.empty(); }); } /** @@ -142,7 +140,6 @@ void DependencyGroup::LoadParents(std::set& parents) const { std::lock_guard lock(m_Mutex); for (auto& [compositeKey, children] : m_Members) { - ASSERT(!children.empty()); // We should never have an empty map for any given key at any given time. parents.insert(std::get<0>(compositeKey)); } } @@ -174,11 +171,12 @@ void DependencyGroup::AddDependency(const Dependency::Ptr& dependency) { std::lock_guard lock(m_Mutex); auto compositeKey(MakeCompositeKeyFor(dependency)); - if (auto it(m_Members.find(compositeKey)); it != m_Members.end()) { - it->second.emplace(dependency->GetChild().get(), dependency.get()); - } else { - m_Members.emplace(compositeKey, MemberValueType{{dependency->GetChild().get(), dependency.get()}}); - } + auto it = m_Members.find(compositeKey); + + // The dependency must be compatible with the group, i.e. its parent config must be known in the group already. + VERIFY(it != m_Members.end()); + + it->second.emplace(dependency->GetChild().get(), dependency.get()); } /** @@ -196,10 +194,6 @@ void DependencyGroup::RemoveDependency(const Dependency::Ptr& dependency) // This will also remove the child Checkable from the multimap container // entirely if this was the last child of it. it->second.erase(childrenIt); - // If the composite key has no more children left, we can remove it entirely as well. - if (it->second.empty()) { - m_Members.erase(it); - } return; } } diff --git a/lib/icinga/dependency.cpp b/lib/icinga/dependency.cpp index 52456ff4b..cf4af8627 100644 --- a/lib/icinga/dependency.cpp +++ b/lib/icinga/dependency.cpp @@ -123,7 +123,7 @@ public: } // Explicitly configured dependency objects - for (const auto& dep : checkable->GetDependencies()) { + for (const auto& dep : checkable->GetDependencies(/* includePending = */ true)) { m_Stack.emplace_back(dep); AssertNoCycle(dep->GetParent()); m_Stack.pop_back(); diff --git a/lib/icinga/dependency.hpp b/lib/icinga/dependency.hpp index dab2bc4b8..26a7dffe0 100644 --- a/lib/icinga/dependency.hpp +++ b/lib/icinga/dependency.hpp @@ -132,7 +132,6 @@ public: using MemberValueType = std::unordered_multimap; using MembersMap = std::map; - explicit DependencyGroup(String name); DependencyGroup(String name, const std::set& dependencies); static DependencyGroup::Ptr Register(const DependencyGroup::Ptr& dependencyGroup); @@ -151,7 +150,7 @@ public: return !m_RedundancyGroupName.IsEmpty(); } - bool IsEmpty() const; + bool HasChildren() const; void AddDependency(const Dependency::Ptr& dependency); void RemoveDependency(const Dependency::Ptr& dependency); std::vector GetDependenciesForChild(const Checkable* child) const;