weave-225: [dns] caching (1/3)

[inercia]

#225 : this pull request implements a caching mechanism in the DNS server. The cache is used for saving positive and negative replies obtained by the local discovery system as well as by the recursive resolver.

Replies found in the cache are modified before being returned to clients by adjusting the remaining TTL and shuffling answers (implementing a basic load balancing mechanism as a side effect, a first step for #226).

[squaremo]

The DNS server is also improved by using a limited number of concurrent resolvers.

Could this be changed in a separate PR, to make things more incremental?

[inercia]

@squaremo It could be done, but that would require some surgery in the server code, and it would make many parts of the cache useless (as the cache is designed for concurrent access)...

[inercia]

@squaremo I have reverted the changes in the server, so it uses the old mechanism + the cache...

[squaremo]

Thanks @inercia, I'll take another look

[rade]

Surely we need to significantly lower the default ttl in dns.go...

const (
    localTTL uint32 = 300 // somewhat arbitrary; we don't expect anyone
    // downstream to cache results
)

See the discussion in #225. IMO anything higher than 30s is going to be problematic in a fair number of situations. I suggest we make that the default, with a facility to override the value via an option to weave launch-dns.

Incidentally, if this PR closes #225, then a separate PR needs to be opened to address the scalability issues identified in #225.

[inercia]

I agree, I will reduce the localTTL... Maybe we could also adopt a common practice in CDNs: to return answers with tiny TTLs for some queries, in this particular case, for local queries. Even when the nominal TTL would be used for things like determining the time answers live in the cache, users would be forced to re-query to WeaveDNS each time they want to resolve a name, and that would give us the opportunity to shuffle answers, or load-balance in some more elaborated way. I think that wouldn't be a problem unless a WeaveDNS servers thousands of containers...

I will open a new PR later on for the scalability issues you described...

[rade]

Thinking about it, 30s still seems quite high. 10s?

[squaremo]

I don't see how there will ever be multiple RRs for a local (on the weave network) name: both the zone and the mdns client return just one RR, which the cache will take as a replacement for any existing answer. The only time multiple answers will come through is from a fallback query (arguably we ought not be permuting those).

In fact, even if the zone or mDNS returned multiple RRs, they would get replaced in the cache, so it would always be the answers from one peer (us, or whoever answers mDNS first).

[inercia]

I assume this comment is about the shuffle operation. I agree that we are not currently using this feature, but we could return multiple answers in the future (eg, #338). It should have no effect or performance cost when returning only one answer...

[squaremo]

I assume this comment is about the shuffle operation.

In part; but see the second paragraph which I think is a more serious problem, unless I've misunderstood the code.

[inercia]

Yes, I thought about that, but we could a) store in the cache the first response we receive (as you said) or b) apply some heuristics and merge all the responses, setting some reasonable values for things like TTLs, etc...

[squaremo]

b) apply some heuristics and merge all the responses,

