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>

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,45 +1235,84 @@ func (c *Container) mountStorage() (_ string, Err error) {
 		}()
 	}
 
-	// Request a mount of all named volumes
+	// We need to mount the container before volumes - to ensure the copyup
-	for _, v := range c.config.NamedVolumes {
+	// works properly.
-		vol, err := c.runtime.state.Volume(v.Name)
-		if err != nil {
-			return "", errors.Wrapf(err, "error retrieving named volume %s for container %s", v.Name, c.ID())
-		}
-
-		if vol.needsMount() {
-			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())
-			}
-			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
 		}
+		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.mountNamedVolume(v, mountPoint)
+		if err != nil {
+			return "", err
+		}
+		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()
+		}()
 	}
 
 	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())
+		}
+
+		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
 func (c *Container) cleanupStorage() error {
 	if !c.state.Mounted {
@@ -1614,15 +1654,11 @@ func (c *Container) unmount(force bool) error {
 }
 
 // this should be from chrootarchive.
-func (c *Container) copyWithTarFromImage(src, dest string) error {
+// Container MUST be mounted before calling.
-	mountpoint, err := c.mount()
+func (c *Container) copyWithTarFromImage(source, dest string) error {
-	if err != nil {
-		return err
-	}
 	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)

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
 	}
 

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)
 	}
 

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

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
 }

test/e2e/run_volume_test.go

@@ -249,4 +249,25 @@ var _ = Describe("Podman run with volumes", func() {
 		fmt.Printf("Output: %s", mountOut3)
 		Expect(strings.Contains(mountOut3, volName)).To(BeFalse())
 	})
+
+	It("podman named volume copyup", func() {
+		baselineSession := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", ALPINE, "ls", "/etc/apk/"})
+		baselineSession.WaitWithDefaultTimeout()
+		Expect(baselineSession.ExitCode()).To(Equal(0))
+		baselineOutput := baselineSession.OutputToString()
+
+		inlineVolumeSession := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", "-v", "testvol1:/etc/apk", ALPINE, "ls", "/etc/apk/"})
+		inlineVolumeSession.WaitWithDefaultTimeout()
+		Expect(inlineVolumeSession.ExitCode()).To(Equal(0))
+		Expect(inlineVolumeSession.OutputToString()).To(Equal(baselineOutput))
+
+		makeVolumeSession := podmanTest.Podman([]string{"volume", "create", "testvol2"})
+		makeVolumeSession.WaitWithDefaultTimeout()
+		Expect(makeVolumeSession.ExitCode()).To(Equal(0))
+
+		separateVolumeSession := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", "-v", "testvol2:/etc/apk", ALPINE, "ls", "/etc/apk/"})
+		separateVolumeSession.WaitWithDefaultTimeout()
+		Expect(separateVolumeSession.ExitCode()).To(Equal(0))
+		Expect(separateVolumeSession.OutputToString()).To(Equal(baselineOutput))
+	})
 })