[3268] Modified NameChangeTransaction to use configurable DNS server timeout

Update manager now passes the D2 config manager reference into transactions
when it creates them. This makes D2 configuration services available at
the transaction level.

Changed NameChangeTransaction::sendUpdate() to get the timeout value from
configuration rather than use hard-coded constant.

src/bin/d2/d2_update_mgr.cc

@@ -195,10 +195,12 @@ D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) {
     NameChangeTransactionPtr trans;
     if (next_ncr->getChangeType() == dhcp_ddns::CHG_ADD) {
         trans.reset(new NameAddTransaction(io_service_, next_ncr,
-                                           forward_domain, reverse_domain));
+                                           forward_domain, reverse_domain,
+                                           cfg_mgr_));
     } else {
         trans.reset(new NameRemoveTransaction(io_service_, next_ncr,
-                                              forward_domain, reverse_domain));
+                                              forward_domain, reverse_domain,
+                                              cfg_mgr_));
     }
 
     // Add the new transaction to the list.

src/bin/d2/nc_add.cc

@@ -38,8 +38,10 @@ NameAddTransaction::
 NameAddTransaction(IOServicePtr& io_service,
                    dhcp_ddns::NameChangeRequestPtr& ncr,
                    DdnsDomainPtr& forward_domain,
-                   DdnsDomainPtr& reverse_domain)
-    : NameChangeTransaction(io_service, ncr, forward_domain, reverse_domain) {
+                   DdnsDomainPtr& reverse_domain,
+                   D2CfgMgrPtr& cfg_mgr)
+    : NameChangeTransaction(io_service, ncr, forward_domain, reverse_domain,
+                            cfg_mgr) {
     if (ncr->getChangeType() != isc::dhcp_ddns::CHG_ADD) {
         isc_throw (NameAddTransactionError,
                    "NameAddTransaction, request type must be CHG_ADD");

src/bin/d2/nc_add.h

@@ -87,13 +87,15 @@ public:
     /// @param ncr is the NameChangeRequest to fulfill
     /// @param forward_domain is the domain to use for forward DNS updates
     /// @param reverse_domain is the domain to use for reverse DNS updates
+    /// @param cfg_mgr pointer to the configuration manager
     ///
     /// @throw NameAddTransaction error if given request is not a CHG_ADD,
     /// NameChangeTransaction error for base class construction errors.
     NameAddTransaction(IOServicePtr& io_service,
                        dhcp_ddns::NameChangeRequestPtr& ncr,
                        DdnsDomainPtr& forward_domain,
-                       DdnsDomainPtr& reverse_domain);
+                       DdnsDomainPtr& reverse_domain,
+                       D2CfgMgrPtr& cfg_mgr);
 
     /// @brief Destructor
     virtual ~NameAddTransaction();

src/bin/d2/nc_remove.cc

@@ -35,8 +35,10 @@ NameRemoveTransaction::
 NameRemoveTransaction(IOServicePtr& io_service,
                    dhcp_ddns::NameChangeRequestPtr& ncr,
                    DdnsDomainPtr& forward_domain,
-                   DdnsDomainPtr& reverse_domain)
-    : NameChangeTransaction(io_service, ncr, forward_domain, reverse_domain) {
+                   DdnsDomainPtr& reverse_domain,
+                   D2CfgMgrPtr& cfg_mgr)
+    : NameChangeTransaction(io_service, ncr, forward_domain, reverse_domain,
+                            cfg_mgr) {
     if (ncr->getChangeType() != isc::dhcp_ddns::CHG_REMOVE) {
         isc_throw (NameRemoveTransactionError,
                    "NameRemoveTransaction, request type must be CHG_REMOVE");

src/bin/d2/nc_remove.h

@@ -83,13 +83,15 @@ public:
     /// @param ncr is the NameChangeRequest to fulfill
     /// @param forward_domain is the domain to use for forward DNS updates
     /// @param reverse_domain is the domain to use for reverse DNS updates
+    /// @param cfg_mgr pointer to the configuration manager
     ///
     /// @throw NameRemoveTransaction error if given request is not a CHG_REMOVE,
     /// NameChangeTransaction error for base class construction errors.
     NameRemoveTransaction(IOServicePtr& io_service,
                           dhcp_ddns::NameChangeRequestPtr& ncr,
                           DdnsDomainPtr& forward_domain,
-                          DdnsDomainPtr& reverse_domain);
+                          DdnsDomainPtr& reverse_domain,
+                          D2CfgMgrPtr& cfg_mgr);
 
     /// @brief Destructor
     virtual ~NameRemoveTransaction();

src/bin/d2/nc_trans.cc

@@ -41,22 +41,22 @@ const int NameChangeTransaction::UPDATE_FAILED_EVT;
 
 const int NameChangeTransaction::NCT_DERIVED_EVENT_MIN;
 
-const unsigned int NameChangeTransaction::DNS_UPDATE_DEFAULT_TIMEOUT;
 const unsigned int NameChangeTransaction::MAX_UPDATE_TRIES_PER_SERVER;
 
 NameChangeTransaction::
 NameChangeTransaction(IOServicePtr& io_service,
                       dhcp_ddns::NameChangeRequestPtr& ncr,
                       DdnsDomainPtr& forward_domain,
-                      DdnsDomainPtr& reverse_domain)
+                      DdnsDomainPtr& reverse_domain,
+                      D2CfgMgrPtr& cfg_mgr)
     : io_service_(io_service), ncr_(ncr), forward_domain_(forward_domain),
      reverse_domain_(reverse_domain), dns_client_(), dns_update_request_(),
      dns_update_status_(DNSClient::OTHER), dns_update_response_(),
      forward_change_completed_(false), reverse_change_completed_(false),
      current_server_list_(), current_server_(), next_server_pos_(0),
-     update_attempts_(0) {
-    // @todo if io_service is NULL we are multi-threading and should
-    // instantiate our own
+     update_attempts_(0), cfg_mgr_(cfg_mgr) {
+    /// @todo if io_service is NULL we are multi-threading and should
+    /// instantiate our own
     if (!io_service_) {
         isc_throw(NameChangeTransactionError, "IOServicePtr cannot be null");
     }
@@ -75,6 +75,11 @@ NameChangeTransaction(IOServicePtr& io_service,
         isc_throw(NameChangeTransactionError,
                  "Reverse change must have a reverse domain");
     }
+
+    if (!cfg_mgr_) {
+        isc_throw(NameChangeTransactionError,
+                  "Configuration manager cannot be null");
+    }
 }
 
 NameChangeTransaction::~NameChangeTransaction(){
@@ -171,11 +176,10 @@ NameChangeTransaction::sendUpdate(const std::string& comment,
         // use_tsig_ is true. We should be able to navigate to the TSIG key
         // for the current server.  If not we would need to add that.
 
-        // @todo time out should ultimately be configurable, down to
-        // server level?
+        D2ParamsPtr d2_params = cfg_mgr_->getD2Params();
         dns_client_->doUpdate(*io_service_, current_server_->getIpAddress(),
                               current_server_->getPort(), *dns_update_request_,
-                              DNS_UPDATE_DEFAULT_TIMEOUT);
+                              d2_params->getDnsServerTimeout());
 
         // Message is on its way, so the next event should be NOP_EVT.
         postNextEvent(NOP_EVT);

src/bin/d2/nc_trans.h

@@ -19,7 +19,7 @@
 
 #include <exceptions/exceptions.h>
 #include <d2/d2_asio.h>
-#include <d2/d2_config.h>
+#include <d2/d2_cfg_mgr.h>
 #include <d2/dns_client.h>
 #include <d2/state_model.h>
 #include <dhcp_ddns/ncr_msg.h>
@@ -175,7 +175,8 @@ public:
     NameChangeTransaction(IOServicePtr& io_service,
                           dhcp_ddns::NameChangeRequestPtr& ncr,
                           DdnsDomainPtr& forward_domain,
-                          DdnsDomainPtr& reverse_domain);
+                          DdnsDomainPtr& reverse_domain,
+                          D2CfgMgrPtr& cfg_mgr);
 
     /// @brief Destructor
     virtual ~NameChangeTransaction();
@@ -570,6 +571,9 @@ private:
 
     /// @brief Number of transmit attempts for the current request.
     size_t update_attempts_;
+
+    /// @brief Pointer to the configuration manager.
+    D2CfgMgrPtr cfg_mgr_;
 };
 
 /// @brief Defines a pointer to a NameChangeTransaction.

src/bin/d2/tests/d2_update_mgr_unittests.cc

@@ -201,29 +201,22 @@ public:
     /// timer, the number of passes through the loop is also limited to
     /// a given number.  This is a failsafe to guard against an infinite loop
     /// in the test.
-    ///
-    /// @param timeout_millisec Maximum amount of time to allow IOService to
-    /// run before failing the test.  Note, If the intent of a test is to
-    /// verify proper handling of DNSClient timeouts, the value must be
-    /// slightly larger than that being used for DNSClient timeout value.
-    /// @param max_passes Maximum number of times through the loop before
-    /// failing the test.  The default value of twenty is likely large enough
-    /// for most tests.  The number of passes required for a given test can
-    /// vary.
-    void processAll(unsigned int timeout_millisec =
-                    NameChangeTransaction::DNS_UPDATE_DEFAULT_TIMEOUT + 100,
-                    size_t max_passes = 100) {
+    void processAll(size_t max_passes = 100) {
         // Loop until all the transactions have been dequeued and run through to
         // completion.
         size_t passes = 0;
         size_t handlers = 0;
+
+        // Set the timeout to slightly more than DNSClient timeout to allow
+        // timeout processing to occur naturally.
+        size_t timeout = cfg_mgr_->getD2Params()->getDnsServerTimeout() + 100;
         while (update_mgr_->getQueueCount() ||
             update_mgr_->getTransactionCount()) {
             ++passes;
             update_mgr_->sweep();
             // If any transactions are waiting on IO, run the service.
             if (anyoneWaiting()) {
-                int cnt = runTimedIO(timeout_millisec);
+                int cnt = runTimedIO(timeout);
 
                 // If cnt is zero then the service stopped unexpectedly.
                 if (cnt == 0) {

src/bin/d2/tests/nc_add_unittests.cc

@@ -35,8 +35,10 @@ public:
     NameAddStub(IOServicePtr& io_service,
                 dhcp_ddns::NameChangeRequestPtr& ncr,
                 DdnsDomainPtr& forward_domain,
-                DdnsDomainPtr& reverse_domain)
-        : NameAddTransaction(io_service, ncr, forward_domain, reverse_domain),
+                DdnsDomainPtr& reverse_domain,
+                D2CfgMgrPtr& cfg_mgr)
+        : NameAddTransaction(io_service, ncr, forward_domain, reverse_domain,
+                             cfg_mgr),
           simulate_send_exception_(false),
           simulate_build_request_exception_(false) {
     }
@@ -212,7 +214,7 @@ public:
         // Now create the test transaction as would occur in update manager.
         return (NameAddStubPtr(new NameAddStub(io_service_, ncr_,
                                                forward_domain_,
-                                               reverse_domain_)));
+                                               reverse_domain_, cfg_mgr_)));
     }
 
     /// @brief Creates a transaction which requests an IPv6 DNS update.
@@ -230,7 +232,8 @@ public:
         // Now create the test transaction as would occur in update manager.
         return (NameAddStubPtr(new NameAddStub(io_service_, ncr_,
                                                forward_domain_,
-                                               reverse_domain_)));
+                                               reverse_domain_,
+                                               cfg_mgr_)));
     }
 
     /// @brief Create a test transaction at a known point in the state model.
