From: Thomas Markwalder Date: Thu, 2 Jul 2015 18:42:58 +0000 (-0400) Subject: [3769] Added env var,KEA_PIDFILE_DIR; D2 now uses a PIDFile X-Git-Tag: trac3785_base~2^2~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d743c5f2743e3e07d83a8f8fa5cf8d7dee8379e5;p=thirdparty%2Fkea.git [3769] Added env var,KEA_PIDFILE_DIR; D2 now uses a PIDFile src/lib/dhcpsrv/daemon.c/h Daemon::Daemon() - Constructor will now override the default PID directory with the value of env variable KEA_PIDFILE_DIR. This provides a simple means to alter the value for tests. Added am_file_author_ flag so Daemon instances will only delete a file they have written. src/lib/testutils/dhcp_test_lib.sh.in - verify_server_pid() - new function which verifies that a server has a PID file and that it contains the server's PID, and that the process is alive. src/bin/keactrl/tests/Makefile.am - added export of KEA_PIDFILE_DIR to override default PID directory during tests Added PID file creation to D2 src/bin/d2/d_controller.cc - DControllerBase::launch() - Added block to createPIDFile() -DControllerBase::parseArgs() Replaced call to Daemon::init() with call to Daemon::setConfigFile() src/bin/d2/tests/Makefile.am - added export of KEA_PIDFILE_DIR to override default PID directory during tests src/bin/d2/tests/d2_process_tests.sh.in - dupcliate_server_start_test() - new test which verifies that D2 cannot be started twice (with the same configuration file) src/bin/d2/tests/d2_unittests.cc - main(int argc, char* argv[]) sets environment variable KEA_PIDFILE_DIR to override default PID diretory during tests src/lib/util/pid_file.cc/h src/lib/util/tests/pid_file_unittest.cc Changed PIDFile::check() to return either the PID contained in the PID file if the process is alive, or 0, rather than bool. This permits callers to see/log the PID. --- diff --git a/src/bin/d2/d2_messages.mes b/src/bin/d2/d2_messages.mes index 42516d13fa..a0193a7683 100644 --- a/src/bin/d2/d2_messages.mes +++ b/src/bin/d2/d2_messages.mes @@ -492,3 +492,20 @@ server. % DHCP_DDNS_UPDATE_RESPONSE_RECEIVED Request ID %1: to server: %2 status: %3 This is a debug message issued when DHCP_DDNS receives sends a DNS update response from a DNS server. + +% DHCP_DDNS_ALREADY_RUNNING %1 already running? %2 +This is an error message that occurs when DHCP_DDNS encounters a pre-existing +PID file which contains the PID of a running process. This most likely +indicates an attempt to start a second instance of DHCP_DDNS using the +same configuration file. It is possible, the unlikely that the PID file +is a remnant left behind by a server crash or power failure and the PID +it contains refers to a process other than DHCP_DDNS. In such an event, +it would be necessary to manually remove the PID file. + +% DHCP_DDNS_PID_FILE_ERROR %1 could not create a PID file: %2 +This is an error message that occurs when DHCP_DDNS is unable to create +its PID file. The log message should contain details sufficient to +determine the underlying cause. The most likely culprits are that +some portion of the pathname does not exist or a permissions issue. The +default path is determined by --localstatedir configure paramter but +may be overridden by setting environment variable, KEA_PIDFILE_DIR. diff --git a/src/bin/d2/d_controller.cc b/src/bin/d2/d_controller.cc index 1cbd9aa598..6bbc304453 100644 --- a/src/bin/d2/d_controller.cc +++ b/src/bin/d2/d_controller.cc @@ -70,6 +70,8 @@ DControllerBase::launch(int argc, char* argv[], const bool test_mode) { throw; // rethrow it } + Daemon::setProcName(bin_name_); + // It is important that we set a default logger name because this name // will be used when the user doesn't provide the logging configuration // in the Kea configuration file. @@ -87,6 +89,18 @@ DControllerBase::launch(int argc, char* argv[], const bool test_mode) { Daemon::loggerInit(bin_name_.c_str(), verbose_); } + try { + createPIDFile(); + } catch (const dhcp::DaemonPIDExists& ex) { + LOG_FATAL(dctl_logger, DHCP_DDNS_ALREADY_RUNNING) + .arg(bin_name_).arg(ex.what()); + isc_throw (LaunchError, "Launch Failed: " << ex.what()); + } catch (const std::exception& ex) { + LOG_FATAL(dctl_logger, DHCP_DDNS_PID_FILE_ERROR) + .arg(app_name_).arg(ex.what()); + isc_throw (LaunchError, "Launch failed: " << ex.what()); + } + // Log the starting of the service. Although this is the controller // module, use a "DHCP_DDNS_" prefix to the module (to conform to the // principle of least astonishment). @@ -173,7 +187,7 @@ DControllerBase::parseArgs(int argc, char* argv[]) isc_throw(InvalidUsage, "configuration file name missing"); } - Daemon::init(optarg); + Daemon::setConfigFile(optarg); break; case '?': { diff --git a/src/bin/d2/d_controller.h b/src/bin/d2/d_controller.h index 61731872c5..19b42c65f4 100644 --- a/src/bin/d2/d_controller.h +++ b/src/bin/d2/d_controller.h @@ -50,6 +50,12 @@ public: isc::Exception(file, line, what) { }; }; +/// @brief Exception thrown when the controller launch fails. +class LaunchError: public isc::Exception { +public: + LaunchError (const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) { }; +}; /// @brief Exception thrown when the application process fails. class ProcessInitError: public isc::Exception { diff --git a/src/bin/d2/tests/Makefile.am b/src/bin/d2/tests/Makefile.am index febe54b8ae..588654f18c 100644 --- a/src/bin/d2/tests/Makefile.am +++ b/src/bin/d2/tests/Makefile.am @@ -13,6 +13,7 @@ check-local: for shtest in $(SHTESTS) ; do \ echo Running test: $$shtest ; \ export KEA_LOCKFILE_DIR=$(abs_top_builddir); \ + export KEA_PIDFILE_DIR=$(abs_top_builddir); \ ${SHELL} $(abs_builddir)/$$shtest || exit ; \ done diff --git a/src/bin/d2/tests/d2_process_tests.sh.in b/src/bin/d2/tests/d2_process_tests.sh.in index 48f67ef5ff..1233b30701 100755 --- a/src/bin/d2/tests/d2_process_tests.sh.in +++ b/src/bin/d2/tests/d2_process_tests.sh.in @@ -1,4 +1,4 @@ -# Copyright (C) 2014 Internet Systems Consortium, Inc. ("ISC") +# Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC") # # Permission to use, copy, modify, and/or distribute this software for any # purpose with or without fee is hereby granted, provided that the above @@ -235,8 +235,50 @@ shutdown_test() { test_finish 0 } +# This test verifies if only one D2 per config can be started. +dupcliate_server_start_test() { + # Log the start of the test and print test name. + test_start "dhcp_ddns.duplicate_server_start_test" + # Remove dangling D2 instances and remove log files. + cleanup + # Create new configuration file. + create_config "${CONFIG}" + # Instruct D2 to log to the specific file. + set_logger + # Start D2. + start_kea ${bin_path}/${bin} + # Wait up to 20s for D2 to start. + wait_for_kea 20 + if [ ${_WAIT_FOR_KEA} -eq 0 ]; then + printf "ERROR: timeout waiting for D2 to start.\n" + clean_exit 1 + fi + + # Verify server is still running + verify_server_pid ${bin} ${CFG_FILE} + + printf "PID file is [%s], PID is [%d]" ${_SERVER_PID_FILE} ${_SERVER_PID} + + # Now try to start a second one + start_kea ${bin_path}/${bin} + + wait_for_message 10 "DHCP_DDNS_ALREADY_RUNNING" 1 + if [ ${_WAIT_FOR_MESSAGE} -eq 0 ]; then + printf "ERROR: Second D2 instance started? PID conflict not reported.\n" + clean_exit 1 + fi + + # Verify server is still running + verify_server_pid ${bin} ${CFG_FILE} + + # All ok. Shut down D2 and exit. + test_finish 0 +} + + dynamic_reconfiguration_test shutdown_test "dhcp-ddns.sigterm_test" 15 shutdown_test "dhcp-ddns.sigint_test" 2 version_test "dhcp-ddns.version" logger_vars_test "dhcp-ddns.variables" +dupcliate_server_start_test diff --git a/src/bin/d2/tests/d2_unittests.cc b/src/bin/d2/tests/d2_unittests.cc index 413abdf527..a5e986dec5 100644 --- a/src/bin/d2/tests/d2_unittests.cc +++ b/src/bin/d2/tests/d2_unittests.cc @@ -25,6 +25,9 @@ main(int argc, char* argv[]) { // src/lib/log/README for info on how to tweak logging isc::log::initLogger(); + // Override --localstatedir value for PID files + setenv("KEA_PIDFILE_DIR", TEST_DATA_BUILDDIR, 1); + int result = RUN_ALL_TESTS(); return (result); diff --git a/src/bin/keactrl/tests/Makefile.am b/src/bin/keactrl/tests/Makefile.am index b692cbe41a..ae0ec665ac 100644 --- a/src/bin/keactrl/tests/Makefile.am +++ b/src/bin/keactrl/tests/Makefile.am @@ -16,6 +16,7 @@ check-local: chmod +x $(abs_builddir)/$$shtest ; \ export KEA_LOCKFILE_DIR=$(abs_top_builddir); \ export KEACTRL_BUILD_DIR=$(abs_top_builddir); \ + export KEA_PIDFILE_DIR=$(abs_top_builddir); \ export KEACTRL_CONF=$(abs_top_builddir)/src/bin/keactrl/tests/keactrl_test.conf; \ ${SHELL} $(abs_builddir)/$$shtest || exit ; \ done diff --git a/src/lib/dhcpsrv/daemon.cc b/src/lib/dhcpsrv/daemon.cc index 9d89bbc098..6fe0172057 100644 --- a/src/lib/dhcpsrv/daemon.cc +++ b/src/lib/dhcpsrv/daemon.cc @@ -39,11 +39,18 @@ std::string Daemon::config_file_ = ""; Daemon::Daemon() : signal_set_(), signal_handler_(), proc_name_(""), - pid_file_dir_(DHCP_DATA_DIR), pid_file_() { + pid_file_dir_(DHCP_DATA_DIR), pid_file_(), am_file_author_(false) { + + // The pid_file_dir can be overridden via environment variable + // This is primarily intended to simplify testing + const char* const env = getenv("KEA_PIDFILE_DIR"); + if (env) { + pid_file_dir_ = env; + } } Daemon::~Daemon() { - if (pid_file_) { + if (pid_file_ && am_file_author_) { pid_file_->deleteFile(); } } @@ -191,9 +198,10 @@ Daemon::createPIDFile(int pid) { } // If we find a pre-existing file containing a live PID we bail. - if (pid_file_->check()) { - isc_throw(DaemonPIDExists, "Daemon::createPIDFile " << proc_name_ - << " already running?, PID file: " << getPIDFileName()); + int chk_pid = pid_file_->check(); + if (chk_pid > 0) { + isc_throw(DaemonPIDExists, "Daemon::createPIDFile: PID: " << chk_pid + << " exists, PID file: " << getPIDFileName()); } if (pid == 0) { @@ -203,6 +211,8 @@ Daemon::createPIDFile(int pid) { // Write the PID we were given pid_file_->write(pid); } + + am_file_author_ = true; } }; diff --git a/src/lib/dhcpsrv/daemon.h b/src/lib/dhcpsrv/daemon.h index bd20e14a05..7d8ffe7673 100644 --- a/src/lib/dhcpsrv/daemon.h +++ b/src/lib/dhcpsrv/daemon.h @@ -263,6 +263,9 @@ private: /// @brief Pointer to the PID file for this process isc::util::PIDFilePtr pid_file_; + + /// @brief Flag indicating if this instance created the file + bool am_file_author_; }; }; // end of isc::dhcp namespace diff --git a/src/lib/testutils/dhcp_test_lib.sh.in b/src/lib/testutils/dhcp_test_lib.sh.in index b02e88a261..f81a3a63ed 100644 --- a/src/lib/testutils/dhcp_test_lib.sh.in +++ b/src/lib/testutils/dhcp_test_lib.sh.in @@ -426,6 +426,60 @@ must be a number" kill -${sig} ${_GET_PIDS} } +# Verifies that a server is up running by its PID file +# The PID file is constructed from the given config file name and +# binary name. If it exists and the PID it contains refers to a +# live process it sets _SERVER_PID_FILE and _SERVER_PID to the +# corresponding values. Otherwise, it emits an error and exits. +verify_server_pid() { + local bin_name="${1}" # binary name of the server + local cfg_file="${2}" # config file name + + # We will construct the PID file name based on the server config + # and binary name + if [ -z ${bin_name} ]; then + test_lib_error "verify_server_pid requires binary name" + clean_exit 1 + fi + + if [ -z ${cfg_file} ]; then + test_lib_error "verify_server_pid requires config file name" + clean_exit 1 + fi + + # Only the file name portio of the config file is used, try and + # extract it. NOTE if this "algorithm" changes this code will need + # to be updated. + fname=`basename ${cfg_file}` + fname=`echo $fname | cut -f1 -d'.'` + + if [ -z ${fname} ]; then + test_lib_error "verify_server_pid could not extract config name" + clean_exit 1 + fi + + # Now we can build the name: + pid_file="$KEA_PIDFILE_DIR/$fname.$bin_name.pid" + + if [ ! -e ${pid_file} ]; then + printf "ERROR: PID file:[%s] does not exist\n" ${pid_file} + clean_exit 1 + fi + + # File exists, does its PID point to a live process? + pid=`cat ${pid_file}` + kill -0 ${pid} + if [ $? -ne 0 ]; then + printf "ERROR: PID file:[%s] exists but PID:[%d] does not\n" \ + ${pid_file} ${pid} + clean_exit 1 + fi + + # Make the values accessible to the caller + _SERVER_PID="${pid}" + _SERVER_PID_FILE="${pid_file}" +} + # This test verifies that the binary is reporting its version properly. version_test() { test_name=${1} # Test name diff --git a/src/lib/util/pid_file.cc b/src/lib/util/pid_file.cc index 8f10b0ae1f..9ebd1be73d 100644 --- a/src/lib/util/pid_file.cc +++ b/src/lib/util/pid_file.cc @@ -28,7 +28,7 @@ PIDFile::PIDFile(const std::string& filename) PIDFile::~PIDFile() { } -bool +int PIDFile::check() const { std::ifstream fs(filename_.c_str()); int pid; @@ -53,11 +53,11 @@ PIDFile::check() const { // If the process is still running return true if (kill(pid, 0) == 0) { - return (true); + return (pid); } // No process - return (false); + return (0); } void diff --git a/src/lib/util/pid_file.h b/src/lib/util/pid_file.h index 58bc93dd1b..236b966ecc 100644 --- a/src/lib/util/pid_file.h +++ b/src/lib/util/pid_file.h @@ -63,11 +63,11 @@ public: /// If the file exists but can't be read or it doesn't have /// the proper format treat it as the process existing. /// - /// @return true if the PID is in use, false otherwise + /// @return returns the PID if it is in, 0 otherwise. /// /// @throw throws PIDCantReadPID if it was able to open the file /// but was unable to read the PID from it. - bool check() const; + int check() const; /// @brief Write the PID to the file. /// diff --git a/src/lib/util/tests/pid_file_unittest.cc b/src/lib/util/tests/pid_file_unittest.cc index a0df57d161..cdad04881d 100644 --- a/src/lib/util/tests/pid_file_unittest.cc +++ b/src/lib/util/tests/pid_file_unittest.cc @@ -127,7 +127,7 @@ TEST_F(PIDFileTest, pidInUse) { pid_file.write(); // Check if we think the process is running - EXPECT_TRUE(pid_file.check()); + EXPECT_EQ(getpid(), pid_file.check()); } /// @brief Test checking a PID. Write a PID that isn't in use @@ -148,7 +148,7 @@ TEST_F(PIDFileTest, pidNotInUse) { pid_file.write(pid); // Check to see if we think the process is running - if (!pid_file.check()) { + if (pid_file.check() == 0) { return; } @@ -159,7 +159,7 @@ TEST_F(PIDFileTest, pidNotInUse) { pid_file.write(pid); // Check to see if we think the process is running - EXPECT_FALSE(pid_file.check()); + EXPECT_EQ(0, pid_file.check()); } /// @brief Test checking a PID. Write garbage to the PID file