IcingaDB: Fix dependency runtime deletion & creation

Author: yhabteab

lib/icinga/checkable-dependency.cpp

@@ -43,7 +43,20 @@ std::vector<DependencyGroup::Ptr> Checkable::GetDependencyGroups() const
 	return {m_DependencyGroups.begin(), m_DependencyGroups.end()};
 }
 
-void Checkable::AddDependency(const Dependency::Ptr& dependency, bool refreshGlobalRegistry)
+/**
+ * Add the provided dependency to the current Checkable list of dependencies.
+ *
+ * Returns an existing dependency group that has been modified due to the addition of the new dependency object.
+ * Meaning, either the dependency groups identity has changed after adding the new dependency to it, or the provided
+ * dependency caused the creation of a new dependency group and decoupling from of the existing one. In the latter case,
+ * the returned value is the previously existing group that the current Checkable was member of.
+ *
+ * @param dependency The dependency to add.
+ * @param refreshGlobalRegistry Whether to automatically refresh the global registry or not.
+ *
+ * @return DependencyGroup::Ptr The dependency group that has been modified.
+ */
+DependencyGroup::Ptr Checkable::AddDependency(const Dependency::Ptr& dependency, bool refreshGlobalRegistry)
 {
 	std::lock_guard lock(m_DependencyMutex);
 	DependencyGroup::Ptr newGroup(new DependencyGroup(dependency->GetRedundancyGroup(), dependency));
@@ -53,6 +66,7 @@ void Checkable::AddDependency(const Dependency::Ptr& dependency, bool refreshGlo
 		// If we're not going to refresh the global registry, we just need to add the dependency to the existing group.
 		// Meaning, the dependency group itself isn't registered globally yet, so we don't need to re-register it.
 		(*it)->AddMember(dependency);
+		return *it;
 	} else {
 		if (auto existingGroup(*it); existingGroup->HasIdenticalMember(dependency)) {
 			// There's already an identical member in the group and this is likely an exact duplicate of it,
@@ -86,11 +100,30 @@ void Checkable::AddDependency(const Dependency::Ptr& dependency, bool refreshGlo
 				}
 				m_DependencyGroups.emplace(DependencyGroup::Register(newGroup));
 			}
+
+			// In both of the above cases, the identity of the existing group is going to probably change,
+			// so we'll need to clean up all the database entries/relations referencing its old identity.
+			return existingGroup;
 		}
 	}
+
+	return nullptr;
 }
 
-void Checkable::RemoveDependency(const Dependency::Ptr& dependency)
+/**
+ * Remove the provided dependency from the current Checkable list of dependencies.
+ *
+ * Removing a dependency from the current Checkable almost always involves refreshing the global registry as well,
+ * regardless of whether it's a runtime deletion or not. However, it is a bit cheaper/easier operation compared to
+ * manually identifying and merging the dependency groups.
+ *
+ * Returns the dependency group the provided dependency object was member of / is removed from, otherwise nullptr.
+ *
+ * @param dependency The dependency to remove.
+ *
+ * @return DependencyGroup::Ptr The dependency group the provided dependency object was removed from.
+ */
+DependencyGroup::Ptr Checkable::RemoveDependency(const Dependency::Ptr& dependency)
 {
 	std::lock_guard lock(m_DependencyMutex);
 	DependencyGroup::Ptr newGroup(new DependencyGroup(dependency->GetRedundancyGroup(), dependency));
@@ -129,7 +162,10 @@ void Checkable::RemoveDependency(const Dependency::Ptr& dependency)
 		if (newGroup->HasMembers()) {
 			m_DependencyGroups.emplace(DependencyGroup::Register(newGroup));
 		}
+		return existingGroup;
 	}
+
+	return nullptr;
 }
 
 std::vector<Dependency::Ptr> Checkable::GetDependencies() const

lib/icinga/checkable.hpp

@@ -187,8 +187,8 @@ class Checkable : public ObjectImpl<Checkable>
 	/* Dependencies */
 	void PushDependencyGroupsToRegistry();
 	std::vector<intrusive_ptr<DependencyGroup>> GetDependencyGroups() const;
