From 26ddda6791c9d0026c16359ca6906865316c35ca Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 25 Mar 2014 11:25:24 +0100 Subject: [PATCH] [3360] Cleanup in the Memfile backend code. --- doc/guide/bind10-guide.xml | 20 +------ src/lib/dhcpsrv/dhcpsrv_messages.mes | 25 ++++++++ src/lib/dhcpsrv/memfile_lease_mgr.cc | 59 ++++++++++++++----- src/lib/dhcpsrv/memfile_lease_mgr.h | 9 +-- .../dhcpsrv/tests/alloc_engine_unittest.cc | 4 +- .../dhcpsrv/tests/dbaccess_parser_unittest.cc | 6 +- .../tests/memfile_lease_mgr_unittest.cc | 28 ++++++++- 7 files changed, 107 insertions(+), 44 deletions(-) diff --git a/doc/guide/bind10-guide.xml b/doc/guide/bind10-guide.xml index f666411b97..89fc6fa74b 100644 --- a/doc/guide/bind10-guide.xml +++ b/doc/guide/bind10-guide.xml @@ -3714,15 +3714,7 @@ Dhcp4/subnet4 [] list (default) Database Configuration All leases issued by the server are stored in the lease database. Currently, - the only supported database is MySQL - - - The server comes with an in-memory database ("memfile") configured as the default - database. This is used for internal testing and is not supported. In addition, - it does not store lease information on disk: lease information will be lost if the - server is restarted. - - , and so the server must be configured to + the only supported database is MySQL and so the server must be configured to access the correct database with the appropriate credentials. @@ -4879,15 +4871,7 @@ Dhcp6/subnet6/ list Database Configuration All leases issued by the server are stored in the lease database. Currently, - the only supported database is MySQL - - - The server comes with an in-memory database ("memfile") configured as the default - database. This is used for internal testing and is not supported. In addition, - it does not store lease information on disk: lease information will be lost if the - server is restarted. - - , and so the server must be configured to + the only supported database is MySQL, and so the server must be configured to access the correct database with the appropriate credentials. diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index b5ff1881c2..df8fa99dc9 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -279,6 +279,31 @@ subnet ID and hardware address. A debug message issued when the server is about to obtain schema version information from the memory file database. +% DHCPSRV_MEMFILE_LEASE_LOAD4 loading lease %1 +A debug message issued when DHCPv4 lease is being loaded from the file to +memory. + +% DHCPSRV_MEMFILE_LEASE_LOAD6 loading lease %1 +A debug message issued when DHCPv6 lease is being loaded from the file to +memory. + +% DHCPSRV_MEMFILE_LEASES_RELOAD4 reloading leases from %1 +An info message issued when server is about to start reading DHCPv4 leases +from the lease file. All leases currently held in the memory will be +replaced by those read from the file. + +% DHCPSRV_MEMFILE_LEASES_RELOAD6 reloading leases from %1 +An info message issued when server is about to start reading DHCPv6 leases +from the lease file. All leases currently held in the memory will be +replaced by those read from the file. + +% DHCPSRV_MEMFILE_NO_STORAGE running in non-persistent mode, leases will be lost after restart +A warning message issued when writes of leases to disk have been disabled +in the configuration. This mode is useful for some kinds of performance +testing but should not be enabled in normal circumstances. Non-persistence +mode is enabled when 'persist4=no persist6=no' parameters are specified +in the database access string. + % DHCPSRV_MEMFILE_ROLLBACK rolling back memory file database The code has issued a rollback call. For the memory file database, this is a no-op. diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index 03fa124410..64cce0ea2d 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -36,6 +36,14 @@ Memfile_LeaseMgr::Memfile_LeaseMgr(const ParameterMap& parameters) lease_file6_->open(); load6(); } + + // If lease persistence have been disabled for both v4 and v6, + // issue a warning. It is ok not to write leases to disk when + // doing testing, but it should not be done in normal server + // operation. + if (!persistLeases(V4) && !persistLeases(V6)) { + LOG_WARN(dhcpsrv_logger, DHCPSRV_MEMFILE_NO_STORAGE); + } } Memfile_LeaseMgr::~Memfile_LeaseMgr() { @@ -62,7 +70,7 @@ Memfile_LeaseMgr::addLease(const Lease4Ptr& lease) { // Try to write a lease to disk first. If this fails, the lease will // not be inserted to the memory and the disk and in-memory data will // remain consistent. - if (lease_file4_) { + if (persistLeases(V4)) { lease_file4_->append(*lease); } @@ -83,7 +91,7 @@ Memfile_LeaseMgr::addLease(const Lease6Ptr& lease) { // Try to write a lease to disk first. If this fails, the lease will // not be inserted to the memory and the disk and in-memory data will // remain consistent. - if (lease_file6_) { + if (persistLeases(V6)) { lease_file6_->append(*lease); } @@ -290,7 +298,7 @@ Memfile_LeaseMgr::updateLease4(const Lease4Ptr& lease) { // Try to write a lease to disk first. If this fails, the lease will // not be inserted to the memory and the disk and in-memory data will // remain consistent. - if (lease_file4_) { + if (persistLeases(V4)) { lease_file4_->append(*lease); } @@ -311,7 +319,7 @@ Memfile_LeaseMgr::updateLease6(const Lease6Ptr& lease) { // Try to write a lease to disk first. If this fails, the lease will // not be inserted to the memory and the disk and in-memory data will // remain consistent. - if (lease_file6_) { + if (persistLeases(V6)) { lease_file6_->append(*lease); } @@ -329,7 +337,7 @@ Memfile_LeaseMgr::deleteLease(const isc::asiolink::IOAddress& addr) { // No such lease return (false); } else { - if (lease_file4_) { + if (persistLeases(V4)) { // Copy the lease. The valid lifetime needs to be modified and // we don't modify the original lease. Lease4 lease_copy = **l; @@ -349,7 +357,7 @@ Memfile_LeaseMgr::deleteLease(const isc::asiolink::IOAddress& addr) { // No such lease return (false); } else { - if (lease_file6_) { + if (persistLeases(V6)) { // Copy the lease. The lifetimes need to be modified and we // don't modify the original lease. Lease6 lease_copy = **l; @@ -415,17 +423,23 @@ Memfile_LeaseMgr::persistLeases(Universe u) const { std::string Memfile_LeaseMgr::initLeaseFilePath(Universe u) { - bool persist = true; + std::string persist_val; + std::string persist_str = (u == V4 ? "persist4" : "persist6"); try { - std::string persist_str = getParameter("persist"); - if (persist_str == "no") { - persist = false; - } + persist_val = getParameter(persist_str); } catch (const Exception& ex) { - persist = true; + // If parameter persist hasn't been specified, we use a default value + // 'yes'. + persist_val = "yes"; } - if (!persist) { + // If persist_val is 'no' we will not store leases to disk, so let's + // return empty file name. + if (persist_val == "no") { return (""); + + } else if (persist_val != "yes") { + isc_throw(isc::BadValue, "invalid value '" << persist_str + << "=" << persist_val << "'"); } std::string param_name = (u == V4 ? "leasefile4" : "leasefile6"); @@ -442,9 +456,13 @@ void Memfile_LeaseMgr::load4() { // If lease file hasn't been opened, we are working in non-persistent mode. // That's fine, just leave. - if (!lease_file4_) { + if (!persistLeases(V4)) { return; } + + LOG_INFO(dhcpsrv_logger, DHCPSRV_MEMFILE_LEASES_RELOAD4) + .arg(lease_file4_->getFilename()); + // Remove existing leases (if any). We will recreate them based on the // data on disk. storage4_.clear(); @@ -462,6 +480,9 @@ Memfile_LeaseMgr::load4() { // If we got the lease, we update the internal container holding // leases. Otherwise, we reached the end of file and we leave. if (lease) { + LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL_DATA, + DHCPSRV_MEMFILE_LEASE_LOAD4) + .arg(lease->toText()); loadLease4(lease); } } while (lease); @@ -496,9 +517,13 @@ void Memfile_LeaseMgr::load6() { // If lease file hasn't been opened, we are working in non-persistent mode. // That's fine, just leave. - if (!lease_file6_) { + if (!persistLeases(V6)) { return; } + + LOG_INFO(dhcpsrv_logger, DHCPSRV_MEMFILE_LEASES_RELOAD6) + .arg(lease_file6_->getFilename()); + // Remove existing leases (if any). We will recreate them based on the // data on disk. storage6_.clear(); @@ -516,6 +541,10 @@ Memfile_LeaseMgr::load6() { // If we got the lease, we update the internal container holding // leases. Otherwise, we reached the end of file and we leave. if (lease) { + LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL_DATA, + DHCPSRV_MEMFILE_LEASE_LOAD6) + .arg(lease->toText()); + loadLease6(lease); } } while (lease); diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.h b/src/lib/dhcpsrv/memfile_lease_mgr.h index 74a6ad6a0c..58b31313db 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.h +++ b/src/lib/dhcpsrv/memfile_lease_mgr.h @@ -57,10 +57,11 @@ namespace dhcp { /// /// Originally, the Memfile backend didn't write leases to disk. This was /// particularly useful for testing server performance in non-disk bound -/// conditions. In order to preserve this capability, the new parameter -/// "persistence=yes/no" in the data base access string has been introduced. -/// For example, database access string: "type=memfile persistence=no" -/// disables disk writes to disk. +/// conditions. In order to preserve this capability, the new parameters +/// "persist4=yes|no" and "persist6=yes|no" has been introduced in the +/// database access string. For example, database access string: +/// "type=memfile persist4=no persist6=yes" disables disk writes to disk +/// of DHCPv4 leases enables writes to disk of DHCPv6 leases. /// /// The lease file locations can be specified for DHCPv4 and DHCPv6 leases /// with the following two parameters in the database access string: diff --git a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc index 47cbec39c1..4930d32ea9 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc @@ -101,7 +101,7 @@ public: initFqdn("", false, false); - factory_.create("type=memfile persist=no"); + factory_.create("type=memfile persist4=no persist6=no"); } /// @brief Configures a subnet and adds one pool to it. @@ -424,7 +424,7 @@ public: subnet_->addPool(pool_); cfg_mgr.addSubnet4(subnet_); - factory_.create("type=memfile persist=no"); + factory_.create("type=memfile persist4=no persist6=no"); } /// @brief checks if Lease4 matches expected configuration diff --git a/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc b/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc index 7d1620bef4..9dff824cf2 100644 --- a/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc @@ -410,7 +410,8 @@ TEST_F(DbAccessParserTest, commit) { }, isc::dhcp::NoLeaseManager); // Set up the parser to open the memfile database. - const char* config[] = {"type", "memfile", "persist", "no", NULL}; + const char* config[] = {"type", "memfile", "persist4", "no", + "persist6", "no", NULL}; string json_config = toJson(config); ConstElementPtr json_elements = Element::fromJSON(json_config); @@ -420,7 +421,8 @@ TEST_F(DbAccessParserTest, commit) { EXPECT_NO_THROW(parser.build(json_elements)); // Ensure that the access string is as expected. - EXPECT_EQ(std::string("persist=no type=memfile"), parser.getDbAccessString()); + EXPECT_EQ(std::string("persist4=no persist6=no type=memfile"), + parser.getDbAccessString()); // Committal of the parser changes should open the database. EXPECT_NO_THROW(parser.commit()); diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc index 9e1c02ce44..dfaaf29859 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc @@ -113,10 +113,31 @@ public: TEST_F(MemfileLeaseMgrTest, constructor) { LeaseMgr::ParameterMap pmap; - pmap["persist"] = "no"; + pmap["persist4"] = "no"; + pmap["persist6"] = "no"; boost::scoped_ptr lease_mgr; - ASSERT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); + EXPECT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); + + pmap["persist4"] = "yes"; + pmap["persist6"] = "yes"; + pmap["leasefile4"] = getLeaseFilePath("leasefile4_1.csv"); + pmap["leasefile6"] = getLeaseFilePath("leasefile6_1.csv"); + EXPECT_NO_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap))); + + // Expecting that persist parameter is yes or no. Everything other than + // that is wrong. + pmap["persist4"] = "bogus"; + pmap["persist6"] = "yes"; + pmap["leasefile4"] = getLeaseFilePath("leasefile4_1.csv"); + pmap["leasefile6"] = getLeaseFilePath("leasefile6_1.csv"); + EXPECT_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap)), isc::BadValue); + + pmap["persist4"] = "yes"; + pmap["persist6"] = "bogus"; + pmap["leasefile4"] = getLeaseFilePath("leasefile4_1.csv"); + pmap["leasefile6"] = getLeaseFilePath("leasefile6_1.csv"); + EXPECT_THROW(lease_mgr.reset(new Memfile_LeaseMgr(pmap)), isc::BadValue); } // Checks if the getType() and getName() methods both return "memfile". @@ -142,7 +163,8 @@ TEST_F(MemfileLeaseMgrTest, getLeaseFilePath) { EXPECT_EQ(pmap["leasefile6"], lease_mgr->getLeaseFilePath(Memfile_LeaseMgr::V6)); - pmap["persist"] = "no"; + pmap["persist4"] = "no"; + pmap["persist6"] = "no"; lease_mgr.reset(new Memfile_LeaseMgr(pmap)); EXPECT_TRUE(lease_mgr->getLeaseFilePath(Memfile_LeaseMgr::V4).empty()); EXPECT_TRUE(lease_mgr->getLeaseFilePath(Memfile_LeaseMgr::V6).empty()); -- GitLab