cri: write generated CNI config atomically #8609
Conversation
samuelkarp
added
cherry-pick/1.6.x
cherry-pick/1.7.x
|
Switching to draft; this will need to be conditioned off for Windows. |
|
Truly impeccable timing from myself 🤣. Whenever this is un-drafted I'll take a peek again |
|
@dcantah Comments are appreciated and useful too 😄 |
|
Believe atomicity can be achieved on windows with a little wrapper around MoveFileEx (in place of os.Rename). You need the |
|
|
|
Mmm, my university wisdom might be misguided now (or perhaps always was LOL). This was a pretty common tactic in many "write files atomically" libraries, and even stdlibs:
This area seems completely fraught with peril though. There's no clear picture or guarantee on docs of atomicity, and everyone seems to just almost pray that their use of MoveFile or MoveFileTransacted (deprecated now) works. A fun read: https://www.postgresql.org/message-id/CAPpHfds9trA6ipezK3BsuuOSQwEmESiqj8pkOxACFJpoLpcoNw%40mail.gmail.com. A recommended approach as recent as 2021 on an article from msft recommends ReplaceFile now for files with "document-like" data. ReplaceFile at least has the advantage that it preserves the following as well:
This is much deeper than my drive-by comment at 1 AM led on 🤣 |
|
@kevpar Curious if anyone from the FS teams could give a blessing on some approach for atomic writes on Windows |
|
I've refactored this a bit to encapsulate the rename-file logic out into its own package and keep the CRI code a bit cleaner.
My curriculum didn't cover Windows, so this is all a bit unfamiliar to me 🤣 @dcantah, I didn't attempt to write a Windows codepath yet. Do you think we should:
|
|
I'd be fine with punting given we have an easy win on unix, but I'm curious if Kevin has any insights. At the very least lets add a TODO to consistentfile's Windows impl to find the right approach |
Done |
|
The filesystem doesn't provide any way to guarantee atomicity for this kind of thing. I think the best you can do is something like |
@kevpar Is there documentation somewhere (or do you have guidance you can share) covering the tradeoffs/recommendation to choose for this use-case? |
I hadn't seen that one either, but I was trying to avoid an interface with |
Thanks for the comment. SGTM. |
samuelkarp
force-pushed
the
issue-8607
branch
2 times, most recently
from
b54d795 to
5d6b8a8
Compare
|
As part of replacing |
Certain files may need to be written atomically so that partial writes are not visible to other processes. On Unix-like platforms such as Linux, FreeBSD, and Darwin, this is accomplished by writing a temporary file, syncing, and renaming over the destination file name. On Windows, the same operations are performed, but Windows does not guarantee that a rename operation is atomic. Partial/inconsistent reads can occur due to: 1. A process attempting to read the file while containerd is writing it (both in the case of a new file with a short/incomplete write or in the case of an existing, updated file where new bytes may be written at the beginning but old bytes may still be present after). 2. Concurrent goroutines in containerd leading to multiple active writers of the same file. The above mechanism explicitly protects against (1) as all writes are to a file with a temporary name. There is no explicit protection against multiple, concurrent goroutines attempting to write the same file. However, atomically writing the file should mean only one writer will "win" and a consistent file will be visible. Signed-off-by: Samuel Karp <[email protected]>
The 10-containerd-net.conflist file generated from the conf_template should be written atomically so that partial writes are not visible to CNI plugins. Use the new consistentfile package to ensure this on Unix-like platforms such as Linux, FreeBSD, and Darwin. Fixes containerd#8607 Signed-off-by: Samuel Karp <[email protected]>
Signed-off-by: Samuel Karp <[email protected]>
Signed-off-by: Samuel Karp <[email protected]>
thaJeztah
added
cherry-picked/1.7.x
thaJeztah
added
cherry-picked/1.6.x
7 participants
The 10-containerd-net.conflist file generated from the conf_template should be written atomically so that partial writes are not visible to CNI plugins. This is accomplished by writing a temporary file, syncing, and renaming over the destination file name.
Partial/inconsistent writes can occur due to:
The above mechanism explicitly protects against (1) as all writes are to a file with a temporary name.
There is no explicit protection against multiple, concurrent CRI calls to UpdateRuntimeConfig. However, atomically writing the generated file should mean only one writer will "win" and a consistent file will be visible.
Fixes #8607