Merge pull request #10126 from Icinga/broken-runtime-config-sync-2.13

Fix broken runtime config sync

lib/remote/CMakeLists.txt

@@ -14,6 +14,7 @@ set(remote_SOURCES
   apilistener-authority.cpp
   apiuser.cpp apiuser.hpp apiuser-ti.hpp
   configfileshandler.cpp configfileshandler.hpp
+  configobjectslock.cpp configobjectslock.hpp
   configobjectutility.cpp configobjectutility.hpp
   configpackageshandler.cpp configpackageshandler.hpp
   configpackageutility.cpp configpackageutility.hpp

lib/remote/apilistener-configsync.cpp

@@ -8,6 +8,7 @@
 #include "base/json.hpp"
 #include "base/convert.hpp"
 #include "config/vmops.hpp"
+#include "remote/configobjectslock.hpp"
 #include <fstream>
 
 using namespace icinga;
@@ -104,6 +105,11 @@ Value ApiListener::ConfigUpdateObjectAPIHandler(const MessageOrigin::Ptr& origin
 		return Empty;
 	}
 
+	// Wait for the object name to become available for processing and block it immediately.
+	// Doing so guarantees that only one (create/update/delete) cluster event or API request of a
+	// given object is being processed at any given time.
+	ObjectNameLock objectNameLock(ptype, objName);
+
 	ConfigObject::Ptr object = ctype->GetObject(objName);
 
 	String config = params->Get("config");
@@ -258,6 +264,11 @@ Value ApiListener::ConfigDeleteObjectAPIHandler(const MessageOrigin::Ptr& origin
 		return Empty;
 	}
 
+	// Wait for the object name to become available for processing and block it immediately.
+	// Doing so guarantees that only one (create/update/delete) cluster event or API request of a
+	// given object is being processed at any given time.
+	ObjectNameLock objectNameLock(ptype, objName);
+
 	ConfigObject::Ptr object = ctype->GetObject(objName);
 
 	if (!object) {

lib/remote/configobjectslock.cpp

@@ -0,0 +1,39 @@
+/* Icinga 2 | (c) 2022 Icinga GmbH | GPLv2+ */
+
+#include "remote/configobjectslock.hpp"
+
+using namespace icinga;
+
+std::mutex ObjectNameLock::m_Mutex;
+std::condition_variable ObjectNameLock::m_CV;
+std::map<Type*, std::set<String>> ObjectNameLock::m_LockedObjectNames;
+
+/**
+ * Locks the specified object name of the given type and unlocks it upon destruction of the instance of this class.
+ *
+ * If it is already locked, the call blocks until the lock is released.
+ *
+ * @param Type::Ptr ptype The type of the object you want to lock
+ * @param String objName The object name you want to lock
+ */
+ObjectNameLock::ObjectNameLock(const Type::Ptr& ptype, const String& objName): m_ObjectName{objName}, m_Type{ptype}
+{
+	std::unique_lock<std::mutex> lock(m_Mutex);
+	m_CV.wait(lock, [this]{
+		auto& locked = m_LockedObjectNames[m_Type.get()];
+		return locked.find(m_ObjectName) == locked.end();
+	});
+
+	// Add the object name to the locked list to block all other threads that try
+	// to process a message affecting the same object.
+	m_LockedObjectNames[ptype.get()].emplace(objName);
+}
+
+ObjectNameLock::~ObjectNameLock()
+{
+	{
+		std::unique_lock<std::mutex> lock(m_Mutex);
+		m_LockedObjectNames[m_Type.get()].erase(m_ObjectName);
+	}
+	m_CV.notify_all();
+}

lib/remote/configobjectslock.hpp

@@ -0,0 +1,39 @@
+/* Icinga 2 | (c) 2023 Icinga GmbH | GPLv2+ */
+
+#pragma once
+
+#include "base/type.hpp"
+#include "base/string.hpp"
+#include <condition_variable>
+#include <map>
+#include <mutex>
+#include <set>
+
+namespace icinga
+{
+
+/**
+ * Allows you to easily lock/unlock a specific object of a given type by its name.
+ *
+ * That way, locking an object "this" of type Host does not affect an object "this" of
+ * type "Service" nor an object "other" of type "Host".
+ *
+ * @ingroup remote
+ */
+class ObjectNameLock
+{
+public:
+	ObjectNameLock(const Type::Ptr& ptype, const String& objName);
+
+	~ObjectNameLock();
+
+private:
+	String m_ObjectName;
+	Type::Ptr m_Type;
+
+	static std::mutex m_Mutex;
+	static std::condition_variable m_CV;
+	static std::map<Type*, std::set<String>> m_LockedObjectNames;
+};
+
+}

lib/remote/configobjectutility.cpp

@@ -5,7 +5,9 @@
 #include "remote/apilistener.hpp"
 #include "config/configcompiler.hpp"
 #include "config/configitem.hpp"
+#include "base/atomic-file.hpp"
 #include "base/configwriter.hpp"
+#include "base/defer.hpp"
 #include "base/exception.hpp"
 #include "base/dependencygraph.hpp"
 #include "base/tlsutility.hpp"
@@ -191,11 +193,16 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 		return false;
 	}
 
+	// AtomicFile doesn't create not yet existing directories, so we have to do it by ourselves.
 	Utility::MkDirP(Utility::DirName(path), 0700);
 
-	std::ofstream fp(path.CStr(), std::ofstream::out | std::ostream::trunc);
+	AtomicFile::Write(path, 0644, config);
-	fp << config;
+
-	fp.close();
+	// Remove the just created config file in all the error cases and if the object creation
+	// succeeds the deferred callback will be cancelled.
+	Defer removeConfigPath([&path]{
+		Utility::Remove(path);
+	});
 
 	std::unique_ptr<Expression> expr = ConfigCompiler::CompileFile(path, String(), "_api");
 
@@ -220,8 +227,6 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 				Log(LogNotice, "ConfigObjectUtility")
 					<< "Failed to commit config item '" << fullName << "'. Aborting and removing config path '" << path << "'.";
 
-				Utility::Remove(path);
-
 				for (const boost::exception_ptr& ex : upq.GetExceptions()) {
 					errors->Add(DiagnosticInformation(ex, false));
 
@@ -243,8 +248,6 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 				Log(LogNotice, "ConfigObjectUtility")
 					<< "Failed to activate config object '" << fullName << "'. Aborting and removing config path '" << path << "'.";
 
-				Utility::Remove(path);
-
 				for (const boost::exception_ptr& ex : upq.GetExceptions()) {
 					errors->Add(DiagnosticInformation(ex, false));
 
@@ -268,16 +271,16 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 		ConfigObject::Ptr obj = ctype->GetObject(fullName);
 
 		if (obj) {
+			// Object is successfully created and activated, so don't remove its config.
+			removeConfigPath.Cancel();
+
 			Log(LogInformation, "ConfigObjectUtility")
 				<< "Created and activated object '" << fullName << "' of type '" << type->GetName() << "'.";
 		} else {
 			Log(LogNotice, "ConfigObjectUtility")
 				<< "Object '" << fullName << "' was not created but ignored due to errors.";
 		}
-
 	} catch (const std::exception& ex) {
-		Utility::Remove(path);
-
 		if (errors)
 			errors->Add(DiagnosticInformation(ex, false));
 

lib/remote/createobjecthandler.cpp

@@ -6,6 +6,7 @@
 #include "remote/jsonrpcconnection.hpp"
 #include "remote/filterutility.hpp"
 #include "remote/apiaction.hpp"
+#include "remote/configobjectslock.hpp"
 #include "remote/zone.hpp"
 #include "base/configtype.hpp"
 #include <set>
@@ -116,6 +117,9 @@ bool CreateObjectHandler::HandleRequest(
 		return true;
 	}
 
+	// Lock the object name of the given type to prevent from being created concurrently.
+	ObjectNameLock objectNameLock(type, name);
+
 	if (!ConfigObjectUtility::CreateObject(type, name, config, errors, diagnosticInformation)) {
 		result1->Set("errors", errors);
 		result1->Set("code", 500);

lib/remote/deleteobjecthandler.cpp

@@ -5,6 +5,7 @@
 #include "remote/httputility.hpp"
 #include "remote/filterutility.hpp"
 #include "remote/apiaction.hpp"
+#include "remote/configobjectslock.hpp"
 #include "config/configitem.hpp"
 #include "base/exception.hpp"
 #include <boost/algorithm/string/case_conv.hpp>
@@ -76,6 +77,9 @@ bool DeleteObjectHandler::HandleRequest(
 		Array::Ptr errors = new Array();
 		Array::Ptr diagnosticInformation = new Array();
 
+		// Lock the object name of the given type to prevent from being modified/deleted concurrently.
+		ObjectNameLock objectNameLock(type, obj->GetName());
+
 		if (!ConfigObjectUtility::DeleteObject(obj, cascade, errors, diagnosticInformation)) {
 			code = 500;
 			status = "Object could not be deleted.";

lib/remote/modifyobjecthandler.cpp

@@ -4,6 +4,7 @@
 #include "remote/httputility.hpp"
 #include "remote/filterutility.hpp"
 #include "remote/apiaction.hpp"
+#include "remote/configobjectslock.hpp"
 #include "base/exception.hpp"
 #include <boost/algorithm/string/case_conv.hpp>
 #include <set>
@@ -87,6 +88,9 @@ bool ModifyObjectHandler::HandleRequest(
 
 		String key;
 
+		// Lock the object name of the given type to prevent from being modified/deleted concurrently.
+		ObjectNameLock objectNameLock(type, obj->GetName());
+
 		try {
 			if (attrs) {
 				ObjectLock olock(attrs);