Add support for anonymous volumes to podman run -v

Previously, when `podman run` encountered a volume mount without separate source and destination (e.g. `-v /run`) we would assume that both were the same - a bind mount of `/run` on the host to `/run` in the container. However, this does not match Docker's behavior - in Docker, this makes an anonymous named volume that will be mounted at `/run`. We already have (more limited) support for these anonymous volumes in the form of image volumes. Extend this support to allow it to be used with user-created volumes coming in from the `-v` flag. This change also affects how named volumes created by the container but given names are treated by `podman run --rm` and `podman rm -v`. Previously, they would be removed with the container in these cases, but this did not match Docker's behaviour. Docker only removed anonymous volumes. With this patch we move to that model as well; `podman run -v testvol:/test` will not have `testvol` survive the container being removed by `podman rm -v`. The sum total of these changes let us turn on volume removal in `--rm` by default. Fixes: #4276 Signed-off-by: Matthew Heon <matthew.heon@pm.me>
2024-10-20 01:03:51 +00:00 · 2019-10-17 11:25:28 -04:00 · 2019-10-17 11:25:28 -04:00 · 0d623914d0
parent d7cbcfadd0
commit 0d623914d0
6 changed files with 146 additions and 28 deletions
--- a/docs/podman-create.1.md
+++ b/docs/podman-create.1.md
@ -800,7 +800,7 @@ Set the UTS mode for the container
    **ns**: specify the user namespace to use.
    Note: the host mode gives the container access to changing the host's hostname and is therefore considered insecure.

-**--volume**, **-v**[=*[HOST-DIR:CONTAINER-DIR[:OPTIONS]]*]
+**--volume**, **-v**[=*[[SOURCE-VOLUME|HOST-DIR:]CONTAINER-DIR[:OPTIONS]]*]

 Create a bind mount. If you specify, ` -v /HOST-DIR:/CONTAINER-DIR`, podman
 bind mounts `/HOST-DIR` in the host to `/CONTAINER-DIR` in the podman
@ -810,11 +810,23 @@ container. The `OPTIONS` are a comma delimited list and can be:
 * [z|Z]
 * [`[r]shared`|`[r]slave`|`[r]private`]

-The `CONTAINER-DIR` must be an absolute path such as `/src/docs`. The `HOST-DIR`
-must be an absolute path as well. Podman bind-mounts the `HOST-DIR` to the
-path you specify. For example, if you supply the `/foo` value, Podman creates a bind-mount.
+The `CONTAINER-DIR` must be an absolute path such as `/src/docs`. The volume
+will be mounted into the container at this directory.

-You can specify multiple  **-v** options to mount one or more mounts to a
+Volumes may specify a source as well, as either a directory on the host or the
+name of a named volume. If no source is given, the volume will be created as an
+anonymous named volume with a randomly generated name, and will be removed when
+the container is removed via the `--rm` flag or `podman rm --volumes`.
+
+If a volume source is specified, it must be a path on the host or the name of a
+named volume. Host paths are allowed to be absolute or relative; relative paths
+are resolved relative to the directory Podman is run in. Any source that does
+not begin with a `.` or `/` it will be treated as the name of a named volume.
+If a volume with that name does not exist, it will be created. Volumes created
+with names are not anonymous and are not removed by `--rm` and
+`podman rm --volumes`.
+
+You can specify multiple  **-v** options to mount one or more volumes into a
 container.

 You can add `:ro` or `:rw` suffix to a volume to mount it  read-only or
--- a/docs/podman-run.1.md
+++ b/docs/podman-run.1.md
@ -839,7 +839,7 @@ Set the UTS mode for the container

 **NOTE**: the host mode gives the container access to changing the host's hostname and is therefore considered insecure.

-**--volume**, **-v**[=*[HOST-DIR-OR-VOUME-NAME:CONTAINER-DIR[:OPTIONS]]*]
+**--volume**, **-v**[=*[[SOURCE-VOLUME|HOST-DIR:]CONTAINER-DIR[:OPTIONS]]*]

 Create a bind mount. If you specify, ` -v /HOST-DIR:/CONTAINER-DIR`, Podman
 bind mounts `/HOST-DIR` in the host to `/CONTAINER-DIR` in the Podman
@ -853,11 +853,23 @@ create one.
 * [`z`|`Z`]
 * [`[r]shared`|`[r]slave`|`[r]private`]

-The `/CONTAINER-DIR` must be an absolute path such as `/src/docs`. The `/HOST-DIR`
-must be an absolute path as well. Podman bind-mounts the `HOST-DIR` to the
-path you specify. For example, if you supply the `/foo` value, Podman creates a bind-mount.
+The `CONTAINER-DIR` must be an absolute path such as `/src/docs`. The volume
+will be mounted into the container at this directory.

