From 008fcd1744a08342271ed374aeaf94b0433b7c82 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Thu, 25 Jan 2024 17:06:12 +0100 Subject: [PATCH 1/3] Preserve runtime objects in a tmp file for the entire validation process Given that the internal `config::Update` cluster events are using this as well to create received runtime objects, we don't want to persist first the conf file and the load and validate it with `CompileFile`. Otherwise, we are forced to remove the newly created file whenever we can't validate, commit or activate it. This also would also have the downside that two cluster events for the same object arriving at the same moment from two different endpoints would result in two different threads simultaneously creating and loading the same config file - whereby only one of the surpasses the validation, while the other is facing an object `re-definition` error and tries to remove that config file it mistakenly thinks it has created. As a consequence, an object successfully created by the former is implicitly deleted by the latter thread, causing the objects to mysteriously disappear. --- lib/remote/configobjectutility.cpp | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/remote/configobjectutility.cpp b/lib/remote/configobjectutility.cpp index 62c910b41..9502bde95 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,13 +199,21 @@ 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); + // Using AtomicFile guarantees that two different threads simultaneously creating and loading the same + // configuration file do not interfere with each other, as the configuration is stored in a unique temp file. + // When one thread fails to pass object validation, it only deletes its temporary file and does not affect + // the other thread in any way. + AtomicFile fp(path, 0644); fp << config; - fp.close(); + // Flush the output buffer to catch any errors ASAP and handle them accordingly! + // Note: AtomicFile places these configs in a temp file and will be automatically + // discarded when it is not committed before going out of scope. + fp.flush(); - std::unique_ptr expr = ConfigCompiler::CompileFile(path, String(), "_api"); + std::unique_ptr expr = ConfigCompiler::CompileText(path, config, String(), "_api"); try { ActivationScope ascope; @@ -225,9 +234,7 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full if (!ConfigItem::CommitItems(ascope.GetContext(), upq, newItems, true)) { if (errors) { Log(LogNotice, "ConfigObjectUtility") - << "Failed to commit config item '" << fullName << "'. Aborting and removing config path '" << path << "'."; - - Utility::Remove(path); + << "Failed to commit config item '" << fullName << "'."; for (const boost::exception_ptr& ex : upq.GetExceptions()) { errors->Add(DiagnosticInformation(ex, false)); @@ -248,9 +255,7 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full if (!ConfigItem::ActivateItems(newItems, true, false, false, cookie)) { if (errors) { Log(LogNotice, "ConfigObjectUtility") - << "Failed to activate config object '" << fullName << "'. Aborting and removing config path '" << path << "'."; - - Utility::Remove(path); + << "Failed to activate config object '" << fullName << "'."; for (const boost::exception_ptr& ex : upq.GetExceptions()) { errors->Add(DiagnosticInformation(ex, false)); @@ -275,6 +280,9 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full ConfigObject::Ptr obj = ctype->GetObject(fullName); if (obj) { + // Object has surpassed the compiling/validation processes, we can safely commit the file! + fp.Commit(); + Log(LogInformation, "ConfigObjectUtility") << "Created and activated object '" << fullName << "' of type '" << type->GetName() << "'."; } else { @@ -283,8 +291,6 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full } } catch (const std::exception& ex) { - Utility::Remove(path); - if (errors) errors->Add(DiagnosticInformation(ex, false)); From 40011b058462720a1c66d4d58a36b06cd630f0d1 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Tue, 13 Feb 2024 14:51:26 +0100 Subject: [PATCH 2/3] Introduce `ObjectNamesMutex` helper class --- lib/remote/apilistener-configsync.cpp | 34 +++++++++++++++++++++++++++ lib/remote/apilistener.hpp | 20 ++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/lib/remote/apilistener-configsync.cpp b/lib/remote/apilistener-configsync.cpp index a12db0bca..d8b8725e3 100644 --- a/lib/remote/apilistener-configsync.cpp +++ b/lib/remote/apilistener-configsync.cpp @@ -462,3 +462,37 @@ void ApiListener::SendRuntimeConfigObjects(const JsonRpcConnection::Ptr& aclient Log(LogInformation, "ApiListener") << "Finished syncing runtime objects to endpoint '" << endpoint->GetName() << "'."; } + +/** + * Locks the specified object name of the given type. 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 + */ +void ObjectNameMutex::Lock(const Type::Ptr& ptype, const String& objName) +{ + std::unique_lock lock(m_Mutex); + m_CV.wait(lock, [this, &ptype, &objName]{ + auto& locked = m_LockedObjectNames[ptype.get()]; + return locked.find(objName) == locked.end(); + }); + + // Add object name to the locked list again to block all other threads that try + // to process a message affecting the same object. + m_LockedObjectNames[ptype.get()].emplace(objName); +} + +/** + * Unlocks the specified object name of the given type. + * + * @param Type::Ptr ptype The type of the object you want to unlock + * @param String objName The name of the object you want to unlock + */ +void ObjectNameMutex::Unlock(const Type::Ptr& ptype, const String& objName) +{ + { + std::unique_lock lock(m_Mutex); + m_LockedObjectNames[ptype.get()].erase(objName); + } + m_CV.notify_all(); +} diff --git a/lib/remote/apilistener.hpp b/lib/remote/apilistener.hpp index fced0a8af..4add0bd11 100644 --- a/lib/remote/apilistener.hpp +++ b/lib/remote/apilistener.hpp @@ -71,6 +71,26 @@ enum class ApiCapabilities : uint_fast64_t IfwApiCheckCommand = 1u << 1u, }; +/** + * 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 ObjectNameMutex +{ +public: + void Lock(const Type::Ptr& ptype, const String& objName); + void Unlock(const Type::Ptr& ptype, const String& objName); + +private: + std::mutex m_Mutex; + std::condition_variable m_CV; + std::map> m_LockedObjectNames; +}; + /** * @ingroup remote */ From 456144c1dc3dd03efd504633246c1c7a5a980993 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Tue, 13 Feb 2024 14:52:20 +0100 Subject: [PATCH 3/3] ApiListener: Process cluster config updates sequentially --- lib/remote/apilistener-configsync.cpp | 19 +++++++++++++++++++ lib/remote/apilistener.hpp | 3 +++ 2 files changed, 22 insertions(+) diff --git a/lib/remote/apilistener-configsync.cpp b/lib/remote/apilistener-configsync.cpp index d8b8725e3..62d342f0f 100644 --- a/lib/remote/apilistener-configsync.cpp +++ b/lib/remote/apilistener-configsync.cpp @@ -7,6 +7,7 @@ #include "base/configtype.hpp" #include "base/json.hpp" #include "base/convert.hpp" +#include "base/defer.hpp" #include "config/vmops.hpp" #include @@ -104,6 +105,15 @@ 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 cluster event (create/update/delete) of a given + // object is being processed at any given time. + listener->m_ObjectConfigChangeLock.Lock(ptype, objName); + + Defer unlockAndNotify([&listener, &ptype, &objName]{ + listener->m_ObjectConfigChangeLock.Unlock(ptype, objName); + }); + ConfigObject::Ptr object = ctype->GetObject(objName); String config = params->Get("config"); @@ -258,6 +268,15 @@ 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 cluster event (create/update/delete) of a given + // object is being processed at any given time. + listener->m_ObjectConfigChangeLock.Lock(ptype, objName); + + Defer unlockAndNotify([&listener, &ptype, &objName]{ + listener->m_ObjectConfigChangeLock.Unlock(ptype, objName); + }); + ConfigObject::Ptr object = ctype->GetObject(objName); if (!object) { diff --git a/lib/remote/apilistener.hpp b/lib/remote/apilistener.hpp index 4add0bd11..a070652e6 100644 --- a/lib/remote/apilistener.hpp +++ b/lib/remote/apilistener.hpp @@ -277,6 +277,9 @@ private: mutable std::mutex m_ActivePackageStagesLock; std::map m_ActivePackageStages; + /* ensures that at most one create/update/delete is being processed per object at each time */ + mutable ObjectNameMutex m_ObjectConfigChangeLock; + void UpdateActivePackageStagesCache(); };