Hello Nick,


On 26 Apr 2024, at 17:48, Nick Howitt <nick@howitts.co.uk> wrote:



On 26/04/2024 16:12, Michael Tremer wrote:

Hello Nick,



On 25 Apr 2024, at 13:43, Nick Howitt <nick@howitts.co.uk> wrote:

Please can I bump this again to request a review of my patch submitted on 28th March? Thanks, Nick.


It is a little bit difficult to test your proposed changes if testers have to copy and paste them out of an email.

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 be operating?

No worries, we are going to sort this out.

So the first email in the thread was a correct patch. It was just the patch in the format that Git uses and it was properly parsed by Patchwork. Later in the same thread, the patch was just copied and pasted which was impossible 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 submitted once again.


I have just submitted a patch set to this list that breaks down all the 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 write function and return False to prevent Unbound being reloaded.

I thought you didn't want me to use filecmp and pushed me to use your file compare routine.

Yeah, I would have preferred that too, but I thought using the module might be quicker now.


We will also avoid replacing the file all the time as that causes lots 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.

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 already there.

Please review and you if you are happy, please add the appropriate Git tag.

Sorry but I don't know what you mean by "adding the appropriate Git tag". I am only using Git locally and have no repo access.

  https://www.ipfire.org/docs/devel/git/tags
How can I pull your changes? Otherwise I'll need to patch my set up manually.

You can apply the patch to your local repository using “git am”:

  https://git-scm.com/docs/git-am
I also have another problem. I have temporarily had to remove my IPF gateway 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 it is also running Jon's mods which I'll have to undo.

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.conf. As far as I see the file is only read once when unbound starts, so it only 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.conf and exits, removing all the daemonising bit and the inode watcher. This file can be run as part of the unbound init routine just before the line that actually 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.

I don’t think that we can fully remove the daemon part, because I don’t think that there is a way to get a script (shell or otherwise) fast enough to perform the job. I also think that we benefit a lot from keeping some state in memory, whereas the script will have to parse the old file first, generate a new file and then compare the result. That will take a long time.
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 configuration we avoid any other problems where hosts might resolve names too early and get a different result compared to a couple of seconds later when the zone is fully initialised.
So all in all, I think the route I would most likely look at is changing the daemon to open a socket. We write minimal scripts which are called by dhcpd that simply pipe their data into the socket which the daemon will then parse. With all available leases in memory already, we can check if a lease has changed or if it has been renewed without any further changes. If nothing has changed, we don’t need to do anything. In case of a change, we will have to write the file again and have Unbound reload, or we even call unbound-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 the daemon could run multiple commands concurrently which dhcpd can’t do.

Also on his patches, I had a significantly faster way of determining the domain name (in bash) and he and I were looking for your feedback on if we'd be allowed to use it.

Anything is allowed. It just has to be fast. And for that, I believe, calling any other binary will be a problem.

-Michael


Regards,

Nick

Best,
-Michael



On 10/04/2024 12:04, Nick Howitt wrote:


Bump

On 28/03/2024 17:25, Nick Howitt wrote:


Next version. 

On 28/03/2024 10:41, Michael Tremer wrote:


Hello, 



On 27 Mar 2024, at 15:28, Nick Howitt <nick@howitts.co.uk> wrote: 



On 26/03/2024 11:10, Michael Tremer wrote:


On 25 Mar 2024, at 13:11, Nick Howitt <nick@howitts.co.uk> wrote: 



On 25/03/2024 11:31, Michael Tremer wrote:


Hello, 
I don’t 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 “r+” you can catch the Exception and skip the comparison and open the file again with “w”.


On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> 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 = not filecmp.cmp(f.name, self.path, shallow=False) 
except: 
RequireUnboundReload = True


I generally don’t 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. 


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=13254, 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 script gets triggered, 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 lease, 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 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 copying 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.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 the file comparison more effective. 
My patch is: 
>From 73873d4944944a2f02317a73074a6894726f36f7 Mon Sep 17 00:00:00 2001 
From: Nick Howitt <nick@howitts.co.uk> 
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 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/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="w", delete=False) 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 = not filecmp.cmp(f.name, self.path, shallow=False) 
+ 
+ # 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 = ["unbound-control"]