-You can specify multiple  **-v** options to mount one or more mounts to a
+Volumes may specify a source as well, as either a directory on the host or the
+name of a named volume. If no source is given, the volume will be created as an
+anonymous named volume with a randomly generated name, and will be removed when
+the container is removed via the `--rm` flag or `podman rm --volumes`.
+
+If a volume source is specified, it must be a path on the host or the name of a
+named volume. Host paths are allowed to be absolute or relative; relative paths
+are resolved relative to the directory Podman is run in. Any source that does
+not begin with a `.` or `/` it will be treated as the name of a named volume.
+If a volume with that name does not exist, it will be created. Volumes created
+with names are not anonymous and are not removed by `--rm` and
+`podman rm --volumes`.
+
+You can specify multiple  **-v** options to mount one or more volumes into a
 container.

 You can add `:ro` or `:rw` suffix to a volume to mount it  read-only or
--- a/libpod/runtime_ctr.go
+++ b/libpod/runtime_ctr.go
@ -295,21 +295,32 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (c *Contai
 	// Maintain an array of them - we need to lock them later.
 	ctrNamedVolumes := make([]*Volume, 0, len(ctr.config.NamedVolumes))
 	for _, vol := range ctr.config.NamedVolumes {
-		// Check if it exists already
-		dbVol, err := r.state.Volume(vol.Name)
-		if err == nil {
-			ctrNamedVolumes = append(ctrNamedVolumes, dbVol)
-			// The volume exists, we're good
-			continue
-		} else if errors.Cause(err) != define.ErrNoSuchVolume {
-			return nil, errors.Wrapf(err, "error retrieving named volume %s for new container", vol.Name)
+		isAnonymous := false
+		if vol.Name == "" {
+			// Anonymous volume. We'll need to create it.
+			// It needs a name first.
+			vol.Name = stringid.GenerateNonCryptoID()
+			isAnonymous = true
+		} else {
+			// Check if it exists already
+			dbVol, err := r.state.Volume(vol.Name)
+			if err == nil {
+				ctrNamedVolumes = append(ctrNamedVolumes, dbVol)
+				// The volume exists, we're good
+				continue
+			} else if errors.Cause(err) != define.ErrNoSuchVolume {
+				return nil, errors.Wrapf(err, "error retrieving named volume %s for new container", vol.Name)
+			}
 		}

 		logrus.Debugf("Creating new volume %s for container", vol.Name)

 		// The volume does not exist, so we need to create it.
-		newVol, err := r.newVolume(ctx, WithVolumeName(vol.Name), withSetCtrSpecific(),
-			WithVolumeUID(ctr.RootUID()), WithVolumeGID(ctr.RootGID()))
+		volOptions := []VolumeCreateOption{WithVolumeName(vol.Name), WithVolumeUID(ctr.RootUID()), WithVolumeGID(ctr.RootGID())}
+		if isAnonymous {
+			volOptions = append(volOptions, withSetCtrSpecific())
+		}
+		newVol, err := r.newVolume(ctx, volOptions...)
 		if err != nil {
 			return nil, errors.Wrapf(err, "error creating named volume %q", vol.Name)
 		}
--- a/pkg/adapter/containers.go
+++ b/pkg/adapter/containers.go
@ -437,7 +437,7 @@ func (r *LocalRuntime) Run(ctx context.Context, c *cliconfig.RunValues, exitCode
 	}

 	if c.IsSet("rm") {
-		if err := r.Runtime.RemoveContainer(ctx, ctr, false, false); err != nil {
+		if err := r.Runtime.RemoveContainer(ctx, ctr, false, true); err != nil {
 			logrus.Errorf("Error removing container %s: %v", ctr.ID(), err)
 		}
 	}
@ -1051,7 +1051,7 @@ func (r *LocalRuntime) CleanupContainers(ctx context.Context, cli *cliconfig.Cle

 // Only used when cleaning up containers
 func removeContainer(ctx context.Context, ctr *libpod.Container, runtime *LocalRuntime) error {
-	if err := runtime.RemoveContainer(ctx, ctr, false, false); err != nil {
+	if err := runtime.RemoveContainer(ctx, ctr, false, true); err != nil {
 		return errors.Wrapf(err, "failed to cleanup and remove container %v", ctr.ID())
 	}
 	return nil
--- a/pkg/spec/storage.go
+++ b/pkg/spec/storage.go
@ -11,7 +11,6 @@ import (
 	"github.com/containers/libpod/libpod"
 	"github.com/containers/libpod/pkg/util"
 	pmount "github.com/containers/storage/pkg/mount"
-	"github.com/containers/storage/pkg/stringid"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@ -648,7 +647,7 @@ func (config *CreateConfig) getVolumeMounts() (map[string]spec.Mount, map[string
 	mounts := make(map[string]spec.Mount)
 	volumes := make(map[string]*libpod.ContainerNamedVolume)

-	volumeFormatErr := errors.Errorf("incorrect volume format, should be host-dir:ctr-dir[:option]")
+	volumeFormatErr := errors.Errorf("incorrect volume format, should be [host-dir:]ctr-dir[:option]")

 	for _, vol := range config.Volumes {
 		var (
@ -665,7 +664,11 @@ func (config *CreateConfig) getVolumeMounts() (map[string]spec.Mount, map[string

 		src = splitVol[0]
 		if len(splitVol) == 1 {
-			dest = src
+			// This is an anonymous named volume. Only thing given
+			// is destination.
+			// Name/source will be blank, and populated by libpod.
+			src = ""
+			dest = splitVol[0]
 		} else if len(splitVol) > 1 {
 			dest = splitVol[1]
 		}
@ -675,8 +678,11 @@ func (config *CreateConfig) getVolumeMounts() (map[string]spec.Mount, map[string
 			}
 		}

-		if err := parse.ValidateVolumeHostDir(src); err != nil {
-			return nil, nil, err
+		// Do not check source dir for anonymous volumes
+		if len(splitVol) > 1 {
+			if err := parse.ValidateVolumeHostDir(src); err != nil {
+				return nil, nil, err
+			}
 		}
 		if err := parse.ValidateVolumeCtrDir(dest); err != nil {
 			return nil, nil, err
@ -736,8 +742,8 @@ func (config *CreateConfig) getImageVolumes() (map[string]spec.Mount, map[string
 			}
 			mounts[vol] = mount
 		} else {
+			// Anonymous volumes have no name.
 			namedVolume := new(libpod.ContainerNamedVolume)
-			namedVolume.Name = stringid.GenerateNonCryptoID()
 			namedVolume.Options = []string{"rprivate", "rw", "nodev"}
 			namedVolume.Dest = cleanDest
 			volumes[vol] = namedVolume
--- a/test/e2e/run_volume_test.go
+++ b/test/e2e/run_volume_test.go
@ -280,4 +280,81 @@ var _ = Describe("Podman run with volumes", func() {
 		session2.WaitWithDefaultTimeout()
 		Expect(session2.ExitCode()).To(Equal(0))
 	})
+
+	It("podman run with anonymous volume", func() {
+		list1 := podmanTest.Podman([]string{"volume", "list", "--quiet"})
+		list1.WaitWithDefaultTimeout()
+		Expect(list1.ExitCode()).To(Equal(0))
+		Expect(list1.OutputToString()).To(Equal(""))
+
+		session := podmanTest.Podman([]string{"create", "-v", "/test", ALPINE, "top"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		list2 := podmanTest.Podman([]string{"volume", "list", "--quiet"})
+		list2.WaitWithDefaultTimeout()
+		Expect(list2.ExitCode()).To(Equal(0))
+		arr := list2.OutputToStringArray()
+		Expect(len(arr)).To(Equal(1))
+		Expect(arr[0]).To(Not(Equal("")))
+	})
+
+	It("podman rm -v removes anonymous volume", func() {
+		list1 := podmanTest.Podman([]string{"volume", "list", "--quiet"})
+		list1.WaitWithDefaultTimeout()
+		Expect(list1.ExitCode()).To(Equal(0))
+		Expect(list1.OutputToString()).To(Equal(""))
+
+		ctrName := "testctr"
+		session := podmanTest.Podman([]string{"create", "--name", ctrName, "-v", "/test", ALPINE, "top"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		list2 := podmanTest.Podman([]string{"volume", "list", "--quiet"})
+		list2.WaitWithDefaultTimeout()
+		Expect(list2.ExitCode()).To(Equal(0))
+		arr := list2.OutputToStringArray()
+		Expect(len(arr)).To(Equal(1))
+		Expect(arr[0]).To(Not(Equal("")))
+
+		remove := podmanTest.Podman([]string{"rm", "-v", ctrName})
+		remove.WaitWithDefaultTimeout()
+		Expect(remove.ExitCode()).To(Equal(0))
+
+		list3 := podmanTest.Podman([]string{"volume", "list", "--quiet"})
+		list3.WaitWithDefaultTimeout()
+		Expect(list3.ExitCode()).To(Equal(0))
+		Expect(list3.OutputToString()).To(Equal(""))
+	})
+
+	It("podman rm -v retains named volume", func() {
+		list1 := podmanTest.Podman([]string{"volume", "list", "--quiet"})
+		list1.WaitWithDefaultTimeout()
+		Expect(list1.ExitCode()).To(Equal(0))
+		Expect(list1.OutputToString()).To(Equal(""))
+
+		ctrName := "testctr"
+		volName := "testvol"
+		session := podmanTest.Podman([]string{"create", "--name", ctrName, "-v", fmt.Sprintf("%s:/test", volName), ALPINE, "top"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		list2 := podmanTest.Podman([]string{"volume", "list", "--quiet"})
+		list2.WaitWithDefaultTimeout()
+		Expect(list2.ExitCode()).To(Equal(0))
+		arr := list2.OutputToStringArray()
+		Expect(len(arr)).To(Equal(1))
+		Expect(arr[0]).To(Equal(volName))
+
+		remove := podmanTest.Podman([]string{"rm", "-v", ctrName})
+		remove.WaitWithDefaultTimeout()
+		Expect(remove.ExitCode()).To(Equal(0))
+
+		list3 := podmanTest.Podman([]string{"volume", "list", "--quiet"})
+		list3.WaitWithDefaultTimeout()
+		Expect(list3.ExitCode()).To(Equal(0))
+		arr2 := list3.OutputToStringArray()
+		Expect(len(arr2)).To(Equal(1))
+		Expect(arr2[0]).To(Equal(volName))
+	})
 })