From 236a888c1bdec3e965f8c74eccecd2349c51d4ea Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Tue, 8 Mar 2022 10:24:47 +0100 Subject: [PATCH 1/3] Defer: Allow to cancel the callback before going out of scope --- lib/base/defer.hpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/base/defer.hpp b/lib/base/defer.hpp index 9290c92b9..2a232619a 100644 --- a/lib/base/defer.hpp +++ b/lib/base/defer.hpp @@ -30,13 +30,21 @@ public: inline ~Defer() { - try { - m_Func(); - } catch (...) { - // https://stackoverflow.com/questions/130117/throwing-exceptions-out-of-a-destructor + if (m_Func) { + try { + m_Func(); + } catch (...) { + // https://stackoverflow.com/questions/130117/throwing-exceptions-out-of-a-destructor + } } } + inline + void Cancel() + { + m_Func = nullptr; + } + private: std::function m_Func; }; From 668eb4bd0ad7b6daefb54f390c9187b33dacd2c7 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 7 Mar 2022 17:50:15 +0100 Subject: [PATCH 2/3] ConfigPackageUtility: Don't reset ongoing package updates on config validation success and process is going to be reloaded --- lib/remote/configpackageutility.cpp | 21 +++++++++++++++++---- lib/remote/configpackageutility.hpp | 8 ++++++-- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/lib/remote/configpackageutility.cpp b/lib/remote/configpackageutility.cpp index b70009997..e79540147 100644 --- a/lib/remote/configpackageutility.cpp +++ b/lib/remote/configpackageutility.cpp @@ -167,7 +167,8 @@ void ConfigPackageUtility::ActivateStage(const String& packageName, const String WritePackageConfig(packageName); } -void ConfigPackageUtility::TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, bool reload) +void ConfigPackageUtility::TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, + bool activate, bool reload, const Shared::Ptr& resetPackageUpdates) { String logFile = GetPackageDir() + "/" + packageName + "/" + stageName + "/startup.log"; std::ofstream fpLog(logFile.CStr(), std::ofstream::out | std::ostream::binary | std::ostream::trunc); @@ -188,8 +189,17 @@ void ConfigPackageUtility::TryActivateStageCallback(const ProcessResult& pr, con ActivateStage(packageName, stageName); } - if (reload) + if (reload) { + /* + * Cancel the deferred callback before going out of scope so that the config stages handler + * flag isn't resetting earlier and allowing other clients to submit further requests while + * Icinga2 is reloading. Otherwise, the ongoing request will be cancelled halfway before the + * operation is completed once the new worker becomes ready. + */ + resetPackageUpdates->Cancel(); + Application::RequestRestart(); + } } } else { Log(LogCritical, "ConfigPackageUtility") @@ -198,7 +208,8 @@ void ConfigPackageUtility::TryActivateStageCallback(const ProcessResult& pr, con } } -void ConfigPackageUtility::AsyncTryActivateStage(const String& packageName, const String& stageName, bool activate, bool reload) +void ConfigPackageUtility::AsyncTryActivateStage(const String& packageName, const String& stageName, bool activate, bool reload, + const Shared::Ptr& resetPackageUpdates) { VERIFY(Application::GetArgC() >= 1); @@ -224,7 +235,9 @@ void ConfigPackageUtility::AsyncTryActivateStage(const String& packageName, cons Process::Ptr process = new Process(Process::PrepareCommand(args)); process->SetTimeout(Application::GetReloadTimeout()); - process->Run([packageName, stageName, activate, reload](const ProcessResult& pr) { TryActivateStageCallback(pr, packageName, stageName, activate, reload); }); + process->Run([packageName, stageName, activate, reload, resetPackageUpdates](const ProcessResult& pr) { + TryActivateStageCallback(pr, packageName, stageName, activate, reload, resetPackageUpdates); + }); } void ConfigPackageUtility::DeleteStage(const String& packageName, const String& stageName) diff --git a/lib/remote/configpackageutility.hpp b/lib/remote/configpackageutility.hpp index 5c16b75ee..240f5919c 100644 --- a/lib/remote/configpackageutility.hpp +++ b/lib/remote/configpackageutility.hpp @@ -8,6 +8,8 @@ #include "base/dictionary.hpp" #include "base/process.hpp" #include "base/string.hpp" +#include "base/defer.hpp" +#include "base/shared.hpp" #include namespace icinga @@ -37,7 +39,8 @@ public: static void SetActiveStage(const String& packageName, const String& stageName); static void SetActiveStageToFile(const String& packageName, const String& stageName); static void ActivateStage(const String& packageName, const String& stageName); - static void AsyncTryActivateStage(const String& packageName, const String& stageName, bool activate, bool reload); + static void AsyncTryActivateStage(const String& packageName, const String& stageName, bool activate, bool reload, + const Shared::Ptr& resetPackageUpdates); static std::vector > GetFiles(const String& packageName, const String& stageName); @@ -59,7 +62,8 @@ private: static void WritePackageConfig(const String& packageName); static void WriteStageConfig(const String& packageName, const String& stageName); - static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, bool reload); + static void TryActivateStageCallback(const ProcessResult& pr, const String& packageName, const String& stageName, bool activate, + bool reload, const Shared::Ptr& resetPackageUpdates); static bool ValidateFreshName(const String& name); }; From 8037a2f3841cb5e10d188122317da6e15d78a517 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 7 Mar 2022 17:52:08 +0100 Subject: [PATCH 3/3] ConfigStagesHandler: Don't allow concurrent package updates anymore To prevent Icinga2 from being restarted while one or more requests are still in progress and end up as corrupted stages without status file and startup logs. --- lib/remote/configstageshandler.cpp | 12 +++++++++++- lib/remote/configstageshandler.hpp | 2 ++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/remote/configstageshandler.cpp b/lib/remote/configstageshandler.cpp index 9d7c23064..b5aaadd6c 100644 --- a/lib/remote/configstageshandler.cpp +++ b/lib/remote/configstageshandler.cpp @@ -5,12 +5,15 @@ #include "remote/httputility.hpp" #include "remote/filterutility.hpp" #include "base/application.hpp" +#include "base/defer.hpp" #include "base/exception.hpp" using namespace icinga; REGISTER_URLHANDLER("/v1/config/stages", ConfigStagesHandler); +std::atomic ConfigStagesHandler::m_RunningPackageUpdates (false); + bool ConfigStagesHandler::HandleRequest( AsioTlsStream& stream, const ApiUser::Ptr& user, @@ -128,12 +131,19 @@ void ConfigStagesHandler::HandlePost( if (reload && !activate) BOOST_THROW_EXCEPTION(std::invalid_argument("Parameter 'reload' must be false when 'activate' is false.")); + if (m_RunningPackageUpdates.exchange(true)) { + return HttpUtility::SendJsonError(response, params, 423, + "Conflicting request, there is already an ongoing package update in progress. Please try it again later."); + } + + auto resetPackageUpdates (Shared::Make([]() { ConfigStagesHandler::m_RunningPackageUpdates.store(false); })); + std::unique_lock lock(ConfigPackageUtility::GetStaticPackageMutex()); stageName = ConfigPackageUtility::CreateStage(packageName, files); /* validate the config. on success, activate stage and reload */ - ConfigPackageUtility::AsyncTryActivateStage(packageName, stageName, activate, reload); + ConfigPackageUtility::AsyncTryActivateStage(packageName, stageName, activate, reload, resetPackageUpdates); } catch (const std::exception& ex) { return HttpUtility::SendJsonError(response, params, 500, "Stage creation failed.", diff --git a/lib/remote/configstageshandler.hpp b/lib/remote/configstageshandler.hpp index c6d644366..88f248c8f 100644 --- a/lib/remote/configstageshandler.hpp +++ b/lib/remote/configstageshandler.hpp @@ -4,6 +4,7 @@ #define CONFIGSTAGESHANDLER_H #include "remote/httphandler.hpp" +#include namespace icinga { @@ -47,6 +48,7 @@ private: const Dictionary::Ptr& params ); + static std::atomic m_RunningPackageUpdates; }; }