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 12:35:46 +0100 Message-ID: In-Reply-To: <97608BD0-80FB-4E43-AF44-7FD9B7010496@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6739658920681472698==" List-Id: --===============6739658920681472698== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Please see the patch set that I just posted which is strongly based on your p= atches, 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 wrot= e: >=20 > Hello, >=20 >> 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 writin= g. Somewhat hypothetical why that would happen, e.g. clustered updates for m= ultiple NICs or whatever similar non-atomicity, but it was easy to express t= hat way, put the update logic in one place (including the initial one at sta= rtup), 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 boo= lean variables helped document the admittedly simple logic too. But none of= that is critical. >=20 > Okay. Thank you for the explanation. >=20 > I will take this into account, but I will add some more changes, since we a= re performing a reload every time something changes which is causing a lot of= load on busy systems. >=20 > Reloading this all multiple times a second isn=E2=80=99t a good idea. We sh= ould rather reload and then wait at least 5 seconds and then reload again if = we had another inotify event. >=20 > This is probably slightly unrelated with your patch, but since we are alrea= dy on it, I would like to have those changes implemented now. >=20 > I will get back to the list with those changes and submit them for review. >=20 > Best, > -Michael >=20 >>=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_MOVE= D_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 >=20 --===============6739658920681472698==--