From 8cb2229cd18c53bdbf36301f26db565a50027d6a Mon Sep 17 00:00:00 2001 From: Anusha Ragunathan Date: Tue, 20 Dec 2016 08:26:58 -0800 Subject: [PATCH] Enforce zero plugin refcount during disable. When plugins have a positive refcount, they were not allowed to be removed. However, plugins could still be disabled when volumes referenced it and containers using them were running. This change fixes that by enforcing plugin refcount during disable. A "force" disable option is also added to ignore reference refcounting. Signed-off-by: Anusha Ragunathan --- api/server/router/plugin/backend.go | 2 +- api/server/router/plugin/plugin_routes.go | 11 ++++++++++- api/types/client.go | 5 +++++ api/types/configs.go | 11 +++++++---- cli/command/plugin/disable.go | 11 ++++++++--- client/interface.go | 2 +- client/plugin_disable.go | 11 +++++++++-- client/plugin_disable_test.go | 5 +++-- docs/reference/commandline/plugin_disable.md | 6 ++++-- integration-cli/docker_cli_daemon_plugins_test.go | 11 +++++++++-- integration-cli/docker_cli_plugins_test.go | 15 +++++---------- plugin/backend_linux.go | 8 ++++++-- plugin/backend_unsupported.go | 2 +- 13 files changed, 69 insertions(+), 31 deletions(-) diff --git a/api/server/router/plugin/backend.go b/api/server/router/plugin/backend.go index fba42f3e81..73bae3b7bf 100644 --- 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 index 6a2ba7dc4f..bb38f21f05 100644 --- 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 index d58a1a10ed..feb7635f47 100644 --- 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 index 4086126232..20c19f2132 100644 --- 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 index 9089a3cf68..5399e61f1b 100644 --- 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 index 6319f34f1e..96d65a428a 100644 --- 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 index 51e4565125..30467db742 100644 --- 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 index 2818008ab9..a4de45be2d 100644 --- 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 index 4bffb2cecf..0d4ab7d308 100644 --- 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 index cb4ce547e1..f22ccd3401 100644 --- 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 index cfc02518e4..ead15877df 100644 --- 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", "disable", pNameWithTag) + c.Assert(out, checker.Contains, "in use") - out, _, err = dockerCmdWithError("plugin", "remove", pNameWithTag) - c.Assert(out, checker.Contains, "is 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 index 894bff889d..858e34fd89 100644 --- 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 index 2d4b365faf..10f530901c 100644 --- a/plugin/backend_unsupported.go +++ b/plugin/backend_unsupported.go @@ -15,7 +15,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 }