From: Thomas Markwalder <tmark@isc.org>
Date: Thu, 13 Feb 2014 19:39:10 +0000 (-0500)
Subject: [3329] D2ClientMgr now supports suspending updates.
X-Git-Tag: bind10-1.2.0beta1-release~45^2~6
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e448f9b0e46ea1a4812572818c5f63051b697ef0;p=thirdparty%2Fkea.git

[3329] D2ClientMgr now supports suspending updates.

Added ability to suspend DDNS updates to D2ClientMgr, so updating can be
stopped when errors communications with D2 fail.

Made D2ClientMgr non-copyable to avoid programmatic errors.

Changed D2ClientMgr to return simply return from startSender() if already
in send mode.
---

diff --git a/src/lib/dhcpsrv/d2_client_mgr.cc b/src/lib/dhcpsrv/d2_client_mgr.cc
index e43bbdbd22..111f3707f0 100644
--- a/src/lib/dhcpsrv/d2_client_mgr.cc
+++ b/src/lib/dhcpsrv/d2_client_mgr.cc
@@ -36,6 +36,19 @@ D2ClientMgr::~D2ClientMgr(){
     stopSender();
 }
 
+void
+D2ClientMgr::suspendUpdates() {
+    if (ddnsEnabled()) {
+        /// @todo For now we will disable updates and stop sending.
+        /// This at least provides a means to shut it off if there are errors.
+        LOG_WARN(dhcpsrv_logger, DHCPSRV_DHCP_DDNS_SUSPEND_UPDATES);
+        d2_client_config_->enableUpdates(false);
+        if (name_change_sender_) {
+            stopSender();
+        }
+    }
+}
+
 void
 D2ClientMgr::setD2ClientConfig(D2ClientConfigPtr& new_config) {
     if (!new_config) {
@@ -45,9 +58,11 @@ D2ClientMgr::setD2ClientConfig(D2ClientConfigPtr& new_config) {
 
     // Don't do anything unless configuration values are actually different.
     if (*d2_client_config_ != *new_config) {
+        // Make sure we stop sending first.
+        stopSender();
         if (!new_config->getEnableUpdates()) {
-            // Updating has been turned off, destroy current sender.
-            // Any queued requests are tossed.
+            // Updating has been turned off.
+            // Destroy current sender (any queued requests are tossed).
             name_change_sender_.reset();
         } else {
             dhcp_ddns::NameChangeSenderPtr new_sender;
@@ -84,7 +99,6 @@ D2ClientMgr::setD2ClientConfig(D2ClientConfigPtr& new_config) {
             /// then the queued contents might now be invalid.  There is
             /// no way to regenerate them if they are wrong.
             if (name_change_sender_) {
-                name_change_sender_->stopSending();
                 new_sender->assumeQueue(*name_change_sender_);
             }
 
@@ -194,15 +208,25 @@ D2ClientMgr::qualifyName(const std::string& partial_name) const {
 
 void
 D2ClientMgr::startSender(D2ClientErrorHandler error_handler) {
+    if (amSending()) {
+        return;
+    }
+
     // Create a our own service instance when we are not being multiplexed
     // into an external service..
     private_io_service_.reset(new asiolink::IOService());
     startSender(error_handler, *private_io_service_);
+    LOG_INFO(dhcpsrv_logger, DHCPSRV_DHCP_DDNS_SENDER_STARTED)
+             .arg(d2_client_config_->toText());
 }
 
 void
 D2ClientMgr::startSender(D2ClientErrorHandler error_handler,
                          isc::asiolink::IOService& io_service) {
+    if (amSending()) {
+        return;
+    }
+
     if (!name_change_sender_)  {
         isc_throw(D2ClientError, "D2ClientMgr::startSender sender is null");
     }
@@ -242,6 +266,7 @@ D2ClientMgr::stopSender() {
     // If its not null, call stop.
     if (name_change_sender_)  {
         name_change_sender_->stopSending();
+        LOG_INFO(dhcpsrv_logger, DHCPSRV_DHCP_DDNS_SENDER_STOPPED);
     }
 }
 
diff --git a/src/lib/dhcpsrv/d2_client_mgr.h b/src/lib/dhcpsrv/d2_client_mgr.h
index de47c122ec..c367551fa5 100644
--- a/src/lib/dhcpsrv/d2_client_mgr.h
+++ b/src/lib/dhcpsrv/d2_client_mgr.h
@@ -25,6 +25,7 @@
 #include <exceptions/exceptions.h>
 
 #include <boost/shared_ptr.hpp>
+#include <boost/noncopyable.hpp>
 
 #include <stdint.h>
 #include <string>
@@ -80,7 +81,8 @@ boost::function<void(const dhcp_ddns::NameChangeSender::Result result,
 /// into the sender.  Using a private service isolates the sender's IO from
 /// any other services.
 ///
-class D2ClientMgr : public dhcp_ddns::NameChangeSender::RequestSendHandler {
+class D2ClientMgr : public dhcp_ddns::NameChangeSender::RequestSendHandler,
+                    boost::noncopyable {
 public:
     /// @brief Constructor
     ///
@@ -284,6 +286,17 @@ public:
     /// NameChangeSender is abstract.
     void runReadyIO();
 
+    /// @brief Suspends sending requests.
+    ///
+    /// This method is intended to be used when IO errors occur.  It toggles
+    /// the enable-updates configuration flag to off, and takes the sender
+    /// out of send mode.  Messages in the sender's queue will remain in the
+    /// queue.
+    /// @todo This logic may change in NameChangeSender is altered allow
+    /// queuing while stopped.  Currently when a sender is not in send mode
+    /// it will not accept additional messages.
+    void suspendUpdates();
+
 protected:
     /// @brief Function operator implementing the NCR sender callback.
     ///
diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes
index e8dd2e7120..63f042a330 100644
--- a/src/lib/dhcpsrv/dhcpsrv_messages.mes
+++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes
@@ -138,11 +138,29 @@ the database access parameters are changed: in the latter case, the
 server closes the currently open database, and opens a database using
 the new parameters.
 
-% DHCPSRV_HOOK_LEASE4_SELECT_SKIP Lease4 creation was skipped, because of callout skip flag.
-This debug message is printed when a callout installed on lease4_select
-hook point sets the skip flag. It means that the server was told that
-no lease4 should be assigned. The server will not put that lease in its
-database and the client will get a NAK packet.
+% DHCPSRV_DHCP_DDNS_ERROR_EXCEPTION error handler for DHCP_DDNS IO generated an expected exception: %1
+This is an error message that occurs when an attempt to send a request to
+b10-dhcp-ddns fails there registered error handler threw an uncaught exception.
+This is a programmatic error which should not occur. By convention, the error
+handler should not propagate exceptions. Please report this error.
+
+% DHCPSRV_DHCP_DDNS_HANDLER_NULL error handler for DHCP_DDNS IO is not set.
+This is an error message that occurs when an attempt to send a request to
+b10-dhcp-ddns fails and there is no registered error handler.  This is a
+programmatic error which should never occur and should be reported.
+
+% DHCPSRV_DHCP_DDNS_NCR_SENT NameChangeRequest sent to b10-dhcp-ddns: %1
+A debug message issued when a NameChangeRequest has been successfully sent to
+b10-dhcp-ddns.
+
+% DHCPSRV_DHCP_DDNS_SENDER_STARTED NameChangeRequest sender has been started: %1
+A informational message issued when a communications with b10-dhcp-ddns has
+been successfully started.
+
+% DHCPSRV_DHCP_DDNS_SENDER_STOPPED NameChangeRequest sender has been stopped.
+A informational message issued when a communications with b10-dhcp-ddns has
+been stopped. This normally occurs during reconfiguration and as part of normal
+shutdown. It may occur if b10-dhcp-ddns communications breakdown.
 
 % DHCPSRV_HOOK_LEASE4_RENEW_SKIP DHCPv4 lease was not renewed because a callout set the skip flag.
 This debug message is printed when a callout installed on lease4_renew
@@ -150,6 +168,12 @@ hook point set the skip flag. For this particular hook point, the setting
 of the flag by a callout instructs the server to not renew a lease. The
 server will use existing lease as it is, without extending its lifetime.
 
+% DHCPSRV_HOOK_LEASE4_SELECT_SKIP Lease4 creation was skipped, because of callout skip flag.
+This debug message is printed when a callout installed on lease4_select
+hook point sets the skip flag. It means that the server was told that
+no lease4 should be assigned. The server will not put that lease in its
+database and the client will get a NAK packet.
+
 % DHCPSRV_HOOK_LEASE6_SELECT_SKIP Lease6 (non-temporary) creation was skipped, because of callout skip flag.
 This debug message is printed when a callout installed on lease6_select
 hook point sets the skip flag. It means that the server was told that
@@ -198,6 +222,11 @@ A debug message issued when the server is attempting to obtain a set of
 IPv4 leases from the memory file database for a client with the specified
 client identification.
 
+% DHCPSRV_MEMFILE_GET_CLIENTID_HWADDR_SUBID obtaining IPv4 lease for client ID %1, hardware address %2 and subnet ID %3
+A debug message issued when the server is attempting to obtain an IPv4
+lease from the memory file database for a client with the specified
+client ID, hardware address and subnet ID.
+
 % DHCPSRV_MEMFILE_GET_HWADDR obtaining IPv4 leases for hardware address %1
 A debug message issued when the server is attempting to obtain a set of
 IPv4 leases from the memory file database for a client with the specified
@@ -213,11 +242,6 @@ A debug message issued when the server is attempting to obtain an IPv6
 lease from the memory file database for a client with the specified IAID
 (Identity Association ID), Subnet ID and DUID (DHCP Unique Identifier).
 
-% DHCPSRV_MEMFILE_GET_CLIENTID_HWADDR_SUBID obtaining IPv4 lease for client ID %1, hardware address %2 and subnet ID %3
-A debug message issued when the server is attempting to obtain an IPv4
-lease from the memory file database for a client with the specified
-client ID, hardware address and subnet ID.
-
 % DHCPSRV_MEMFILE_GET_SUBID_CLIENTID obtaining IPv4 lease for subnet ID %1 and client ID %2
 A debug message issued when the server is attempting to obtain an IPv4
 lease from the memory file database for a client with the specified
@@ -345,17 +369,9 @@ indicate an error in the source code, please submit a bug report.
 The database access string specified a database type (given in the
 message) that is unknown to the software.  This is a configuration error.
 
-% DHCPSRV_DHCP_DDNS_HANDLER_NULL error handler for DHCP_DDNS IO is not set.
-This is an error message that occurs when an attempt to send a request to
-b10-dhcp-ddns fails and there is no registered error handler.  This is a
-programmatic error which should never occur and should be reported.
-
-% DHCPSRV_DHCP_DDNS_ERROR_EXCEPTION error handler for DHCP_DDNS IO generated an expected exception: %1
-This is an error message that occurs when an attempt to send a request to
-b10-dhcp-ddns fails there registered error handler threw an uncaught exception.
-This is a programmatic error which should not occur. By convention, the error
-handler should not propagate exceptions. Please report this error.
-
-% DHCPSRV_DHCP_DDNS_NCR_SENT NameChangeRequest sent to b10-dhcp-ddns: %1
-A debug message issued when a NameChangeRequest has been successfully sent to
-b10-dhcp-ddns.
+% DHCPSRV_DHCP_DDNS_SUSPEND_UPDATES DHCP_DDNS updates are being suspended.
+This is a warning message indicating the DHCP_DDNS updates have been turned
+off.  This should only occur if IO errors communicating with b10-dhcp-ddns
+have been experienced.  Any such errors should have preceding entries in the
+log with details.  No further attempts to communicate with b10-dhcp-ddns will
+be made without intervention.
diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
index e0639dc2b9..ee54c3d94a 100644
--- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
@@ -903,7 +903,7 @@ TEST_F(CfgMgrTest, d2ClientConfig) {
     // After CfgMgr construction, D2ClientMgr member should be initialized
     // with a D2 configuration that is disabled.
     // Verify we can Fetch the mgr.
-    D2ClientMgr d2_mgr = CfgMgr::instance().getD2ClientMgr();
+    D2ClientMgr& d2_mgr = CfgMgr::instance().getD2ClientMgr();
     EXPECT_FALSE(d2_mgr.ddnsEnabled());
 
     // Make sure the convenience method fetches the config correctly.
diff --git a/src/lib/dhcpsrv/tests/d2_udp_unittest.cc b/src/lib/dhcpsrv/tests/d2_udp_unittest.cc
index 5b51530173..99538421a8 100644
--- a/src/lib/dhcpsrv/tests/d2_udp_unittest.cc
+++ b/src/lib/dhcpsrv/tests/d2_udp_unittest.cc
@@ -326,55 +326,58 @@ TEST_F(D2ClientMgrTest, udpSendExternalIOService) {
 /// when send errors occur.
 TEST_F(D2ClientMgrTest, udpSendErrorHandler) {
     // Enable DDNS with server at 127.0.0.1/prot 53001 via UDP.
-    enableDdns("127.0.0.1", 530001, dhcp_ddns::NCR_UDP);
-
-    // Trying to fetch the select-fd when not sending should fail.
-    ASSERT_THROW(getSelectFd(), D2ClientError);
-
     // Place sender in send mode.
+    enableDdns("127.0.0.1", 530001, dhcp_ddns::NCR_UDP);
     ASSERT_NO_THROW(startSender(getErrorHandler()));
 
-    // select_fd should evaluate to NOT ready to read.
-    selectCheck(false);
-
     // Simulate a failed response in the send call back. This should
     // cause the error handler to get invoked.
     simulate_send_failure_ = true;
 
+    // Verify error count is zero.
     ASSERT_EQ(0, error_handler_count_);
 
     // Send a test request.
     dhcp_ddns::NameChangeRequestPtr ncr = buildTestNcr();
     ASSERT_NO_THROW(sendRequest(ncr));
 
-    // select_fd should evaluate to ready to read.
-    selectCheck(true);
+    // Call the ready handler. This should complete the message with an error.
+    ASSERT_NO_THROW(runReadyIO());
 
-    // Call service handler.
-    runReadyIO();
+    // If we executed error handler properly, the error count should one.
+    ASSERT_EQ(1, error_handler_count_);
+}
 
-    // select_fd should evaluate to not ready to read.
-    selectCheck(false);
 
-    ASSERT_EQ(1, error_handler_count_);
+/// @brief Checks that client error handler exceptions are handled gracefully.
+TEST_F(D2ClientMgrTest, udpSendErrorHandlerThrow) {
+    // Enable DDNS with server at 127.0.0.1/prot 53001 via UDP.
+    // Place sender in send mode.
+    enableDdns("127.0.0.1", 530001, dhcp_ddns::NCR_UDP);
+    ASSERT_NO_THROW(startSender(getErrorHandler()));
 
-    // Simulate a failed response in the send call back. This should
-    // cause the error handler to get invoked.
+    // Simulate a failed response in the send call back and
+    // force a throw in the error handler.
     simulate_send_failure_ = true;
     error_handler_throw_ = true;
 
+    // Verify error count is zero.
+    ASSERT_EQ(0, error_handler_count_);
+
     // Send a test request.
-    ncr = buildTestNcr();
+    dhcp_ddns::NameChangeRequestPtr ncr = buildTestNcr();
     ASSERT_NO_THROW(sendRequest(ncr));
 
-    // Call the io service handler.
-    runReadyIO();
+    // Call the ready handler. This should complete the message with an error.
+    // The handler should throw but the exception should not escape.
+    ASSERT_NO_THROW(runReadyIO());
 
-    // Simulation flag should be false.
+    // If throw flag is false, then we were in the error handler should
+    // have thrown.
     ASSERT_FALSE(error_handler_throw_);
 
-    // Count should still be 1.
-    ASSERT_EQ(1, error_handler_count_);
+    // If error count is still zero, then we did throw.
+    ASSERT_EQ(0, error_handler_count_);
 }
 
 TEST_F(D2ClientMgrTest, ifaceRegister) {
@@ -390,6 +393,7 @@ TEST_F(D2ClientMgrTest, ifaceRegister) {
         ASSERT_NO_THROW(sendRequest(ncr));
     }
 
+    // Make sure queue count is correct.
     EXPECT_EQ(3, getQueueSize());
 
     // select_fd should evaluate to ready to read.
@@ -417,5 +421,36 @@ TEST_F(D2ClientMgrTest, ifaceRegister) {
     ASSERT_EQ(0, error_handler_count_);
 }
 
+/// @brief Checks that D2ClientMgr suspendUpdates works properly.
+TEST_F(D2ClientMgrTest, udpSuspendUpdates) {
+    // Enable DDNS with server at 127.0.0.1/prot 53001 via UDP.
+    // Place sender in send mode.
+    enableDdns("127.0.0.1", 530001, dhcp_ddns::NCR_UDP);
+    ASSERT_NO_THROW(startSender(getErrorHandler()));
+
+    // Send a test request.
+    for (int i = 0; i < 3; ++i) {
+        dhcp_ddns::NameChangeRequestPtr ncr = buildTestNcr();
+        ASSERT_NO_THROW(sendRequest(ncr));
+    }
+    ASSERT_EQ(3, getQueueSize());
+
+    // Call the ready handler. This should complete the first message
+    // and initiate sending the second message.
+    ASSERT_NO_THROW(runReadyIO());
+
+    // Queue count should have gone down by 1.
+    ASSERT_EQ(2, getQueueSize());
+
+    // Suspend updates. This should disable updates and stop the sender.
+    ASSERT_NO_THROW(suspendUpdates());
+
+    EXPECT_FALSE(ddnsEnabled());
+    EXPECT_FALSE(amSending());
+
+    // Stopping the sender should have completed the second message's
+    // in-progess send, so queue size should be 1.
+    ASSERT_EQ(1, getQueueSize());
+}
 
 } // end of anonymous namespace