For Loki Sources position files: Use a best effort atomic rename #5772
Conversation
mattdurham
changed the title
mattdurham
requested review from
tpaschalis,
erikbaranowski and
thampiotr
and removed request for
tpaschalis
| //go:build windows | ||
| // +build windows | ||
|
|
||
| // This code is copied from Promtail with minor changes. |
There was a problem hiding this comment.
Can we add a commit sha of what promtail it's copied from? Makes it easier to refresh with changes in the future.
There was a problem hiding this comment.
Sure, I can add that in.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // otherwise open the current one. | ||
| file, err := os.OpenFile(path, os.O_RDWR, 0666) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fileContent, err := io.ReadAll(file) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fileString := string(fileContent) | ||
| // load the current bookmark. | ||
| bm, err := win_eventlog.CreateBookmark(fileString) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &bookMark{ | ||
| handle: bm, | ||
| path: path, | ||
| isNew: fileString == "", | ||
| buf: buf, | ||
| }, nil |
There was a problem hiding this comment.
In this fragment of code few things can still go wrong and possibly the agent will be crashlooping until someone manually repairs the bookmark file or, more likely, deletes it.
Should we instead have on a high-level something like this?
func newBookmark() {
if not fileExists {
createNewOne()
} else {
err := useExistingFile()
if err != nil {
renameCorruptedFile()
createNewOne()
}
}
}
The idea being that if we detect a corrupted file, we attempt to recover.
There was a problem hiding this comment.
If we error on the create bookmark then we could call it again with a blank string to force a new bookmark.
|
Unit tests are hard since it would need to stop during the write to disk. |
github-actions
bot
added
the
frozen-due-to-age
PR Description
This switches the bookmark and positions file in loki.sources to use a more resilient method. This is primarily an issue on windows where a reboot on writing to the file causes an issue. The default os.rename in windows is not reliable. The atomic package uses a much more reliable method though there are issues where it doesnt work. These are generally limited to network file systems and non-ntfs. This should cover 99.9% of our cases without problem.
Most of the changes are fork lifting from promtail.
Which issue(s) this PR fixes
Closes #4916
Notes to the Reviewer
PR Checklist