Fix volume handling in podman

iFix builtin volumes to work with podman volume

Currently builtin volumes are not recored in podman volumes when
they are created automatically. This patch fixes this.

Remove container volumes when requested

Currently the --volume option on podman remove does nothing.
This will implement the changes needed to remove the volumes
if the user requests it.

When removing a volume make sure that no container uses the volume.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

8 files changed, 57 insertions, 132 deletions

diff --git a/libpod/adapter/runtime_remote.go b/libpod/adapter/runtime_remote.go
index 66ff5ed51..a96676ee2 100644
--- a/libpod/adapter/runtime_remote.go
+++ b/libpod/adapter/runtime_remote.go
@@ -544,7 +544,7 @@ func (r *LocalRuntime) GetContainers(filters ...libpod.ContainerFilter) ([]*libp
 // RemoveContainer removes the given container
 // If force is specified, the container will be stopped first
 // Otherwise, RemoveContainer will return an error if the container is running
-func (r *LocalRuntime) RemoveContainer(ctx context.Context, c *libpod.Container, force bool) error {
+func (r *LocalRuntime) RemoveContainer(ctx context.Context, c *libpod.Container, force, volumes bool) error {
 	return libpod.ErrNotImplemented
 }
 
diff --git a/libpod/container.go b/libpod/container.go
index fec61533d..75f4a4a4f 100644
--- a/libpod/container.go
+++ b/libpod/container.go
@@ -358,8 +358,7 @@ type ContainerConfig struct {
 	ExitCommand []string `json:"exitCommand,omitempty"`
 	// LocalVolumes are the built-in volumes we get from the --volumes-from flag
 	// It picks up the built-in volumes of the container used by --volumes-from
-	LocalVolumes []string
-
+	LocalVolumes []spec.Mount
 	// IsInfra is a bool indicating whether this container is an infra container used for
 	// sharing kernel namespaces in a pod
 	IsInfra bool `json:"pause"`
diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index f82cbd674..b2ebad777 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -10,21 +10,16 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
-	"syscall"
 	"time"
 
-	"github.com/containers/buildah/imagebuildah"
 	"github.com/containers/libpod/pkg/ctime"
 	"github.com/containers/libpod/pkg/hooks"
 	"github.com/containers/libpod/pkg/hooks/exec"
 	"github.com/containers/libpod/pkg/rootless"
 	"github.com/containers/storage"
 	"github.com/containers/storage/pkg/archive"
-	"github.com/containers/storage/pkg/chrootarchive"
 	"github.com/containers/storage/pkg/mount"
-	"github.com/opencontainers/runc/libcontainer/user"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
-	"github.com/opencontainers/runtime-tools/generate"
 	"github.com/opencontainers/selinux/go-selinux/label"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -1053,113 +1048,6 @@ func (c *Container) writeStringToRundir(destFile, output string) (string, error)
 	return filepath.Join(c.state.DestinationRunDir, destFile), nil
 }
 
-func (c *Container) addLocalVolumes(ctx context.Context, g *generate.Generator, execUser *user.ExecUser) error {
-	var uid, gid int
-	mountPoint := c.state.Mountpoint
-	if !c.state.Mounted {
-		return errors.Wrapf(ErrInternal, "container is not mounted")
-	}
-	newImage, err := c.runtime.imageRuntime.NewFromLocal(c.config.RootfsImageID)
-	if err != nil {
-		return err
-	}
-	imageData, err := newImage.Inspect(ctx)
-	if err != nil {
-		return err
-	}
-	// Add the built-in volumes of the container passed in to --volumes-from
-	for _, vol := range c.config.LocalVolumes {
-		if imageData.Config.Volumes == nil {
-			imageData.Config.Volumes = map[string]struct{}{
-				vol: {},
-			}
-		} else {
-			imageData.Config.Volumes[vol] = struct{}{}
-		}
-	}
-
-	if c.config.User != "" {
-		if execUser == nil {
-			return errors.Wrapf(ErrInternal, "nil pointer passed to addLocalVolumes for execUser")
-		}
-		uid = execUser.Uid
-		gid = execUser.Gid
-	}
-
-	for k := range imageData.Config.Volumes {
-		mount := spec.Mount{
-			Destination: k,
-			Type:        "bind",
-			Options:     []string{"private", "bind", "rw"},
-		}
-		if MountExists(g.Mounts(), k) {
-			continue
-		}
-		volumePath := filepath.Join(c.config.StaticDir, "volumes", k)
-
-		// Ensure the symlinks are resolved
-		resolvedSymlink, err := imagebuildah.ResolveSymLink(mountPoint, k)
-		if err != nil {
-			return errors.Wrapf(ErrCtrStateInvalid, "cannot resolve %s in %s for container %s", k, mountPoint, c.ID())
-		}
-		var srcPath string
-		if resolvedSymlink != "" {
-			srcPath = filepath.Join(mountPoint, resolvedSymlink)
-		} else {
-			srcPath = filepath.Join(mountPoint, k)
-		}
-
-		if _, err := os.Stat(srcPath); os.IsNotExist(err) {
-			logrus.Infof("Volume image mount point %s does not exist in root FS, need to create it", k)
-			if err = os.MkdirAll(srcPath, 0755); err != nil {
-				return errors.Wrapf(err, "error creating directory %q for volume %q in container %q", volumePath, k, c.ID())
-			}
-
-			if err = os.Chown(srcPath, uid, gid); err != nil {
-				return errors.Wrapf(err, "error chowning directory %q for volume %q in container %q", srcPath, k, c.ID())
-			}
-		}
-
-		if _, err := os.Stat(volumePath); os.IsNotExist(err) {
-			if err = os.MkdirAll(volumePath, 0755); err != nil {
-				return errors.Wrapf(err, "error creating directory %q for volume %q in container %q", volumePath, k, c.ID())
-			}
-
-			if err = os.Chown(volumePath, uid, gid); err != nil {
-				return errors.Wrapf(err, "error chowning directory %q for volume %q in container %q", volumePath, k, c.ID())
-			}
-
-			if err = label.Relabel(volumePath, c.config.MountLabel, false); err != nil {
-				return errors.Wrapf(err, "error relabeling directory %q for volume %q in container %q", volumePath, k, c.ID())
-			}
-			if err = chrootarchive.NewArchiver(nil).CopyWithTar(srcPath, volumePath); err != nil && !os.IsNotExist(err) {
-				return errors.Wrapf(err, "error populating directory %q for volume %q in container %q using contents of %q", volumePath, k, c.ID(), srcPath)
-			}
-
-			// Set the volume path with the same owner and permission of source path
-			sstat, _ := os.Stat(srcPath)
-			st, ok := sstat.Sys().(*syscall.Stat_t)
-			if !ok {
-				return fmt.Errorf("could not convert to syscall.Stat_t")
-			}
-			uid := int(st.Uid)
-			gid := int(st.Gid)
-
-			if err := os.Lchown(volumePath, uid, gid); err != nil {
-				return err
-			}
-			if os.Chmod(volumePath, sstat.Mode()); err != nil {
-				return err
-			}
-
-		}
-
-		mount.Source = volumePath
-		g.AddMount(mount)
-	}
-	return nil
-}
-
 // Save OCI spec to disk, replacing any existing specs for the container
 func (c *Container) saveSpec(spec *spec.Spec) error {
 	// If the OCI spec already exists, we need to replace it
@@ -1303,3 +1191,30 @@ func getExcludedCGroups() (excludes []string) {
 	excludes = []string{"rdma"}
 	return
 }
+
+// namedVolumes returns named volumes for the container
+func (c *Container) namedVolumes() ([]string, error) {
+	var volumes []string
+	for _, vol := range c.config.Spec.Mounts {
+		if strings.HasPrefix(vol.Source, c.runtime.config.VolumePath) {
+			volume := strings.TrimPrefix(vol.Source, c.runtime.config.VolumePath+"/")
+			split := strings.Split(volume, "/")
+			volume = split[0]
+			if _, err := c.runtime.state.Volume(volume); err == nil {
+				volumes = append(volumes, volume)
+			}
+		}
+	}
+	return volumes, nil
+}
+
+// this should be from chrootarchive.
+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)
+	return a.CopyWithTar(source, dest)
+}
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index bcdfdaee3..65cb47c8c 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -235,13 +235,6 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
 		}
 	}
 
