[master] Merge branch 'master' of gitlab.isc.org:isc-projects/kea

Re-merged with master.

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

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).
   </para>
   <para>
-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:
 <screen>
-"Dhcp4": { "lease-database": { <userinput>"reconnect-wait-time" : <replaceable>number-of-seconds</replaceable></userinput>, ... }, ... }
+"Dhcp4": { "lease-database": { <userinput>"reconnect-wait-time" : <replaceable>number-of-milliseconds</replaceable></userinput>, ... }, ... }
 </screen>
-A value of zero (the default) disables automatic recovery and the server will exit
+The default value for MySQL and Postgres is 0, which disables automatic recovery and
-immediately upon detecting a loss of connectivity (MySQL and Postgres only).
+causes the server to exit immediately upon detecting the loss of connectivity.
+The default value for Cassandra is 2000 ms.
     </para>
+
+    <note>
+      <para>
+        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.
+      </para>
+    </note>
+
   <para>Finally, the credentials of the account under which the server will
   access the database should be set:
 <screen>
@@ -662,15 +676,27 @@ recovery and the server will exit immediately upon detecting a loss of connectiv
 (MySQL and Postgres only).
   </para>
   <para>
-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:
 <screen>
-"Dhcp4": { "hosts-database": { <userinput>"reconnect-wait-time" : <replaceable>number-of-seconds</replaceable></userinput>, ... }, ... }
+"Dhcp4": { "hosts-database": { <userinput>"reconnect-wait-time" : <replaceable>number-of-milliseconds</replaceable></userinput>, ... }, ... }
 </screen>
-A value of zero (the default) disables automatic recovery and the server will exit
+The default value for MySQL and Postgres is 0, which disables automatic recovery and
-immediately upon detecting a loss of connectivity (MySQL and Postgres only).
+causes the server to exit immediately upon detecting the loss of connectivity.
+The default value for Cassandra is 2000 ms.
     </para>
 
+    <note>
+      <para>
+        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.
+      </para>
+    </note>
   </para>
   <para>Finally, the credentials of the account under which the server will
   access the database should be set:

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).
   </para>
   <para>
-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:
 <screen>
-"Dhcp6": { "lease-database": { <userinput>"reconnect-wait-time" : <replaceable>number-of-seconds</replaceable></userinput>, ... }, ... }
+"Dhcp6": { "lease-database": { <userinput>"reconnect-wait-time" : <replaceable>number-of-milliseconds</replaceable></userinput>, ... }, ... }
 </screen>
-A value of zero (the default) disables automatic recovery and the server will exit
+The default value for MySQL and Postgres is 0, which disables automatic recovery and
-immediately upon detecting a loss of connectivity (MySQL and Postgres only).
+causes the server to exit immediately upon detecting the loss of connectivity.
+The default value for Cassandra is 2000 ms.
   </para>
+
+  <note>
+    <para>
+      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.
+    </para>
+  </note>
+
   <para>
     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).
   </para>
   <para>
-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:
 <screen>
-"Dhcp6": { "hosts-database": { <userinput>"reconnect-wait-time" : <replaceable>number-of-seconds</replaceable></userinput>, ... }, ... }
+"Dhcp6": { "hosts-database": { <userinput>"reconnect-wait-time" : <replaceable>number-of-milliseconds</replaceable></userinput>, ... }, ... }
 </screen>
-A value of zero (the default) disables automatic recovery and the server will exit
+The default value for MySQL and Postgres is 0, which disables automatic recovery and
-immediately upon detecting a loss of connectivity (MySQL and Postgres only).
+causes the server to exit immediately upon detecting the loss of connectivity.
+The default value for Cassandra is 2000 ms.
   </para>
+
+    <note>
+      <para>
+        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.
+      </para>
+    </note>
+
   <para>Finally, the credentials of the account under which the server will
   access the database should be set:
 <screen>

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);
         }
 

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

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);
         }
 

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

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

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

src/lib/dhcpsrv/tests/database_connection_unittest.cc

@@ -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 <config.h>
-#include <exceptions/exceptions.h>
-#include <database/database_connection.h>
-#include <gtest/gtest.h>
-
-#include <boost/bind.hpp>
-
-// 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"]);
-}