From 3816b514387efd24394f0b8e61d55502aa6ac9ac Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Fri, 20 Oct 2017 15:39:47 -0400 Subject: [PATCH] Fixup some issues with plugin refcounting In some circumstances we were not properly releasing plugin references, leading to failures in removing a plugin with no way to recover other than restarting the daemon. 1. If volume create fails (in the driver) 2. If a driver validation fails (should be rare) 3. If trying to get a plugin that does not match the passed in capability Ideally the test for 1 and 2 would just be a unit test, however the plugin interfaces are too complicated as `plugingetter` relies on github.com/pkg/plugin/Client (a concrete type), which will require spinning up services from within the unit test... it just wouldn't be a unit test at this point. I attempted to refactor this a bit, but since both libnetwork and swarmkit are reliant on `plugingetter` as well, this would not work. This really requires a re-write of the lower-level plugin management to decouple these pieces. Signed-off-by: Brian Goff --- integration/plugin/volume/cmd/cmd_test.go | 1 + .../plugin/volume/cmd/create-error/main.go | 23 +++++++ .../volume/cmd/create-error/main_test.go | 1 + integration/plugin/volume/create_test.go | 51 ++++++++++++++ integration/plugin/volume/main_test.go | 69 +++++++++++++++++++ pkg/plugingetter/getter.go | 4 +- plugin/store.go | 9 ++- plugin/store_test.go | 31 +++++++++ volume/drivers/extpoint.go | 11 ++- volume/store/store.go | 5 +- 10 files changed, 199 insertions(+), 6 deletions(-) create mode 100644 integration/plugin/volume/cmd/cmd_test.go create mode 100644 integration/plugin/volume/cmd/create-error/main.go create mode 100644 integration/plugin/volume/cmd/create-error/main_test.go create mode 100644 integration/plugin/volume/create_test.go create mode 100644 integration/plugin/volume/main_test.go diff --git a/integration/plugin/volume/cmd/cmd_test.go b/integration/plugin/volume/cmd/cmd_test.go new file mode 100644 index 0000000000..1d619dd05e --- /dev/null +++ b/integration/plugin/volume/cmd/cmd_test.go @@ -0,0 +1 @@ +package cmd diff --git a/integration/plugin/volume/cmd/create-error/main.go b/integration/plugin/volume/cmd/create-error/main.go new file mode 100644 index 0000000000..f23be51d66 --- /dev/null +++ b/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) +} diff --git a/integration/plugin/volume/cmd/create-error/main_test.go b/integration/plugin/volume/cmd/create-error/main_test.go new file mode 100644 index 0000000000..06ab7d0f9a --- /dev/null +++ b/integration/plugin/volume/cmd/create-error/main_test.go @@ -0,0 +1 @@ +package main diff --git a/integration/plugin/volume/create_test.go b/integration/plugin/volume/create_test.go new file mode 100644 index 0000000000..ce9b4dca43 --- /dev/null +++ b/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) + } +} diff --git a/integration/plugin/volume/main_test.go b/integration/plugin/volume/main_test.go new file mode 100644 index 0000000000..8cfbf37978 --- /dev/null +++ b/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) + } +} diff --git a/pkg/plugingetter/getter.go b/pkg/plugingetter/getter.go index b04b7bc828..b9ffa547a4 100644 --- a/pkg/plugingetter/getter.go +++ b/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 diff --git a/plugin/store.go b/plugin/store.go index 8349f34f1c..b3398145d7 100644 --- a/plugin/store.go +++ b/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) diff --git a/plugin/store_test.go b/plugin/store_test.go index d3876daa34..5c61cc61c7 100644 --- a/plugin/store_test.go +++ b/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) + } +} diff --git a/volume/drivers/extpoint.go b/volume/drivers/extpoint.go index ee42f2f5ed..c360b37a2b 100644 --- a/volume/drivers/extpoint.go +++ b/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 } diff --git a/volume/store/store.go b/volume/store/store.go index e47ec0e7da..5402c6bd9f 100644 --- a/volume/store/store.go +++ b/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()