Stop unbound-dhcp-leases-bridge from continually restarting unbound

[Nick Howitt <nick@howitts.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 4396 bytes --]

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.

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(a)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"]
-- 
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?

Regards,

Nick