Opened 9 years ago

Closed 9 years ago

#493 closed enhancement (complete)

Negative cache

Reported by: shane Owned by: ocean
Priority: medium Milestone: R-Team-Sprint-20110316
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 5.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Negative cache (possibly large task)

  • Quick & dirty hack

Subtickets

Change History (9)

comment:1 Changed 9 years ago by stephen

Suggest that we use the IXFR-style indication for negative caching of a given domain:

  • An entry for a given QTYPE with no RRsets indicates no data for that particular type.
  • An entry with QTYPE=ANY and no RRsets indicates no data for that name.

comment:2 Changed 9 years ago by ocean

  • Owner set to ocean
  • Status changed from new to assigned

comment:3 Changed 9 years ago by ocean

  • Owner changed from ocean to stephen
  • Status changed from assigned to reviewing

comment:4 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to ocean

src/lib/cache/message_cache.{cc,h}
Following recent discussions about future code stubs, I suggest that the stubs for the MessageCache methods dump()/load() and resize() are removed.

MessageCache constructor: Suggest that the first and last arguments (rrset_cache and negative_soa_cache) are passed by reference.

MessageCache constructor: Suggest that the type of the first and last arguments are set to RRsetCachePtr (this is defined in rrset_cache.h).

src/lib/cache/message_entry.{cc,h}
RRsetRef constructor: the "cache" parameter should be passed by reference and the type can be defined as a RRsetCachePtr.

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.

MessageEntry constructor. Both the cache parameters can be passed by reference (and both types set as RRsetCachePtr).

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?)

src/lib/cache/message_utility.{cc,h}
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.

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.

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.

src/lib/cache/resolver_cache.{cc,h}
ResolverClassCache(CacheSizeInfo): the argument would be better passed as a const CacheSizeInfo&.

ResolverClassCache::update(const isc::dns::ConstRRsetPtr rrset_ptr): the argument should be passed by reference.

src/lib/cache/tests/message_cache_unittest.cc
DerivedMessageCache constructor: suggest use of RRsetCachePtr as argument type instead of boost::shared_ptr<RRsetCache>

src/lib/cache/tests/message_entry_unittest.cc
DerivedMessageEntry constructor: suggest use of RRsetCachePtr as argument type instead of boost::shared_ptr<RRsetCache>

src/lib/cache/tests/negative_cache_unittest.cc
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).

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).

Suggest tests with different SOA records: 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.)

testNXDOMAINCname: need to check that the sections in the msg_nxdomain_cname message returned from the cache are as expected.

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.

comment:5 in reply to: ↑ 4 Changed 9 years ago by ocean

Replying to stephen:

src/lib/cache/message_cache.{cc,h}
Following 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}
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}
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}
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
DerivedMessageCache constructor: suggest use of RRsetCachePtr as argument type instead of boost::shared_ptr<RRsetCache>

Done

src/lib/cache/tests/message_entry_unittest.cc
DerivedMessageEntry constructor: suggest use of RRsetCachePtr as argument type instead of boost::shared_ptr<RRsetCache>

Done

src/lib/cache/tests/negative_cache_unittest.cc
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: 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

comment:6 Changed 9 years ago by ocean

  • Owner changed from ocean to stephen

comment:7 Changed 9 years ago by stephen

  • Owner changed from stephen to ocean

src/lib/cache/message_utility.{cc,h}
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.

OK - I've added ticket #688 to address this.

Suggest tests with different SOA records...

I'm afraid my comment was due to a mis-reading of RFC 2308: use of the SOA minimum field applies to authoritative servers and is concerned with setting the TTL of the record in the response. So you are right in just taking the TTL of the returned SOA record and limiting it to a maximum value.

All looks OK, please merge.

comment:8 Changed 9 years ago by ocean

Merged to master branch

git f8fb852bc6aef292555063590c361f01cf29e5ca

comment:9 Changed 9 years ago by ocean

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.