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

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

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

Hi Adolf,

Thanks. I'd read that link before but missed changing the Subject line 
when I created the email. As you see, I've resubmitted it.

Regards,

Nick

On 24/03/2024 15:38, Adolf Belka wrote:
> 
> 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"]
>