-	void AddDependency(const intrusive_ptr<Dependency>& dependency, bool refreshGlobalRegistry = false);
-	void RemoveDependency(const intrusive_ptr<Dependency>& dependency);
+	intrusive_ptr<DependencyGroup> AddDependency(const intrusive_ptr<Dependency>& dependency, bool refreshGlobalRegistry = false);
+	intrusive_ptr<DependencyGroup> RemoveDependency(const intrusive_ptr<Dependency>& dependency);
 	std::vector<intrusive_ptr<Dependency> > GetDependencies() const;
 	bool HasAnyDependencies() const;
 

lib/icinga/dependency-group.cpp

@@ -6,7 +6,7 @@
 
 using namespace icinga;
 
-boost::signals2::signal<void (const Dependency::Ptr&, const std::vector<DependencyGroup::Ptr>&)> DependencyGroup::OnMembersChanged;
+boost::signals2::signal<void(const DependencyGroup::Ptr&, const Dependency::Ptr&)> DependencyGroup::OnMembersChanged;
 
 std::mutex DependencyGroup::m_RegistryMutex;
 DependencyGroup::RegistryType DependencyGroup::m_Registry;

lib/icinga/dependency.cpp

@@ -205,13 +205,8 @@ void Dependency::OnAllConfigLoaded()
 void Dependency::Start(bool runtimeCreated)
 {
 	if (runtimeCreated) {
-		std::vector<DependencyGroup::Ptr> previousGroups;
-		if (!GetRedundancyGroup().IsEmpty()) {
-			previousGroups = m_Child->GetDependencyGroups();
-		}
-
 		// We need to directly refresh the global registry here only when the Checkable itself isn't created at runtime.
-		m_Child->AddDependency(this, m_Child->IsActive());
+		auto modifiedGroup(m_Child->AddDependency(this, m_Child->IsActive()));
 		m_Parent->AddReverseDependency(this);
 
 		try {
@@ -227,18 +222,7 @@ void Dependency::Start(bool runtimeCreated)
 		// isn't active yet, it means that the Checkable is also created at runtime, and we don't need
 		// to notify Icinga DB, as the Checkable's OnVersionChanged signal will do that for us.
 		if (m_Child->IsActive()) {
-			std::vector<DependencyGroup::Ptr> outdatedGroups;
-			if (!previousGroups.empty()) {
-				auto newGroups(m_Child->GetDependencyGroups());
-				std::set_difference(
-					previousGroups.begin(),
-					previousGroups.end(),
-					newGroups.begin(),
-					newGroups.end(),
-					std::back_inserter(outdatedGroups)
-				);
-			}
-			DependencyGroup::OnMembersChanged(this, outdatedGroups);
+			DependencyGroup::OnMembersChanged(modifiedGroup, this);
 		}
 	}
 
@@ -249,36 +233,11 @@ void Dependency::Stop(bool runtimeRemoved)
 {
 	ObjectImpl<Dependency>::Stop(runtimeRemoved);
 
-	std::vector<DependencyGroup::Ptr> previousGroups;
-	std::set<DependencyGroup::Ptr> outdatedGroups;
-	if (runtimeRemoved && !GetRedundancyGroup().IsEmpty()) {
-		previousGroups = m_Child->GetDependencyGroups();
-		// Find the dependency group that this dependency is a member of, and add it to the outdated groups list.
-		// Otherwise, once the dependency is unregistered, we won't be able to identify the group that this dependency
-		// was member of anymore.
-		for (auto& dependencyGroup : previousGroups) {
-			if (dependencyGroup->HasIdenticalMember(this)) {
-				outdatedGroups.emplace(dependencyGroup);
-				break;
-			}
-		}
-	}
-
-	m_Child->RemoveDependency(this);
+	auto modifiedGroup(m_Child->RemoveDependency(this));
 	GetParent()->RemoveReverseDependency(this);
 
 	if (runtimeRemoved) {
-		if (!previousGroups.empty()) {
-			auto newGroups(m_Child->GetDependencyGroups());
-			std::set_difference(
-				previousGroups.begin(),
-				previousGroups.end(),
-				newGroups.begin(),
-				newGroups.end(),
-				std::inserter(outdatedGroups, outdatedGroups.end())
-			);
-		}
-		DependencyGroup::OnMembersChanged(this, {outdatedGroups.begin(), outdatedGroups.end()});
+		DependencyGroup::OnMembersChanged(modifiedGroup, this);
 	}
 }
 

lib/icinga/dependency.hpp

@@ -190,7 +190,7 @@ class DependencyGroup final : public SharedObject
 
 	State GetState(DependencyType dt = DependencyState, int rstack = 0) const;
 
-	static boost::signals2::signal<void (const Dependency::Ptr&, const std::vector<DependencyGroup::Ptr>&)> OnMembersChanged;
+	static boost::signals2::signal<void(const DependencyGroup::Ptr&, const Dependency::Ptr&)> OnMembersChanged;
 
 private:
 	void MoveMembersTo(const DependencyGroup::Ptr& other);

lib/icingadb/icingadb-objects.cpp

@@ -133,7 +133,7 @@ void IcingaDB::ConfigStaticInitialize()
 		IcingaDB::NextCheckUpdatedHandler(checkable);
 	});
 
