If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up:
try: RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False) except: RequireUnboundReload = True
This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once.
On 24/03/2024 15:49, Nick Howitt wrote:
Hi all,
**** This is a repost of an email incorrectly submitted. ****
Please bear with me as I am new to IPFire and not at all used to this style of development, so any guidance would be appreciated.
There is a bug, https://bugzilla.ipfire.org/show_bug.cgi?id=13254, where unbound-dhcp-leases-bridge restarts unbound a lot when it is totally unnecessary. It appears to be because when a lease is renewed, the script gets triggered, created a new /etc/unbound/dhcp-leases.conf then restarts Unbound to read the file. Generally this seems to be unnecessary as, with a renewed lease, the old and new /etc/unbound/dhcp-leases.conf files are the same. With 5 leases in that file (one for a machine not active) I am getting 3-4 restarts an hour of Unbound when I have a min/max lease time of 60/120min.
Looking at the code, it is fairly easy to fix. The current code creates a temp file then copies it into place then restarts unbound. All I am doing is doing a file comparison before the copy and skipping the restart if the files are the same.
There were a couple of gotchas because setting the file attributes and copying the file were done inside the "with" block for generating the temporary file. This meant a file comparison always returned False as the temp file was still open and so never the same is the old file.
I moved those two statements outside the "with". This forced me to change the fchmod to chmod.
It could be argued that the file copy should not be done if the files are not different, but it would take an extra "if" and you'd have to remember to delete the temp file. If required, I can make that change.
Also, one small thing I noticed once is that the old and new dhcp-leases.conf files could occasionally contain the same leases but in a different order. I have been unable to reproduce, but to sidestep it, instead of stepping through the leases variable directly, I sort it and step through that. It should make the resulting file completely deterministic and make the file comparison more effective.
My patch is:
From 73873d4944944a2f02317a73074a6894726f36f7 Mon Sep 17 00:00:00 2001 From: Nick Howitt nick@howitts.co.uk Date: Sun, 24 Mar 2024 15:17:19 +0000 Subject: [PATCH] Stop unbound-dhcp-leases-bridge from restarting unbound every time it write dhcp-leases.conf as it is very often unchanged and does not require a restart.
config/unbound/unbound-dhcp-leases-bridge | 28 +++++++++++++++-------- 1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge index e9f022aff..d22772066 100644 --- a/config/unbound/unbound-dhcp-leases-bridge +++ b/config/unbound/unbound-dhcp-leases-bridge @@ -22,6 +22,7 @@ import argparse import datetime import daemon +import filecmp import functools import ipaddress import logging @@ -516,26 +517,35 @@ class UnboundConfigWriter(object):
def update_dhcp_leases(self, leases): # Write out all leases - self.write_dhcp_leases(leases) + if self.write_dhcp_leases(leases):
- log.debug("Reloading Unbound...") + log.debug("Reloading Unbound...")
- # Reload the configuration without dropping the cache - self._control("reload_keep_cache") + # Reload the configuration without dropping the cache + self._control("reload_keep_cache")
+ else:
+ log.debug("Not reloading Unbound. Leases not changed.")
def write_dhcp_leases(self, leases): log.debug("Writing DHCP leases...")
with tempfile.NamedTemporaryFile(mode="w", delete=False) as f: - for l in leases: + for l in sorted(leases): for rr in l.rrset: f.write("local-data: "%s"\n" % " ".join(rr))
- # Make file readable for everyone - os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH) + filecmp.clear_cache() + RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False)
+ # Make file readable for everyone + os.chmod(f.name, stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)
+ # Move the file to its destination + os.rename(f.name, self.path)
- # Move the file to its destination - os.rename(f.name, self.path) + return RequireUnboundReload
def _control(self, *args): command = ["unbound-control"]