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

Hello,

This is indeed an absolute nightmare what I built here.

It has some advantages over the alternatives of which one is to have the DHCP server execute a couple of commands. The problems there are as follows:

* unbound does not keep a state, so we will have to make sure that we keep the state of all records somewhere and can reload it when the system roboots.

* I have installations with thousands of devices in a single subnet. The DHCP server would just be busy constantly executing any commands which would cause a lot of load. The current Python bridge can use up to 40% of one CPU core at busy times on the same system. I would argue that this approach does not scale well.

* RFC2136 would be great, but we lack too many things to run this properly.

That being said, this is not a great way to solve this problem. I would like to replace some components entirely but probably not soon. The approach that we have works and - apart from the bug - doesn’t suck too hard. Should we do it this way again? No, but we had to try to learn that lesson.

-Michael

> On 28 Mar 2022, at 20:32, Jon Murphy <jcmurphy26(a)gmail.com> wrote:
> 
> Sorry for interrupting this train of thought.  Wouldn’t it be easier to use the tools available from dhcpd?
> 
> dhcpd seems to have a way execute commands.  And I would guess this would be cleaner than "watching" a file.
> 
> There are three execute commands:
> on commit
> on release
> on expiry
> 
> and these could be used to launch the needed unbound bridge command.
> 
> Here is where I found this:
> https://jpmens.net/2011/07/06/execute-a-script-when-isc-dhcp-hands-out-a-new-lease/
> 
> And here is additional info:
> https://wiki.samba.org/index.php/Configure_DHCP_to_update_DNS_records
> 
> (Search for "on commit" on this page)
> 
> 
> Jon
> 
> 
>> On Mar 28, 2022, at 12:00 PM, Michael Tremer <michael.tremer(a)ipfire.org> 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
>>> 
>> 
>