From 015374e69db2a8d37d3c8f88a5c871329e1b81e8 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Wed, 4 Dec 2024 08:03:01 +0100 Subject: [PATCH 1/3] DependencyGraph: Allow lookups by parent & child dependencies --- lib/base/dependencygraph.cpp | 62 ++++++++++++++++++++--------- lib/base/dependencygraph.hpp | 75 +++++++++++++++++++++++++++++++++++- 2 files changed, 116 insertions(+), 21 deletions(-) diff --git a/lib/base/dependencygraph.cpp b/lib/base/dependencygraph.cpp index 58f685ce01..a9d0dbb95c 100644 --- a/lib/base/dependencygraph.cpp +++ b/lib/base/dependencygraph.cpp @@ -5,45 +5,69 @@ using namespace icinga; std::mutex DependencyGraph::m_Mutex; -std::map> DependencyGraph::m_Dependencies; +DependencyGraph::DependencyMap DependencyGraph::m_Dependencies; void DependencyGraph::AddDependency(ConfigObject* child, ConfigObject* parent) { std::unique_lock lock(m_Mutex); - m_Dependencies[parent][child]++; + if (auto [it, inserted] = m_Dependencies.insert(Edge(parent, child)); !inserted) { + m_Dependencies.modify(it, [](Edge& e) { e.count++; }); + } } void DependencyGraph::RemoveDependency(ConfigObject* child, ConfigObject* parent) { std::unique_lock lock(m_Mutex); - auto& refs = m_Dependencies[parent]; - auto it = refs.find(child); + if (auto it(m_Dependencies.find(Edge(parent, child))); it != m_Dependencies.end()) { + // A number <= 1 means, this isn't referenced by anyone and should be erased from the container. + if (it->count == 1) { + m_Dependencies.erase(it); + return; + } - if (it == refs.end()) - return; + // Otherwise, each remove operation will should decrement this by 1 till it reaches <= 1 + // and causes the edge to completely be erased from the container. + m_Dependencies.modify(it, [](Edge& e) { e.count--; }); + } +} - it->second--; +/** + * Returns all the parent objects of the given child object. + * + * @param child The child object. + * + * @returns A list of the parent objects. + */ +std::vector DependencyGraph::GetParents(const ConfigObject::Ptr& child) +{ + std::vector objects; - if (it->second == 0) - refs.erase(it); + std::unique_lock lock(m_Mutex); + auto [begin, end] = m_Dependencies.get<2>().equal_range(child.get()); + std::transform(begin, end, std::back_inserter(objects), [](const Edge& edge) { + return edge.parent; + }); - if (refs.empty()) - m_Dependencies.erase(parent); + return objects; } +/** + * Returns all the dependent objects of the given parent object. + * + * @param parent The parent object. + * + * @returns A list of the dependent objects. + */ std::vector DependencyGraph::GetChildren(const ConfigObject::Ptr& parent) { std::vector objects; - std::unique_lock lock(m_Mutex); - auto it = m_Dependencies.find(parent.get()); - - if (it != m_Dependencies.end()) { - for (auto& kv : it->second) { - objects.emplace_back(kv.first); - } - } + std::unique_lock lock(m_Mutex); + auto [begin, end] = m_Dependencies.get<1>().equal_range(parent.get()); + std::transform(begin, end, std::back_inserter(objects), [](const Edge& edge) { + return edge.child; + }); return objects; } diff --git a/lib/base/dependencygraph.hpp b/lib/base/dependencygraph.hpp index 6afb08fc59..ef1a2a4a61 100644 --- a/lib/base/dependencygraph.hpp +++ b/lib/base/dependencygraph.hpp @@ -5,7 +5,9 @@ #include "base/i2-base.hpp" #include "base/configobject.hpp" -#include +#include +#include +#include #include namespace icinga { @@ -20,13 +22,82 @@ class DependencyGraph public: static void AddDependency(ConfigObject* child, ConfigObject* parent); static void RemoveDependency(ConfigObject* child, ConfigObject* parent); + static std::vector GetParents(const ConfigObject::Ptr& child); static std::vector GetChildren(const ConfigObject::Ptr& parent); private: DependencyGraph(); + /** + * Represents an undirected dependency edge between two objects. + * + * It allows to traverse the graph in both directions, i.e. from parent to child and vice versa. + */ + struct Edge + { + ConfigObject* parent; // The parent object of the child one. + ConfigObject* child; // The dependent object of the parent. + // Counter for the number of parent <-> child edges to allow duplicates. + int count; + + Edge(ConfigObject* parent, ConfigObject* child, int count = 1): parent(parent), child(child), count(count) + { + } + + struct Hash + { + /** + * Generates a unique hash of the given Edge object. + * + * Note, the hash value is generated only by combining the hash values of the parent and child pointers. + * + * @param edge The Edge object to be hashed. + * + * @return size_t The resulting hash value of the given object. + */ + size_t operator()(const Edge& edge) const + { + size_t seed = 0; + boost::hash_combine(seed, edge.parent); + boost::hash_combine(seed, edge.child); + + return seed; + } + }; + + struct Equal + { + /** + * Compares whether the two Edge objects contain the same parent and child pointers. + * + * Note, the member property count is not taken into account for equality checks. + * + * @param a The first Edge object to compare. + * @param b The second Edge object to compare. + * + * @return bool Returns true if the two objects are equal, false otherwise. + */ + bool operator()(const Edge& a, const Edge& b) const + { + return a.parent == b.parent && a.child == b.child; + } + }; + }; + + using DependencyMap = boost::multi_index_container< + Edge, // The value type we want to sore in the container. + boost::multi_index::indexed_by< + // The first indexer is used for lookups by the Edge from child to parent, thus it + // needs its own hash function and comparison predicate. + boost::multi_index::hashed_unique, Edge::Hash, Edge::Equal>, + // These two indexers are used for lookups by the parent and child pointers. + boost::multi_index::hashed_non_unique>, + boost::multi_index::hashed_non_unique> + > + >; + static std::mutex m_Mutex; - static std::map> m_Dependencies; + static DependencyMap m_Dependencies; }; } From ff0e12e6ac71ffd8f93db7330551e386c0c7a9d7 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 16 Sep 2024 09:20:43 +0200 Subject: [PATCH 2/3] ApiListener: Sync runtime configs in order --- lib/remote/apilistener-configsync.cpp | 60 +++++++++++++++++++++------ lib/remote/apilistener.hpp | 2 + 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/lib/remote/apilistener-configsync.cpp b/lib/remote/apilistener-configsync.cpp index 04436ad8b9..6d97258ec5 100644 --- a/lib/remote/apilistener-configsync.cpp +++ b/lib/remote/apilistener-configsync.cpp @@ -5,11 +5,13 @@ #include "remote/configobjectutility.hpp" #include "remote/jsonrpc.hpp" #include "base/configtype.hpp" -#include "base/json.hpp" #include "base/convert.hpp" +#include "base/dependencygraph.hpp" +#include "base/json.hpp" #include "config/vmops.hpp" #include "remote/configobjectslock.hpp" #include +#include using namespace icinga; @@ -393,6 +395,40 @@ void ApiListener::UpdateConfigObject(const ConfigObject::Ptr& object, const Mess } } +/** + * Syncs the specified object and its direct and indirect parents to the provided client + * in topological order of their dependency graph recursively. + * + * Objects that the client does not have access to are skipped without going through their dependency graph. + * + * Please do not use this method to forward remote generated cluster updates; it should only be used to + * send local updates to that specific non-nullptr client. + * + * @param object The config object you want to sync. + * @param azone The zone of the client you want to send the update to. + * @param client The JsonRpc client you send the update to. + * @param syncedObjects Used to cache the already synced objects. + */ +void ApiListener::UpdateConfigObjectWithParents(const ConfigObject::Ptr& object, const Zone::Ptr& azone, + const JsonRpcConnection::Ptr& client, std::unordered_set& syncedObjects) +{ + if (syncedObjects.find(object.get()) != syncedObjects.end()) { + return; + } + + /* don't sync objects for non-matching parent-child zones */ + if (!azone->CanAccessObject(object)) { + return; + } + syncedObjects.emplace(object.get()); + + for (const auto& parent : DependencyGraph::GetParents(object)) { + UpdateConfigObjectWithParents(parent, azone, client, syncedObjects); + } + + /* send the config object to the connected client */ + UpdateConfigObject(object, nullptr, client); +} void ApiListener::DeleteConfigObject(const ConfigObject::Ptr& object, const MessageOrigin::Ptr& origin, const JsonRpcConnection::Ptr& client) @@ -454,19 +490,17 @@ void ApiListener::SendRuntimeConfigObjects(const JsonRpcConnection::Ptr& aclient Log(LogInformation, "ApiListener") << "Syncing runtime objects to endpoint '" << endpoint->GetName() << "'."; + std::unordered_set syncedObjects; for (const Type::Ptr& type : Type::GetAllTypes()) { - auto *dtype = dynamic_cast(type.get()); - - if (!dtype) - continue; - - for (const ConfigObject::Ptr& object : dtype->GetObjects()) { - /* don't sync objects for non-matching parent-child zones */ - if (!azone->CanAccessObject(object)) - continue; - - /* send the config object to the connected client */ - UpdateConfigObject(object, nullptr, aclient); + if (auto *ctype = dynamic_cast(type.get())) { + for (const auto& object : ctype->GetObjects()) { + // All objects must be synced sorted by their dependency graph. + // Otherwise, downtimes/comments etc. might get synced before their respective Checkables, which will + // result in comments and downtimes being ignored by the other endpoint since it does not yet know + // about their checkables. Given that the runtime config updates event does not trigger a reload on the + // remote endpoint, these objects won't be synced again until the next reload. + UpdateConfigObjectWithParents(object, azone, aclient, syncedObjects); + } } } diff --git a/lib/remote/apilistener.hpp b/lib/remote/apilistener.hpp index fced0a8afb..eae1fa03e6 100644 --- a/lib/remote/apilistener.hpp +++ b/lib/remote/apilistener.hpp @@ -247,6 +247,8 @@ class ApiListener final : public ObjectImpl /* configsync */ void UpdateConfigObject(const ConfigObject::Ptr& object, const MessageOrigin::Ptr& origin, const JsonRpcConnection::Ptr& client = nullptr); + void UpdateConfigObjectWithParents(const ConfigObject::Ptr& object, const Zone::Ptr& azone, + const JsonRpcConnection::Ptr& client, std::unordered_set& syncedObjects); void DeleteConfigObject(const ConfigObject::Ptr& object, const MessageOrigin::Ptr& origin, const JsonRpcConnection::Ptr& client = nullptr); void SendRuntimeConfigObjects(const JsonRpcConnection::Ptr& aclient); From cf125dd8d5271951f32b327ef1dd383df01fd70c Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Thu, 19 Dec 2024 12:24:26 +0100 Subject: [PATCH 3/3] Simplify `DependencyGraph:RemoveDependency()` method --- lib/base/dependencygraph.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/base/dependencygraph.cpp b/lib/base/dependencygraph.cpp index a9d0dbb95c..06446cfc28 100644 --- a/lib/base/dependencygraph.cpp +++ b/lib/base/dependencygraph.cpp @@ -20,15 +20,14 @@ void DependencyGraph::RemoveDependency(ConfigObject* child, ConfigObject* parent std::unique_lock lock(m_Mutex); if (auto it(m_Dependencies.find(Edge(parent, child))); it != m_Dependencies.end()) { - // A number <= 1 means, this isn't referenced by anyone and should be erased from the container. - if (it->count == 1) { + if (it->count > 1) { + // Remove a duplicate edge from child to node, i.e. decrement the corresponding counter. + m_Dependencies.modify(it, [](Edge& e) { e.count--; }); + } else { + // Remove the last edge from child to node (decrementing the counter would set it to 0), + // thus remove that connection from the data structure completely. m_Dependencies.erase(it); - return; } - - // Otherwise, each remove operation will should decrement this by 1 till it reaches <= 1 - // and causes the edge to completely be erased from the container. - m_Dependencies.modify(it, [](Edge& e) { e.count--; }); } }