watcher: notify when watched files are modified by jparise · Pull Request #6215 · envoyproxy/envoy

jparise · 2019-03-08T00:16:51Z

The file system watcher previously only reported file movement events,
but it's also useful to track changes to watched files (to trigger a
reload, for example).

This change adds support for tracking file modification events reports
them using a new Watcher::Events::Modified. Both inotify (IN_MODIFY) and
kqueue (NOTE_WRITE) are supported.

This also reduces the set of watched inotify events to just the ones we
need: IN_MODIFY | IN_MOVED_TO.

Risk Level: Low (nothing is using this new event type yet)
Testing: New unit test (tested on macOS and Linux)

Signed-off-by: Jon Parise jon@pinterest.com

The file system watcher previously only reported file movement events, but it's also useful to track changes to watched files (to trigger a reload, for example). This change adds support for tracking file modification events reports them using a new Watcher::Events::Modified. Both inotify (IN_MODIFY) and kqueue (NOTE_WRITE) are supported. This also reduces the set of watched inotify events to just the ones we need: IN_MODIFY | IN_MOVED_TO. Signed-off-by: Jon Parise <jon@pinterest.com>

jparise · 2019-03-08T00:53:45Z

Two further considerations:

OpenBSD (kqueue) also provides a NOTE_TRUNCATE event type. FreeBSD and Darwin do not. Should be conditionally compile support for this, as well?
I haven’t done anything to detect modified files within watched directories. I don’t know how well that’s supported by kqueue and inotify, but I could research what’s involved.

jparise · 2019-03-10T20:27:41Z

One other note regarding the motivation behind this change:

External systems that write watched files should do so using an atomic rename operation to avoid races. We've found a few systems that aren't doing that, and we intend to fix them. Once that's done, Envoy's existing Events::MovedTo support should work fine for our use cases.

This change has been useful in the meantime, but I would understand if it isn't something that we'd want to maintain in Envoy proper.

source/common/filesystem/inotify/watcher_impl.cc

stale · 2019-03-17T21:01:19Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jmarantz · 2019-03-17T21:07:27Z

/wait
sorry this slipped through the cracks; I'll get to this tomorrow.

jmarantz

Many apologies for the delay. Code looks great. A few comments about tests.

source/common/filesystem/kqueue/watcher_impl.cc

jmarantz · 2019-03-20T18:23:45Z

test/common/filesystem/watcher_impl_test.cc

+  std::ofstream file(TestEnvironment::temporaryPath("envoy_test/watcher_target"));
+
+  WatchCallback callback;
+  EXPECT_CALL(callback, called(Watcher::Events::Modified));


Move this EXPECT_CALL to right before where you append to the file?

Also I think you also want to declare testing::InSequence as the first line of this test.

I followed the pattern used in the other tests in this suite, which all appear to declare their EXPECT_CALLs right after before watcher->addWatch() is called. Given that, would you like me to leave this as-is, change it for just this new test, or revise all of the existing tests?

As for testing::InSequence, I don't think that would make much of a difference here because this test only makes a single assertion/expectation. I'm happy to add it for correctness, though. Would you like me to add that to the existing tests that would benefit from it, too?

I'm not sure about InSequence but I think if you move the EXPECT_CALL to right before line 109, then the test would fail if the Modified event were called in the first call to dispatcher_->run().

It would be nice to also make similar improvements in precision to the other tests but if that winds up taking you down a rathole I'd be happy with this one.

Makes sense to me. 👍

source/common/filesystem/inotify/watcher_impl.cc

Signed-off-by: Jon Parise <jon@pinterest.com>

jparise

Thanks for the review! I added a few questions that will help inform how I address your feedback.

source/common/filesystem/kqueue/watcher_impl.cc

jparise · 2019-03-21T17:26:35Z

test/common/filesystem/watcher_impl_test.cc

+  std::ofstream file(TestEnvironment::temporaryPath("envoy_test/watcher_target"));
+
+  WatchCallback callback;
+  EXPECT_CALL(callback, called(Watcher::Events::Modified));


I followed the pattern used in the other tests in this suite, which all appear to declare their EXPECT_CALLs right after before watcher->addWatch() is called. Given that, would you like me to leave this as-is, change it for just this new test, or revise all of the existing tests?

As for testing::InSequence, I don't think that would make much of a difference here because this test only makes a single assertion/expectation. I'm happy to add it for correctness, though. Would you like me to add that to the existing tests that would benefit from it, too?

source/common/filesystem/inotify/watcher_impl.cc

stale · 2019-03-28T18:14:35Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Jon Parise <jon@pinterest.com>

* master: (137 commits) test: router upstream log to v2 config stubs (envoyproxy#6499) remove idle timeout validation (envoyproxy#6500) build: Change namespace of chromium_url. (envoyproxy#6506) coverage: exclude chromium_url (envoyproxy#6498) fix(tracing): allow 256 chars in path tag (envoyproxy#6492) Common: Introduce StopAllIteration filter status for decoding and encoding filters (envoyproxy#5954) build: update PGV url (envoyproxy#6495) subset lb: avoid partitioning host lists on worker threads (envoyproxy#6302) ci: Make envoy_select_quiche no-op. (envoyproxy#6393) watcher: notify when watched files are modified (envoyproxy#6215) stat: Add counterFromStatName(), gaugeFromStatName(), and histogramFromStatName() (envoyproxy#6475) bump to 1.11.0-dev (envoyproxy#6490) release: bump to 1.10.0 (envoyproxy#6489) hcm: path normalization. (#1) build: import manually minified Chrome URL lib. (envoyproxy#3) codec: reject embedded NUL in headers. (envoyproxy#2) Added veryfication if path contains query params and add them to path header (envoyproxy#6466) redis: basic integration test for redis_proxy (envoyproxy#6450) stats: report sample count as an integer to prevent loss of precision (envoyproxy#6274) Added VHDS protobuf message and updated RouteConfig to include it. (envoyproxy#6418) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

mattklein123 assigned jmarantz Mar 10, 2019

jparise commented Mar 10, 2019

View reviewed changes

source/common/filesystem/inotify/watcher_impl.cc Show resolved Hide resolved

jparise mentioned this pull request Mar 10, 2019

inotify watcher performance issues #3308

Open

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 17, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 17, 2019

repokitteh-read-only bot added the waiting label Mar 17, 2019

jmarantz reviewed Mar 20, 2019

View reviewed changes

watcher: declare kqueue flags using u_int

a408688

Signed-off-by: Jon Parise <jon@pinterest.com>

repokitteh-read-only bot removed the waiting label Mar 21, 2019

jparise commented Mar 21, 2019

View reviewed changes

stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Mar 28, 2019

Move EXPECT_CALL right before triggering condition

a8ef26c

Signed-off-by: Jon Parise <jon@pinterest.com>

jmarantz approved these changes Apr 4, 2019

View reviewed changes

mattklein123 merged commit ba1ecbb into envoyproxy:master Apr 5, 2019

jparise deleted the watcher-modified branch April 5, 2019 23:31

mattklein123 mentioned this pull request Jun 11, 2020

Fix runtime config hot reload envoyproxy/ratelimit#150

Closed

Conversation

jparise commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jparise commented Mar 8, 2019

Uh oh!

jparise commented Mar 10, 2019

Uh oh!

Uh oh!

stale bot commented Mar 17, 2019

Uh oh!

jmarantz commented Mar 17, 2019

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

jparise Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

jparise Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jparise left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jparise Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stale bot commented Mar 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jparise commented Mar 8, 2019 •

edited

Loading