@@ -266,6 +269,7 @@ public:
 /// 2. Valid construction functions properly
 TEST(NameAddTransaction, construction) {
     IOServicePtr io_service(new isc::asiolink::IOService());
+    D2CfgMgrPtr cfg_mgr(new D2CfgMgr());
 
     const char* msg_str =
         "{"
@@ -291,13 +295,14 @@ TEST(NameAddTransaction, construction) {
 
     // Verify that construction with wrong change type fails.
     EXPECT_THROW(NameAddTransaction(io_service, ncr,
-                                    forward_domain, reverse_domain),
+                                    forward_domain, reverse_domain, cfg_mgr),
                                     NameAddTransactionError);
 
     // Verify that a valid construction attempt works.
     ncr->setChangeType(isc::dhcp_ddns::CHG_ADD);
     EXPECT_NO_THROW(NameAddTransaction(io_service, ncr,
-                                       forward_domain, reverse_domain));
+                                       forward_domain, reverse_domain,
+                                       cfg_mgr));
 }
 
 /// @brief Tests event and state dictionary construction and verification.

src/bin/d2/tests/nc_remove_unittests.cc

@@ -35,9 +35,10 @@ public:
     NameRemoveStub(IOServicePtr& io_service,
                    dhcp_ddns::NameChangeRequestPtr& ncr,
                    DdnsDomainPtr& forward_domain,
-                   DdnsDomainPtr& reverse_domain)
+                   DdnsDomainPtr& reverse_domain,
+                   D2CfgMgrPtr& cfg_mgr)
         : NameRemoveTransaction(io_service, ncr, forward_domain,
-                                reverse_domain),
+                                reverse_domain, cfg_mgr),
           simulate_send_exception_(false),
           simulate_build_request_exception_(false) {
     }
