From: Francis Dupont Date: Tue, 23 Sep 2025 17:16:22 +0000 (+0200) Subject: [#4090] Checkpoint X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4f1c5b565f6a6078a4032f5fbd79f7ad682cb9a8;p=thirdparty%2Fkea.git [#4090] Checkpoint --- diff --git a/src/bin/lfc/lfc.dox b/src/bin/lfc/lfc.dox index 769a3ff308..e3c788e6ed 100644 --- a/src/bin/lfc/lfc.dox +++ b/src/bin/lfc/lfc.dox @@ -42,7 +42,13 @@ It uses isc::util::PIDFile to manipulate a PID file to mediate access to the leases. When a new process is started it will check the PID file. If the PID file exists and a process with that ID is still running already the new process will be terminated. If no other process is running the PID of the -new process is written to the file. +new process is written to the file. Since kea 3.1.3 this part of the code +was moved to the memfile lease manager which acquires the file lock, +checks running lfc, spawn lfc, write the lfc pid to the PID file and +release the lock. The lfc process waits for the lock and is prepared +to an already existing PID file containing its own pid. Note the lfc +process is still responsible to delete the PID file before existing even +this is not critical. It uses the isc::dhcp::LeaseFileLoader class to first read all of the leases into either isc::dhcp::Lease6Storage or isc::dhcp::Lease4Storage containers. diff --git a/src/bin/lfc/lfc_controller.cc b/src/bin/lfc/lfc_controller.cc index ebe11b3ceb..f9f6d215f2 100644 --- a/src/bin/lfc/lfc_controller.cc +++ b/src/bin/lfc/lfc_controller.cc @@ -84,16 +84,24 @@ LFCController::launch(int argc, char* argv[], const bool test_mode) { try { // Acquire a lock for check and write operations. - PIDLock pid_lock(pid_file.getLockname()); + PIDLock pid_lock(pid_file.getLockname(), true); - if (!pid_lock.isLocked() || pid_file.check()) { + if (!pid_lock.isLocked()) { + isc_throw(Unexpected, "failed to acquire the lock?"); + } + + int existing = pid_file.check(); + + if ((existing != 0) && (existing != getpid())) { // Already running instance, bail out LOG_FATAL(lfc_logger, LFC_RUNNING); return; } // create the pid file for this instance - pid_file.write(); + if (existing == 0) { + pid_file.write(); + } } catch (const PIDFileError& pid_ex) { LOG_FATAL(lfc_logger, LFC_FAIL_PID_CREATE).arg(pid_ex.what()); return; diff --git a/src/bin/lfc/tests/lfc_controller_unittests.cc b/src/bin/lfc/tests/lfc_controller_unittests.cc index 026d3e9355..871fa21ced 100644 --- a/src/bin/lfc/tests/lfc_controller_unittests.cc +++ b/src/bin/lfc/tests/lfc_controller_unittests.cc @@ -8,11 +8,13 @@ #include #include +#include #include #include #include using namespace isc::lfc; +using namespace isc::util; using namespace std; namespace { @@ -106,8 +108,6 @@ protected: void launch(LFCController lfc_controller, int argc, char* argv[]) { lfc_controller.launch(argc, argv, true); } - -private: }; std::string @@ -673,6 +673,77 @@ TEST_F(LFCControllerTest, launch6) { EXPECT_TRUE(noExistIOFP()); } -// @todo double launch (how to do that) +/// @brief Verify that the directory of the pid file must exists. +TEST_F(LFCControllerTest, badPidPath) { + /// LogContentTest does not work with lfc so to see errors + /// set the KEA_LOGGER_DESTINATION env var. + string bad_pid_file("/does/not/exists.pid"); + char* argv[] = { const_cast("progName"), + const_cast("-4"), + const_cast("-x"), + const_cast(xstr_.c_str()), + const_cast("-i"), + const_cast(istr_.c_str()), + const_cast("-o"), + const_cast(ostr_.c_str()), + const_cast("-c"), + const_cast(cstr_.c_str()), + const_cast("-f"), + const_cast(fstr_.c_str()), + const_cast("-p"), + const_cast(bad_pid_file.c_str()) + + }; + int argc = 14; + LFCController lfc_controller; + launch(lfc_controller, argc, argv); +} + +/// @brief Verify that two instances can run at the same time. +/// LogContentTest does not work with lfc so to see errors +/// set the KEA_LOGGER_DESTINATION env var. +TEST_F(LFCControllerTest, alreadyRunning) { + char* argv[] = { const_cast("progName"), + const_cast("-4"), + const_cast("-x"), + const_cast(xstr_.c_str()), + const_cast("-i"), + const_cast(istr_.c_str()), + const_cast("-o"), + const_cast(ostr_.c_str()), + const_cast("-c"), + const_cast(cstr_.c_str()), + const_cast("-f"), + const_cast(fstr_.c_str()), + const_cast("-p"), + const_cast(pstr_.c_str()) + + }; + int argc = 14; + PIDFile pid_file(pstr_); + string lockname = pstr_ + ".lock"; + int parent_pid = getpid(); + int child_pid; + LFCController lfc_controller; + { + PIDLock parent_lock(lockname); + ASSERT_TRUE(parent_lock.isLocked()); + ASSERT_EQ(0, pid_file.check()); + ASSERT_NO_THROW(pid_file.write()); + + child_pid = fork(); + ASSERT_NE(-1, child_pid); + if (child_pid == 0) { + launch(lfc_controller, argc, argv); + _exit(0); + } + } + int status; + // If this stalls add a timeout... + static_cast(waitpid(child_pid, &status, 0)); + EXPECT_EQ(parent_pid, pid_file.check()); +} + +// @todo double launch. } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.cc b/src/lib/dhcpsrv/dhcpsrv_messages.cc index 3f106c6394..30fc1bf0b1 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.cc +++ b/src/lib/dhcpsrv/dhcpsrv_messages.cc @@ -131,9 +131,11 @@ extern const isc::log::MessageID DHCPSRV_MEMFILE_LEASE_FILE_LOAD = "DHCPSRV_MEMF extern const isc::log::MessageID DHCPSRV_MEMFILE_LEASE_LOAD = "DHCPSRV_MEMFILE_LEASE_LOAD"; extern const isc::log::MessageID DHCPSRV_MEMFILE_LEASE_LOAD_ROW_ERROR = "DHCPSRV_MEMFILE_LEASE_LOAD_ROW_ERROR"; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_EXECUTE = "DHCPSRV_MEMFILE_LFC_EXECUTE"; +extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_FAIL_PID_CREATE = "DHCPSRV_MEMFILE_LFC_FAIL_PID_CREATE"; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_LEASE_FILE_RENAME_FAIL = "DHCPSRV_MEMFILE_LFC_LEASE_FILE_RENAME_FAIL"; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_LEASE_FILE_REOPEN_FAIL = "DHCPSRV_MEMFILE_LFC_LEASE_FILE_REOPEN_FAIL"; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_RESCHEDULED = "DHCPSRV_MEMFILE_LFC_RESCHEDULED"; +extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_RUNNING = "DHCPSRV_MEMFILE_LFC_RUNNING"; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_SETUP = "DHCPSRV_MEMFILE_LFC_SETUP"; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_SPAWN_FAIL = "DHCPSRV_MEMFILE_LFC_SPAWN_FAIL"; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_START = "DHCPSRV_MEMFILE_LFC_START"; @@ -312,9 +314,11 @@ const char* values[] = { "DHCPSRV_MEMFILE_LEASE_LOAD", "loading lease %1", "DHCPSRV_MEMFILE_LEASE_LOAD_ROW_ERROR", "discarding row %1, error: %2", "DHCPSRV_MEMFILE_LFC_EXECUTE", "executing Lease File Cleanup using: %1", + "DHCPSRV_MEMFILE_LFC_FAIL_PID_CREATE", "Lease File Cleanup pid file create: %1", "DHCPSRV_MEMFILE_LFC_LEASE_FILE_RENAME_FAIL", "failed to rename the current lease file %1 to %2, reason: %3", "DHCPSRV_MEMFILE_LFC_LEASE_FILE_REOPEN_FAIL", "failed to reopen lease file %1 after preparing input file for lease file cleanup, reason: %2, new leases will not persist!", "DHCPSRV_MEMFILE_LFC_RESCHEDULED", "rescheduled Lease File Cleanup", + "DHCPSRV_MEMFILE_LFC_RUNNING", "Lease File Cleanup instance already running", "DHCPSRV_MEMFILE_LFC_SETUP", "setting up the Lease File Cleanup interval to %1 sec", "DHCPSRV_MEMFILE_LFC_SPAWN_FAIL", "lease file cleanup failed to run because kea-lfc process couldn't be spawned", "DHCPSRV_MEMFILE_LFC_START", "starting Lease File Cleanup", diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.h b/src/lib/dhcpsrv/dhcpsrv_messages.h index 7f6fc855cc..1e88bdaa10 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.h +++ b/src/lib/dhcpsrv/dhcpsrv_messages.h @@ -132,9 +132,11 @@ extern const isc::log::MessageID DHCPSRV_MEMFILE_LEASE_FILE_LOAD; extern const isc::log::MessageID DHCPSRV_MEMFILE_LEASE_LOAD; extern const isc::log::MessageID DHCPSRV_MEMFILE_LEASE_LOAD_ROW_ERROR; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_EXECUTE; +extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_FAIL_PID_CREATE; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_LEASE_FILE_RENAME_FAIL; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_LEASE_FILE_REOPEN_FAIL; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_RESCHEDULED; +extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_RUNNING; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_SETUP; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_SPAWN_FAIL; extern const isc::log::MessageID DHCPSRV_MEMFILE_LFC_START; diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index 94c56d77f9..d5dd3ce7f3 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -744,6 +744,10 @@ This may indicate a corrupt lease file. An informational message issued when the memfile lease database backend starts a new process to perform Lease File Cleanup. +% DHCPSRV_MEMFILE_LFC_FAIL_PID_CREATE Lease File Cleanup pid file create: %1 +This error message is issued if the LFC execute code detects a failure +when trying to create the PID file. It includes a more specific error string. + % DHCPSRV_MEMFILE_LFC_LEASE_FILE_RENAME_FAIL failed to rename the current lease file %1 to %2, reason: %3 An error message logged when the memfile lease database backend fails to move the current lease file to a new file on which the cleanup should @@ -761,6 +765,10 @@ An informational message issued when the memfile lease database backend rescheduled the periodic Lease File Cleanup at the reception of a "kea-lfc-start" command. +% DHCPSRV_MEMFILE_LFC_RUNNING Lease File Cleanup instance already running +This informational message is issued when the LFC execute code detects that +a previous instance of the LFC process is still running via the PID check. + % DHCPSRV_MEMFILE_LFC_SETUP setting up the Lease File Cleanup interval to %1 sec An informational message logged when the memfile lease database backend configures the LFC to be executed periodically. The argument holds the diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index c387b118ca..e35f4dfa31 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -101,7 +101,7 @@ public: bool run_once_now = false); /// @brief Spawns a new process. - void execute(); + void execute(const std::string& lease_file); /// @brief Checks if the lease file cleanup is in progress. /// @@ -111,6 +111,9 @@ public: /// @brief Returns exit code of the last completed cleanup. int getExitStatus() const; + /// @brief Returns pid of the last lease file cleanup. + int getLastPid() const; + private: /// @brief A pointer to the @c ProcessSpawn object used to execute @@ -231,14 +234,74 @@ LFCSetup::setup(const uint32_t lfc_interval, } void -LFCSetup::execute() { +LFCSetup::execute(const std::string& lease_file) { + PIDFile pid_file(Memfile_LeaseMgr::appendSuffix(lease_file, + Memfile_LeaseMgr::FILE_PID)); try { + // Look at lfc.dox for a description of this. + + // Try to recover a deleted pid file. + bool is_running = false; + if (pid_ != 0) { + try { + is_running = isRunning(); + } catch (...) { + // Ignore errors. + } + } + // Do not trust the process spawn isRunning method. + if (is_running && (kill(pid_, 0) != 0)) { + is_running = false; + } + + // Try to acquire the lock for the pid file. + PIDLock pid_lock(pid_file.getLockname()); + + // Verify that no lfc is still running. + if (is_running || !pid_lock.isLocked() || pid_file.check()) { + LOG_INFO(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_RUNNING); + if (is_running && pid_lock.isLocked() && !pid_file.check()) { + // The pid file was deleted and the process is still running! + pid_file.write(pid_); + } + return; + } + + // Cleanup the state (memory leak fix). + if (pid_ != 0) { + try { + process_->clearState(pid_); + } catch (...) { + // Ignore errors (and keep a possible slow memory leak). + } + } + // Reset pid to -1. + pid_ = -1; + + // Check the pid file is writable. + pid_file.write(0); + LOG_INFO(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_EXECUTE) .arg(process_->getCommandLine()); pid_ = process_->spawn(); + // Write the pid of the child in the pid file. + pid_file.write(pid_); + + } catch (const PIDFileError& ex) { + LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_FAIL_PID_CREATE) + .arg(ex.what()); } catch (const ProcessSpawnError&) { LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_SPAWN_FAIL); + // Reacquire the lock to remove the pid file. + PIDLock pid_lock(pid_file.getLockname()); + if (pid_lock.isLocked() && !pid_file.check()) { + try { + pid_file.deleteFile(); + } catch (...) { + // Ignore errors. + } + } } } @@ -256,6 +319,10 @@ LFCSetup::getExitStatus() const { return (process_->getExitStatus(pid_)); } +int +LFCSetup::getLastPid() const { + return (pid_); +} /// @brief Base Memfile derivation of the statistical lease data query /// @@ -2468,7 +2535,6 @@ Memfile_LeaseMgr::loadLeasesFromFiles(Universe u, const std::string& filename, return (conversion_needed); } - bool Memfile_LeaseMgr::isLFCRunning() const { return (lfc_setup_->isRunning()); @@ -2479,6 +2545,11 @@ Memfile_LeaseMgr::getLFCExitStatus() const { return (lfc_setup_->getExitStatus()); } +int +Memfile_LeaseMgr::getLFCLastPid() const { + return (lfc_setup_->getLastPid()); +} + void Memfile_LeaseMgr::lfcCallback() { // Check if we're in the v4 or v6 space and use the appropriate file. @@ -2571,7 +2642,7 @@ Memfile_LeaseMgr::lfcExecute(boost::shared_ptr& lease_file) { // Once the files have been rotated, or untouched if another LFC had // not finished, a new process is started. if (do_lfc) { - lfc_setup_->execute(); + lfc_setup_->execute(lease_file->getFilename()); } } @@ -2590,12 +2661,20 @@ Memfile_LeaseMgr::lfcStartHandler() { TimerMgr::instance()->setup("memfile-lfc"); LOG_INFO(dhcpsrv_logger, DHCPSRV_MEMFILE_LFC_RESCHEDULED); } + int previous_pid = getLFCLastPid(); if (lease_file4_) { lfcExecute(lease_file4_); } else if (lease_file6_) { lfcExecute(lease_file6_); } - return (createAnswer(CONTROL_RESULT_SUCCESS, "kea-lfc started")); + int new_pid = getLFCLastPid(); + if (new_pid == -1) { + return (createAnswer(CONTROL_RESULT_ERROR, "failed to start kea-lfc")); + } else if (new_pid != previous_pid) { + return (createAnswer(CONTROL_RESULT_SUCCESS, "kea-lfc started")); + } else { + return (createAnswer(CONTROL_RESULT_EMPTY, "kea-lfc already running")); + } } LeaseStatsQueryPtr diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.h b/src/lib/dhcpsrv/memfile_lease_mgr.h index 0c6e67a639..e2eec15f47 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.h +++ b/src/lib/dhcpsrv/memfile_lease_mgr.h @@ -1168,6 +1168,9 @@ public: /// @brief Returns the status code returned by the last executed /// LFC process. int getLFCExitStatus() const; + + /// @brief Returns the last lfc process id. + int getLFCLastPid() const; //@} /// @brief Creates and runs the IPv4 lease stats query diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc index 868c68b216..c0d3abd8cf 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc @@ -4598,6 +4598,10 @@ public: removeFiles(getLeaseFilePath("leasefile6_0.csv")); // Remove other files. removeOtherFiles(); + // Remove pid file. + if (pid_file_) { + static_cast(remove(pid_file_->getFilename().c_str())); + } // Disable multi-threading. MultiThreadingMgr::instance().setMode(false); @@ -4612,6 +4616,8 @@ public: /// @brief Stores the pre-test DHCP data directory. std::string original_datadir_; + + boost::scoped_ptr pid_file_; }; /// @brief Verifies that a warning is logged when given @@ -4679,7 +4685,7 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerLfcInterval0) { pmap["name"] = getLeaseFilePath("leasefile6_0.csv"); boost::scoped_ptr lease_mgr; - // Persist is false so there is no lease file... + // Persist is true so there is a lease file. ASSERT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); ASSERT_TRUE(lease_mgr); ConstElementPtr response; @@ -4695,6 +4701,11 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerLfcInterval0) { EXPECT_EQ(0, lease_mgr->getLFCExitStatus()) << "environment not available to LFC"; + // Pid file should no longer exist. + pid_file_.reset(new PIDFile(Memfile_LeaseMgr::appendSuffix(pmap["name"], + Memfile_LeaseMgr::FILE_PID))); + EXPECT_FALSE(file::exists(pid_file_->getFilename())); + // No reschedule. string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED "; msg += "rescheduled Lease File Cleanup"; @@ -4715,7 +4726,7 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandler) { pmap["name"] = getLeaseFilePath("leasefile4_0.csv"); boost::scoped_ptr lease_mgr; - // Persist is false so there is no lease file... + // Persist is true so there is a lease file. ASSERT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); ASSERT_TRUE(lease_mgr); ConstElementPtr response; @@ -4731,6 +4742,139 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandler) { EXPECT_EQ(0, lease_mgr->getLFCExitStatus()) << "environment not available to LFC"; + // Pid file should no longer exist. + pid_file_.reset(new PIDFile(Memfile_LeaseMgr::appendSuffix(pmap["name"], + Memfile_LeaseMgr::FILE_PID))); + EXPECT_FALSE(file::exists(pid_file_->getFilename())); + + // Reschedule. + string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED "; + msg += "rescheduled Lease File Cleanup"; + EXPECT_EQ(1, countFile(msg)); + + // Start. + msg = "DHCPSRV_MEMFILE_LFC_START "; + msg += "starting Lease File Cleanup"; + EXPECT_EQ(1, countFile(msg)); +} + +/// @brief Verifies that lfcStartHandler reschedules and but not start lfc +/// if the pid file lock is already taken. +TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerLocked) { + DatabaseConnection::ParameterMap pmap; + pmap["universe"] = "6"; + pmap["persist"] = "true"; + pmap["lfc-interval"] = "10"; + pmap["name"] = getLeaseFilePath("leasefile6_0.csv"); + boost::scoped_ptr lease_mgr; + + // Persist is true so there is a lease file. + ASSERT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); + ASSERT_TRUE(lease_mgr); + + // Acquire the pid file lock. + pid_file_.reset(new PIDFile(Memfile_LeaseMgr::appendSuffix(pmap["name"], + Memfile_LeaseMgr::FILE_PID))); + PIDLock pid_lock(pid_file_->getLockname()); + ASSERT_TRUE(pid_lock.isLocked()); + + ConstElementPtr response; + EXPECT_NO_THROW(response = lease_mgr->lfcStartHandler()); + ASSERT_TRUE(response); + string expected = "{ \"result\": 3, \"text\": \"kea-lfc already running\" }"; + EXPECT_EQ(expected, response->str()); + + // Reschedule. + string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED "; + msg += "rescheduled Lease File Cleanup"; + EXPECT_EQ(1, countFile(msg)); + + // Start. + msg = "DHCPSRV_MEMFILE_LFC_START "; + msg += "starting Lease File Cleanup"; + EXPECT_EQ(1, countFile(msg)); + + // Locked. + msg = "DHCPSRV_MEMFILE_LFC_RUNNING "; + msg += "Lease File Cleanup instance already running"; + EXPECT_EQ(1, countFile(msg)); +} + +/// @brief Verifies that lfcStartHandler reschedules and but not start lfc +/// if the pid file check says lfc is already running. +TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerAlreadyRunning) { + DatabaseConnection::ParameterMap pmap; + pmap["universe"] = "4"; + pmap["persist"] = "true"; + pmap["lfc-interval"] = "10"; + pmap["name"] = getLeaseFilePath("leasefile4_0.csv"); + boost::scoped_ptr lease_mgr; + + // Persist is true so there is a lease file. + ASSERT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); + ASSERT_TRUE(lease_mgr); + + // Write the pid file. + pid_file_.reset(new PIDFile(Memfile_LeaseMgr::appendSuffix(pmap["name"], + Memfile_LeaseMgr::FILE_PID))); + { + PIDLock pid_lock(pid_file_->getLockname()); + ASSERT_TRUE(pid_lock.isLocked()); + pid_file_->write(); + } + + ConstElementPtr response; + EXPECT_NO_THROW(response = lease_mgr->lfcStartHandler()); + ASSERT_TRUE(response); + string expected = "{ \"result\": 3, \"text\": \"kea-lfc already running\" }"; + EXPECT_EQ(expected, response->str()); + + // Reschedule. + string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED "; + msg += "rescheduled Lease File Cleanup"; + EXPECT_EQ(1, countFile(msg)); + + // Start. + msg = "DHCPSRV_MEMFILE_LFC_START "; + msg += "starting Lease File Cleanup"; + EXPECT_EQ(1, countFile(msg)); + + // Already running. + msg = "DHCPSRV_MEMFILE_LFC_RUNNING "; + msg += "Lease File Cleanup instance already running"; + EXPECT_EQ(1, countFile(msg)); +} + +/// @brief Verifies that lfcStartHandler reschedules and but not start lfc +/// if the pid file is not writable. +TEST_F(MemfileLeaseMgrLogTest, lfcStartHandlerPidNotWritable) { + DatabaseConnection::ParameterMap pmap; + pmap["universe"] = "6"; + pmap["persist"] = "true"; + pmap["lfc-interval"] = "10"; + pmap["name"] = getLeaseFilePath("leasefile6_0.csv"); + boost::scoped_ptr lease_mgr; + + // Persist is true so there is a lease file. + ASSERT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); + ASSERT_TRUE(lease_mgr); + + // Write the pid file and change it mode to read only. + pid_file_.reset(new PIDFile(Memfile_LeaseMgr::appendSuffix(pmap["name"], + Memfile_LeaseMgr::FILE_PID))); + { + PIDLock pid_lock(pid_file_->getLockname()); + ASSERT_TRUE(pid_lock.isLocked()); + pid_file_->write(0); + static_cast(chmod(pid_file_->getFilename().c_str(), 0400)); + } + + ConstElementPtr response; + EXPECT_NO_THROW(response = lease_mgr->lfcStartHandler()); + ASSERT_TRUE(response); + string expected = "{ \"result\": 1, \"text\": \"failed to start kea-lfc\" }"; + EXPECT_EQ(expected, response->str()); + // Reschedule. string msg = "DHCPSRV_MEMFILE_LFC_RESCHEDULED "; msg += "rescheduled Lease File Cleanup"; @@ -4740,6 +4884,16 @@ TEST_F(MemfileLeaseMgrLogTest, lfcStartHandler) { msg = "DHCPSRV_MEMFILE_LFC_START "; msg += "starting Lease File Cleanup"; EXPECT_EQ(1, countFile(msg)); + + // PID file is not writable. + msg = "DHCPSRV_MEMFILE_LFC_FAIL_PID_CREATE "; + msg += "Lease File Cleanup pid file create: "; + msg += "Unable to open PID file '"; + msg += pid_file_->getFilename(); + msg += "' for write"; + EXPECT_EQ(1, countFile(msg)); } +/// Check ProcessSpawnError (no known way to trigger it). + } // namespace diff --git a/src/lib/util/pid_file.cc b/src/lib/util/pid_file.cc index 085ffd0381..85de5fdd26 100644 --- a/src/lib/util/pid_file.cc +++ b/src/lib/util/pid_file.cc @@ -86,12 +86,12 @@ PIDFile::deleteFile() const { } } -PIDLock::PIDLock(const std::string& lockname) + PIDLock::PIDLock(const std::string& lockname, bool blocking) : lockname_(lockname), fd_(-1), locked_(false) { // Open the lock file. fd_ = open(lockname_.c_str(), O_RDONLY | O_CREAT, 0600); if (fd_ == -1) { - if (errno == ENOENT) { + if ((errno == ENOENT) && !blocking) { // Ignoring missing component in the path. locked_ = true; return; @@ -102,7 +102,7 @@ PIDLock::PIDLock(const std::string& lockname) } // Try to acquire the lock. If we can't somebody else is actively // using it. - int ret = flock(fd_, LOCK_EX | LOCK_NB); + int ret = flock(fd_, LOCK_EX | (blocking ? 0 : LOCK_NB)); if (ret == 0) { locked_ = true; return; diff --git a/src/lib/util/pid_file.h b/src/lib/util/pid_file.h index d228ae0ddd..961a8c390d 100644 --- a/src/lib/util/pid_file.h +++ b/src/lib/util/pid_file.h @@ -107,10 +107,11 @@ private: typedef boost::shared_ptr PIDFilePtr; /// @brief RAII device to handle a lock file to avoid race conditions. -/// @note If there is a missing component in the path the lock is considered -/// as being acquired: this does not change the behavior of PIDFile methods -/// (check() will succeed and write() throw) and does not require unit test -/// updates as if the constructor throws. +/// @note If there is a missing component in the path and the lock is +/// not blocking the lock is considered as being acquired: this does not +/// change the behavior of PIDFile methods (check() will succeed and +/// write() throw) and does not require unit test updates as if the +/// constructor throws. class PIDLock { public: /// @brief Constructor @@ -118,7 +119,8 @@ public: /// Try to get a lock. /// /// @param lockname Lock filename. - PIDLock(const std::string& lockname); + /// @param blocking If true (default is false) block when already locked. + PIDLock(const std::string& lockname, bool blocking = false); /// @brief Destructor /// diff --git a/src/lib/util/tests/pid_file_unittest.cc b/src/lib/util/tests/pid_file_unittest.cc index 09b3bd1adf..182219d5ce 100644 --- a/src/lib/util/tests/pid_file_unittest.cc +++ b/src/lib/util/tests/pid_file_unittest.cc @@ -243,6 +243,8 @@ TEST_F(PIDFileTest, lockNoent) { ASSERT_NO_THROW(lock.reset(new PIDLock("/does/not/exist.lock"))); ASSERT_TRUE(lock); EXPECT_TRUE(lock->isLocked()); + EXPECT_THROW(lock.reset(new PIDLock("/does/not/exist.lock", true)), + PIDFileError); } } // end of anonymous namespace