Fix bug in volume driver test implementation

Also cleans up some of the driver handlers and converts this to use
checkers everywhere.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

integration-cli/docker_cli_start_volume_driver_unix_test.go

@@ -5,6 +5,7 @@ package main
 import (
 	"encoding/json"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
@@ -55,75 +56,115 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
 	s.server = httptest.NewServer(mux)
 
 	type pluginRequest struct {
-		name string
+		Name string
+	}
+
+	type pluginResp struct {
+		Mountpoint string `json:",omitempty"`
+		Err        string `json:",omitempty"`
+	}
+
+	read := func(b io.ReadCloser) (pluginRequest, error) {
+		defer b.Close()
+		var pr pluginRequest
+		if err := json.NewDecoder(b).Decode(&pr); err != nil {
+			return pr, err
+		}
+		return pr, nil
+	}
+
+	send := func(w http.ResponseWriter, data interface{}) {
+		switch t := data.(type) {
+		case error:
+			http.Error(w, t.Error(), 500)
+		case string:
+			w.Header().Set("Content-Type", "application/vnd.docker.plugins.v1+json")
+			fmt.Fprintln(w, t)
+		default:
+			w.Header().Set("Content-Type", "application/vnd.docker.plugins.v1+json")
+			json.NewEncoder(w).Encode(&data)
+		}
 	}
 
 	mux.HandleFunc("/Plugin.Activate", func(w http.ResponseWriter, r *http.Request) {
 		s.ec.activations++
-
-		w.Header().Set("Content-Type", "application/vnd.docker.plugins.v1+json")
-		fmt.Fprintln(w, `{"Implements": ["VolumeDriver"]}`)
+		send(w, `{"Implements": ["VolumeDriver"]}`)
 	})
 
 	mux.HandleFunc("/VolumeDriver.Create", func(w http.ResponseWriter, r *http.Request) {
 		s.ec.creations++
 
-		w.Header().Set("Content-Type", "application/vnd.docker.plugins.v1+json")
-		fmt.Fprintln(w, `{}`)
+		_, err := read(r.Body)
+		if err != nil {
+			send(w, err)
+			return
+		}
+
+		send(w, nil)
 	})
 
 	mux.HandleFunc("/VolumeDriver.Remove", func(w http.ResponseWriter, r *http.Request) {
 		s.ec.removals++
 
-		w.Header().Set("Content-Type", "application/vnd.docker.plugins.v1+json")
-		fmt.Fprintln(w, `{}`)
+		pr, err := read(r.Body)
+		if err != nil {
+			send(w, err)
+			return
+		}
+		if err := os.RemoveAll(hostVolumePath(pr.Name)); err != nil {
+			send(w, &pluginResp{Err: err.Error()})
+			return
+		}
+
+		send(w, nil)
 	})
 
 	mux.HandleFunc("/VolumeDriver.Path", func(w http.ResponseWriter, r *http.Request) {
 		s.ec.paths++
 
-		var pr pluginRequest
-		if err := json.NewDecoder(r.Body).Decode(&pr); err != nil {
-			http.Error(w, err.Error(), 500)
+		pr, err := read(r.Body)
+		if err != nil {
+			send(w, err)
+			return
 		}
+		p := hostVolumePath(pr.Name)
 
-		p := hostVolumePath(pr.name)
-
-		w.Header().Set("Content-Type", "application/vnd.docker.plugins.v1+json")
 		fmt.Fprintln(w, fmt.Sprintf("{\"Mountpoint\": \"%s\"}", p))
 	})
 
 	mux.HandleFunc("/VolumeDriver.Mount", func(w http.ResponseWriter, r *http.Request) {
 		s.ec.mounts++
 
-		var pr pluginRequest
-		if err := json.NewDecoder(r.Body).Decode(&pr); err != nil {
-			http.Error(w, err.Error(), 500)
+		pr, err := read(r.Body)
+		if err != nil {
+			send(w, err)
+			return
 		}
 
-		p := hostVolumePath(pr.name)
+		p := hostVolumePath(pr.Name)
 		if err := os.MkdirAll(p, 0755); err != nil {
-			http.Error(w, err.Error(), 500)
+			send(w, &pluginResp{Err: err.Error()})
+			return
 		}
 
 		if err := ioutil.WriteFile(filepath.Join(p, "test"), []byte(s.server.URL), 0644); err != nil {
-			http.Error(w, err.Error(), 500)
+			send(w, err)
+			return
 		}
 
-		w.Header().Set("Content-Type", "application/vnd.docker.plugins.v1+json")
-		fmt.Fprintln(w, fmt.Sprintf("{\"Mountpoint\": \"%s\"}", p))
+		send(w, &pluginResp{Mountpoint: p})
 	})
 
 	mux.HandleFunc("/VolumeDriver.Unmount", func(w http.ResponseWriter, r *http.Request) {
 		s.ec.unmounts++
 
-		var pr pluginRequest
-		if err := json.NewDecoder(r.Body).Decode(&pr); err != nil {
-			http.Error(w, err.Error(), 500)
+		_, err := read(r.Body)
+		if err != nil {
+			send(w, err)
+			return
 		}
 
-		w.Header().Set("Content-Type", "application/vnd.docker.plugins.v1+json")
-		fmt.Fprintln(w, `{}`)
+		fmt.Fprintln(w, nil)
 	})
 
 	err := os.MkdirAll("/etc/docker/plugins", 0755)
@@ -140,22 +181,18 @@ func (s *DockerExternalVolumeSuite) TearDownSuite(c *check.C) {
 	c.Assert(err, checker.IsNil)
 }
 
-func (s *DockerExternalVolumeSuite) TestStartExternalNamedVolumeDriver(c *check.C) {
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverNamed(c *check.C) {
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 
 	out, err := s.d.Cmd("run", "--rm", "--name", "test-data", "-v", "external-volume-test:/tmp/external-volume-test", "--volume-driver", "test-external-volume-driver", "busybox:latest", "cat", "/tmp/external-volume-test/test")
-	c.Assert(err, checker.IsNil)
-
+	c.Assert(err, checker.IsNil, check.Commentf(out))
 	c.Assert(out, checker.Contains, s.server.URL)
 
 	p := hostVolumePath("external-volume-test")
 	_, err = os.Lstat(p)
 	c.Assert(err, checker.NotNil)
-
-	if !os.IsNotExist(err) {
-		c.Fatalf("Expected volume path in host to not exist: %s, %v\n", p, err)
-	}
+	c.Assert(os.IsNotExist(err), checker.True, check.Commentf("Expected volume path in host to not exist: %s, %v\n", p, err))
 
 	c.Assert(s.ec.activations, checker.Equals, 1)
 	c.Assert(s.ec.creations, checker.Equals, 1)
@@ -164,13 +201,12 @@ func (s *DockerExternalVolumeSuite) TestStartExternalNamedVolumeDriver(c *check.
 	c.Assert(s.ec.unmounts, checker.Equals, 1)
 }
 
-func (s *DockerExternalVolumeSuite) TestStartExternalVolumeUnnamedDriver(c *check.C) {
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnnamed(c *check.C) {
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 
 	out, err := s.d.Cmd("run", "--rm", "--name", "test-data", "-v", "/tmp/external-volume-test", "--volume-driver", "test-external-volume-driver", "busybox:latest", "cat", "/tmp/external-volume-test/test")
-	c.Assert(err, checker.IsNil)
-
+	c.Assert(err, checker.IsNil, check.Commentf(out))
 	c.Assert(out, checker.Contains, s.server.URL)
 
 	c.Assert(s.ec.activations, checker.Equals, 1)
@@ -180,7 +216,7 @@ func (s *DockerExternalVolumeSuite) TestStartExternalVolumeUnnamedDriver(c *chec
 	c.Assert(s.ec.unmounts, checker.Equals, 1)
 }
 
-func (s DockerExternalVolumeSuite) TestStartExternalVolumeDriverVolumesFrom(c *check.C) {
+func (s DockerExternalVolumeSuite) TestExternalVolumeDriverVolumesFrom(c *check.C) {
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 
@@ -200,7 +236,7 @@ func (s DockerExternalVolumeSuite) TestStartExternalVolumeDriverVolumesFrom(c *c
 	c.Assert(s.ec.unmounts, checker.Equals, 2)
 }
 
-func (s DockerExternalVolumeSuite) TestStartExternalVolumeDriverDeleteContainer(c *check.C) {
+func (s DockerExternalVolumeSuite) TestExternalVolumeDriverDeleteContainer(c *check.C) {
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 
@@ -221,12 +257,11 @@ func hostVolumePath(name string) string {
 	return fmt.Sprintf("/var/lib/docker/volumes/%s", name)
 }
 
-func (s *DockerExternalVolumeSuite) TestStartExternalNamedVolumeDriverCheckBindLocalVolume(c *check.C) {
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverNamedCheckBindLocalVolume(c *check.C) {
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 
 	expected := s.server.URL
-
 	dockerfile := fmt.Sprintf(`FROM busybox:latest
 	RUN mkdir /nobindthenlocalvol
 	RUN echo %s > /nobindthenlocalvol/test
@@ -250,7 +285,7 @@ func (s *DockerExternalVolumeSuite) TestStartExternalNamedVolumeDriverCheckBindL
 }
 
 // Make sure a request to use a down driver doesn't block other requests
-func (s *DockerExternalVolumeSuite) TestStartExternalVolumeDriverLookupNotBlocked(c *check.C) {
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverLookupNotBlocked(c *check.C) {
 	specPath := "/etc/docker/plugins/down-driver.spec"
 	err := ioutil.WriteFile("/etc/docker/plugins/down-driver.spec", []byte("tcp://127.0.0.7:9999"), 0644)
 	c.Assert(err, checker.IsNil)
@@ -285,8 +320,7 @@ func (s *DockerExternalVolumeSuite) TestStartExternalVolumeDriverLookupNotBlocke
 		cmd2.Process.Kill()
 	}
 }
-
-func (s *DockerExternalVolumeSuite) TestStartExternalVolumeDriverRetryNotImmediatelyExists(c *check.C) {
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverRetryNotImmediatelyExists(c *check.C) {
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 
@@ -304,9 +338,8 @@ func (s *DockerExternalVolumeSuite) TestStartExternalVolumeDriverRetryNotImmedia
 	go func() {
 		// wait for a retry to occur, then create spec to allow plugin to register
 		time.Sleep(2000 * time.Millisecond)
-		if err := ioutil.WriteFile(specPath, []byte(s.server.URL), 0644); err != nil {
-			c.Fatal(err)
-		}
+		// no need to check for an error here since it will get picked up by the timeout later
+		ioutil.WriteFile(specPath, []byte(s.server.URL), 0644)
 	}()
 
 	select {
@@ -323,7 +356,7 @@ func (s *DockerExternalVolumeSuite) TestStartExternalVolumeDriverRetryNotImmedia
 	c.Assert(s.ec.unmounts, checker.Equals, 1)
 }
 
-func (s *DockerExternalVolumeSuite) TestStartExternalVolumeDriverBindExternalVolume(c *check.C) {
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverBindExternalVolume(c *check.C) {
 	dockerCmd(c, "volume", "create", "-d", "test-external-volume-driver", "--name", "foo")
 	dockerCmd(c, "run", "-d", "--name", "testing", "-v", "foo:/bar", "busybox", "top")