@@ -212,7 +213,8 @@ public:
         // Now create the test transaction as would occur in update manager.
         return (NameRemoveStubPtr(new NameRemoveStub(io_service_, ncr_,
                                                      forward_domain_,
-                                                     reverse_domain_)));
+                                                     reverse_domain_,
+                                                     cfg_mgr_)));
     }
 
     /// @brief Creates a transaction which requests an IPv6 DNS update.
@@ -230,7 +232,8 @@ public:
         // Now create the test transaction as would occur in update manager.
         return (NameRemoveStubPtr(new NameRemoveStub(io_service_, ncr_,
                                                      forward_domain_,
-                                                     reverse_domain_)));
+                                                     reverse_domain_,
+                                                     cfg_mgr_)));
     }
 
     /// @brief Create a test transaction at a known point in the state model.
@@ -268,6 +271,7 @@ public:
 /// 2. Valid construction functions properly
 TEST(NameRemoveTransaction, construction) {
     IOServicePtr io_service(new isc::asiolink::IOService());
+    D2CfgMgrPtr cfg_mgr(new D2CfgMgr());
 
     const char* msg_str =
         "{"
@@ -293,13 +297,14 @@ TEST(NameRemoveTransaction, construction) {
 
     // Verify that construction with wrong change type fails.
     EXPECT_THROW(NameRemoveTransaction(io_service, ncr,
-                                       forward_domain, reverse_domain),
+                                       forward_domain, reverse_domain, cfg_mgr),
                                        NameRemoveTransactionError);
 
     // Verify that a valid construction attempt works.
     ncr->setChangeType(isc::dhcp_ddns::CHG_REMOVE);
     EXPECT_NO_THROW(NameRemoveTransaction(io_service, ncr,
-                                          forward_domain, reverse_domain));
+                                          forward_domain, reverse_domain,
+                                          cfg_mgr));
 }
 
 /// @brief Tests event and state dictionary construction and verification.

