Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upHandle errors on close in config file write. #2595
Conversation
cpuguy83
force-pushed the
cpuguy83:handle_close_error_on_save
branch
2 times, most recently
from
cb2ff4c
to
6ac4868
I'm not sure if this fixes anything, however I have seen some weird behavior on Windows where temp config files are left around and there doesn't seem to be any errors reported. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
cpuguy83
force-pushed the
cpuguy83:handle_close_error_on_save
branch
from
6ac4868
to
d021730
Codecov Report
@@ Coverage Diff @@
## master #2595 +/- ##
==========================================
- Coverage 58.05% 58.04% -0.01%
==========================================
Files 295 295
Lines 21165 21170 +5
==========================================
+ Hits 12288 12289 +1
- Misses 7975 7977 +2
- Partials 902 904 +2 |
|
The existence of https://success.docker.com/article/warning-error-loading-configjson seems to indicate there is a problem here |
That seems to be about loading the config file, not saving? Or is that hitting this code?
Also wondering if that's related to empty config files, and if we should ignore empty files (just like we ignore empty Testing if we're still producing an error for empty files, and it looks like we do; |
| temp.Close() | ||
| if retErr != nil { | ||
| if err := os.Remove(temp.Name()); err != nil { | ||
| logrus.WithError(err).WithField("file", temp.Name()).Debug("Error cleaning up temp file") |
thaJeztah
Jun 22, 2020
Member
Given that in this case, we have exited early, and didn't update the actual ~/.docker/config.json, perhaps we should print this as a warning?
Something like
WARNING: Failed to update config file: error cleaning up temp file
WDYT?
Given that in this case, we have exited early, and didn't update the actual ~/.docker/config.json, perhaps we should print this as a warning?
Something like
WARNING: Failed to update config file: error cleaning up temp file
WDYT?
cpuguy83
Jun 22, 2020
Author
Collaborator
We could, but we should already get an error returned.
We could, but we should already get an error returned.
Right but how did it get that way? |
|
The symptom I've seen is multiple temp files left behind and an empty config file, btw. |
| defer func() { | ||
| temp.Close() | ||
| if retErr != nil { | ||
| if err := os.Remove(temp.Name()); err != nil { |
thaJeztah
Jun 23, 2020
Member
Not sure if there's a situation where that's possible (thinking if an error occurs on os.Rename(); not sure how atomic that is); should we ignore errors if the file didn't exist?
Suggested change
if err := os.Remove(temp.Name()); err != nil {
if err := os.Remove(temp.Name()); err != nil && !os.IsNotExist(err) {
Guess it's not needed 😅
Not sure if there's a situation where that's possible (thinking if an error occurs on os.Rename(); not sure how atomic that is); should we ignore errors if the file didn't exist?
| if err := os.Remove(temp.Name()); err != nil { | |
| if err := os.Remove(temp.Name()); err != nil && !os.IsNotExist(err) { |
Guess it's not needed
|
Should we consider using something like https://github.com/moby/moby/blob/4609153995b59f54b507446eb2b2cca9ebbbf8e8/pkg/ioutils/fswriters.go#L13 or https://github.com/google/renameio (which seems to handle specific corner-cases)? Problem I see with that approach is that we would (at least not "out of the box") loose the Hm.. looks like we may need to have a look at that in general. Here's some fun thing I tried; mkdir -p ~/.docker
touch ~/real-config.json
ln -s ~/real-config.json ~/.docker/config.json
ls -la ~/.docker/config.json
# lrwxrwxrwx 1 root root 22 Jun 23 12:34 /root/.docker/config.json -> /root/real-config.json
docker login
# Username: thajeztah
# Password:
# Login Succeeded
ls -la ~/.docker/config.json
-rw-r--r-- 1 root root 229 Jun 23 12:36 /root/.docker/config.jsonLooks like we need to resolve symlinks before we rename. Something like; cfgFile := configFile.Filename
if f, err := os.Readlink(cfgFile); err == nil {
cfgFile = f
}
// Try copying the current config file (if any) ownership and permissions
copyFilePermissions(cfgFile, temp.Name())
return os.Rename(temp.Name(), cfgFile) |
|
Opened #2599 for consideration (if we think it's ok to ignore an empty config file); #2595 (comment) |
|
Ping |
|
LGTM |
|
LGTM |
silvin-lubecki
merged commit e4e754b
into
docker:master
I'm not sure if this fixes anything, however I have seen some weird
behavior on Windows where temp config files are left around and there
doesn't seem to be any errors reported, and an empty config.json.