From b22d07f51573e39069b4e3a6d8b0580958412e72 Mon Sep 17 00:00:00 2001 From: Victor Vieux Date: Wed, 7 Sep 2016 06:59:15 -0700 Subject: [PATCH] add check plugin is not used before rm Signed-off-by: Victor Vieux --- integration-cli/docker_cli_plugins_test.go | 27 ++++++++++++++++++++++ plugin/backend.go | 16 +++++++++++-- plugin/store/interface.go | 9 ++++++++ plugin/store/legacy.go | 2 +- plugin/store/store.go | 5 +++- plugin/v2/plugin.go | 1 + volume/drivers/extpoint.go | 24 ++++++++++++++++--- volume/store/store.go | 4 ++-- 8 files changed, 79 insertions(+), 9 deletions(-) diff --git a/integration-cli/docker_cli_plugins_test.go b/integration-cli/docker_cli_plugins_test.go index 7d0ad926de..f838d60d51 100644 --- a/integration-cli/docker_cli_plugins_test.go +++ b/integration-cli/docker_cli_plugins_test.go @@ -58,6 +58,33 @@ func (s *DockerSuite) TestPluginForceRemove(c *check.C) { c.Assert(out, checker.Contains, pNameWithTag) } +func (s *DockerSuite) TestPluginActive(c *check.C) { + testRequires(c, DaemonIsLinux, ExperimentalDaemon) + out, _, err := dockerCmdWithError("plugin", "install", "--grant-all-permissions", pNameWithTag) + c.Assert(err, checker.IsNil) + + out, _, err = dockerCmdWithError("volume", "create", "-d", pNameWithTag) + c.Assert(err, checker.IsNil) + + vID := strings.TrimSpace(out) + + out, _, err = dockerCmdWithError("plugin", "remove", pNameWithTag) + c.Assert(out, checker.Contains, "is in use") + + _, _, err = dockerCmdWithError("volume", "rm", vID) + c.Assert(err, checker.IsNil) + + out, _, err = dockerCmdWithError("plugin", "remove", pNameWithTag) + c.Assert(out, checker.Contains, "is enabled") + + _, _, err = dockerCmdWithError("plugin", "disable", pNameWithTag) + c.Assert(err, checker.IsNil) + + out, _, err = dockerCmdWithError("plugin", "remove", pNameWithTag) + c.Assert(err, checker.IsNil) + c.Assert(out, checker.Contains, pNameWithTag) +} + func (s *DockerSuite) TestPluginInstallDisable(c *check.C) { testRequires(c, DaemonIsLinux, ExperimentalDaemon) out, _, err := dockerCmdWithError("plugin", "install", "--grant-all-permissions", "--disable", pName) diff --git a/plugin/backend.go b/plugin/backend.go index 4c57325412..56d361076b 100644 --- a/plugin/backend.go +++ b/plugin/backend.go @@ -147,14 +147,26 @@ func (pm *Manager) Remove(name string, config *types.PluginRmConfig) error { if err != nil { return err } - if p.IsEnabled() { - if !config.ForceRemove { + + if !config.ForceRemove { + p.RLock() + if p.RefCount > 0 { + p.RUnlock() + return fmt.Errorf("plugin %s is in use", p.Name()) + } + p.RUnlock() + + if p.IsEnabled() { return fmt.Errorf("plugin %s is enabled", p.Name()) } + } + + if p.IsEnabled() { if err := pm.disable(p); err != nil { logrus.Errorf("failed to disable plugin '%s': %s", p.Name(), err) } } + pm.pluginStore.Remove(p) pm.pluginEventLogger(p.GetID(), name, "remove") return nil diff --git a/plugin/store/interface.go b/plugin/store/interface.go index 88277b498c..d33a5f5dde 100644 --- a/plugin/store/interface.go +++ b/plugin/store/interface.go @@ -2,6 +2,15 @@ package store import "github.com/docker/docker/pkg/plugins" +const ( + // LOOKUP doesn't update RefCount + LOOKUP = 0 + // CREATE increments RefCount + CREATE = 1 + // REMOVE decrements RefCount + REMOVE = -1 +) + // CompatPlugin is an abstraction to handle both new and legacy (v1) plugins. type CompatPlugin interface { Client() *plugins.Client diff --git a/plugin/store/legacy.go b/plugin/store/legacy.go index ecd3f045e4..86b276fc5d 100644 --- a/plugin/store/legacy.go +++ b/plugin/store/legacy.go @@ -20,6 +20,6 @@ func FindWithCapability(capability string) ([]CompatPlugin, error) { } // LookupWithCapability returns a plugin matching the given name and capability. -func LookupWithCapability(name, capability string) (CompatPlugin, error) { +func LookupWithCapability(name, capability string, _ int) (CompatPlugin, error) { return plugins.Get(name, capability) } diff --git a/plugin/store/store.go b/plugin/store/store.go index 1d91816607..4063e762ea 100644 --- a/plugin/store/store.go +++ b/plugin/store/store.go @@ -160,7 +160,7 @@ func (ps *PluginStore) updatePluginDB() error { } // LookupWithCapability returns a plugin matching the given name and capability. -func LookupWithCapability(name, capability string) (CompatPlugin, error) { +func LookupWithCapability(name, capability string, mode int) (CompatPlugin, error) { var ( p *v2.Plugin err error @@ -181,6 +181,9 @@ func LookupWithCapability(name, capability string) (CompatPlugin, error) { } p, err = store.GetByName(fullName) if err == nil { + p.Lock() + p.RefCount += mode + p.Unlock() return p.FilterByCap(capability) } if _, ok := err.(ErrNotFound); !ok { diff --git a/plugin/v2/plugin.go b/plugin/v2/plugin.go index 48e1be576c..80cd3d80e0 100644 --- a/plugin/v2/plugin.go +++ b/plugin/v2/plugin.go @@ -35,6 +35,7 @@ type Plugin struct { RestartManager restartmanager.RestartManager `json:"-"` RuntimeSourcePath string `json:"-"` ExitChan chan bool `json:"-"` + RefCount int `json:"-"` } func newPluginObj(name, id, tag string) types.Plugin { diff --git a/volume/drivers/extpoint.go b/volume/drivers/extpoint.go index 6048f6e5b9..314535ca24 100644 --- a/volume/drivers/extpoint.go +++ b/volume/drivers/extpoint.go @@ -91,7 +91,7 @@ func Unregister(name string) bool { // lookup returns the driver associated with the given name. If a // driver with the given name has not been registered it checks if // there is a VolumeDriver plugin available with the given name. -func lookup(name string) (volume.Driver, error) { +func lookup(name string, mode int) (volume.Driver, error) { drivers.driverLock.Lock(name) defer drivers.driverLock.Unlock(name) @@ -102,7 +102,7 @@ func lookup(name string) (volume.Driver, error) { return ext, nil } - p, err := pluginStore.LookupWithCapability(name, extName) + p, err := pluginStore.LookupWithCapability(name, extName, mode) if err != nil { return nil, fmt.Errorf("Error looking up volume plugin %s: %v", name, err) } @@ -134,7 +134,25 @@ func GetDriver(name string) (volume.Driver, error) { if name == "" { name = volume.DefaultDriverName } - return lookup(name) + return lookup(name, pluginStore.LOOKUP) +} + +// CreateDriver returns a volume driver by its name and increments RefCount. +// If the driver is empty, it looks for the local driver. +func CreateDriver(name string) (volume.Driver, error) { + if name == "" { + name = volume.DefaultDriverName + } + return lookup(name, pluginStore.CREATE) +} + +// RemoveDriver 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) { + if name == "" { + name = volume.DefaultDriverName + } + return lookup(name, pluginStore.REMOVE) } // GetDriverList returns list of volume drivers registered. diff --git a/volume/store/store.go b/volume/store/store.go index 55fa7cf209..e7f10439a5 100644 --- a/volume/store/store.go +++ b/volume/store/store.go @@ -264,7 +264,7 @@ func (s *VolumeStore) create(name, driverName string, opts, labels map[string]st } } - vd, err := volumedrivers.GetDriver(driverName) + vd, err := volumedrivers.CreateDriver(driverName) if err != nil { return nil, &OpErr{Op: "create", Name: name, Err: err} @@ -416,7 +416,7 @@ func (s *VolumeStore) Remove(v volume.Volume) error { return &OpErr{Err: errVolumeInUse, Name: v.Name(), Op: "remove", Refs: refs} } - vd, err := volumedrivers.GetDriver(v.DriverName()) + vd, err := volumedrivers.RemoveDriver(v.DriverName()) if err != nil { return &OpErr{Err: err, Name: vd.Name(), Op: "remove"} }