src/bin/d2/tests/nc_test_utils.cc

@@ -197,7 +197,7 @@ const unsigned int TransactionTest::FORWARD_CHG = 0x01;
 const unsigned int TransactionTest::REVERSE_CHG = 0x02;
 const unsigned int TransactionTest::FWD_AND_REV_CHG = REVERSE_CHG | FORWARD_CHG;
 
-TransactionTest::TransactionTest() : ncr_() {
+TransactionTest::TransactionTest() : ncr_(), cfg_mgr_(new D2CfgMgr()) {
 }
 
 TransactionTest::~TransactionTest() {

src/bin/d2/tests/nc_test_utils.h

@@ -166,6 +166,7 @@ public:
     dhcp_ddns::NameChangeRequestPtr ncr_;
     DdnsDomainPtr forward_domain_;
     DdnsDomainPtr reverse_domain_;
+    D2CfgMgrPtr cfg_mgr_;
 
     /// #brief constants used to specify change directions for a transaction.
     static const unsigned int FORWARD_CHG;      // Only forward change.

src/bin/d2/tests/nc_trans_unittests.cc

@@ -28,11 +28,13 @@
 
 #include <boost/function.hpp>
 #include <boost/bind.hpp>
+#include <boost/date_time/posix_time/posix_time.hpp>
 #include <gtest/gtest.h>
 
 using namespace std;
 using namespace isc;
 using namespace isc::d2;
+using namespace boost::posix_time;
 
 namespace {
 
@@ -59,10 +61,11 @@ public:
     /// Parameters match those needed by NameChangeTransaction.
     NameChangeStub(IOServicePtr& io_service,
                    dhcp_ddns::NameChangeRequestPtr& ncr,
-                   DdnsDomainPtr forward_domain,
-                   DdnsDomainPtr reverse_domain)
+                   DdnsDomainPtr& forward_domain,
+                   DdnsDomainPtr& reverse_domain,
+                   D2CfgMgrPtr& cfg_mgr)
         : NameChangeTransaction(io_service, ncr, forward_domain,
-                                reverse_domain),
+                                reverse_domain, cfg_mgr),
                                 use_stub_callback_(false) {
     }
 
@@ -296,7 +299,7 @@ public:
         // Now create the test transaction as would occur in update manager.
         // Instantiate the transaction as would be done by update manager.
         return (NameChangeStubPtr(new NameChangeStub(io_service_, ncr_,
-                                  forward_domain_, reverse_domain_)));
+                                  forward_domain_, reverse_domain_, cfg_mgr_)));
     }
 
     /// @brief Builds and then sends an update request
