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.

Sign up for GitHub

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

Jump to bottom

Commit registry writes to stable storage to avoid corrupt registry files #6877

Merged
merged 2 commits into from
Apr 26, 2018
Merged

Commit registry writes to stable storage to avoid corrupt registry files #6877

merged 2 commits into from
Apr 26, 2018

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Apr 16, 2018 •
edited
Loading

Called Sync before closing the file in order to flush everything to disk before closing the registry file.

Closes #6792

@kvch kvch added in progress Pull request is currently in progress. Filebeat Filebeat labels Apr 16, 2018
@urso
Copy link

urso commented Apr 16, 2018

I don't think we need O_DIRECT. Alternatively to the different Open functions, we could call Sync before Close. This way we don't need the specialized windows vs. Posix code. The (*os.File).Sync code translates to the correct syscalls (fsync on Unix, FlushFileBuffers on Windows).

Something went wrong. Almost all filebeat tests are failing.

@urso urso requested a review from andrewkroh April 16, 2018 19:06
@andrewkroh
Copy link
Member

I agree with @urso. I see that there is a missing fsync in Winlogbeat before the rename.

@ruflin
Copy link
Member

ruflin commented Apr 17, 2018

PR will need a changelog.

@kvch
Copy link
Contributor Author

kvch commented Apr 17, 2018

@urso @andrewkroh Thanks for the early review! I assumed that something is not right that's why I haven't added the label "review" to my PR. :)

@kvch kvch force-pushed the fix/filebeat/write-state-directly-to-disk branch from 7caee89 to 98ef1fe Compare April 17, 2018 13:26
@kvch kvch changed the title Write state changes directly to disk both in Filebeat and Winlogbeat Commit registry writes to stable storage to avoid corrupt registry files Apr 17, 2018
@kvch kvch added review in progress Pull request is currently in progress. and removed in progress Pull request is currently in progress. labels Apr 17, 2018
@kvch
Copy link
Contributor Author

kvch commented Apr 17, 2018 •
edited
Loading

Apparently, the CIs are on fire...
EDIT: No, everything is green.

@ruflin
Copy link
Member

ruflin commented Apr 18, 2018

The original change to windlogbeat has been quite a bit more extensive: #2434 @urso I wonder if f.Sync() is going to cover the same cases and if yes, we should probably also simplify it in winlogbeat or even better extract it to have only 1 way of doing it.

@kvch
Copy link
Contributor Author

kvch commented Apr 23, 2018

Please, image that I have removed the label "in progress" from this PR. :D

@andrewkroh andrewkroh removed the in progress Pull request is currently in progress. label Apr 24, 2018
andrewkroh
andrewkroh approved these changes Apr 26, 2018
View reviewed changes
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

We could also make the same change in Winlogbeat and remove the specialized code I added.

@andrewkroh andrewkroh merged commit 4d2150d into elastic:master Apr 26, 2018
@urso urso mentioned this pull request May 2, 2018
Merged
@urso urso added the needs_backport PR is waiting to be backported to other branches. label May 4, 2018
urso pushed a commit to urso/beats that referenced this pull request May 4, 2018
@kvch
Commit registry writes to stable storage to avoid corrupt registry fi…
b84237f 
…les (elastic#6877)

* fsync contents of registry file to storage

(cherry picked from commit 4d2150d)
@urso urso mentioned this pull request May 4, 2018
Merged
@urso urso added v6.3.0 and removed needs_backport PR is waiting to be backported to other branches. labels May 4, 2018
ruflin pushed a commit that referenced this pull request May 7, 2018
@ruflin
Commit registry writes to stable storage to avoid corrupt registry fi…
25343b4 
…les (#6877) (#7019)

* fsync contents of registry file to storage

(cherry picked from commit 4d2150d)
@urso urso mentioned this pull request Jun 18, 2018
Merged
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
@ruflin
Commit registry writes to stable storage to avoid corrupt registry fi…
b72da66 
…les (elastic#6877) (elastic#7019)

* fsync contents of registry file to storage

(cherry picked from commit 6babaae)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkroh andrewkroh andrewkroh approved these changes

Assignees
No one assigned
Labels
Filebeat Filebeat review v6.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kvch @urso @andrewkroh @ruflin