fix: cleanup volume dir if create volume failed #2865
Conversation
|
How to test this? |
This scene is quite rare. There is no space left on device, and a directory is just deleted. step1 step2 Look into /var/lib/nerdctl/1935db59/volumes/default/v2, it's an empty dir #nerdctl volume ls |
|
This case is quite rare, so I expect the user to just run |
Volume is not created successfully. The creation process was only halfway complete. #nerdctl volume rm v2 In fact, get function is called before remove function. |
| if err := os.Mkdir(volPath, 0700); err != nil { | ||
| return err | ||
| } | ||
| defer func() { | ||
| if err != nil { | ||
| os.Remove(volPath) |
There was a problem hiding this comment.
Volume path should be cleaned up if create volume failed.
cleanup doesn't seem to happen for errors outside of this fn?
There was a problem hiding this comment.
cleanup is called in this fn for the two errors, json.MarshalIndent and os.WriteFile
Creation is done after this fn is completed. No other new errors outside of it. If some action added after lockutil.WithDirLock(vs.dir, fn) in future, cleanup will be moved out of this fn.
There was a problem hiding this comment.
cleanup is called in this fn for the two errors, json.MarshalIndent and os.WriteFile
cleanup seems to fail if that dir isn't empty
in future, cleanup will be moved out of this fn.
should be documented
There was a problem hiding this comment.
cleanup seems to fail if that dir isn't empty
dir is empty as expected if writing json file failed. Or use os.RemoveAll instead ?
should be documented
OK, should I add some comments？
There was a problem hiding this comment.
RemoveAll can be dangerous, so I don't prefer it
There was a problem hiding this comment.
dir is empty as expected if writing json file failed.
https://pkg.go.dev/os#WriteFile
Since WriteFile requires multiple system calls to complete, a failure mid-operation can leave the file in a partially written state.
There was a problem hiding this comment.
How about these steps in cleanup ?
step 1. check json file exists and remove it
step 2. remove volume data path dir
step 3. remove volume path dir
There was a problem hiding this comment.
the path was created on multiple steps so I beleive that cleanup should follow the same pattern and be cleaned on multiple steps
There was a problem hiding this comment.
How about these steps in cleanup ?
step 1. check json file exists and remove it
fixed
Use stat to check json file exist
Also add some comments.
| defer func() { | ||
| if err != nil { | ||
| // ignore cleanup error | ||
| if _, serr := os.Stat(volFilePath); serr == nil || !os.IsNotExist(serr) { |
There was a problem hiding this comment.
If stat returns error for the file, it should print error and shouldn't continue cleanup.
There was a problem hiding this comment.
it should print error and shouldn't continue cleanup.
volFilePath does not exist and volDataPath exists => we should continue cleanup
just drop stat and log remove error should be fine
There was a problem hiding this comment.
If stat returns error for the file, it should print error and shouldn't continue cleanup.
Fixed.
| if err := os.Mkdir(volDataPath, 0755); err != nil { | ||
| return err | ||
| } | ||
| defer func() { | ||
| if err != nil { | ||
| // ignore cleanup error |
There was a problem hiding this comment.
error shouldn't be ignored. can we print log for this?
There was a problem hiding this comment.
error shouldn't be ignored. can we print log for this?
volFilePath, volDataPath and volPath are removed in cleanup. It looks like complicated if we print all the remove error log. How about only printing stat error which will abort cleanup function
There was a problem hiding this comment.
error shouldn't be ignored. can we print log for this?
I don't print log for cleanup error as some other cleanup functions defer os.Remove in the code.
Of course, I can also add some logs for these remove errors
| defer func() { | ||
| if err != nil { | ||
| // ignore cleanup error | ||
| if _, serr := os.Stat(volFilePath); serr == nil || !os.IsNotExist(serr) { |
There was a problem hiding this comment.
it should print error and shouldn't continue cleanup.
volFilePath does not exist and volDataPath exists => we should continue cleanup
just drop stat and log remove error should be fine
@ktock Could you please rerun the integration test? The failure is not likely related with nerdctl/volume |
|
@zjumoon01 can you rebase to get the latest change in main? The cosign-related CI errors should be fixed now #2896. |
I rebased, but CI failed again caused by 'run container error' in TestVolumeRemove. Do you have any ideas ? |
Volume path should be cleaned up if create volume failed. Otherwise, the same volume will be created failed again next time with a 'file exists' error on os.Mkdir. Signed-off-by: baijia <baijia.wr@antgroup.com>
Volume path should be cleaned up if create volume failed.
Otherwise, the volume will be created failed again next time with a 'file exists' error on os.Mkdir.