-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
file watcher to be used for configuration auto-reload feature #12301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a cleanup suggestion nit.
func randomStr(length int) string { | ||
const charset = "abcdefghijklmnopqrstuvwxyz" + | ||
"ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" | ||
var seededRand *rand.Rand = rand.New( | ||
rand.NewSource(time.Now().UnixNano())) | ||
b := make([]byte, length) | ||
for i := range b { | ||
b[i] = charset[seededRand.Intn(len(charset))] | ||
} | ||
return string(b) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this helper to generate random strings, but if we already have something similar I could reuse.
agent/config/file_watcher.go
Outdated
} | ||
|
||
// Stop the file watcher | ||
// calling Stop multiple times is a noop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also might be useful to say "Stop must only be called after Start" since the sequence of Stop, Start, Stop
would leave the goroutine running.
require.NoError(t, assertEvent(file.Name(), w.EventsCh, defaultTimeout)) | ||
} | ||
|
||
func TestEventWatcherRead(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this testing that reading from a file (which may touch atime
) doesn't notify you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this should not trigger any event but just in case another filesystem do something special.
agent/config/file_watcher_test.go
Outdated
_ = w.Stop() | ||
}() | ||
|
||
file.Chmod(0777) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may error.
recreated, err := os.Create(filepath) | ||
require.NoError(t, err) | ||
_, err = recreated.WriteString("config 2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's interesting you include this test setup since the order of operations here is actually the "unsafe" one you discuss elsewhere. In this case the filesystem sees:
- file name=X is erased
- file name=X is created and is empty
- file name=X is updated to not be empty
as 3 separate events, i guess here you're only going to see one of these. You could i guess end the test with an assert that times out to prove that you don't wake up more times than you expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some non-blocking remaining comments.
1a3b21d
to
479b602
Compare
Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/590786. |
This PR add a file watcher that combine the use of https://github.com/fsnotify/fsnotify and a reconcile loop that id files by inode ID in linux and Creation Time in Windows to detect if file has been recreated and call a handler with the event accordingly.
For the time being this is not yet used in Consul but the the intent is to use it to automatically reload some of the configuration options when the file get recreated.
The 2 events that could trigger the handlers are:
Symlinks are not supported as fsnotify/fsnotify and the go filesystem packages are not consistent on how to treat symlinks and could get different behaviours (follow or not follow) depending on OS. If a symlink is added an error will be returned.