When first mounting any named volume, copy up

Previously, we only did this for volumes created at the same time
as the container. However, this is not correct behavior - Docker
does so for all named volumes, even those made with
'podman volume create' and mounted into a container later.

Fixes #3945

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

5 files changed, 80 insertions, 37 deletions

diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index ffc6c11ee..78305d761 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -21,6 +21,7 @@ import (
 	"github.com/containers/storage"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/mount"
+	"github.com/cyphar/filepath-securejoin"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -1234,43 +1235,82 @@ func (c *Container) mountStorage() (_ string, Err error) {
 		}()
 	}
 
+	// We need to mount the container before volumes - to ensure the copyup
+	// works properly.
+	mountPoint := c.config.Rootfs
+	if mountPoint == "" {
+		mountPoint, err = c.mount()
+		if err != nil {
+			return "", err
+		}
+		defer func() {
+			if Err != nil {
+				if err := c.unmount(false); err != nil {
+					logrus.Errorf("Error unmounting container %s after mount error: %v", c.ID(), err)
+				}
+			}
+		}()
+	}
+
 	// Request a mount of all named volumes
 	for _, v := range c.config.NamedVolumes {
-		vol, err := c.runtime.state.Volume(v.Name)
+		vol, err := c.mountNamedVolume(v, mountPoint)
 		if err != nil {
-			return "", errors.Wrapf(err, "error retrieving named volume %s for container %s", v.Name, c.ID())
+			return "", err
 		}
-
-		if vol.needsMount() {
+		defer func() {
+			if Err == nil {
+				return
+			}
 			vol.lock.Lock()
-			if err := vol.mount(); err != nil {
-				vol.lock.Unlock()
-				return "", errors.Wrapf(err, "error mounting volume %s for container %s", vol.Name(), c.ID())
+			if err := vol.unmount(false); err != nil {
+				logrus.Errorf("Error unmounting volume %s after error mounting container %s: %v", vol.Name(), c.ID(), err)
 			}
 			vol.lock.Unlock()
-			defer func() {
-				if Err == nil {
-					return
-				}
-				vol.lock.Lock()
-				if err := vol.unmount(false); err != nil {
-					logrus.Errorf("Error unmounting volume %s after error mounting container %s: %v", vol.Name(), c.ID(), err)
-				}
-				vol.lock.Unlock()
-			}()
-		}
+		}()
 	}
 
-	// TODO: generalize this mount code so it will mount every mount in ctr.config.Mounts
-	mountPoint := c.config.Rootfs
-	if mountPoint == "" {
-		mountPoint, err = c.mount()
-		if err != nil {
-			return "", err
+	return mountPoint, nil
+}
+
+// Mount a single named volume into the container.
+// If necessary, copy up image contents into the volume.
+// Does not verify that the name volume given is actually present in container
+// config.
+// Returns the volume that was mounted.
+func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string) (*Volume, error) {
+	vol, err := c.runtime.state.Volume(v.Name)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error retrieving named volume %s for container %s", v.Name, c.ID())
+	}
+
+	vol.lock.Lock()
+	defer vol.lock.Unlock()
+	if vol.needsMount() {
+		if err := vol.mount(); err != nil {
+			return nil, errors.Wrapf(err, "error mounting volume %s for container %s", vol.Name(), c.ID())
 		}
 	}
+	// The volume may need a copy-up. Check the state.
+	if err := vol.update(); err != nil {
+		return nil, err
+	}
+	if vol.state.NeedsCopyUp {
+		logrus.Debugf("Copying up contents from container %s to volume %s", c.ID(), vol.Name())
+		srcDir, err := securejoin.SecureJoin(mountpoint, v.Dest)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error calculating destination path to copy up container %s volume %s", c.ID(), vol.Name())
+		}
+		if err := c.copyWithTarFromImage(srcDir, vol.MountPoint()); err != nil && !os.IsNotExist(err) {
+			return nil, errors.Wrapf(err, "error copying content from container %s into volume %s", c.ID(), vol.Name())
+		}
 
-	return mountPoint, nil
+		vol.state.NeedsCopyUp = false
+		if err := vol.save(); err != nil {
+			return nil, err
+		}
+	}
+	return vol, nil
 }
 
 // cleanupStorage unmounts and cleans up the container's root filesystem
@@ -1614,15 +1654,11 @@ func (c *Container) unmount(force bool) error {
 }
 
 // this should be from chrootarchive.
-func (c *Container) copyWithTarFromImage(src, dest string) error {
-	mountpoint, err := c.mount()
-	if err != nil {
-		return err
-	}
+// Container MUST be mounted before calling.
+func (c *Container) copyWithTarFromImage(source, dest string) error {
 	a := archive.NewDefaultArchiver()
-	source := filepath.Join(mountpoint, src)
 
-	if err = c.copyOwnerAndPerms(source, dest); err != nil {
+	if err := c.copyOwnerAndPerms(source, dest); err != nil {
 		return err
 	}
 	return a.CopyWithTar(source, dest)
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index e96af8536..03374a060 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -115,7 +115,9 @@ func (c *Container) prepare() (Err error) {
 		createErr = createNetNSErr
 	}
 	if mountStorageErr != nil {
-		logrus.Errorf("Error preparing container %s: %v", c.ID(), createErr)
+		if createErr != nil {
+			logrus.Errorf("Error preparing container %s: %v", c.ID(), createErr)
+		}
 		createErr = mountStorageErr
 	}
 
diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go
index acd317d20..0b4fbda7a 100644
--- a/libpod/runtime_ctr.go
+++ b/libpod/runtime_ctr.go
@@ -275,10 +275,6 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (c *Contai
 			return nil, errors.Wrapf(err, "error creating named volume %q", vol.Name)
 		}
 
-		if err := ctr.copyWithTarFromImage(vol.Dest, newVol.MountPoint()); err != nil && !os.IsNotExist(err) {
-			return nil, errors.Wrapf(err, "Failed to copy content into new volume mount %q", vol.Name)
-		}
-
 		ctrNamedVolumes = append(ctrNamedVolumes, newVol)
 	}
 
diff --git a/libpod/volume.go b/libpod/volume.go
index b4de3aedc..c4771bbb8 100644
--- a/libpod/volume.go
+++ b/libpod/volume.go
@@ -57,6 +57,13 @@ type VolumeState struct {
 	// On incrementing from 0, the volume will be mounted on the host.
 	// On decrementing to 0, the volume will be unmounted on the host.
 	MountCount uint `json:"mountCount"`
+	// NeedsCopyUp indicates that the next time the volume is mounted into
+	// a container, the container will "copy up" the contents of the
+	// mountpoint into the volume.
+	// This should only be done once. As such, this is set at container
+	// create time, then cleared after the copy up is done and never set
+	// again.
+	NeedsCopyUp bool `json:"notYetMounted,omitempty"`
 }
 
 // Name retrieves the volume's name
diff --git a/libpod/volume_internal.go b/libpod/volume_internal.go
index 2e886e1b0..42b935e7c 100644
--- a/libpod/volume_internal.go
+++ b/libpod/volume_internal.go
@@ -11,9 +11,11 @@ import (
 func newVolume(runtime *Runtime) (*Volume, error) {
 	volume := new(Volume)
 	volume.config = new(VolumeConfig)
+	volume.state = new(VolumeState)
 	volume.runtime = runtime
 	volume.config.Labels = make(map[string]string)
 	volume.config.Options = make(map[string]string)
+	volume.state.NeedsCopyUp = true
 
 	return volume, nil
 }