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

10.0 dev reload with inotify #31855

Open
wants to merge 4 commits into
base: 10.0
from

Conversation

Projects
None yet
5 participants
@Icallhimtest
Copy link
Contributor

Icallhimtest commented Mar 14, 2019

Add the alternative of inotify instead of watchdog to watch the addons path the server was started with.

Reason: watchdog spawns 2 threads per path to watch. When there are a lot of addons paths, this can become too costly. With inotify we watch all the repositories in a single thread.

https://github.com/dsoprea/PyInotify

installation:
pip install inotify

This PR is a bit more dirty than what's suggested in https://github.com/odoo-dev/odoo/tree/10.0-dev-reload-dve but a lot more efficient.

Icallhimtest added some commits Mar 14, 2019

[FIX] server.py: FileNotFoundError doesn't exist in P2
Python2 uses IOError instead.

/!\ DO NOT FORWARD PORT AFTER 11.* /!\
[FIX] server.py: don't start multiple FSWatcher in multiworker mode
Before this commit every worker would start his own FSWatcher.

@Icallhimtest Icallhimtest changed the title 10.0 dev reload dve 10.0 dev reload with inotify Mar 14, 2019

@robodoo robodoo added the seen 🙂 label Mar 14, 2019

@Icallhimtest Icallhimtest requested a review from KangOl Mar 14, 2019

@C3POdoo C3POdoo added the RD label Mar 14, 2019

@robodoo robodoo added the CI 🤖 label Mar 14, 2019

if 'reload' in config['dev_mode']:
if watchdog:
if 'reload' in config['dev_mode'] and not odoo.evented:
if inotify and not odoo.evented:

This comment has been minimized.

@KangOl

KangOl Mar 14, 2019

Contributor

check on odoo.evented is useless (already checked the line before)

This comment has been minimized.

@Icallhimtest

Icallhimtest Mar 14, 2019

Author Contributor

whoops, indeed

if not self.watcher:
self.watcher = InotifyTree(path, mask=INOTIFY_LISTEN_EVENTS, block_duration_s=1.0)
else:
self.watcher._InotifyTree__load_tree(path)

This comment has been minimized.

@KangOl

KangOl Mar 14, 2019

Contributor

Ho yes, it's dirty

This comment has been minimized.

@Icallhimtest

Icallhimtest Mar 14, 2019

Author Contributor

Yeah, But this avoids having to iterate through different InotifyTree and have all the events on a single fd, which, on my 500 addons-paths test prevents a constant 2% CPU usage to iterate on every path every second.

while self.started:
for event in self.watcher.event_gen(timeout_s=0, yield_nones=False):
(_, type_names, path, filename) = event
if filename.endswith('.py') and not filename.startswith('.~') and 'IN_ISDIR' not in type_names:

This comment has been minimized.

@KangOl

KangOl Mar 14, 2019

Contributor

Can you extract this block inside a common function with FSWatcher using a common ancestor?

This comment has been minimized.

@Icallhimtest

Icallhimtest Mar 14, 2019

Author Contributor

will do

if os.name == 'posix':
module = 'inotify'
else:
module = 'Watchdog'

This comment has been minimized.

@KangOl

KangOl Mar 14, 2019

Contributor

lowercase

watcher = FSWatcher()
watcher.start()
else:
_logger.warning("'watchdog' module not installed. Code autoreload feature is disabled")
if os.name == 'posix':

This comment has been minimized.

@KangOl

KangOl Mar 14, 2019

Contributor

there is not inotify on osx. watchdog is still needed

@Icallhimtest Icallhimtest force-pushed the odoo-dev:10.0-dev-reload-dirty-dve branch from 18b4645 Mar 15, 2019

@robodoo robodoo removed the CI 🤖 label Mar 15, 2019

@Icallhimtest

This comment has been minimized.

Copy link
Contributor Author

Icallhimtest commented Mar 15, 2019

@KangOl @antonylesuisse
The FSWatcherInotify runs fine for the expected scenarios, but a few questions remain:

  1. Inotify behaves a bit differently when moving folders containing files than watchdog. Watchdog will give the notifications for each file in the folder, while inotify only gives the folder move event (I believe because the files get created before inotify gets a chance to add the folder to its watchlist, not sure how watchdog handles this). The question becomes:

