Remove hardcoded/unnecessary local-ttl/cache-size values in dnsmasq.conf

[celly]

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

• [✔️] I have read and understood the contributors guide.
• [✔️] I have checked that another pull request for this purpose does not exist.
• [✔️] I have considered, and confirmed that this submission will be valuable to others.
• [✔️] I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
• [✔️] I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

3.2

---

Most users do not need these values set, and they seem to be an artifact of the original code base. By having them hard coded in this file, it makes it so you cannot permanently overwrite them since they will be overwritten every time this the config file is written. You can also not add them to a custom config, since they will be error as dupe.

This template was created based on the work of udemy-dl.

[CLAassistant]

All committers have signed the CLA.

[AzureMarker]

Perhaps a better way to go about this would be to add a setupVars config setting with a default assigned by the installer? Also, make sure to base your code off of development.

[celly]

@Mcat12 I'll be more than happy to do the leg work to move these into there, however -- majority of users don't want or need these set. In fact, looking at the commits for these (1,2) those seem to either be accidental or artifacts. I just happened upon them while debugging dhcp delays.

[mibere]

Without setting cache-size at all the default is just 150 - much too low in my opinion.

[celly]

@mibere In a world of 300s ttl's and client side caching I don't think the cache is being used as much as you think. In additions, since your client and pi-hole will have matching ttl's follow up requests from the same client will be misses. So, the only cache hits you get are for common duplicate requests across clients. So a large cache here just burns memory.

If you want details on the performance of pi-hole dns cache:

$ sudo pkill -USR1 dnsmasq
$ cat /var/log/pihole.log | grep "cache size"

Wait a bit, and do it again, and you'll get a good idea of effectiveness.

Sep 20 19:15:02 dnsmasq[14028]: cache size 150, 7541/125481 cache insertions re-used unexpired cache entries.
Sep 20 19:36:44 dnsmasq[3193]: cache size 150, 0/801 cache insertions re-used unexpired cache entries.

[WaLLy3K]

We've been having some discussion internally about this. Firstly, local-ttl from man dnsmasq:

When replying with information from /etc/hosts or configuration or the DHCP leases file dnsmasq by default sets the time-to-live field to zero, meaning that the requester should not itself cache the information. This is the correct thing to do in almost all situations. This option allows a time-to-live (in seconds) to be given for these replies. This will reduce the load on the server at the expense of clients using stale data under some circumstances.

We've verified that setting this does not affect the upstream TTL's served to the Pi-hole clients, but does affect domains sinkholed by gravity - so we've agreed that changing this to 2 will make the whitelisting experience much better for the end user. As for DHCP leases, having a low TTL doesn't negatively affect clients who see Pi-hole as their DHCP server as all queries can immediately be answered from dnsmasq's cache.

This leads us to the discussion on cache-size:

Set the size of dnsmasq's cache. The default is 150 names. Setting the cache size to zero disables caching.

We believe (through our own testing and observation) that altering cache-size may affect performance due to the domains we sinkhole (especially when we're seeing people block in excess of 3 million), so we're not comfortable with removing this right now. If one was to alter dnsmasq's cache-size (again, according to our own tests and observations), the daemon memory usage would be:

• Caching disabled: 3392 KB
• Caching at 100,000 (PH default): 4292 KB

With this in mind, what benefits would you personally see by removing these values?

[celly]

@WaLLy3K Thanks for the great response.

1. Setting local-ttl to 2, makes sense, think that is a fine solution.

2. I don't think I'm going to fight this much longer, because I was wrong. And, looking at the DNSMasq code, the memory increase you see is just the allocation for the hash, and not populated, but that seems to be a moot point now.

I apologize in advance -- I should of checked the DNSmasq code first. I think you're right, I may be way off on this. Looking at how dm caching works it does appear that a larger cache value will greatly benefit users with large block lists. With minimal impact for more casual users. Since, to be honest, if I was really hard up for memory I'd be optimizing php for the admin...

So, I think it'll be best if I close this PR, and if I get a chance pull these out as hardcodes, and resubmit.

[WaLLy3K]

Hey @celly,

I'm glad you're happy with the proposed solution for local-ttl=2! Your PR prompted us to review how we were handling this, and it definitely made total sense to change the value (so hey, don't apologise!). I'll make a PR for this.

Personally, I was open to having cache-size as a user set value - but 100,000 (what we use as default) is the hardcoded limit for dnsmasq. That does mean that smaller networks will have expired TTL's, but my understanding is that it doesn't really matter other than "statistics for statistics sake" - so I've been convinced that there's not really usefulness in the ability to customise this.

Also (from what we understand), the memory value for a cache-size of 100,000 is statically set and shouldn't dynamically increase over time.

We're looking back over the codebase and seeing what we can optimise - among other things, Web Admin is definitely something we've got in mind. 😄

Since you've mentioned it, I'll go ahead and close this PR - but thank you for the resulting conversation!