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.

Sign up for GitHub

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

Jump to bottom

Enable polling in file watcher #322

Merged
merged 2 commits into from
Sep 23, 2024
Merged

Enable polling in file watcher #322

merged 2 commits into from
Sep 23, 2024

Conversation

daniel-zullo-frequenz
Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz commented Sep 18, 2024 •
edited
Loading

inotify does not work reliably with network file systems (e.g., NFS, CIFS) commonly used in cloud environments. These
file systems may not propagate file system events correctly, causing inotify to miss changes. To ensure consistent file
monitoring across these environments, polling is enabled by default in FileWatcher.

Fixes #256

@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`) labels Sep 18, 2024
@daniel-zullo-frequenz daniel-zullo-frequenz added the type:bug Something isn't working label Sep 18, 2024
llucax
llucax reviewed Sep 19, 2024
View reviewed changes
src/frequenz/channels/file_watcher.py Outdated
@@ -127,13 +128,19 @@ def __init__(
self,
paths: list[pathlib.Path | str],
event_types: abc.Iterable[EventType] = frozenset(EventType),
*,
enable_polling: bool = True,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are using a positive term then I would use the same as watchfiles (force_polling), as probably even when passing force_polling=False (enable_polling=False) polling will probably be used if I notify or other event based system calls are not available in the OS.

tests/test_file_watcher_integration.py Show resolved Hide resolved
@daniel-zullo-frequenz daniel-zullo-frequenz marked this pull request as ready for review September 19, 2024 08:29
@daniel-zullo-frequenz daniel-zullo-frequenz requested a review from a team as a code owner September 19, 2024 08:29
@daniel-zullo-frequenz
Copy link
Contributor Author

Updated based on initial suggestions from Luca

@github-actions github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Sep 19, 2024
@daniel-zullo-frequenz
Copy link
Contributor Author

Added a new commit to remove the redundant TARGETPLATFORM in Dockerfile

llucax
llucax reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

I have the same concern as with #321 , this is actually mixing one bug fix (setting force_poll=True, which depending on how we see it it could even be a breaking change1) and a new feature (being able to configure force_polling).

Footnotes

  1. Maybe this is worth documenting in the Upgrading section, mentioning the default mechanism was changed and updates might have a bit of a delay now and watching a large amount of files might be less efficient, and letting the user know how to go back to the previous default. ↩

.github/containers/test-installation/Dockerfile Show resolved Hide resolved
@llucax
Copy link
Contributor

llucax commented Sep 19, 2024

For simplicity I think we can release all of this in v1.2.0 and that's it.

llucax
llucax previously approved these changes Sep 19, 2024
View reviewed changes
@daniel-zullo-frequenz
Remove redundant TARGETPLATFORM in Dockerfile
1a262ef 
Setting platform to predefined $TARGETPLATFORM is
redundant as this is the default behavior.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz
Copy link
Contributor Author

Updated release-notes to add an entry in Upgrading. And rebased onto the latest v1.x.x

llucax
llucax reviewed Sep 23, 2024
View reviewed changes
RELEASE_NOTES.md Outdated Show resolved Hide resolved
tests/test_file_watcher_integration.py Show resolved Hide resolved
@daniel-zullo-frequenz
Enable polling in file watcher
bbb1198 
`inotify` does not work reliably with network file systems
(e.g., NFS, CIFS) commonly used in cloud environments. These
file systems may not propagate file system events correctly,
causing `inotify` to miss changes. To ensure consistent file
monitoring across these environments, polling is enabled by
default in FileWatcher.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz daniel-zullo-frequenz added this pull request to the merge queue Sep 23, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit 13cf174 Sep 23, 2024
14 checks passed
@daniel-zullo-frequenz daniel-zullo-frequenz deleted the fix/file-watcher-polling branch September 23, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llucax llucax llucax approved these changes

@shsms shsms Awaiting requested review from shsms shsms is a code owner automatically assigned from frequenz-floss/python-sdk-team

Assignees
No one assigned
Labels
part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`) type:bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filewatcher is not reporting events in CIFS mounts
2 participants
@daniel-zullo-frequenz @llucax