Re: Stop unbound-dhcp-leases-bridge from continually restarting unbound

[Adolf Belka <adolf.belka@ipfire.org>]

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

Hi Nick,

You need to send the patch, as you have it below to the ipfire 
development list.

With PATCH in the subject line it will be recognised by the IPFire 
development mailing list as a patch and will also be picked up in the 
IPFire Patchwork system.

https://patchwork.ipfire.org/project/ipfire/list/

As this email has a line that contains Subject: [PATCH] it has already 
been picked up by Patchwork. You can check via the above link./

Then the patch will be reviewed on this mailing list. If accepted, it 
will be merged by one of the devs and if not accepted you will get a 
response as a reply to the patch so a history will be kept of the whole 
patch review/discussion etc.

There is a wiki page on patch submission for more details.

https://www.ipfire.org/docs/devel/submit-patches

Regards,

Adolf.


On 24/03/2024 16:25, Nick Howitt wrote:
> Hi all,
>
> 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"]

-- 
Sent from my laptop