Skip to content

Refactor the dns_adb unit

Ondřej Surý requested to merge 3239-fix-cleaning-of-adb-buckets into main

The dns_adb unit has been refactored to be much simpler. Following changes have been made:

  1. Simplify the ADB to always allow GLUE and hints

    There were only two places where dns_adb_createfind() was used - in the dns_resolver unit where hints and GLUE addresses were ok, and in the dns_zone where dns_adb_createfind() would be called without DNS_ADBFIND_HINTOK and DNS_ADBFIND_GLUEOK set.

    Simplify the logic by allowing hint and GLUE addresses when looking up the nameserver addresses to notify. The difference is negligible and would cause a difference in the notified addresses only when there's mismatch between the parent and child addresses and we haven't cached the child addresses yet.

  2. Drop the namebuckets and entrybuckets

    Formerly, the namebuckets and entrybuckets were used to reduced the lock contention when accessing the double-linked lists stored in each bucket. In the previous refactoring, the custom hashtable for the buckets has been replaced with isc_ht/isc_hashmap, so only a single item (mostly, see below) would end up in each bucket.

    Removing the entrybuckets has been straightforward, the only matching was done on the isc_sockaddr_t member of the dns_adbentry.

    Removing the zonebuckets required GLUEOK and HINTOK bits to be removed because the find could match entries with-or-without the bits set, and creating a custom key that stores the DNS_ADBFIND_STARTATZONE in the first byte of the key, so we can do a straightforward lookup into the hashtable without traversing a list that contains items with different flags.

  3. Remove unassociated entries from ADB database

    Previously, the adbentries could live in the ADB database even after unlinking them from dns_adbnames. Such entries would show up as "Unassociated entries" in the ADB dump. The benefit of keeping such entries is little - the chance that we link such entry to a adbname is small, and it's simpler to evict unlinked entries from the ADB cache (and the hashtable) than create second LRU cleaning mechanism.

    Unlinked ADB entries are now directly deleted from the hash table (hashmap) upon destruction.

  4. Cleanup expired entries from the hash table

    When buckets were still in place, the code would keep the buckets tables, because we almost always need to use a write lock for properly purging the hashtables. The ADB database needs to be sharded (similar to the effect that buckets had in the past). Each shard would contain own hashmap and own LRU list.

Future work:

  1. Lock contention

    In this commit, the focus was on correctness of the data structure, but in the future, the lock contention in the ADB database needs to be addressed. Currently, we use simple mutex to lock the hash tables, because we almost always need to use a write lock for properly purging the hashtables. The ADB database needs to be sharded (similar to the effect that buckets had in the past). Each shard would contain own hashmap and own LRU list.

  2. Time-based purging

    The ADB names and entries stay intact when there are no lookups. When we add separate shards, a timer needs to be added for time-based cleaning in case there's no traffic hashing to the inactive shard.

  3. Revisit the 30 minutes limit

    The ADB cache is capped at 30 minutes. This needs to be revisited, and at least the limit should be configurable (in both directions).

Closes #3239 (closed), #3238 (closed), #2615 (closed), #2078, #2437 (closed), #3312 (closed), #2441 (closed)

Edited by Ondřej Surý

Merge request reports