copy command: bug fix and enhance

.gitignore

@@ -13,3 +13,4 @@ coverage.txt
 .DS_Store
 bin/
 coverage/
+test/testdata/cp/*

daemon/mgr/container_copy.go

@@ -14,8 +14,11 @@ import (
 
 	"github.com/docker/docker/pkg/archive"
 	"github.com/docker/docker/pkg/chrootarchive"
+	"github.com/docker/docker/pkg/fileutils"
 	"github.com/docker/docker/pkg/mount"
 	"github.com/go-openapi/strfmt"
+	pkgerrors "github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 )
 
 // StatPath stats the dir info at the specified path in the container.
@@ -27,27 +30,31 @@ func (mgr *ContainerManager) StatPath(ctx context.Context, name, path string) (s
 	c.Lock()
 	defer c.Unlock()
 
+	if c.State.Dead {
+		return nil, pkgerrors.Errorf("container(%s) has been deleted", c.ID)
+	}
+
 	running := c.IsRunningOrPaused()
 	err = mgr.Mount(ctx, c, false)
 	if err != nil {
-		return nil, err
+		return nil, pkgerrors.Wrapf(err, "failed to mount cid(%s)", c.ID)
 	}
 
 	defer mgr.Unmount(ctx, c, false, !running)
 
 	if !running {
 		err = mgr.attachVolumes(ctx, c)
 		if err != nil {
-			return nil, err
+			return nil, pkgerrors.Wrapf(err, "failed to attachVolumes cid(%s)", c.ID)
 		}
 		defer mgr.detachVolumes(ctx, c, false)
 	}
 
-	err = c.mountVolumes(!running)
+	err = c.mountVolumes()
 	if err != nil {
-		return nil, err
+		return nil, pkgerrors.Wrapf(err, "failed to mountVolumes cid(%s)", c.ID)
 	}
-	defer c.unmountVolumes(!running)
+	defer c.unmountVolumes()
 
 	resolvedPath, absPath := c.getResolvedPath(path)
 	lstat, err := os.Lstat(resolvedPath)
@@ -78,10 +85,14 @@ func (mgr *ContainerManager) ArchivePath(ctx context.Context, name, path string)
 		}
 	}()
 
+	if c.State.Dead {
+		return nil, nil, pkgerrors.Errorf("container(%s) has been deleted", c.ID)
+	}
+
 	running := c.IsRunningOrPaused()
 	err = mgr.Mount(ctx, c, false)
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, pkgerrors.Wrapf(err, "failed to mount cid(%s)", c.ID)
 	}
 	defer func() {
 		if err0 != nil {
@@ -92,7 +103,7 @@ func (mgr *ContainerManager) ArchivePath(ctx context.Context, name, path string)
 	if !running {
 		err = mgr.attachVolumes(ctx, c)
 		if err != nil {
-			return nil, nil, err
+			return nil, nil, pkgerrors.Wrapf(err, "failed to attachVolumes cid(%s)", c.ID)
 		}
 		defer func() {
 			if err0 != nil {
@@ -101,13 +112,13 @@ func (mgr *ContainerManager) ArchivePath(ctx context.Context, name, path string)
 		}()
 	}
 
-	err = c.mountVolumes(!running)
+	err = c.mountVolumes()
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, pkgerrors.Wrapf(err, "failed to mountVolumes cid(%s)", c.ID)
 	}
 	defer func() {
 		if err0 != nil {
-			defer c.unmountVolumes(!running)
+			defer c.unmountVolumes()
 		}
 	}()
 
@@ -140,7 +151,7 @@ func (mgr *ContainerManager) ArchivePath(ctx context.Context, name, path string)
 		if !running {
 			mgr.detachVolumes(ctx, c, false)
 		}
-		c.unmountVolumes(!running)
+		c.unmountVolumes()
 		mgr.Unmount(ctx, c, false, !running)
 		c.Unlock()
 		return err
@@ -158,26 +169,30 @@ func (mgr *ContainerManager) ExtractToDir(ctx context.Context, name, path string
 	c.Lock()
 	defer c.Unlock()
 
+	if c.State.Dead {
+		return pkgerrors.Errorf("container(%s) has been deleted", c.ID)
+	}
+
 	running := c.IsRunningOrPaused()
 	err = mgr.Mount(ctx, c, false)
 	if err != nil {
-		return err
+		return pkgerrors.Wrapf(err, "failed to mount cid(%s)", c.ID)
 	}
 	defer mgr.Unmount(ctx, c, false, !running)
 
 	if !running {
 		err = mgr.attachVolumes(ctx, c)
 		if err != nil {
-			return err
+			return pkgerrors.Wrapf(err, "failed to attachVolumes cid(%s)", c.ID)
 		}
 		defer mgr.detachVolumes(ctx, c, false)
 	}
 
-	err = c.mountVolumes(!running)
+	err = c.mountVolumes()
 	if err != nil {
-		return err
+		return pkgerrors.Wrapf(err, "failed to mountVolumes cid(%s)", c.ID)
 	}
-	defer c.unmountVolumes(!running)
+	defer c.unmountVolumes()
 
 	resolvedPath, _ := c.getResolvedPath(path)
 
@@ -229,20 +244,38 @@ func (c *Container) getResolvedPath(path string) (resolvedPath, absPath string)
 	return resolvedPath, absPath
 }
 
-func (c *Container) mountVolumes(created bool) error {
+func (c *Container) mountVolumes() (err0 error) {
+	rollbackMounts := make([]string, 0, len(c.Mounts))
+
+	defer func() {
+		if err0 != nil {
+			for _, dest := range rollbackMounts {
+				if err := mount.Unmount(dest); err != nil {
+					logrus.Warnf("[mountVolumes:rollback] failed to unmount(%s), err(%v)", dest, err)
+				} else {
+					logrus.Debugf("[mountVolumes:rollback] unmount(%s)", dest)
+				}
+			}
+		}
+	}()
 
 	for _, m := range c.Mounts {
 		dest, _ := c.getResolvedPath(m.Destination)
 
-		_, err := os.Stat(m.Source)
+		logrus.Debugf("try to mount volume(source %s -> dest %s", m.Source, dest)
+
+		stat, err := os.Stat(m.Source)
 		if err != nil {
 			return err
 		}
 
-		if created {
-			if e := os.MkdirAll(dest, 0755); e != nil {
-				return e
-			}
+		// /dev /proc /tmp is mounted by default oci spec. Those mount
+		// points are mounted by runC. In k8s, the daemonset will mount
+		// log into /dev/termination-log. Before mount it, we need to
+		// create folder or empty file for the target. It will not
+		// impact the running container.
+		if err = fileutils.CreateIfNotExists(dest, stat.IsDir()); err != nil {
+			return err
 		}
 
 		writeMode := "ro"
@@ -263,23 +296,20 @@ func (c *Container) mountVolumes(created bool) error {
 		if err := mount.Mount(m.Source, dest, "", opts); err != nil {
 			return err
 		}
+
+		rollbackMounts = append(rollbackMounts, dest)
 	}
 
 	return nil
 }
 
-func (c *Container) unmountVolumes(remove bool) error {
+func (c *Container) unmountVolumes() error {
 	for _, m := range c.Mounts {
 		dest, _ := c.getResolvedPath(m.Destination)
 
 		if err := mount.Unmount(dest); err != nil {
 			return err
 		}
-		if remove {
-			if err := os.RemoveAll(dest); err != nil {
-				return err
-			}
-		}
 	}
 	return nil
 }

daemon/mgr/container_storage.go

@@ -668,7 +668,19 @@ func (mgr *ContainerManager) detachVolumes(ctx context.Context, c *Container, re
 	return nil
 }
 
-func (mgr *ContainerManager) attachVolumes(ctx context.Context, c *Container) error {
+func (mgr *ContainerManager) attachVolumes(ctx context.Context, c *Container) (err0 error) {
+	rollbackVolumes := make([]string, 0, len(c.Mounts))
+
+	defer func() {
+		if err0 != nil {
+			for _, name := range rollbackVolumes {
+				if _, err := mgr.VolumeMgr.Detach(ctx, name, map[string]string{volumetypes.OptionRef: c.ID}); err != nil {
+					logrus.Warnf("[rollback] failed to detach volume(%s), err(%v)", name, err)
+				}
+			}
+		}
+	}()
+
 	for _, mount := range c.Mounts {
 		name := mount.Name
 		if name == "" {
@@ -680,6 +692,8 @@ func (mgr *ContainerManager) attachVolumes(ctx context.Context, c *Container) er
 			logrus.Warnf("failed to attach volume(%s), err(%v)", name, err)
 			return err
 		}
+
+		rollbackVolumes = append(rollbackVolumes, name)
 	}
 	return nil
 }

test/cli_container_cp_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"io/ioutil"
 	"os"
 
 	"github.com/alibaba/pouch/test/command"
@@ -15,8 +16,6 @@ import (
 // APIContainerCopySuite is the test suite for container cp CLI.
 type PouchContainerCopySuite struct{}
 
-var testDataPath = "testdata/cp"
-
 func init() {
 	check.Suite(&PouchContainerCopySuite{})
 }
@@ -25,18 +24,14 @@ func init() {
 func (suite *PouchContainerCopySuite) SetUpSuite(c *check.C) {
 	SkipIfFalse(c, environment.IsLinux)
 	PullImage(c, busyboxImage)
-	err := os.Mkdir(testDataPath, 0755)
-	c.Assert(err, check.IsNil)
-}
-
-// TearDownSuite does cleanup work in the end of each test suite.
-func (suite *PouchContainerCopySuite) TearDownSuite(c *check.C) {
-	err := os.RemoveAll(testDataPath)
-	c.Assert(err, check.IsNil)
 }
 
 // Test pouch cp, basic usage
 func (suite *PouchContainerCopySuite) TestPouchCopy(c *check.C) {
+	testDataPath, err := ioutil.TempDir("", "test-pouch-copy")
+	c.Assert(err, check.IsNil)
+	defer os.RemoveAll(testDataPath)
+
 	// test copy from container
 	name := "TestPouchCopy"
 	command.PouchRun("run",
@@ -56,7 +51,7 @@ func (suite *PouchContainerCopySuite) TestPouchCopy(c *check.C) {
 	command.PouchRun("cp", localTestPath, containerTestPath).Assert(c, icmd.Success)
 	res := command.PouchRun("exec", name, "cat", "test.txt")
 	res.Assert(c, icmd.Success)
-	err := util.PartialEqual(res.Stdout(), "test pouch cp")
+	err = util.PartialEqual(res.Stdout(), "test pouch cp")
 	c.Assert(err, check.IsNil)
 
 	// test copy to container with non-dir
@@ -67,6 +62,10 @@ func (suite *PouchContainerCopySuite) TestPouchCopy(c *check.C) {
 
 // Test pouch cp, where dir locate in volume
 func (suite *PouchContainerCopySuite) TestVolumeCopy(c *check.C) {
+	testDataPath, err := ioutil.TempDir("", "test-volume-copy")
+	c.Assert(err, check.IsNil)
+	defer os.RemoveAll(testDataPath)
+
 	// test mount rw and copy from container
 	name := "TestVolumeCopy"
 	command.PouchRun("volume", "create", "--name", name).Assert(c, icmd.Success)
@@ -90,7 +89,7 @@ func (suite *PouchContainerCopySuite) TestVolumeCopy(c *check.C) {
 	command.PouchRun("cp", localTestPath, containerTestPath).Assert(c, icmd.Success)
 	res := command.PouchRun("exec", name, "cat", "/test/test.txt")
 	res.Assert(c, icmd.Success)
-	err := util.PartialEqual(res.Stdout(), "test pouch cp")
+	err = util.PartialEqual(res.Stdout(), "test pouch cp")
 	c.Assert(err, check.IsNil)
 
 	// test mount only ro
@@ -111,6 +110,10 @@ func (suite *PouchContainerCopySuite) TestVolumeCopy(c *check.C) {
 
 // TestStopContainerCopy tests stopped container can work well
 func (suite *PouchContainerCopySuite) TestStopContainerCopy(c *check.C) {
+	testDataPath := "testdata/cp/test-stop-container-copy"
+	c.Assert(os.MkdirAll(testDataPath, 0755), check.IsNil)
+	defer os.RemoveAll(testDataPath)
+
 	name := "TestStopContainerCopy"
 	command.PouchRun("run", "-d",
 		"--name", name,