BIND 10 #493: Negative cache

[BIND 10 Development]

#493: Negative cache
-------------------------------------+-------------------------------------
                 Reporter:  shane    |                Owner:  ocean
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  R-Team-
                 Priority:  major    |  Sprint-20110316
                Component:           |           Resolution:
  resolver                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  5.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by ocean):

 Replying to [comment:4 stephen]:
 >
 > '''src/lib/cache/message_cache.{cc,h}'''[[BR]]
 > Following [http://bind10.isc.org/wiki/WeeklyMinutes20110301 recent
 discussions] about future code stubs, I suggest that the stubs for the
 !MessageCache methods dump()/load() and resize() are removed.
 >
 This will be processed in #639
 > !MessageCache constructor: Suggest that the first and last arguments
 (rrset_cache and negative_soa_cache) are passed by reference.
 >
 Done
 > !MessageCache constructor: Suggest that the type of the first and last
 arguments are set to  RRsetCachePtr (this is defined in rrset_cache.h).
 Done
 >
 >
 >
 >
 > '''src/lib/cache/message_entry.{cc,h}'''[[BR]]
 > RRsetRef constructor: the "cache" parameter should be passed by
 reference and the type can be defined as a RRsetCachePtr.
 Done
 >
 > Comment: possible future optimisation: is there a need to store the
 pointer to the cache in RRsetRef as a shared_ptr?  RRsetRef is only used
 in !MessageEntry and has no existence outside it (arguably, for this
 reason it would be better defined within the !MessageEntry class).  It
 points to either the rrset_cache or the negative_soa_cache and both these
 are are kept in existence for as long as the RRsetRef exists by the
 shared_ptrs held in the !MessageEntry class.
 Done. Use raw pointer instead of shared_ptr and move the declaration of
 RRsetRef as inner struct.
 >
 > !MessageEntry constructor.  Both the cache parameters can be passed by
 reference (and both types set as RRsetCachePtr).
 Done
 >
 > The headerflag_aa_ flag is set from the message when the entry is
 initialized (in initMessageEntry).  The AA bit is set in the response from
 a resolver if the resolver has just read the data from an authoritative
 server, but is clear in any response that comes from cache.  At the
 moment, if the bit is set when the entry is created it remains set as
 there does not appear to be any place where it is cleared. (Perhaps it
 should be cleared in genMessage() before the new message is returned?)
 >
 >
 Done. Clear the AA flag in genMessage(). The unit tests are updated.
 >
 > '''src/lib/cache/message_utility.{cc,h}'''[[BR]]
 > hasTheRecordInAuthoritySection(): I suggest that a comment be added
 noting that you just need to know about the presence of an SOA record but
 don't know what name it will be associated with, which is why
 Message::hasRRset is not used.
 >
 Done. Add a comment. This is because that the isc::dns::Message:hasRRset()
 is not const-qualified.
 > canMessageBeCached(): the criteria for caching the message is in the .cc
 file, although the .h file is usually the place for the detailed
 documentation.  Also it would be useful to mention why an NXDOMAIN
 response lacking an SOA cannot be cached.
 >
 Done. Add some content of RFC2308 as the comments for why the negative
 message cannot be cached if no SOA record.
 > Comment: the methods in !ResponseClassifier seem to logically belong
 with isNegativeResponse() and hasTheRecordInAuthoritySection(), although
 lib/cache seems the wrong place for a set of general message utility
 functions.  At the next F2F we should talk about where these should live.
 >
 Agree:)
 >
 > '''src/lib/cache/resolver_cache.{cc,h}'''[[BR]]
 > !ResolverClassCache(!CacheSizeInfo): the argument would be better passed
 as a const !CacheSizeInfo&.
 >
 Done
 > ResolverClassCache::update(const isc::dns::ConstRRsetPtr rrset_ptr): the
 argument should be passed by reference.
 Done
 >
 >
 > '''src/lib/cache/tests/message_cache_unittest.cc'''[[BR]]
 > !DerivedMessageCache constructor: suggest use of RRsetCachePtr as
 argument type instead of boost::shared_ptr<RRsetCache>
 >
 Done
 >
 > '''src/lib/cache/tests/message_entry_unittest.cc'''[[BR]]
 > !DerivedMessageEntry constructor: suggest use of RRsetCachePtr as
 argument type instead of boost::shared_ptr<RRsetCache>
 Done
 >
 >
 > '''src/lib/cache/tests/negative_cache_unittest.cc'''[[BR]]
 > testNXDOMAIN test: after the first lookup, I suggest more checks on the
 contents of returned messages (e.g. for msg_nxdomain, one could check that
 the answer and authority sections are empty, and that the authority
 section contains a single RR that is an SOA).
 >
 Done
 > testNXDOMAIN test: two queries are done for "nonexist.example.com" and
 both checks that the TTL of the SOA is 86400.  If a two-second sleep is
 inserted before the second lookup, a check could be made that the TTL is
 less than 86400 by that amount (or, to be more robust, between about 86397
 and 86399).
 >
 Done
 > Suggest tests with different SOA records:
 [http://tools.org.ietf/html/rfc2308 RFC 2308] says that the TTL of the
 negative response SOA is the minimum of the TTL of the SOA itself and the
 SOA "minimum" field.  Need three tests, with "SOA TTL" less that, equal to
 and greater than "SOA Minimum" to check that the TTL entered into the
 cache is the the correct value.  (Should also do the same for an SOA entry
 going into the normal cache to ensure that the TTL associated with the
 record is not influenced by the setting of the "minimum" value.)
 The following is feedback from Mark Andrews for this problem:
   ''Reduce it if it is greater than the cache's max negative cache ttl
 otherwise just honour it.''

   ''As with caching positive responses it is sensible for a resolver to
   limit for how long it will cache a negative response as the protocol
   supports caching for up to 68 years.  Such a limit should not be
   greater than that applied to positive answers and preferably be
   tunable.  Values of one to three hours have been found to work well
   and would make sensible a default.  Values exceeding one day have
   been found to be problematic.''

 So I add a MAX_NORMAL_CACHE_TTL and MAX_NEGATIVE_CACHE_TTL, if the ttl is
 not larger than it, just keep it. The default value is as the same as
 bind9, but for bind9 this is user configurable. So I add a TODO note as a
 future improvement.

 >
 > testNXDOMAINCname: need to check that the sections in the
 msg_nxdomain_cname message returned from the cache are as expected.
 >
 Done
 > testNoerrorNodata and testReferralResponse: same comments as above apply
 to the checking of the SOA TTLs and the contents of the other sections in
 the message.
 Done

-- 
Ticket URL: <http://bind10.isc.org/ticket/493#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development