Merge pull request #10292 from Icinga/rpc-sync-failures

Runtime RPC sync failures

lib/base/dependencygraph.cpp

@@ -5,46 +5,68 @@
 using namespace icinga;
 
 std::mutex DependencyGraph::m_Mutex;
-std::map<Object *, std::map<Object *, int> > DependencyGraph::m_Dependencies;
+DependencyGraph::DependencyMap DependencyGraph::m_Dependencies;
 
-void DependencyGraph::AddDependency(Object *parent, Object *child)
+void DependencyGraph::AddDependency(ConfigObject* child, ConfigObject* parent)
 {
 	std::unique_lock<std::mutex> lock(m_Mutex);
-	m_Dependencies[child][parent]++;
+	if (auto [it, inserted] = m_Dependencies.insert(Edge(parent, child)); !inserted) {
+		m_Dependencies.modify(it, [](Edge& e) { e.count++; });
+	}
 }
 
-void DependencyGraph::RemoveDependency(Object *parent, Object *child)
+void DependencyGraph::RemoveDependency(ConfigObject* child, ConfigObject* parent)
 {
 	std::unique_lock<std::mutex> lock(m_Mutex);
 
-	auto& refs = m_Dependencies[child];
+	if (auto it(m_Dependencies.find(Edge(parent, child))); it != m_Dependencies.end()) {
-	auto it = refs.find(parent);
+		if (it->count > 1) {
-
+			// Remove a duplicate edge from child to node, i.e. decrement the corresponding counter.
-	if (it == refs.end())
+			m_Dependencies.modify(it, [](Edge& e) { e.count--; });
-		return;
+		} else {
-
+			// Remove the last edge from child to node (decrementing the counter would set it to 0),
-	it->second--;
+			// thus remove that connection from the data structure completely.
-
+			m_Dependencies.erase(it);
-	if (it->second == 0)
+		}
-		refs.erase(it);
+	}
-
-	if (refs.empty())
-		m_Dependencies.erase(child);
 }
 
