Merge pull request #29599 from anusha-ragunathan/refcount

Enforce zero plugin refcount during disable, not remove.
moby · Dec 22, 2016 · d1dfc1a · d1dfc1a
2 parents d25186a + 8cb2229
commit d1dfc1a
Show file tree

Hide file tree

Showing 13 changed files with 69 additions and 31 deletions.
diff --git a/api/server/router/plugin/backend.go b/api/server/router/plugin/backend.go
@@ -10,7 +10,7 @@ import (
 
 // Backend for Plugin
 type Backend interface {
-	Disable(name string) error
+	Disable(name string, config *enginetypes.PluginDisableConfig) error
 	Enable(name string, config *enginetypes.PluginEnableConfig) error
 	List() ([]enginetypes.Plugin, error)
 	Inspect(name string) (enginetypes.Plugin, error)

diff --git a/api/server/router/plugin/plugin_routes.go b/api/server/router/plugin/plugin_routes.go
@@ -99,7 +99,16 @@ func (pr *pluginRouter) enablePlugin(ctx context.Context, w http.ResponseWriter,
 }
 
 func (pr *pluginRouter) disablePlugin(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
-	return pr.backend.Disable(vars["name"])
+	if err := httputils.ParseForm(r); err != nil {
+		return err
+	}
+
+	name := vars["name"]
+	config := &types.PluginDisableConfig{
+		ForceDisable: httputils.BoolValue(r, "force"),
+	}
+
+	return pr.backend.Disable(name, config)
 }
 
 func (pr *pluginRouter) removePlugin(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {

diff --git a/api/types/client.go b/api/types/client.go
@@ -340,6 +340,11 @@ type PluginEnableOptions struct {
 	Timeout int
 }
 
+// PluginDisableOptions holds parameters to disable plugins.
+type PluginDisableOptions struct {
+	Force bool
+}
+
 // PluginInstallOptions holds parameters to install a plugin.
 type PluginInstallOptions struct {
 	Disabled              bool

diff --git a/api/types/configs.go b/api/types/configs.go
@@ -53,14 +53,17 @@ type ExecConfig struct {
 	Cmd          []string // Execution commands and args
 }
 
-// PluginRmConfig holds arguments for the plugin remove
-// operation. This struct is used to tell the backend what operations
-// to perform.
+// PluginRmConfig holds arguments for plugin remove.
 type PluginRmConfig struct {
 	ForceRemove bool
 }
 
-// PluginEnableConfig holds arguments for the plugin enable
+// PluginEnableConfig holds arguments for plugin enable
 type PluginEnableConfig struct {
 	Timeout int
 }
+
+// PluginDisableConfig holds arguments for plugin disable.
+type PluginDisableConfig struct {
+	ForceDisable bool
+}
diff --git a/cli/command/plugin/disable.go b/cli/command/plugin/disable.go
@@ -3,6 +3,7 @@ package plugin
 import (
 	"fmt"
 
+	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/cli"
 	"github.com/docker/docker/cli/command"
 	"github.com/docker/docker/reference"
@@ -11,19 +12,23 @@ import (
 )
 
 func newDisableCommand(dockerCli *command.DockerCli) *cobra.Command {
+	var force bool
+
 	cmd := &cobra.Command{
 		Use:   "disable PLUGIN",
 		Short: "Disable a plugin",
 		Args:  cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return runDisable(dockerCli, args[0])
+			return runDisable(dockerCli, args[0], force)
 		},
 	}
 
+	flags := cmd.Flags()
+	flags.BoolVarP(&force, "force", "f", false, "Force the disable of an active plugin")
 	return cmd
 }
 
-func runDisable(dockerCli *command.DockerCli, name string) error {
+func runDisable(dockerCli *command.DockerCli, name string, force bool) error {
 	named, err := reference.ParseNamed(name) // FIXME: validate
 	if err != nil {
 		return err
@@ -35,7 +40,7 @@ func runDisable(dockerCli *command.DockerCli, name string) error {
 	if !ok {
 		return fmt.Errorf("invalid name: %s", named.String())
 	}
-	if err := dockerCli.Client().PluginDisable(context.Background(), ref.String()); err != nil {
+	if err := dockerCli.Client().PluginDisable(context.Background(), ref.String(), types.PluginDisableOptions{Force: force}); err != nil {
 		return err
 	}
 	fmt.Fprintln(dockerCli.Out(), name)

diff --git a/client/interface.go b/client/interface.go
@@ -110,7 +110,7 @@ type PluginAPIClient interface {
 	PluginList(ctx context.Context) (types.PluginsListResponse, error)
 	PluginRemove(ctx context.Context, name string, options types.PluginRemoveOptions) error
 	PluginEnable(ctx context.Context, name string, options types.PluginEnableOptions) error
-	PluginDisable(ctx context.Context, name string) error
+	PluginDisable(ctx context.Context, name string, options types.PluginDisableOptions) error
 	PluginInstall(ctx context.Context, name string, options types.PluginInstallOptions) error
 	PluginPush(ctx context.Context, name string, registryAuth string) error
 	PluginSet(ctx context.Context, name string, args []string) error

diff --git a/client/plugin_disable.go b/client/plugin_disable.go
@@ -1,12 +1,19 @@
 package client
 
 import (
+	"net/url"
+
+	"github.com/docker/docker/api/types"
 	"golang.org/x/net/context"
 )
 
 // PluginDisable disables a plugin
-func (cli *Client) PluginDisable(ctx context.Context, name string) error {
-	resp, err := cli.post(ctx, "/plugins/"+name+"/disable", nil, nil, nil)
+func (cli *Client) PluginDisable(ctx context.Context, name string, options types.PluginDisableOptions) error {
+	query := url.Values{}
+	if options.Force {
+		query.Set("force", "1")
+	}
+	resp, err := cli.post(ctx, "/plugins/"+name+"/disable", query, nil, nil)
 	ensureReaderClosed(resp)
 	return err
 }
diff --git a/client/plugin_disable_test.go b/client/plugin_disable_test.go
@@ -8,6 +8,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/docker/docker/api/types"
 	"golang.org/x/net/context"
 )
 
@@ -16,7 +17,7 @@ func TestPluginDisableError(t *testing.T) {
 		client: newMockClient(errorMock(http.StatusInternalServerError, "Server error")),
 	}
 
-	err := client.PluginDisable(context.Background(), "plugin_name")
+	err := client.PluginDisable(context.Background(), "plugin_name", types.PluginDisableOptions{})
 	if err == nil || err.Error() != "Error response from daemon: Server error" {
 		t.Fatalf("expected a Server Error, got %v", err)
 	}
@@ -40,7 +41,7 @@ func TestPluginDisable(t *testing.T) {
 		}),
 	}
 
-	err := client.PluginDisable(context.Background(), "plugin_name")
+	err := client.PluginDisable(context.Background(), "plugin_name", types.PluginDisableOptions{})
 	if err != nil {
 		t.Fatal(err)
 	}

diff --git a/docs/reference/commandline/plugin_disable.md b/docs/reference/commandline/plugin_disable.md
@@ -21,11 +21,13 @@ Usage:  docker plugin disable PLUGIN
 Disable a plugin
 
 Options:
-      --help   Print usage
+  -f, --force   Force the disable of an active plugin
+      --help    Print usage
 ```
 
 Disables a plugin. The plugin must be installed before it can be disabled,
-see [`docker plugin install`](plugin_install.md).
+see [`docker plugin install`](plugin_install.md). Without the `-f` option,
+a plugin that has references (eg, volumes, networks) cannot be disabled.
 
 
 The following example shows that the `no-remove` plugin is installed

diff --git a/integration-cli/docker_cli_daemon_plugins_test.go b/integration-cli/docker_cli_daemon_plugins_test.go
@@ -268,10 +268,17 @@ func (s *DockerDaemonSuite) TestPluginVolumeRemoveOnRestart(c *check.C) {
 	s.d.Restart(c, "--live-restore=true")
 
 	out, err = s.d.Cmd("plugin", "disable", pName)
-	c.Assert(err, checker.IsNil, check.Commentf(out))
-	out, err = s.d.Cmd("plugin", "rm", pName)
 	c.Assert(err, checker.NotNil, check.Commentf(out))
 	c.Assert(out, checker.Contains, "in use")
+
+	out, err = s.d.Cmd("volume", "rm", "test")
+	c.Assert(err, checker.IsNil, check.Commentf(out))
+
+	out, err = s.d.Cmd("plugin", "disable", pName)
+	c.Assert(err, checker.IsNil, check.Commentf(out))
+
+	out, err = s.d.Cmd("plugin", "rm", pName)
+	c.Assert(err, checker.IsNil, check.Commentf(out))
 }
 
 func existsMountpointWithPrefix(mountpointPrefix string) (bool, error) {

diff --git a/integration-cli/docker_cli_plugins_test.go b/integration-cli/docker_cli_plugins_test.go
@@ -64,23 +64,18 @@ func (s *DockerSuite) TestPluginForceRemove(c *check.C) {
 
 func (s *DockerSuite) TestPluginActive(c *check.C) {
 	testRequires(c, DaemonIsLinux, IsAmd64, Network)
-	out, _, err := dockerCmdWithError("plugin", "install", "--grant-all-permissions", pNameWithTag)
+	_, _, err := dockerCmdWithError("plugin", "install", "--grant-all-permissions", pNameWithTag)
 	c.Assert(err, checker.IsNil)
 
-	out, _, err = dockerCmdWithError("volume", "create", "-d", pNameWithTag)
+	_, _, err = dockerCmdWithError("volume", "create", "-d", pNameWithTag, "--name", "testvol1")
 	c.Assert(err, checker.IsNil)
 
-	vID := strings.TrimSpace(out)
-
-	out, _, err = dockerCmdWithError("plugin", "remove", pNameWithTag)
-	c.Assert(out, checker.Contains, "is in use")
+	out, _, err := dockerCmdWithError("plugin", "disable", pNameWithTag)
+	c.Assert(out, checker.Contains, "in use")
 
-	_, _, err = dockerCmdWithError("volume", "rm", vID)
+	_, _, err = dockerCmdWithError("volume", "rm", "testvol1")
 	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)
 

diff --git a/plugin/backend_linux.go b/plugin/backend_linux.go
@@ -31,8 +31,8 @@ var (
 	validPartialID = regexp.MustCompile(`^([a-f0-9]{1,64})$`)
 )
 
-// Disable deactivates a plugin, which implies that they cannot be used by containers.
-func (pm *Manager) Disable(name string) error {
+// Disable deactivates a plugin. This means resources (volumes, networks) cant use them.
+func (pm *Manager) Disable(name string, config *types.PluginDisableConfig) error {
 	p, err := pm.pluginStore.GetByName(name)
 	if err != nil {
 		return err
@@ -41,6 +41,10 @@ func (pm *Manager) Disable(name string) error {
 	c := pm.cMap[p]
 	pm.mu.RUnlock()
 
+	if !config.ForceDisable && p.GetRefCount() > 0 {
+		return fmt.Errorf("plugin %s is in use", p.Name())
+	}
+
 	if err := pm.disable(p, c); err != nil {
 		return err
 	}

diff --git a/plugin/backend_unsupported.go b/plugin/backend_unsupported.go
@@ -14,7 +14,7 @@ import (
 var errNotSupported = errors.New("plugins are not supported on this platform")
 
 // Disable deactivates a plugin, which implies that they cannot be used by containers.
-func (pm *Manager) Disable(name string) error {
+func (pm *Manager) Disable(name string, config *types.PluginDisableConfig) error {
 	return errNotSupported
 }