fixes #16021 - restart inotify monitoring on queue overflow

[domcleal]

seq -f "/tmp/isc/test.%g" 1 50000 | xargs touch seems to be an effective reproducer for me, if /tmp/isc/ is the location that contains the leases file.

[dmitri-d]

Do you want to try splitting the queues?

One queue for watching the changes on the lease file, the other queue for watching :move events on the directory. Still need to handle the QueueOverflowError, but I suspect there will be fewer of those...

[domcleal]

I think the watch could be made a lot more specific actually, so instead of watching for all events in the target directory, it only watches for modifications on the lease file and moved_to events for the lease file.

It could still use a single queue as the likelihood of queue exhaustion from other events in the same directory will be greatly reduced by being more specific, saving another thread and complexity.

[domcleal]

Updated to have specific watches for file modification and incoming moved files. This should help reduce the queue size significantly, as no longer will inotify events be generated for every single unrelated file open/read/close in the same directory.

[dmitri-d]

This breaks after leases.file is re-created by dhcpd -- no events are being caught after that.

I think what's going on is that inotify_add_watch call uses the file path that was passed to it to open a file descriptor. After dhcpd re-creates the leases file, inotify continues to use the file descriptor that now points to the old copy of the leases file (dhcpd deletes it, but I think it will kick around until the fd we are holding is closed).

I think on catching :move_to event the old :modify event watch should be deleted and a new one, pointing to the same path created. There is a chance that we are going to miss an update between observer.leases_recreated call and when the new watch is created, but we'll pick up the changes during the next :modify event.

[domcleal]

Thanks, good catch!

The inotify watch remains on the old inode so would need re-registering. I tried to implement the re-registering of :modify when the file's recreated, but there's a bug in rb-inotify 0.9.7 which raises undefined method 'callback!' for nil:NilClass (NoMethodError) after the old watcher is unregistered. This is fixed in master, but unreleased, so I've filed http://projects.theforeman.org/issues/16140 to do it later.

Instead, I've reverted to the original implementation, but changed the watch on :all_events to just the modify/moved_to events, which should prevent file reads, opens etc from generating events and reduce the queue size.

[dmitri-d]

: moved_to here?

[domcleal]

Sure, updated. Both :move and :moved_to flags are present.

[dmitri-d]

We could create a notifier/watcher pair per event and discard the one used for 'modify' events when the leases is recreated, but I think having re-creating 'modify' watcher is cleaner.

[domcleal]

We could create a notifier/watcher pair per event and discard the one used for 'modify' events when the leases is recreated, but I think having re-creating 'modify' watcher is cleaner.

Yeah, recreating the whole notifier is also more complex as it'd probably have to run in a separate thread (calling its own #run). I think re-creating the watcher will be a far simpler step when rb-inotify's updated.

[dmitri-d]

merged as 2e0765f. Thanks, @domcleal.

[domcleal]

@witlessbird the commit pushed is different to the current PR, it's the version that didn't re-register the :modify watcher after the inode changed. Shall I open another PR to fix that back?

[domcleal]

I opened #456 with the latest changes in this PR.