Re: [PATCH 1/3] unbound-dhcp-leases-bridge : fix bug 12694 - DHCP hosts not reliably propagated to DNS

[Michael Tremer <michael.tremer@ipfire.org>]

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

Please see the patch set that I just posted which is strongly based on your patches, but incorporates a couple of new changes, too.

Let me know if this looks good to you.

Best,
-Michael

> On 29 Mar 2022, at 10:39, Michael Tremer <michael.tremer(a)ipfire.org> wrote:
> 
> Hello,
> 
>> On 28 Mar 2022, at 20:55, Anthony Heading <ajrh(a)ajrh.net> wrote:
>> 
>> Hello Michael,
>> 
>> That was exactly it, just sidestepping an intuitive race with dhcpd writing.  Somewhat hypothetical why that would happen, e.g. clustered updates for multiple NICs or whatever similar non-atomicity,  but it was easy to express that way,  put the update logic in one place (including the initial one at startup), and made the event timestamps sequences a little easier to follow when it wasn't clear why the updates were getting lost at all.  I thought the boolean variables helped document the admittedly simple logic too.   But none of that is critical.
> 
> Okay. Thank you for the explanation.
> 
> I will take this into account, but I will add some more changes, since we are performing a reload every time something changes which is causing a lot of load on busy systems.
> 
> Reloading this all multiple times a second isn’t a good idea. We should rather reload and then wait at least 5 seconds and then reload again if we had another inotify event.
> 
> This is probably slightly unrelated with your patch, but since we are already on it, I would like to have those changes implemented now.
> 
> I will get back to the list with those changes and submit them for review.
> 
> Best,
> -Michael
> 
>> 
>> A
>> 
>> On Mon, Mar 28, 2022, at 1:00 PM, Michael Tremer wrote:
>>> Hello Anthony,
>>> 
>>> Thank you very much for submitting this patch and welcome to the list.
>>> 
>>> I understand that it is easier to track any changes in the directory, 
>>> but I don’t quite understand why the logic had to be changed that the 
>>> changes will be applied at the end only.
>>> 
>>> Is this just to avoid that multiple updates happen one after the other 
>>> or what are you trying to achieve?
>>> 
>>> -Michael
>>> 
>>>> On 22 Mar 2022, at 03:47, Anthony Heading <ajrh(a)ajrh.net> wrote:
>>>> 
>>>> Switch from inotify watching individual files to monitoring the
>>>> containing directories, as because dhcpd renames its leases file into a
>>>> backup, monitoring the single inode does not work well.  Additionally,
>>>> python appears to have a bug with replacing expired inotify watches on
>>>> single files.
>>>> ---
>>>> unbound-dhcp-leases-bridge | 47 +++++++++++++++++++++++++-------------
>>>> 1 file changed, 31 insertions(+), 16 deletions(-)
>>>> 
>>>> diff --git unbound-dhcp-leases-bridge unbound-dhcp-leases-bridge
>>>> index a2df5f1..6e22066 100644
>>>> --- unbound-dhcp-leases-bridge
>>>> +++ unbound-dhcp-leases-bridge
>>>> @@ -72,6 +72,15 @@ class UnboundDHCPLeasesBridge(object):
>>>> 		self.fix_leases_file = fix_leases_file
>>>> 		self.hosts_file = hosts_file
>>>> 
>>>> +		# base mask for a completed file change
>>>> +		mask = inotify.constants.IN_CLOSE_WRITE | inotify.constants.IN_MOVED_TO
>>>> +		# IN_MODIFY since dhcpd appends lease updates to an open file
>>>> +		self.watches = {
>>>> +		    self.leases_file: mask | inotify.constants.IN_MODIFY,
>>>> +		    self.fix_leases_file: mask,
>>>> +		    self.hosts_file: mask
>>>> +		}
>>>> +
>>>> 		self.unbound = UnboundConfigWriter(unbound_leases_file)
>>>> 		self.running = False
>>>> 
>>>> @@ -80,36 +89,42 @@ class UnboundDHCPLeasesBridge(object):
>>>> 		self.running = True
>>>> 
>>>> 		# Initial setup
>>>> -		self.hosts = self.read_static_hosts()
>>>> -		self.update_dhcp_leases()
>>>> +		update_hosts = True
>>>> +		update_leases = True
>>>> +
>>>> +		i = inotify.adapters.Inotify()
>>>> 
>>>> -		i = inotify.adapters.Inotify([
>>>> -			self.leases_file,
>>>> -			self.fix_leases_file,
>>>> -			self.hosts_file,
>>>> -		])
>>>> +		for f in self.watches:
>>>> +			i.add_watch(os.path.dirname(f), self.watches[f])
>>>> 
>>>> 		for event in i.event_gen():
>>>> 			# End if we are requested to terminate
>>>> 			if not self.running:
>>>> 				break
>>>> 
>>>> +			# Make pending updates once inotify queue is empty
>>>> 			if event is None:
>>>> +				if update_hosts:
>>>> +					self.hosts = self.read_static_hosts()
>>>> +					update_hosts = False
>>>> +				if update_leases:
>>>> +					self.update_dhcp_leases()
>>>> +					update_leases = False
>>>> 				continue
>>>> 
>>>> 			header, type_names, watch_path, filename = event
>>>> 
>>>> -			# Update leases after leases file has been modified
>>>> -			if "IN_MODIFY" in type_names:
>>>> -				# Reload hosts
>>>> -				if watch_path == self.hosts_file:
>>>> -					self.hosts = self.read_static_hosts()
>>>> +			file = os.path.join(watch_path, filename)
>>>> +
>>>> +			if not file in self.watches:
>>>> +				continue
>>>> +
>>>> +			log.debug("Inotify %s: %s", file, " ".join(type_names))
>>>> 
>>>> -				self.update_dhcp_leases()
>>>> +			update_leases = True
>>>> 
>>>> -			# If the file is deleted, we re-add the watcher
>>>> -			if "IN_IGNORED" in type_names:
>>>> -				i.add_watch(watch_path)
>>>> +			if file == self.hosts_file:
>>>> +				update_hosts = True
>>>> 
>>>> 		log.info("Unbound DHCP Leases Bridge terminated")
>>>> 
>>>> -- 
>>>> 2.34.1
>>>> 
>