Improve reliability, simplicity and recoverability of BGSAVE #153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fix is a very important one in two ways. Say we have an user A. They go ahead and launch skyd. skyd creates a data.bin file. Now A just deletes the data.bin file for fun. Funny enough, this never causes flock to error! Why? Well because the descriptor/handle is still valid and was just unlinked from the current directory. But this might seem silly since the user exits with a 'successfully saved notice' only to find that the file never existed and all of their data was lost. That's bad. There's a hidden problem in our current approach too, apart from this. Our writing process begins by truncating the old file and then writing to it by placing the cursor at 0. Nice, but what if this operation just crashes. So we lost the current data AND the old data. Not good. This commit does a better thing: it creates a new temporary file, locks it before writing and then flushes the current data to the temporary file. Once that succeeds, it replaces the old data.bin file with the newly created file. This solves both the problems mentioned here for us: 1. No more of the silly error 2. If BGSAVE crashes in between, we can be sure that at least the last data.bin file is in proper shape and not half truncated or so. This commit further moves the background services into their own module(s) for easy management.
Fixes: 1. Our custom runner (drone/.ci.yml) was modified to kill the skyd process once done since this pipeline is not ephemeral. 2. GHA for some reason ignores any error in the test step and proceeds to kill the skyd process without erroring. Since GHA runners are ephemeral, we don't need to do this manually.
ohsayan
added
C-bug
|
@stellaroid r=me |
glydr
approved these changes
May 16, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a complement to #152 that further improves the reliability of BGSAVE. Refer to the commit messages for more information. BGSAVE can now self recover which it was supposed to be able to but due to implementation inconsistencies it wasn't able to.