Implement DNS cache recycling #1840
Conversation
…cking of certain domain/client/type combinations Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
|
This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there: https://discourse.pi-hole.net/t/pi-hole-blocking-domain-it-shouldnt/67111/14 |
…one already stored in the query, we have to re-lookup the cache ID. This can happen when a CNAME chain is followed and analyzed Signed-off-by: DL6ER <dl6er@dl6er.de>
|
Confirmed working by OP on Discourse |
| @@ -378,17 +377,17 @@ void runGC(const time_t now, time_t *lastGCrun, const bool flush) | |||
|
|
|||
| // Update reply counters | |||
| counters->reply[query->reply]--; | |||
| log_debug(DEBUG_GC, "reply type %d removed (GC), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]); | |||
| log_debug(DEBUG_STATUS, "reply type %d removed (GC), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]); | |||
There was a problem hiding this comment.
Why did you change all the instances from DEBUG_GC > DEBUG_STATUS?
Here it even talks about the garbage collection.
There was a problem hiding this comment.
Because the status debug flag is meant specifically to log every single counter change (for every query!) so it's very very verbose. The GC debug flag prints much fewer lines with more condensed content. As these lines make no sense without DEBUG_STATUS being set at the same time (for context), I moved them over there
| @@ -482,6 +482,7 @@ void log_counter_info(void) | |||
| log_info(" -> Unknown DNS queries: %i", counters->status[QUERY_UNKNOWN]); | |||
| log_info(" -> Unique domains: %i", counters->domains); | |||
| log_info(" -> Unique clients: %i", counters->clients); | |||
| log_info(" -> DNS cache records: %i", counters->dns_cache_size); | |||
There was a problem hiding this comment.
What does "DNS cache records" tell us? How does it relate to the imported long-term data?
2024-01-04 22:11:36.590 [430399M] INFO: Imported 8396 queries from the long-term database
2024-01-04 22:11:36.598 [430399M] INFO: -> Total DNS queries: 8396
2024-01-04 22:11:36.599 [430399M] INFO: -> Cached DNS queries: 5696
2024-01-04 22:11:36.599 [430399M] INFO: -> Forwarded DNS queries: 1889
2024-01-04 22:11:36.599 [430399M] INFO: -> Blocked DNS queries: 35
2024-01-04 22:11:36.599 [430399M] INFO: -> Unknown DNS queries: 4
2024-01-04 22:11:36.599 [430399M] INFO: -> Unique domains: 583
2024-01-04 22:11:36.599 [430399M] INFO: -> Unique clients: 3
2024-01-04 22:11:36.599 [430399M] INFO: -> DNS cache records: 937
2024-01-04 22:11:36.599 [430399M] INFO: -> Known forward destinations: 2
There was a problem hiding this comment.
Every unique client/domain combination creates a unique DNS cache record. Two queries for the same domain done by the same client use one cache record. Queries for the same domain by three clients create three cache records (because some clients may be blocking a domain while others don't due to groups). Thia is just yet another metric used by FTL for quite some time and which seemed useful to include here.
What does this implement/fix?
This PR completes what was started in #1694 - memory recycling. The issue at hand was caused by adding the facility to recycle DNS cache records but then never actually doing it. The missing bits are added in this PR.
This fixes an issue described on Discourse (see link below): After a certain time has passed, clients and domains may be recycled, however the cache records saying that "clientID=123,domainID=234 -> blocked" remained. This was harmless until a new (possibly but not necessarily the old) client/domain combination in exactly this configuration appeared which then lead to a cached reply which was actually intended for something entirely different.
This is fixed by recycling cache records as soon as they are not referenced by any query. This is similar to how we recycle clients and domains, too - we remove them when the last query referencing them disappeared due to aging (> 24h).
Related issue or feature (if applicable): N/A
Pull request in docs with documentation (if applicable): N/A
By submitting this pull request, I confirm the following:
git rebase)Checklist:
developmentalbranch.