Merge pull request #10476 from Icinga/broken-config-stage-updates

Handle concurrent config package updates gracefully

lib/remote/configpackageshandler.cpp

@@ -2,6 +2,7 @@
 
 #include "remote/configpackageshandler.hpp"
 #include "remote/configpackageutility.hpp"
+#include "remote/configobjectslock.hpp"
 #include "remote/httputility.hpp"
 #include "remote/filterutility.hpp"
 #include "base/exception.hpp"
@@ -111,6 +112,12 @@ void ConfigPackagesHandler::HandlePost(
 		return;
 	}
 
+	ConfigObjectsSharedLock configObjectsSharedLock(std::try_to_lock);
+	if (!configObjectsSharedLock) {
+		HttpUtility::SendJsonError(response, params, 503, "Icinga is reloading");
+		return;
+	}
+
 	try {
 		std::unique_lock<std::mutex> lock(ConfigPackageUtility::GetStaticPackageMutex());
 
@@ -157,6 +164,12 @@ void ConfigPackagesHandler::HandleDelete(
 		return;
 	}
 
+	ConfigObjectsSharedLock lock(std::try_to_lock);
+	if (!lock) {
+		HttpUtility::SendJsonError(response, params, 503, "Icinga is reloading");
+		return;
+	}
+
 	try {
 		ConfigPackageUtility::DeletePackage(packageName);
 	} catch (const std::exception& ex) {

lib/remote/configpackageutility.cpp

@@ -3,6 +3,7 @@
 #include "remote/configpackageutility.hpp"
 #include "remote/apilistener.hpp"
 #include "base/application.hpp"
+#include "base/atomic-file.hpp"
 #include "base/exception.hpp"
 #include "base/utility.hpp"
 #include <boost/algorithm/string.hpp>
@@ -119,14 +120,9 @@ String ConfigPackageUtility::CreateStage(const String& packageName, const Dictio
 void ConfigPackageUtility::WritePackageConfig(const String& packageName)
 {
 	String stageName = GetActiveStage(packageName);
+	AtomicFile::Write(GetPackageDir() + "/" + packageName + "/include.conf", 0644, "include \"*/include.conf\"\n");
 
-	String includePath = GetPackageDir() + "/" + packageName + "/include.conf";
-	std::ofstream fpInclude(includePath.CStr(), std::ofstream::out | std::ostream::binary | std::ostream::trunc);
-	fpInclude << "include \"*/include.conf\"\n";
-	fpInclude.close();
-
-	String activePath = GetPackageDir() + "/" + packageName + "/active.conf";
-	std::ofstream fpActive(activePath.CStr(), std::ofstream::out | std::ostream::binary | std::ostream::trunc);
+	AtomicFile fpActive(GetPackageDir() + "/" + packageName + "/active.conf", 0644);
 	fpActive << "if (!globals.contains(\"ActiveStages\")) {\n"
 		<< "  globals.ActiveStages = {}\n"
 		<< "}\n"
@@ -145,19 +141,18 @@ void ConfigPackageUtility::WritePackageConfig(const String& packageName)
 		<< "if (!ActiveStages.contains(\"" << packageName << "\")) {\n"
 		<< "  ActiveStages[\"" << packageName << "\"] = \"" << stageName << "\"\n"
 		<< "}\n";
-	fpActive.close();
+	fpActive.Commit();
 }
 
 void ConfigPackageUtility::WriteStageConfig(const String& packageName, const String& stageName)
 {
-	String path = GetPackageDir() + "/" + packageName + "/" + stageName + "/include.conf";
-	std::ofstream fp(path.CStr(), std::ofstream::out | std::ostream::binary | std::ostream::trunc);
+	AtomicFile fp(GetPackageDir() + "/" + packageName + "/" + stageName + "/include.conf", 0644);
 	fp << "include \"../active.conf\"\n"
 		<< "if (ActiveStages[\"" << packageName << "\"] == \"" << stageName << "\") {\n"
 		<< "  include_recursive \"conf.d\"\n"
 		<< "  include_zones \"" << packageName << "\", \"zones.d\"\n"
 		<< "}\n";
-	fp.close();
+	fp.Commit();
 }
 
 void ConfigPackageUtility::ActivateStage(const String& packageName, const String& stageName)
@@ -284,12 +279,7 @@ String ConfigPackageUtility::GetActiveStageFromFile(const String& packageName)
 void ConfigPackageUtility::SetActiveStageToFile(const String& packageName, const String& stageName)
 {
 	std::unique_lock<std::mutex> lock(GetStaticActiveStageMutex());
-
-	String activeStagePath = GetPackageDir() + "/" + packageName + "/active-stage";
-
-	std::ofstream fpActiveStage(activeStagePath.CStr(), std::ofstream::out | std::ostream::binary | std::ostream::trunc); //TODO: fstream exceptions
-	fpActiveStage << stageName;
-	fpActiveStage.close();
+	AtomicFile::Write(GetPackageDir() + "/" + packageName + "/active-stage", 0644, stageName);
 }
 
 String ConfigPackageUtility::GetActiveStage(const String& packageName)

lib/remote/configstageshandler.cpp

@@ -2,6 +2,7 @@
 
 #include "remote/configstageshandler.hpp"
 #include "remote/configpackageutility.hpp"
+#include "remote/configobjectslock.hpp"
 #include "remote/httputility.hpp"
 #include "remote/filterutility.hpp"
 #include "base/application.hpp"
@@ -12,7 +13,10 @@ using namespace icinga;
 
 REGISTER_URLHANDLER("/v1/config/stages", ConfigStagesHandler);
 
-std::atomic<bool> ConfigStagesHandler::m_RunningPackageUpdates (false);
+static bool l_RunningPackageUpdates(false);
+// A timestamp that indicates the last time an Icinga 2 reload failed.
+static double l_LastReloadFailedTime(0);
+static std::mutex l_RunningPackageUpdatesMutex; // Protects the above two variables.
 
 bool ConfigStagesHandler::HandleRequest(
 	const WaitGroup::Ptr&,
@@ -132,12 +136,42 @@ 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.");
+		ConfigObjectsSharedLock configObjectsSharedLock(std::try_to_lock);
+		if (!configObjectsSharedLock) {
+			HttpUtility::SendJsonError(response, params, 503, "Icinga is reloading");
+			return;
 		}
 
-		auto resetPackageUpdates (Shared<Defer>::Make([]() { ConfigStagesHandler::m_RunningPackageUpdates.store(false); }));
+		{
+			std::lock_guard runningPackageUpdatesLock(l_RunningPackageUpdatesMutex);
+			double currentReloadFailedTime = Application::GetLastReloadFailed();
+
+			/**
+			 * Once the m_RunningPackageUpdates flag is set, it typically remains set until the current worker process is
+			 * terminated, in which case the new worker will have its own m_RunningPackageUpdates flag set to false.
+			 * However, if the reload fails for any reason, the m_RunningPackageUpdates flag will remain set to true
+			 * in the current worker process, which will prevent any further package updates from being processed until
+			 * the next Icinga 2 restart.
+			 *
+			 * So, in order to prevent such a situation, we are additionally tracking the last time a reload failed
+			 * and allow to bypass the m_RunningPackageUpdates flag only if the last reload failed time was changed
+			 * since the previous request.
+			 */
+			if (l_RunningPackageUpdates && l_LastReloadFailedTime == currentReloadFailedTime) {
+				return HttpUtility::SendJsonError(
+					response, params, 423,
+					"Conflicting request, there is already an ongoing package update in progress. Please try it again later."
+				);
+			}
+
+			l_RunningPackageUpdates = true;
+			l_LastReloadFailedTime = currentReloadFailedTime;
+		}
+
+		auto resetPackageUpdates (Shared<Defer>::Make([]() {
+			std::lock_guard lock(l_RunningPackageUpdatesMutex);
+			l_RunningPackageUpdates = false;
+		}));
 
 		std::unique_lock<std::mutex> lock(ConfigPackageUtility::GetStaticPackageMutex());
 
@@ -201,6 +235,12 @@ void ConfigStagesHandler::HandleDelete(
 	if (!ConfigPackageUtility::ValidateStageName(stageName))
 		return HttpUtility::SendJsonError(response, params, 400, "Invalid stage name '" + stageName + "'.");
 
+	ConfigObjectsSharedLock lock(std::try_to_lock);
+	if (!lock) {
+		HttpUtility::SendJsonError(response, params, 503, "Icinga is reloading");
+		return;
+	}
+
 	try {
 		ConfigPackageUtility::DeleteStage(packageName, stageName);
 	} catch (const std::exception& ex) {

lib/remote/configstageshandler.hpp

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