From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Howitt To: development@lists.ipfire.org Subject: Re: [PATCH] Stop unbound-dhcp-leases-bridge from continually restarting unbound Date: Mon, 25 Mar 2024 10:10:08 +0000 Message-ID: <6b53833f-3527-4832-9561-ba3fe3a2ef23@howitts.co.uk> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4033059137210857040==" List-Id: --===============4033059137210857040== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable If there is a risk that the /etc/unbound/dhcp-leases.conf does not=20 exist, for example on a clean install (I don't know), the filecmp.cmp=20 line needs to be wrapped in a try...except to stop it blowing up: try: RequireUnboundReload =3D not filecmp.cmp(f.name, self.path, shallow=3DFalse) except: RequireUnboundReload =3D True This will force the file to be to be generated, even if it is only=20 copying in a blank temp file, and unbound will restart. It will only=20 ever happen once. On 24/03/2024 15:49, Nick Howitt wrote: >=20 > Hi all, >=20 > **** This is a repost of an email incorrectly submitted. **** >=20 > 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. >=20 > There is a bug, https://bugzilla.ipfire.org/show_bug.cgi?id=3D13254, where = > 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. >=20 > 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. >=20 > 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. >=20 > I moved those two statements outside the "with". This forced me to=20 > change the fchmod to chmod. >=20 > 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. >=20 > 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. >=20 > My patch is: >=20 > 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 doe= s not > =C2=A0require a restart. >=20 > --- > =C2=A0config/unbound/unbound-dhcp-leases-bridge | 28 +++++++++++++++------= -- > =C2=A01 file changed, 19 insertions(+), 9 deletions(-) >=20 > 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): >=20 > =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(lease= s): >=20 > -=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.deb= ug("Reloading Unbound...") >=20 > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Reload the configuration with= out dropping the cache > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 self._control("reload_keep_cach= e") > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Reloa= d 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._c= ontrol("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.deb= ug("Not reloading Unbound. Leases not changed.") >=20 > =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 l= eases...") >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 with tempfile.NamedTempor= aryFile(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 i= n 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 i= n 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)) >=20 > -=C2=A0=C2=A0=C2=A0=C2=A0=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=C2=A0=C2=A0=C2=A0=C2=A0 os.fchm= od(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 fi= lecmp.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 everyo= ne > +=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 destinat= ion > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 os.rename(f.name, self.path) >=20 > -=C2=A0=C2=A0=C2=A0=C2=A0=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=C2=A0=C2=A0=C2=A0=C2=A0 os.rena= me(f.name, self.path) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return RequireUnboundReload >=20 > =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-con= trol"] --===============4033059137210857040==--