From ce01ee60c2749160819d6f7a6a74140c72373028 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Thu, 27 Apr 2017 23:33:33 -0400 Subject: [PATCH] Add refcount for MountPoint This makes sure that multiple users of MountPoint pointer can mount/unmount without affecting each other. Before this PR, if you run a container (stay running), then do `docker cp`, when the `docker cp` is done the MountPoint is mutated such that when the container stops the volume driver will not get an Unmount request. Effectively there would be two mounts with only one unmount. Signed-off-by: Brian Goff Upstream-commit: df0d317a64e4a74433359e826bc1d606e050a5ed Component: engine --- components/engine/container/container.go | 27 +++++++++---------- ...er_cli_external_volume_driver_unix_test.go | 15 +++++++++++ components/engine/volume/volume.go | 26 ++++++++++++++++++ 3 files changed, 54 insertions(+), 14 deletions(-) diff --git a/components/engine/container/container.go b/components/engine/container/container.go index 98173f4321..972c6e5630 100644 --- a/components/engine/container/container.go +++ b/components/engine/container/container.go @@ -400,21 +400,20 @@ func (container *Container) AddMountPointWithVolume(destination string, vol volu func (container *Container) UnmountVolumes(volumeEventLog func(name, action string, attributes map[string]string)) error { var errors []string for _, volumeMount := range container.MountPoints { - // Check if the mountpoint has an ID, this is currently the best way to tell if it's actually mounted - // TODO(cpuguyh83): there should be a better way to handle this - if volumeMount.Volume != nil && volumeMount.ID != "" { - if err := volumeMount.Volume.Unmount(volumeMount.ID); err != nil { - errors = append(errors, err.Error()) - continue - } - volumeMount.ID = "" - - attributes := map[string]string{ - "driver": volumeMount.Volume.DriverName(), - "container": container.ID, - } - volumeEventLog(volumeMount.Volume.Name(), "unmount", attributes) + if volumeMount.Volume == nil { + continue } + + if err := volumeMount.Cleanup(); err != nil { + errors = append(errors, err.Error()) + continue + } + + attributes := map[string]string{ + "driver": volumeMount.Volume.DriverName(), + "container": container.ID, + } + volumeEventLog(volumeMount.Volume.Name(), "unmount", attributes) } if len(errors) > 0 { return fmt.Errorf("error while unmounting volumes for container %s: %s", container.ID, strings.Join(errors, "; ")) diff --git a/components/engine/integration-cli/docker_cli_external_volume_driver_unix_test.go b/components/engine/integration-cli/docker_cli_external_volume_driver_unix_test.go index d6d1999c23..5fe417c2c8 100644 --- a/components/engine/integration-cli/docker_cli_external_volume_driver_unix_test.go +++ b/components/engine/integration-cli/docker_cli_external_volume_driver_unix_test.go @@ -616,3 +616,18 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnmountOnMountFail(c out, _ = s.d.Cmd("run", "-w", "/foo", "-v", "testumount:/foo", "busybox", "true") c.Assert(s.ec.unmounts, checker.Equals, 0, check.Commentf(out)) } + +func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnmountOnCp(c *check.C) { + s.d.StartWithBusybox(c) + s.d.Cmd("volume", "create", "-d", "test-external-volume-driver", "--name=test") + + out, _ := s.d.Cmd("run", "-d", "--name=test", "-v", "test:/foo", "busybox", "/bin/sh", "-c", "touch /test && top") + c.Assert(s.ec.mounts, checker.Equals, 1, check.Commentf(out)) + + out, _ = s.d.Cmd("cp", "test:/test", "/tmp/test") + c.Assert(s.ec.mounts, checker.Equals, 2, check.Commentf(out)) + c.Assert(s.ec.unmounts, checker.Equals, 1, check.Commentf(out)) + + out, _ = s.d.Cmd("kill", "test") + c.Assert(s.ec.unmounts, checker.Equals, 2, check.Commentf(out)) +} diff --git a/components/engine/volume/volume.go b/components/engine/volume/volume.go index d73e2d511f..890ce6ff1e 100644 --- a/components/engine/volume/volume.go +++ b/components/engine/volume/volume.go @@ -120,6 +120,28 @@ type MountPoint struct { // Sepc is a copy of the API request that created this mount. Spec mounttypes.Mount + + // Track usage of this mountpoint + // Specicially needed for containers which are running and calls to `docker cp` + // because both these actions require mounting the volumes. + active int +} + +// Cleanup frees resources used by the mountpoint +func (m *MountPoint) Cleanup() error { + if m.Volume == nil || m.ID == "" { + return nil + } + + if err := m.Volume.Unmount(m.ID); err != nil { + return errors.Wrapf(err, "error unmounting volume %s", m.Volume.Name()) + } + + m.active-- + if m.active == 0 { + m.ID = "" + } + return nil } // Setup sets up a mount point by either mounting the volume if it is @@ -147,12 +169,16 @@ func (m *MountPoint) Setup(mountLabel string, rootUID, rootGID int) (path string if err != nil { return "", errors.Wrapf(err, "error while mounting volume '%s'", m.Source) } + m.ID = id + m.active++ return path, nil } + if len(m.Source) == 0 { return "", fmt.Errorf("Unable to setup mount point, neither source nor volume defined") } + // system.MkdirAll() produces an error if m.Source exists and is a file (not a directory), if m.Type == mounttypes.TypeBind { // idtools.MkdirAllNewAs() produces an error if m.Source exists and is a file (not a directory)