From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 1/3] unbound-dhcp-leases-bridge : fix bug 12694 - DHCP hosts not reliably propagated to DNS Date: Tue, 29 Mar 2022 10:39:53 +0100 Message-ID: <97608BD0-80FB-4E43-AF44-7FD9B7010496@ipfire.org> In-Reply-To: <7f606217-40a8-43ee-97f0-e4f83b6fd652@www.fastmail.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0022471767936986381==" List-Id: --===============0022471767936986381== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 28 Mar 2022, at 20:55, Anthony Heading wrote: >=20 > Hello Michael, >=20 > That was exactly it, just sidestepping an intuitive race with dhcpd writing= . Somewhat hypothetical why that would happen, e.g. clustered updates for mu= ltiple NICs or whatever similar non-atomicity, but it was easy to express th= at way, put the update logic in one place (including the initial one at star= tup), 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 bool= ean 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 l= oad on busy systems. Reloading this all multiple times a second isn=E2=80=99t a good idea. We shou= ld 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 >=20 > A >=20 > On Mon, Mar 28, 2022, at 1:00 PM, Michael Tremer wrote: >> Hello Anthony, >>=20 >> Thank you very much for submitting this patch and welcome to the list. >>=20 >> I understand that it is easier to track any changes in the directory,=20 >> but I don=E2=80=99t quite understand why the logic had to be changed that = the=20 >> changes will be applied at the end only. >>=20 >> Is this just to avoid that multiple updates happen one after the other=20 >> or what are you trying to achieve? >>=20 >> -Michael >>=20 >>> On 22 Mar 2022, at 03:47, Anthony Heading wrote: >>>=20 >>> 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(-) >>>=20 >>> 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 =3D fix_leases_file >>> self.hosts_file =3D hosts_file >>>=20 >>> + # base mask for a completed file change >>> + mask =3D inotify.constants.IN_CLOSE_WRITE | inotify.constants.IN_MOVED= _TO >>> + # IN_MODIFY since dhcpd appends lease updates to an open file >>> + self.watches =3D { >>> + self.leases_file: mask | inotify.constants.IN_MODIFY, >>> + self.fix_leases_file: mask, >>> + self.hosts_file: mask >>> + } >>> + >>> self.unbound =3D UnboundConfigWriter(unbound_leases_file) >>> self.running =3D False >>>=20 >>> @@ -80,36 +89,42 @@ class UnboundDHCPLeasesBridge(object): >>> self.running =3D True >>>=20 >>> # Initial setup >>> - self.hosts =3D self.read_static_hosts() >>> - self.update_dhcp_leases() >>> + update_hosts =3D True >>> + update_leases =3D True >>> + >>> + i =3D inotify.adapters.Inotify() >>>=20 >>> - i =3D 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]) >>>=20 >>> for event in i.event_gen(): >>> # End if we are requested to terminate >>> if not self.running: >>> break >>>=20 >>> + # Make pending updates once inotify queue is empty >>> if event is None: >>> + if update_hosts: >>> + self.hosts =3D self.read_static_hosts() >>> + update_hosts =3D False >>> + if update_leases: >>> + self.update_dhcp_leases() >>> + update_leases =3D False >>> continue >>>=20 >>> header, type_names, watch_path, filename =3D event >>>=20 >>> - # Update leases after leases file has been modified >>> - if "IN_MODIFY" in type_names: >>> - # Reload hosts >>> - if watch_path =3D=3D self.hosts_file: >>> - self.hosts =3D self.read_static_hosts() >>> + file =3D os.path.join(watch_path, filename) >>> + >>> + if not file in self.watches: >>> + continue >>> + >>> + log.debug("Inotify %s: %s", file, " ".join(type_names)) >>>=20 >>> - self.update_dhcp_leases() >>> + update_leases =3D True >>>=20 >>> - # If the file is deleted, we re-add the watcher >>> - if "IN_IGNORED" in type_names: >>> - i.add_watch(watch_path) >>> + if file =3D=3D self.hosts_file: >>> + update_hosts =3D True >>>=20 >>> log.info("Unbound DHCP Leases Bridge terminated") >>>=20 >>> --=20 >>> 2.34.1 >>>=20 --===============0022471767936986381==--