[#3652] Fix memory increase on reconfiguration

Caused by config history. The config history was soft-disabled.
Code had to be refactored to enable CONFIG_LIST_SIZE == 0.

5 files changed, 43 insertions, 28 deletions

diff --git a/src/lib/dhcpsrv/cfgmgr.cc b/src/lib/dhcpsrv/cfgmgr.cc
index ef156dd22c..cdd3c307ea 100644
--- a/src/lib/dhcpsrv/cfgmgr.cc
+++ b/src/lib/dhcpsrv/cfgmgr.cc
@@ -19,7 +19,9 @@ using namespace isc::util;
 namespace isc {
 namespace dhcp {
 
-const size_t CfgMgr::CONFIG_LIST_SIZE = 10;
+// Value 0 effectively disables configuration history. Higher values can lead to proportionally
+// increased memory usage which can be extreme in case of sizable configurations.
+const size_t CfgMgr::CONFIG_LIST_SIZE = 0;
 
 CfgMgr&
 CfgMgr::instance() {
@@ -69,7 +71,7 @@ CfgMgr::getD2ClientMgr() {
 
 void
 CfgMgr::ensureCurrentAllocated() {
-    if (!configuration_ || configs_.empty()) {
+    if (!configuration_) {
         configuration_.reset(new SrvConfig());
         configs_.push_back(configuration_);
     }
@@ -79,6 +81,7 @@ void
 CfgMgr::clear() {
     if (configuration_) {
         configuration_->removeStatistics();
+        configuration_.reset();
     }
     configs_.clear();
     external_configs_.clear();
@@ -92,18 +95,14 @@ CfgMgr::commit() {
 
     // First we need to remove statistics. The new configuration can have fewer
     // subnets. Also, it may change subnet-ids. So we need to remove them all
-    // and add it back.
+    // and add them back.
     configuration_->removeStatistics();
 
-    if (!configs_.back()->sequenceEquals(*configuration_)) {
+    bool promoted(false);
+    if (!configs_.empty() && !configs_.back()->sequenceEquals(*configuration_)) {
+        // Promote the staging configuration to the current configuration.
         configuration_ = configs_.back();
-        // Keep track of the maximum size of the configs history. Before adding
-        // new element, we have to remove the oldest one.
-        if (configs_.size() > CONFIG_LIST_SIZE) {
-            SrvConfigList::iterator it = configs_.begin();
-            std::advance(it, configs_.size() - CONFIG_LIST_SIZE);
-            configs_.erase(configs_.begin(), it);
-        }
+        promoted = true;
     }
 
     // Set the last commit timestamp.
@@ -114,12 +113,21 @@ CfgMgr::commit() {
     configuration_->updateStatistics();
 
     configuration_->configureLowerLevelLibraries();
+
+    if (promoted) {
+        // Keep track of the maximum size of the configs history. Remove the oldest elements.
+        if (configs_.size() > CONFIG_LIST_SIZE) {
+            SrvConfigList::const_iterator it = configs_.begin();
+            std::advance(it, configs_.size() - CONFIG_LIST_SIZE);
+            configs_.erase(configs_.begin(), it);
+        }
+    }
 }
 
 void
 CfgMgr::rollback() {
     ensureCurrentAllocated();
-    if (!configuration_->sequenceEquals(*configs_.back())) {
+    if (!configs_.empty() && !configuration_->sequenceEquals(*configs_.back())) {
         configs_.pop_back();
     }
 }
@@ -130,9 +138,9 @@ CfgMgr::revert(const size_t index) {
     if (index == 0) {
         isc_throw(isc::OutOfRange, "invalid commit index 0 when reverting"
                   " to an old configuration");
-    } else if (index > configs_.size() - 1) {
+    } else if (configs_.size() <= index) {
         isc_throw(isc::OutOfRange, "unable to revert to commit index '"
-                  << index << "', only '" << configs_.size() - 1
+                  << index << "', only '" << (configs_.size() == 0 ? 0 : configs_.size() - 1)
                   << "' previous commits available");
     }
 
@@ -143,18 +151,20 @@ CfgMgr::revert(const size_t index) {
     // configuration is destroyed by this rollback.
     rollback();
 
-    // Get the iterator to the current configuration and then advance to the
-    // desired one.
-    SrvConfigList::const_reverse_iterator it = configs_.rbegin();
-    std::advance(it, index);
+    if (!configs_.empty()) {
+        // Get the iterator to the current configuration and then advance to the
+        // desired one.
+        SrvConfigList::const_reverse_iterator it = configs_.rbegin();
+        std::advance(it, index);
 
-    // Copy the desired configuration to the new staging configuration. The
-    // staging configuration is re-created here because we rolled back earlier
-    // in this function.
-    (*it)->copy(*getStagingCfg());
+        // Copy the desired configuration to the new staging configuration. The
+        // staging configuration is re-created here because we rolled back earlier
+        // in this function.
+        (*it)->copy(*getStagingCfg());
 
-    // Make the staging configuration a current one.
-    commit();
+        // Make the staging configuration a current one.
+        commit();
+    }
 }
 
 SrvConfigPtr
@@ -166,7 +176,7 @@ CfgMgr::getCurrentCfg() {
 SrvConfigPtr
 CfgMgr::getStagingCfg() {
     ensureCurrentAllocated();
-    if (configuration_->sequenceEquals(*configs_.back())) {
+    if (configs_.empty() || configuration_->sequenceEquals(*configs_.back())) {
         uint32_t sequence = configuration_->getSequence();
         configs_.push_back(SrvConfigPtr(new SrvConfig(++sequence)));
     }
diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h
index a3aa4ccbf4..d5492ed29b 100644
--- a/src/lib/dhcpsrv/cfgmgr.h
+++ b/src/lib/dhcpsrv/cfgmgr.h
@@ -150,7 +150,7 @@ public:
     /// @brief Commits the staging configuration.
     ///
     /// The staging configuration becomes current configuration when this
-    /// function is called. It removes the oldest configuration held in the
+    /// function is called. It removes the oldest configurations held in the
     /// history so as the size of the list of configuration does not exceed
     /// the @c CONFIG_LIST_SIZE.
     ///
diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc
index 54c42f6e9e..115d58550c 100644
--- a/src/lib/dhcpsrv/srv_config.cc
+++ b/src/lib/dhcpsrv/srv_config.cc
@@ -412,7 +412,7 @@ SrvConfig::updateStatistics() {
     // a lease manager, such as D2. @todo We should probably examine why
     // "SrvConfig" is being used by D2.
     if (LeaseMgrFactory::haveInstance()) {
-        // Updates  statistics for v4 and v6 subnets
+        // Updates statistics for v4 and v6 subnets.
         getCfgSubnets4()->updateStatistics();
         getCfgSubnets6()->updateStatistics();
     }
diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
index 44699638f3..e6caa3ae55 100644
--- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
@@ -543,6 +543,10 @@ TEST_F(CfgMgrTest, revert) {
     // Value of 0 also doesn't make sense.
     ASSERT_THROW(cfg_mgr.revert(0), isc::OutOfRange);
 
+    // Return early because the checks that follow simply are not supported by
+    // CONFIG_LIST_SIZE == 0 at the time of writing.
+    return;
+
     // We should be able to revert to configuration with debuglevel = 10.
     ASSERT_NO_THROW(cfg_mgr.revert(4));
     // And this configuration should be now the current one and the debuglevel
diff --git a/src/lib/dhcpsrv/testutils/alloc_engine_utils.cc b/src/lib/dhcpsrv/testutils/alloc_engine_utils.cc
index 21ef2ba6b7..910e98dc0a 100644
--- a/src/lib/dhcpsrv/testutils/alloc_engine_utils.cc
+++ b/src/lib/dhcpsrv/testutils/alloc_engine_utils.cc
@@ -17,6 +17,7 @@
 #include <hooks/hooks_manager.h>
 #include <hooks/callout_handle.h>
 #include <stats/stats_mgr.h>
+#include <testutils/gtest_utils.h>
 
 #include <dhcpsrv/testutils/test_utils.h>
 #include <dhcpsrv/testutils/alloc_engine_utils.h>
@@ -642,7 +643,7 @@ AllocEngine4Test::initSubnet(const asiolink::IOAddress& pool_start,
     pool_ = Pool4Ptr(new Pool4(pool_start, pool_end));
     subnet_->addPool(pool_);
 
-    cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_);
+    EXPECT_NO_THROW_LOG(cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_));
 }
 
 AllocEngine4Test::AllocEngine4Test() {