From 73b8fa7bc61a13003a2bab1727fa5df90cdebd88 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 26 Jun 2017 14:54:14 -0400 Subject: [PATCH 1/3] Make plugin removes more resilient to failure Before this patch, if the plugin's `config.json` is successfully removed but the main plugin state dir could not be removed for some reason (e.g. leaked mount), it will prevent the daemon from being able to be restarted. This patches changes this to atomically remove the plugin such that on daemon restart we can detect that there was an error and re-try. It also changes the logic so that it only logs errors on restore rather than erroring out the daemon. This also removes some code which is now duplicated elsewhere. Signed-off-by: Brian Goff (cherry picked from commit 11cf394e5ea964636294a219872b188fe5bdf4dd) Signed-off-by: Andrew Hsu --- .../integration-cli/docker_cli_daemon_test.go | 46 +++++++++++++++++++ components/engine/plugin/backend_linux.go | 39 +++++----------- components/engine/plugin/manager.go | 23 +++++++++- components/engine/plugin/manager_linux.go | 2 +- 4 files changed, 81 insertions(+), 29 deletions(-) diff --git a/components/engine/integration-cli/docker_cli_daemon_test.go b/components/engine/integration-cli/docker_cli_daemon_test.go index 4ec5ac230d..d09fce6f35 100644 --- a/components/engine/integration-cli/docker_cli_daemon_test.go +++ b/components/engine/integration-cli/docker_cli_daemon_test.go @@ -5,6 +5,7 @@ package main import ( "bufio" "bytes" + "context" "encoding/json" "fmt" "io" @@ -25,6 +26,9 @@ import ( "crypto/x509" "github.com/cloudflare/cfssl/helpers" + "github.com/docker/docker/api" + "github.com/docker/docker/api/types" + "github.com/docker/docker/client" "github.com/docker/docker/integration-cli/checker" "github.com/docker/docker/integration-cli/cli" "github.com/docker/docker/integration-cli/daemon" @@ -2980,3 +2984,45 @@ func (s *DockerDaemonSuite) TestShmSizeReload(c *check.C) { c.Assert(err, check.IsNil, check.Commentf("Output: %s", out)) c.Assert(strings.TrimSpace(out), check.Equals, fmt.Sprintf("%v", size)) } + +// TestFailedPluginRemove makes sure that a failed plugin remove does not block +// the daemon from starting +func (s *DockerDaemonSuite) TestFailedPluginRemove(c *check.C) { + testRequires(c, DaemonIsLinux, IsAmd64, SameHostDaemon) + d := daemon.New(c, dockerBinary, dockerdBinary, daemon.Config{}) + d.Start(c) + cli, err := client.NewClient(d.Sock(), api.DefaultVersion, nil, nil) + c.Assert(err, checker.IsNil) + + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Second) + defer cancel() + + name := "test-plugin-rm-fail" + out, err := cli.PluginInstall(ctx, name, types.PluginInstallOptions{ + Disabled: true, + AcceptAllPermissions: true, + RemoteRef: "cpuguy83/docker-logdriver-test", + }) + c.Assert(err, checker.IsNil) + defer out.Close() + io.Copy(ioutil.Discard, out) + + ctx, cancel = context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + p, _, err := cli.PluginInspectWithRaw(ctx, name) + c.Assert(err, checker.IsNil) + + // simulate a bad/partial removal by removing the plugin config. + configPath := filepath.Join(d.Root, "plugins", p.ID, "config.json") + c.Assert(os.Remove(configPath), checker.IsNil) + + d.Restart(c) + ctx, cancel = context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + _, err = cli.Ping(ctx) + c.Assert(err, checker.IsNil) + + _, _, err = cli.PluginInspectWithRaw(ctx, name) + // plugin should be gone since the config.json is gone + c.Assert(err, checker.NotNil) +} diff --git a/components/engine/plugin/backend_linux.go b/components/engine/plugin/backend_linux.go index 1d7f3a838c..5c024e9764 100644 --- a/components/engine/plugin/backend_linux.go +++ b/components/engine/plugin/backend_linux.go @@ -13,7 +13,6 @@ import ( "os" "path" "path/filepath" - "sort" "strings" "github.com/Sirupsen/logrus" @@ -32,6 +31,7 @@ import ( "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/pools" "github.com/docker/docker/pkg/progress" + "github.com/docker/docker/pkg/system" "github.com/docker/docker/plugin/v2" refstore "github.com/docker/docker/reference" "github.com/opencontainers/go-digest" @@ -631,14 +631,20 @@ func (pm *Manager) Remove(name string, config *types.PluginRmConfig) error { }() id := p.GetID() - pm.config.Store.Remove(p) pluginDir := filepath.Join(pm.config.Root, id) - if err := recursiveUnmount(pluginDir); err != nil { - logrus.WithField("dir", pluginDir).WithField("id", id).Warn(err) + + if err := mount.RecursiveUnmount(pluginDir); err != nil { + return errors.Wrap(err, "error unmounting plugin data") } - if err := os.RemoveAll(pluginDir); err != nil { - logrus.Warnf("unable to remove %q from plugin remove: %v", pluginDir, err) + + if err := os.Rename(pluginDir, pluginDir+"-removing"); err != nil { + return errors.Wrap(err, "error performing atomic remove of plugin dir") } + + if err := system.EnsureRemoveAll(pluginDir); err != nil { + return errors.Wrap(err, "error removing plugin dir") + } + pm.config.Store.Remove(p) pm.config.LogPluginEvent(id, name, "remove") return nil } @@ -659,27 +665,6 @@ func getMounts(root string) ([]string, error) { return mounts, nil } -func recursiveUnmount(root string) error { - mounts, err := getMounts(root) - if err != nil { - return err - } - - // sort in reverse-lexicographic order so the root mount will always be last - sort.Sort(sort.Reverse(sort.StringSlice(mounts))) - - for i, m := range mounts { - if err := mount.Unmount(m); err != nil { - if i == len(mounts)-1 { - return errors.Wrapf(err, "error performing recursive unmount on %s", root) - } - logrus.WithError(err).WithField("mountpoint", m).Warn("could not unmount") - } - } - - return nil -} - // Set sets plugin args func (pm *Manager) Set(name string, args []string) error { p, err := pm.config.Store.GetV2Plugin(name) diff --git a/components/engine/plugin/manager.go b/components/engine/plugin/manager.go index f1c5788a9f..165fdaf8ef 100644 --- a/components/engine/plugin/manager.go +++ b/components/engine/plugin/manager.go @@ -161,6 +161,19 @@ func (pm *Manager) StateChanged(id string, e libcontainerd.StateInfo) error { return nil } +func handleLoadError(err error, id string) { + if err == nil { + return + } + logger := logrus.WithError(err).WithField("id", id) + if os.IsNotExist(errors.Cause(err)) { + // Likely some error while removing on an older version of docker + logger.Warn("missing plugin config, skipping: this may be caused due to a failed remove and requires manual cleanup.") + return + } + logger.Error("error loading plugin, skipping") +} + func (pm *Manager) reload() error { // todo: restore dir, err := ioutil.ReadDir(pm.config.Root) if err != nil { @@ -171,9 +184,17 @@ func (pm *Manager) reload() error { // todo: restore if validFullID.MatchString(v.Name()) { p, err := pm.loadPlugin(v.Name()) if err != nil { - return err + handleLoadError(err, v.Name()) + continue } plugins[p.GetID()] = p + } else { + if validFullID.MatchString(strings.TrimSuffix(v.Name(), "-removing")) { + // There was likely some error while removing this plugin, let's try to remove again here + if err := system.EnsureRemoveAll(v.Name()); err != nil { + logrus.WithError(err).WithField("id", v.Name()).Warn("error while attempting to clean up previously removed plugin") + } + } } } diff --git a/components/engine/plugin/manager_linux.go b/components/engine/plugin/manager_linux.go index 80fc041623..21e3621d9a 100644 --- a/components/engine/plugin/manager_linux.go +++ b/components/engine/plugin/manager_linux.go @@ -203,7 +203,7 @@ func (pm *Manager) upgradePlugin(p *v2.Plugin, configDigest digest.Digest, blobs // Make sure nothing is mounted // This could happen if the plugin was disabled with `-f` with active mounts. // If there is anything in `orig` is still mounted, this should error out. - if err := recursiveUnmount(orig); err != nil { + if err := mount.RecursiveUnmount(orig); err != nil { return err } From 3218b57038a439f9b36db4f8b60f88b3ab00db3c Mon Sep 17 00:00:00 2001 From: Andrew Hsu Date: Tue, 4 Jul 2017 05:00:31 +0000 Subject: [PATCH 2/3] import system for plugin/manager.go To get the cherry-pick ba42966 to merge smoothly, expecting import of system. The import of system was added by https://github.com/docker/docker-ce/commit/8508f49 but not taking entire commit. Signed-off-by: Andrew Hsu --- components/engine/plugin/manager.go | 1 + 1 file changed, 1 insertion(+) diff --git a/components/engine/plugin/manager.go b/components/engine/plugin/manager.go index 165fdaf8ef..411883d248 100644 --- a/components/engine/plugin/manager.go +++ b/components/engine/plugin/manager.go @@ -21,6 +21,7 @@ import ( "github.com/docker/docker/pkg/authorization" "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/mount" + "github.com/docker/docker/pkg/system" "github.com/docker/docker/plugin/v2" "github.com/docker/docker/registry" "github.com/opencontainers/go-digest" From 371fed7778f1c25ead9d8590a8c987d73e6fb127 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 4 Jul 2017 07:37:26 -0400 Subject: [PATCH 3/3] Fix plugin remove dir name after rename. Signed-off-by: Brian Goff (cherry picked from commit 4bf263c19873718394f8161dbc020bf4be30f9d6) Signed-off-by: Andrew Hsu --- components/engine/plugin/backend_linux.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/components/engine/plugin/backend_linux.go b/components/engine/plugin/backend_linux.go index 5c024e9764..2b2cb05443 100644 --- a/components/engine/plugin/backend_linux.go +++ b/components/engine/plugin/backend_linux.go @@ -637,11 +637,12 @@ func (pm *Manager) Remove(name string, config *types.PluginRmConfig) error { return errors.Wrap(err, "error unmounting plugin data") } - if err := os.Rename(pluginDir, pluginDir+"-removing"); err != nil { + removeDir := pluginDir + "-removing" + if err := os.Rename(pluginDir, removeDir); err != nil { return errors.Wrap(err, "error performing atomic remove of plugin dir") } - if err := system.EnsureRemoveAll(pluginDir); err != nil { + if err := system.EnsureRemoveAll(removeDir); err != nil { return errors.Wrap(err, "error removing plugin dir") } pm.config.Store.Remove(p)