Watch for config file changes in fileprovider #5945
Conversation
f947437 to
e46c443
Compare
Codecov Report
@@ Coverage Diff @@
## main #5945 +/- ##
==========================================
+ Coverage 91.90% 91.95% +0.05%
==========================================
Files 200 201 +1
Lines 12414 12568 +154
==========================================
+ Hits 11409 11557 +148
- Misses 793 797 +4
- Partials 212 214 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
d4efd80 to
6fd2146
Compare
There was a problem hiding this comment.
I love this idea, but my gut says we should not make it a default for fileprovide initially. The current expectation for the file provide (and performance expectations) is that the file is not watched. It feels appropriate to either hide this capability behind a feature gate or as a different provider/flag. That way existing customers have to opt-in for now.
Sounds reasonable to me. It'd be pretty straightforward to gate this, considering all the new functionality is hidden behind a single conditional right now. |
6fd2146 to
287c510
Compare
|
In all honesty, after thinking about it more, different usecases may want to have this enabled or disabled in general. In Kubernetes, for example, you typically want to treat Pods as immutable, and don't want them to quietly reload configuration just because you made a change to a ConfigMap. Arguably it should be the users' responsibility to make sure Kubernetes doesn't reload the file inside the Pod, but that's an obscure enough topic that I'm not confident in users not shooting themselves in the foot with it. I think I'm going to open an issue about this so we can more carefully weigh the pros and cons. This probably applies more generally to configuration reloading for any provider, this one is simply the first in line to have it implemented. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
| } | ||
|
|
||
| func metadataEqual(first os.FileInfo, second os.FileInfo) bool { | ||
| return first.Size() == second.Size() && first.ModTime() == second.ModTime() |
There was a problem hiding this comment.
nit: some file file systems might be mounted with the norelatime option, which can lead to funny and unlikely bugs.
There was a problem hiding this comment.
Will it? It was my understanding that norelatime only affects access time, whereas I'm checking modification time here.
There was a problem hiding this comment.
My mistake, you are right. this will only affect the access time.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
Make the file config provider watch for changes to the config file.
This is done via polling the file every second, with an additional grace period of two seconds for actually delivering updates to the collector. We're not using notification mechanisms provided by the OS because they're unreliable or annoyingly complex to make work for some popular use cases - most notably Kubernetes ConfigMaps. As we don't need frequent updates and our watched files are small, polling is both performant and easier to understand than, say, inotify.
This change is more complex than it technically needs to be because it tries to reasonably deal with misbehaving users. For example, it'll quietly accept a user calling
.Shutdownbefore.Close:and
r.Close()is even thread-safe. If we're ok with the above resulting in a panic or a deadlock, I can make both the provider and the filewatcher simpler.Link to tracking Issue: #273
Testing:
Added additional tests for the provider and separate unit tests for the filewatcher. I also did a couple manual e2e tests of the more interesting edge cases, like network filesystems and deep symlink chains.