From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Howitt To: development@lists.ipfire.org Subject: Re: Stop unbound-dhcp-leases-bridge from continually restarting unbound Date: Sun, 24 Mar 2024 15:51:15 +0000 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6824404125386126525==" List-Id: --===============6824404125386126525== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Adolf, Thanks. I'd read that link before but missed changing the Subject line=20 when I created the email. As you see, I've resubmitted it. Regards, Nick On 24/03/2024 15:38, Adolf Belka wrote: >=20 > Hi Nick, >=20 > You need to send the patch, as you have it below to the ipfire=20 > development list. >=20 > 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. >=20 > https://patchwork.ipfire.org/project/ipfire/list/ >=20 > 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./ >=20 > 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. >=20 > There is a wiki page on patch submission for more details. >=20 > https://www.ipfire.org/docs/devel/submit-patches >=20 > Regards, >=20 > Adolf. >=20 >=20 > 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 doe= s=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(leas= es): >> >> -=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.de= bug("Reloading Unbound...") >> >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Reload the configuration wit= hout dropping the cache >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 self._control("reload_keep_cac= he") >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # Relo= ad 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._= control("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.de= bug("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 l= eases...") >> >> =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 = 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= 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.fch= mod(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 f= ilecmp.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 every= one >> +=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 destina= tion >> +=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.ren= ame(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-con= trol"] >=20 --===============6824404125386126525==--