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@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"]