Hello,
On 24 Mar 2024, at 15:25, Nick Howitt nick@howitts.co.uk wrote:
Hi all,
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.
No worries, feel free to ask any questions that you have… There is documentation around all of this here:
https://www.ipfire.org/docs/devel
Especially here:
https://www.ipfire.org/docs/devel/submit-patches
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.
I am still not entirely sure why it is such an issue to reload Unbound that often, but I am happy to avoid doing this whenever possible.
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.
Yeah, I think this is a workable solution to mitigate the problem slightly.
Note that Jon has sent a couple of ideas a while back solving this problem by removing the bridge and running shell commands instead, but I did not have the time to look into how robust that solution would be. With the bridge, we have at least a certain amount of confidence.
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.
In theory you could have left it there, because even if the file is being removed later, there is no harm in changing permissions before.
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.
I think I can live with that. This while thing is very far from efficient anyways and serves as some glue between Unbound and ISC DHCP. The latter will definitely go soon as it has become EOL and whenever we make that transition we will have to come up with something new which hopefully does not have the shortcomings that the bridge has.
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.
I think we are evaluating where to do the maths. I always considered it perfectly safe to just throw the new file into the Unbound configuration, have Unbound read it again and then everything is good. I would also argue that that works without any problems even in places where I have 2000 active leases with a rather short lease time. However, there is the log message…
Sorting creates some extra work, but I am sure that this is a lot cheaper than reloading Unbound. So once again, I am happy to go down this route.
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
This module does however not seem very fit for purpose for me. It tries to check the metadata of the file using stat() calls which will not do at all what we want them to do and it will cache any results which is also not at all helpful.
I would suggest to simply implement this function yourself like so:
def compare(f1, f2): buffersize = 16 * 1024
# 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
You can pass the file descriptors to each of the files and since we are now changing a couple of things, former assumptions can be dropped:
* Unbound used to reload the configuration file, but there could have been the case of concurrent reads. Therefore the entire atomic file replacement can be removed. * With that, you could remove the delete=False attribute from the temporary file. You already have the target file open and you can just copy the content from the temporary file. Because of the former assumption I would consider that safe.
That way, you will be able to avoid copying anything if the files don’t match and you will not have to introduce any extra code to remove the temporary file. And the chmod statement can go, too :)
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)
Oh and please use snake case for variables. It is Python after all.
- # 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"] -- 2.43.0
I have this locally on a branch on my firewall. If it is OK, do I need some sort of rights to push my branch and then how I get it merged?
I can create your own Git repository for you to backup/manage your own changes, but we do not allow anyone apart from two people to push into the master repository.
I am also very happy to see your first code contribution to IPFire!
-Michael
Regards,
Nick