a) Do we automatically reload upon directory moves?
b) Do we os.walk the directory in search of python files and reload only if it contains one & it compiles?
c) Do we ignore them? <-- current implementation

  1. Current watchdog implementation ignores when we delete or move a file away from the folder, even a python file.

Is this intended behaviour? Current inotify implementation copies this. It doesn't listen to IN_DELETE, IN_DELETE_SELF, IN_MOVE_SELF, IN_MOVED_FROM events. ( http://man7.org/linux/man-pages/man7/inotify.7.html )

  1. Is it ok to use the __load_tree method from the InotifyTree object? Its signature hasn't changed since its addition 4 years ago. https://github.com/dsoprea/PyInotify/blame/master/inotify/adapters.py#L342

@robodoo robodoo added the CI 🤖 label Mar 15, 2019

@KangOl

This comment has been minimized.

Copy link
Contributor

KangOl commented Mar 15, 2019

@Icallhimtest

This comment has been minimized.

Copy link
Contributor Author

Icallhimtest commented Mar 15, 2019

@KangOl whoops, good thing you saw the InotifyTrees class, I completely missed it. It'll be nice & clean this way.

For point 1, looking at watchdog, they generate all the events manually when a directory is created:

https://github.com/gorakhargosh/watchdog/blob/master/src/watchdog/observers/inotify.py#L156
https://github.com/gorakhargosh/watchdog/blob/master/src/watchdog/events.py#L612

I'm hesitating between submitting a PR to inotify adding this feature somewhere here https://github.com/dsoprea/PyInotify/blame/master/inotify/adapters.py#L287
or simply doing it in Odoo.

@Icallhimtest Icallhimtest force-pushed the odoo-dev:10.0-dev-reload-dirty-dve branch Mar 15, 2019

@robodoo robodoo removed the CI 🤖 label Mar 15, 2019

@KangOl

This comment has been minimized.

Copy link
Contributor

KangOl commented Mar 15, 2019

From the reactivity of maintainer of pyinotify, it's better to handle it inside odoo, even if a PR is made upstream.

@Icallhimtest Icallhimtest force-pushed the odoo-dev:10.0-dev-reload-dirty-dve branch 2 times, most recently to ecbb498 Mar 15, 2019

@robodoo robodoo added the CI 🤖 label Mar 15, 2019

Icallhimtest added some commits Mar 14, 2019

[IMP] server.py: --dev=reload with inotify
Add the alternative of inotify instead of watchdog to watch the addons
paths the server was started with.

Reason: watchdog spawns 2 threads per path to watch. When there are a
lot of addons paths, this can become too costly. With inotify we watch
all the repositories in a single thread.

https://github.com/dsoprea/PyInotify

installation:

    pip install inotify
[FIX] server.py: stop the FSWatcher when stopping the server
Avoid potential tracebacks from the FSWatcher's thread being killed.

Drawback: Server shut down can have an extra small delay

(only applies when the --dev=reload option is given)

@Icallhimtest Icallhimtest force-pushed the odoo-dev:10.0-dev-reload-dirty-dve branch from ecbb498 to 0085192 Mar 15, 2019

@robodoo robodoo removed the CI 🤖 label Mar 15, 2019

@Icallhimtest

This comment has been minimized.

Copy link
Contributor Author

Icallhimtest commented Mar 15, 2019

@KangOl alrighty,

1b) applied & tested on my end
2) kept same as watchdog
3) done

Should be good to go.

@antonylesuisse

This comment has been minimized.

Copy link
Contributor

antonylesuisse commented Mar 18, 2019

Make sure it also works for editor that write in place.

@Icallhimtest

This comment has been minimized.

Copy link
Contributor Author

Icallhimtest commented Mar 18, 2019

@antonylesuisse Yeah, it does.

It listens to IN_WRITE or IN_MOVED_TO events. (+ IN_CREATE)

Both swapping or writing in place are covered by this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.