integrate pkg/atomicwriter #185
Merged
Conversation
This file contains hidden or 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
Signed-off-by: Tonis Tiigi <[email protected]>
Perform chmod before rename with the atomic file writer. Ensure writeErr is set on short write and file is removed on write error. Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Fixes case where shutdown occurs before content is synced to disked on layer creation. This case can leave the layer store in an bad state and require manual recovery. This change ensures all files are synced to disk before a layer is committed. Any shutdown that occurs will only cause the layer to not show up but will allow it to be repulled or recreated without error. Added generic io logic to ioutils package to abstract it out of the layer store package. Signed-off-by: Derek McGowan <[email protected]>
Signed-off-by: Cristian Staretu <[email protected]>
Signed-off-by: Daniel Nephin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
Replace gometalinter with golangci-lint
The io/ioutil package has been deprecated in Go 1.16. This commit replaces the existing io/ioutil functions with their new definitions in io and os packages. Signed-off-by: Eng Zer Jun <[email protected]>
pkg/directory/directory.go:9:49: empty-lines: extra empty line at the start of a block (revive)
pkg/pubsub/publisher.go:8:48: empty-lines: extra empty line at the start of a block (revive)
pkg/loopback/attach_loopback.go:96:69: empty-lines: extra empty line at the start of a block (revive)
pkg/devicemapper/devmapper_wrapper.go:136:48: empty-lines: extra empty line at the start of a block (revive)
pkg/devicemapper/devmapper.go:391:35: empty-lines: extra empty line at the end of a block (revive)
pkg/devicemapper/devmapper.go:676:35: empty-lines: extra empty line at the end of a block (revive)
pkg/archive/changes_posix_test.go:15:38: empty-lines: extra empty line at the end of a block (revive)
pkg/devicemapper/devmapper.go:241:51: empty-lines: extra empty line at the start of a block (revive)
pkg/fileutils/fileutils_test.go:17:47: empty-lines: extra empty line at the end of a block (revive)
pkg/fileutils/fileutils_test.go:34:48: empty-lines: extra empty line at the end of a block (revive)
pkg/fileutils/fileutils_test.go:318:32: empty-lines: extra empty line at the end of a block (revive)
pkg/tailfile/tailfile.go:171:6: empty-lines: extra empty line at the end of a block (revive)
pkg/tarsum/fileinfosums_test.go:16:41: empty-lines: extra empty line at the end of a block (revive)
pkg/tarsum/tarsum_test.go:198:42: empty-lines: extra empty line at the start of a block (revive)
pkg/tarsum/tarsum_test.go:294:25: empty-lines: extra empty line at the start of a block (revive)
pkg/tarsum/tarsum_test.go:407:34: empty-lines: extra empty line at the end of a block (revive)
pkg/ioutils/fswriters_test.go:52:45: empty-lines: extra empty line at the end of a block (revive)
pkg/ioutils/writers_test.go:24:39: empty-lines: extra empty line at the end of a block (revive)
pkg/ioutils/bytespipe_test.go:78:26: empty-lines: extra empty line at the end of a block (revive)
pkg/sysinfo/sysinfo_linux_test.go:13:37: empty-lines: extra empty line at the end of a block (revive)
pkg/archive/archive_linux_test.go:57:64: empty-lines: extra empty line at the end of a block (revive)
pkg/archive/changes.go:248:72: empty-lines: extra empty line at the start of a block (revive)
pkg/archive/changes_posix_test.go:15:38: empty-lines: extra empty line at the end of a block (revive)
pkg/archive/copy.go:248:124: empty-lines: extra empty line at the end of a block (revive)
pkg/archive/diff_test.go:198:44: empty-lines: extra empty line at the end of a block (revive)
pkg/archive/archive.go:304:12: empty-lines: extra empty line at the end of a block (revive)
pkg/archive/archive.go:749:37: empty-lines: extra empty line at the end of a block (revive)
pkg/archive/archive.go:812:81: empty-lines: extra empty line at the start of a block (revive)
pkg/archive/copy_unix_test.go:347:34: empty-lines: extra empty line at the end of a block (revive)
pkg/system/path.go:11:39: empty-lines: extra empty line at the end of a block (revive)
pkg/system/meminfo_linux.go:29:21: empty-lines: extra empty line at the end of a block (revive)
pkg/plugins/plugins.go:135:32: empty-lines: extra empty line at the end of a block (revive)
pkg/authorization/response.go:71:48: empty-lines: extra empty line at the start of a block (revive)
pkg/authorization/api_test.go:18:51: empty-lines: extra empty line at the end of a block (revive)
pkg/authorization/middleware_test.go:23:44: empty-lines: extra empty line at the end of a block (revive)
pkg/authorization/middleware_unix_test.go:17:46: empty-lines: extra empty line at the end of a block (revive)
pkg/authorization/api_test.go:57:45: empty-lines: extra empty line at the end of a block (revive)
pkg/authorization/response.go:83:50: empty-lines: extra empty line at the start of a block (revive)
pkg/authorization/api_test.go:66:47: empty-lines: extra empty line at the end of a block (revive)
pkg/authorization/middleware_unix_test.go:45:48: empty-lines: extra empty line at the end of a block (revive)
pkg/authorization/response.go:145:75: empty-lines: extra empty line at the start of a block (revive)
pkg/authorization/middleware_unix_test.go:56:51: empty-lines: extra empty line at the end of a block (revive)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
fix (whitespace) formatting in preparation of enabling more linters
Formatting the code with https://github.com/mvdan/gofumpt Signed-off-by: Sebastiaan van Stijn <[email protected]>
- remove gotest.tools dependency as it was only used in one test, and only for a trivial check - use t.TempDir() - rename vars that collided with package types - don't use un-keyed structs - explicitly ignore some errors to please linters - use iotest.ErrReader - TestReadCloserWrapperClose: verify reading works before closing :) Signed-off-by: Sebastiaan van Stijn <[email protected]>
Unlike its stdlib counterparts, AtomicFileWriter does not take into consideration umask due to its use of chmod. Failure to recognize this may cause subtle problems like the one described in #47498. Therefore the documentation has been updated to let users know that umask is not taken into consideration when using AtomicFileWriter. Closes #47516. Signed-off-by: Antonio Aguilar <[email protected]>
…) package Signed-off-by: Sebastiaan van Stijn <[email protected]>
The temp-file was created before trying to make the given filename an absolute path. Reverse the order of code so that we don't create a temp-file if an error happens. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Use an absolute path for both the temp-file and the destination-file. Signed-off-by: Sebastiaan van Stijn <[email protected]>
- rename tests to match the function tested - remove init func in favor or a test-helper - rename some vars to prevent shadowing - update example values to be more descriptive - add a utility for asserting file content and mode Signed-off-by: Sebastiaan van Stijn <[email protected]>
We were testing this function implicitly through `TestWriteFile`, but not verifying the behavior of `New` in isolation. Add separate tests for this function. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Creating a writer (`atomicwriter.New()`) and closing it without a write ever happening, would replace the destination file with an empty file. This patch adds a check whether a write was performed (either successful or unsuccessful); if no write happened, we cleanup the tempfile without replacing the destination file. Signed-off-by: Sebastiaan van Stijn <[email protected]>
- test errors returned for non-existing destination - test that files are cleaned up after - test writing to a symlinked file (to be fixed) Signed-off-by: Sebastiaan van Stijn <[email protected]>
- Disallow empty filenames - Don't allow writing to a directory - Return early if parent dir doesn't exist - TBD: do we want to allow symlinks to be followed, or disallow? Signed-off-by: Sebastiaan van Stijn <[email protected]>
Using sequential file access ([FILE_FLAG_SEQUENTIAL_SCAN]) prevents Windows from aggressively keeping files in the cache, freeing up system memory for other tasks. On Linux, these changes have no effect, as the sequential package use the standard (os.CreateTemp, os.OpenFile) on non-Windows platforms. Refer to the [Win32 API documentation] for details on sequential file access. [Win32 API documentation]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN [FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN Signed-off-by: Sebastiaan van Stijn <[email protected]>
cpuguy83
requested changes
Apr 2, 2025
Member
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
I get this is just copy/paste from moby/moby, but probably good to deal with some of the unknown edge cases.
atomicwriter/atomicwriter.go
Outdated
| case mode&os.ModeDir != 0: | ||
| return errors.New("cannot write to a directory") | ||
| // TODO(thaJeztah): decide whether we want to disallow symlinks or to follow them. | ||
| // case mode&os.ModeSymlink != 0: |
Member
There was a problem hiding this comment.
Maybe we can get rid of this todo and just error out unless we are explicitly going to add a case to follow symlinks along with a test.
Member
Author
There was a problem hiding this comment.
It's a slightly tricky one; if we want this writer to be a drop-in for a regular os.WriteFile, then we should probably follow the symlink. We could resolve the symlink when creating the writer, then use that value as destination, but of course we may need to check where it's currently used (in case this would open up some symlink attack vector in moby?)
Erroring out would (in the meantime?) also be fine; currently it's kinda horrible, because if the destination is a symlink, the os.Rename just replaces the symlink with a file, which is not very expected I guess.
Member
Author
There was a problem hiding this comment.
fixed in moby/moby#49741 - now producing an error (at least for now) for symlinked target
Signed-off-by: Sebastiaan van Stijn <[email protected]>
The implementation uses "os.Rename" to move the temporary file to the destination, which does not follow symlinks, and because of this would replace a symlink with a file. We can consider adding support for symlinked files in future, so that WriteFile can be used as a drop-in replacement for `os.WriteFile()` but in the meantime, let's produce an error so that nobody can depend on this. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Previously, we were silently discarding this situation and hoping that it would work; let's produce an error instead (we can add additional filemodes when they arrive and if we need them) Signed-off-by: Sebastiaan van Stijn <[email protected]>
While the target-file does not have to exist, its parent must, and must be a directory. This adds a test-case to verify the behavior if the parent is not a directory. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Rewrite `validateDestination` to first check if the destination path
exists. This slightly simplifies the logic (allowing returning
early in each step of the validation) and slightly improves the
error produced.
Before this, the error confusingly would mention the full path
not being a directory. While this _does_ match what `os.Writefile`
would return, it's .. confusing:
failed to stat output path: lstat ./not-a-dir/new-file.txt: not a directory
After this, the error would mention the directory that doesn't exist:
invalid output path: stat ./not-a-dir: not a directory
A slight optimization is made as well, now checking for _both_ "."
and ".." as special case, as either path should exist given any current
working directory (unless the working directory has been deleted, but we'd
fail further down the line).
With this change in order, we can also merge `validateFileMode` into
`validateDestination`.
Signed-off-by: Sebastiaan van Stijn <[email protected]>
kolyshkin
reviewed
Apr 3, 2025
Makefile
Outdated
| test: foreach | ||
|
|
||
| # Test the mount module against the local mountinfo source code instead of the | ||
| # Test the mount module against the local sequential and mountinfo source code instead of the |
Collaborator
There was a problem hiding this comment.
This is not entirely correct (mount is not tested against sequential).
I'd replace all this with something like:
# Some modules in this repo have interdependencies:
# - mount depends on mountinfo
# - atomicwrite depedns on sequential
#
# The code below tests these modules against their local dependencies,
# to catch regressions / breaking changes early.
Member
Author
There was a problem hiding this comment.
Oh! Good one; yes will update.
FWIW; I opened a PR in moby to address Brian's comments; moby/moby#49741
(I'm considering to explore if we could / should make the "write to symlink" work, but with it being changed to an error in that PR, it's something we can always to in a follow-up)
If those are accepted and merged, I'll re-do this PR. Let me move it back to draft until I'm ready to do so. 👍
Member
Author
There was a problem hiding this comment.
Applied your suggestions 👍
This integrates the atomicwriter package from moby at commit;
d7b743b8569e253b61ea8d70e75146ff35d50417 ([1]).
Migration was done using the following steps:
# install filter-repo (https://github.com/newren/git-filter-repo/blob/main/INSTALL.md)
brew install git-filter-repo
# create a temporary clone of docker
cd ~/Projects
git clone https://github.com/moby/moby.git moby_temp
cd moby_temp
# commit taken from
git rev-parse --verify HEAD
d7b743b8569e253b61ea8d70e75146ff35d50417
git filter-repo --analyze
# remove all code, except for 'pkg/atomicwriter', and rename to /atomicwriter
git filter-repo \
--path 'pkg/ioutils/fswriters.go' \
--path 'pkg/ioutils/fswriters_test.go' \
--path 'pkg/atomicwriter' \
--path-rename pkg/atomicwriter:atomicwriter
# go to the target github.com/moby/sys repository
cd ~/go/src/github.com/moby/sys
# create a branch to work with
git checkout -b integrate_atomicwriter
# add the temporary repository as an upstream and make sure it's up-to-date
git remote add moby_temp ~/Projects/moby_temp
git fetch moby_temp
# merge the upstream code
git merge --allow-unrelated-histories --signoff -S moby_temp/master
[1]: moby/moby@d7b743b
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
integrate_atomicwriter
branch
from
b1c5060 to
17fe011
Compare
Member
Author
|
Alright; updated; @cpuguy83 @kolyshkin @dmcgowan PTAL 🤗 |
This was referenced Apr 4, 2025
cpuguy83
approved these changes
Apr 4, 2025
3 tasks
Member
Author
|
Thanks! I'll go ahead and bring this in; might tag a first version later. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
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.
/pkgmoby#32989integrate pkg/atomicwriter
This integrates the atomicwriter package from moby at commit;
d7b743b8569e253b61ea8d70e75146ff35d50417.
Migration was done using the following steps: