dnsdist: keepStaleData is not that useful; entries expiring when servers online

[karelbilek]

In dnsdist, caches have keepStaleData option.

This option is not that useful; if the entries expire while the backends are still online, they are still deleted from the cache.

We need a cache that will keep the stale data there even when the backends are online; while they will not be served when they are online, they will also not be removed. (We need it for a local stub resolver that has the cache as a fallback in the case the actual resolver is down.)

I am currently implementing option called (name work-in-progress 😆 ) alwaysKeepStaleData, that will always keep the stale data.

There is a problem though with the cache potentially being full, as nothing gets deleted; what I did is detect full shard and call something similar to expunge to 1/2 in case it's full. (The expectation - in our case - is, that full cache is relatively rare and should not happen that often; so it's fine.)

expunge as currently written just takes the elements ~randomly (the order is implementation detail of std::unordered_map), which is fine I guess? I don't want to spend too much time on sorting the entries by validity and removing the oldest, because that would lock the shard.

Not sure if to add this feature as a new option, or to add it to keepStaleData.

[karelbilek]

I am making WIP draft on this branch

https://github.com/karelbilek/pdns/tree/kb/alwaysKeepStaleData

I will make it into a MR when I test it a bit more

[karelbilek]

I realized when experimenting it will be better to implement it as LRU cache; so, in case of the cache filling, it will just remove the last accessed; it should be easy to implement with linked list

edit: however that changes the locking logic; it doesn't seem like a simple task to keep both the current implementation (for the current behaviour) AND LRU/FIFO cache. Hm...

[rgacogne]

Note that we are currently not using a LRU/FIFO/etc design because it requires updating the order of entries, and thus taking a lock, for every cache hit, which is terrible for performance. I looked at S3-FIFO recently which is better in that area (most of the time we are only reading an atomic counter on a cache hit, sometimes updating it) but it was still introducing a noticeable performance hit, at least with my implementation.

[karelbilek]

if you use FIFO, you need to reorder only on insert (or rewrite), not on cache hit, no?

[rgacogne]

Correct, but IMHO plain FIFO is not very efficient with DNS because it doesn't take the TTL into account.

[karelbilek]

Yes true.

What I seem to be hitting is, that we don't really care that much about performance here; we don't get many requests here (the local DNS have just a few queries); I think I might not upstream the patches in this case.

[karelbilek]

Maybe I can still upstream it with a note about performance; I will need to figure out how to keep both of the current logic and the fifo queue code in the code without a performance hit. But not sure if it's worth it @rgacogne .