-	DependencyGroup::OnMembersChanged.connect(&IcingaDB::DependencyGroupsChangedHandler);
+	DependencyGroup::OnMembersChanged.connect(&IcingaDB::DependencyGroupChangedHandler);
 
 	Service::OnHostProblemChanged.connect([](const Service::Ptr& service, const CheckResult::Ptr&, const MessageOrigin::Ptr&) {
 		IcingaDB::HostProblemChangedHandler(service);
@@ -2803,59 +2803,69 @@ void IcingaDB::SendCustomVarsChanged(const ConfigObject::Ptr& object, const Dict
 	}
 }
 
-void IcingaDB::SendDependencyGroupsChanged(const Dependency::Ptr& dep, const std::vector<DependencyGroup::Ptr>& outdatedGroups)
+void IcingaDB::SendDependencyGroupChanged(const DependencyGroup::Ptr& modifiedGroup, const Dependency::Ptr& member)
 {
 	if (!m_Rcon || !m_Rcon->IsConnected()) {
 		return;
 	}
 
-	auto parent(dep->GetParent());
-	auto child(dep->GetChild());
+	auto child(member->GetChild());
 	if (!child->HasAnyDependencies()) {
 		// If the child Checkable has no parent and reverse dependencies, we can safely remove the dependency node.
 		DeleteRelationship(GetObjectIdentifier(child), "dependency:node");
 	}
 
 	RedisConnection::Queries hdels, xAdds;
-	auto deleteState([this, &hdels, &xAdds](const String& redisKey, const String& id) {
+	auto deleteState([this, &hdels, &xAdds](const String& id, const String& redisKey) {
 		hdels.emplace_back(RedisConnection::Query{"HDEL", m_PrefixConfigObject + redisKey, id});
 		xAdds.emplace_back(RedisConnection::Query{
 			"XADD", "icinga:runtime:state", "MAXLEN", "~", "1000000", "*", "runtime_type", "delete",
 			"redis_key", m_PrefixConfigObject + redisKey, "id", id
 		});
 	});
 
-	if (dep->GetRedundancyGroup().IsEmpty() && !dep->IsActive() && dep->GetExtension("ConfigObjectDeleted")) {
-		auto identifier(HashValue(new Array{GetObjectIdentifier(child), GetObjectIdentifier(parent)}));
+	auto parent(member->GetParent());
+	if (!modifiedGroup->IsRedundancyGroup() && !member->IsActive() && member->GetExtension("ConfigObjectDeleted")) {
+		auto edgeId(HashValue(new Array{GetObjectIdentifier(child), GetObjectIdentifier(parent)}));
 		// Remove the edge between the parent and child Checkable linked through the removed dependency.
-		DeleteRelationship(identifier, "dependency:edge");
-		// The dependency object is going to be deleted, so we need to remove its state from Redis as well.
-		deleteState("dependency:edge:state", identifier);
-	}
+		DeleteRelationship(edgeId, "dependency:edge");
+
+		if (!modifiedGroup->HasMembers() || !modifiedGroup->HasIdenticalMember(member)) {
+			DependencyGroup::Ptr dummyGroup(new DependencyGroup("", member));
+			// In order to recreate the edge state ID, we need to copy the members of the modified group
+			// to a dummy one and use its composite key to generate its previous edge state ID.
+			for (auto dependency : modifiedGroup->GetMembers(child.get())) {
+				dummyGroup->AddMember(dependency);
+			}
 
-	auto newGroups(child->GetDependencyGroups());
-	for (auto& group : outdatedGroups) {
-		String redundancyGroupId(group->GetIcingaDBIdentifier());
-		bool sameCompositeKey(redundancyGroupId == HashValue(new Array{m_EnvironmentId, group->GetCompositeKey()}));
-		if (!sameCompositeKey || !group->HasMembers() || std::find(newGroups.begin(), newGroups.end(), group) == newGroups.end()) {
+			deleteState(
+				// Keep this edge state ID generation in sync with the one in SerializeDependencyEdgeState.
+				HashValue(new Array{dummyGroup->GetCompositeKey(), GetObjectIdentifier(parent)}),
+				"dependency:edge:state"
+			);
+		}
+	} else if (modifiedGroup->IsRedundancyGroup()) {
+		auto newGroups(child->GetDependencyGroups());
+		String redundancyGroupId(modifiedGroup->GetIcingaDBIdentifier());
+		bool sameCompositeKey(redundancyGroupId == HashValue(new Array{m_EnvironmentId, modifiedGroup->GetCompositeKey()}));
+		if (!sameCompositeKey || !modifiedGroup->HasMembers() || std::find(newGroups.begin(), newGroups.end(), modifiedGroup) == newGroups.end()) {
 			// Remove the connection from the child Checkable to the redundancy group.
 			DeleteRelationship(HashValue(new Array{GetObjectIdentifier(child), redundancyGroupId}), "dependency:edge");
 
-			if (!sameCompositeKey || !group->HasMembers()) {
-				auto members(group->GetMembers(child.get()));
-				members.emplace_back(dep);
-				for (auto& member : members) {
+			if (!sameCompositeKey || !modifiedGroup->HasMembers()) {
+				auto members(modifiedGroup->GetMembers(child.get()));
+				members.emplace_back(member);
+				for (auto& dependency : members) {
 					// Remove the connection from the redundancy group to the parent Checkable of the removed
 					// dependency, if there's no other member in the redundancy group that references that very
-					// same parent Checkable or the dependency Group's Icinga DB identifier has changed.
-					auto groupParentId(HashValue(new Array{redundancyGroupId, GetObjectIdentifier(member->GetParent())}));
-					DeleteRelationship(groupParentId, "dependency:edge");
-					deleteState("dependency:edge:state", groupParentId);
+					// same parent Checkable or the redundancy group's Icinga DB identifier has changed.
+					auto id(HashValue(new Array{redundancyGroupId, GetObjectIdentifier(dependency->GetParent())}));
+					DeleteRelationship(id, "dependency:edge");
+					deleteState(id, "dependency:edge:state");
 				}
 
-				// Remove the group entirely as it either has no members left or it's Icinga DB identifier has changed.
-				deleteState("dependency:edge:state", redundancyGroupId);
-				deleteState("redundancygroup:state", redundancyGroupId);
+				deleteState(redundancyGroupId, "dependency:edge:state" );
+				deleteState(redundancyGroupId, "redundancygroup:state");
 				DeleteRelationship(redundancyGroupId, "dependency:node");
 				DeleteRelationship(redundancyGroupId, "redundancygroup");
 			}
@@ -2865,16 +2875,16 @@ void IcingaDB::SendDependencyGroupsChanged(const Dependency::Ptr& dep, const std
 	if (!hdels.empty()) {
 		m_Rcon->FireAndForgetQueries(std::move(hdels), Prio::RuntimeStateSync);
 		m_Rcon->FireAndForgetQueries(std::move(xAdds), Prio::RuntimeStateStream, {0, 1});
+    }
 
-		if (!newGroups.empty()) {
-			std::map<String, RedisConnection::Query> hMSets;
-			std::vector<Dictionary::Ptr> runtimeUpdates;
-			InsertCheckableDependencies(child, hMSets, &runtimeUpdates);
+	if (child->HasAnyDependencies()) {
+		std::map<String, RedisConnection::Query> hMSets;
+		std::vector<Dictionary::Ptr> runtimeUpdates;
+		InsertCheckableDependencies(child, hMSets, &runtimeUpdates);
+		ExecuteRedisTransaction(hMSets, runtimeUpdates);
 
-			ExecuteRedisTransaction(hMSets, runtimeUpdates);
-			UpdateState(child, StateUpdate::Full);
-		}
-    }
+		UpdateState(child, StateUpdate::Full);
+	}
 
 	// The affect{ed,s}_children might now have different outcome, so we need to update the parent Checkable as well.
 	SendConfigUpdate(parent, true);
@@ -3163,10 +3173,10 @@ void IcingaDB::NextCheckUpdatedHandler(const Checkable::Ptr& checkable)
 	}
 }
 
-void IcingaDB::DependencyGroupsChangedHandler(const Dependency::Ptr& dep, const std::vector<DependencyGroup::Ptr>& outdatedGroups)
+void IcingaDB::DependencyGroupChangedHandler(const DependencyGroup::Ptr& modifiedGroup, const Dependency::Ptr& member)
 {
 	for (auto& rw : ConfigType::GetObjectsByType<IcingaDB>()) {
-		rw->SendDependencyGroupsChanged(dep, outdatedGroups);
+		rw->SendDependencyGroupChanged(modifiedGroup, member);
 	}
 }
 

lib/icingadb/icingadb.hpp

@@ -140,7 +140,7 @@ class IcingaDB : public ObjectImpl<IcingaDB>
 	void SendCommandEnvChanged(const ConfigObject::Ptr& command, const Dictionary::Ptr& oldValues, const Dictionary::Ptr& newValues);
 	void SendCommandArgumentsChanged(const ConfigObject::Ptr& command, const Dictionary::Ptr& oldValues, const Dictionary::Ptr& newValues);
 	void SendCustomVarsChanged(const ConfigObject::Ptr& object, const Dictionary::Ptr& oldValues, const Dictionary::Ptr& newValues);
-	void SendDependencyGroupsChanged(const Dependency::Ptr& dep, const std::vector<DependencyGroup::Ptr>& outdatedGroups);
+	void SendDependencyGroupChanged(const DependencyGroup::Ptr& modifiedGroup, const Dependency::Ptr& member);
 
 	void ForwardHistoryEntries();
 
@@ -187,7 +187,7 @@ class IcingaDB : public ObjectImpl<IcingaDB>
 	static void FlappingChangeHandler(const Checkable::Ptr& checkable, double changeTime);
 	static void NewCheckResultHandler(const Checkable::Ptr& checkable);
 	static void NextCheckUpdatedHandler(const Checkable::Ptr& checkable);
-	static void DependencyGroupsChangedHandler(const Dependency::Ptr& dep, const std::vector<DependencyGroup::Ptr>& outdatedGroups);
+	static void DependencyGroupChangedHandler(const DependencyGroup::Ptr& modifiedGroup, const Dependency::Ptr& member);
 	static void HostProblemChangedHandler(const Service::Ptr& service);
 	static void AcknowledgementSetHandler(const Checkable::Ptr& checkable, const String& author, const String& comment, AcknowledgementType type, bool persistent, double changeTime, double expiry);
 	static void AcknowledgementClearedHandler(const Checkable::Ptr& checkable, const String& removedBy, double changeTime);