From c56beddcc1f8601ff657d8af9703e72f1fb7af40 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 6 Sep 2019 13:52:13 -0400 Subject: [PATCH] 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 --- libpod/container_internal.go | 70 ++++++++++++++++++++---------- libpod/container_internal_linux.go | 4 +- libpod/runtime_ctr.go | 4 -- libpod/volume.go | 7 +++ libpod/volume_internal.go | 2 + 5 files changed, 59 insertions(+), 28 deletions(-) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index ffc6c11ee878..59f3f311d287 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1236,29 +1236,20 @@ func (c *Container) mountStorage() (_ string, Err error) { // 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) 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 @@ -1273,6 +1264,42 @@ func (c *Container) mountStorage() (_ string, Err error) { 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) (*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()) + if err := c.copyWithTarFromImage(v.Dest, 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,13 +1641,10 @@ func (c *Container) unmount(force bool) error { } // this should be from chrootarchive. +// Container MUST be mounted before calling. func (c *Container) copyWithTarFromImage(src, dest string) error { - mountpoint, err := c.mount() - if err != nil { - return err - } a := archive.NewDefaultArchiver() - source := filepath.Join(mountpoint, src) + source := filepath.Join(c.state.Mountpint, src) if err = c.copyOwnerAndPerms(source, dest); err != nil { return err diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index e96af8536a89..03374a060ece 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 acd317d20947..0b4fbda7a2a3 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 b4de3aedcac8..c4771bbb81d3 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 2e886e1b0fc8..42b935e7ca8e 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 }