[#892] addressed review comments

src/bin/dhcp4/ctrl_dhcp4_srv.cc

@@ -90,9 +90,10 @@ ControlledDhcpv4Srv::init(const std::string& file_name) {
 
     // Configure the server using JSON file.
     ConstElementPtr result = loadConfigFile(file_name);
+
     int rcode;
     ConstElementPtr comment = isc::config::parseAnswer(rcode, result);
-    if (rcode != 0) {
+    if (rcode != CONTROL_RESULT_SUCCESS) {
         string reason = comment ? comment->stringValue() :
             "no details available";
         isc_throw(isc::BadValue, reason);
@@ -176,7 +177,7 @@ ControlledDhcpv4Srv::loadConfigFile(const std::string& file_name) {
         // (see @ref isc::config::parseAnswer).
         int rcode;
         ConstElementPtr comment = isc::config::parseAnswer(rcode, result);
-        if (rcode != 0) {
+        if (rcode != CONTROL_RESULT_SUCCESS) {
             string reason = comment ? comment->stringValue() :
                 "no details available";
             isc_throw(isc::BadValue, reason);
@@ -192,10 +193,14 @@ ControlledDhcpv4Srv::loadConfigFile(const std::string& file_name) {
                   << file_name << "': " << ex.what());
     }
 
+    LOG_INFO(dhcp4_logger, DHCP4_MULTI_THREADING_INFO)
+        .arg(MultiThreadingMgr::instance().getMode())
+        .arg(MultiThreadingMgr::instance().getPktThreadPoolSize())
+        .arg(CfgMgr::instance().getCurrentCfg()->getServerMaxThreadQueueSize());
+
     return (result);
 }
 
-
 ConstElementPtr
 ControlledDhcpv4Srv::commandShutdownHandler(const string&, ConstElementPtr) {
     if (ControlledDhcpv4Srv::getInstance()) {
@@ -896,18 +901,18 @@ ControlledDhcpv4Srv::ControlledDhcpv4Srv(uint16_t server_port /*= DHCP4_SERVER_P
     CommandMgr::instance().registerCommand("config-write",
         boost::bind(&ControlledDhcpv4Srv::commandConfigWriteHandler, this, _1, _2));
 
-    CommandMgr::instance().registerCommand("dhcp-enable",
-        boost::bind(&ControlledDhcpv4Srv::commandDhcpEnableHandler, this, _1, _2));
-
     CommandMgr::instance().registerCommand("dhcp-disable",
         boost::bind(&ControlledDhcpv4Srv::commandDhcpDisableHandler, this, _1, _2));
 
-    CommandMgr::instance().registerCommand("libreload",
+    CommandMgr::instance().registerCommand("dhcp-enable",
-        boost::bind(&ControlledDhcpv4Srv::commandLibReloadHandler, this, _1, _2));
+        boost::bind(&ControlledDhcpv4Srv::commandDhcpEnableHandler, this, _1, _2));
 
     CommandMgr::instance().registerCommand("leases-reclaim",
         boost::bind(&ControlledDhcpv4Srv::commandLeasesReclaimHandler, this, _1, _2));
 
+    CommandMgr::instance().registerCommand("libreload",
+        boost::bind(&ControlledDhcpv4Srv::commandLibReloadHandler, this, _1, _2));
+
     CommandMgr::instance().registerCommand("server-tag-get",
         boost::bind(&ControlledDhcpv4Srv::commandServerTagGetHandler, this, _1, _2));
 
@@ -924,18 +929,18 @@ ControlledDhcpv4Srv::ControlledDhcpv4Srv(uint16_t server_port /*= DHCP4_SERVER_P
     CommandMgr::instance().registerCommand("statistic-get",
         boost::bind(&StatsMgr::statisticGetHandler, _1, _2));
 
-    CommandMgr::instance().registerCommand("statistic-reset",
-        boost::bind(&StatsMgr::statisticResetHandler, _1, _2));
-
-    CommandMgr::instance().registerCommand("statistic-remove",
-        boost::bind(&StatsMgr::statisticRemoveHandler, _1, _2));
-
     CommandMgr::instance().registerCommand("statistic-get-all",
         boost::bind(&StatsMgr::statisticGetAllHandler, _1, _2));
 
+    CommandMgr::instance().registerCommand("statistic-reset",
+        boost::bind(&StatsMgr::statisticResetHandler, _1, _2));
+
     CommandMgr::instance().registerCommand("statistic-reset-all",
         boost::bind(&StatsMgr::statisticResetAllHandler, _1, _2));
 
+    CommandMgr::instance().registerCommand("statistic-remove",
+        boost::bind(&StatsMgr::statisticRemoveHandler, _1, _2));
+
     CommandMgr::instance().registerCommand("statistic-remove-all",
         boost::bind(&StatsMgr::statisticRemoveAllHandler, _1, _2));
 

src/bin/dhcp4/dhcp4_messages.cc

@@ -1,4 +1,4 @@
-// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Fri Jan 31 2020 15:04
+// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Wed Feb 19 2020 16:53
 
 #include <cstddef>
 #include <log/message_types.h>
@@ -81,6 +81,7 @@ extern const isc::log::MessageID DHCP4_INIT_FAIL = "DHCP4_INIT_FAIL";
 extern const isc::log::MessageID DHCP4_INIT_REBOOT = "DHCP4_INIT_REBOOT";
 extern const isc::log::MessageID DHCP4_LEASE_ADVERT = "DHCP4_LEASE_ADVERT";
 extern const isc::log::MessageID DHCP4_LEASE_ALLOC = "DHCP4_LEASE_ALLOC";
+extern const isc::log::MessageID DHCP4_MULTI_THREADING_INFO = "DHCP4_MULTI_THREADING_INFO";
 extern const isc::log::MessageID DHCP4_NCR_CREATE = "DHCP4_NCR_CREATE";
 extern const isc::log::MessageID DHCP4_NCR_CREATION_FAILED = "DHCP4_NCR_CREATION_FAILED";
 extern const isc::log::MessageID DHCP4_NOT_RUNNING = "DHCP4_NOT_RUNNING";
@@ -223,6 +224,7 @@ const char* values[] = {
     "DHCP4_INIT_REBOOT", "%1: client is in INIT-REBOOT state and requests address %2",
     "DHCP4_LEASE_ADVERT", "%1: lease %2 will be advertised",
     "DHCP4_LEASE_ALLOC", "%1: lease %2 has been allocated for %3 seconds",
+    "DHCP4_MULTI_THREADING_INFO", "enabled: %1, number of threads: %2, queue size per thread: %3",
     "DHCP4_NCR_CREATE", "%1: DDNS updates enabled, therefore sending name change requests",
     "DHCP4_NCR_CREATION_FAILED", "%1: failed to generate name change requests for DNS: %2",
     "DHCP4_NOT_RUNNING", "DHCPv4 server is not running",

src/bin/dhcp4/dhcp4_messages.h

@@ -1,4 +1,4 @@
-// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Fri Jan 31 2020 15:04
+// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Wed Feb 19 2020 16:53
 
 #ifndef DHCP4_MESSAGES_H
 #define DHCP4_MESSAGES_H
@@ -82,6 +82,7 @@ extern const isc::log::MessageID DHCP4_INIT_FAIL;
 extern const isc::log::MessageID DHCP4_INIT_REBOOT;
 extern const isc::log::MessageID DHCP4_LEASE_ADVERT;
 extern const isc::log::MessageID DHCP4_LEASE_ALLOC;
+extern const isc::log::MessageID DHCP4_MULTI_THREADING_INFO;
 extern const isc::log::MessageID DHCP4_NCR_CREATE;
 extern const isc::log::MessageID DHCP4_NCR_CREATION_FAILED;
 extern const isc::log::MessageID DHCP4_NOT_RUNNING;

src/bin/dhcp4/dhcp4_messages.mes

@@ -806,6 +806,10 @@ This is a debug message issued during the DHCPv4 server startup.
 It lists some information about the parameters with which the server
 is running.
 
+% DHCP4_MULTI_THREADING_INFO enabled: %1, number of threads: %2, queue size per thread: %3
+This is a message listing some information about the multi-threading parameters
+with which the server is running.
+
 % DHCP4_SUBNET_DATA %1: the selected subnet details: %2
 This debug message includes the details of the subnet selected for
 the client. The first argument includes the client and the

src/bin/dhcp6/ctrl_dhcp6_srv.cc

@@ -108,8 +108,8 @@ ControlledDhcpv6Srv::loadConfigFile(const std::string& file_name) {
     try {
         if (file_name.empty()) {
             // Basic sanity check: file name must not be empty.
-            isc_throw(isc::BadValue, "JSON configuration file not specified. Please "
+            isc_throw(isc::BadValue, "JSON configuration file not specified."
-                      "use -c command line option.");
+                      " Please use -c command line option.");
         }
 
         // Read contents of the file and parse it as JSON
@@ -147,9 +147,8 @@ ControlledDhcpv6Srv::loadConfigFile(const std::string& file_name) {
         // Now check is the returned result is successful (rcode=0) or not
         // (see @ref isc::config::parseAnswer).
         int rcode;
-        isc::data::ConstElementPtr comment =
+        ConstElementPtr comment = isc::config::parseAnswer(rcode, result);
-            isc::config::parseAnswer(rcode, result);
+        if (rcode != CONTROL_RESULT_SUCCESS) {
-        if (rcode != 0) {
             string reason = comment ? comment->stringValue() :
                 "no details available";
             isc_throw(isc::BadValue, reason);
@@ -165,6 +164,11 @@ ControlledDhcpv6Srv::loadConfigFile(const std::string& file_name) {
                   << file_name << "': " << ex.what());
     }
 
+    LOG_INFO(dhcp6_logger, DHCP6_MULTI_THREADING_INFO)
+        .arg(MultiThreadingMgr::instance().getMode())
+        .arg(MultiThreadingMgr::instance().getPktThreadPoolSize())
+        .arg(CfgMgr::instance().getCurrentCfg()->getServerMaxThreadQueueSize());
+
     return (result);
 }
 
@@ -178,7 +182,7 @@ ControlledDhcpv6Srv::init(const std::string& file_name) {
 
     int rcode;
     ConstElementPtr comment = isc::config::parseAnswer(rcode, result);
-    if (rcode != 0) {
+    if (rcode != CONTROL_RESULT_SUCCESS) {
         string reason = comment ? comment->stringValue() :
             "no details available";
         isc_throw(isc::BadValue, reason);
@@ -523,7 +527,7 @@ ControlledDhcpv6Srv::commandBuildReportHandler(const string&,
 ConstElementPtr
 ControlledDhcpv6Srv::commandLeasesReclaimHandler(const string&,
                                                  ConstElementPtr args) {
-    int status_code = 1;
+    int status_code = CONTROL_RESULT_ERROR;
     string message;
 
     // args must be { "remove": <bool> }
@@ -908,6 +912,9 @@ ControlledDhcpv6Srv::ControlledDhcpv6Srv(uint16_t server_port,
     CommandMgr::instance().registerCommand("config-reload",
         boost::bind(&ControlledDhcpv6Srv::commandConfigReloadHandler, this, _1, _2));
 
+    CommandMgr::instance().registerCommand("config-set",
+        boost::bind(&ControlledDhcpv6Srv::commandConfigSetHandler, this, _1, _2));
+
     CommandMgr::instance().registerCommand("config-test",
         boost::bind(&ControlledDhcpv6Srv::commandConfigTestHandler, this, _1, _2));
 
@@ -923,14 +930,11 @@ ControlledDhcpv6Srv::ControlledDhcpv6Srv(uint16_t server_port,
     CommandMgr::instance().registerCommand("leases-reclaim",
         boost::bind(&ControlledDhcpv6Srv::commandLeasesReclaimHandler, this, _1, _2));
 
-    CommandMgr::instance().registerCommand("server-tag-get",
-        boost::bind(&ControlledDhcpv6Srv::commandServerTagGetHandler, this, _1, _2));
-
     CommandMgr::instance().registerCommand("libreload",
         boost::bind(&ControlledDhcpv6Srv::commandLibReloadHandler, this, _1, _2));
 
-    CommandMgr::instance().registerCommand("config-set",
+    CommandMgr::instance().registerCommand("server-tag-get",
-        boost::bind(&ControlledDhcpv6Srv::commandConfigSetHandler, this, _1, _2));
+        boost::bind(&ControlledDhcpv6Srv::commandServerTagGetHandler, this, _1, _2));
 
     CommandMgr::instance().registerCommand("shutdown",
         boost::bind(&ControlledDhcpv6Srv::commandShutdownHandler, this, _1, _2));
@@ -995,8 +999,8 @@ ControlledDhcpv6Srv::~ControlledDhcpv6Srv() {
         CommandMgr::instance().deregisterCommand("build-report");
         CommandMgr::instance().deregisterCommand("config-backend-pull");
         CommandMgr::instance().deregisterCommand("config-get");
-        CommandMgr::instance().deregisterCommand("config-set");
         CommandMgr::instance().deregisterCommand("config-reload");
+        CommandMgr::instance().deregisterCommand("config-set");
         CommandMgr::instance().deregisterCommand("config-test");
         CommandMgr::instance().deregisterCommand("config-write");
         CommandMgr::instance().deregisterCommand("dhcp-disable");
@@ -1115,7 +1119,8 @@ ControlledDhcpv6Srv::dbLostCallback(ReconnectCtlPtr db_reconnect_ctl) {
         return (false);
     }
 
-    // If reconnect isn't enabled, log it and return false
+    // If reconnect isn't enabled or we're out of retries,
+    // log it, schedule a shutdown,  and return false
     if (!db_reconnect_ctl->retriesLeft() ||
         !db_reconnect_ctl->retryInterval()) {
         LOG_INFO(dhcp6_logger, DHCP6_DB_RECONNECT_DISABLED)

src/bin/dhcp6/dhcp6_messages.cc

@@ -1,4 +1,4 @@
-// File created from ../../../src/bin/dhcp6/dhcp6_messages.mes on Sun Oct 27 2019 19:41
+// File created from ../../../src/bin/dhcp6/dhcp6_messages.mes on Wed Feb 19 2020 16:59
 
 #include <cstddef>
 #include <log/message_types.h>
@@ -83,6 +83,7 @@ extern const isc::log::MessageID DHCP6_LEASE_DATA = "DHCP6_LEASE_DATA";
 extern const isc::log::MessageID DHCP6_LEASE_NA_WITHOUT_DUID = "DHCP6_LEASE_NA_WITHOUT_DUID";
 extern const isc::log::MessageID DHCP6_LEASE_PD_WITHOUT_DUID = "DHCP6_LEASE_PD_WITHOUT_DUID";
 extern const isc::log::MessageID DHCP6_LEASE_RENEW = "DHCP6_LEASE_RENEW";
+extern const isc::log::MessageID DHCP6_MULTI_THREADING_INFO = "DHCP6_MULTI_THREADING_INFO";
 extern const isc::log::MessageID DHCP6_NOT_RUNNING = "DHCP6_NOT_RUNNING";
 extern const isc::log::MessageID DHCP6_NO_INTERFACES = "DHCP6_NO_INTERFACES";
 extern const isc::log::MessageID DHCP6_NO_SOCKETS_OPEN = "DHCP6_NO_SOCKETS_OPEN";
@@ -227,6 +228,7 @@ const char* values[] = {
     "DHCP6_LEASE_NA_WITHOUT_DUID", "%1: address lease for address %2 does not have a DUID",
     "DHCP6_LEASE_PD_WITHOUT_DUID", "%1: lease for prefix %2/%3 does not have a DUID",
     "DHCP6_LEASE_RENEW", "%1: lease for address %2 and iaid=%3 has been allocated",
+    "DHCP6_MULTI_THREADING_INFO", "enabled: %1, number of threads: %2, queue size per thread: %3",
     "DHCP6_NOT_RUNNING", "IPv6 DHCP server is not running",
     "DHCP6_NO_INTERFACES", "failed to detect any network interfaces",
     "DHCP6_NO_SOCKETS_OPEN", "no interface configured to listen to DHCP traffic",

src/bin/dhcp6/dhcp6_messages.h

@@ -1,4 +1,4 @@
-// File created from ../../../src/bin/dhcp6/dhcp6_messages.mes on Sun Oct 27 2019 19:41
+// File created from ../../../src/bin/dhcp6/dhcp6_messages.mes on Wed Feb 19 2020 16:59
 
 #ifndef DHCP6_MESSAGES_H
 #define DHCP6_MESSAGES_H
@@ -84,6 +84,7 @@ extern const isc::log::MessageID DHCP6_LEASE_DATA;
 extern const isc::log::MessageID DHCP6_LEASE_NA_WITHOUT_DUID;
 extern const isc::log::MessageID DHCP6_LEASE_PD_WITHOUT_DUID;
 extern const isc::log::MessageID DHCP6_LEASE_RENEW;
+extern const isc::log::MessageID DHCP6_MULTI_THREADING_INFO;
 extern const isc::log::MessageID DHCP6_NOT_RUNNING;
 extern const isc::log::MessageID DHCP6_NO_INTERFACES;
 extern const isc::log::MessageID DHCP6_NO_SOCKETS_OPEN;

src/bin/dhcp6/dhcp6_messages.mes

@@ -822,6 +822,10 @@ This is a debug message issued during the IPv6 DHCP server startup.
 It lists some information about the parameters with which the server
 is running.
 
+% DHCP6_MULTI_THREADING_INFO enabled: %1, number of threads: %2, queue size per thread: %3
+This is a message listing some information about the multi-threading parameters
+with which the server is running.
+
 % DHCP6_SUBNET_DATA %1: the selected subnet details: %2
 This debug message includes the details of the subnet selected for
 the client. The first argument includes the client and the

src/lib/util/multi_threading_mgr.cc

@@ -75,6 +75,7 @@ MultiThreadingMgr::apply(bool enabled, uint32_t thread_count) {
     } else {
         pkt_thread_pool_.reset();
         setMode(false);
+        setPktThreadPoolSize(thread_count);
     }
 }
 

src/lib/util/tests/multi_threading_mgr_unittest.cc

@@ -12,15 +12,91 @@
 
 using namespace isc::util;
 
-// Verifies that the default mode is false (MT disabled).
+/// @brief Verifies that the default mode is false (MT disabled).
 TEST(MultiThreadingMgrTest, default) {
+    // MT should be disabled
     EXPECT_FALSE(MultiThreadingMgr::instance().getMode());
 }
 
-// Verifies that the setter works.
+/// @brief Verifies that the mode setter works.
 TEST(MultiThreadingMgrTest, setMode) {
+    // enable MT
     EXPECT_NO_THROW(MultiThreadingMgr::instance().setMode(true));
+    // MT should be enabled
     EXPECT_TRUE(MultiThreadingMgr::instance().getMode());
+    // disable MT
     EXPECT_NO_THROW(MultiThreadingMgr::instance().setMode(false));
+    // MT should be disabled
     EXPECT_FALSE(MultiThreadingMgr::instance().getMode());
 }
+
+/// @brief Verifies that the thread pool size setter works.
+TEST(MultiThreadingMgrTest, pktThreadPoolSize) {
+    // default thread count is 0
+    EXPECT_EQ(MultiThreadingMgr::instance().getPktThreadPoolSize(), 0);
+    // set thread count to 16
+    EXPECT_NO_THROW(MultiThreadingMgr::instance().setPktThreadPoolSize(16));
+    // thread count should be 16
+    EXPECT_EQ(MultiThreadingMgr::instance().getPktThreadPoolSize(), 16);
+    // set thread count to 0
+    EXPECT_NO_THROW(MultiThreadingMgr::instance().setPktThreadPoolSize(0));
+    // thread count should be 0
+    EXPECT_EQ(MultiThreadingMgr::instance().getPktThreadPoolSize(), 0);
+}
+
+/// @brief Verifies that determining supported thread count works.
+TEST(MultiThreadingMgrTest, supportedThreadCount) {
+    // determining supported thread count should work
+    EXPECT_NE(MultiThreadingMgr::supportedThreadCount(), 0);
+}
+
+/// @brief Verifies that accessing the thread pool works.
+TEST(MultiThreadingMgrTest, pktThreadPool) {
+    // get the thread pool
+    EXPECT_NO_THROW(MultiThreadingMgr::instance().getPktThreadPool());
+}
+
+/// @brief Verifies that apply settings works.
+TEST(MultiThreadingMgrTest, applyConfig) {
+    // get the thread pool
+    auto& thread_pool = MultiThreadingMgr::instance().getPktThreadPool();
+    // MT should be disabled
+    EXPECT_FALSE(MultiThreadingMgr::instance().getMode());
+    // default thread count is 0
+    EXPECT_EQ(MultiThreadingMgr::instance().getPktThreadPoolSize(), 0);
+    // thread pool should be stopped
+    EXPECT_EQ(thread_pool.size(), 0);
+    // enable MT with 16 threads
+    EXPECT_NO_THROW(MultiThreadingMgr::instance().apply(true, 16));
+    // MT should be enabled
+    EXPECT_TRUE(MultiThreadingMgr::instance().getMode());
+    // thread count should be 16
+    EXPECT_EQ(MultiThreadingMgr::instance().getPktThreadPoolSize(), 16);
+    // thread pool should be started
+    EXPECT_EQ(thread_pool.size(), 16);
+    // disable MT
+    EXPECT_NO_THROW(MultiThreadingMgr::instance().apply(false, 16));
+    // MT should be disabled
+    EXPECT_FALSE(MultiThreadingMgr::instance().getMode());
+    // thread count should be 0
+    EXPECT_EQ(MultiThreadingMgr::instance().getPktThreadPoolSize(), 0);
+    // thread pool should be stopped
+    EXPECT_EQ(thread_pool.size(), 0);
+    // enable MT with auto scaling
+    EXPECT_NO_THROW(MultiThreadingMgr::instance().apply(true, 0));
+    // MT should be enabled
+    EXPECT_TRUE(MultiThreadingMgr::instance().getMode());
+    // thread count should be maximum
+    EXPECT_EQ(MultiThreadingMgr::instance().getPktThreadPoolSize(), MultiThreadingMgr::supportedThreadCount());
+    // thread pool should be started
+    EXPECT_EQ(thread_pool.size(), MultiThreadingMgr::supportedThreadCount());
+    // disable MT
+    EXPECT_NO_THROW(MultiThreadingMgr::instance().apply(false, 0));
+    // MT should be disabled
+    EXPECT_FALSE(MultiThreadingMgr::instance().getMode());
+    // thread count should be 0
+    EXPECT_EQ(MultiThreadingMgr::instance().getPktThreadPoolSize(), 0);
+    // thread pool should be stopped
+    EXPECT_EQ(thread_pool.size(), 0);
+}
+