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: 4990 bytes --]

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
>>>