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

Add an mtime file watcher. #549

Merged
merged 2 commits into from
May 31, 2018
Merged

Conversation

dlorenc
Copy link
Contributor

@dlorenc dlorenc commented May 14, 2018

No description provided.

@dlorenc
Copy link
Contributor Author

dlorenc commented May 14, 2018

This can help us work around the issues caused by inotify-style watchers.

@dlorenc dlorenc added the kokoro:run runs the kokoro jobs on a PR label May 14, 2018
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 14, 2018
@dlorenc
Copy link
Contributor Author

dlorenc commented May 14, 2018

TODO:

  • Figure out the best way to configure which watcher to use
  • Tests
  • benchmarking

@ajbouh
Copy link
Contributor

ajbouh commented May 15, 2018

In other projects, I've been very satisfied with using https://facebook.github.io/watchman/

It's a separate service that's optimized for efficient file watching on major operating systems. It supports features like a logical clock and providing a digest of fes when they change. There's a json or binary protocol that's accessible via a Unix socket.

@r2d4 r2d4 added the wip label May 24, 2018
@dgageot
Copy link
Contributor

dgageot commented May 25, 2018

@dlorenc @r2d4 I think we could get rid of the inotify watcher altogether because it won't go any further. Changing the list of dependencies based on file changes with inotify is too tricky

@dlorenc
Copy link
Contributor Author

dlorenc commented May 25, 2018

@dlorenc @r2d4 I think we could get rid of the inotify watcher altogether because it won't go any further. Changing the list of dependencies based on file changes with inotify is too tricky

SGTM, how about an environment variable to switch between the two for now in-case there is a large perf hit for some workloads? We can switch the default to mtime in this PR but leave inotify until we stop hearing complaints about mtime.

@dlorenc
Copy link
Contributor Author

dlorenc commented May 25, 2018

An env var would be easier than a flag because we can remove it without breaking people that pass it on the cmd line.

@dlorenc dlorenc force-pushed the mtime branch 2 times, most recently from fa89fb4 to 3e24a8f Compare May 25, 2018 16:04
@dlorenc dlorenc changed the title WIP: add an mtime file watcher. Add an mtime file watcher. May 25, 2018
@dlorenc
Copy link
Contributor Author

dlorenc commented May 25, 2018

Dropping the WIP, this should be ready for a look.

I prioritized ease-of-removal of fsnotify rather than doing a full refactor.

@dlorenc
Copy link
Contributor Author

dlorenc commented May 25, 2018

These tests are passing for me but failing in travis, i'll keep debugging:

--- PASS: TestWatch (5.03s)
    --- PASS: TestWatch/watch_unknown_file_mtime (0.00s)
    --- PASS: TestWatch/write_files_mtime (2.01s)
    --- PASS: TestWatch/ignore_file_mtime (2.00s)
    --- PASS: TestWatch/watch_unknown_file_fsnotify (0.00s)
    --- PASS: TestWatch/write_files_fsnotify (0.51s)
    --- PASS: TestWatch/ignore_file_fsnotify (0.51s)

@dlorenc dlorenc force-pushed the mtime branch 2 times, most recently from 8816d5b to b46dfde Compare May 25, 2018 17:14
@dlorenc dlorenc removed the wip label May 25, 2018
@dlorenc
Copy link
Contributor Author

dlorenc commented May 25, 2018

Tests passing now.

@r2d4
Copy link
Contributor

r2d4 commented May 25, 2018

I think we should do some testing with minikube/d4d to make sure this works well. We still have the clock skew issues in minikube.

@dlorenc
Copy link
Contributor Author

dlorenc commented May 25, 2018

I think we should do some testing with minikube/d4d to make sure this works well. We still have the clock skew issues in minikube.

The skew isn't important, we take an initial snapshot then just check to see if the values have changed at all from that.

What exactly do you mean by testing in d4d? Watch happens on the local machine, not in the build step itself.

@dgageot
Copy link
Contributor

dgageot commented May 26, 2018

@dlorenc I'm sorry you have to rebase the code.

This watcher is on by default, but the old one can be enabled by setting:

SKAFFOLD_FILE_WATCHER=fsnotify

The old one will be removed after a release.
@dlorenc
Copy link
Contributor Author

dlorenc commented May 31, 2018

@dgageot anything blocking this?

@dgageot
Copy link
Contributor

dgageot commented May 31, 2018

Absolutely not! Let me merge it
LGTM

@dgageot dgageot merged commit 389905f into GoogleContainerTools:master May 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants