Merge pull request #35265 from cpuguy83/32609_defreference_voldriver_on_error

Fixup some issues with plugin refcounting

integration/plugin/volume/cmd/cmd_test.go

@@ -0,0 +1 @@
+package cmd

integration/plugin/volume/cmd/create-error/main.go

@@ -0,0 +1,23 @@
+package main
+
+import (
+	"net"
+	"net/http"
+)
+
+func main() {
+	l, err := net.Listen("unix", "/run/docker/plugins/plugin.sock")
+	if err != nil {
+		panic(err)
+	}
+
+	mux := http.NewServeMux()
+	server := http.Server{
+		Addr:    l.Addr().String(),
+		Handler: http.NewServeMux(),
+	}
+	mux.HandleFunc("/VolumeDriver.Create", func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, "error during create", http.StatusInternalServerError)
+	})
+	server.Serve(l)
+}

integration/plugin/volume/cmd/create-error/main_test.go

@@ -0,0 +1 @@
+package main

integration/plugin/volume/create_test.go

@@ -0,0 +1,51 @@
+// +build linux
+
+package volume
+
+import (
+	"context"
+	"testing"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/volume"
+	"github.com/docker/docker/integration-cli/daemon"
+)
+
+// TestCreateDerefOnError ensures that if a volume create fails, that the plugin is dereferenced
+// Normally 1 volume == 1 reference to a plugin, which prevents a plugin from being removed.
+// If the volume create fails, we should make sure to dereference the plugin.
+func TestCreateDerefOnError(t *testing.T) {
+	t.Parallel()
+
+	d := daemon.New(t, "", dockerdBinary, daemon.Config{})
+	d.Start(t)
+	defer d.Stop(t)
+
+	c, err := d.NewClient()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	pName := "testderef"
+	createPlugin(t, c, pName, "create-error", asVolumeDriver)
+
+	if err := c.PluginEnable(context.Background(), pName, types.PluginEnableOptions{Timeout: 30}); err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = c.VolumeCreate(context.Background(), volume.VolumesCreateBody{
+		Driver: pName,
+		Name:   "fake",
+	})
+	if err == nil {
+		t.Fatal("volume create should have failed")
+	}
+
+	if err := c.PluginDisable(context.Background(), pName, types.PluginDisableOptions{}); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := c.PluginRemove(context.Background(), pName, types.PluginRemoveOptions{}); err != nil {
+		t.Fatal(err)
+	}
+}

integration/plugin/volume/main_test.go

@@ -0,0 +1,69 @@
+package volume
+
+import (
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/integration-cli/fixtures/plugin"
+	"github.com/docker/docker/pkg/locker"
+	"github.com/pkg/errors"
+)
+
+const dockerdBinary = "dockerd"
+
+var pluginBuildLock = locker.New()
+
+func ensurePlugin(t *testing.T, name string) string {
+	pluginBuildLock.Lock(name)
+	defer pluginBuildLock.Unlock(name)
+
+	installPath := filepath.Join(os.Getenv("GOPATH"), "bin", name)
+	if _, err := os.Stat(installPath); err == nil {
+		return installPath
+	}
+
+	goBin, err := exec.LookPath("go")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	cmd := exec.Command(goBin, "build", "-o", installPath, "./"+filepath.Join("cmd", name))
+	cmd.Env = append(cmd.Env, "CGO_ENABLED=0")
+	if out, err := cmd.CombinedOutput(); err != nil {
+		t.Fatal(errors.Wrapf(err, "error building basic plugin bin: %s", string(out)))
+	}
+
+	return installPath
+}
+
+func asVolumeDriver(cfg *plugin.Config) {
+	cfg.Interface.Types = []types.PluginInterfaceType{
+		{Capability: "volumedriver", Prefix: "docker", Version: "1.0"},
+	}
+}
+
+func withSockPath(name string) func(*plugin.Config) {
+	return func(cfg *plugin.Config) {
+		cfg.Interface.Socket = name
+	}
+}
+
+func createPlugin(t *testing.T, client plugin.CreateClient, alias, bin string, opts ...plugin.CreateOpt) {
+	pluginBin := ensurePlugin(t, bin)
+
+	opts = append(opts, withSockPath("plugin.sock"))
+	opts = append(opts, plugin.WithBinary(pluginBin))
+
+	ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
+	err := plugin.Create(ctx, client, alias, opts...)
+	cancel()
+
+	if err != nil {
+		t.Fatal(err)
+	}
+}

pkg/plugingetter/getter.go

@@ -1,6 +1,8 @@
 package plugingetter
 
