Fix 'invalid magic number 0' bug#11338
Conversation
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
be6a036 to
2ae668c
Compare
|
I think this fix is good to detect 0 magic number segment file and remove it. But I also think we should maybe call From what I've read, cutting a new segment file in head chunk involves
According to Linux's close man page data are not guaranteed to be written to disk, and I think Go's File.Close pretty much just make a underlying OS system call. I don't think the I think in this PR maybe we can add a I also see the comment for I am wondering why doing fsync is not desirable? Is it because of perf? |
tsdb/chunks/head_chunks.go
Outdated
| return files, errors.Wrap(err, "read file during last head chunk file repair") | ||
| } | ||
|
|
||
| remove = binary.BigEndian.Uint32(buf) == 0 |
There was a problem hiding this comment.
I think we should log something here to tell the user that "hey, we encountered magic number is 0 error".
There was a problem hiding this comment.
Hmm, I don't think adding this log line is useful or actionable. Also, this package does not log anywhere and I would like to keep it that way. Deleting the last file is not a big issue.
There was a problem hiding this comment.
Yea, if we don't want to do fsync I don't think the log will be useful too. I mainly suggested this to see if doing fsync would fix this problem too :)
|
Ok, I see the argument for not doing a sync after writing magic number is noted in #8061 (comment), which I can agree because I know But, I wonder sync 4 bytes of magic number would take seconds; if it indeed take seconds, isn't the disk overloaded and you have bigger problem? I get the idea of "fixing" the problem at "read path" to avoid fsync when cutting new segment file in head chunk but;
|
tsdb/chunks/head_chunks.go
Outdated
| if info.Size() == 0 { | ||
|
|
||
| remove := info.Size() == 0 // Empty file. | ||
| if info.Size() >= MagicChunksSize { |
There was a problem hiding this comment.
nit: I think it might be better to move this if block to a helper function so the code can read like:
remove := info.Size() ==0
remove = !remove && noMagicNubmer(...)
if remove {
....
}
There was a problem hiding this comment.
Since this logic is only used here and that repairLastChunkFile is not too huge, I prefer to not create another function here.
There was a problem hiding this comment.
Make sense; you have a good point.
|
It is not just about syncing the magic number. In some OS the whole file is preallocated even before you write the magic number (for example Windows). So any abrupt shutdown before you sync the magic number still needs to be detected (you can see that we already had a code in place to detect empty files, but that is apparently not enough). Also, fsync being on the hot path can be a dealbreaker (though with the addition of async queue and another open work, it won't be in hot path in future). This repair mechanism is acceptable and I don't see it as a maintainence burden here; a repair mechanism for database is desirable instead of a hard fail IMO (you cannot expect your hardware to work properly all the time, issues happen). Additionally, if there was no magic number, it also means there were no chunks flushed to the file, so flushing the magic number does not help us in any way. If you see, trying to sync magic number still leaves a room for the error to occur. |
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
|
@codesome yes, you are absolutely right, what you did in this PR is needed; I wouldn't remove it, it's awesome fix :)
Right, when you preallocate, I think different OS may do different thing, some may fill it with If we don't do fysnc, then I can see how the actual segment file can be empty, filled with In other words, I think if we do fsync before renaming the file, we should avoid empty file problem as well as magic number being 0 problem.
My question is more on why having fsync on hot path is a dealbreaker, is the fsync performance really that bad for typical use cases? If we can do some quick perf test on it it would be nice. To put it more bluntly, I wonder if not doing a fsync is premature optimization, but of course I know I can be wrong :-) The argument of fsync can hurt performance totally make sense, but I am just if it really hurts "that much" :) |
|
It is already a problem without fsync on high loads: when the ChunkDiskMapper is trying to flush/mmap many chunks to the disk and it sometimes causes high write latencies because many chunks are waiting to be flushed (this happens on hot path). Trying to fsync (which can happen when writing chunks because you need to go to the next file) will only make it worse. Again, file getting corrupted like this during abrupt shutdown is not a problem, we can gracefully recover from it without any data loss. Also, trying to fsync does not take the problem away: the abrupt shutdown can happen right when trying to fsync. So I don't see an benefit of a fsync here. |
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
I see, thanks for the follow up :) |
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> (cherry picked from commit 83d738e)
OCPBUGS-2640: Fix 'invalid magic number 0' bug (prometheus#11338)
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> (cherry picked from commit 83d738e)
|
I observed the following error on v2.45.0 |
Closes #7397
We already had a repair mechanism for this file if the file was empty, but it can happen that file is not 0 size but filled with 0s. So this PR extends the repair logic and repairs for file with 0s as well.
I would like this to be a part of v2.39