[5046] Move apply logging and config commit from processConfig to set-config handler

src/bin/dhcp4/ctrl_dhcp4_srv.cc
    commandConfigReloadHandler() - use commandSetConfigHandler()
    instead of processConfig() to account for logging config

    commandSetConfigHandler() - apply logging config and commit config here instead of
    in processConfig()

src/bin/dhcp4/tests/dhcp4_test_utils.h
     ~NakedDhcpv4Srv() - removed unecesary initLogger call

src/bin/dhcp4/tests/kea_controller_unittest.cc
     ~JSONFileBackendTest() - removed unecessary call to setDefaultLogging

src/bin/dhcp6/ctrl_dhcp6_srv.cc
    commandConfigReloadHandler() - use commandSetConfigHandler()
    instead of processConfig() to account for logging config

    commandSetConfigHandler() - apply logging config and commit config here instead of
    in processConfig()

src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
    createUnixChannelServer() -  added config commit so command channel behavior
    is correct

    TEST_F(CtrlDhcpv6SrvTest, configReload)
        Wrap configuration in Dhcp6 element

    TEST_F(CtrlChannelDhcpv6SrvTest, set_config)
        Turn off timers in config

src/lib/dhcpsrv/srv_config.cc
    SrvConfig::applyLoggingCfg() - remove logic added to not call manager.process
    when there are no specs.

src/bin/dhcp4/ctrl_dhcp4_srv.cc

@@ -62,7 +62,8 @@ ControlledDhcpv4Srv::commandLibReloadHandler(const string&, ConstElementPtr) {
 ConstElementPtr
 ControlledDhcpv4Srv::commandConfigReloadHandler(const string&,
                                                 ConstElementPtr args) {
-    return (processConfig(args));
+    // Use set-config as it handles logging and server config
+    return (commandSetConfigHandler("", args));
 }
 
 ConstElementPtr
@@ -72,14 +73,10 @@ ControlledDhcpv4Srv::commandSetConfigHandler(const string&,
     ConstElementPtr dhcp4;
     string message;
 
-    // We are starting the configuration process so we should remove any
-    // staging configuration that has been created during previous
-    // configuration attempts.
-    CfgMgr::instance().rollback();
-
     // Command arguments are expected to be:
     // { "Dhcp4": { ... }, "Logging": { ... } }
-    // The Logging component is technically optional, but very recommended.
+    // The Logging component is technically optional. If it's not supplied
+    // logging will revert to default logging.
     if (!args) {
         message = "Missing mandatory 'arguments' parameter.";
     } else {
@@ -98,16 +95,36 @@ ControlledDhcpv4Srv::commandSetConfigHandler(const string&,
         return (result);
     }
 
+    // We are starting the configuration process so we should remove any
+    // staging configuration that has been created during previous
+    // configuration attempts.
+    CfgMgr::instance().rollback();
+
     // Logging is a sibling element and must be be parsed explicitly.
     // The call to configureLogger parses the given Logging element if
-    // not null, into the staging config.  Note this DOES alter the
+    // not null, into the staging config.  Note this does not alter the
     // current loggers, they remain in effect until we apply the
-    // logging config.
+    // logging config below.  If no logging is supplied logging will
+    // revert to default logging.
     Daemon::configureLogger(args->get("Logging"),
                             CfgMgr::instance().getStagingCfg());
 
     // Now we configure the server proper.
-    return (processConfig(dhcp4));
+    ConstElementPtr result = processConfig(dhcp4);
+
+    // If the configuration parsed successfully, apply the new logger
+    // configuration and the commit the new configuration.  We apply
+    // the logging first in case there's a configuration failure.
+    int rcode = 0;
+    isc::config::parseAnswer(rcode, result);
+    if (rcode == 0) {
+        CfgMgr::instance().getStagingCfg()->applyLoggingCfg();
+
+        // Use new configuration.
+        CfgMgr::instance().commit();
+    }
+
+    return (result);
 }
 
 ConstElementPtr
@@ -283,14 +300,6 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) {
         }
     }
 
