From aab82bfc9cb94b0d6b5dc347f74b2c43b1f1f4e1 Mon Sep 17 00:00:00 2001
From: Brian Goff <cpuguy83@gmail.com>
Date: Wed, 2 Aug 2017 14:28:49 -0400
Subject: [PATCH] Ignore exist/not-exist errors on plugin remove

During a plugin remove, docker performs an `os.Rename` to move the
plugin data dir to a new location before removing to acheive an atomic
removal.

`os.Rename` can return either a `NotExist` error if the source path
doesn't exist, or an `Exist` error if the target path already exists.
Both these cases can happen when there is an error on the final
`os.Remove` call, which is common on older kernels (`device or resource
busy`).

When calling rename, we can safely ignore these error types and proceed
to try and remove the plugin.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Upstream-commit: 93027b1ff2475b66b6321b5722009fad4def8187
Component: engine
---
 components/engine/plugin/backend_linux.go     | 40 +++++++--
 .../engine/plugin/backend_linux_test.go       | 81 +++++++++++++++++++
 2 files changed, 115 insertions(+), 6 deletions(-)
 create mode 100644 components/engine/plugin/backend_linux_test.go

diff --git a/components/engine/plugin/backend_linux.go b/components/engine/plugin/backend_linux.go
index 28a6c18ab5..5bcc49e54e 100644
--- a/components/engine/plugin/backend_linux.go
+++ b/components/engine/plugin/backend_linux.go
@@ -638,14 +638,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})
@@ -834,3 +830,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/components/engine/plugin/backend_linux_test.go b/components/engine/plugin/backend_linux_test.go
new file mode 100644
index 0000000000..6cbe4cde12
--- /dev/null
+++ b/components/engine/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)
+	}
+}