Skip to content
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

promtail: write positions to new file first, move to target location afterwards #1304

Merged
merged 1 commit into from
Nov 21, 2019
Merged

promtail: write positions to new file first, move to target location afterwards #1304

merged 1 commit into from
Nov 21, 2019

Conversation

pstibrany
Copy link
Member

promtail uses positions file to write positions in watched log files. It's doing so in an unsafe way, overwriting existing file. If error happens while writing to the file, file ends up being inconsistent. And old version is gone.

This PR changes positions writing, so that it's first written to new file, and only if that succeeds, is it move to target position. New file is file in the same directory, but with "-new" appended.

os.Rename guarantees that it replaces existing file, or it fails, but it cannot leave file inconsistent.

…rds.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@rfratto
Copy link
Member

rfratto commented Nov 21, 2019

If the rename fails, should the new file be deleted? WDYT?

@pstibrany
Copy link
Member Author

If the rename fails, should the new file be deleted? WDYT?

Nah, it will be overwritten on next try, or can be left there for inspection / manual rename by admin.

@roidelapluie
Copy link
Contributor

Should we use https://github.com/natefinch/atomic/ ?

@pstibrany
Copy link
Member Author

pstibrany commented Nov 21, 2019

Should we use https://github.com/natefinch/atomic/ ?

From what I can tell, it's doing exactly the same as Go stdlib: calls rename on Unix/Linux, and uses same MoveFileEx system call on Windows with MOVEFILE_REPLACE_EXISTING flag that Go stdlib does.

Perhaps Go didn't do that in the past? This lib seems to be 5+ years old.

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice catch!

@rfratto rfratto merged commit fd25e6d into grafana:master Nov 21, 2019
@pstibrany pstibrany deleted the write-positions-safely branch November 21, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants