Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auditbeat recursive file watches for Windows #6893

Merged
merged 5 commits into from
Apr 19, 2018

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Apr 18, 2018

Added recursive file watches support to Auditbeat using the Windows API

This helps prevent a deadlock under Windows when a lot of file events are generated while watches are being installed.

Fixes #6864

@ruflin ruflin added in progress Pull request is currently in progress. Auditbeat labels Apr 18, 2018
@adriansr adriansr force-pushed the fix/auditbeat/6864 branch 6 times, most recently from b62f997 to 9eaf837 Compare April 19, 2018 14:09
@adriansr adriansr changed the title [WIP] Auditbeat recursive watches for Windows Auditbeat recursive file watches for Windows Apr 19, 2018
@adriansr adriansr added bug enhancement review and removed in progress Pull request is currently in progress. labels Apr 19, 2018
@andrewkroh
Copy link
Member

I don't see the fsnotify changes to add SetRecursive in the PR. And I left a comment at adriansr/fsnotify@c9bbe1f#r28653958.

Do you plan to try to contribute those changes upstream? I think I'd like to keep the import statements as github.com/fsnotify/fsnotify and have govendor point to your fork (e.g. govendor fetch github.com/fsnotify/fsnotify/...::github.com/adriansr/fsnotify).

@adriansr
Copy link
Contributor Author

@andrewkroh answered your comment in adriansr/fsnotify and changed the dependency to fsnotify/fsnotify as well

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just had the one question.


addWatchesFirst := runtime.GOOS != "windows"

if addWatchesFirst {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of configuring the paths before Start'ing in the non-windows case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was afraid it could introduce problems in platforms other than Windows. That's why I created the system test, but at the end forgot to update this code.

I've pushed a new version were watches are always installed after Start().

Allow system tests to search for text in the beat logfile using
case-insensitive search. This is necessary to match paths in
case-insensitive file systems, where the path logged may have different
capitalisation than the one used in the system test.
@adriansr adriansr force-pushed the fix/auditbeat/6864 branch 4 times, most recently from 0414de1 to 307d779 Compare April 19, 2018 17:36
Under Windows, directories to be watched need to be installed after the
event consumer loop is started. Otherwise there's a chance of a
potential deadlock, as fsnotify under Windows uses a single goroutine to
deliver events and install watches. If the channel it uses to deliver
events is full, it will block and won't be able to install further
watches.
This patch enables OS-supported recursive watching in fsnotify if it is
available. Currently only supported under Windows via our custom
fsnotify fork.
@andrewkroh
Copy link
Member

It looks like Windows CI in green now. 👍

Did you want these changes squashed or left as is?

@adriansr
Copy link
Contributor Author

No squash please

@andrewkroh andrewkroh merged commit 8088e53 into elastic:master Apr 19, 2018
@andrewkroh
Copy link
Member

I totally read that as "No, squash please". 🤦‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants