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

Ensure that config files are placed atomically #103

Closed
dannysauer opened this issue Apr 9, 2019 · 5 comments
Closed

Ensure that config files are placed atomically #103

dannysauer opened this issue Apr 9, 2019 · 5 comments

Comments

@dannysauer
Copy link
Contributor

dannysauer commented Apr 9, 2019

Per the discussion linked below, "someone" should verify that the file copy process is performing atomic copies. Namely, that it's creating a $tempname on the destination (in the same directory as $realname, so the filesystem is the same and the actual rename syscall can be used), populating the contents to that temp name, and then renaming $tempname to $realname.

Originally posted by @dannysauer in https://github.com/SUSE/caaspctl/pull/98/review_comment/create
(renamed to #98)

@ereslibre
Copy link
Contributor

ereslibre commented Apr 9, 2019

in the same directory as $realname, so the filesystem is the same and the actual rename syscall can be used

Funnily enough this was the approach that salt followed and that brought us some headaches when we were writing manifests in /etc/kubernetes/manifests. Since the kubelet was listening for changes in the directory, sometimes, when salt wrote tempfiles in the same directory it was making the kubelet behave in weird ways (sometimes we saw two haproxy instances for example).

While it seems the safest bet, using the target directory to write the tempfile can have unexpected side effects.

@dannysauer
Copy link
Contributor Author

I'm going to call that an error in kublet, given that using a tempname in the target directory is pretty standard practice in all config management tools I'm aware of. :D

In that kind of rare situation, doing a stat and choosing a target which has the same device number is really what's important. Any POSIX filesystem guarantees that a rename operation is atomic, so there is never a time when the target file is partially-populated. It's either all the new file, or (in the case of a rename) all the old file. When the device number isn't the same, though, mv (as well as most filesystem convenience libraries) falls back to doing a file copy, and that is not atomic.

Regarding the specific issue, it was, in fact, a bug in kubernetes. As with most systems like that, there is an ignore pattern. k8s ignores dot files, so you can do things like edit a manifest using vim without having the .swap file picked up as a new manifest. :D The defect was fixed in https://github.com/kubernetes/kubernetes/pull/45111. I strongly suspect that a leading period will work for anything we deal with, specifically because of vim's default behavior. Surely anything we're editing, someone somewhere has previously edited it with vim... :)

@evrardjp
Copy link
Contributor

Does this still apply? I am looking to cleanup all the cruft in issues :)

@dannysauer
Copy link
Contributor Author

Following up. One of us will either close or do some research.

@dannysauer
Copy link
Contributor Author

This is still technically "a thing", but somewhat insignificant given future direction. Closing.

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

No branches or pull requests

3 participants