Browse Source

[master] Merge branch 'trac3089'

  Integrates d2::NameAddTransaction and d2::NameRemoveTransaction into
  d2::D2UpdateMgr.
Thomas Markwalder 11 years ago
parent
commit
9ff948a169

+ 19 - 3
src/bin/d2/d2_config.cc

@@ -21,6 +21,8 @@
 #include <boost/foreach.hpp>
 #include <boost/lexical_cast.hpp>
 
+#include <string>
+
 namespace isc {
 namespace d2 {
 
@@ -49,6 +51,20 @@ DnsServerInfo::DnsServerInfo(const std::string& hostname,
 DnsServerInfo::~DnsServerInfo() {
 }
 
+std::string
+DnsServerInfo::toText() const {
+    std::ostringstream stream;
+    stream << (getIpAddress().toText()) << " port:" << getPort();
+    return (stream.str());
+}
+
+
+std::ostream&
+operator<<(std::ostream& os, const DnsServerInfo& server) {
+    os << server.toText();
+    return (os);
+}
+
 // *********************** DdnsDomain  *************************
 
 DdnsDomain::DdnsDomain(const std::string& name, const std::string& key_name,
@@ -253,7 +269,7 @@ TSIGKeyInfoParser::commit() {
 
 TSIGKeyInfoListParser::TSIGKeyInfoListParser(const std::string& list_name,
                                        TSIGKeyInfoMapPtr keys)
-    :list_name_(list_name), keys_(keys), local_keys_(new TSIGKeyInfoMap()), 
+    :list_name_(list_name), keys_(keys), local_keys_(new TSIGKeyInfoMap()),
      parsers_() {
     if (!keys_) {
         isc_throw(D2CfgError, "TSIGKeyInfoListParser ctor:"
@@ -291,9 +307,9 @@ TSIGKeyInfoListParser::commit() {
     BOOST_FOREACH(isc::dhcp::ParserPtr parser, parsers_) {
         parser->commit();
     }
-  
+
     // Now that we know we have a valid list, commit that list to the
-    // area given to us during construction (i.e. to the d2 context).   
+    // area given to us during construction (i.e. to the d2 context).
     *keys_ = *local_keys_;
 }
 

+ 8 - 1
src/bin/d2/d2_config.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -159,6 +159,7 @@ public:
     /// @param name the unique label used to identify this key
     /// @param algorithm the name of the encryption alogirthm this key uses.
     /// (@todo This will be a fixed list of choices)
+    ///
     /// @param secret the secret component of this key
     TSIGKeyInfo(const std::string& name, const std::string& algorithm,
                 const std::string& secret);
@@ -288,6 +289,9 @@ public:
         enabled_ = false;
     }
 
+    /// @brief Returns a text representation for the server.
+    std::string toText() const;
+
 
 private:
     /// @brief The resolvable name of the server. If not blank, then the
@@ -306,6 +310,9 @@ private:
     bool enabled_;
 };
 
+std::ostream&
+operator<<(std::ostream& os, const DnsServerInfo& server);
+
 /// @brief Defines a pointer for DnsServerInfo instances.
 typedef boost::shared_ptr<DnsServerInfo> DnsServerInfoPtr;
 

+ 10 - 1
src/bin/d2/d2_messages.mes

@@ -1,4 +1,4 @@
-# Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+# Copyright (C) 2013-2014  Internet Systems Consortium, Inc. ("ISC")
 #
 # Permission to use, copy, modify, and/or distribute this software for any
 # purpose with or without fee is hereby granted, provided that the above
@@ -438,3 +438,12 @@ This is an error message issued after DHCP_DDNS attempts to submit DNS mapping
 entry removals have failed.  The precise reason for the failure should be
 documented in preceding log entries.
 
+% DHCP_DDNS_STARTING_TRANSACTION Transaction Key: %1
+
+% DHCP_DDNS_UPDATE_REQUEST_SENT for transaction key: %1 to server: %2
+This is a debug message issued when DHCP_DDNS sends a DNS request to a DNS
+server.
+
+% DHCP_DDNS_UPDATE_RESPONSE_RECEIVED for transaction key: %1  to server: %2 status: %3
+This is a debug message issued when DHCP_DDNS receives sends a DNS update
+response from a DNS server.

+ 27 - 12
src/bin/d2/d2_update_mgr.cc

@@ -13,6 +13,8 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <d2/d2_update_mgr.h>
+#include <d2/nc_add.h>
+#include <d2/nc_remove.h>
 
 #include <sstream>
 #include <iostream>
@@ -85,16 +87,12 @@ D2UpdateMgr::checkFinishedTransactions() {
     TransactionList::iterator it = transaction_list_.begin();
     while (it != transaction_list_.end()) {
         NameChangeTransactionPtr trans = (*it).second;
-        switch (trans->getNcrStatus())  {
-        case dhcp_ddns::ST_COMPLETED:
+        if (trans->isModelDone()) {
+            // @todo  Addtional actions based on NCR status could be
+            // performed here.
             transaction_list_.erase(it++);
-            break;
-        case dhcp_ddns::ST_FAILED:
-            transaction_list_.erase(it++);
-            break;
-        default:
+        } else {
             ++it;
-            break;
         }
     }
 }
@@ -163,12 +161,24 @@ D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) {
     }
 
     // We matched to the required servers, so construct the transaction.
-    NameChangeTransactionPtr trans(new NameChangeTransaction(io_service_,
-                                                             next_ncr,
-                                                             forward_domain,
-                                                             reverse_domain));
+    // @todo If multi-threading is implemented, one would pass in an
+    // empty IOServicePtr, rather than our instance value.  This would cause
+    // the transaction to instantiate its own, separate IOService to handle
+    // the transaction's IO.
+    NameChangeTransactionPtr trans;
+    if (next_ncr->getChangeType() == dhcp_ddns::CHG_ADD) {
+        trans.reset(new NameAddTransaction(io_service_, next_ncr,
+                                           forward_domain, reverse_domain));
+    } else {
+        trans.reset(new NameRemoveTransaction(io_service_, next_ncr,
+                                              forward_domain, reverse_domain));
+    }
+
     // Add the new transaction to the list.
     transaction_list_[key] = trans;
+
+    // Start it.
+    trans->startTransaction();
 }
 
 TransactionList::iterator
@@ -190,6 +200,11 @@ D2UpdateMgr::removeTransaction(const TransactionKey& key) {
 }
 
 TransactionList::iterator
+D2UpdateMgr::transactionListBegin() {
+    return (transaction_list_.begin());
+}
+
+TransactionList::iterator
 D2UpdateMgr::transactionListEnd() {
     return (transaction_list_.end());
 }

+ 10 - 0
src/bin/d2/d2_update_mgr.h

@@ -162,6 +162,13 @@ protected:
     void makeTransaction(isc::dhcp_ddns::NameChangeRequestPtr& ncr);
 
 public:
+    /// @brief Gets the UpdateMgr's IOService.
+    ///
+    /// @return returns a reference to the IOService
+    const IOServicePtr& getIOService() {
+        return (io_service_);
+    }
+
     /// @brief Returns the maximum number of concurrent transactions.
     size_t getMaxTransactions() const {
         return (max_transactions_);
@@ -187,6 +194,9 @@ public:
     /// @brief Returns the transaction list end position.
     TransactionList::iterator transactionListEnd();
 
+    /// @brief Returns the transaction list beg position.
+    TransactionList::iterator transactionListBegin();
+
     /// @brief Convenience method that checks transaction list for the given key
     ///
     /// @param key the transaction key value for which to search.

+ 22 - 8
src/bin/d2/dns_client.cc

@@ -44,8 +44,17 @@ class DNSClientImpl : public asiodns::IOFetch::Callback {
 public:
     // A buffer holding response from a DNS.
     util::OutputBufferPtr in_buf_;
-    // A caller-supplied object holding a parsed response from DNS.
-    D2UpdateMessagePtr response_;
+    // A caller-supplied object which will hold the parsed response from DNS.
+    // The response object is (or descends from) isc::dns::Message and is
+    // populated using Message::fromWire().  This method may only be called
+    // once in the lifetime of a Message instance.  Therefore, response_ is a
+    // pointer reference thus allowing this class to replace the object
+    // pointed to with a new Message instance each time a message is
+    // received. This allows a single DNSClientImpl instance to be used for
+    // multiple, sequential IOFetch calls. (@todo Trac# 3286 has been opened
+    // against dns::Message::fromWire.  Should the behavior of fromWire change
+    // the behavior here with could be rexamined).
+    D2UpdateMessagePtr& response_;
     // A caller-supplied external callback which is invoked when DNS message
     // exchange is complete or interrupted.
     DNSClient::Callback* callback_;
@@ -81,6 +90,12 @@ DNSClientImpl::DNSClientImpl(D2UpdateMessagePtr& response_placeholder,
     : in_buf_(new OutputBuffer(DEFAULT_BUFFER_SIZE)),
       response_(response_placeholder), callback_(callback), proto_(proto) {
 
+    // Response should be an empty pointer. It gets populated by the
+    // operator() method.
+    if (response_) {
+        isc_throw(isc::BadValue, "Response buffer pointer should be null");
+    }
+
     // @todo Currently we only support UDP. The support for TCP is planned for
     // the future release.
     if (proto_ == DNSClient::TCP) {
@@ -104,12 +119,6 @@ DNSClientImpl::DNSClientImpl(D2UpdateMessagePtr& response_placeholder,
                       << proto_ << "' specified for DNS Updates");
         }
     }
-
-    if (!response_) {
-        isc_throw(BadValue, "a pointer to an object to encapsulate the DNS"
-                  " server must be provided; found NULL value");
-
-    }
 }
 
 DNSClientImpl::~DNSClientImpl() {
@@ -122,6 +131,11 @@ DNSClientImpl::operator()(asiodns::IOFetch::Result result) {
     DNSClient::Status status = getStatus(result);
     if (status == DNSClient::SUCCESS) {
         InputBuffer response_buf(in_buf_->getData(), in_buf_->getLength());
+        // Allocate a new response message. (Note that Message::fromWire
+        // may only be run once per message, so we need to start fresh
+        // each time.)
+        response_.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
+
         // Server's response may be corrupted. In such case, fromWire will
         // throw an exception. We want to catch this exception to return
         // appropriate status code to the caller and log this event.

+ 2 - 2
src/bin/d2/dns_client.h

@@ -93,8 +93,8 @@ public:
 
     /// @brief Constructor.
     ///
-    /// @param response_placeholder Pointer to an object which will hold a
-    /// DNS server's response. Caller is responsible for allocating this object.
+    /// @param response_placeholder Messge object pointer which will be updated
+    /// with dynamically allocated object holding the DNS server's response.
     /// @param callback Pointer to an object implementing @c DNSClient::Callback
     /// class. This object will be called when DNS message exchange completes or
     /// if an error occurs. NULL value disables callback invocation.

+ 13 - 13
src/bin/d2/nc_add.cc

@@ -230,7 +230,7 @@ NameAddTransaction::addingFwdAddrsHandler() {
                 // If we get not authorized should we try the next server in
                 // the list? @todo  This needs some discussion perhaps.
                 LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_ADD_REJECTED)
-                          .arg(getCurrentServer()->getIpAddress())
+                          .arg(getCurrentServer()->toText())
                           .arg(getNcr()->getFqdn())
                           .arg(rcode.getCode());
                 transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
@@ -247,7 +247,7 @@ NameAddTransaction::addingFwdAddrsHandler() {
             // is not entirely clear if this is accurate.
             LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_ADD_IO_ERROR)
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             retryTransition(SELECTING_FWD_SERVER_ST);
             break;
@@ -256,7 +256,7 @@ NameAddTransaction::addingFwdAddrsHandler() {
             // A response was received but was corrupt. Retry it like an IO
             // error.
             LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_ADD_RESP_CORRUPT)
-                      .arg(getCurrentServer()->getIpAddress())
+                      .arg(getCurrentServer()->toText())
                       .arg(getNcr()->getFqdn());
 
             retryTransition(SELECTING_FWD_SERVER_ST);
@@ -268,7 +268,7 @@ NameAddTransaction::addingFwdAddrsHandler() {
             LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_ADD_BAD_DNSCLIENT_STATUS)
                       .arg(getDnsUpdateStatus())
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
             break;
@@ -342,7 +342,7 @@ NameAddTransaction::replacingFwdAddrsHandler() {
                 // If we get not authorized should try the next server in
                 // the list? @todo  This needs some discussion perhaps.
                 LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_REPLACE_REJECTED)
-                          .arg(getCurrentServer()->getIpAddress())
+                          .arg(getCurrentServer()->toText())
                           .arg(getNcr()->getFqdn())
                           .arg(rcode.getCode());
                 transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
@@ -359,7 +359,7 @@ NameAddTransaction::replacingFwdAddrsHandler() {
             // is not entirely clear if this is accurate.
             LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_REPLACE_IO_ERROR)
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             // If we are out of retries on this server, we go back and start
             // all over on a new server.
@@ -370,7 +370,7 @@ NameAddTransaction::replacingFwdAddrsHandler() {
             // A response was received but was corrupt. Retry it like an IO
             // error.
             LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_REPLACE_RESP_CORRUPT)
-                      .arg(getCurrentServer()->getIpAddress())
+                      .arg(getCurrentServer()->toText())
                       .arg(getNcr()->getFqdn());
 
             // If we are out of retries on this server, we go back and start
@@ -385,7 +385,7 @@ NameAddTransaction::replacingFwdAddrsHandler() {
                       DHCP_DDNS_FORWARD_REPLACE_BAD_DNSCLIENT_STATUS)
                       .arg(getDnsUpdateStatus())
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
             break;
@@ -476,7 +476,7 @@ NameAddTransaction::replacingRevPtrsHandler() {
                 // If we get not authorized should try the next server in
                 // the list? @todo  This needs some discussion perhaps.
                 LOG_ERROR(dctl_logger, DHCP_DDNS_REVERSE_REPLACE_REJECTED)
-                          .arg(getCurrentServer()->getIpAddress())
+                          .arg(getCurrentServer()->toText())
                           .arg(getNcr()->getFqdn())
                           .arg(rcode.getCode());
                 transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
@@ -493,7 +493,7 @@ NameAddTransaction::replacingRevPtrsHandler() {
             // is not entirely clear if this is accurate.
             LOG_ERROR(dctl_logger, DHCP_DDNS_REVERSE_REPLACE_IO_ERROR)
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             // If we are out of retries on this server, we go back and start
             // all over on a new server.
@@ -504,7 +504,7 @@ NameAddTransaction::replacingRevPtrsHandler() {
             // A response was received but was corrupt. Retry it like an IO
             // error.
             LOG_ERROR(dctl_logger, DHCP_DDNS_REVERSE_REPLACE_RESP_CORRUPT)
-                      .arg(getCurrentServer()->getIpAddress())
+                      .arg(getCurrentServer()->toText())
                       .arg(getNcr()->getFqdn());
 
             // If we are out of retries on this server, we go back and start
@@ -519,7 +519,7 @@ NameAddTransaction::replacingRevPtrsHandler() {
                       DHCP_DDNS_REVERSE_REPLACE_BAD_DNSCLIENT_STATUS)
                       .arg(getDnsUpdateStatus())
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
             break;
@@ -557,7 +557,7 @@ NameAddTransaction::processAddFailedHandler() {
     case UPDATE_FAILED_EVT:
     case NO_MORE_SERVERS_EVT:
         LOG_ERROR(dctl_logger, DHCP_DDNS_ADD_FAILED).arg(getNcr()->toText())
-        .arg(getContextStr());
+        .arg(getEventLabel(getNextEvent()));
         setNcrStatus(dhcp_ddns::ST_FAILED);
         endModel();
         break;

+ 12 - 12
src/bin/d2/nc_remove.cc

@@ -225,7 +225,7 @@ NameRemoveTransaction::removingFwdAddrsHandler() {
                 // If we get not authorized should we try the next server in
                 // the list? @todo  This needs some discussion perhaps.
                 LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_REMOVE_ADDRS_REJECTED)
-                          .arg(getCurrentServer()->getIpAddress())
+                          .arg(getCurrentServer()->toText())
                           .arg(getNcr()->getFqdn())
                           .arg(rcode.getCode());
                 transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
@@ -242,7 +242,7 @@ NameRemoveTransaction::removingFwdAddrsHandler() {
             // is not entirely clear if this is accurate.
             LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_REMOVE_ADDRS_IO_ERROR)
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             retryTransition(SELECTING_FWD_SERVER_ST);
             break;
@@ -251,7 +251,7 @@ NameRemoveTransaction::removingFwdAddrsHandler() {
             // A response was received but was corrupt. Retry it like an IO
             // error.
             LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_REMOVE_ADDRS_RESP_CORRUPT)
-                      .arg(getCurrentServer()->getIpAddress())
+                      .arg(getCurrentServer()->toText())
                       .arg(getNcr()->getFqdn());
 
             retryTransition(SELECTING_FWD_SERVER_ST);
@@ -264,7 +264,7 @@ NameRemoveTransaction::removingFwdAddrsHandler() {
                       DHCP_DDNS_FORWARD_REMOVE_ADDRS_BAD_DNSCLIENT_STATUS)
                       .arg(getDnsUpdateStatus())
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
             break;
@@ -340,7 +340,7 @@ NameRemoveTransaction::removingFwdRRsHandler() {
                 // If we get not authorized should try the next server in
                 // the list? @todo  This needs some discussion perhaps.
                 LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_REMOVE_RRS_REJECTED)
-                          .arg(getCurrentServer()->getIpAddress())
+                          .arg(getCurrentServer()->toText())
                           .arg(getNcr()->getFqdn())
                           .arg(rcode.getCode());
                 transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
@@ -357,7 +357,7 @@ NameRemoveTransaction::removingFwdRRsHandler() {
             // is not entirely clear if this is accurate.
             LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_REMOVE_RRS_IO_ERROR)
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             // @note If we exhaust the IO retries for the current server
             // due to IO failures, we will abort the remaining updates.
@@ -374,7 +374,7 @@ NameRemoveTransaction::removingFwdRRsHandler() {
             // A response was received but was corrupt. Retry it like an IO
             // error.
             LOG_ERROR(dctl_logger, DHCP_DDNS_FORWARD_REMOVE_RRS_RESP_CORRUPT)
-                      .arg(getCurrentServer()->getIpAddress())
+                      .arg(getCurrentServer()->toText())
                       .arg(getNcr()->getFqdn());
 
             // If we are out of retries on this server abandon the transaction.
@@ -389,7 +389,7 @@ NameRemoveTransaction::removingFwdRRsHandler() {
                       DHCP_DDNS_FORWARD_REMOVE_RRS_BAD_DNSCLIENT_STATUS)
                       .arg(getDnsUpdateStatus())
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
             break;
@@ -483,7 +483,7 @@ NameRemoveTransaction::removingRevPtrsHandler() {
                 // If we get not authorized should try the next server in
                 // the list? @todo  This needs some discussion perhaps.
                 LOG_ERROR(dctl_logger, DHCP_DDNS_REVERSE_REMOVE_REJECTED)
-                          .arg(getCurrentServer()->getIpAddress())
+                          .arg(getCurrentServer()->toText())
                           .arg(getNcr()->getFqdn())
                           .arg(rcode.getCode());
                 transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
@@ -500,7 +500,7 @@ NameRemoveTransaction::removingRevPtrsHandler() {
             // is not entirely clear if this is accurate.
             LOG_ERROR(dctl_logger, DHCP_DDNS_REVERSE_REMOVE_IO_ERROR)
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             // If we are out of retries on this server, we go back and start
             // all over on a new server.
@@ -511,7 +511,7 @@ NameRemoveTransaction::removingRevPtrsHandler() {
             // A response was received but was corrupt. Retry it like an IO
             // error.
             LOG_ERROR(dctl_logger, DHCP_DDNS_REVERSE_REMOVE_RESP_CORRUPT)
-                      .arg(getCurrentServer()->getIpAddress())
+                      .arg(getCurrentServer()->toText())
                       .arg(getNcr()->getFqdn());
 
             // If we are out of retries on this server, we go back and start
@@ -526,7 +526,7 @@ NameRemoveTransaction::removingRevPtrsHandler() {
                       DHCP_DDNS_REVERSE_REMOVE_BAD_DNSCLIENT_STATUS)
                       .arg(getDnsUpdateStatus())
                       .arg(getNcr()->getFqdn())
-                      .arg(getCurrentServer()->getIpAddress());
+                      .arg(getCurrentServer()->toText());
 
             transition(PROCESS_TRANS_FAILED_ST, UPDATE_FAILED_EVT);
             break;

+ 19 - 2
src/bin/d2/nc_trans.cc

@@ -80,6 +80,11 @@ NameChangeTransaction::~NameChangeTransaction(){
 
 void
 NameChangeTransaction::startTransaction() {
+    LOG_DEBUG(dctl_logger, DBGLVL_TRACE_DETAIL,
+              DHCP_DDNS_STARTING_TRANSACTION)
+              .arg(getTransactionKey().toStr());
+
+    setNcrStatus(dhcp_ddns::ST_PENDING);
     startModel(READY_ST);
 }
 
@@ -89,6 +94,12 @@ NameChangeTransaction::operator()(DNSClient::Status status) {
     // set to indicate IO completed.
     // runModel is exception safe so we are good to call it here.
     // It won't exit until we hit the next IO wait or the state model ends.
+    LOG_DEBUG(dctl_logger, DBGLVL_TRACE_DETAIL,
+              DHCP_DDNS_UPDATE_RESPONSE_RECEIVED)
+              .arg(getTransactionKey().toStr())
+              .arg(current_server_->toText())
+              .arg(status);
+
     setDnsUpdateStatus(status);
     runModel(IO_COMPLETED_EVT);
 }
@@ -109,6 +120,10 @@ NameChangeTransaction::sendUpdate(bool /* use_tsig_ */) {
 
         // Message is on its way, so the next event should be NOP_EVT.
         postNextEvent(NOP_EVT);
+        LOG_DEBUG(dctl_logger, DBGLVL_TRACE_DETAIL,
+                  DHCP_DDNS_UPDATE_REQUEST_SENT)
+                  .arg(getTransactionKey().toStr())
+                  .arg(current_server_->toText());
     } catch (const std::exception& ex) {
         // We were unable to initiate the send.
         // It is presumed that any throw from doUpdate is due to a programmatic
@@ -200,6 +215,7 @@ NameChangeTransaction::setDnsUpdateRequest(D2UpdateMessagePtr& request) {
 
 void
 NameChangeTransaction::clearDnsUpdateRequest() {
+    update_attempts_ = 0;
     dns_update_request_.reset();
 }
 
@@ -354,8 +370,9 @@ NameChangeTransaction::selectNextServer() {
     if ((current_server_list_) &&
         (next_server_pos_ < current_server_list_->size())) {
         current_server_  = (*current_server_list_)[next_server_pos_];
-        dns_update_response_.reset(new
-                                   D2UpdateMessage(D2UpdateMessage::INBOUND));
+        // Toss out any previous response.
+        dns_update_response_.reset();
+
         // @todo  Protocol is set on DNSClient constructor.  We need
         // to propagate a configuration value downward, probably starting
         // at global, then domain, then server

+ 21 - 17
src/bin/d2/nc_trans.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -152,7 +152,10 @@ public:
     //@}
 
     /// @brief Defualt time to assign to a single DNS udpate.
-    static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 5 * 1000;
+    /// @todo  This value will be made configurable in the very near future
+    /// under trac3268. For now we will define it to 100 milliseconds
+    /// so unit tests will run within a reasonable amount of time.
+    static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 100;
 
     /// @brief Maximum times to attempt a single update on a given server.
     static const unsigned int MAX_UPDATE_TRIES_PER_SERVER = 3;
@@ -287,7 +290,8 @@ protected:
     /// @param request is the new request packet to assign.
     void setDnsUpdateRequest(D2UpdateMessagePtr& request);
 
-    /// @brief Destroys the current update request packet.
+    /// @brief Destroys the current update request packet and resets
+    /// udpate attempts count.
     void clearDnsUpdateRequest();
 
     /// @brief Sets the update status to the given status value.
@@ -341,17 +345,6 @@ protected:
     /// servers from which to select.
     bool selectNextServer();
 
-    /// @brief Fetches the currently selected server.
-    ///
-    /// @return A const pointer reference to the DnsServerInfo of the current
-    /// server.
-    const DnsServerInfoPtr& getCurrentServer() const;
-
-    /// @brief Fetches the DNSClient instance
-    ///
-    /// @return A const pointer reference to the DNSClient
-    const DNSClientPtr& getDNSClient() const;
-
     /// @brief Sets the update attempt count to the given value.
     ///
     /// @param value is the new value to assign.
@@ -380,7 +373,7 @@ protected:
     /// Creates an in::A() or in:AAAA() RData instance from the NCR
     /// lease address and adds it to the given RRset.
     ///
-    /// @param RRset RRset to which to add the RData
+    /// @param rrset RRset to which to add the RData
     ///
     /// @throw NameChangeTransactionError if RData cannot be constructed or
     /// the RData cannot be added to the given RRset.
@@ -391,7 +384,7 @@ protected:
     /// Creates an in::DHCID() RData instance from the NCR DHCID and adds
     /// it to the given RRset.
     ///
-    /// @param RRset RRset to which to add the RData
+    /// @param rrset RRset to which to add the RData
     ///
     /// @throw NameChangeTransactionError if RData cannot be constructed or
     /// the RData cannot be added to the given RRset.
@@ -402,7 +395,7 @@ protected:
     /// Creates an in::PTR() RData instance from the NCR FQDN and adds
     /// it to the given RRset.
     ///
-    /// @param RRset RRset to which to add the RData
+    /// @param rrset RRset to which to add the RData
     ///
     /// @throw NameChangeTransactionError if RData cannot be constructed or
     /// the RData cannot be added to the given RRset.
@@ -445,6 +438,17 @@ public:
     /// the request does not include a reverse change, the pointer will empty.
     DdnsDomainPtr& getReverseDomain();
 
+    /// @brief Fetches the currently selected server.
+    ///
+    /// @return A const pointer reference to the DnsServerInfo of the current
+    /// server.
+    const DnsServerInfoPtr& getCurrentServer() const;
+
+    /// @brief Fetches the DNSClient instance
+    ///
+    /// @return A const pointer reference to the DNSClient
+    const DNSClientPtr& getDNSClient() const;
+
     /// @brief Fetches the current DNS update request packet.
     ///
     /// @return A const pointer reference to the current D2UpdateMessage

+ 355 - 33
src/bin/d2/tests/d2_update_mgr_unittests.cc

@@ -16,6 +16,7 @@
 #include <d2/d2_update_mgr.h>
 #include <util/time_utilities.h>
 #include <d_test_stubs.h>
+#include <nc_test_utils.h>
 
 #include <boost/function.hpp>
 #include <boost/bind.hpp>
@@ -31,9 +32,9 @@ using namespace isc::d2;
 
 namespace {
 
-/// @brief Wrapper class for D2UpdateMgr to provide acces non-public methods.
+/// @brief Wrapper class for D2UpdateMgr providing access to non-public methods.
 ///
-/// This class faciliates testing by making non-public methods accessible so
+/// This class facilitates testing by making non-public methods accessible so
 /// they can be invoked directly in test routines.
 class D2UpdateMgrWrapper : public D2UpdateMgr {
 public:
@@ -66,18 +67,15 @@ typedef boost::shared_ptr<D2UpdateMgrWrapper> D2UpdateMgrWrapperPtr;
 /// D2CfgMgr.  This fixture provides an instance of each, plus a canned,
 /// valid DHCP_DDNS configuration sufficient to test D2UpdateMgr's basic
 /// functions.
-class D2UpdateMgrTest : public ConfigParseTest {
+class D2UpdateMgrTest : public TimedIO, public ConfigParseTest {
 public:
-    IOServicePtr io_service_;
     D2QueueMgrPtr queue_mgr_;
     D2CfgMgrPtr cfg_mgr_;
-    //D2UpdateMgrPtr update_mgr_;
     D2UpdateMgrWrapperPtr update_mgr_;
     std::vector<NameChangeRequestPtr> canned_ncrs_;
     size_t canned_count_;
 
     D2UpdateMgrTest() {
-        io_service_.reset(new isc::asiolink::IOService());
         queue_mgr_.reset(new D2QueueMgr(io_service_));
         cfg_mgr_.reset(new D2CfgMgr());
         update_mgr_.reset(new D2UpdateMgrWrapper(queue_mgr_, cfg_mgr_,
@@ -99,8 +97,8 @@ public:
         " \"change_type\" : 0 , "
         " \"forward_change\" : true , "
         " \"reverse_change\" : false , "
-        " \"fqdn\" : \"walah.walah.org.\" , "
-        " \"ip_address\" : \"192.168.2.1\" , "
+        " \"fqdn\" : \"my.example.com.\" , "
+        " \"ip_address\" : \"192.168.1.2\" , "
         " \"dhcid\" : \"0102030405060708\" , "
         " \"lease_expires_on\" : \"20130121132405\" , "
         " \"lease_length\" : 1300 "
@@ -112,6 +110,8 @@ public:
             dhcp_ddns::NameChangeRequestPtr ncr = NameChangeRequest::
                                                   fromJSON(msg_str);
             ncr->setDhcid(dhcids[i]);
+            ncr->setChangeType(i % 2 == 0 ?
+                               dhcp_ddns::CHG_ADD : dhcp_ddns::CHG_REMOVE);
             canned_ncrs_.push_back(ncr);
         }
     }
@@ -126,9 +126,9 @@ public:
                   "\"tsig_keys\": [] ,"
                   "\"forward_ddns\" : {"
                   "\"ddns_domains\": [ "
-                  "{ \"name\": \"two.three.org.\" , "
+                  "{ \"name\": \"example.com.\" , "
                   "  \"dns_servers\" : [ "
-                  "  { \"ip_address\": \"127.0.0.1\" } "
+                  "  { \"ip_address\": \"127.0.0.1\", \"port\" : 5301  } "
                   "  ] },"
                   "{ \"name\": \"org.\" , "
                   "  \"dns_servers\" : [ "
@@ -139,7 +139,7 @@ public:
                   "\"ddns_domains\": [ "
                   "{ \"name\": \"1.168.192.in-addr.arpa.\" , "
                   "  \"dns_servers\" : [ "
-                  "  { \"ip_address\": \"127.0.0.1\" } "
+                  "  { \"ip_address\": \"127.0.0.1\", \"port\" : 5301 } "
                   "  ] }, "
                   "{ \"name\": \"2.0.3.0.8.B.D.0.1.0.0.2.ip6.arpa.\" , "
                   "  \"dns_servers\" : [ "
@@ -153,6 +153,101 @@ public:
         ASSERT_TRUE(checkAnswer(0));
     }
 
+    /// @brief Fakes the completion of a given transaction.
+    ///
+    /// @param index index of the request from which the transaction was formed.
+    /// @param status completion status to assign to the request
+    void completeTransaction(const size_t index,
+                             const dhcp_ddns::NameChangeStatus& status) {
+        // add test on index
+        if (index >= canned_count_) {
+            ADD_FAILURE() << "request index is out of range: " << index;
+        }
+
+        const dhcp_ddns::D2Dhcid key = canned_ncrs_[index]->getDhcid();
+
+        // locate the transaction based on the request DHCID
+        TransactionList::iterator pos = update_mgr_->findTransaction(key);
+        if (pos == update_mgr_->transactionListEnd()) {
+            ADD_FAILURE() << "cannot find transaction for key: " << key.toStr();
+        }
+
+        NameChangeTransactionPtr trans = (*pos).second;
+        // Update the status of the request
+        trans->getNcr()->setStatus(status);
+        // End the model.
+        trans->endModel();
+    }
+
+    /// @brief Determines if any transactions are waiting for IO completion.
+    ///
+    /// @returns True if isModelWaiting() is true for at least one of the current
+    /// transactions.
+    bool anyoneWaiting() {
+        TransactionList::iterator it = update_mgr_->transactionListBegin();
+        while (it != update_mgr_->transactionListEnd()) {
+            if (((*it).second)->isModelWaiting()) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    /// @brief Process events until all requests have been completed.
+    ///
+    /// This method iteratively calls D2UpdateMgr::sweep and executes
+    /// IOService calls until both the request queue and transaction list
+    /// are empty or a timeout occurs.  Note that in addition to the safety
+    /// 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 = 20) {
+        // Loop until all the transactions have been dequeued and run through to
+        // completion.
+        size_t passes = 0;
+        size_t handlers = 0;
+        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);
+
+                // If cnt is zero then the service stopped unexpectedly.
+                if (cnt == 0) {
+                    ADD_FAILURE()
+                        << "processALL: IO service stopped unexpectedly,"
+                        << " passes: " << passes << ", handlers executed: "
+                        << handlers;
+                }
+
+                handlers += cnt;
+            }
+
+            // This is a last resort fail safe to ensure we don't go around
+            // forever. We cut it off the number of passes at 20.  This is
+            // roughly twice the number for the longest test (currently,
+            // multiTransactionTimeout).
+            if (passes > max_passes) {
+                ADD_FAILURE() << "processALL failed, too many passes: "
+                    << passes <<  ", total handlers executed: " << handlers;
+            }
+        }
+    }
+
 };
 
 /// @brief Tests the D2UpdateMgr construction.
@@ -168,12 +263,12 @@ TEST(D2UpdateMgr, construction) {
     D2CfgMgrPtr cfg_mgr;
     D2UpdateMgrPtr update_mgr;
 
-    // Verify that constrctor fails if given an invalid queue manager.
+    // Verify that constructor fails if given an invalid queue manager.
     ASSERT_NO_THROW(cfg_mgr.reset(new D2CfgMgr()));
     EXPECT_THROW(D2UpdateMgr(queue_mgr, cfg_mgr, io_service),
                  D2UpdateMgrError);
 
-    // Verify that constrctor fails if given an invalid config manager.
+    // Verify that constructor fails if given an invalid config manager.
     ASSERT_NO_THROW(queue_mgr.reset(new D2QueueMgr(io_service)));
     ASSERT_NO_THROW(cfg_mgr.reset());
     EXPECT_THROW(D2UpdateMgr(queue_mgr, cfg_mgr, io_service),
@@ -212,10 +307,10 @@ TEST(D2UpdateMgr, construction) {
 /// This test verifies that:
 /// 1. A transaction can be added to the list.
 /// 2. Finding a transaction in the list by key works correctly.
-/// 3. Looking for a non-existant transaction works properly.
+/// 3. Looking for a non-existent transaction works properly.
 /// 4. Attempting to add a transaction for a DHCID already in the list fails.
 /// 5. Removing a transaction by key works properly.
-/// 6. Attempting to remove an non-existant transaction does no harm.
+/// 6. Attempting to remove an non-existent transaction does no harm.
 TEST_F(D2UpdateMgrTest, transactionList) {
     // Grab a canned request for test purposes.
     NameChangeRequestPtr& ncr = canned_ncrs_[0];
@@ -248,7 +343,7 @@ TEST_F(D2UpdateMgrTest, transactionList) {
     EXPECT_NO_THROW(update_mgr_->removeTransaction(ncr->getDhcid()));
     EXPECT_EQ(0, update_mgr_->getTransactionCount());
 
-    // Verify the we can try to remove a non-existant transaction without harm.
+    // Verify the we can try to remove a non-existent transaction without harm.
     EXPECT_NO_THROW(update_mgr_->removeTransaction(ncr->getDhcid()));
 }
 
@@ -266,12 +361,32 @@ TEST_F(D2UpdateMgrTest, checkFinishedTransaction) {
     for (int i = 0; i < canned_count_; i++) {
         EXPECT_NO_THROW(update_mgr_->makeTransaction(canned_ncrs_[i]));
     }
-    // Verfiy we have that the transaçtion count is correct.
+    // Verify we have that the transaction count is correct.
+    EXPECT_EQ(canned_count_, update_mgr_->getTransactionCount());
+
+    // Verify that all four transactions have been started.
+    TransactionList::iterator pos;
+    EXPECT_NO_THROW(pos = update_mgr_->transactionListBegin());
+    while (pos != update_mgr_->transactionListEnd()) {
+        NameChangeTransactionPtr trans = (*pos).second;
+        ASSERT_EQ(dhcp_ddns::ST_PENDING, trans->getNcrStatus());
+        ASSERT_TRUE(trans->isModelRunning());
+        ++pos;
+    }
+
+    // Verify that invoking checkFinishedTransactions does not throw.
+    EXPECT_NO_THROW(update_mgr_->checkFinishedTransactions());
+
+    // Since nothing is running IOService, the all four transactions should
+    // still be in the list.
     EXPECT_EQ(canned_count_, update_mgr_->getTransactionCount());
 
-    // Set two of the transactions to finished states.
-    (canned_ncrs_[1])->setStatus(dhcp_ddns::ST_COMPLETED);
-    (canned_ncrs_[3])->setStatus(dhcp_ddns::ST_FAILED);
+    // Now "complete" two of the four.
+    // Simulate a successful completion.
+    completeTransaction(1, dhcp_ddns::ST_COMPLETED);
+
+    // Simulate a failed completion.
+    completeTransaction(3, dhcp_ddns::ST_FAILED);
 
     // Verify that invoking checkFinishedTransactions does not throw.
     EXPECT_NO_THROW(update_mgr_->checkFinishedTransactions());
@@ -323,9 +438,9 @@ TEST_F(D2UpdateMgrTest, pickNextJob) {
     EXPECT_EQ(1, update_mgr_->getQueueCount());
 
     // Verify that invoking pickNextJob:
-    // 1. does not throw
-    // 2. does not make a new transaction
-    // 3. does not dequeu the entry
+    // 1. Does not throw
+    // 2. Does not make a new transaction
+    // 3. Does not dequeue the entry
     EXPECT_NO_THROW(update_mgr_->pickNextJob());
     EXPECT_EQ(canned_count_, update_mgr_->getTransactionCount());
     EXPECT_EQ(1, update_mgr_->getQueueCount());
@@ -345,9 +460,9 @@ TEST_F(D2UpdateMgrTest, pickNextJob) {
     ASSERT_NO_THROW(queue_mgr_->enqueue(bogus_ncr));
 
     // Verify that invoking pickNextJob:
-    // 1. does not throw
-    // 2. does not make a new transaction
-    // 3. does dequeue the entry
+    // 1. Does not throw
+    // 2. Does not make a new transaction
+    // 3. Does dequeue the entry
     EXPECT_NO_THROW(update_mgr_->pickNextJob());
     EXPECT_EQ(0, update_mgr_->getTransactionCount());
     EXPECT_EQ(0, update_mgr_->getQueueCount());
@@ -360,17 +475,17 @@ TEST_F(D2UpdateMgrTest, pickNextJob) {
 
     // Verify that invoking pickNextJob:
     // 1. does not throw
-    // 2. does not make a new transaction
-    // 3. does dequeue the entry
+    // 2. Does not make a new transaction
+    // 3. Does dequeue the entry
     EXPECT_NO_THROW(update_mgr_->pickNextJob());
     EXPECT_EQ(0, update_mgr_->getTransactionCount());
     EXPECT_EQ(0, update_mgr_->getQueueCount());
 }
 
 /// @brief Tests D2UpdateManager's sweep method.
-/// Since sweep is primarly a wrapper around chechFinishedTransactions and
+/// Since sweep is primarily a wrapper around checkFinishedTransactions and
 /// pickNextJob, along with checks on maximum transaction limits, it mostly
-/// verifies that these three pieces work togther to move process jobs.
+/// verifies that these three pieces work together to move process jobs.
 /// Most of what is tested here is tested above.
 TEST_F(D2UpdateMgrTest, sweep) {
     // Ensure we have at least 4 canned requests with which to work.
@@ -399,7 +514,7 @@ TEST_F(D2UpdateMgrTest, sweep) {
     // Verify max transactions can't be less than current transaction count.
     EXPECT_THROW(update_mgr_->setMaxTransactions(1), D2UpdateMgrError);
 
-    // Queue up a request for a DCHID which has a transaction in progress.
+    // Queue up a request for a DHCID which has a transaction in progress.
     dhcp_ddns::NameChangeRequestPtr
         subsequent_ncr(new dhcp_ddns::NameChangeRequest(*(canned_ncrs_[2])));
     EXPECT_NO_THROW(queue_mgr_->enqueue(subsequent_ncr));
@@ -412,7 +527,7 @@ TEST_F(D2UpdateMgrTest, sweep) {
     EXPECT_EQ(1, update_mgr_->getQueueCount());
 
     // Mark the transaction complete.
-    (canned_ncrs_[2])->setStatus(dhcp_ddns::ST_COMPLETED);
+    completeTransaction(2, dhcp_ddns::ST_COMPLETED);
 
     // Verify that invoking sweep, cleans up the completed transaction,
     // dequeues the queued job and adds its transaction to the list.
@@ -428,7 +543,7 @@ TEST_F(D2UpdateMgrTest, sweep) {
     EXPECT_EQ(1, update_mgr_->getQueueCount());
 
     // Verify that sweep does not dequeue the new request as we are at
-    // transaction count.
+    // maximum transaction count.
     EXPECT_NO_THROW(update_mgr_->sweep());
     EXPECT_EQ(canned_count_, update_mgr_->getTransactionCount());
     EXPECT_EQ(1, update_mgr_->getQueueCount());
@@ -447,4 +562,211 @@ TEST_F(D2UpdateMgrTest, sweep) {
     EXPECT_EQ(0, update_mgr_->getTransactionCount());
 }
 
+/// @brief Tests integration of NameAddTransaction
+/// This test verifies that update manager can create and manage a
+/// NameAddTransaction from start to finish.  It utilizes a fake server
+/// which responds to all requests sent with NOERROR, simulating a
+/// successful addition.  The transaction processes both forward and
+/// reverse changes.
+TEST_F(D2UpdateMgrTest, addTransaction) {
+    // Put each transaction on the queue.
+    canned_ncrs_[0]->setChangeType(dhcp_ddns::CHG_ADD);
+    canned_ncrs_[0]->setReverseChange(true);
+    ASSERT_NO_THROW(queue_mgr_->enqueue(canned_ncrs_[0]));
+
+    // Call sweep once, this should:
+    // 1. Dequeue the request
+    // 2. Create the transaction
+    // 3. Start the transaction
+    ASSERT_NO_THROW(update_mgr_->sweep());
+
+    // Get a copy of the transaction.
+    TransactionList::iterator pos = update_mgr_->transactionListBegin();
+    ASSERT_TRUE (pos != update_mgr_->transactionListEnd());
+    NameChangeTransactionPtr trans = (*pos).second;
+    ASSERT_TRUE(trans);
+
+    // At this point the transaction should have constructed
+    // and sent the DNS request.
+    ASSERT_TRUE(trans->getCurrentServer());
+    ASSERT_TRUE(trans->isModelRunning());
+    ASSERT_EQ(1, trans->getUpdateAttempts());
+    ASSERT_EQ(StateModel::NOP_EVT, trans->getNextEvent());
+
+    // Create a server based on the transaction's current server, and
+    // start it listening.
+    FauxServer server(*io_service_, *(trans->getCurrentServer()));
+    server.receive(FauxServer::USE_RCODE, dns::Rcode::NOERROR());
+
+    // Run sweep and IO until everything is done.
+    processAll();
+
+    // Verify that model succeeded.
+    EXPECT_FALSE(trans->didModelFail());
+
+    // Both completion flags should be true.
+    EXPECT_TRUE(trans->getForwardChangeCompleted());
+    EXPECT_TRUE(trans->getReverseChangeCompleted());
+
+    // Verify that we went through success state.
+    EXPECT_EQ(NameChangeTransaction::PROCESS_TRANS_OK_ST,
+              trans->getPrevState());
+    EXPECT_EQ(NameChangeTransaction::UPDATE_OK_EVT,
+              trans->getLastEvent());
+}
+
+/// @brief Tests integration of NameRemoveTransaction
+/// This test verifies that update manager can create and manage a
+/// NameRemoveTransaction from start to finish.  It utilizes a fake server
+/// which responds to all requests sent with NOERROR, simulating a
+/// successful addition.  The transaction processes both forward and
+/// reverse changes.
+TEST_F(D2UpdateMgrTest, removeTransaction) {
+    // Put each transaction on the queue.
+    canned_ncrs_[0]->setChangeType(dhcp_ddns::CHG_REMOVE);
+    canned_ncrs_[0]->setReverseChange(true);
+    ASSERT_NO_THROW(queue_mgr_->enqueue(canned_ncrs_[0]));
+
+    // Call sweep once, this should:
+    // 1. Dequeue the request
+    // 2. Create the transaction
+    // 3. Start the transaction
+    ASSERT_NO_THROW(update_mgr_->sweep());
+
+    // Get a copy of the transaction.
+    TransactionList::iterator pos = update_mgr_->transactionListBegin();
+    ASSERT_TRUE (pos != update_mgr_->transactionListEnd());
+    NameChangeTransactionPtr trans = (*pos).second;
+    ASSERT_TRUE(trans);
+
+    // At this point the transaction should have constructed
+    // and sent the DNS request.
+    ASSERT_TRUE(trans->getCurrentServer());
+    ASSERT_TRUE(trans->isModelRunning());
+    ASSERT_EQ(1, trans->getUpdateAttempts());
+    ASSERT_EQ(StateModel::NOP_EVT, trans->getNextEvent());
+
+    // Create a server based on the transaction's current server,
+    // and start it listening.
+    FauxServer server(*io_service_, *(trans->getCurrentServer()));
+    server.receive(FauxServer::USE_RCODE, dns::Rcode::NOERROR());
+
+    // Run sweep and IO until everything is done.
+    processAll();
+
+    // Verify that model succeeded.
+    EXPECT_FALSE(trans->didModelFail());
+
+    // Both completion flags should be true.
+    EXPECT_TRUE(trans->getForwardChangeCompleted());
+    EXPECT_TRUE(trans->getReverseChangeCompleted());
+
+    // Verify that we went through success state.
+    EXPECT_EQ(NameChangeTransaction::PROCESS_TRANS_OK_ST,
+              trans->getPrevState());
+    EXPECT_EQ(NameChangeTransaction::UPDATE_OK_EVT,
+              trans->getLastEvent());
+}
+
+
+/// @brief Tests handling of a transaction which fails.
+/// This test verifies that update manager correctly concludes a transaction
+/// which fails to complete successfully.  The failure simulated is repeated
+/// corrupt responses from the server, which causes an exhaustion of the
+/// available servers.
+TEST_F(D2UpdateMgrTest, errorTransaction) {
+    // Put each transaction on the queue.
+    ASSERT_NO_THROW(queue_mgr_->enqueue(canned_ncrs_[0]));
+
+    // Call sweep once, this should:
+    // 1. Dequeue the request
+    // 2. Create the transaction
+    // 3. Start the transaction
+    ASSERT_NO_THROW(update_mgr_->sweep());
+
+    // Get a copy of the transaction.
+    TransactionList::iterator pos = update_mgr_->transactionListBegin();
+    ASSERT_TRUE (pos != update_mgr_->transactionListEnd());
+    NameChangeTransactionPtr trans = (*pos).second;
+    ASSERT_TRUE(trans);
+
+    ASSERT_TRUE(trans->isModelRunning());
+    ASSERT_EQ(1, trans->getUpdateAttempts());
+    ASSERT_EQ(StateModel::NOP_EVT, trans->getNextEvent());
+    ASSERT_TRUE(trans->getCurrentServer());
+
+    // Create a server and start it listening.
+    FauxServer server(*io_service_, *(trans->getCurrentServer()));
+    server.receive(FauxServer::CORRUPT_RESP);
+
+    // Run sweep and IO until everything is done.
+    processAll();
+
+    // Verify that model succeeded.
+    EXPECT_FALSE(trans->didModelFail());
+
+    // Both completion flags should be false.
+    EXPECT_FALSE(trans->getForwardChangeCompleted());
+    EXPECT_FALSE(trans->getReverseChangeCompleted());
+
+    // Verify that we went through success state.
+    EXPECT_EQ(NameChangeTransaction::PROCESS_TRANS_FAILED_ST,
+              trans->getPrevState());
+    EXPECT_EQ(NameChangeTransaction::NO_MORE_SERVERS_EVT,
+              trans->getLastEvent());
+
+
+}
+
+/// @brief Tests processing of multiple transactions.
+/// This test verifies that update manager can create and manage a multiple
+/// transactions, concurrently.  It uses a fake server that responds to all
+/// requests sent with NOERROR, simulating successful DNS updates. The
+/// transactions are a mix of both adds and removes.
+TEST_F(D2UpdateMgrTest, multiTransaction) {
+    // Queue up all the requests.
+    int test_count = canned_count_;
+    for (int i = test_count; i > 0; i--) {
+        canned_ncrs_[i-1]->setReverseChange(true);
+        ASSERT_NO_THROW(queue_mgr_->enqueue(canned_ncrs_[i-1]));
+    }
+
+    // Create a server and start it listening. Note this relies on the fact
+    // that all of configured servers have the same address.
+    // and start it listening.
+    asiolink::IOAddress server_ip("127.0.0.1");
+    FauxServer server(*io_service_, server_ip, 5301);
+    server.receive(FauxServer::USE_RCODE, dns::Rcode::NOERROR());
+
+    // Run sweep and IO until everything is done.
+    processAll();
+
+    for (int i = 0; i < test_count; i++) {
+        EXPECT_EQ(dhcp_ddns::ST_COMPLETED, canned_ncrs_[i]->getStatus());
+    }
+}
+
+/// @brief Tests processing of multiple transactions.
+/// This test verifies that update manager can create and manage a multiple
+/// transactions, concurrently.  It uses a fake server that responds to all
+/// requests sent with NOERROR, simulating successful DNS updates. The
+/// transactions are a mix of both adds and removes.
+TEST_F(D2UpdateMgrTest, multiTransactionTimeout) {
+    // Queue up all the requests.
+    int test_count = canned_count_;
+    for (int i = test_count; i > 0; i--) {
+        canned_ncrs_[i-1]->setReverseChange(true);
+        ASSERT_NO_THROW(queue_mgr_->enqueue(canned_ncrs_[i-1]));
+    }
+
+    // No server is running, so everything will time out.
+
+    // Run sweep and IO until everything is done.
+    processAll();
+
+    for (int i = 0; i < test_count; i++) {
+        EXPECT_EQ(dhcp_ddns::ST_FAILED, canned_ncrs_[i]->getStatus());
+    }
+}
+
 }

+ 26 - 9
src/bin/d2/tests/dns_client_unittests.cc

@@ -25,6 +25,7 @@
 #include <boost/bind.hpp>
 #include <boost/scoped_ptr.hpp>
 #include <gtest/gtest.h>
+#include <nc_test_utils.h>
 
 using namespace std;
 using namespace isc;
@@ -69,6 +70,8 @@ public:
     bool corrupt_response_;
     bool expect_response_;
     asiolink::IntervalTimer test_timer_;
+    int received_;
+    int expected_;
 
     // @brief Constructor.
     //
@@ -83,9 +86,10 @@ public:
           status_(DNSClient::SUCCESS),
           corrupt_response_(false),
           expect_response_(true),
-          test_timer_(service_) {
+          test_timer_(service_),
+          received_(0), expected_(0) {
         asiodns::logger.setSeverity(isc::log::INFO);
-        response_.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
+        response_.reset();
         dns_client_.reset(new DNSClient(response_, this));
 
         // Set the test timeout to break any running tasks if they hang.
@@ -108,7 +112,10 @@ public:
     // @param status A status code returned by DNSClient.
     virtual void operator()(DNSClient::Status status) {
         status_ = status;
-        service_.stop();
+        if (!expected_ || (expected_ == ++received_))
+        {
+            service_.stop();
+        }
 
         if (expect_response_) {
             if (!corrupt_response_) {
@@ -121,7 +128,7 @@ public:
                           response_->getRRCount(D2UpdateMessage::SECTION_ZONE));
                 D2ZonePtr zone = response_->getZone();
                 ASSERT_TRUE(zone);
-                EXPECT_EQ("example.com.", zone->getName().toText());
+                EXPECT_EQ("response.example.com.", zone->getName().toText());
                 EXPECT_EQ(RRClass::IN().getCode(), zone->getClass().getCode());
 
             } else {
@@ -187,9 +194,6 @@ public:
     // It also verifies that the constructor will not throw if the supplied
     // callback object is NULL.
     void runConstructorTest() {
-        D2UpdateMessagePtr null_response;
-        EXPECT_THROW(DNSClient(null_response, this, DNSClient::UDP),
-                     isc::BadValue);
         EXPECT_NO_THROW(DNSClient(response_, NULL, DNSClient::UDP));
 
         // The TCP Transport is not supported right now. So, we return exception
@@ -284,7 +288,7 @@ public:
         // Create a request DNS Update message.
         D2UpdateMessage message(D2UpdateMessage::OUTBOUND);
         ASSERT_NO_THROW(message.setRcode(Rcode(Rcode::NOERROR_CODE)));
-        ASSERT_NO_THROW(message.setZone(Name("example.com"), RRClass::IN()));
+        ASSERT_NO_THROW(message.setZone(Name("response.example.com"), RRClass::IN()));
 
         // In order to perform the full test, when the client sends the request
         // and receives a response from the server, we have to emulate the
@@ -322,11 +326,13 @@ public:
         // The socket is now ready to receive the data. Let's post some request
         // message then.
         const int timeout = 5;
+        expected_++;
         dns_client_->doUpdate(service_, IOAddress(TEST_ADDRESS), TEST_PORT,
                              message, timeout);
 
         // It is possible to request that two packets are sent concurrently.
         if (two_sends) {
+            expected_++;
             dns_client_->doUpdate(service_, IOAddress(TEST_ADDRESS), TEST_PORT,
                                   message, timeout);
 
@@ -338,6 +344,10 @@ public:
 
         udp_socket.close();
 
+        // Since the callback, operator(), calls stop() on the io_service,
+        // we must reset it in order for subsequent calls to run() or
+        // run_one() to work.
+        service_.get_io_service().reset();
     }
 };
 
@@ -393,6 +403,7 @@ TEST_F(DNSClientTest, sendReceiveCurrupted) {
 TEST_F(DNSClientTest, sendReceiveTwice) {
     runSendReceiveTest(false, false);
     runSendReceiveTest(false, false);
+    EXPECT_EQ(2, received_);
 }
 
 // Verify that it is possible to use the DNSClient instance to perform the
@@ -401,7 +412,13 @@ TEST_F(DNSClientTest, sendReceiveTwice) {
 // 2. send
 // 3. receive
 // 4. receive
-TEST_F(DNSClientTest, concurrentSendReceive) {
+// @todo  THIS Test does not function. The method runSendReceive only
+// schedules one "server" receive.  In other words only one request is
+// listened for and then received. Once it is received, the operator()
+// method calls stop() on the io_service, which causes the second receive
+// to be cancelled.  It is also unclear, what the asio layer does with a
+// second receive on the same socket.
+TEST_F(DNSClientTest, DISABLED_concurrentSendReceive) {
     runSendReceiveTest(false, true);
 }
 

+ 62 - 18
src/bin/d2/tests/nc_test_utils.cc

@@ -37,7 +37,8 @@ const bool NO_RDATA = false;
 FauxServer::FauxServer(asiolink::IOService& io_service,
                        asiolink::IOAddress& address, size_t port)
     :io_service_(io_service), address_(address), port_(port),
-     server_socket_() {
+     server_socket_(), receive_pending_(false), perpetual_receive_(true) {
+
     server_socket_.reset(new asio::ip::udp::socket(io_service_.get_io_service(),
                                                    asio::ip::udp::v4()));
     server_socket_->set_option(asio::socket_base::reuse_address(true));
@@ -47,7 +48,8 @@ FauxServer::FauxServer(asiolink::IOService& io_service,
 FauxServer::FauxServer(asiolink::IOService& io_service,
                        DnsServerInfo& server)
     :io_service_(io_service), address_(server.getIpAddress()),
-     port_(server.getPort()), server_socket_() {
+     port_(server.getPort()), server_socket_(), receive_pending_(false),
+     perpetual_receive_(true) {
     server_socket_.reset(new asio::ip::udp::socket(io_service_.get_io_service(),
                                                    asio::ip::udp::v4()));
     server_socket_->set_option(asio::socket_base::reuse_address(true));
@@ -61,6 +63,11 @@ FauxServer::~FauxServer() {
 void
 FauxServer::receive (const ResponseMode& response_mode,
                      const dns::Rcode& response_rcode) {
+    if (receive_pending_) {
+        return;
+    }
+
+    receive_pending_ = true;
     server_socket_->async_receive_from(asio::buffer(receive_buffer_,
                                                     sizeof(receive_buffer_)),
                                        remote_,
@@ -79,6 +86,7 @@ FauxServer::requestHandler(const asio::error_code& error,
     // We expect the client to send good requests.
     if (error.value() != 0 || bytes_recvd < 1) {
         ADD_FAILURE() << "FauxServer receive failed" << error.message();
+        receive_pending_ = false;
         return;
     }
 
@@ -95,10 +103,11 @@ FauxServer::requestHandler(const asio::error_code& error,
         // If the request cannot be parsed, then fail the test.
         // We expect the client to send good requests.
         ADD_FAILURE() << "FauxServer request is corrupt:" << ex.what();
+        receive_pending_ = false;
         return;
     }
 
-    // The request parsed ok, so let's build a response.
+    // The request parsed OK, so let's build a response.
     // We must use the QID we received in the response or IOFetch will
     // toss the response out, resulting in eventual timeout.
     // We fill in the zone with data we know is from the "server".
@@ -125,7 +134,7 @@ FauxServer::requestHandler(const asio::error_code& error,
         response_buf.writeUint16At(0xFFFF, 2);
     }
 
-    // Ship the reponse via synchronous send.
+    // Ship the response via synchronous send.
     try {
         int cnt = server_socket_->send_to(asio::
                                           buffer(response_buf.getData(),
@@ -139,35 +148,56 @@ FauxServer::requestHandler(const asio::error_code& error,
     } catch (const std::exception& ex) {
         ADD_FAILURE() << "FauxServer send failed: " << ex.what();
     }
+
+    receive_pending_ = false;
+    if (perpetual_receive_) {
+        // Schedule the next receive
+        receive (response_mode, response_rcode);
+    }
 }
 
-//********************** TransactionTest class ***********************
 
-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;
+//********************** TimedIO class ***********************
 
-TransactionTest::TransactionTest()
-    : io_service_(new isc::asiolink::IOService()), ncr_(),
-    timer_(*io_service_), run_time_(0) {
+TimedIO::TimedIO()
+    : io_service_(new isc::asiolink::IOService()),
+     timer_(*io_service_), run_time_(0) {
 }
 
-TransactionTest::~TransactionTest() {
+TimedIO::~TimedIO() {
 }
 
-void
-TransactionTest::runTimedIO(int run_time) {
+int
+TimedIO::runTimedIO(int run_time) {
     run_time_ = run_time;
-    timer_.setup(boost::bind(&TransactionTest::timesUp, this), run_time_);
-    io_service_->run();
+    int cnt = io_service_->get_io_service().poll();
+    if (cnt == 0) {
+        timer_.setup(boost::bind(&TransactionTest::timesUp, this), run_time_);
+        cnt = io_service_->get_io_service().run_one();
+        timer_.cancel();
+    }
+
+    return (cnt);
 }
 
 void
-TransactionTest::timesUp() {
+TimedIO::timesUp() {
     io_service_->stop();
     FAIL() << "Test Time: " << run_time_ << " expired";
 }
 
+//********************** TransactionTest class ***********************
+
+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() {
+}
+
 void
 TransactionTest::setupForIPv4Transaction(dhcp_ddns::NameChangeType chg_type,
                                          int change_mask) {
@@ -299,7 +329,7 @@ checkRR(dns::RRsetPtr rrset, const std::string& exp_name,
     EXPECT_EQ(exp_class.getCode(), rrset->getClass().getCode());
     EXPECT_EQ(exp_type.getCode(), rrset->getType().getCode());
     EXPECT_EQ(exp_ttl, rrset->getTTL().getValue());
-    if ((!has_rdata) || 
+    if ((!has_rdata) ||
        (exp_type == dns::RRType::ANY() || exp_class == dns::RRClass::ANY())) {
         // ANY types do not have RData
         ASSERT_EQ(0, rrset->getRdataCount());
@@ -653,6 +683,20 @@ void checkRemoveRevPtrsRequest(NameChangeTransaction& tran) {
     ASSERT_NO_THROW(request->toWire(renderer));
 }
 
+std::string toHexText(const uint8_t* data, size_t len) {
+    std::ostringstream stream;
+    stream << "Data length is: " << len << std::endl;
+    for (int i = 0; i < len; ++i) {
+        if (i > 0 && ((i % 16) == 0)) {
+            stream << std::endl;
+        }
+
+        stream << setfill('0') << setw(2) << setbase(16)
+               << static_cast<unsigned int>(data[i]) << " ";
+    }
+
+    return (stream.str());
+}
 
 }; // namespace isc::d2
 }; // namespace isc

+ 79 - 21
src/bin/d2/tests/nc_test_utils.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -44,12 +44,25 @@ public:
         CORRUPT_RESP  // Generate a corrupt response
     };
 
+    // Reference to IOService to use for IO processing.
     asiolink::IOService& io_service_;
+    // IP address at which to listen for requests.
     const asiolink::IOAddress& address_;
+    // Port on which to listen for requests.
     size_t port_;
+    // Socket on which listening is done.
     SocketPtr server_socket_;
+    // Stores the end point of requesting client.
     asio::ip::udp::endpoint remote_;
+    // Buffer in which received packets are stuffed.
     uint8_t receive_buffer_[TEST_MSG_MAX];
+    // Flag which indicates if a receive has been initiated but
+    // not yet completed.
+    bool receive_pending_;
+    // Indicates if server is in perpetual receive mode. If true once
+    // a receive has been completed, a new one will be automatically
+    // initiated.
+    bool perpetual_receive_;
 
     /// @brief Constructor
     ///
@@ -62,7 +75,7 @@ public:
     /// @brief Constructor
     ///
     /// @param io_service IOService to be used for socket IO.
-    /// @param server DnServerInfo of server the DNS server. This supplies the
+    /// @param server DnsServerInfo of server the DNS server. This supplies the
     /// server's ip address and port.
     FauxServer(asiolink::IOService& io_service, DnsServerInfo& server);
 
@@ -93,17 +106,66 @@ public:
                         std::size_t bytes_recvd,
                         const ResponseMode& response_mode,
                         const dns::Rcode& response_rcode);
+
+    /// @brief Returns true if a receive has been started but not completed.
+    bool isReceivePending() {
+        return receive_pending_;
+    }
 };
 
-/// @brief Base class Test fixture for testing transactions.
-class TransactionTest : public ::testing::Test {
+/// @brief Provides a means to process IOService IO for a finite amount of time.
+///
+/// This class instantiates an IOService provides a single method, runTimedIO
+/// which will run the IOService for no more than a finite amount of time,
+/// at least one event is executed or the IOService is stopped.
+/// It provides an virtual handler for timer expiration event.  It is
+/// intended to be used as a base class for test fixtures that need to process
+/// IO by providing them a consistent way to do so while retaining a safety
+/// valve so tests do not hang.
+class TimedIO  {
 public:
     IOServicePtr io_service_;
+    asiolink::IntervalTimer timer_;
+    int run_time_;
+
+    // Constructor
+    TimedIO();
+
+    // Destructor
+    virtual ~TimedIO();
+
+    /// @brief IO Timer expiration handler
+    ///
+    /// Stops the IOService and fails the current test.
+    virtual void timesUp();
+
+    /// @brief Processes IO till time expires or at least one handler executes.
+    ///
+    /// This method first polls IOService to run any ready handlers.  If no
+    /// handlers are ready, it starts the internal time to run for the given
+    /// amount of time and invokes service's run_one method.  This method
+    /// blocks until at least one handler executes or the IO Service is stopped.
+    /// Upon completion of this method the timer is cancelled.  Should the
+    /// timer expires prior to run_one returning, the timesUp handler will be
+    /// invoked which stops the IO service and fails the test.
+    ///
+    /// Note that this method closely mimics the runIO method in D2Process.
+    ///
+    /// @param run_time maximum length of time to run in milliseconds before
+    /// timing out.
+    ///
+    /// @return Returns the number of handlers executed or zero. A return of
+    /// zero indicates that the IOService has been stopped.
+    int runTimedIO(int run_time);
+
+};
+
+/// @brief Base class Test fixture for testing transactions.
+class TransactionTest : public TimedIO,  public ::testing::Test {
+public:
     dhcp_ddns::NameChangeRequestPtr ncr_;
     DdnsDomainPtr forward_domain_;
     DdnsDomainPtr reverse_domain_;
-    asiolink::IntervalTimer timer_;
-    int run_time_;
 
     /// #brief constants used to specify change directions for a transaction.
     static const unsigned int FORWARD_CHG;      // Only forward change.
@@ -113,20 +175,6 @@ public:
     TransactionTest();
     virtual ~TransactionTest();
 
-    /// @brief Run the IO service for no more than a given amount of time.
-    ///
-    /// Uses an IntervalTimer to interrupt the invocation of IOService run(),
-    /// after the given number of milliseconds elapse.  The timer executes
-    /// the timesUp() method if it expires.
-    ///
-    /// @param run_time amount of time in milliseconds to allow run to execute.
-    void runTimedIO(int run_time);
-
-    /// @brief IO Timer expiration handler
-    ///
-    /// Stops the IOSerivce and fails the current test.
-    virtual void timesUp();
-
     /// @brief Creates a transaction which requests an IPv4 DNS update.
     ///
     /// The transaction is constructed around a predefined (i.e. "canned")
@@ -190,7 +238,7 @@ extern void checkZone(const D2UpdateMessagePtr& request,
 /// @param has_rdata if true, RRset's rdata will be checked based on it's
 /// RRType.  Set this to false if the RRset's type supports Rdata but it does
 /// not contain it.  For instance, prerequisites of type NONE have no Rdata
-/// where udpates of type NONE may.
+/// where updates of type NONE may.
 extern void checkRR(dns::RRsetPtr rrset, const std::string& exp_name,
                     const dns::RRClass& exp_class, const dns::RRType& exp_type,
                     unsigned int exp_ttl, dhcp_ddns::NameChangeRequestPtr ncr,
@@ -308,6 +356,16 @@ extern void addDomainServer(DdnsDomainPtr& domain, const std::string& name,
                             const std::string& ip = TEST_DNS_SERVER_IP,
                             const size_t port = TEST_DNS_SERVER_PORT);
 
+/// @brief Creates a hex text dump of the given data buffer.
+///
+/// This method is not used for testing but is handy for debugging.  It creates
+/// a pleasantly formatted string of 2-digits per byte separated by spaces with
+/// 16 bytes per line.
+///
+/// @param data pointer to the data to dump
+/// @param len size (in bytes) of data
+extern std::string toHexText(const uint8_t* data, size_t len);
+
 }; // namespace isc::d2
 }; // namespace isc
 

+ 29 - 9
src/bin/d2/tests/nc_trans_unittests.cc

@@ -578,7 +578,16 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) {
     // they are correct after each selection.
     DnsServerInfoPtr prev_server = name_change->getCurrentServer();
     DNSClientPtr prev_client = name_change->getDNSClient();
-    D2UpdateMessagePtr prev_response = name_change->getDnsUpdateResponse();
+
+    // Verify response pointer is empty.
+    EXPECT_FALSE(name_change->getDnsUpdateResponse());
+
+    // Create dummy response so we can verify it is cleared at each
+    // new server select.
+    D2UpdateMessagePtr dummyResp;
+    dummyResp.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
+    ASSERT_NO_THROW(name_change->setDnsUpdateResponse(dummyResp));
+    ASSERT_TRUE(name_change->getDnsUpdateResponse());
 
     // Iteratively select through the list of servers.
     int passes = 0;
@@ -591,17 +600,22 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) {
         // Verify that the new values are not empty.
         EXPECT_TRUE(server);
         EXPECT_TRUE(client);
-        EXPECT_TRUE(response);
+
+        // Verify response pointer is now empty.
+        EXPECT_FALSE(name_change->getDnsUpdateResponse());
 
         // Verify that the new values are indeed new.
         EXPECT_NE(server, prev_server);
         EXPECT_NE(client, prev_client);
-        EXPECT_NE(response, prev_response);
 
         // Remember the selected values for the next pass.
         prev_server = server;
         prev_client = client;
-        prev_response = response;
+
+        // Create new dummy response.
+        dummyResp.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
+        ASSERT_NO_THROW(name_change->setDnsUpdateResponse(dummyResp));
+        ASSERT_TRUE(name_change->getDnsUpdateResponse());
 
         ++passes;
     }
@@ -626,12 +640,11 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) {
     ASSERT_NO_THROW(name_change->initServerSelection(domain));
 
     // The server selection process determines the current server,
-    // instantiates a new DNSClient, and a DNS response message buffer.
+    // instantiates a new DNSClient, and resets the DNS response message buffer.
     // We need to save the values before each selection, so we can verify
     // they are correct after each selection.
     prev_server = name_change->getCurrentServer();
     prev_client = name_change->getDNSClient();
-    prev_response = name_change->getDnsUpdateResponse();
 
     // Iteratively select through the list of servers.
     passes = 0;
@@ -644,17 +657,22 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) {
         // Verify that the new values are not empty.
         EXPECT_TRUE(server);
         EXPECT_TRUE(client);
-        EXPECT_TRUE(response);
+
+        // Verify response pointer is now empty.
+        EXPECT_FALSE(name_change->getDnsUpdateResponse());
 
         // Verify that the new values are indeed new.
         EXPECT_NE(server, prev_server);
         EXPECT_NE(client, prev_client);
-        EXPECT_NE(response, prev_response);
 
         // Remember the selected values for the next pass.
         prev_server = server;
         prev_client = client;
-        prev_response = response;
+
+        // Create new dummy response.
+        dummyResp.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND));
+        ASSERT_NO_THROW(name_change->setDnsUpdateResponse(dummyResp));
+        ASSERT_TRUE(name_change->getDnsUpdateResponse());
 
         ++passes;
     }
@@ -686,6 +704,7 @@ TEST_F(NameChangeTransactionTest, modelFailure) {
 TEST_F(NameChangeTransactionTest, successfulUpdateTest) {
     NameChangeStubPtr name_change;
     ASSERT_NO_THROW(name_change = makeCannedTransaction());
+    ASSERT_TRUE(name_change->selectFwdServer());
 
     EXPECT_TRUE(name_change->isModelNew());
     EXPECT_FALSE(name_change->getForwardChangeCompleted());
@@ -722,6 +741,7 @@ TEST_F(NameChangeTransactionTest, successfulUpdateTest) {
 TEST_F(NameChangeTransactionTest, failedUpdateTest) {
     NameChangeStubPtr name_change;
     ASSERT_NO_THROW(name_change = makeCannedTransaction());
+    ASSERT_TRUE(name_change->selectFwdServer());
 
     // Launch the transaction by calling startTransaction.  The state model
     // should run up until the "IO" operation is initiated in DOING_UPDATE_ST.