-    // Configuration was parsed successfully, apply the new logger
-    // configuration to log4cplus. It is done before commit in case
-    // something goes wrong.
-    CfgMgr::instance().getStagingCfg()->applyLoggingCfg();
-
-    // Use new configuration.
-    CfgMgr::instance().commit();
-
     return (answer);
 }
 

src/bin/dhcp4/kea_controller.cc

@@ -137,7 +137,7 @@ ControlledDhcpv4Srv::init(const std::string& file_name) {
 
     // We don't need to call openActiveSockets() or startD2() as these
     // methods are called in processConfig() which is called by
-    // processCommand("reload-config", ...)
+    // processCommand("set-config", ...)
 
     // Set signal handlers. When the SIGHUP is received by the process
     // the server reconfiguration will be triggered. When SIGTERM or

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -168,8 +168,6 @@ public:
     }
 
     virtual ~NakedDhcpv4Srv() {
-        // Revert to unit test logging
-        isc::log::initLogger();
     }
 
     /// @brief Dummy server identifier option used by various tests.

src/bin/dhcp4/tests/kea_controller_unittest.cc

@@ -62,7 +62,6 @@ public:
 
     ~JSONFileBackendTest() {
         LeaseMgrFactory::destroy();
-        isc::log::setDefaultLoggingOutput();
         static_cast<void>(remove(TEST_FILE));
     };
 

src/bin/dhcp6/ctrl_dhcp6_srv.cc

@@ -67,7 +67,8 @@ ControlledDhcpv6Srv::commandLibReloadHandler(const string&, ConstElementPtr) {
 
 ConstElementPtr
 ControlledDhcpv6Srv::commandConfigReloadHandler(const string&, ConstElementPtr args) {
-    return (processConfig(args));
+    // Use set-config as it handles logging and server config
+    return (commandSetConfigHandler("", args));
 }
 
 ConstElementPtr
@@ -77,14 +78,10 @@ ControlledDhcpv6Srv::commandSetConfigHandler(const string&,
     ConstElementPtr dhcp6;
     string message;
 
-    // We are starting the configuration process so we should remove any
-    // staging configuration that has been created during previous
-    // configuration attempts.
-    CfgMgr::instance().rollback();
-
     // Command arguments are expected to be:
     // { "Dhcp6": { ... }, "Logging": { ... } }
-    // The Logging component is technically optional, but very recommended.
+    // The Logging component is technically optional. If it's not supplied
+    // logging will revert to default logging.
     if (!args) {
         message = "Missing mandatory 'arguments' parameter.";
     } else {
@@ -103,16 +100,36 @@ ControlledDhcpv6Srv::commandSetConfigHandler(const string&,
         return (result);
     }
 
+    // We are starting the configuration process so we should remove any
+    // staging configuration that has been created during previous
+    // configuration attempts.
+    CfgMgr::instance().rollback();
+
     // Logging is a sibling element and must be be parsed explicitly.
     // The call to configureLogger parses the given Logging element if
-    // not null, into the staging config.  Note this DOES alter the
+    // not null, into the staging config.  Note this does not alter the
     // current loggers, they remain in effect until we apply the
-    // logging config.
+    // logging config below.  If no logging is supplied logging will
+    // revert to default logging.
     Daemon::configureLogger(args->get("Logging"),
                             CfgMgr::instance().getStagingCfg());
 
     // Now we configure the server proper.
-    return (processConfig(dhcp6));
+    ConstElementPtr result = processConfig(dhcp6);
+
+    // If the configuration parsed successfully, apply the new logger
+    // configuration and the commit the new configuration.  We apply
+    // the logging first in case there's a configuration failure.
+    int rcode = 0;
+    isc::config::parseAnswer(rcode, result);
+    if (rcode == 0) {
+        CfgMgr::instance().getStagingCfg()->applyLoggingCfg();
+
+        // Use new configuration.
+        CfgMgr::instance().commit();
+    }
+
+    return (result);
 }
 
 
@@ -308,14 +325,6 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) {
         }
     }
 
