diff --git a/ChangeLog b/ChangeLog index caa82d5f29..d3d4ca9276 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +1498. [bug] marcin + Corrected behavior of the DHCP servers with respect to the + "reconnect-wait-time" parameter setting. This parameter is + specified in milliseconds, but the servers used to interpret + it as specified in seconds. + (Gitlab #173,!154, git 377f49e84ad6ebc91cbeac4116d24a15571c522d) + 1497. [func] fdupont All YANG modules now have a revision specified. When starting, kea-netconf daemon will now check if the required modules are diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index 709c35e1fb..7befc88735 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -496,14 +496,28 @@ recovery and the server will exit immediately upon detecting a loss of connectiv (MySQL and Postgres only). -The number of seconds the server will wait in between attempts to reconnect to the +The number of milliseconds the server will wait in between attempts to reconnect to the lease database after connectivity has been lost may also be specified: -"Dhcp4": { "lease-database": { "reconnect-wait-time" : number-of-seconds, ... }, ... } +"Dhcp4": { "lease-database": { "reconnect-wait-time" : number-of-milliseconds, ... }, ... } -A value of zero (the default) disables automatic recovery and the server will exit -immediately upon detecting a loss of connectivity (MySQL and Postgres only). +The default value for MySQL and Postgres is 0, which disables automatic recovery and +causes the server to exit immediately upon detecting the loss of connectivity. +The default value for Cassandra is 2000 ms. + + + + Automatic reconnect to database backends is configured individually per backend. + This allows you to tailor the recovery parameters to the each backend you use. + We do suggest that you either enable it for all backends or no backends so you + have consistent behavior. Losing connectivity to a backend for which reconnect + is disabled will result in the server shutting itself down. This includes the + cases when lease database backend and hosts database backend is connected to + the same database instance. + + + Finally, the credentials of the account under which the server will access the database should be set: @@ -662,15 +676,27 @@ recovery and the server will exit immediately upon detecting a loss of connectiv (MySQL and Postgres only). -The number of seconds the server will wait in between attempts to reconnect to the +The number of milliseconds the server will wait in between attempts to reconnect to the host database after connectivity has been lost may also be specified: -"Dhcp4": { "hosts-database": { "reconnect-wait-time" : number-of-seconds, ... }, ... } +"Dhcp4": { "hosts-database": { "reconnect-wait-time" : number-of-milliseconds, ... }, ... } -A value of zero (the default) disables automatic recovery and the server will exit -immediately upon detecting a loss of connectivity (MySQL and Postgres only). +The default value for MySQL and Postgres is 0, which disables automatic recovery and +causes the server to exit immediately upon detecting the loss of connectivity. +The default value for Cassandra is 2000 ms. + + + Automatic reconnect to database backends is configured individually per backend. + This allows you to tailor the recovery parameters to the each backend you use. + We do suggest that you either enable it for all backends or no backends so you + have consistent behavior. Losing connectivity to a backend for which reconnect + is disabled will result in the server shutting itself down. This includes the + cases when lease database backend and hosts database backend is connected to + the same database instance. + + Finally, the credentials of the account under which the server will access the database should be set: diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 230fbfe3e5..968eee6006 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -493,14 +493,28 @@ recovery and the server will exit immediately upon detecting a loss of connectiv (MySQL and Postgres only). -The number of seconds the server will wait in between attempts to reconnect to the +The number of milliseconds the server will wait in between attempts to reconnect to the lease database after connectivity has been lost may also be specified: -"Dhcp6": { "lease-database": { "reconnect-wait-time" : number-of-seconds, ... }, ... } +"Dhcp6": { "lease-database": { "reconnect-wait-time" : number-of-milliseconds, ... }, ... } -A value of zero (the default) disables automatic recovery and the server will exit -immediately upon detecting a loss of connectivity (MySQL and Postgres only). +The default value for MySQL and Postgres is 0, which disables automatic recovery and +causes the server to exit immediately upon detecting the loss of connectivity. +The default value for Cassandra is 2000 ms. + + + + Automatic reconnect to database backends is configured individually per backend. + This allows you to tailor the recovery parameters to the each backend you use. + We do suggest that you either enable it for all backends or no backends so you + have consistent behavior. Losing connectivity to a backend for which reconnect + is disabled will result in the server shutting itself down. This includes the + cases when lease database backend and hosts database backend is connected to + the same database instance. + + + Note that host parameter is used by MySQL and PostgreSQL backends. Cassandra has a concept of contact points that could be @@ -599,14 +613,28 @@ recovery and the server will exit immediately upon detecting a loss of connectiv (MySQL and Postgres only). -The number of seconds the server will wait in between attempts to reconnect to the +The number of milliseconds the server will wait in between attempts to reconnect to the host database after connectivity has been lost may also be specified: -"Dhcp6": { "hosts-database": { "reconnect-wait-time" : number-of-seconds, ... }, ... } +"Dhcp6": { "hosts-database": { "reconnect-wait-time" : number-of-milliseconds, ... }, ... } -A value of zero (the default) disables automatic recovery and the server will exit -immediately upon detecting a loss of connectivity (MySQL and Postgres only). +The default value for MySQL and Postgres is 0, which disables automatic recovery and +causes the server to exit immediately upon detecting the loss of connectivity. +The default value for Cassandra is 2000 ms. + + + + Automatic reconnect to database backends is configured individually per backend. + This allows you to tailor the recovery parameters to the each backend you use. + We do suggest that you either enable it for all backends or no backends so you + have consistent behavior. Losing connectivity to a backend for which reconnect + is disabled will result in the server shutting itself down. This includes the + cases when lease database backend and hosts database backend is connected to + the same database instance. + + + Finally, the credentials of the account under which the server will access the database should be set: diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 06e1c3927d..766e1026eb 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -904,7 +904,7 @@ ControlledDhcpv4Srv::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { TimerMgr::instance()->registerTimer("Dhcp4DbReconnectTimer", boost::bind(&ControlledDhcpv4Srv::dbReconnect, this, db_reconnect_ctl), - db_reconnect_ctl->retryInterval() * 1000, + db_reconnect_ctl->retryInterval(), asiolink::IntervalTimer::ONE_SHOT); } diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 7b6d455e0f..f0d486a6c3 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -158,7 +158,7 @@ An error message indicating that an attempt to reconnect to the lease and/or host data bases has failed. This occurs after connectivity to either one has been lost and an automatic attempt to reconnect has failed. -% DHCP4_DB_RECONNECT_ATTEMPT_SCHEDULE scheduling attempt %1 of %2 in %3 seconds +% DHCP4_DB_RECONNECT_ATTEMPT_SCHEDULE scheduling attempt %1 of %2 in %3 milliseconds An informational message indicating that the server is scheduling the next attempt to reconnect to its lease and/or host databases. This occurs when the server has lost databse connectivity and is attempting to reconnect diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 989840089f..b45ba517c0 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -924,7 +924,7 @@ ControlledDhcpv6Srv::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { TimerMgr::instance()->registerTimer("Dhcp6DbReconnectTimer", boost::bind(&ControlledDhcpv6Srv::dbReconnect, this, db_reconnect_ctl), - db_reconnect_ctl->retryInterval() * 1000, + db_reconnect_ctl->retryInterval(), asiolink::IntervalTimer::ONE_SHOT); } diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index c7e3aab83e..4cdcc57f35 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -118,7 +118,7 @@ An error message indicating that an attempt to reconnect to the lease and/or host data bases has failed. This occurs after connectivity to either one has been lost and an automatic attempt to reconnect has failed. -% DHCP6_DB_RECONNECT_ATTEMPT_SCHEDULE scheduling attempt %1 of %2 in %3 seconds +% DHCP6_DB_RECONNECT_ATTEMPT_SCHEDULE scheduling attempt %1 of %2 in %3 milliseconds An informational message indicating that the server is scheduling the next attempt to reconnect to its lease and/or host databases. This occurs when the server has lost databse connectivity and is attempting to reconnect diff --git a/src/lib/database/tests/database_connection_unittest.cc b/src/lib/database/tests/database_connection_unittest.cc index 4e562d0d9c..1effe43e56 100644 --- a/src/lib/database/tests/database_connection_unittest.cc +++ b/src/lib/database/tests/database_connection_unittest.cc @@ -67,7 +67,7 @@ TEST_F(DatabaseConnectionCallbackTest, NoDbLostCallback) { DatabaseConnection::ParameterMap pmap; pmap[std::string("type")] = std::string("test"); pmap[std::string("max-reconnect-tries")] = std::string("3"); - pmap[std::string("reconnect-wait-time")] = std::string("60"); + pmap[std::string("reconnect-wait-time")] = std::string("60000"); DatabaseConnection datasrc(pmap); bool ret = false; @@ -87,7 +87,7 @@ TEST_F(DatabaseConnectionCallbackTest, dbLostCallback) { DatabaseConnection::ParameterMap pmap; pmap[std::string("type")] = std::string("test"); pmap[std::string("max-reconnect-tries")] = std::string("3"); - pmap[std::string("reconnect-wait-time")] = std::string("60"); + pmap[std::string("reconnect-wait-time")] = std::string("60000"); /// Install the callback. DatabaseConnection::db_lost_callback = @@ -104,7 +104,7 @@ TEST_F(DatabaseConnectionCallbackTest, dbLostCallback) { ASSERT_EQ("test", db_reconnect_ctl_->backendType()); ASSERT_EQ(3, db_reconnect_ctl_->maxRetries()); ASSERT_EQ(3, db_reconnect_ctl_->retriesLeft()); - EXPECT_EQ(60, db_reconnect_ctl_->retryInterval()); + EXPECT_EQ(60000, db_reconnect_ctl_->retryInterval()); /// Verify that checkRetries() correctly decrements /// down to zero, and that retriesLeft() returns diff --git a/src/lib/dhcpsrv/tests/Makefile.am b/src/lib/dhcpsrv/tests/Makefile.am index ea30468e7a..253ef0d727 100644 --- a/src/lib/dhcpsrv/tests/Makefile.am +++ b/src/lib/dhcpsrv/tests/Makefile.am @@ -84,7 +84,6 @@ libdhcpsrv_unittests_SOURCES += csv_lease_file4_unittest.cc libdhcpsrv_unittests_SOURCES += csv_lease_file6_unittest.cc libdhcpsrv_unittests_SOURCES += d2_client_unittest.cc libdhcpsrv_unittests_SOURCES += d2_udp_unittest.cc -libdhcpsrv_unittests_SOURCES += database_connection_unittest.cc libdhcpsrv_unittests_SOURCES += dhcp_queue_control_parser_unittest.cc libdhcpsrv_unittests_SOURCES += dhcp4o6_ipc_unittest.cc libdhcpsrv_unittests_SOURCES += duid_config_parser_unittest.cc diff --git a/src/lib/dhcpsrv/tests/database_connection_unittest.cc b/src/lib/dhcpsrv/tests/database_connection_unittest.cc deleted file mode 100644 index 76eda4ee43..0000000000 --- a/src/lib/dhcpsrv/tests/database_connection_unittest.cc +++ /dev/null @@ -1,251 +0,0 @@ -// Copyright (C) 2015-2018 Internet Systems Consortium, Inc. ("ISC") -// -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at http://mozilla.org/MPL/2.0/. - -#include -#include -#include -#include - -#include - -// using namespace isc::dhcp; -using namespace isc::db; - -/// @brief Test fixture for exercising DbLostCallback invocation -class DatabaseConnectionCallbackTest : public ::testing::Test { -public: - /// Constructor - DatabaseConnectionCallbackTest() - : db_reconnect_ctl_(0) { - DatabaseConnection::db_lost_callback = 0; - } - - /// @brief Callback to register with a DatabaseConnection - /// - /// @param db_reconnect_ctl ReconnectCtl containing reconnect - /// parameters - bool dbLostCallback(ReconnectCtlPtr db_reconnect_ctl) { - if (!db_reconnect_ctl) { - isc_throw(isc::BadValue, "db_reconnect_ctl should not be null"); - } - - db_reconnect_ctl_ = db_reconnect_ctl; - return (true); - } - - /// @brief Retainer for the control passed into the callback - ReconnectCtlPtr db_reconnect_ctl_; -}; - -/// @brief getParameter test -/// -/// This test checks if the LeaseMgr can be instantiated and that it -/// parses parameters string properly. -TEST(DatabaseConnectionTest, getParameter) { - - DatabaseConnection::ParameterMap pmap; - pmap[std::string("param1")] = std::string("value1"); - pmap[std::string("param2")] = std::string("value2"); - DatabaseConnection datasrc(pmap); - - EXPECT_EQ("value1", datasrc.getParameter("param1")); - EXPECT_EQ("value2", datasrc.getParameter("param2")); - EXPECT_THROW(datasrc.getParameter("param3"), isc::BadValue); -} - -/// @brief NoDbLostCallback -/// -/// This test verifies that DatabaseConnection::invokeDbLostCallback -/// returns a false if there is connection has no registered -/// DbLostCallback. -TEST_F(DatabaseConnectionCallbackTest, NoDbLostCallback) { - DatabaseConnection::ParameterMap pmap; - pmap[std::string("type")] = std::string("test"); - pmap[std::string("max-reconnect-tries")] = std::string("3"); - pmap[std::string("reconnect-wait-time")] = std::string("60"); - DatabaseConnection datasrc(pmap); - - bool ret = false; - ASSERT_NO_THROW(ret = datasrc.invokeDbLostCallback()); - EXPECT_FALSE(ret); - EXPECT_FALSE(db_reconnect_ctl_); -} - -/// @brief dbLostCallback -/// -/// This test verifies that DatabaseConnection::invokeDbLostCallback -/// safely invokes the registered DbLostCallback. It also tests -/// operation of DbReconnectCtl retry accounting methods. -TEST_F(DatabaseConnectionCallbackTest, dbLostCallback) { - /// Create a Database configuration that includes the reconnect - /// control parameters. - DatabaseConnection::ParameterMap pmap; - pmap[std::string("type")] = std::string("test"); - pmap[std::string("max-reconnect-tries")] = std::string("3"); - pmap[std::string("reconnect-wait-time")] = std::string("60"); - - /// Install the callback. - DatabaseConnection::db_lost_callback = - boost::bind(&DatabaseConnectionCallbackTest::dbLostCallback, this, _1); - /// Create the connection.. - DatabaseConnection datasrc(pmap); - - /// We should be able to invoke the callback and glean - /// the correct reconnect contorl parameters from it. - bool ret = false; - ASSERT_NO_THROW(ret = datasrc.invokeDbLostCallback()); - EXPECT_TRUE(ret); - ASSERT_TRUE(db_reconnect_ctl_); - ASSERT_EQ("test", db_reconnect_ctl_->backendType()); - ASSERT_EQ(3, db_reconnect_ctl_->maxRetries()); - ASSERT_EQ(3, db_reconnect_ctl_->retriesLeft()); - EXPECT_EQ(60, db_reconnect_ctl_->retryInterval()); - - /// Verify that checkRetries() correctly decrements - /// down to zero, and that retriesLeft() returns - /// the correct value. - for (int i = 3; i > 1 ; --i) { - ASSERT_EQ(i, db_reconnect_ctl_->retriesLeft()); - ASSERT_TRUE(db_reconnect_ctl_->checkRetries()); - } - - /// Retries are exhausted, verify that's reflected. - EXPECT_FALSE(db_reconnect_ctl_->checkRetries()); - EXPECT_EQ(0, db_reconnect_ctl_->retriesLeft()); - EXPECT_EQ(3, db_reconnect_ctl_->maxRetries()); -} - -// This test checks that a database access string can be parsed correctly. -TEST(DatabaseConnectionTest, parse) { - - DatabaseConnection::ParameterMap parameters = DatabaseConnection::parse( - "user=me password=forbidden name=kea somethingelse= type=mysql"); - - EXPECT_EQ(5, parameters.size()); - EXPECT_EQ("me", parameters["user"]); - EXPECT_EQ("forbidden", parameters["password"]); - EXPECT_EQ("kea", parameters["name"]); - EXPECT_EQ("mysql", parameters["type"]); - EXPECT_EQ("", parameters["somethingelse"]); -} - -// This test checks that an invalid database access string behaves as expected. -TEST(DatabaseConnectionTest, parseInvalid) { - - // No tokens in the string, so we expect no parameters - std::string invalid = ""; - DatabaseConnection::ParameterMap parameters = DatabaseConnection::parse(invalid); - EXPECT_EQ(0, parameters.size()); - - // With spaces, there are some tokens so we expect invalid parameter - // as there are no equals signs. - invalid = " \t "; - EXPECT_THROW(DatabaseConnection::parse(invalid), isc::InvalidParameter); - - invalid = " noequalshere "; - EXPECT_THROW(DatabaseConnection::parse(invalid), isc::InvalidParameter); - - // A single "=" is valid string, but is placed here as the result is - // expected to be nothing. - invalid = "="; - parameters = DatabaseConnection::parse(invalid); - EXPECT_EQ(1, parameters.size()); - EXPECT_EQ("", parameters[""]); -} - -/// @brief redactConfigString test -/// -/// Checks that the redacted configuration string includes the password only -/// as a set of asterisks. -TEST(DatabaseConnectionTest, redactAccessString) { - - DatabaseConnection::ParameterMap parameters = - DatabaseConnection::parse("user=me password=forbidden name=kea type=mysql"); - EXPECT_EQ(4, parameters.size()); - EXPECT_EQ("me", parameters["user"]); - EXPECT_EQ("forbidden", parameters["password"]); - EXPECT_EQ("kea", parameters["name"]); - EXPECT_EQ("mysql", parameters["type"]); - - // Redact the result. To check, break the redacted string down into its - // components. - std::string redacted = DatabaseConnection::redactedAccessString(parameters); - parameters = DatabaseConnection::parse(redacted); - - EXPECT_EQ(4, parameters.size()); - EXPECT_EQ("me", parameters["user"]); - EXPECT_EQ("*****", parameters["password"]); - EXPECT_EQ("kea", parameters["name"]); - EXPECT_EQ("mysql", parameters["type"]); -} - -/// @brief redactConfigString test - empty password -/// -/// Checks that the redacted configuration string includes the password only -/// as a set of asterisks, even if the password is null. -TEST(DatabaseConnectionTest, redactAccessStringEmptyPassword) { - - DatabaseConnection::ParameterMap parameters = - DatabaseConnection::parse("user=me name=kea type=mysql password="); - EXPECT_EQ(4, parameters.size()); - EXPECT_EQ("me", parameters["user"]); - EXPECT_EQ("", parameters["password"]); - EXPECT_EQ("kea", parameters["name"]); - EXPECT_EQ("mysql", parameters["type"]); - - // Redact the result. To check, break the redacted string down into its - // components. - std::string redacted = DatabaseConnection::redactedAccessString(parameters); - parameters = DatabaseConnection::parse(redacted); - - EXPECT_EQ(4, parameters.size()); - EXPECT_EQ("me", parameters["user"]); - EXPECT_EQ("*****", parameters["password"]); - EXPECT_EQ("kea", parameters["name"]); - EXPECT_EQ("mysql", parameters["type"]); - - // ... and again to check that the position of the empty password in the - // string does not matter. - parameters = DatabaseConnection::parse("user=me password= name=kea type=mysql"); - EXPECT_EQ(4, parameters.size()); - EXPECT_EQ("me", parameters["user"]); - EXPECT_EQ("", parameters["password"]); - EXPECT_EQ("kea", parameters["name"]); - EXPECT_EQ("mysql", parameters["type"]); - - redacted = DatabaseConnection::redactedAccessString(parameters); - parameters = DatabaseConnection::parse(redacted); - - EXPECT_EQ(4, parameters.size()); - EXPECT_EQ("me", parameters["user"]); - EXPECT_EQ("*****", parameters["password"]); - EXPECT_EQ("kea", parameters["name"]); - EXPECT_EQ("mysql", parameters["type"]); -} - -/// @brief redactConfigString test - no password -/// -/// Checks that the redacted configuration string excludes the password if there -/// was no password to begin with. -TEST(DatabaseConnectionTest, redactAccessStringNoPassword) { - - DatabaseConnection::ParameterMap parameters = - DatabaseConnection::parse("user=me name=kea type=mysql"); - EXPECT_EQ(3, parameters.size()); - EXPECT_EQ("me", parameters["user"]); - EXPECT_EQ("kea", parameters["name"]); - EXPECT_EQ("mysql", parameters["type"]); - - // Redact the result. To check, break the redacted string down into its - // components. - std::string redacted = DatabaseConnection::redactedAccessString(parameters); - parameters = DatabaseConnection::parse(redacted); - - EXPECT_EQ(3, parameters.size()); - EXPECT_EQ("me", parameters["user"]); - EXPECT_EQ("kea", parameters["name"]); - EXPECT_EQ("mysql", parameters["type"]); -}