Merge pull request #9267 from Icinga/bugfix/parallel-api-package-calls-do-not-finish-while-reload

Worker process doesn't let parallel API package stage updates to complete when terminated

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<void()> m_Func;
 };

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<Defer>::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<Defer>::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)

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 <vector>
 
 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<Defer>::Ptr& resetPackageUpdates);
 
 	static std::vector<std::pair<String, bool> > 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<Defer>::Ptr& resetPackageUpdates);
 
 	static bool ValidateFreshName(const String& name);
 };

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<bool> 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<Defer>::Make([]() { ConfigStagesHandler::m_RunningPackageUpdates.store(false); }));
+
 		std::unique_lock<std::mutex> 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.",

lib/remote/configstageshandler.hpp

@@ -4,6 +4,7 @@
 #define CONFIGSTAGESHANDLER_H
 
 #include "remote/httphandler.hpp"
+#include <atomic>
 
 namespace icinga
 {
@@ -47,6 +48,7 @@ private:
 		const Dictionary::Ptr& params
 	);
 
+	static std::atomic<bool> m_RunningPackageUpdates;
 };
 
 }