-import "github.com/docker/docker/pkg/plugins"
+import (
+	"github.com/docker/docker/pkg/plugins"
+)
 
 const (
 	// Lookup doesn't update RefCount

plugin/store.go

@@ -115,10 +115,15 @@ func (ps *Store) Get(name, capability string, mode int) (plugingetter.CompatPlug
 	if ps != nil {
 		p, err := ps.GetV2Plugin(name)
 		if err == nil {
-			p.AddRefCount(mode)
 			if p.IsEnabled() {
-				return p.FilterByCap(capability)
+				fp, err := p.FilterByCap(capability)
+				if err != nil {
+					return nil, err
+				}
+				p.AddRefCount(mode)
+				return fp, nil
 			}
+
 			// Plugin was found but it is disabled, so we should not fall back to legacy plugins
 			// but we should error out right away
 			return nil, errDisabled(name)

plugin/store_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/pkg/plugingetter"
 	"github.com/docker/docker/plugin/v2"
 )
 
@@ -31,3 +32,33 @@ func TestFilterByCapPos(t *testing.T) {
 		t.Fatalf("expected no error, got %v", err)
 	}
 }
+
+func TestStoreGetPluginNotMatchCapRefs(t *testing.T) {
+	s := NewStore()
+	p := v2.Plugin{PluginObj: types.Plugin{Name: "test:latest"}}
+
+	iType := types.PluginInterfaceType{Capability: "whatever", Prefix: "docker", Version: "1.0"}
+	i := types.PluginConfigInterface{Socket: "plugins.sock", Types: []types.PluginInterfaceType{iType}}
+	p.PluginObj.Config.Interface = i
+
+	if err := s.Add(&p); err != nil {
+		t.Fatal(err)
+	}
+
+	if _, err := s.Get("test", "volumedriver", plugingetter.Acquire); err == nil {
+		t.Fatal("exepcted error when getting plugin that doesn't match the passed in capability")
+	}
+
+	if refs := p.GetRefCount(); refs != 0 {
+		t.Fatalf("reference count should be 0, got: %d", refs)
+	}
+
+	p.PluginObj.Enabled = true
+	if _, err := s.Get("test", "volumedriver", plugingetter.Acquire); err == nil {
+		t.Fatal("exepcted error when getting plugin that doesn't match the passed in capability")
+	}
+
+	if refs := p.GetRefCount(); refs != 0 {
+		t.Fatalf("reference count should be 0, got: %d", refs)
+	}
+}

volume/drivers/extpoint.go

@@ -11,6 +11,7 @@ import (
 	getter "github.com/docker/docker/pkg/plugingetter"
 	"github.com/docker/docker/volume"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 )
 
 // currently created by hand. generation tool would generate this like:
@@ -130,6 +131,12 @@ func lookup(name string, mode int) (volume.Driver, error) {
 
 		d := NewVolumeDriver(p.Name(), p.BasePath(), p.Client())
 		if err := validateDriver(d); err != nil {
+			if mode > 0 {
+				// Undo any reference count changes from the initial `Get`
+				if _, err := drivers.plugingetter.Get(name, extName, mode*-1); err != nil {
+					logrus.WithError(err).WithField("action", "validate-driver").WithField("plugin", name).Error("error releasing reference to plugin")
+				}
+			}
 			return nil, err
 		}
 
@@ -169,9 +176,9 @@ func CreateDriver(name string) (volume.Driver, error) {
 	return lookup(name, getter.Acquire)
 }
 
-// RemoveDriver returns a volume driver by its name and decrements RefCount..
+// ReleaseDriver returns a volume driver by its name and decrements RefCount..
 // If the driver is empty, it looks for the local driver.
-func RemoveDriver(name string) (volume.Driver, error) {
+func ReleaseDriver(name string) (volume.Driver, error) {
 	if name == "" {
 		name = volume.DefaultDriverName
 	}

volume/store/store.go

@@ -145,7 +145,7 @@ func (s *VolumeStore) Purge(name string) {
 	s.globalLock.Lock()
 	v, exists := s.names[name]
 	if exists {
-		if _, err := volumedrivers.RemoveDriver(v.DriverName()); err != nil {
+		if _, err := volumedrivers.ReleaseDriver(v.DriverName()); err != nil {
 			logrus.Errorf("Error dereferencing volume driver: %v", err)
 		}
 	}
@@ -409,6 +409,9 @@ func (s *VolumeStore) create(name, driverName string, opts, labels map[string]st
 	}
 	v, err = vd.Create(name, opts)
 	if err != nil {
+		if _, err := volumedrivers.ReleaseDriver(driverName); err != nil {
+			logrus.WithError(err).WithField("driver", driverName).Error("Error releasing reference to volume driver")
+		}
 		return nil, err
 	}
 	s.globalLock.Lock()