[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.

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.

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 '?': {

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 {

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
 

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

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);

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

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;
 }
 
 };

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

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

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

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.
     ///

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