NameserverEntry does not forget RTT on update

The RTT values of previous addresses are kept and reused, if the same
addresses arrive.

git-svn-id: svn://bind10.isc.org/svn/bind10/branches/trac408@3751 e5f2f494-b856-4b98-b285-d166d9295462

src/lib/nsas/TODO

@@ -1,6 +1,3 @@
-NameserverEntry:
-* Reuse data on timeout/requery
-
 Global:
 * There are TODO notes in tests, some more tests should be added to stress
   and test it more.

src/lib/nsas/nameserver_entry.cc

@@ -196,8 +196,20 @@ class NameserverEntry::ResolverCallback : public ResolverInterface::Callback {
             // TODO Remove at merge with trunk
             i->first();
             while (! i->isLast()) {
+                // Try to find the original value and reuse its rtt
+                string address(i->getCurrent().toText());
+                int curr_rtt(-1);
+                BOOST_FOREACH(const AddressEntry& entry,
+                    entry_->previous_addresses_)
+                {
+                    if (entry.getAddress().toText() == address) {
+                        curr_rtt = entry.getRTT();
+                        break;
+                    }
+                }
                 entries.push_back(AddressEntry(IOAddress(
-                    i->getCurrent().toText()), ++ rtt_));
+                    i->getCurrent().toText()),
+                    curr_rtt == -1 ? ++ rtt_ : curr_rtt));
                 i->next();
             }
 
@@ -212,6 +224,8 @@ class NameserverEntry::ResolverCallback : public ResolverInterface::Callback {
                 // Everything is here
                 if (!entry_->expect_address_[ANY_OK]) {
                     entry_->setState(READY);
+                    // We can drop the original addresses, we took everything
+                    entry_->previous_addresses_.clear();
                 }
                 // We have some address
                 entry_->has_address_[ANY_OK] =
@@ -323,10 +337,10 @@ NameserverEntry::askIP(shared_ptr<ResolverInterface> resolver,
         // We will request the addresses
 
         // Set internal state first
-        // TODO: We might want to save the addresses somewhere so we do not
-        // lose RTT. This might get tricky. Would the trick with map as in
-        // ZoneEntry work as well?
-        address_.clear();
+        // We store the old addresses so we can pick their RTT when
+        // we get the same addresses again (most probably)
+        previous_addresses_.clear();
+        address_.swap(previous_addresses_);
         setState(IN_PROGRESS);
         has_address_[V4_ONLY] = has_address_[V6_ONLY] = has_address_[ANY_OK] =
             false;

src/lib/nsas/nameserver_entry.h

@@ -223,7 +223,8 @@ private:
     mutable boost::mutex    mutex_;     ///< Mutex protecting this object
     std::string     name_;              ///< Canonical name of the nameserver
     isc::dns::RRClass classCode_;       ///< Class of the nameserver
-    std::vector<AddressEntry> address_; ///< Set of V4/V6 addresses
+    /// The current addresses and addresses from previous request to keep RTT
+    std::vector<AddressEntry> address_, previous_addresses_;
     time_t          expiration_;        ///< Summary expiration time. 0 = unset
     // Do we have some addresses already? Do we expect some to come?
     // These are set after asking for IP, if NOT_ASKED, they are uninitialized

src/lib/nsas/tests/nameserver_entry_unittest.cc

@@ -411,4 +411,62 @@ TEST_F(NameserverEntryTest, ChangedExpired) {
     EXPECT_EQ("2001:db8::2", addresses[3].getAddress().toText());
 }
 
+/*
+ * When the data expire and is asked again, the original RTT is kept.
+ */
+TEST_F(NameserverEntryTest, KeepRTT) {
+    shared_ptr<NameserverEntry> entry(new NameserverEntry(EXAMPLE_CO_UK,
+        RRClass::IN()));
+    shared_ptr<Callback> callback(new Callback);
+    shared_ptr<TestResolver> resolver(new TestResolver);
+
+    // Ask the first time
+    entry->askIP(resolver, callback, V4_ONLY);
+    entry->askIP(resolver, callback, V6_ONLY);
+    EXPECT_TRUE(resolver->asksIPs(Name(EXAMPLE_CO_UK), 0, 1));
+    EXPECT_EQ(Fetchable::IN_PROGRESS, entry->getState());
+    resolver->answer(0, Name(EXAMPLE_CO_UK), RRType::A(),
+        rdata::in::A("192.0.2.1"), 0);
+    resolver->answer(1, Name(EXAMPLE_CO_UK), RRType::AAAA(),
+        rdata::in::AAAA("2001:db8::1"), 0);
+    EXPECT_EQ(2, callback->count);
+    NameserverEntry::AddressVector addresses;
+    // We must accept expired as well, as the TTL is 0 (and it is OK,
+    // because we just got the callback)
+    EXPECT_EQ(Fetchable::READY, entry->getAddresses(addresses, V4_ONLY, true));
+    ASSERT_EQ(1, addresses.size());
+    EXPECT_EQ("192.0.2.1", addresses[0].getAddress().toText());
+    EXPECT_EQ(Fetchable::READY, entry->getAddresses(addresses, V6_ONLY, true));
+    ASSERT_EQ(2, addresses.size());
+    EXPECT_EQ("2001:db8::1", addresses[1].getAddress().toText());
+    BOOST_FOREACH(const AddressEntry& address, addresses) {
+        entry->setAddressRTT(address.getAddress(), 123);
+    }
+
+    // Ask the second time. The callbacks should not fire right away and it
+    // should request the addresses again
+    entry->askIP(resolver, callback, V4_ONLY);
+    entry->askIP(resolver, callback, V6_ONLY);
+    EXPECT_EQ(2, callback->count);
+    EXPECT_TRUE(resolver->asksIPs(Name(EXAMPLE_CO_UK), 2, 3));
+    EXPECT_EQ(Fetchable::IN_PROGRESS, entry->getState());
+    resolver->answer(0, Name(EXAMPLE_CO_UK), RRType::A(),
+        rdata::in::A("192.0.2.1"));
+    resolver->answer(1, Name(EXAMPLE_CO_UK), RRType::AAAA(),
+        rdata::in::AAAA("2001:db8::1"));
+    // We should get the new addresses and they should not expire,
+    // so we should get them without accepting expired
+    addresses.clear();
+    EXPECT_EQ(Fetchable::READY, entry->getAddresses(addresses, V4_ONLY));
+    ASSERT_EQ(1, addresses.size());
+    EXPECT_EQ("192.0.2.1", addresses[0].getAddress().toText());
+    EXPECT_EQ(Fetchable::READY, entry->getAddresses(addresses, V6_ONLY));
+    ASSERT_EQ(2, addresses.size());
+    EXPECT_EQ("2001:db8::1", addresses[1].getAddress().toText());
+    // They should have the RTT we set to them
+    BOOST_FOREACH(const AddressEntry& address, addresses) {
+        EXPECT_EQ(123, address.getRTT());
+    }
+}
+
 }   // namespace