Re: [PATCH] Stop unbound-dhcp-leases-bridge from continually restarting unbound

[Nick Howitt <nick@howitts.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 5700 bytes --]

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(a)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"]