From 2218ebd6b0a1c16fe8a578cbefe60745d0300b83 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Fri, 1 Mar 2024 10:11:17 +0100 Subject: [PATCH 1/5] `ConfigObjectUtility`: Use `AtomicFile` to store object config files --- lib/remote/configobjectutility.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/remote/configobjectutility.cpp b/lib/remote/configobjectutility.cpp index 62c910b41f4..9b5cf88b5ac 100644 --- a/lib/remote/configobjectutility.cpp +++ b/lib/remote/configobjectutility.cpp @@ -5,6 +5,7 @@ #include "remote/apilistener.hpp" #include "config/configcompiler.hpp" #include "config/configitem.hpp" +#include "base/atomic-file.hpp" #include "base/configwriter.hpp" #include "base/exception.hpp" #include "base/dependencygraph.hpp" @@ -198,11 +199,10 @@ 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); - fp << config; - fp.close(); + AtomicFile::Write(path, 0644, config); std::unique_ptr expr = ConfigCompiler::CompileFile(path, String(), "_api"); From 1a55b68541d540cdfa7e019a2784cbf01a30412a Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Fri, 8 Mar 2024 09:58:32 +0100 Subject: [PATCH 2/5] Introduce RAII style `ObjectNameLock` class --- lib/remote/configobjectslock.cpp | 41 ++++++++++++++++++++++++++++++-- lib/remote/configobjectslock.hpp | 30 +++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/lib/remote/configobjectslock.cpp b/lib/remote/configobjectslock.cpp index e529c832b09..f2165f2ce1e 100644 --- a/lib/remote/configobjectslock.cpp +++ b/lib/remote/configobjectslock.cpp @@ -1,13 +1,16 @@ /* Icinga 2 | (c) 2022 Icinga GmbH | GPLv2+ */ -#ifndef _WIN32 +#include "remote/configobjectslock.hpp" +#ifndef _WIN32 #include "base/shared-memory.hpp" -#include "remote/configobjectslock.hpp" #include +#endif /* _WIN32 */ using namespace icinga; +#ifndef _WIN32 + // On *nix one process may write config objects while another is loading the config, so this uses IPC. static SharedMemory l_ConfigObjectsMutex; @@ -22,3 +25,37 @@ ConfigObjectsSharedLock::ConfigObjectsSharedLock(std::try_to_lock_t) } #endif /* _WIN32 */ + +std::mutex ObjectNameLock::m_Mutex; +std::condition_variable ObjectNameLock::m_CV; +std::map> 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 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 lock(m_Mutex); + m_LockedObjectNames[m_Type.get()].erase(m_ObjectName); + } + m_CV.notify_all(); +} diff --git a/lib/remote/configobjectslock.hpp b/lib/remote/configobjectslock.hpp index ee909815f7f..6b75139b66b 100644 --- a/lib/remote/configobjectslock.hpp +++ b/lib/remote/configobjectslock.hpp @@ -2,7 +2,12 @@ #pragma once +#include "base/type.hpp" +#include "base/string.hpp" +#include +#include #include +#include #ifndef _WIN32 #include @@ -69,4 +74,29 @@ class ConfigObjectsSharedLock #endif /* _WIN32 */ + +/** + * 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> m_LockedObjectNames; +}; + } From 433e2de13ab86b6ed518865f447fa1f230e8fd73 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Fri, 8 Mar 2024 10:09:53 +0100 Subject: [PATCH 3/5] ApiListener: Process cluster config updates sequentially --- lib/remote/apilistener-configsync.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/remote/apilistener-configsync.cpp b/lib/remote/apilistener-configsync.cpp index a12db0bca73..04436ad8b99 100644 --- a/lib/remote/apilistener-configsync.cpp +++ b/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 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) { From 099f664ce65c7b825b9cc6e79178725d34b10a21 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Fri, 8 Mar 2024 10:16:33 +0100 Subject: [PATCH 4/5] `ConfigObjectUtility#CreateObject()`: Use `Defer` for config path cleanup --- lib/remote/configobjectutility.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/remote/configobjectutility.cpp b/lib/remote/configobjectutility.cpp index 9b5cf88b5ac..60268f6e195 100644 --- a/lib/remote/configobjectutility.cpp +++ b/lib/remote/configobjectutility.cpp @@ -7,6 +7,7 @@ #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" @@ -204,6 +205,12 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full AtomicFile::Write(path, 0644, config); + // 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 expr = ConfigCompiler::CompileFile(path, String(), "_api"); try { @@ -227,8 +234,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)); @@ -250,8 +255,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)); @@ -275,16 +278,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)); From 546dea95a2b4fea316cd47a9ccfeb3dcff87155e Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 11 Mar 2024 12:34:14 +0100 Subject: [PATCH 5/5] Don't allow to modify/create/delete an object concurrently --- lib/remote/createobjecthandler.cpp | 3 +++ lib/remote/deleteobjecthandler.cpp | 3 +++ lib/remote/modifyobjecthandler.cpp | 3 +++ 3 files changed, 9 insertions(+) diff --git a/lib/remote/createobjecthandler.cpp b/lib/remote/createobjecthandler.cpp index 598eeec3b8b..89977a3d36d 100644 --- a/lib/remote/createobjecthandler.cpp +++ b/lib/remote/createobjecthandler.cpp @@ -124,6 +124,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); diff --git a/lib/remote/deleteobjecthandler.cpp b/lib/remote/deleteobjecthandler.cpp index d79d7ef1fd0..6a7d194d4dc 100644 --- a/lib/remote/deleteobjecthandler.cpp +++ b/lib/remote/deleteobjecthandler.cpp @@ -84,6 +84,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."; diff --git a/lib/remote/modifyobjecthandler.cpp b/lib/remote/modifyobjecthandler.cpp index d6fa98b2e32..a817faad814 100644 --- a/lib/remote/modifyobjecthandler.cpp +++ b/lib/remote/modifyobjecthandler.cpp @@ -112,6 +112,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 (restoreAttrs) { ObjectLock oLock (restoreAttrs);