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, 13 May 2024 11:27:14 +0100 Message-ID: In-Reply-To: <72D03FC7-620E-459E-B3F6-1D9B28CB96B2@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0283921510682190184==" List-Id: --===============0283921510682190184== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Just for reference, yes: https://bugzilla.ipfire.org/show_bug.cgi?id=3D13254#c134 > On 4 May 2024, at 03:16, jon wrote: >=20 >=20 >> To be honest, I kind of lost track where we are with that now.=20 >>=20 >=20 > Michael, >=20 > If you are referring to my question: Are you OK with moving forward using t= he DCHP on-commit, on-release, on-expiry?? (like the proof-of-concept) >=20 >=20 > Jon >=20 >=20 >> On Apr 30, 2024, at 7:01=E2=80=AFAM, Michael Tremer wrote: >>=20 >> Hello Nick, >>=20 >>> On 30 Apr 2024, at 12:58, Nick Howitt wrote: >>>=20 >>>=20 >>>=20 >>> On 27/04/2024 10:53, Nick Howitt wrote: >>>>=20 >>>>=20 >>>> On 27/04/2024 09:35, Michael Tremer wrote: >>>>> Hello Nick, >>>>>=20 >>>>>=20 >>>>>> On 26 Apr 2024, at 17:48, Nick Howitt wrote: >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>> On 26/04/2024 16:12, Michael Tremer wrote: >>>>>>=20 >>>>>>> Hello Nick, >>>>>>>=20 >>>>>>>=20 >>>>>>>=20 >>>>>>>> On 25 Apr 2024, at 13:43, Nick Howitt wrote: >>>>>>>>=20 >>>>>>>> Please can I bump this again to request a review of my patch submitt= ed on 28th March? Thanks, Nick. >>>>>>>>=20 >>>>>>>>=20 >>>>>>> It is a little bit difficult to test your proposed changes if testers= have to copy and paste them out of an email. >>>>>>>=20 >>>>>> Confused. I was told I had to submit the diff in an email with [PATCH]= in the title and not to attach a file. Please can you tell me how I should b= e operating? >>>>>>=20 >>>>> No worries, we are going to sort this out. >>>>>=20 >>>>> So the first email in the thread was a correct patch. It was just the p= atch in the format that Git uses and it was properly parsed by Patchwork. Lat= er in the same thread, the patch was just copied and pasted which was impossi= ble for Patchwork to pick up. There is nothing wrong with using bits of code = in the conversation, but it would have been nice to have the final version su= bmitted once again. >>>>>=20 >>>>>=20 >>>>>>> I have just submitted a patch set to this list that breaks down all t= he changes that you proposed and I cleaned up the code so that we can use the= filecmp function without any extra steps. We can simply jump out of the writ= e function and return False to prevent Unbound being reloaded. >>>>>>>=20 >>>>>> I thought you didn't want me to use filecmp and pushed me to use your = file compare routine. >>>>>>=20 >>>>> Yeah, I would have preferred that too, but I thought using the module m= ight be quicker now. >>>>>=20 >>>>>=20 >>>>>>> We will also avoid replacing the file all the time as that causes lot= s of unnecessary disk I/O. I am not saying it makes a significant difference,= but flash has a limited write count that we are still wasting. >>>>>>>=20 >>>>>> OK, but my changes were no worse than before, either doing a delete or= rename whereas before it always did a rename. The "unnecessary I/O" was alre= ady there. >>>>>>=20 >>>>>>> Please review and you if you are happy, please add the appropriate Gi= t tag. >>>>>>>=20 >>>>>> Sorry but I don't know what you mean by "adding the appropriate Git ta= g". I am only using Git locally and have no repo access. >>>>>>=20 >>>>> https://www.ipfire.org/docs/devel/git/tags >>>=20 >>> I tested the patches yesterday and today and I am happy with them. I hope= I have signed them off correctly. >>=20 >> Perfect. Thank you very much. >>=20 >>> FWIW I hand patched the program as it was easier than learning something = new in this case as the patch set is so small. >>=20 >> Haha, okay :) >>=20 >>> One thing not mentioned in the bug report. Not only is unbound restarting= on renewals which this fixes, it also restarts on new leases **and** expirin= g leases. >>=20 >> I think this is the correct way though, but I agree it creates extra work. >>=20 >>> For big work environments, I would have thought the best workaround is to= set the default DHCP lease to 24h. It would help during the week to lessen t= he restarts but not on Monday mornings. It may also need a bigger address poo= l to cope with devices randomising MAC addresses. >>=20 >> It depends on where you deploy this. In any office/home environment 24h is= probably a good lease time. Maybe even 12 or 10 is a good option. I have rar= ely seen lease times of a week or so recently; in fact the default seems to b= e closer to an hour. That should however not be a problem for us any more bec= ause we don=E2=80=99t reload Unbound if a client simply renews their lease. O= nly if the join or leave the network.=20 >>=20 >>> Roll on Jon's fix as it would get round all these restarts. I use it modi= fied not to update the dhcp-leases.conf. and another couple of tweaks. Are yo= u able to test it, but it does mean piecing it together? >>=20 >> To be honest, I kind of lost track where we are with that now. I did look = at a couple of the references from your last email, but didn=E2=80=99t make i= t very far, yet. >>=20 >> -Michael >>=20 >>>> Thanks. >>>>>> How can I pull your changes? Otherwise I'll need to patch my set up ma= nually. >>>>>>=20 >>>>> You can apply the patch to your local repository using =E2=80=9Cgit am= =E2=80=9D: >>>>>=20 >>>>> https://git-scm.com/docs/git-am >>>> Thanks again. >>>>>> I also have another problem. I have temporarily had to remove my IPF g= ateway for a business reason and don't know when I'll be able to add it back = in. I'll have to see if I can insert it in my LAN somehow. At the same time i= t is also running Jon's mods which I'll have to undo. >>>>>>=20 >>>>>> With respect to Jon's mods, I've asked a couple of times on bugzilla, = but is there any point in his routine maintaining /etc/unbound/dhcp-leases.co= nf. As far as I see the file is only read once when unbound starts, so it onl= y has to be valid when unbound starts and not at any other point in time. To = this effect he can remove the file maintenance and I attached a modified copy= unbound-dhcp-leases-bridge to his repo which generates a new dhcp-leases.con= f and exits, removing all the daemonising bit and the inode watcher. This fil= e can be run as part of the unbound init routine just before the line that ac= tually starts or reloads unbound. It also has the advantage of cutting down a= load of disk I/O, which is important as you've just pointed out. >>>>>>=20 >>>>> I don=E2=80=99t think that we can fully remove the daemon part, because= I don=E2=80=99t think that there is a way to get a script (shell or otherwis= e) fast enough to perform the job. I also think that we benefit a lot from ke= eping some state in memory, whereas the script will have to parse the old fil= e first, generate a new file and then compare the result. That will take a lo= ng time. >>>> I had Jon's script down to 70ms after I removed the dhcp-leases file upd= ates, used my fast method for deriving the domain name and removed Jon's init= ial setting up bit which is not needed once the changes are accepted. This is= on a 2c VM running at 2GHz. I thought the goal was < 100ms which has been do= ne comfortably. >>>>=20 >>>> FWIW, if you no longer maintain dhcp-leases in the Jon's script you neve= r read the old file or do a compare. All you do is read the hosts and static-= leases files and nothing is written to file. >>>>> Do we needed the leases file generated at startup? Yes, I think so, too= . DNS is quite essential for the network and for local services to be able to= start. So it has to come up very early. If it comes up with proper configura= tion we avoid any other problems where hosts might resolve names too early an= d get a different result compared to a couple of seconds later when the zone = is fully initialised. >>>> My cut-down version of unbound-dhcp-leases-bridge is at https://bugzilla= .ipfire.org/attachment.cgi?id=3D1516 on https://bugzilla.ipfire.org/show_bug.= cgi?id=3D13254. On my test system it takes 219ms to run so delays unbound sta= rting up minimally. >>>>=20 >>>> At the same time there is a gain in the dhcp start up as the unbound-dhc= p-leases-bridge is no longer needed and it therefore avoids triggering an unb= ound restart which happens every time unbound-dhcp-leases-bridge is run. >>>>=20 >>>> If we are generating dhcp-leases.conf at start up, then there is no poin= t in maintaining it while unbound is running as the maintained file will neve= r be read. >>>>> So all in all, I think the route I would most likely look at is changin= g the daemon to open a socket. We write minimal scripts which are called by d= hcpd that simply pipe their data into the socket which the daemon will then p= arse. With all available leases in memory already, we can check if a lease ha= s changed or if it has been renewed without any further changes. If nothing h= as changed, we don=E2=80=99t need to do anything. In case of a change, we wil= l have to write the file again and have Unbound reload, or we even call unbou= nd-control and make the one change without a reload. Doing that in the daemon= should give us more flexibility and we avoid any performance penalties as th= e daemon could run multiple commands concurrently which dhcpd can=E2=80=99t d= o. >>>>>=20 >>>> Sockets are beyond me, I am afraid. >>>>=20 >>>> Part of the goal of all this is to avoid reloading unbound and part of m= y goal is to avoid writing dhcp-leases.conf unless unbound is restarted. I am= not sure why you are still mentioning writing a file and restarting/reloadin= g unbound. >>>>>> Also on his patches, I had a significantly faster way of determining t= he domain name (in bash) and he and I were looking for your feedback on if we= 'd be allowed to use it. >>>>>>=20 >>>>> Anything is allowed. It just has to be fast. And for that, I believe, c= alling any other binary will be a problem. >>>>>=20 >>>> OK. I was proposing to read the DOMAINNAME with: >>>> DOMAINNAME=3D${HOSTNAME#*.*} >>>> which is many times faster than the conventional route: >>>> eval $(/usr/local/bin/readhash /var/ipfire/main/settings) >>>> If there is any concern with bash not having the full environment when t= he program is called, the shebang could be changed to: >>>> #!/bin/bash -l >>>> Nick >>>>> -Michael >>>>>=20 >>>>>=20 >>>>>> Regards, >>>>>>=20 >>>>>> Nick >>>>>>=20 >>>>>>> Best, >>>>>>> -Michael >>>>>>>=20 >>>>>>>=20 >>>>>>>=20 >>>>>>>> On 10/04/2024 12:04, Nick Howitt wrote: >>>>>>>>=20 >>>>>>>>=20 >>>>>>>>> Bump >>>>>>>>>=20 >>>>>>>>> On 28/03/2024 17:25, Nick Howitt wrote: >>>>>>>>>=20 >>>>>>>>>=20 >>>>>>>>>> Next version.=20 >>>>>>>>>>=20 >>>>>>>>>> On 28/03/2024 10:41, Michael Tremer wrote: >>>>>>>>>>=20 >>>>>>>>>>=20 >>>>>>>>>>> Hello,=20 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>> On 27 Mar 2024, at 15:28, Nick Howitt wro= te:=20 >>>>>>>>>>>>=20 >>>>>>>>>>>>=20 >>>>>>>>>>>>=20 >>>>>>>>>>>> On 26/03/2024 11:10, Michael Tremer wrote: >>>>>>>>>>>>=20 >>>>>>>>>>>>=20 >>>>>>>>>>>>>> On 25 Mar 2024, at 13:11, Nick Howitt w= rote:=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> On 25/03/2024 11:31, Michael Tremer wrote: >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> Hello,=20 >>>>>>>>>>>>>>> I don=E2=80=99t think that the risk is very high, but I would= account for this problem.=20 >>>>>>>>>>>>>>> With my proposed changes from my previous email this should n= ot 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 compariso= n and open the file again with =E2=80=9Cw=E2=80=9D. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>> On 25 Mar 2024, at 10:10, Nick Howitt = wrote:=20 >>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>> If there is a risk that the /etc/unbound/dhcp-leases.conf do= es 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 >>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>> try:=20 >>>>>>>>>>>>>>>> RequireUnboundReload =3D not filecmp.cmp(f.name, self.path, = shallow=3DFalse)=20 >>>>>>>>>>>>>>>> except:=20 >>>>>>>>>>>>>>>> RequireUnboundReload =3D True >>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>> I generally don=E2=80=99t like blanco exception catches. You = will never know about the daemons that you are hiding. >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>> This will force the file to be to be generated, even if it i= s only copying in a blank temp file, and unbound will restart. It will only e= ver happen once.=20 >>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>> On 24/03/2024 15:49, Nick Howitt wrote: >>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>>> Hi all,=20 >>>>>>>>>>>>>>>>> **** This is a repost of an email incorrectly submitted. **= **=20 >>>>>>>>>>>>>>>>> Please bear with me as I am new to IPFire and not at all us= ed to this style of development, so any guidance would be appreciated.=20 >>>>>>>>>>>>>>>>> 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 unnecessary. It appears to be because when a lease is renewed, the sc= ript gets triggered, created a new /etc/unbound/dhcp-leases.conf then restart= s Unbound to read 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 s= ame. 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.= =20 >>>>>>>>>>>>>>>>> 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 resta= rt if the files are the same.=20 >>>>>>>>>>>>>>>>> There were a couple of gotchas because setting the file att= ributes and copying the file were done inside the "with" block for generating= the temporary file. This meant a file comparison always returned False as th= e temp file was still open and so never the same is the old file.=20 >>>>>>>>>>>>>>>>> I moved those two statements outside the "with". This force= d me to change the fchmod to chmod.=20 >>>>>>>>>>>>>>>>> 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.=20 >>>>>>>>>>>>>>>>> Also, one small thing I noticed once is that the old and ne= w dhcp-leases.conf 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 through the leases variable directly, I sort it and step through= that. It should make the resulting file completely deterministic and make th= e file comparison more effective.=20 >>>>>>>>>>>>>>>>> My patch is: >>>>>>>>>>>>>>>>>> From 73873d4944944a2f02317a73074a6894726f36f7 Mon Sep 17 0= 0:00:00 2001 >>>>>>>>>>>>>>>>> From: Nick Howitt =20 >>>>>>>>>>>>>>>>> Date: Sun, 24 Mar 2024 15:17:19 +0000=20 >>>>>>>>>>>>>>>>> Subject: [PATCH] Stop unbound-dhcp-leases-bridge from resta= rting unbound every=20 >>>>>>>>>>>>>>>>> time it write dhcp-leases.conf as it is very often unchange= d and does not=20 >>>>>>>>>>>>>>>>> require a restart.=20 >>>>>>>>>>>>>>>>> ---=20 >>>>>>>>>>>>>>>>> config/unbound/unbound-dhcp-leases-bridge | 28 ++++++++++++= +++--------=20 >>>>>>>>>>>>>>>>> 1 file changed, 19 insertions(+), 9 deletions(-)=20 >>>>>>>>>>>>>>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/co= nfig/unbound/unbound-dhcp-leases-bridge=20 >>>>>>>>>>>>>>>>> index e9f022aff..d22772066 100644=20 >>>>>>>>>>>>>>>>> --- a/config/unbound/unbound-dhcp-leases-bridge=20 >>>>>>>>>>>>>>>>> +++ b/config/unbound/unbound-dhcp-leases-bridge=20 >>>>>>>>>>>>>>>>> @@ -22,6 +22,7 @@=20 >>>>>>>>>>>>>>>>> import argparse=20 >>>>>>>>>>>>>>>>> import datetime=20 >>>>>>>>>>>>>>>>> import daemon=20 >>>>>>>>>>>>>>>>> +import filecmp=20 >>>>>>>>>>>>>>>>> import functools=20 >>>>>>>>>>>>>>>>> import ipaddress=20 >>>>>>>>>>>>>>>>> import logging=20 >>>>>>>>>>>>>>>>> @@ -516,26 +517,35 @@ class UnboundConfigWriter(object):=20 >>>>>>>>>>>>>>>>> def update_dhcp_leases(self, leases):=20 >>>>>>>>>>>>>>>>> # Write out all leases=20 >>>>>>>>>>>>>>>>> - self.write_dhcp_leases(leases)=20 >>>>>>>>>>>>>>>>> + if self.write_dhcp_leases(leases):=20 >>>>>>>>>>>>>>>>> - log.debug("Reloading Unbound...")=20 >>>>>>>>>>>>>>>>> + log.debug("Reloading Unbound...")=20 >>>>>>>>>>>>>>>>> - # Reload the configuration without dropping the cache=20 >>>>>>>>>>>>>>>>> - self._control("reload_keep_cache")=20 >>>>>>>>>>>>>>>>> + # Reload the configuration without dropping the cache=20 >>>>>>>>>>>>>>>>> + self._control("reload_keep_cache")=20 >>>>>>>>>>>>>>>>> +=20 >>>>>>>>>>>>>>>>> + else:=20 >>>>>>>>>>>>>>>>> +=20 >>>>>>>>>>>>>>>>> + log.debug("Not reloading Unbound. Leases not changed.")=20 >>>>>>>>>>>>>>>>> def write_dhcp_leases(self, leases):=20 >>>>>>>>>>>>>>>>> log.debug("Writing DHCP leases...")=20 >>>>>>>>>>>>>>>>> with tempfile.NamedTemporaryFile(mode=3D"w", delete=3DFalse= ) as f:=20 >>>>>>>>>>>>>>>>> - for l in leases:=20 >>>>>>>>>>>>>>>>> + for l in sorted(leases):=20 >>>>>>>>>>>>>>>>> for rr in l.rrset:=20 >>>>>>>>>>>>>>>>> f.write("local-data: \"%s\"\n" % " ".join(rr))=20 >>>>>>>>>>>>>>>>> - # Make file readable for everyone=20 >>>>>>>>>>>>>>>>> - os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IR= GRP|stat.S_IROTH)=20 >>>>>>>>>>>>>>>>> + filecmp.clear_cache()=20 >>>>>>>>>>>>>>>>> + RequireUnboundReload =3D not filecmp.cmp(f.name, self.pat= h, shallow=3DFalse)=20 >>>>>>>>>>>>>>>>> +=20 >>>>>>>>>>>>>>>>> + # Make file readable for everyone=20 >>>>>>>>>>>>>>>>> + os.chmod(f.name, stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|s= tat.S_IROTH)=20 >>>>>>>>>>>>>>>>> +=20 >>>>>>>>>>>>>>>>> + # Move the file to its destination=20 >>>>>>>>>>>>>>>>> + os.rename(f.name, self.path)=20 >>>>>>>>>>>>>>>>> - # Move the file to its destination=20 >>>>>>>>>>>>>>>>> - os.rename(f.name, self.path)=20 >>>>>>>>>>>>>>>>> + return RequireUnboundReload=20 >>>>>>>>>>>>>>>>> def _control(self, *args):=20 >>>>>>>>>>>>>>>>> command =3D ["unbound-control"] >>>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> Resubmitting the patch.=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> Following my previous reply, I would like to stick with filecm= p as I am using 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 descri= ptors, atomic file replacement etc., scares me. >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>> File descriptors are great. They avoid race-conditions and all = sorts of things. >>>>>>>>>>>>>=20 >>>>>>>>>>>>>=20 >>>>>>>>>>>> But I have never used them so I don't intend to go that way. Too= much for me to get my brain round and a limited ability to test. >>>>>>>>>>>>=20 >>>>>>>>>>>>=20 >>>>>>>>>>> Well, I pretty much gave you the entire solution=E2=80=A6 There w= as nothing left to do apart from copying and pasting the snippets.=20 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>> Ok I've tried to use your version but struggled to get it going. I= f I used it as was and called compare(file1_name, file2_name) it failed as "c= ompare" was not defined.=20 >>>>>>>>>> I could call it using self.compare but then it failed with "TypeEr= ror: UnboundConfigWriter.compare() takes 2 positional arguments but 3 were gi= ven". I changed the function to compare(self, f1, f2).=20 >>>>>>>>>> Then it failed with "AttributeError: 'str' object has no attribute= 'seek'", so I changed it again to accept file names then open the files befo= re doing the seeks.=20 >>>>>>>>>> I really hope it is OK as I am out of my depth when defining funct= ions with "self" as an argument. Please can you check it? If it is not OK, ca= n I revert to filecmp?=20 >>>>>>>>>>=20 >>>>>>>>>> I have made one futher change, the sorted(leases) line was not wor= king and not sorting the object ever, and I saw lines changing places in the = resulting leases file. I have changed it to "sorted(leases, key=3Dlambda x: x= .ipaddr)". It definitely sorts this time.=20 >>>>>>>>>>=20 >>>>>>>>>>=20 >>>>>>>>>>=20 >>>>>>>>>>>>> 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 i= n the first place? I don=E2=80=99t see where the cache helps. >>>>>>>>>>>>>=20 >>>>>>>>>>>>>=20 >>>>>>>>>>>> From the filecmp.clear_cache documentation:=20 >>>>>>>>>>>>=20 >>>>>>>>>>>> Clear the filecmp cache. This may be useful if a file is compare= d so=20 >>>>>>>>>>>> quickly after it is modified that it is within the mtime resolut= ion=20 >>>>>>>>>>>> of the underlying filesystem.=20 >>>>>>>>>>>>=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 un= derlying code. 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. >>>>>>>>>>>>=20 >>>>>>>>>>>>=20 >>>>>>>>>>> The function is here and it is rather simple: https://github.com/= python/cpython/blob/3.12/Lib/filecmp.py#L30=20 >>>>>>>>>>>=20 >>>>>>>>>>> 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 n= ot 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.=20 >>>>>>>>>>>=20 >>>>>>>>>>> On top of that, there is a cache which will always be used but wi= ll never return a match for us because the temporary file at least will alway= s have another mtime.=20 >>>>>>>>>>>=20 >>>>>>>>>>> You could clear the cache after every call, but I would rather no= t have our code messy and care about implementation details of some Python mo= dule.=20 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>> The module is chosen because all the references I read about com= paring files preferred this one-liner for comparing files, although other met= hods were given and there were lots of discussions about large files (GB+) be= cause if you chomp them it may be much quicker to say they are not the same w= hen you find the first block that does not match. Having said this, I don't k= now how filecmp works, but we only have a small file. At the same time, I am = not a regular programmer so simple one-liners are good for me. >>>>>>>>>>>>=20 >>>>>>>>>>>>=20 >>>>>>>>>>> You would always want to read both files block for block and comp= are the blocks. There is no way to implement this significantly faster.=20 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>>>> I have modified the earlier try/except I proposed to catch onl= y FileNotFoundError, and I have changed CamelCase to snake_case.=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> ---=20 >>>>>>>>>>>>>> config/unbound/unbound-dhcp-leases-bridge | 31 +++++++++++++++= +-------=20 >>>>>>>>>>>>>> 1 file changed, 22 insertions(+), 9 deletions(-)=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/confi= g/unbound/unbound-dhcp-leases-bridge=20 >>>>>>>>>>>>>> index e9f022aff..a9ec8e1e2 100644=20 >>>>>>>>>>>>>> --- a/config/unbound/unbound-dhcp-leases-bridge=20 >>>>>>>>>>>>>> +++ b/config/unbound/unbound-dhcp-leases-bridge=20 >>>>>>>>>>>>>> @@ -22,6 +22,7 @@=20 >>>>>>>>>>>>>> import argparse=20 >>>>>>>>>>>>>> import datetime=20 >>>>>>>>>>>>>> import daemon=20 >>>>>>>>>>>>>> +import filecmp=20 >>>>>>>>>>>>>> import functools=20 >>>>>>>>>>>>>> import ipaddress=20 >>>>>>>>>>>>>> import logging=20 >>>>>>>>>>>>>> @@ -516,26 +517,38 @@ class UnboundConfigWriter(object):=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> def update_dhcp_leases(self, leases):=20 >>>>>>>>>>>>>> # Write out all leases=20 >>>>>>>>>>>>>> - self.write_dhcp_leases(leases)=20 >>>>>>>>>>>>>> + if self.write_dhcp_leases(leases):=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> - log.debug("Reloading Unbound...")=20 >>>>>>>>>>>>>> + log.debug("Reloading Unbound...")=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> - # Reload the configuration without dropping the cache=20 >>>>>>>>>>>>>> - self._control("reload_keep_cache")=20 >>>>>>>>>>>>>> + # Reload the configuration without dropping the cache=20 >>>>>>>>>>>>>> + self._control("reload_keep_cache")=20 >>>>>>>>>>>>>> +=20 >>>>>>>>>>>>>> + else:=20 >>>>>>>>>>>>>> +=20 >>>>>>>>>>>>>> + log.debug("Not reloading Unbound. Leases not changed.")=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> def write_dhcp_leases(self, leases):=20 >>>>>>>>>>>>>> log.debug("Writing DHCP leases...")=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> with tempfile.NamedTemporaryFile(mode=3D"w", delete=3DFalse) a= s f:=20 >>>>>>>>>>>>>> - for l in leases:=20 >>>>>>>>>>>>>> + for l in sorted(leases):=20 >>>>>>>>>>>>>> for rr in l.rrset:=20 >>>>>>>>>>>>>> f.write("local-data: \"%s\"\n" % " ".join(rr))=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> - # Make file readable for everyone=20 >>>>>>>>>>>>>> - os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP= |stat.S_IROTH)=20 >>>>>>>>>>>>>> + filecmp.clear_cache()=20 >>>>>>>>>>>>>> + try:=20 >>>>>>>>>>>>>> + require_unbound_reload =3D not filecmp.cmp(f.name, self.path= , shallow=3DFalse)=20 >>>>>>>>>>>>>> + except FileNotFoundError:=20 >>>>>>>>>>>>>> + require_unbound_reload =3D True >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>> If you want to go this route, you will have to flush the tempor= ary file first. Otherwise you might have some data left in the buffer that ha= s not been written (because the file is still open). You will then compare in= complete output. This would not become a problem if you used the file descrip= tors. >>>>>>>>>>>>>=20 >>>>>>>>>>>>>=20 >>>>>>>>>>>> The temp file is closed at the point of doing the filecmp, and t= he 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.flus= h() with the same indent as "for l in sorted(leases):" >>>>>>>>>>>>=20 >>>>>>>>>>>>=20 >>>>>>>>>>> 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 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>=20 >>>>>>>>>>>>> Catching FileNotFound would work for me. >>>>>>>>>>>>>=20 >>>>>>>>>>>>>=20 >>>>>>>>>>>> Good >>>>>>>>>>>>=20 >>>>>>>>>>>>=20 >>>>>>>>>>>>>> +=20 >>>>>>>>>>>>>> + # Make file readable for everyone=20 >>>>>>>>>>>>>> + os.chmod(f.name, stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat= .S_IROTH)=20 >>>>>>>>>>>>>> +=20 >>>>>>>>>>>>>> + # Move the file to its destination=20 >>>>>>>>>>>>>> + os.rename(f.name, self.path)=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> - # Move the file to its destination=20 >>>>>>>>>>>>>> - os.rename(f.name, self.path)=20 >>>>>>>>>>>>>> + return require_unbound_reload=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> def _control(self, *args):=20 >>>>>>>>>>>>>> command =3D ["unbound-control"]=20 >>>>>>>>>>>>>> --=20 >>>>>>>>>>>>>> 2.43.0=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>> Nick >>>>>>>>>>>>>>=20 >>>>>>>>>>>>>>=20 >>>>>>>>>> ---=20 >>>>>>>>>> config/unbound/unbound-dhcp-leases-bridge | 50 +++++++++++++++++++= ----=20 >>>>>>>>>> 1 file changed, 41 insertions(+), 9 deletions(-)=20 >>>>>>>>>>=20 >>>>>>>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/un= bound/unbound-dhcp-leases-bridge=20 >>>>>>>>>> index e9f022aff..80d9523ea 100644=20 >>>>>>>>>> --- a/config/unbound/unbound-dhcp-leases-bridge=20 >>>>>>>>>> +++ b/config/unbound/unbound-dhcp-leases-bridge=20 >>>>>>>>>> @@ -516,26 +516,37 @@ class UnboundConfigWriter(object):=20 >>>>>>>>>>=20 >>>>>>>>>> def update_dhcp_leases(self, leases):=20 >>>>>>>>>> # Write out all leases=20 >>>>>>>>>> - self.write_dhcp_leases(leases)=20 >>>>>>>>>> + if self.write_dhcp_leases(leases):=20 >>>>>>>>>>=20 >>>>>>>>>> - log.debug("Reloading Unbound...")=20 >>>>>>>>>> + log.debug("Reloading Unbound...")=20 >>>>>>>>>>=20 >>>>>>>>>> - # Reload the configuration without dropping the cache=20 >>>>>>>>>> - self._control("reload_keep_cache")=20 >>>>>>>>>> + # Reload the configuration without dropping the cache=20 >>>>>>>>>> + self._control("reload_keep_cache")=20 >>>>>>>>>> +=20 >>>>>>>>>> + else:=20 >>>>>>>>>> +=20 >>>>>>>>>> + log.debug("Not reloading Unbound. Leases not changed.")=20 >>>>>>>>>>=20 >>>>>>>>>> def write_dhcp_leases(self, leases):=20 >>>>>>>>>> log.debug("Writing DHCP leases...")=20 >>>>>>>>>>=20 >>>>>>>>>> with tempfile.NamedTemporaryFile(mode=3D"w", delete=3DFalse) as f:= =20 >>>>>>>>>> - for l in leases:=20 >>>>>>>>>> + for l in sorted(leases, key=3Dlambda x: x.ipaddr):=20 >>>>>>>>>> for rr in l.rrset:=20 >>>>>>>>>> f.write("local-data: \"%s\"\n" % " ".join(rr))=20 >>>>>>>>>>=20 >>>>>>>>>> - # Make file readable for everyone=20 >>>>>>>>>> - os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|sta= t.S_IROTH)=20 >>>>>>>>>> + try:=20 >>>>>>>>>> + require_unbound_reload =3D not self.compare(f.name, self.path)=20 >>>>>>>>>> + except FileNotFoundError:=20 >>>>>>>>>> + require_unbound_reload =3D True=20 >>>>>>>>>> +=20 >>>>>>>>>> + # Make file readable for everyone=20 >>>>>>>>>> + os.chmod(f.name, stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_I= ROTH)=20 >>>>>>>>>> +=20 >>>>>>>>>> + # Move the file to its destination=20 >>>>>>>>>> + os.rename(f.name, self.path)=20 >>>>>>>>>>=20 >>>>>>>>>> - # Move the file to its destination=20 >>>>>>>>>> - os.rename(f.name, self.path)=20 >>>>>>>>>> + return require_unbound_reload=20 >>>>>>>>>>=20 >>>>>>>>>> def _control(self, *args):=20 >>>>>>>>>> command =3D ["unbound-control"]=20 >>>>>>>>>> @@ -551,6 +562,27 @@ class UnboundConfigWriter(object):=20 >>>>>>>>>>=20 >>>>>>>>>> raise e=20 >>>>>>>>>>=20 >>>>>>>>>> + def compare(self, if1, if2):=20 >>>>>>>>>> + buffersize =3D 16 * 1024=20 >>>>>>>>>> +=20 >>>>>>>>>> + f1 =3D open(if1, "r")=20 >>>>>>>>>> + f2 =3D open(if2, "r")=20 >>>>>>>>>> +=20 >>>>>>>>>> + # Reset file handles=20 >>>>>>>>>> + f1.seek(0)=20 >>>>>>>>>> + f2.seek(0)=20 >>>>>>>>>> +=20 >>>>>>>>>> + while True:=20 >>>>>>>>>> + b1 =3D f1.read(buffersize)=20 >>>>>>>>>> + b2 =3D f2.read(buffersize)=20 >>>>>>>>>> +=20 >>>>>>>>>> + if not b1 =3D=3D b2:=20 >>>>>>>>>> + return False=20 >>>>>>>>>> +=20 >>>>>>>>>> + elif not b1 or not b2:=20 >>>>>>>>>> + break=20 >>>>>>>>>> +=20 >>>>>>>>>> + return True=20 >>>>>>>>>>=20 >>>>>>>>>> if __name__ =3D=3D "__main__":=20 >>>>>>>>>> parser =3D argparse.ArgumentParser(description=3D"Bridge for DHCP = Leases and Unbound DNS") >>>>>>>>>>=20 >>>>>>>>>>=20 >>>>=20 >>>=20 >>=20 >>=20 >=20 >=20 >=20 >=20 >=20 > Jon Murphy > jon.murphy(a)ipfire.org >=20 >=20 >=20 --===============0283921510682190184==--