From 05cc737f5411a0effd299429140d031c4ad8dd05 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 29 Mar 2016 15:27:04 -0700 Subject: [PATCH] Fix container mount cleanup issues - Refactor generic and path based cleanup functions into a single function. - Include aufs and zfs mounts in the mounts cleanup. - Containers that receive exit event on restore don't require manual cleanup. - Make missing sandbox id message a warning because currently sandboxes are always cleared on startup. libnetwork#975 - Don't unmount volumes for containers that don't have base path. Shouldn't be needed after #21372 Signed-off-by: Tonis Tiigi --- daemon/container_operations.go | 2 +- daemon/daemon.go | 4 - daemon/daemon_linux.go | 90 ++++++------------- daemon/daemon_linux_test.go | 4 +- daemon/start.go | 6 +- libcontainerd/client_shutdownrestore_linux.go | 2 + 6 files changed, 35 insertions(+), 73 deletions(-) diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 45c6c2acd6..310f9bb497 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -720,7 +720,7 @@ func (daemon *Daemon) releaseNetwork(container *container.Container) { sb, err := daemon.netController.SandboxByID(sid) if err != nil { - logrus.Errorf("error locating sandbox id %s: %v", sid, err) + logrus.Warnf("error locating sandbox id %s: %v", sid, err) return } diff --git a/daemon/daemon.go b/daemon/daemon.go index e51be663bc..73cc54b5b9 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -304,10 +304,6 @@ func (daemon *Daemon) restore() error { mapLock.Lock() restartContainers[c] = make(chan struct{}) mapLock.Unlock() - } else if !c.IsRunning() && !c.IsPaused() { - if mountid, err := daemon.layerStore.GetMountID(c.ID); err == nil { - daemon.cleanupMountsByID(mountid) - } } // if c.hostConfig.Links is nil (not just empty), then it is using the old sqlite links and needs to be migrated diff --git a/daemon/daemon_linux.go b/daemon/daemon_linux.go index deb3291155..9bdf6e2b79 100644 --- a/daemon/daemon_linux.go +++ b/daemon/daemon_linux.go @@ -5,7 +5,7 @@ import ( "fmt" "io" "os" - "path/filepath" + "regexp" "strings" "github.com/Sirupsen/logrus" @@ -28,91 +28,53 @@ func (daemon *Daemon) cleanupMountsFromReaderByID(reader io.Reader, id string, u return nil } var errors []string - mountRoot := "" - shmSuffix := "/" + id + "/shm" - mergedSuffix := "/" + id + "/merged" + + regexps := getCleanPatterns(id) sc := bufio.NewScanner(reader) for sc.Scan() { - line := sc.Text() - fields := strings.Fields(line) - if strings.HasPrefix(fields[4], daemon.root) { - logrus.Debugf("Mount base: %v", fields[4]) - mnt := fields[4] - if strings.HasSuffix(mnt, shmSuffix) || strings.HasSuffix(mnt, mergedSuffix) { - logrus.Debugf("Unmounting %v", mnt) - if err := unmount(mnt); err != nil { - logrus.Error(err) - errors = append(errors, err.Error()) + if fields := strings.Fields(sc.Text()); len(fields) >= 4 { + if mnt := fields[4]; strings.HasPrefix(mnt, daemon.root) { + for _, p := range regexps { + if p.MatchString(mnt) { + if err := unmount(mnt); err != nil { + logrus.Error(err) + errors = append(errors, err.Error()) + } + } } - } else if mountBase := filepath.Base(mnt); mountBase == id { - mountRoot = mnt } } } - if mountRoot != "" { - logrus.Debugf("Unmounting %v", mountRoot) - if err := unmount(mountRoot); err != nil { - logrus.Error(err) - errors = append(errors, err.Error()) - } - } - if err := sc.Err(); err != nil { return err } if len(errors) > 0 { - return fmt.Errorf("Error cleaningup mounts:\n%v", strings.Join(errors, "\n")) + return fmt.Errorf("Error cleaning up mounts:\n%v", strings.Join(errors, "\n")) } - logrus.Debugf("Cleaning up old container shm/mqueue/rootfs mounts: done.") + logrus.Debugf("Cleaning up old mountid %v: done.", id) return nil } // cleanupMounts umounts shm/mqueue mounts for old containers func (daemon *Daemon) cleanupMounts() error { - logrus.Debugf("Cleaning up old container shm/mqueue/rootfs mounts: start.") - f, err := os.Open("/proc/self/mountinfo") - if err != nil { - return err - } - defer f.Close() - - return daemon.cleanupMountsFromReader(f, mount.Unmount) + return daemon.cleanupMountsByID("") } -func (daemon *Daemon) cleanupMountsFromReader(reader io.Reader, unmount func(target string) error) error { - if daemon.root == "" { - return nil +func getCleanPatterns(id string) (regexps []*regexp.Regexp) { + var patterns []string + if id == "" { + id = "[0-9a-f]{64}" + patterns = append(patterns, "containers/"+id+"/shm") } - sc := bufio.NewScanner(reader) - var errors []string - for sc.Scan() { - line := sc.Text() - fields := strings.Fields(line) - if strings.HasPrefix(fields[4], daemon.root) { - logrus.Debugf("Mount base: %v", fields[4]) - mnt := fields[4] - mountBase := filepath.Base(mnt) - if mountBase == "shm" || mountBase == "merged" { - logrus.Debugf("Unmounting %v", mnt) - if err := unmount(mnt); err != nil { - logrus.Error(err) - errors = append(errors, err.Error()) - } - } + patterns = append(patterns, "aufs/mnt/"+id+"$", "overlay/"+id+"/merged$", "zfs/graph/"+id+"$") + for _, p := range patterns { + r, err := regexp.Compile(p) + if err == nil { + regexps = append(regexps, r) } } - - if err := sc.Err(); err != nil { - return err - } - - if len(errors) > 0 { - return fmt.Errorf("Error cleaningup mounts:\n%v", strings.Join(errors, "\n")) - } - - logrus.Debugf("Cleaning up old container shm/mqueue/rootfs mounts: done.") - return nil + return } diff --git a/daemon/daemon_linux_test.go b/daemon/daemon_linux_test.go index 672d8fc72e..c40b13ba4c 100644 --- a/daemon/daemon_linux_test.go +++ b/daemon/daemon_linux_test.go @@ -59,7 +59,7 @@ func TestCleanupMounts(t *testing.T) { return nil } - d.cleanupMountsFromReader(strings.NewReader(mountsFixture), unmount) + d.cleanupMountsFromReaderByID(strings.NewReader(mountsFixture), "", unmount) if unmounted != 1 { t.Fatalf("Expected to unmount the shm (and the shm only)") @@ -97,7 +97,7 @@ func TestNotCleanupMounts(t *testing.T) { return nil } mountInfo := `234 232 0:59 / /dev/shm rw,nosuid,nodev,noexec,relatime - tmpfs shm rw,size=65536k` - d.cleanupMountsFromReader(strings.NewReader(mountInfo), unmount) + d.cleanupMountsFromReaderByID(strings.NewReader(mountInfo), "", unmount) if unmounted { t.Fatalf("Expected not to clean up /dev/shm") } diff --git a/daemon/start.go b/daemon/start.go index 7f5173773c..a8504c0ff0 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -174,8 +174,10 @@ func (daemon *Daemon) Cleanup(container *container.Container) { daemon.unregisterExecCommand(container, eConfig) } - if err := container.UnmountVolumes(false, daemon.LogVolumeEvent); err != nil { - logrus.Warnf("%s cleanup: Failed to umount volumes: %v", container.ID, err) + if container.BaseFS != "" { + if err := container.UnmountVolumes(false, daemon.LogVolumeEvent); err != nil { + logrus.Warnf("%s cleanup: Failed to umount volumes: %v", container.ID, err) + } } container.CancelAttachContext() } diff --git a/libcontainerd/client_shutdownrestore_linux.go b/libcontainerd/client_shutdownrestore_linux.go index 9d32b1d6ca..52ea2a6180 100644 --- a/libcontainerd/client_shutdownrestore_linux.go +++ b/libcontainerd/client_shutdownrestore_linux.go @@ -31,8 +31,10 @@ func (clnt *client) Restore(containerID string, options ...CreateOption) error { select { case <-time.After(2 * time.Second): case <-w.wait(): + return nil } case <-w.wait(): + return nil } } return clnt.setExited(containerID)