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 13:11:27 +0000 Message-ID: <7314ef76-9784-4663-b168-c2e71ce1ff8e@howitts.co.uk> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8035118427747505977==" List-Id: --===============8035118427747505977== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 25/03/2024 11:31, Michael Tremer wrote: >=20 > Hello, >=20 > I don=E2=80=99t think that the risk is very high, but I would account for t= his problem. >=20 > 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. >=20 >> On 25 Mar 2024, at 10:10, Nick Howitt wrote: >> >> 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: >> >> try: >> RequireUnboundReload =3D not filecmp.cmp(f.name, self.path, shallow=3DFals= e) >> except: >> RequireUnboundReload =3D True >=20 > I generally don=E2=80=99t like blanco exception catches. You will never kno= w about the daemons that you are hiding. >=20 >> This will force the file to be to be generated, even if it is only copying= in a blank temp file, and unbound will restart. It will only ever happen onc= e. >> >> >> 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 sty= le of development, so any guidance would be appreciated. >>> There is a bug, https://bugzilla.ipfire.org/show_bug.cgi?id=3D13254, wher= e unbound-dhcp-leases-bridge restarts unbound a lot when it is totally unnece= ssary. It appears to be because when a lease is renewed, the script gets trig= gered, created a new /etc/unbound/dhcp-leases.conf then restarts Unbound to r= ead the file. Generally this seems to be unnecessary as, with a renewed lease= , the old and new /etc/unbound/dhcp-leases.conf files are the same. With 5 le= ases in that file (one for a machine not active) I am getting 3-4 restarts an= 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 is= doing a file comparison before the copy and skipping the restart if the file= s are the same. >>> There were a couple of gotchas because setting the file attributes and co= pying the file were done inside the "with" block for generating the temporary= file. This meant a file comparison always returned False as the temp file wa= s still open and so never the same is the old file. >>> I moved those two statements outside the "with". This forced me to change= the fchmod to chmod. >>> It could be argued that the file copy should not be done if the files are= not different, but it would take an extra "if" and you'd have to 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 dhcp-leases.= conf files could occasionally contain the same leases but in a different orde= r. I have been unable to reproduce, but to sidestep it, instead of stepping t= hrough the leases variable directly, I sort it and step through that. It shou= ld make the resulting file completely deterministic and make the file compari= son 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/u= nbound-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_IRGRP= |stat.S_IROTH) >>> + filecmp.clear_cache() >>> + RequireUnboundReload =3D not filecmp.cmp(f.name, self.path, shal= low=3DFalse) >>> + >>> + # Make file readable for everyone >>> + os.chmod(f.name, stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_I= ROTH) >>> + >>> + # 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"] >=20 Resubmitting the patch. Following my previous reply, I would like to stick with filecmp as I am=20 using it with "shallow=3DFalse" to avoid doing the comparison just with=20 stat() calls. Note that I also flush the filecmp cache before using the=20 command so it is running with the latest file data. Talking about file=20 descriptors, atomic file replacement etc., scares me. I have modified the earlier try/except I proposed to catch only=20 FileNotFoundError, and I have changed CamelCase to snake_case. --- config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/config/unbound/unbound-dhcp-leases-bridge=20 b/config/unbound/unbound-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): 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(),=20 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,=20 shallow=3DFalse) + except FileNotFoundError: + require_unbound_reload =3D True + + # 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 require_unbound_reload def _control(self, *args): command =3D ["unbound-control"] --=20 2.43.0 Nick --===============8035118427747505977==--