From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH] Stop unbound-dhcp-leases-bridge from continually restarting unbound Date: Tue, 26 Mar 2024 11:10:53 +0000 Message-ID: In-Reply-To: <7314ef76-9784-4663-b168-c2e71ce1ff8e@howitts.co.uk> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7844778328792449633==" List-Id: --===============7844778328792449633== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable > On 25 Mar 2024, at 13:11, Nick Howitt wrote: >=20 >=20 >=20 > On 25/03/2024 11:31, Michael Tremer wrote: >> Hello, >> I don=E2=80=99t think that the risk is very high, but I would account for = this problem. >> With my proposed changes from my previous email this should not be a large= problem at all. When opening /etc/unbound/dhcp-leases.conf with mode =E2=80= =9Cr+=E2=80=9D you can catch the Exception and skip the comparison and open t= he file again with =E2=80=9Cw=E2=80=9D. >>> On 25 Mar 2024, at 10:10, Nick Howitt wrote: >>>=20 >>> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist,= for example on a clean install (I don't know), the filecmp.cmp line needs to= be wrapped in a try...except to stop it blowing up: >>>=20 >>> try: >>> RequireUnboundReload =3D not filecmp.cmp(f.name, self.path, shallow=3DFal= se) >>> except: >>> RequireUnboundReload =3D True >> I generally don=E2=80=99t like blanco exception catches. You will never kn= ow about the daemons that you are hiding. >>> This will force the file to be to be generated, even if it is only copyin= g in a blank temp file, and unbound will restart. It will only ever happen on= ce. >>>=20 >>>=20 >>> On 24/03/2024 15:49, Nick Howitt wrote: >>>> 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 st= yle of development, so any guidance would be appreciated. >>>> There is a bug, https://bugzilla.ipfire.org/show_bug.cgi?id=3D13254, whe= re unbound-dhcp-leases-bridge restarts unbound a lot when it is totally unnec= essary. It appears to be because when a lease is renewed, the script gets tri= ggered, 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 leas= e, the old and new /etc/unbound/dhcp-leases.conf files are the same. With 5 l= eases in that file (one for a machine not active) I am getting 3-4 restarts a= n 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 i= s doing a file comparison before the copy and skipping the restart if the fil= es are the same. >>>> There were a couple of gotchas because setting the file attributes and c= opying the file were done inside the "with" block for generating the temporar= y file. This meant a file comparison always returned False as the temp file w= as still open and so never the same is the old file. >>>> I moved those two statements outside the "with". This forced me to chang= e the fchmod to chmod. >>>> It could be argued that the file copy should not be done if the files ar= e not different, but it would take an extra "if" and you'd have to remember t= o 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 ord= er. 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 sho= uld make the resulting file completely deterministic and make the file compar= ison 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= every >>>> time it write dhcp-leases.conf as it is very often unchanged and does n= ot >>>> 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=3D"w", delete=3DFalse) 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_IRGR= P|stat.S_IROTH) >>>> + filecmp.clear_cache() >>>> + RequireUnboundReload =3D not filecmp.cmp(f.name, self.path, sha= llow=3DFalse) >>>> + >>>> + # 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 =3D ["unbound-control"] > Resubmitting the patch. >=20 > Following my previous reply, I would like to stick with filecmp as I am usi= ng it with "shallow=3DFalse" to avoid doing the comparison just with stat() c= alls. Note that I also flush the filecmp cache before using the command so it= is running with the latest file data. Talking about file descriptors, atomic= file replacement etc., scares me. File descriptors are great. They avoid race-conditions and all sorts of thing= s. I don=E2=80=99t think that module is the best choice really if you have to ha= ck around and flush the cache. Why would it even cache things in the first pl= ace? I don=E2=80=99t see where the cache helps. > I have modified the earlier try/except I proposed to catch only FileNotFoun= dError, and I have changed CamelCase to snake_case. >=20 > --- > config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++------- > 1 file changed, 22 insertions(+), 9 deletions(-) >=20 > diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unb= ound-dhcp-leases-bridge > index e9f022aff..a9ec8e1e2 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,38 @@ class UnboundConfigWriter(object): >=20 > def update_dhcp_leases(self, leases): > # Write out all leases > - self.write_dhcp_leases(leases) > + if self.write_dhcp_leases(leases): >=20 > - log.debug("Reloading Unbound...") > + log.debug("Reloading Unbound...") >=20 > - # 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.") >=20 > def write_dhcp_leases(self, leases): > log.debug("Writing DHCP leases...") >=20 > with tempfile.NamedTemporaryFile(mode=3D"w", delete=3DFalse) as f: > - for l in leases: > + for l in sorted(leases): > for rr in l.rrset: > f.write("local-data: \"%s\"\n" % " ".join(rr)) >=20 > - # Make file readable for everyone > - os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH) > + filecmp.clear_cache() > + try: > + require_unbound_reload =3D not filecmp.cmp(f.name, self.path, shallow=3DF= alse) > + except FileNotFoundError: > + require_unbound_reload =3D True If you want to go this route, you will have to flush the temporary file first= . Otherwise you might have some data left in the buffer that has not been wri= tten (because the file is still open). You will then compare incomplete outpu= t. This would not become a problem if you used the file descriptors. Catching FileNotFound would work for me. > + > + # 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) >=20 > - # Move the file to its destination > - os.rename(f.name, self.path) > + return require_unbound_reload >=20 > def _control(self, *args): > command =3D ["unbound-control"] > --=20 > 2.43.0 >=20 >=20 > Nick --===============7844778328792449633==--