@@ -348,6 +351,7 @@ public:
 /// 4. Valid construction functions properly
 TEST(NameChangeTransaction, construction) {
     IOServicePtr io_service(new isc::asiolink::IOService());
+    D2CfgMgrPtr cfg_mgr(new D2CfgMgr());
 
     const char* msg_str =
         "{"
@@ -377,43 +381,54 @@ TEST(NameChangeTransaction, construction) {
     // @todo Subject to change if multi-threading is implemented.
     IOServicePtr empty;
     EXPECT_THROW(NameChangeTransaction(empty, ncr,
-                                       forward_domain, reverse_domain),
+                                       forward_domain, reverse_domain, cfg_mgr),
                                        NameChangeTransactionError);
 
     // Verify that construction with an empty NameChangeRequest throws.
     EXPECT_THROW(NameChangeTransaction(io_service, empty_ncr,
-                                       forward_domain, reverse_domain),
+                                       forward_domain, reverse_domain, cfg_mgr),
                                         NameChangeTransactionError);
 
+    // Verify that construction with an empty D2CfgMgr throws.
+    D2CfgMgrPtr empty_cfg;
+    EXPECT_THROW(NameChangeTransaction(io_service, empty_ncr,
+                                       forward_domain, reverse_domain,
+                                       empty_cfg),
+                                       NameChangeTransactionError);
+
+
     // Verify that construction with an empty forward domain when the
     // NameChangeRequest calls for a forward change throws.
     EXPECT_THROW(NameChangeTransaction(io_service, ncr,
-                                       empty_domain, reverse_domain),
+                                       empty_domain, reverse_domain, cfg_mgr),
                                        NameChangeTransactionError);
 
     // Verify that construction with an empty reverse domain when the
     // NameChangeRequest calls for a reverse change throws.
     EXPECT_THROW(NameChangeTransaction(io_service, ncr,
-                                       forward_domain, empty_domain),
+                                       forward_domain, empty_domain, cfg_mgr),
                                        NameChangeTransactionError);
 
     // Verify that a valid construction attempt works.
     EXPECT_NO_THROW(NameChangeTransaction(io_service, ncr,
-                                          forward_domain, reverse_domain));
+                                          forward_domain, reverse_domain,
+                                          cfg_mgr));
 
     // Verify that an empty forward domain is allowed when the requests does
     // not include a forward change.
     ncr->setForwardChange(false);
     ncr->setReverseChange(true);
     EXPECT_NO_THROW(NameChangeTransaction(io_service, ncr,
-                                          empty_domain, reverse_domain));
+                                          empty_domain, reverse_domain,
+                                          cfg_mgr));
 
     // Verify that an empty reverse domain is allowed when the requests does
     // not include a reverse change.
     ncr->setForwardChange(true);
     ncr->setReverseChange(false);
     EXPECT_NO_THROW(NameChangeTransaction(io_service, ncr,
-                                          forward_domain, empty_domain));
+                                          forward_domain, empty_domain,
+                                          cfg_mgr));
 }
 
 /// @brief General testing of member accessors.
@@ -944,9 +959,11 @@ TEST_F(NameChangeTransactionTest, sendUpdateTimeout) {
     // not only it but the invoking test as well. In other words, if the
     // doOneExchange blows up the rest of test is pointless. I use
     // ASSERT_NO_FATAL_FAILURE to abort the test immediately.
-    ASSERT_NO_FATAL_FAILURE(doOneExchange(name_change,
-                                          NameChangeTransaction::
-                                          DNS_UPDATE_DEFAULT_TIMEOUT + 100));
+
+    D2ParamsPtr d2_params = cfg_mgr_->getD2Params();
+    size_t timeout = d2_params->getDnsServerTimeout() + 100;
+
+    ASSERT_NO_FATAL_FAILURE(doOneExchange(name_change, timeout));
 
     // Verify that next event is IO_COMPLETED_EVT and DNS status is TIMEOUT.
     ASSERT_EQ(NameChangeTransaction::IO_COMPLETED_EVT,