-std::vector<Object::Ptr> DependencyGraph::GetParents(const Object::Ptr& child)
+/**
+ * Returns all the parent objects of the given child object.
+ *
+ * @param child The child object.
+ *
+ * @returns A list of the parent objects.
+ */
+std::vector<ConfigObject::Ptr> DependencyGraph::GetParents(const ConfigObject::Ptr& child)
 {
-	std::vector<Object::Ptr> objects;
+	std::vector<ConfigObject::Ptr> objects;
 
-	std::unique_lock<std::mutex> lock(m_Mutex);
+	std::unique_lock lock(m_Mutex);
-	auto it = m_Dependencies.find(child.get());
+	auto [begin, end] = m_Dependencies.get<2>().equal_range(child.get());
-
+	std::transform(begin, end, std::back_inserter(objects), [](const Edge& edge) {
-	if (it != m_Dependencies.end()) {
+		return edge.parent;
-		typedef std::pair<Object *, int> kv_pair;
+	});
-		for (const kv_pair& kv : it->second) {
+
-			objects.emplace_back(kv.first);
+	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<ConfigObject::Ptr> DependencyGraph::GetChildren(const ConfigObject::Ptr& parent)
+{
+	std::vector<ConfigObject::Ptr> objects;
+
+	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;
 }

lib/base/dependencygraph.hpp

@@ -4,8 +4,10 @@
 #define DEPENDENCYGRAPH_H
 
 #include "base/i2-base.hpp"
-#include "base/object.hpp"
+#include "base/configobject.hpp"
-#include <map>
+#include <boost/multi_index_container.hpp>
+#include <boost/multi_index/hashed_index.hpp>
+#include <boost/multi_index/member.hpp>
 #include <mutex>
 
 namespace icinga {
@@ -18,15 +20,84 @@ namespace icinga {
 class DependencyGraph
 {
 public:
-	static void AddDependency(Object *parent, Object *child);
+	static void AddDependency(ConfigObject* child, ConfigObject* parent);
-	static void RemoveDependency(Object *parent, Object *child);
+	static void RemoveDependency(ConfigObject* child, ConfigObject* parent);
-	static std::vector<Object::Ptr> GetParents(const Object::Ptr& child);
+	static std::vector<ConfigObject::Ptr> GetParents(const ConfigObject::Ptr& child);
+	static std::vector<ConfigObject::Ptr> 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<boost::multi_index::identity<Edge>, 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::member<Edge, ConfigObject*, &Edge::parent>>,
+			boost::multi_index::hashed_non_unique<boost::multi_index::member<Edge, ConfigObject*, &Edge::child>>
+		>
+	>;
+
 	static std::mutex m_Mutex;
-	static std::map<Object *, std::map<Object *, int> > m_Dependencies;
+	static DependencyMap m_Dependencies;
 };
 
 }

lib/base/scriptutils.cpp

@@ -520,7 +520,7 @@ String ScriptUtils::MsiGetComponentPathShim(const String& component)
 
 Array::Ptr ScriptUtils::TrackParents(const Object::Ptr& child)
 {
-	return Array::FromVector(DependencyGraph::GetParents(child));
+	return Array::FromVector(DependencyGraph::GetChildren(dynamic_pointer_cast<ConfigObject>(child)));
 }
 
 double ScriptUtils::Ptr(const Object::Ptr& object)

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 <fstream>
+#include <unordered_set>
 
 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<ConfigObject*>& 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<ConfigObject*> syncedObjects;
 	for (const Type::Ptr& type : Type::GetAllTypes()) {
-		auto *dtype = dynamic_cast<ConfigType *>(type.get());
+		if (auto *ctype = dynamic_cast<ConfigType *>(type.get())) {
-
+			for (const auto& object : ctype->GetObjects()) {
-		if (!dtype)
+				// All objects must be synced sorted by their dependency graph.
-			continue;
+				// 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
-		for (const ConfigObject::Ptr& object : dtype->GetObjects()) {
+				// about their checkables. Given that the runtime config updates event does not trigger a reload on the
-			/* don't sync objects for non-matching parent-child zones */
+				// remote endpoint, these objects won't be synced again until the next reload.
-			if (!azone->CanAccessObject(object))
+				UpdateConfigObjectWithParents(object, azone, aclient, syncedObjects);
-				continue;
+			}
-
-			/* send the config object to the connected client */
-			UpdateConfigObject(object, nullptr, aclient);
 		}
 	}
 

lib/remote/apilistener.hpp

@@ -247,6 +247,8 @@ private:
 	/* 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<ConfigObject*>& syncedObjects);
 	void DeleteConfigObject(const ConfigObject::Ptr& object, const MessageOrigin::Ptr& origin,
 		const JsonRpcConnection::Ptr& client = nullptr);
 	void SendRuntimeConfigObjects(const JsonRpcConnection::Ptr& aclient);

lib/remote/configobjectutility.cpp

@@ -303,7 +303,7 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 bool ConfigObjectUtility::DeleteObjectHelper(const ConfigObject::Ptr& object, bool cascade,
 	const Array::Ptr& errors, const Array::Ptr& diagnosticInformation, const Value& cookie)
 {
-	std::vector<Object::Ptr> parents = DependencyGraph::GetParents(object);
+	auto parents (DependencyGraph::GetChildren(object));
 
 	Type::Ptr type = object->GetReflectionType();
 
@@ -319,12 +319,7 @@ bool ConfigObjectUtility::DeleteObjectHelper(const ConfigObject::Ptr& object, bo
 		return false;
 	}
 
-	for (const Object::Ptr& pobj : parents) {
+	for (auto& parentObj : parents) {
-		ConfigObject::Ptr parentObj = dynamic_pointer_cast<ConfigObject>(pobj);
-
-		if (!parentObj)
-			continue;
-
 		DeleteObjectHelper(parentObj, cascade, errors, diagnosticInformation, cookie);
 	}
 

lib/remote/objectqueryhandler.cpp

@@ -208,13 +208,7 @@ bool ObjectQueryHandler::HandleRequest(
 					Array::Ptr used_by = new Array();
 					metaAttrs.emplace_back("used_by", used_by);
 
-					for (const Object::Ptr& pobj : DependencyGraph::GetParents((obj)))
+					for (auto& configObj : DependencyGraph::GetChildren(obj)) {
-					{
-						ConfigObject::Ptr configObj = dynamic_pointer_cast<ConfigObject>(pobj);
-
-						if (!configObj)
-							continue;
-
 						used_by->Add(new Dictionary({
 							{ "type", configObj->GetReflectionType()->GetName() },
 							{ "name", configObj->GetName() }

tools/mkclass/classcompiler.cpp

@@ -894,13 +894,13 @@ void ClassCompiler::HandleClass(const Klass& klass, const ClassDebugInfo&)
 					m_Impl << "\t" << "if (oldValue) {" << std::endl
 						<< "\t\t" << "ObjectLock olock(oldValue);" << std::endl
 						<< "\t\t" << "for (const String& ref : oldValue) {" << std::endl
-						<< "\t\t\t" << "DependencyGraph::RemoveDependency(this, ConfigObject::GetObject";
+						<< "\t\t\tDependencyGraph::RemoveDependency(";
 
 					/* Ew */
 					if (field.Type.TypeName == "Zone" && m_Library == "base")
-						m_Impl << "(\"Zone\", ";
+						m_Impl << "dynamic_cast<ConfigObject*>(this), ConfigObject::GetObject(\"Zone\", ";
 					else
-						m_Impl << "<" << field.Type.TypeName << ">(";
+						m_Impl << "this, ConfigObject::GetObject<" << field.Type.TypeName << ">(";
 
 					m_Impl << "ref).get());" << std::endl
 						<< "\t\t" << "}" << std::endl
@@ -908,36 +908,36 @@ void ClassCompiler::HandleClass(const Klass& klass, const ClassDebugInfo&)
 						<< "\t" << "if (newValue) {" << std::endl
 						<< "\t\t" << "ObjectLock olock(newValue);" << std::endl
 						<< "\t\t" << "for (const String& ref : newValue) {" << std::endl
-						<< "\t\t\t" << "DependencyGraph::AddDependency(this, ConfigObject::GetObject";
+						<< "\t\t\tDependencyGraph::AddDependency(";
 
 					/* Ew */
 					if (field.Type.TypeName == "Zone" && m_Library == "base")
-						m_Impl << "(\"Zone\", ";
+						m_Impl << "dynamic_cast<ConfigObject*>(this), ConfigObject::GetObject(\"Zone\", ";
 					else
-						m_Impl << "<" << field.Type.TypeName << ">(";
+						m_Impl << "this, ConfigObject::GetObject<" << field.Type.TypeName << ">(";
 
 					m_Impl << "ref).get());" << std::endl
 						<< "\t\t" << "}" << std::endl
 						<< "\t" << "}" << std::endl;
 				} else {
 					m_Impl << "\t" << "if (!oldValue.IsEmpty())" << std::endl
-						<< "\t\t" << "DependencyGraph::RemoveDependency(this, ConfigObject::GetObject";
+						<< "\t\tDependencyGraph::RemoveDependency(";
 
 					/* Ew */
 					if (field.Type.TypeName == "Zone" && m_Library == "base")
-						m_Impl << "(\"Zone\", ";
+						m_Impl << "dynamic_cast<ConfigObject*>(this), ConfigObject::GetObject(\"Zone\", ";
 					else
-						m_Impl << "<" << field.Type.TypeName << ">(";
+						m_Impl << "this, ConfigObject::GetObject<" << field.Type.TypeName << ">(";
 
 					m_Impl << "oldValue).get());" << std::endl
 						<< "\t" << "if (!newValue.IsEmpty())" << std::endl
-						<< "\t\t" << "DependencyGraph::AddDependency(this, ConfigObject::GetObject";
+						<< "\t\tDependencyGraph::AddDependency(";
 
 					/* Ew */
 					if (field.Type.TypeName == "Zone" && m_Library == "base")
-						m_Impl << "(\"Zone\", ";
+						m_Impl << "dynamic_cast<ConfigObject*>(this), ConfigObject::GetObject(\"Zone\", ";
 					else
-						m_Impl << "<" << field.Type.TypeName << ">(";
+						m_Impl << "this, ConfigObject::GetObject<" << field.Type.TypeName << ">(";
 
 					m_Impl << "newValue).get());" << std::endl;
 				}