From 19afcd894a15b6b5ba4d162a73db1b7895eef3cc Mon Sep 17 00:00:00 2001 From: Gerd von Egidy Date: Sun, 13 Apr 2014 21:12:18 +0200 Subject: [PATCH 1/6] Split ConfigItem::ActivateItems() into ConfigItem::ValidateItems() and ConfigItem::ActivateItems(). Also removes the -Z commandline parameter: won't be needed when this feature is done. Refs #5788 --- etc/init.d/icinga2.cmake | 2 +- icinga-app/icinga.cpp | 31 ++++++++++----------- lib/config/configitem.cpp | 57 +++++++++++++++++---------------------- lib/config/configitem.h | 10 ++----- 4 files changed, 42 insertions(+), 58 deletions(-) diff --git a/etc/init.d/icinga2.cmake b/etc/init.d/icinga2.cmake index 82d922095..33419bb13 100644 --- a/etc/init.d/icinga2.cmake +++ b/etc/init.d/icinga2.cmake @@ -62,7 +62,7 @@ start() { chmod 2755 $ICINGA2_STATE_DIR/run/icinga2/cmd echo "Starting Icinga 2: " - $DAEMON -c $ICINGA2_CONFIG_FILE -Z -d -e $ICINGA2_ERROR_LOG -u $ICINGA2_USER -g $ICINGA2_GROUP + $DAEMON -c $ICINGA2_CONFIG_FILE -d -e $ICINGA2_ERROR_LOG -u $ICINGA2_USER -g $ICINGA2_GROUP echo "Done" echo diff --git a/icinga-app/icinga.cpp b/icinga-app/icinga.cpp index 4e34019e2..0c538f822 100644 --- a/icinga-app/icinga.cpp +++ b/icinga-app/icinga.cpp @@ -67,7 +67,7 @@ static String LoadAppType(const String& typeSpec) return typeSpec.SubStr(index + 1); } -static bool LoadConfigFiles(const String& appType, ValidationType validate) +static bool LoadConfigFiles(const String& appType) { ConfigCompilerContext::GetInstance()->Reset(); @@ -88,7 +88,7 @@ static bool LoadConfigFiles(const String& appType, ValidationType validate) ConfigItem::Ptr item = builder->Compile(); item->Register(); - bool result = ConfigItem::ActivateItems(validate); + bool result = ConfigItem::ValidateItems(); int warnings = 0, errors = 0; @@ -250,7 +250,6 @@ int Main(void) ("config,c", po::value >(), "parse a configuration file") ("no-config,z", "start without a configuration file") ("validate,C", "exit after validating the configuration") - ("no-validate,Z", "skip validating the configuration") ("debug,x", "enable debugging") ("errorlog,e", po::value(), "log fatal errors to the specified log file (only works in combination with --daemonize)") #ifndef _WIN32 @@ -406,6 +405,14 @@ int Main(void) return EXIT_FAILURE; } + if (!LoadConfigFiles(appType)) + return EXIT_FAILURE; + + if (g_AppParams.count("validate")) { + Log(LogInformation, "icinga-app", "Finished validating the configuration file(s)."); + return EXIT_SUCCESS; + } + if (g_AppParams.count("daemonize")) { String errorLog; @@ -416,22 +423,12 @@ int Main(void) Logger::DisableConsoleLog(); } - ValidationType validate = ValidateStart; - - if (g_AppParams.count("validate")) - validate = ValidateOnly; - - if (g_AppParams.count("no-validate")) - validate = ValidateNone; - - if (!LoadConfigFiles(appType, validate)) + // activate config only after daemonization: it starts threads and that is not compatible with fork() + if (!ConfigItem::ActivateItems()) { + Log(LogCritical, "icinga-app", "Error activating configuration."); return EXIT_FAILURE; - - if (validate == ValidateOnly) { - Log(LogInformation, "icinga-app", "Finished validating the configuration file(s)."); - return EXIT_SUCCESS; } - + #ifndef _WIN32 struct sigaction sa; memset(&sa, 0, sizeof(sa)); diff --git a/lib/config/configitem.cpp b/lib/config/configitem.cpp index c1fb97e72..28bf248c9 100644 --- a/lib/config/configitem.cpp +++ b/lib/config/configitem.cpp @@ -274,29 +274,23 @@ void ConfigItem::ValidateItem(void) m_Validated = true; } -bool ConfigItem::ActivateItems(ValidationType validate) +bool ConfigItem::ValidateItems(void) { if (ConfigCompilerContext::GetInstance()->HasErrors()) return false; - if (ConfigCompilerContext::GetInstance()->HasErrors()) - return false; - ParallelWorkQueue upq; - if (validate != ValidateNone) { - Log(LogInformation, "config", "Validating config items (step 1)..."); + Log(LogInformation, "config", "Validating config items (step 1)..."); - BOOST_FOREACH(const ItemMap::value_type& kv, m_Items) { - upq.Enqueue(boost::bind(&ConfigItem::ValidateItem, kv.second)); - } + BOOST_FOREACH(const ItemMap::value_type& kv, m_Items) { + upq.Enqueue(boost::bind(&ConfigItem::ValidateItem, kv.second)); + } - upq.Join(); + upq.Join(); - if (ConfigCompilerContext::GetInstance()->HasErrors()) - return false; - } else - Log(LogInformation, "config", "Skipping validating config items (step 1)..."); + if (ConfigCompilerContext::GetInstance()->HasErrors()) + return false; Log(LogInformation, "config", "Committing config items"); @@ -328,35 +322,32 @@ bool ConfigItem::ActivateItems(ValidationType validate) Log(LogInformation, "config", "Evaluating 'object' rules..."); ObjectRule::EvaluateRules(); - if (validate != ValidateNone) { - Log(LogInformation, "config", "Validating config items (step 2)..."); + Log(LogInformation, "config", "Validating config items (step 2)..."); - BOOST_FOREACH(const ItemMap::value_type& kv, m_Items) { - upq.Enqueue(boost::bind(&ConfigItem::ValidateItem, kv.second)); - } + BOOST_FOREACH(const ItemMap::value_type& kv, m_Items) { + upq.Enqueue(boost::bind(&ConfigItem::ValidateItem, kv.second)); + } - upq.Join(); - } else - Log(LogInformation, "config", "Skipping validating config items (step 2)..."); + upq.Join(); ConfigItem::DiscardItems(); ConfigType::DiscardTypes(); - if (validate != ValidateNone) { - /* log stats for external parsers */ - BOOST_FOREACH(const DynamicType::Ptr& type, DynamicType::GetTypes()) { - int count = std::distance(type->GetObjects().first, type->GetObjects().second); - if (count > 0) - Log(LogInformation, "config", "Checked " + Convert::ToString(count) + " " + type->GetName() + "(s)."); - } + /* log stats for external parsers */ + BOOST_FOREACH(const DynamicType::Ptr& type, DynamicType::GetTypes()) { + int count = std::distance(type->GetObjects().first, type->GetObjects().second); + if (count > 0) + Log(LogInformation, "config", "Checked " + Convert::ToString(count) + " " + type->GetName() + "(s)."); } + return !ConfigCompilerContext::GetInstance()->HasErrors(); +} + +bool ConfigItem::ActivateItems(void) +{ if (ConfigCompilerContext::GetInstance()->HasErrors()) return false; - if (validate == ValidateOnly) - return true; - /* restore the previous program state */ try { DynamicObject::RestoreObjects(Application::GetStatePath()); @@ -366,6 +357,8 @@ bool ConfigItem::ActivateItems(ValidationType validate) Log(LogInformation, "config", "Triggering Start signal for config items"); + ParallelWorkQueue upq; + BOOST_FOREACH(const DynamicType::Ptr& type, DynamicType::GetTypes()) { BOOST_FOREACH(const DynamicObject::Ptr& object, type->GetObjects()) { if (object->IsActive()) diff --git a/lib/config/configitem.h b/lib/config/configitem.h index 09f1c33d8..370195ec8 100644 --- a/lib/config/configitem.h +++ b/lib/config/configitem.h @@ -27,13 +27,6 @@ namespace icinga { -enum ValidationType -{ - ValidateNone, - ValidateOnly, - ValidateStart -}; - /** * A configuration item. Non-abstract configuration items can be used to * create configuration objects at runtime. @@ -70,7 +63,8 @@ public: void ValidateItem(void); - static bool ActivateItems(ValidationType validate); + static bool ValidateItems(void); + static bool ActivateItems(void); static void DiscardItems(void); private: From 33bd909b712152ee1ed5d6d5342a820e2f2f2424 Mon Sep 17 00:00:00 2001 From: Gerd von Egidy Date: Mon, 14 Apr 2014 00:16:48 +0200 Subject: [PATCH 2/6] Add --reload command-line parameter. Refs #5788 --- etc/init.d/icinga2.cmake | 23 +++++++---------- icinga-app/icinga.cpp | 46 +++++++++++++++++++++++++++++++++ lib/base/application.cpp | 56 +++++++++++++++++++++++++++++++++++++++- lib/base/application.h | 1 + 4 files changed, 111 insertions(+), 15 deletions(-) diff --git a/etc/init.d/icinga2.cmake b/etc/init.d/icinga2.cmake index 33419bb13..1920c2325 100644 --- a/etc/init.d/icinga2.cmake +++ b/etc/init.d/icinga2.cmake @@ -61,11 +61,13 @@ start() { chown $ICINGA2_USER:$ICINGA2_COMMAND_GROUP $ICINGA2_STATE_DIR/run/icinga2/cmd chmod 2755 $ICINGA2_STATE_DIR/run/icinga2/cmd - echo "Starting Icinga 2: " - $DAEMON -c $ICINGA2_CONFIG_FILE -d -e $ICINGA2_ERROR_LOG -u $ICINGA2_USER -g $ICINGA2_GROUP - - echo "Done" - echo + echo "Starting Icinga 2: " + if ! $DAEMON -c $ICINGA2_CONFIG_FILE -d -e $ICINGA2_ERROR_LOG -u $ICINGA2_USER -g $ICINGA2_GROUP; then + echo "Error starting Icinga." + exit 1 + else + echo "Done" + fi } # Restart Icinga 2 @@ -104,15 +106,9 @@ stop() { # Reload Icinga 2 reload() { printf "Reloading Icinga 2: " - if [ ! -e $ICINGA2_PID_FILE ]; then - echo "The PID file '$ICINGA2_PID_FILE' does not exist." + if ! $DAEMON -c $ICINGA2_CONFIG_FILE -d --reload -e $ICINGA2_ERROR_LOG -u $ICINGA2_USER -g $ICINGA2_GROUP; then + echo "Error reloading Icinga." exit 1 - fi - - pid=`cat $ICINGA2_PID_FILE` - - if ! kill -HUP $pid >/dev/null 2>&1; then - echo "Failed - Icinga 2 is not running." else echo "Done" fi @@ -162,7 +158,6 @@ case "$1" in start ;; reload) - checkconfig reload fail reload ;; checkconfig) diff --git a/icinga-app/icinga.cpp b/icinga-app/icinga.cpp index 0c538f822..d2f9834f0 100644 --- a/icinga-app/icinga.cpp +++ b/icinga-app/icinga.cpp @@ -187,6 +187,34 @@ static bool Daemonize(const String& stderrFile) return true; } +/** + * Terminate another process and wait till it has ended + * + * @params target PID of the process to end + */ +static void TerminateAndWaitForEnd(pid_t target) +{ +#ifndef _WIN32 + // allow 15 seconds timeout + double timeout = Utility::GetTime() + 15; + + int ret = kill(target, SIGTERM); + + while(Utility::GetTime() < timeout && (ret==0 || errno!=ESRCH)) + { + Utility::Sleep(0.1); + ret = kill(target, 0); + } + + // timeout and the process still seems to live: kill it + if (ret == 0 || errno != ESRCH) + kill(target, SIGKILL); + +#else + // TODO: implement this for Win32 +#endif /* _WIN32 */ +} + int Main(void) { int argc = Application::GetArgC(); @@ -252,6 +280,7 @@ int Main(void) ("validate,C", "exit after validating the configuration") ("debug,x", "enable debugging") ("errorlog,e", po::value(), "log fatal errors to the specified log file (only works in combination with --daemonize)") + ("reload,r", "reload a running icinga instance") #ifndef _WIN32 ("daemonize,d", "detach from the controlling terminal") ("user,u", po::value(), "user to run Icinga as") @@ -405,6 +434,17 @@ int Main(void) return EXIT_FAILURE; } + pid_t runningpid = Application::ReadPidFile(Application::GetPidPath()); + if (g_AppParams.count("reload")) { + if (runningpid < 0) { + Log(LogCritical, "icinga-app", "No instance of Icinga currently running: can't reload."); + return EXIT_FAILURE; + } + } else if (!g_AppParams.count("validate") && runningpid >= 0) { + Log(LogCritical, "icinga-app", "Another instance of Icinga already running at PID " + Convert::ToString(runningpid)); + return EXIT_FAILURE; + } + if (!LoadConfigFiles(appType)) return EXIT_FAILURE; @@ -413,6 +453,12 @@ int Main(void) return EXIT_SUCCESS; } + if (g_AppParams.count("reload")) { + Log(LogInformation, "icinga-app", "Terminating previous instance of icinga (PID " + Convert::ToString(runningpid) + ")"); + TerminateAndWaitForEnd(runningpid); + Log(LogInformation, "icinga-app", "Previous instance has ended, taking over now."); + } + if (g_AppParams.count("daemonize")) { String errorLog; diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 6e94b35bf..fc19b93b3 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -638,7 +638,7 @@ void Application::UpdatePidFile(const String& filename) } #endif /* _WIN32 */ - fprintf(m_PidFile, "%d", Utility::GetPid()); + fprintf(m_PidFile, "%d\n", Utility::GetPid()); fflush(m_PidFile); } @@ -656,6 +656,60 @@ void Application::ClosePidFile(void) m_PidFile = NULL; } +/** + * Checks if another process currently owns the pidfile and read it + * + * @param filename The name of the PID file. + * @returns -1: no process owning the pidfile, pid of the process otherwise + */ +pid_t Application::ReadPidFile(const String& filename) +{ + FILE *pidfile = fopen(filename.CStr(), "r"); + + if (pidfile == NULL) + return -1; + +#ifndef _WIN32 + int fd = fileno(pidfile); + + struct flock lock; + + lock.l_start = 0; + lock.l_len = 0; + lock.l_type = F_WRLCK; + lock.l_whence = SEEK_SET; + + if (fcntl(fd, F_GETLK, &lock) < 0) { + int error = errno; + fclose(pidfile); + BOOST_THROW_EXCEPTION(posix_error() + << boost::errinfo_api_function("fcntl") + << boost::errinfo_errno(error)); + } + + if (lock.l_type == F_UNLCK) { + // nobody has locked the file: no icinga running + fclose(pidfile); + return -1; + } +#endif /* _WIN32 */ + + pid_t runningpid; + int res = fscanf(pidfile, "%d", &runningpid); + fclose(pidfile); + + // bogus result? + if (res != 1) + return -1; + +#ifdef _WIN32 + // TODO: add check if the read pid is still running or not +#endif /* _WIN32 */ + + return runningpid; +} + + /** * Retrieves the path of the installation prefix. * diff --git a/lib/base/application.h b/lib/base/application.h index 6df97f737..9597409e5 100644 --- a/lib/base/application.h +++ b/lib/base/application.h @@ -69,6 +69,7 @@ public: void UpdatePidFile(const String& filename); void ClosePidFile(void); + static pid_t ReadPidFile(const String& filename); static String GetExePath(const String& argv0); From 3a294bbd5da6e71fad96841f5413152a3ed9e64c Mon Sep 17 00:00:00 2001 From: Gerd von Egidy Date: Sun, 27 Apr 2014 21:47:25 +0200 Subject: [PATCH 3/6] Fork new process from previous daemon on reload. The previously planned logic of forking a new daemon from the reload-process didn't work with systemd: systemd does not allow long-running processes started from within the reload command. Replaces parameter --reload with --reload-internal which is used when starting the new daemon. Refs #5788 --- etc/init.d/icinga2.cmake | 10 +++--- icinga-app/icinga.cpp | 21 +++++------- lib/base/application.cpp | 71 +++++++++++++++++++++++++--------------- lib/base/application.h | 6 ++++ lib/base/utility.cpp | 15 +++++++++ lib/base/utility.h | 1 + 6 files changed, 82 insertions(+), 42 deletions(-) diff --git a/etc/init.d/icinga2.cmake b/etc/init.d/icinga2.cmake index 1920c2325..f181fa90a 100644 --- a/etc/init.d/icinga2.cmake +++ b/etc/init.d/icinga2.cmake @@ -106,11 +106,13 @@ stop() { # Reload Icinga 2 reload() { printf "Reloading Icinga 2: " - if ! $DAEMON -c $ICINGA2_CONFIG_FILE -d --reload -e $ICINGA2_ERROR_LOG -u $ICINGA2_USER -g $ICINGA2_GROUP; then - echo "Error reloading Icinga." - exit 1 - else + + pid=`cat $ICINGA2_PID_FILE` + if kill -HUP $pid >/dev/null 2>&1; then echo "Done" + else + echo "Error: Icinga not running" + exit 3 fi } diff --git a/icinga-app/icinga.cpp b/icinga-app/icinga.cpp index d2f9834f0..4eb7779fb 100644 --- a/icinga-app/icinga.cpp +++ b/icinga-app/icinga.cpp @@ -203,7 +203,7 @@ static void TerminateAndWaitForEnd(pid_t target) while(Utility::GetTime() < timeout && (ret==0 || errno!=ESRCH)) { Utility::Sleep(0.1); - ret = kill(target, 0); + ret = kill(target, SIGTERM); } // timeout and the process still seems to live: kill it @@ -280,7 +280,7 @@ int Main(void) ("validate,C", "exit after validating the configuration") ("debug,x", "enable debugging") ("errorlog,e", po::value(), "log fatal errors to the specified log file (only works in combination with --daemonize)") - ("reload,r", "reload a running icinga instance") + ("reload-internal", "used internally to implement config reload: do not call manually, send SIGHUP instead") #ifndef _WIN32 ("daemonize,d", "detach from the controlling terminal") ("user,u", po::value(), "user to run Icinga as") @@ -434,15 +434,12 @@ int Main(void) return EXIT_FAILURE; } - pid_t runningpid = Application::ReadPidFile(Application::GetPidPath()); - if (g_AppParams.count("reload")) { - if (runningpid < 0) { - Log(LogCritical, "icinga-app", "No instance of Icinga currently running: can't reload."); + if (!g_AppParams.count("validate") && !g_AppParams.count("reload-internal")) { + pid_t runningpid = Application::ReadPidFile(Application::GetPidPath()); + if (runningpid >= 0) { + Log(LogCritical, "icinga-app", "Another instance of Icinga already running with PID " + Convert::ToString(runningpid)); return EXIT_FAILURE; } - } else if (!g_AppParams.count("validate") && runningpid >= 0) { - Log(LogCritical, "icinga-app", "Another instance of Icinga already running at PID " + Convert::ToString(runningpid)); - return EXIT_FAILURE; } if (!LoadConfigFiles(appType)) @@ -453,9 +450,9 @@ int Main(void) return EXIT_SUCCESS; } - if (g_AppParams.count("reload")) { - Log(LogInformation, "icinga-app", "Terminating previous instance of icinga (PID " + Convert::ToString(runningpid) + ")"); - TerminateAndWaitForEnd(runningpid); + if(g_AppParams.count("reload-internal")) { + Log(LogInformation, "icinga-app", "Terminating previous instance of Icinga (PID " + Convert::ToString(Utility::GetParentPid()) + ")"); + TerminateAndWaitForEnd(Utility::GetParentPid()); Log(LogInformation, "icinga-app", "Previous instance has ended, taking over now."); } diff --git a/lib/base/application.cpp b/lib/base/application.cpp index fc19b93b3..a3a602905 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -46,6 +46,7 @@ REGISTER_TYPE(Application); Application *Application::m_Instance = NULL; bool Application::m_ShuttingDown = false; +bool Application::m_RequestRestart = false; bool Application::m_Restarting = false; bool Application::m_Debugging = false; int Application::m_ArgC; @@ -219,7 +220,8 @@ void Application::RunEventLoop(void) const double lastLoop = Utility::GetTime(); - while (!m_ShuttingDown && !m_Restarting) { +mainloop: + while (!m_ShuttingDown && !m_RequestRestart) { /* Watches for changes to the system time. Adjusts timers if necessary. */ Utility::Sleep(2.5); @@ -238,8 +240,21 @@ void Application::RunEventLoop(void) const } lastLoop = now; - } + if (m_RequestRestart) { + m_RequestRestart = false; // we are now handling the request, once is enough + + // are we already restarting? ignore request if we already are + if (m_Restarting) + goto mainloop; + + m_Restarting = true; + StartReloadProcess(); + + goto mainloop; + } + } + Log(LogInformation, "base", "Shutting down Icinga..."); DynamicObject::StopObjects(); Application::GetInstance()->OnShutdown(); @@ -259,6 +274,33 @@ void Application::OnShutdown(void) /* Nothing to do here. */ } +void Application::StartReloadProcess(void) const +{ + Log(LogInformation, "base", "Got reload command: Starting new instance."); + + // prepare arguments + std::vector args; + args.push_back(GetExePath(m_ArgV[0])); + + for (int i=1; i < Application::GetArgC(); i++) + if (std::string(Application::GetArgV()[i]) != "--reload-internal") + args.push_back(Application::GetArgV()[i]); + args.push_back("--reload-internal"); + + Process::Ptr process = make_shared(args); + + process->SetTimeout(15); + + process->Run(boost::bind(&Application::ReloadProcessCallback, _1)); +} + +void Application::ReloadProcessCallback(const ProcessResult& pr) +{ + if (pr.ExitStatus != 0) + Log(LogCritical, "base", "Found error in config: reloading aborted"); + m_Restarting=false; +} + /** * Signals the application to shut down during the next * execution of the event loop. @@ -274,7 +316,7 @@ void Application::RequestShutdown(void) */ void Application::RequestRestart(void) { - m_Restarting = true; + m_RequestRestart = true; } /** @@ -563,29 +605,6 @@ int Application::Run(void) result = Main(); - if (m_Restarting) { - Log(LogInformation, "base", "Restarting application."); - -#ifndef _WIN32 - String exePath = GetExePath(m_ArgV[0]); - - int fdcount = getdtablesize(); - - for (int i = 3; i < fdcount; i++) - (void) close(i); - - (void) execv(exePath.CStr(), m_ArgV); -#else /* _WIN32 */ - STARTUPINFO si; - PROCESS_INFORMATION pi; - memset(&si, 0, sizeof(si)); - si.cb = sizeof(si); - CreateProcess(NULL, GetCommandLine(), NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi); -#endif /* _WIN32 */ - - _exit(0); - } - return result; } diff --git a/lib/base/application.h b/lib/base/application.h index 9597409e5..9f5e5ebe1 100644 --- a/lib/base/application.h +++ b/lib/base/application.h @@ -24,6 +24,7 @@ #include "base/application.th" #include "base/threadpool.h" #include "base/dynamicobject.h" +#include "base/process.h" namespace icinga { @@ -109,6 +110,8 @@ protected: void RunEventLoop(void) const; + void StartReloadProcess(void) const; + virtual void OnShutdown(void); private: @@ -116,6 +119,7 @@ private: static bool m_ShuttingDown; /**< Whether the application is in the process of shutting down. */ + static bool m_RequestRestart; static bool m_Restarting; static int m_ArgC; /**< The number of command-line arguments. */ static char **m_ArgV; /**< Command-line arguments. */ @@ -135,6 +139,8 @@ private: static void SigAbrtHandler(int signum); static void ExceptionHandler(void); + + static void ReloadProcessCallback(const ProcessResult& pr); }; } diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index 56559fea2..dc3c8232b 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -331,6 +331,21 @@ pid_t Utility::GetPid(void) #endif /* _WIN32 */ } +/** + * Returns the ID of the parent process. + * + * @returns The PID. + */ +pid_t Utility::GetParentPid(void) +{ +#ifndef _WIN32 + return getppid(); +#else /* _WIN32 */ + // TODO + return 0; +#endif /* _WIN32 */ +} + /** * Sleeps for the specified amount of time. * diff --git a/lib/base/utility.h b/lib/base/utility.h index f8c05a5fe..ada33d43a 100644 --- a/lib/base/utility.h +++ b/lib/base/utility.h @@ -74,6 +74,7 @@ public: static double GetTime(void); static pid_t GetPid(void); + static pid_t GetParentPid(void); static void Sleep(double timeout); From 3ece2ba6435c6d0d7c73c586e0d40367b42dd079 Mon Sep 17 00:00:00 2001 From: Gerd von Egidy Date: Sun, 27 Apr 2014 23:40:37 +0200 Subject: [PATCH 4/6] Fix logging during shutdown procedure. Refs #5788 --- icinga-app/icinga.cpp | 5 ++--- lib/base/streamlogger.cpp | 9 +++++++++ lib/base/streamlogger.h | 1 + 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/icinga-app/icinga.cpp b/icinga-app/icinga.cpp index 4eb7779fb..4208bed6a 100644 --- a/icinga-app/icinga.cpp +++ b/icinga-app/icinga.cpp @@ -200,10 +200,9 @@ static void TerminateAndWaitForEnd(pid_t target) int ret = kill(target, SIGTERM); - while(Utility::GetTime() < timeout && (ret==0 || errno!=ESRCH)) - { + while (Utility::GetTime() < timeout && (ret == 0 || errno != ESRCH)) { Utility::Sleep(0.1); - ret = kill(target, SIGTERM); + ret = kill(target, 0); } // timeout and the process still seems to live: kill it diff --git a/lib/base/streamlogger.cpp b/lib/base/streamlogger.cpp index fc457034d..6a3b11669 100644 --- a/lib/base/streamlogger.cpp +++ b/lib/base/streamlogger.cpp @@ -42,6 +42,15 @@ void StreamLogger::Start(void) m_Tty = false; } +void StreamLogger::Stop(void) +{ + Logger::Stop(); + + // make sure we flush the log data on shutdown, even if we don't call the destructor + if (m_Stream) + m_Stream->flush(); +} + /** * Destructor for the StreamLogger class. */ diff --git a/lib/base/streamlogger.h b/lib/base/streamlogger.h index 07fedf092..1d7246009 100644 --- a/lib/base/streamlogger.h +++ b/lib/base/streamlogger.h @@ -39,6 +39,7 @@ public: DECLARE_PTR_TYPEDEFS(StreamLogger); virtual void Start(void); + virtual void Stop(void); ~StreamLogger(void); void BindStream(std::ostream *stream, bool ownsStream); From 9f56b6ee74765187902565b2f642aed7cabaa260 Mon Sep 17 00:00:00 2001 From: Gerd von Egidy Date: Mon, 28 Apr 2014 23:30:56 +0200 Subject: [PATCH 5/6] Fix handling of m_RequestRestart in RunEventLoop, improve reload timeout Refs #5788 --- lib/base/application.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/base/application.cpp b/lib/base/application.cpp index a3a602905..6cef2b83f 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -240,19 +240,19 @@ mainloop: } lastLoop = now; - - if (m_RequestRestart) { - m_RequestRestart = false; // we are now handling the request, once is enough + } - // are we already restarting? ignore request if we already are - if (m_Restarting) - goto mainloop; - - m_Restarting = true; - StartReloadProcess(); + if (m_RequestRestart) { + m_RequestRestart = false; // we are now handling the request, once is enough + // are we already restarting? ignore request if we already are + if (m_Restarting) goto mainloop; - } + + m_Restarting = true; + StartReloadProcess(); + + goto mainloop; } Log(LogInformation, "base", "Shutting down Icinga..."); @@ -289,7 +289,7 @@ void Application::StartReloadProcess(void) const Process::Ptr process = make_shared(args); - process->SetTimeout(15); + process->SetTimeout(300); process->Run(boost::bind(&Application::ReloadProcessCallback, _1)); } From 1e321f0959643f905a3af2505f90c838293f37ca Mon Sep 17 00:00:00 2001 From: Gerd von Egidy Date: Tue, 29 Apr 2014 00:05:39 +0200 Subject: [PATCH 6/6] Fix possible race when the reload-process determines it's parent pid and the true parent has ended Now transfers the true parent pid as parameter to --reload-internal. Refs #5788 --- icinga-app/icinga.cpp | 11 ++++++----- lib/base/application.cpp | 7 ++++++- lib/base/utility.cpp | 15 --------------- lib/base/utility.h | 1 - 4 files changed, 12 insertions(+), 22 deletions(-) diff --git a/icinga-app/icinga.cpp b/icinga-app/icinga.cpp index 4208bed6a..8c7940ca1 100644 --- a/icinga-app/icinga.cpp +++ b/icinga-app/icinga.cpp @@ -195,8 +195,8 @@ static bool Daemonize(const String& stderrFile) static void TerminateAndWaitForEnd(pid_t target) { #ifndef _WIN32 - // allow 15 seconds timeout - double timeout = Utility::GetTime() + 15; + // allow 30 seconds timeout + double timeout = Utility::GetTime() + 30; int ret = kill(target, SIGTERM); @@ -279,8 +279,8 @@ int Main(void) ("validate,C", "exit after validating the configuration") ("debug,x", "enable debugging") ("errorlog,e", po::value(), "log fatal errors to the specified log file (only works in combination with --daemonize)") - ("reload-internal", "used internally to implement config reload: do not call manually, send SIGHUP instead") #ifndef _WIN32 + ("reload-internal", po::value(), "used internally to implement config reload: do not call manually, send SIGHUP instead") ("daemonize,d", "detach from the controlling terminal") ("user,u", po::value(), "user to run Icinga as") ("group,g", po::value(), "group to run Icinga as") @@ -450,8 +450,9 @@ int Main(void) } if(g_AppParams.count("reload-internal")) { - Log(LogInformation, "icinga-app", "Terminating previous instance of Icinga (PID " + Convert::ToString(Utility::GetParentPid()) + ")"); - TerminateAndWaitForEnd(Utility::GetParentPid()); + int parentpid = g_AppParams["reload-internal"].as(); + Log(LogInformation, "icinga-app", "Terminating previous instance of Icinga (PID " + Convert::ToString(parentpid) + ")"); + TerminateAndWaitForEnd(parentpid); Log(LogInformation, "icinga-app", "Previous instance has ended, taking over now."); } diff --git a/lib/base/application.cpp b/lib/base/application.cpp index 6cef2b83f..5a5f36757 100644 --- a/lib/base/application.cpp +++ b/lib/base/application.cpp @@ -26,6 +26,7 @@ #include "base/utility.h" #include "base/debug.h" #include "base/type.h" +#include "base/convert.h" #include "base/scriptvariable.h" #include "icinga-version.h" #include @@ -282,10 +283,14 @@ void Application::StartReloadProcess(void) const std::vector args; args.push_back(GetExePath(m_ArgV[0])); - for (int i=1; i < Application::GetArgC(); i++) + for (int i=1; i < Application::GetArgC(); i++) { if (std::string(Application::GetArgV()[i]) != "--reload-internal") args.push_back(Application::GetArgV()[i]); + else + i++; // the next parameter after --reload-internal is the pid, remove that too + } args.push_back("--reload-internal"); + args.push_back(Convert::ToString(Utility::GetPid())); Process::Ptr process = make_shared(args); diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index dc3c8232b..56559fea2 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -331,21 +331,6 @@ pid_t Utility::GetPid(void) #endif /* _WIN32 */ } -/** - * Returns the ID of the parent process. - * - * @returns The PID. - */ -pid_t Utility::GetParentPid(void) -{ -#ifndef _WIN32 - return getppid(); -#else /* _WIN32 */ - // TODO - return 0; -#endif /* _WIN32 */ -} - /** * Sleeps for the specified amount of time. * diff --git a/lib/base/utility.h b/lib/base/utility.h index ada33d43a..f8c05a5fe 100644 --- a/lib/base/utility.h +++ b/lib/base/utility.h @@ -74,7 +74,6 @@ public: static double GetTime(void); static pid_t GetPid(void); - static pid_t GetParentPid(void); static void Sleep(double timeout);