diff --git a/plugin/backend_linux.go b/plugin/backend_linux.go index 7d71808e34..92e8a9c5aa 100644 --- a/plugin/backend_linux.go +++ b/plugin/backend_linux.go @@ -639,14 +639,10 @@ func (pm *Manager) Remove(name string, config *types.PluginRmConfig) error { return errors.Wrap(err, "error unmounting plugin data") } - removeDir := pluginDir + "-removing" - if err := os.Rename(pluginDir, removeDir); err != nil { - return errors.Wrap(err, "error performing atomic remove of plugin dir") + if err := atomicRemoveAll(pluginDir); err != nil { + return err } - if err := system.EnsureRemoveAll(removeDir); err != nil { - return errors.Wrap(err, "error removing plugin dir") - } pm.config.Store.Remove(p) pm.config.LogPluginEvent(id, name, "remove") pm.publisher.Publish(EventRemove{Plugin: p.PluginObj}) @@ -835,3 +831,35 @@ func splitConfigRootFSFromTar(in io.ReadCloser, config *[]byte) io.ReadCloser { }() return pr } + +func atomicRemoveAll(dir string) error { + renamed := dir + "-removing" + + err := os.Rename(dir, renamed) + switch { + case os.IsNotExist(err), err == nil: + // even if `dir` doesn't exist, we can still try and remove `renamed` + case os.IsExist(err): + // Some previous remove failed, check if the origin dir exists + if e := system.EnsureRemoveAll(renamed); e != nil { + return errors.Wrap(err, "rename target already exists and could not be removed") + } + if _, err := os.Stat(dir); os.IsNotExist(err) { + // origin doesn't exist, nothing left to do + return nil + } + + // attempt to rename again + if err := os.Rename(dir, renamed); err != nil { + return errors.Wrap(err, "failed to rename dir for atomic removal") + } + default: + return errors.Wrap(err, "failed to rename dir for atomic removal") + } + + if err := system.EnsureRemoveAll(renamed); err != nil { + os.Rename(renamed, dir) + return err + } + return nil +} diff --git a/plugin/backend_linux_test.go b/plugin/backend_linux_test.go new file mode 100644 index 0000000000..6cbe4cde12 --- /dev/null +++ b/plugin/backend_linux_test.go @@ -0,0 +1,81 @@ +package plugin + +import ( + "io/ioutil" + "os" + "path/filepath" + "testing" +) + +func TestAtomicRemoveAllNormal(t *testing.T) { + dir, err := ioutil.TempDir("", "atomic-remove-with-normal") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) // just try to make sure this gets cleaned up + + if err := atomicRemoveAll(dir); err != nil { + t.Fatal(err) + } + + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Fatalf("dir should be gone: %v", err) + } + if _, err := os.Stat(dir + "-removing"); !os.IsNotExist(err) { + t.Fatalf("dir should be gone: %v", err) + } +} + +func TestAtomicRemoveAllAlreadyExists(t *testing.T) { + dir, err := ioutil.TempDir("", "atomic-remove-already-exists") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) // just try to make sure this gets cleaned up + + if err := os.MkdirAll(dir+"-removing", 0755); err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir + "-removing") + + if err := atomicRemoveAll(dir); err != nil { + t.Fatal(err) + } + + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Fatalf("dir should be gone: %v", err) + } + if _, err := os.Stat(dir + "-removing"); !os.IsNotExist(err) { + t.Fatalf("dir should be gone: %v", err) + } +} + +func TestAtomicRemoveAllNotExist(t *testing.T) { + if err := atomicRemoveAll("/not-exist"); err != nil { + t.Fatal(err) + } + + dir, err := ioutil.TempDir("", "atomic-remove-already-exists") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) // just try to make sure this gets cleaned up + + // create the removing dir, but not the "real" one + foo := filepath.Join(dir, "foo") + removing := dir + "-removing" + if err := os.MkdirAll(removing, 0755); err != nil { + t.Fatal(err) + } + + if err := atomicRemoveAll(dir); err != nil { + t.Fatal(err) + } + + if _, err := os.Stat(foo); !os.IsNotExist(err) { + t.Fatalf("dir should be gone: %v", err) + } + if _, err := os.Stat(removing); !os.IsNotExist(err) { + t.Fatalf("dir should be gone: %v", err) + } +}