Resubmitting the patch. 

Following my previous reply, I would like to stick with filecmp as I am using it with "shallow=False" 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, atomic file replacement etc., scares me.


File descriptors are great. They avoid race-conditions and all sorts of things.


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.


Well, I pretty much gave you the entire solution… There was nothing left to do apart from copying and pasting the snippets. 



Ok I've tried to use your version but struggled to get it going. If I used it as was and called compare(file1_name, file2_name) it failed as "compare" was not defined. 
I could call it using self.compare but then it failed with "TypeError: UnboundConfigWriter.compare() takes 2 positional arguments but 3 were given". I changed the function to compare(self, f1, f2). 
Then it failed with "AttributeError: 'str' object has no attribute 'seek'", so I changed it again to accept file names then open the files before doing the seeks. 
I really hope it is OK as I am out of my depth when defining functions with "self" as an argument. Please can you check it? If it is not OK, can I revert to filecmp? 

I have made one futher change, the sorted(leases) line was not working 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=lambda x: x.ipaddr)". It definitely sorts this time. 



I don’t 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’t see where the cache helps.


From the filecmp.clear_cache documentation: 

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. 

Now this makes me believe that the clear_cache is only relevant when using shallow=True, which I'm not and I don't know how to check the underlying 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.


The function is here and it is rather simple: https://github.com/python/cpython/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 return a match for us because the temporary file at least will always have another mtime. 

You could clear the cache after every call, but I would rather not have our code messy and care about implementation details of some Python module. 



The module is chosen because all the references I read about comparing files preferred this one-liner for comparing files, although other methods were given and there were lots of discussions about large files (GB+) because if you chomp them it may be much quicker to say they are not the same when you find the first block that does not match. Having said this, I don't know 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.


You would always want to read both files block for block and compare the blocks. There is no way to implement this significantly faster. 



I have modified the earlier try/except I proposed to catch only 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 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="w", delete=False) 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() 
+ try: 
+ require_unbound_reload = not filecmp.cmp(f.name, self.path, shallow=False) 
+ except FileNotFoundError: 
+ require_unbound_reload = 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 written (because the file is still open). You will then compare incomplete output. 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 the 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. 



Catching FileNotFound would work for me.


Good


+ 
+ # 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 = ["unbound-control"] 
-- 
2.43.0 


Nick


--- 
config/unbound/unbound-dhcp-leases-bridge | 50 +++++++++++++++++++---- 
1 file changed, 41 insertions(+), 9 deletions(-) 

diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge 
index e9f022aff..80d9523ea 100644 
--- a/config/unbound/unbound-dhcp-leases-bridge 
+++ b/config/unbound/unbound-dhcp-leases-bridge 
@@ -516,26 +516,37 @@ 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="w", delete=False) as f: 
- for l in leases: 
+ for l in sorted(leases, key=lambda x: x.ipaddr): 
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) 
+ try: 
+ require_unbound_reload = not self.compare(f.name, self.path) 
+ except FileNotFoundError: 
+ require_unbound_reload = 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 = ["unbound-control"] 
@@ -551,6 +562,27 @@ class UnboundConfigWriter(object): 

raise e 

+ def compare(self, if1, if2): 
+ buffersize = 16 * 1024 
+ 
+ f1 = open(if1, "r") 
+ f2 = open(if2, "r") 
+ 
+ # Reset file handles 
+ f1.seek(0) 
+ f2.seek(0) 
+ 
+ while True: 
+ b1 = f1.read(buffersize) 
+ b2 = f2.read(buffersize) 
+ 
+ if not b1 == b2: 
+ return False 
+ 
+ elif not b1 or not b2: 
+ break 
+ 
+ return True 

if __name__ == "__main__": 
parser = argparse.ArgumentParser(description="Bridge for DHCP Leases and Unbound DNS")

