[PATCH] unbound-dhcp-leases-bridge: Reload unbound to import leases

[PATCH] unbound-dhcp-leases-bridge: Reload unbound to import leases

[Michael Tremer]

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

This changes the old "diff" algorithm that we needed to have before
Unbound was able to reload its own configuration.

Now, it can do this even without dropping the cache. This should
hopefully perform much better and be more reliable than the old way.

Signed-off-by: Michael Tremer <michael.tremer(a)ipfire.org>
---
 config/unbound/unbound-dhcp-leases-bridge | 52 ++++-------------------
 1 file changed, 8 insertions(+), 44 deletions(-)

diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge
index e89e0446b..e9f022aff 100644
--- a/config/unbound/unbound-dhcp-leases-bridge
+++ b/config/unbound/unbound-dhcp-leases-bridge
@@ -514,56 +514,19 @@ class UnboundConfigWriter(object):
 	def __init__(self, path):
 		self.path = path
 
-		self._cached_leases = []
-
 	def update_dhcp_leases(self, leases):
-		# Find any leases that have expired or do not exist any more
-		# but are still in the unbound local data
-		removed_leases = [l for l in self._cached_leases if not l in leases]
-
-		# Find any leases that have been added
-		new_leases = [l for l in leases if l not in self._cached_leases]
-
-		# End here if nothing has changed
-		if not new_leases and not removed_leases:
-			return
-
 		# Write out all leases
 		self.write_dhcp_leases(leases)
 
-		# Update unbound about changes
-		for l in removed_leases:
-			try:
-				for name, ttl, type, content in l.rrset:
-					log.debug("Removing records for %s" % name)
-					self._control("local_data_remove", name)
-
-			# If the lease cannot be removed we will try the next one
-			except:
-				continue
-
-			# If the removal was successful, we will remove it from the cache
-			else:
-				self._cached_leases.remove(l)
-
-		for l in new_leases:
-			try:
-				for rr in l.rrset:
-					log.debug("Adding new record %s" % " ".join(rr))
-					self._control("local_data", *rr)
-
-			# If the lease cannot be added we will try the next one
-			except:
-				continue
+		log.debug("Reloading Unbound...")
 
-			# Add lease to cache when successfully added
-			else:
-				self._cached_leases.append(l)
+		# Reload the configuration without dropping the cache
+		self._control("reload_keep_cache")
 
 	def write_dhcp_leases(self, leases):
-		with tempfile.NamedTemporaryFile(mode="w", delete=False) as f:
-			filename = f.name
+		log.debug("Writing DHCP leases...")
 
+		with tempfile.NamedTemporaryFile(mode="w", delete=False) as f:
 			for l in leases:
 				for rr in l.rrset:
 					f.write("local-data: \"%s\"\n" % " ".join(rr))
@@ -571,7 +534,8 @@ class UnboundConfigWriter(object):
 			# Make file readable for everyone
 			os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)
 
-		os.rename(filename, self.path)
+			# Move the file to its destination
+			os.rename(f.name, self.path)
 
 	def _control(self, *args):
 		command = ["unbound-control"]
@@ -585,7 +549,7 @@ class UnboundConfigWriter(object):
 			log.critical("Could not run %s, error code: %s: %s" % (
 				" ".join(command), e.returncode, e.output))
 
-			raise
+			raise e
 
 
 if __name__ == "__main__":
-- 
2.39.2

Re: [PATCH] unbound-dhcp-leases-bridge: Reload unbound to import leases

[Peter Müller]

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

Acked-by: Peter Müller <peter.mueller(a)ipfire.org>

> This changes the old "diff" algorithm that we needed to have before
> Unbound was able to reload its own configuration.
> 
> Now, it can do this even without dropping the cache. This should
> hopefully perform much better and be more reliable than the old way.
> 
> Signed-off-by: Michael Tremer <michael.tremer(a)ipfire.org>
> ---
>  config/unbound/unbound-dhcp-leases-bridge | 52 ++++-------------------
>  1 file changed, 8 insertions(+), 44 deletions(-)
> 
> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge
> index e89e0446b..e9f022aff 100644
> --- a/config/unbound/unbound-dhcp-leases-bridge
> +++ b/config/unbound/unbound-dhcp-leases-bridge
> @@ -514,56 +514,19 @@ class UnboundConfigWriter(object):
>  	def __init__(self, path):
>  		self.path = path
>  
> -		self._cached_leases = []
> -
>  	def update_dhcp_leases(self, leases):
> -		# Find any leases that have expired or do not exist any more
> -		# but are still in the unbound local data
> -		removed_leases = [l for l in self._cached_leases if not l in leases]
> -
> -		# Find any leases that have been added
> -		new_leases = [l for l in leases if l not in self._cached_leases]
> -
> -		# End here if nothing has changed
> -		if not new_leases and not removed_leases:
> -			return
> -
>  		# Write out all leases
>  		self.write_dhcp_leases(leases)
>  
> -		# Update unbound about changes
> -		for l in removed_leases:
> -			try:
> -				for name, ttl, type, content in l.rrset:
> -					log.debug("Removing records for %s" % name)
> -					self._control("local_data_remove", name)
> -
> -			# If the lease cannot be removed we will try the next one
> -			except:
> -				continue
> -
> -			# If the removal was successful, we will remove it from the cache
> -			else:
> -				self._cached_leases.remove(l)
> -
> -		for l in new_leases:
> -			try:
> -				for rr in l.rrset:
> -					log.debug("Adding new record %s" % " ".join(rr))
> -					self._control("local_data", *rr)
> -
> -			# If the lease cannot be added we will try the next one
> -			except:
> -				continue
> +		log.debug("Reloading Unbound...")
>  
> -			# Add lease to cache when successfully added
> -			else:
> -				self._cached_leases.append(l)
> +		# Reload the configuration without dropping the cache
> +		self._control("reload_keep_cache")
>  
>  	def write_dhcp_leases(self, leases):
> -		with tempfile.NamedTemporaryFile(mode="w", delete=False) as f:
> -			filename = f.name
> +		log.debug("Writing DHCP leases...")
>  
> +		with tempfile.NamedTemporaryFile(mode="w", delete=False) as f:
>  			for l in leases:
>  				for rr in l.rrset:
>  					f.write("local-data: \"%s\"\n" % " ".join(rr))
> @@ -571,7 +534,8 @@ class UnboundConfigWriter(object):
>  			# Make file readable for everyone
>  			os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)
>  
> -		os.rename(filename, self.path)
> +			# Move the file to its destination
> +			os.rename(f.name, self.path)
>  
>  	def _control(self, *args):
>  		command = ["unbound-control"]
> @@ -585,7 +549,7 @@ class UnboundConfigWriter(object):
>  			log.critical("Could not run %s, error code: %s: %s" % (
>  				" ".join(command), e.returncode, e.output))
>  
> -			raise
> +			raise e
>  
>  
>  if __name__ == "__main__":

