Merge pull request #34372 from cpuguy83/more_error_handling_for_pluginrm

Ignore exist/not-exist errors on plugin remove

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 := atomicRemoveAll(pluginDir); err != nil {
-	if err := os.Rename(pluginDir, removeDir); err != nil {
+		return err
-		return errors.Wrap(err, "error performing atomic remove of plugin dir")
 	}
 
-	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
+}

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)
+	}
+}