1. docker
  2. Commit 93027b1ff2475b66b6321b5722009fad4def8187
Browse code

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>

Brian Goff authored on 2017/08/03 03:28:49
Showing 2 changed files
plugin/backend_linux.go
History View file @ 93027b1
... ... 
@@ -638,14 +638,10 @@ func (pm *Manager) Remove(name string, config *types.PluginRmConfig) error {
638 638 
 		return errors.Wrap(err, "error unmounting plugin data")
639 639 
 	}
640 640
641 
-	removeDir := pluginDir + "-removing"
642 
-	if err := os.Rename(pluginDir, removeDir); err != nil {
643 
-		return errors.Wrap(err, "error performing atomic remove of plugin dir")
641 
+	if err := atomicRemoveAll(pluginDir); err != nil {
642 
+		return err
644 643 
 	}
645 644
646 
-	if err := system.EnsureRemoveAll(removeDir); err != nil {
647 
-		return errors.Wrap(err, "error removing plugin dir")
648 
-	}
649 645 
 	pm.config.Store.Remove(p)
650 646 
 	pm.config.LogPluginEvent(id, name, "remove")
651 647 
 	pm.publisher.Publish(EventRemove{Plugin: p.PluginObj})
... ... 
@@ -834,3 +830,35 @@ func splitConfigRootFSFromTar(in io.ReadCloser, config *[]byte) io.ReadCloser {
834 834 
 	}()
835 835 
 	return pr
836 836 
 }
837 
+
838 
+func atomicRemoveAll(dir string) error {
839 
+	renamed := dir + "-removing"
840 
+
841 
+	err := os.Rename(dir, renamed)
842 
+	switch {
843 
+	case os.IsNotExist(err), err == nil:
844 
+		// even if `dir` doesn't exist, we can still try and remove `renamed`
845 
+	case os.IsExist(err):
846 
+		// Some previous remove failed, check if the origin dir exists
847 
+		if e := system.EnsureRemoveAll(renamed); e != nil {
848 
+			return errors.Wrap(err, "rename target already exists and could not be removed")
849 
+		}
850 
+		if _, err := os.Stat(dir); os.IsNotExist(err) {
851 
+			// origin doesn't exist, nothing left to do
852 
+			return nil
853 
+		}
854 
+
855 
+		// attempt to rename again
856 
+		if err := os.Rename(dir, renamed); err != nil {
857 
+			return errors.Wrap(err, "failed to rename dir for atomic removal")
858 
+		}
859 
+	default:
860 
+		return errors.Wrap(err, "failed to rename dir for atomic removal")
861 
+	}
862 
+
863 
+	if err := system.EnsureRemoveAll(renamed); err != nil {
864 
+		os.Rename(renamed, dir)
865 
+		return err
866 
+	}
867 
+	return nil
868 
+}
plugin/backend_linux_test.go
History View file @ 93027b1
837 869 
new file mode 100644
... ... 
@@ -0,0 +1,81 @@
0 
+package plugin
1 
+
2 
+import (
3 
+	"io/ioutil"
4 
+	"os"
5 
+	"path/filepath"
6 
+	"testing"
7 
+)
8 
+
9 
+func TestAtomicRemoveAllNormal(t *testing.T) {
10 
+	dir, err := ioutil.TempDir("", "atomic-remove-with-normal")
11 
+	if err != nil {
12 
+		t.Fatal(err)
13 
+	}
14 
+	defer os.RemoveAll(dir) // just try to make sure this gets cleaned up
15 
+
16 
+	if err := atomicRemoveAll(dir); err != nil {
17 
+		t.Fatal(err)
18 
+	}
19 
+
20 
+	if _, err := os.Stat(dir); !os.IsNotExist(err) {
21 
+		t.Fatalf("dir should be gone: %v", err)
22 
+	}
23 
+	if _, err := os.Stat(dir + "-removing"); !os.IsNotExist(err) {
24 
+		t.Fatalf("dir should be gone: %v", err)
25 
+	}
26 
+}
27 
+
28 
+func TestAtomicRemoveAllAlreadyExists(t *testing.T) {
29 
+	dir, err := ioutil.TempDir("", "atomic-remove-already-exists")
30 
+	if err != nil {
31 
+		t.Fatal(err)
32 
+	}
33 
+	defer os.RemoveAll(dir) // just try to make sure this gets cleaned up
34 
+
35 
+	if err := os.MkdirAll(dir+"-removing", 0755); err != nil {
36 
+		t.Fatal(err)
37 
+	}
38 
+	defer os.RemoveAll(dir + "-removing")
39 
+
40 
+	if err := atomicRemoveAll(dir); err != nil {
41 
+		t.Fatal(err)
42 
+	}
43 
+
44 
+	if _, err := os.Stat(dir); !os.IsNotExist(err) {
45 
+		t.Fatalf("dir should be gone: %v", err)
46 
+	}
47 
+	if _, err := os.Stat(dir + "-removing"); !os.IsNotExist(err) {
48 
+		t.Fatalf("dir should be gone: %v", err)
49 
+	}
50 
+}
51 
+
52 
+func TestAtomicRemoveAllNotExist(t *testing.T) {
53 
+	if err := atomicRemoveAll("/not-exist"); err != nil {
54 
+		t.Fatal(err)
55 
+	}
56 
+
57 
+	dir, err := ioutil.TempDir("", "atomic-remove-already-exists")
58 
+	if err != nil {
59 
+		t.Fatal(err)
60 
+	}
61 
+	defer os.RemoveAll(dir) // just try to make sure this gets cleaned up
62 
+
63 
+	// create the removing dir, but not the "real" one
64 
+	foo := filepath.Join(dir, "foo")
65 
+	removing := dir + "-removing"
66 
+	if err := os.MkdirAll(removing, 0755); err != nil {
67 
+		t.Fatal(err)
68 
+	}
69 
+
70 
+	if err := atomicRemoveAll(dir); err != nil {
71 
+		t.Fatal(err)
72 
+	}
73 
+
74 
+	if _, err := os.Stat(foo); !os.IsNotExist(err) {
75 
+		t.Fatalf("dir should be gone: %v", err)
76 
+	}
77 
+	if _, err := os.Stat(removing); !os.IsNotExist(err) {
78 
+		t.Fatalf("dir should be gone: %v", err)
79 
+	}
80 
+}