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: Thu, 28 Mar 2024 10:41:48 +0000 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7457607662660441117==" List-Id: --===============7457607662660441117== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 27 Mar 2024, at 15:28, Nick Howitt wrote: >=20 >=20 >=20 > On 26/03/2024 11:10, Michael Tremer wrote: >>=20 >>=20 >>> 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 fo= r this problem. >>>> With my proposed changes from my previous email this should not be a lar= ge 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 ope= n the 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 exis= t, 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=3DF= alse) >>>>> 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 copy= ing 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 = style of development, so any guidance would be appreciated. >>>>>> There is a bug, https://bugzilla.ipfire.org/show_bug.cgi?id=3D13254, w= here unbound-dhcp-leases-bridge restarts unbound a lot when it is totally unn= ecessary. It appears to be because when a lease is renewed, the script gets t= riggered, created a new /etc/unbound/dhcp-leases.conf then restarts Unbound t= o read the file. Generally this seems to be unnecessary as, with a renewed le= ase, the old and new /etc/unbound/dhcp-leases.conf files are the same. With 5= leases 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 create= s 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 f= iles are the same. >>>>>> There were a couple of gotchas because setting the file attributes and= copying the file were done inside the "with" block for generating the tempor= ary 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 cha= nge 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-leas= es.conf files could occasionally contain the same leases but in a different o= rder. I have been unable to reproduce, but to sidestep it, instead of steppin= g through the leases variable directly, I sort it and step through that. It s= hould make the resulting file completely deterministic and make the file comp= arison 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 unbou= nd every >>>>>> 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/unboun= d/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_IR= GRP|stat.S_IROTH) >>>>>> + filecmp.clear_cache() >>>>>> + RequireUnboundReload =3D not filecmp.cmp(f.name, self.path, s= hallow=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 u= sing it with "shallow=3DFalse" to avoid doing the comparison just with stat()= calls. 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, atom= ic file replacement etc., scares me. >> File descriptors are great. They avoid race-conditions and all sorts of th= ings. > But I have never used them so I don't intend to go that way. Too much for m= e to get my brain round and a limited ability to test. Well, I pretty much gave you the entire solution=E2=80=A6 There was nothing l= eft to do apart from copying and pasting the snippets. >> I don=E2=80=99t think that module is the best choice really if you have to= hack around and flush the cache. Why would it even cache things in the first= place? I don=E2=80=99t see where the cache helps. > From the filecmp.clear_cache documentation: >=20 > Clear the filecmp cache. This may be useful if a file is compared so > quickly after it is modified that it is within the mtime resolution > of the underlying filesystem. >=20 > Now this makes me believe that the clear_cache is only relevant when using = shallow=3DTrue, which I'm not and I don't know how to check the underlying co= de. It is used to make sure the stat() info is updated, but we are not using = the stat() info. I guess we could as easily add a one second sleep instead as= that is way longer than the mtime resolution. The function is here and it is rather simple: https://github.com/python/cpyth= on/blob/3.12/Lib/filecmp.py#L30 It will always stat() both files and compares three items in case shallow is = False: The type of the file, the size and mtime. If the file is not a regular= file, the function will exist rather early. So in our case, that is all work= the function is doing for thing. It boils down to something very similar to = what I suggested to use to compare the files. On top of that, there is a cache which will always be used but will never ret= urn a match for us because the temporary file at least will always have anoth= er mtime. You could clear the cache after every call, but I would rather not have our c= ode messy and care about implementation details of some Python module. > The module is chosen because all the references I read about comparing file= s preferred this one-liner for comparing files, although other methods were g= iven and there were lots of discussions about large files (GB+) because if yo= u chomp them it may be much quicker to say they are not the same when you fin= d the first block that does not match. Having said this, I don't know how fil= ecmp works, but we only have a small file. At the same time, I am not a regul= ar programmer so simple one-liners are good for me. You would always want to read both files block for block and compare the bloc= ks. There is no way to implement this significantly faster. >>> I have modified the earlier try/except I proposed to catch only FileNotFo= undError, 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/u= nbound-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_IRO= TH) >>> + filecmp.clear_cache() >>> + try: >>> + require_unbound_reload =3D not filecmp.cmp(f.name, self.path, shallow= =3DFalse) >>> + except FileNotFoundError: >>> + require_unbound_reload =3D True >> If you want to go this route, you will have to flush the temporary file fi= rst. Otherwise you might have some data left in the buffer that has not been = written (because the file is still open). You will then compare incomplete ou= tput. This would not become a problem if you used the file descriptors. > The temp file is closed at the point of doing the filecmp, and the filecmp = cache has been flushed. How do I flush the temporary file further. Reading up= , is it just having the last statement in the with block as f.flush() with th= e same indent as "for l in sorted(leases):" If you have left the with block you will already have flushed the buffers and= closed the file. It looks like I misread the indentation in the patch then. >>=20 >> Catching FileNotFound would work for me. > Good >>=20 >>> + >>> + # 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 --===============7457607662660441117==--