[3360] Cleanup in the Memfile backend code.

doc/guide/bind10-guide.xml

@@ -3714,15 +3714,7 @@ Dhcp4/subnet4	[]	list	(default)
       <title>Database Configuration</title>
       <para>
       All leases issued by the server are stored in the lease database.  Currently,
-      the only supported database is MySQL
-      <footnote>
-      <para>
-      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.
-      </para>
-      </footnote>, 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.
       </para>
         <note>
@@ -4879,15 +4871,7 @@ Dhcp6/subnet6/	list
       <title>Database Configuration</title>
       <para>
       All leases issued by the server are stored in the lease database.  Currently,
-      the only supported database is MySQL
-      <footnote>
-      <para>
-      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.
-      </para>
-      </footnote>, 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.
       </para>
       <note>

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.

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

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:

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

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

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