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: Mon, 25 Mar 2024 11:31:04 +0000 Message-ID: In-Reply-To: <6b53833f-3527-4832-9561-ba3fe3a2ef23@howitts.co.uk> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4113352309897409743==" List-Id: --===============4113352309897409743== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, I don=E2=80=99t think that the risk is very high, but I would account for thi= s problem. With my proposed changes from my previous email this should not be a large pr= oblem 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 the f= ile 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, f= or example on a clean install (I don't know), the filecmp.cmp line needs to b= e wrapped in a try...except to stop it blowing up: >=20 > try: > RequireUnboundReload =3D not filecmp.cmp(f.name, self.path, shallow=3DFalse) > except: > RequireUnboundReload =3D True I generally don=E2=80=99t like blanco exception catches. You will never know = about the daemons that you are hiding. > 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 once. >=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 styl= e of development, so any guidance would be appreciated. >> 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 unneces= sary. It appears to be because when a lease is renewed, the script gets trigg= ered, created a new /etc/unbound/dhcp-leases.conf then restarts Unbound to re= ad 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 lea= ses 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 files= are the same. >> There were a couple of gotchas because setting the file attributes and cop= ying 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 was= 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.c= onf files could occasionally contain the same leases but in a different order= . I have been unable to reproduce, but to sidestep it, instead of stepping th= rough the leases variable directly, I sort it and step through that. It shoul= d make the resulting file completely deterministic and make the file comparis= on 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 e= very >> time it write dhcp-leases.conf as it is very often unchanged and does not >> 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/un= bound-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, shall= ow=3DFalse) >> + >> + # Make file readable for everyone >> + os.chmod(f.name, stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IR= OTH) >> + >> + # 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"] --===============4113352309897409743==--