Something like that would have to be done, yes, but also lookups would have to be tried in parallel (since in general you'd want to return all answers on the network). I think the interaction with the cache would be quite different, and possibly using mDNS wouldn't be workable.

In any case, at the minute the behaviour will be:

1. Answer with whatever's in the cache, until it expires
2. Get another answer

whereas if we are sticking to single answers I think we want

1. Answer with one thing that's in the cache that's not expired
2. If there are no answers, start looking for some

[inercia]

Yes, there could be a periodic process that, for local queries, could try to get more answers with mDNS and merge answers and store them in the cache for future queries...

[squaremo]

think that the complexity will not be in this cache: the complexity will really be in the gossiping mechanism...

The complexity will be in the gossiping, in the zone DB, in the cache, and in the interactions among them and with other code. Please can we solve one thing at a time, until it's demonstrated that e.g., caching local query responses -- as well as having the records in the zone DB -- is necessary.

[rade]

Please can we solve one thing at a time, until it's demonstrated that [...] is necessary.

IMO that also applies to caching results of recursive queries.

[rade]

Do we know how well-known caching DNS servers, such as dnsmasq, perform caching? I am concerned that a cache keyed on the octet-sequence of a question, and storing entire answer sets, might be semantically suspect.

[inercia]

@rade I think solutions applied by others do not need to be valid or applicable for us, as we do not have the resources others have (otherwise, we could reimplement BIND9 for Weave). Using the question as the key should be perfectly valid. A better granularity could lead to better cache usage, but that would add more complexity and I don't think we really need to optimize space here...

Regarding recursive lookups, I do not understand why you are so reluctant to use the cache for them. I wouldn't underestimate these resolutions: they could have a big impact for users in some scenarios (e.g., a user setting HostnameLookups On in apache.conf). Caching these resolutions comes for free and, in fact, using the same mechanism for both local and recursive lookups simplifies the code a lot, so it is a win-win for me...

[squaremo]

I think solutions applied by others do not need to be valid or applicable for us, as we do not have the resources others have

The suggestion is to look for indications of what is the correct way to handle proxied responses -- whether it's OK to pass every record on. djbdns, for example, appears to do a lot of work to sanitise responses. If we aren't prepared to figure this out we have no business providing a caching name server.

Caching these resolutions comes for free

Only if you think development and maintenance effort on our part, and troubleshooting and operation on our users' part, are free.

[inercia]

@squaremo In conclusion, do you want to remove the cache for recursive queries?

[squaremo]

In conclusion, do you want to remove the cache for recursive queries?

No, I just want us to be clear where we are providing benefit, and what the trade-off is. For example, many deployments will already have a caching nameserver, or be happily using 8.8.8.8, and in those deployments we are effectively adding needless machinery; some won't, but solutions exist already -- use dnsmasq or whatever -- and are we sure ours is really better?

I know you've done some benchmarking, so perhaps you can make a quantitative argument in this direction.

[rade]

There are a bunch of RFCs pertaining to caching...

• RFC1034 (NB: results of inverse queries must not be cached!)
• RFC1035
• RFC2181
• RFC2308 (Caching negative results is hard)

The upshot is that there is a lot to consider when caching answers from recursive queries. Most of these do not apply for local queries because, a) as Michael said, we really should consider the collection of all weaveDNS servers as a single server, and b) our servers only deal with a very limited number of query / record types.

[inercia]

Ok, no problem, I'll remove caching for recursive resolutions.

[inercia]

@rade and @squaremo , I have updated this PR with the following changes:

• the cache for recursive queries has been disabled
• I have squashed all the commits in the branch (rebasing master was being really annoying...)
• I have merged the changes in the PR 3/3 (weave-225: [dns] caching (3/3) #447) (to use a heap for optimizing garbage collections)

Let me know what you think about this...

[squaremo]

What is the rationale behind keeping the heap of entries in order of expiry? I would have thought either of insertion time (nice and easy) or LRU (more trouble but more useful perhaps) would be preferable.

[inercia]

@squaremo insertion time wouldn't give you the best order for getting rid of entries (could lead to useful entries being evicted before useless entries), and I'm not sure about the LRU (it would require more logic, and maybe it could make Get()s more expensive...). I think the heap for validUntil is the nice and easy solution...

[squaremo]

insertion time wouldn't give you the best order for getting rid of entries (could lead to useful entries being evicted before useless entries)

It depends what you think is useful or useless. An entry with a long TTL that is asked for very infrequently could be considered useless, while an fast-moving entry that gets asked for continually could be considered useful. Ordering the evictions by how soon they expire will tend to preserve the former while evicting the latter, sometimes ahead of schedule.

Ordering by time of insertion means that you will tend to refresh (reinsert because of expiry) fast-moving entries well before they can get evicted, while slow-moving entries might make it to the top of the heap before being refreshed, but if so weren't being asked for very often anyway.