Re: [PATCH] unbound-dhcp-leases-bridge: Reload unbound to import leases

[jon]

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

Michael and everyone,

Since I upgraded from CU 176 to CU 177 I haven been seeing lots of Unbound restarts.  I went from less than 5 per week to more than 3500 unbound restarts per week.

It looks like unbound-dhcp-leases-bridge was updated near the same time. And I see the code that was added:
> +	self._control("reload_keep_cache")


Here is the bugzilla report:
https://bugzilla.ipfire.org/show_bug.cgi?id=13254

As Adolf aptly stated:
  [quote="Adolf Belka, post:41, topic:10500, username:bonnietwin"]
    Maybe it is just a natural consequence of the changes that were made to the unbound-dhcp-leases-bridge code.
  [/quote]

So the question is: should I be seeing more than more than 3500 unbound restarts per week.  Or is this a possible bug with unbound?

Any input would be greatly appreciated.

Jon 


> On Jul 11, 2023, at 8:29 AM, Michael Tremer <michael.tremer(a)ipfire.org> wrote:
> 
> This changes the old "diff" algorithm that we needed to have before
> Unbound was able to reload its own configuration.
> 
> Now, it can do this even without dropping the cache. This should
> hopefully perform much better and be more reliable than the old way.
> 
> Signed-off-by: Michael Tremer <michael.tremer(a)ipfire.org>
> ---
> config/unbound/unbound-dhcp-leases-bridge | 52 ++++-------------------
> 1 file changed, 8 insertions(+), 44 deletions(-)
> 
> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge
> index e89e0446b..e9f022aff 100644
> --- a/config/unbound/unbound-dhcp-leases-bridge
> +++ b/config/unbound/unbound-dhcp-leases-bridge
> @@ -514,56 +514,19 @@ class UnboundConfigWriter(object):
> 	def __init__(self, path):
> 		self.path = path
> 
> -		self._cached_leases = []
> -
> 	def update_dhcp_leases(self, leases):
> -		# Find any leases that have expired or do not exist any more
> -		# but are still in the unbound local data
> -		removed_leases = [l for l in self._cached_leases if not l in leases]
> -
> -		# Find any leases that have been added
> -		new_leases = [l for l in leases if l not in self._cached_leases]
> -
> -		# End here if nothing has changed
> -		if not new_leases and not removed_leases:
> -			return
> -
> 		# Write out all leases
> 		self.write_dhcp_leases(leases)
> 
> -		# Update unbound about changes
> -		for l in removed_leases:
> -			try:
> -				for name, ttl, type, content in l.rrset:
> -					log.debug("Removing records for %s" % name)
> -					self._control("local_data_remove", name)
> -
> -			# If the lease cannot be removed we will try the next one
> -			except:
> -				continue
> -
> -			# If the removal was successful, we will remove it from the cache
> -			else:
> -				self._cached_leases.remove(l)
> -
> -		for l in new_leases:
> -			try:
> -				for rr in l.rrset:
> -					log.debug("Adding new record %s" % " ".join(rr))
> -					self._control("local_data", *rr)
> -
> -			# If the lease cannot be added we will try the next one
> -			except:
> -				continue
> +		log.debug("Reloading Unbound...")
> 
> -			# Add lease to cache when successfully added
> -			else:
> -				self._cached_leases.append(l)
> +		# Reload the configuration without dropping the cache
> +		self._control("reload_keep_cache")
> 
> 	def write_dhcp_leases(self, leases):
> -		with tempfile.NamedTemporaryFile(mode="w", delete=False) as f:
> -			filename = f.name
> +		log.debug("Writing DHCP leases...")
> 
> +		with tempfile.NamedTemporaryFile(mode="w", delete=False) as f:
> 			for l in leases:
> 				for rr in l.rrset:
> 					f.write("local-data: \"%s\"\n" % " ".join(rr))
> @@ -571,7 +534,8 @@ class UnboundConfigWriter(object):
> 			# Make file readable for everyone
> 			os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)
> 
> -		os.rename(filename, self.path)
> +			# Move the file to its destination
> +			os.rename(f.name, self.path)
> 
> 	def _control(self, *args):
> 		command = ["unbound-control"]
> @@ -585,7 +549,7 @@ class UnboundConfigWriter(object):
> 			log.critical("Could not run %s, error code: %s: %s" % (
> 				" ".join(command), e.returncode, e.output))
> 
> -			raise
> +			raise e
> 
> 
> if __name__ == "__main__":
> -- 
> 2.39.2
>