From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Heading 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 23:30:10 -0400 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3133717240864442630==" List-Id: --===============3133717240864442630== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Michael, Looks great to me. Glad we're getting it fixed. Only very minor comments: 1) It looks like stderr is now left open even in daemon mode? I doubt it's = connected to anything that would block, nor even that anything is actually w= ritten to stderr, but not sure if you intended it. 2) More generally, the bridge is somewhat taciturn in comparison to both dhc= pd and unbound, I think a little more logging by default would be helpful for= users looking at the syslog logs. Either adding a single '-v' to the init.d= script, or perhaps you might want to rebalance it differently, but logging = the DNS changes that are being made would be very useful I think, and that w= ould not be heavy when compared to what dhcpd is already generating in syslog. Regards Anthony =20 On Tue, Mar 29, 2022, at 7:35 AM, Michael Tremer wrote: > Please see the patch set that I just posted which is strongly based on=20 > 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 wro= te: >>=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 writi= ng. 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 st= artup), and made the event timestamps sequences a little easier to follow whe= n it wasn't clear why the updates were getting lost at all. I thought the bo= olean variables helped document the admittedly simple logic too. But none o= f 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 = are performing a reload every time something changes which is causing a lot o= f load on busy systems. >>=20 >> Reloading this all multiple times a second isn=E2=80=99t a good idea. We s= hould 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 alre= ady 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 tha= t 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_MOV= ED_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 >> --===============3133717240864442630==--