filewatcher: Implement support for directories #561
Conversation
fishy
requested review from
kylelemons,
pacejackson and
ghirsch-reddit
and removed request for
a team
filewatcher/filewatcher.go
Outdated
| } else { | ||
| r.data.Store(&atomicData{ | ||
| data: d, | ||
| mtime: mtime, | ||
| }) | ||
| // remove all previously watched files | ||
| for _, path := range watchedFiles { |
There was a problem hiding this comment.
loop through watcher.WatchedFiles() or whatever instead?
There was a problem hiding this comment.
that's actually added by a fork of the original fsnotify package we were using, but since the original one is already stopped developing it's time to switch to the fork.
| // of fs events happens (for example, when multiple files within the | ||
| // directory changed). | ||
| if timer == nil { | ||
| timer = time.AfterFunc(fsEventsDelay, forceReload) |
There was a problem hiding this comment.
there's still a race condition here it looks like... if it takes longer than 200ms for the files to get written, and then you have an error watching the file(s), I think it can cause the watcher to effectively "forget" about the files forever. It doesn't currently look to me like the polling interval will recover, though I may be missing code that would make it do so.
There was a problem hiding this comment.
we only remove and readd watched files after parser returned success, we also log and ignore all failures of adding files to watch, so failure to watch one file would not affect other files (but yes that particular file could be forgotten forever).
if it takes longer than 200ms for the files to get written
The intended usage of filewatcher is really for those rename swaps, not really open the files one-by-one and write the updates (we can probably add some more documents to clarify on that). and if someone really is using it to watch directories that's updated one-by-one, the delay is still configurable so they can configure it to be a larger number to mitigate those risks.
There was a problem hiding this comment.
So the problem that I forsee is that this is used to monitor a CSI driver mount point, and those currently (and for a long time since they haven't even fixed it yet) rm the files before writing the new ones, it seems very easy for a lot of or large files to exceed 200ms, especially given the issues we've seen with iops being exhausted during pod churn.
I recognize that this is pretty hard to do though, and this is already a general purpose API, so we're slightly tied to it. This is why for v2 we're not using a general purpose library, so we can paper over these issues. In this case, we can do so because we know the exact file paths we expect to not forget.
There was a problem hiding this comment.
yea, the k8s implementation is not great :/
do you want to make the default longer? I do not want to make it too long mainly to not make the secrets test to sleep for too long (we currently don't expose the api to customize it so it has to use the default one). but maybe 1s is not too bad?
fishy
force-pushed
the
directory-watch
branch
2 times, most recently
from
669f2b6
to
cbf06a7
Compare
filewatcher/filewatcher.go
Outdated
| // Please note that DirParser should always return the consistent type. | ||
| // Inconsistent type will cause panic, as does returning nil data and nil error. |
There was a problem hiding this comment.
| // Please note that DirParser should always return the consistent type. | |
| // Inconsistent type will cause panic, as does returning nil data and nil error. | |
| // Please note that a DirParser must return a consistent type. | |
| // Inconsistent types will cause a panic, as does returning nil data and nil error. |
Why would a nil error cause a panic?
There was a problem hiding this comment.
That's the caveat from using atomic.Value:
Store sets the value of the Value to x. All calls to Store for a given Value must use values of the same concrete type. Store of an inconsistent type panics, as does Store(nil).
We can bypass this by doing something similar to 316e844, but I don't think we should do that unless absolutely necessary.
filewatcher/filewatcher.go
Outdated
| for _, path := range watcher.WatchList() { | ||
| watcher.Remove(path) | ||
| } | ||
| // then readd all new files to watch |
There was a problem hiding this comment.
| // then readd all new files to watch | |
| // then read all new files to watch |
filewatcher/filewatcher.go
Outdated
| // Optional, the delay between fs events and actually read and parse the | ||
| // changes. |
There was a problem hiding this comment.
| // Optional, the delay between fs events and actually read and parse the | |
| // changes. | |
| // Optional, the delay between receiving the fs events and actually reading and parsing the | |
| // changes. |
filewatcher/filewatcher.go
Outdated
| // Optional, the delay between fs events and actually read and parse the | ||
| // changes. | ||
| // | ||
| // It's used to avoid short burst of fs events (for example, when watching a |
There was a problem hiding this comment.
| // It's used to avoid short burst of fs events (for example, when watching a | |
| // It's used to avoid short bursts of fs events (for example, when watching a |
fishy
force-pushed
the
directory-watch
branch
from
cbf06a7
to
e98a2d9
Compare
This replaces the filewatcher part of changes of reddit#472.
fishy
force-pushed
the
directory-watch
branch
from
e98a2d9
to
4b304ab
Compare
This replaces the filewatcher part of changes of
#472.