-	// Bind builtin image volumes
-	if c.config.Rootfs == "" && c.config.ImageVolumes {
-		if err := c.addLocalVolumes(ctx, &g, execUser); err != nil {
-			return nil, errors.Wrapf(err, "error mounting image volumes")
-		}
-	}
-
 	if c.config.User != "" {
 		// User and Group must go together
 		g.SetProcessUID(uint32(execUser.Uid))
diff --git a/libpod/options.go b/libpod/options.go
index d965c058e..06737776b 100644
--- a/libpod/options.go
+++ b/libpod/options.go
@@ -11,6 +11,7 @@ import (
 	"github.com/containers/storage"
 	"github.com/containers/storage/pkg/idtools"
 	"github.com/cri-o/ocicni/pkg/ocicni"
+	spec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 )
 
@@ -1058,7 +1059,7 @@ func WithUserVolumes(volumes []string) CtrCreateOption {
 // from a container passed in to the --volumes-from flag.
 // This stores the built-in volume information in the Config so we can
 // add them when creating the container.
-func WithLocalVolumes(volumes []string) CtrCreateOption {
+func WithLocalVolumes(volumes []spec.Mount) CtrCreateOption {
 	return func(ctr *Container) error {
 		if ctr.valid {
 			return ErrCtrFinalized
diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go
index 4f8192198..185090cf7 100644
--- a/libpod/runtime_ctr.go
+++ b/libpod/runtime_ctr.go
@@ -177,9 +177,12 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options ..
 			if err != nil {
 				newVol, err := r.newVolume(ctx, WithVolumeName(vol.Source))
 				if err != nil {
-					logrus.Errorf("error creating named volume %q: %v", vol.Source, err)
+					return nil, errors.Wrapf(err, "error creating named volume %q", vol.Source)
 				}
 				ctr.config.Spec.Mounts[i].Source = newVol.MountPoint()
+				if err := ctr.copyWithTarFromImage(ctr.config.Spec.Mounts[i].Destination, ctr.config.Spec.Mounts[i].Source); err != nil && !os.IsNotExist(err) {
+					return nil, errors.Wrapf(err, "Failed to copy content into new volume mount %q", vol.Source)
+				}
 				continue
 			}
 			ctr.config.Spec.Mounts[i].Source = volInfo.MountPoint()
@@ -225,17 +228,19 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options ..
 
 // RemoveContainer removes the given container
 // If force is specified, the container will be stopped first
+// If removeVolume is specified, named volumes used by the container will
+// be removed also if and only if the container is the sole user
 // Otherwise, RemoveContainer will return an error if the container is running
-func (r *Runtime) RemoveContainer(ctx context.Context, c *Container, force bool) error {
+func (r *Runtime) RemoveContainer(ctx context.Context, c *Container, force bool, removeVolume bool) error {
 	r.lock.Lock()
 	defer r.lock.Unlock()
 
-	return r.removeContainer(ctx, c, force)
+	return r.removeContainer(ctx, c, force, removeVolume)
 }
 
 // Internal function to remove a container
 // Locks the container, but does not lock the runtime
-func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool) error {
+func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool, removeVolume bool) error {
 	if !c.valid {
 		if ok, _ := r.state.HasContainer(c.ID()); !ok {
 			// Container probably already removed
@@ -248,6 +253,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool)
 	// To avoid races around removing a container and the pod it is in
 	var pod *Pod
 	var err error
+	runtime := c.runtime
 	if c.config.Pod != "" {
 		pod, err = r.state.Pod(c.config.Pod)
 		if err != nil {
@@ -333,6 +339,13 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool)
 		return errors.Wrapf(ErrCtrExists, "container %s has dependent containers which must be removed before it: %s", c.ID(), depsStr)
 	}
 
+	var volumes []string
+	if removeVolume {
+		volumes, err = c.namedVolumes()
+		if err != nil {
+			logrus.Errorf("unable to retrieve builtin volumes for container %v: %v", c.ID(), err)
+		}
+	}
 	var cleanupErr error
 	// Remove the container from the state
 	if c.config.Pod != "" {
@@ -397,6 +410,14 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool)
 		}
 	}
 
+	for _, v := range volumes {
+		if volume, err := runtime.state.Volume(v); err == nil {
+			if err := runtime.removeVolume(ctx, volume, false, true); err != nil && err != ErrNoSuchVolume && err != ErrVolumeBeingUsed {
+				logrus.Errorf("cleanup volume (%s): %v", v, err)
+			}
+		}
+	}
+
 	return cleanupErr
 }
 
diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go
index c20aa77a3..1e9689362 100644
--- a/libpod/runtime_img.go
+++ b/libpod/runtime_img.go
@@ -43,7 +43,7 @@ func (r *Runtime) RemoveImage(ctx context.Context, img *image.Image, force bool)
 	if len(imageCtrs) > 0 && len(img.Names()) <= 1 {
 		if force {
 			for _, ctr := range imageCtrs {
-				if err := r.removeContainer(ctx, ctr, true); err != nil {
+				if err := r.removeContainer(ctx, ctr, true, false); err != nil {
 					return "", errors.Wrapf(err, "error removing image %s: container %s using image could not be removed", img.ID(), ctr.ID())
 				}
 			}
diff --git a/libpod/volume_internal.go b/libpod/volume_internal.go
index 800e6d106..0de8a2350 100644
--- a/libpod/volume_internal.go
+++ b/libpod/volume_internal.go
@@ -5,10 +5,6 @@ import (
 	"path/filepath"
 )
 
-// VolumePath is the path under which all volumes that are created using the
-// local driver will be created
-// const VolumePath = "/var/lib/containers/storage/volumes"
-
 // Creates a new volume
 func newVolume(runtime *Runtime) (*Volume, error) {
 	volume := new(Volume)