From 354e68adecaa81b1ab8811ce64f1afe18bd6fe41 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 15 Dec 2016 16:15:26 -0500 Subject: [PATCH] [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 | 45 +++++++++++-------- src/bin/dhcp4/kea_controller.cc | 2 +- src/bin/dhcp4/tests/dhcp4_test_utils.h | 2 - .../dhcp4/tests/kea_controller_unittest.cc | 1 - src/bin/dhcp6/ctrl_dhcp6_srv.cc | 45 +++++++++++-------- .../dhcp6/tests/ctrl_dhcp6_srv_unittest.cc | 22 ++++++--- src/lib/dhcpsrv/srv_config.cc | 9 +--- 7 files changed, 73 insertions(+), 53 deletions(-) diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 3ac909d914..05879f7235 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/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); } diff --git a/src/bin/dhcp4/kea_controller.cc b/src/bin/dhcp4/kea_controller.cc index d79d9d037f..ed74214045 100644 --- a/src/bin/dhcp4/kea_controller.cc +++ b/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 diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index 98520cba16..e438a58172 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/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. diff --git a/src/bin/dhcp4/tests/kea_controller_unittest.cc b/src/bin/dhcp4/tests/kea_controller_unittest.cc index c42a022c88..78d2c5651c 100644 --- a/src/bin/dhcp4/tests/kea_controller_unittest.cc +++ b/src/bin/dhcp4/tests/kea_controller_unittest.cc @@ -62,7 +62,6 @@ public: ~JSONFileBackendTest() { LeaseMgrFactory::destroy(); - isc::log::setDefaultLoggingOutput(); static_cast(remove(TEST_FILE)); }; diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 7bbf673349..c9ff04d85a 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/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(); diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index 98c88b44d8..7c9fbe97d4 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/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 (:12:26)\" }", + "\"text\": \"unsupported parameter: BOGUS (:16:26)\" }", response); // Check that the config was not lost diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index 017d83767e..37cac2f89d 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/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 -- 2.47.3