From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Howitt To: development@lists.ipfire.org Subject: [PATCH] Stop unbound-dhcp-leases-bridge from continually restarting unbound Date: Sun, 24 Mar 2024 15:49:46 +0000 Message-ID: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1832935776148055295==" List-Id: --===============1832935776148055295== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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=20 style of development, so any guidance would be appreciated. There is a bug, https://bugzilla.ipfire.org/show_bug.cgi?id=3D13254, where=20 unbound-dhcp-leases-bridge restarts unbound a lot when it is totally=20 unnecessary. It appears to be because when a lease is renewed, the=20 script gets triggered, created a new /etc/unbound/dhcp-leases.conf then=20 restarts Unbound to read the file. Generally this seems to be=20 unnecessary as, with a renewed lease, the old and new=20 /etc/unbound/dhcp-leases.conf files are the same. With 5 leases in that=20 file (one for a machine not active) I am getting 3-4 restarts an hour of=20 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=20 a temp file then copies it into place then restarts unbound. All I am=20 doing is doing a file comparison before the copy and skipping the=20 restart if the files are the same. There were a couple of gotchas because setting the file attributes and=20 copying the file were done inside the "with" block for generating the=20 temporary file. This meant a file comparison always returned False as=20 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=20 change the fchmod to chmod. It could be argued that the file copy should not be done if the files=20 are not different, but it would take an extra "if" and you'd have to=20 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=20 dhcp-leases.conf files could occasionally contain the same leases but in=20 a different order. I have been unable to reproduce, but to sidestep it,=20 instead of stepping through the leases variable directly, I sort it and=20 step through that. It should make the resulting file completely=20 deterministic and make the file comparison more effective. My patch is: From 73873d4944944a2f02317a73074a6894726f36f7 Mon Sep 17 00:00:00 2001 From: Nick Howitt Date: Sun, 24 Mar 2024 15:17:19 +0000 Subject: [PATCH] Stop unbound-dhcp-leases-bridge from restarting unbound=20 every =C2=A0time it write dhcp-leases.conf as it is very often unchanged and does = not =C2=A0require a restart. --- =C2=A0config/unbound/unbound-dhcp-leases-bridge | 28 +++++++++++++++-------- =C2=A01 file changed, 19 insertions(+), 9 deletions(-) diff --git a/config/unbound/unbound-dhcp-leases-bridge=20 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 @@ =C2=A0import argparse =C2=A0import datetime =C2=A0import daemon +import filecmp =C2=A0import functools =C2=A0import ipaddress =C2=A0import logging @@ -516,26 +517,35 @@ class UnboundConfigWriter(object): =C2=A0=C2=A0=C2=A0=C2=A0 def update_dhcp_leases(self, leases): =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Write out all leases -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 self.write_dhcp_leases(leases) +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if self.write_dhcp_leases(leases): -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 log.debug("Reloading Unbound...") +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 log.debug= ("Reloading Unbound...") -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Reload the configuration withou= t dropping the cache -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 self._control("reload_keep_cache") +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Reload = the configuration without dropping the cache +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 self._con= trol("reload_keep_cache") + +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else: + +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 log.debug= ("Not reloading Unbound. Leases not changed.") =C2=A0=C2=A0=C2=A0=C2=A0 def write_dhcp_leases(self, leases): =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 log.debug("Writing DHCP lea= ses...") =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 with tempfile.NamedTemporar= yFile(mode=3D"w", delete=3DFalse) as f: -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for l in = leases: +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for l in = sorted(leases): =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 for rr in l.rrset: =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 f.write("local-data: \"%s\"\n" = % " ".join(rr)) -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Make fi= le readable for everyone -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 os.fchmod= (f.fileno(),=20 stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH) +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 filecmp.clear_cache() +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 RequireUnboundReload =3D not file= cmp.cmp(f.name, self.path,=20 shallow=3DFalse) + +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Make file readable for everyone +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 os.chmod(f.name,=20 stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH) + +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Move the file to its destination +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 os.rename(f.name, self.path) -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Move th= e file to its destination -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 os.rename= (f.name, self.path) +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return RequireUnboundReload =C2=A0=C2=A0=C2=A0=C2=A0 def _control(self, *args): =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 command =3D ["unbound-contr= ol"] --=20 2.43.0 I have this locally on a branch on my firewall. If it is OK, do I need=20 some sort of rights to push my branch and then how I get it merged? Regards, Nick --===============1832935776148055295==--