-    // Configuration was parsed successfully, apply the new logger
-    // configuration to log4cplus. It is done before commit in case
-    // something goes wrong.
-    CfgMgr::instance().getStagingCfg()->applyLoggingCfg();
-
-    // Use new configuration.
-    CfgMgr::instance().commit();
-
     // Finally, we can commit runtime option definitions in libdhcp++. This is
     // exception free.
     LibDHCP::commitRuntimeOptionDefs();

src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc

@@ -149,6 +149,12 @@ public:
         ConstElementPtr config = Element::fromJSON(config_txt);
 
         ConstElementPtr answer = server_->processConfig(config);
+
+        // Commit the configuration so any subsequent reconfigurations
+        // will only close the command channel if its configuration has
+        // changed.
+        CfgMgr::instance().commit();
+
         ASSERT_TRUE(answer);
 
         int status = 0;
@@ -302,7 +308,7 @@ TEST_F(CtrlDhcpv6SrvTest, configReload) {
 
     // Use empty parameters list
     // Prepare configuration file.
-    string config_txt = "{ \"interfaces-config\": {"
+    string config_txt = "{ \"Dhcp6\": { \"interfaces-config\": {"
         "  \"interfaces\": [ \"*\" ]"
         "},"
         "\"preferred-lifetime\": 3000,"
@@ -321,7 +327,7 @@ TEST_F(CtrlDhcpv6SrvTest, configReload) {
         "    \"pools\": [ { \"pool\": \"2001:db8:3::/80\" } ],"
         "    \"subnet\": \"2001:db8:3::/64\" "
         " } ],"
-        "\"valid-lifetime\": 4000 }";
+        "\"valid-lifetime\": 4000 }}";
 
     ElementPtr config = Element::fromJSON(config_txt);
 
@@ -361,6 +367,11 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) {
         "        \"valid-lifetime\": 4000, \n"
         "        \"renew-timer\": 1000, \n"
         "        \"rebind-timer\": 2000, \n"
+        "       \"expired-leases-processing\": { \n"
+        "            \"reclaim-timer-wait-time\": 0, \n"
+        "            \"hold-reclaimed-time\": 0, \n"
+        "            \"flush-reclaimed-timer-wait-time\": 0 \n"
+        "        },"
         "        \"subnet6\": [ \n";
     string subnet1 =
         "               {\"subnet\": \"3002::/64\", \n"
@@ -382,9 +393,8 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) {
     string logger_txt =
         "    \"Logging\": { \n"
         "        \"loggers\": [ { \n"
-        "            \"name\": \"*\", \n"
-        "            \"severity\": \"INFO\", \n"
-        "            \"debuglevel\": 0, \n"
+        "            \"name\": \"kea\", \n"
+        "            \"severity\": \"FATAL\", \n"
         "            \"output_options\": [{ \n"
         "                \"output\": \"/dev/null\" \n"
         "            }] \n"
@@ -438,7 +448,7 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) {
 
     // Should fail with a syntax error
     EXPECT_EQ("{ \"result\": 1, "
-              "\"text\": \"unsupported parameter: BOGUS (<string>:12:26)\" }",
+              "\"text\": \"unsupported parameter: BOGUS (<string>:16:26)\" }",
               response);
 
     // Check that the config was not lost

src/lib/dhcpsrv/srv_config.cc

@@ -118,13 +118,8 @@ SrvConfig::applyLoggingCfg() const {
         specs.push_back(it->toSpec());
     }
 
-    // If there are no specs, then leave logging alone. This will leave
-    // the existing logging setup intact. Calling process() with no
-    // specs will reset the logging heirarchy and so forth.
-    if (specs.size()) {
-        LoggerManager manager;
-        manager.process(specs.begin(), specs.end());
-    }
+    LoggerManager manager;
+    manager.process(specs.begin(), specs.end());
 }
 
 bool