From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adolf Belka To: development@lists.ipfire.org Subject: Re: Stop unbound-dhcp-leases-bridge from continually restarting unbound Date: Sun, 24 Mar 2024 16:38:51 +0100 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7825481225945211964==" List-Id: --===============7825481225945211964== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Nick, You need to send the patch, as you have it below to the ipfire=20 development list. With PATCH in the subject line it will be recognised by the IPFire=20 development mailing list as a patch and will also be picked up in the=20 IPFire Patchwork system. https://patchwork.ipfire.org/project/ipfire/list/ As this email has a line that contains Subject: [PATCH] it has already=20 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=20 will be merged by one of the devs and if not accepted you will get a=20 response as a reply to the patch so a history will be kept of the whole=20 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=20 > style of development, so any guidance would be appreciated. > > There is a bug, https://bugzilla.ipfire.org/show_bug.cgi?id=3D13254,=20 > where unbound-dhcp-leases-bridge restarts unbound a lot when it is=20 > totally unnecessary. It appears to be because when a lease is renewed,=20 > the script gets triggered, created a new /etc/unbound/dhcp-leases.conf=20 > then 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=20 > that file (one for a machine not active) I am getting 3-4 restarts an=20 > 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=20 > creates a temp file then copies it into place then restarts unbound.=20 > All I am doing is doing a file comparison before the copy and skipping=20 > the 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=20 > in a different order. I have been unable to reproduce, but to sidestep=20 > it, instead of stepping through the leases variable directly, I sort=20 > it and 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=20 > unbound every > =C2=A0time it write dhcp-leases.conf as it is very often unchanged and does= =20 > 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(lease= s): > > -=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...") > > -=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.") > > =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 le= ases...") > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 with tempfile.NamedTempora= ryFile(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)) > > -=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) > > -=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 > > =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-cont= rol"] --=20 Sent from my laptop --===============7825481225945211964==--