Switch Libpod over to new explicit named volumes

This swaps the previous handling (parse all volume mounts on the
container and look for ones that might refer to named volumes)
for the new, explicit named volume lists stored per-container.

It also deprecates force-removing volumes that are in use. I
don't know how we want to handle this yet, but leaving containers
that depend on a volume that no longer exists is definitely not
correct.

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

9 files changed, 126 insertions, 166 deletions

diff --git a/libpod/boltdb_state.go b/libpod/boltdb_state.go
index 92a7b1538..d8cfa2bda 100644
--- a/libpod/boltdb_state.go
+++ b/libpod/boltdb_state.go
@@ -1358,56 +1358,6 @@ func (s *BoltState) AddVolume(volume *Volume) error {
 	return err
 }
 
-// RemoveVolCtrDep updates the container dependencies sub bucket of the given volume.
-// It deletes it from the bucket when found.
-// This is important when force removing a volume and we want to get rid of the dependencies.
-func (s *BoltState) RemoveVolCtrDep(volume *Volume, ctrID string) error {
-	if ctrID == "" {
-		return nil
-	}
-
-	if !s.valid {
-		return ErrDBBadConfig
-	}
-
-	if !volume.valid {
-		return ErrVolumeRemoved
-	}
-
-	volName := []byte(volume.Name())
-
-	db, err := s.getDBCon()
-	if err != nil {
-		return err
-	}
-	defer s.closeDBCon(db)
-
-	err = db.Update(func(tx *bolt.Tx) error {
-		volBkt, err := getVolBucket(tx)
-		if err != nil {
-			return err
-		}
-
-		volDB := volBkt.Bucket(volName)
-		if volDB == nil {
-			volume.valid = false
-			return errors.Wrapf(ErrNoSuchVolume, "no volume with name %s found in database", volume.Name())
-		}
-
-		// Make a subbucket for the containers using the volume
-		ctrDepsBkt := volDB.Bucket(volDependenciesBkt)
-		depCtrID := []byte(ctrID)
-		if depExists := ctrDepsBkt.Get(depCtrID); depExists != nil {
-			if err := ctrDepsBkt.Delete(depCtrID); err != nil {
-				return errors.Wrapf(err, "error deleting container dependencies %q for volume %s in ctrDependencies bucket in DB", ctrID, volume.Name())
-			}
-		}
-
-		return nil
-	})
-	return err
-}
-
 // RemoveVolume removes the given volume from the state
 func (s *BoltState) RemoveVolume(volume *Volume) error {
 	if !s.valid {
@@ -1433,6 +1383,11 @@ func (s *BoltState) RemoveVolume(volume *Volume) error {
 			return err
 		}
 
+		ctrBkt, err := getCtrBucket(tx)
+		if err != nil {
+			return err
+		}
+
 		// Check if the volume exists
 		volDB := volBkt.Bucket(volName)
 		if volDB == nil {
@@ -1448,6 +1403,18 @@ func (s *BoltState) RemoveVolume(volume *Volume) error {
 		if volCtrsBkt != nil {
 			var deps []string
 			err = volCtrsBkt.ForEach(func(id, value []byte) error {
+				// Alright, this is ugly.
+				// But we need it to work around the change in
+				// volume dependency handling, to make sure that
+				// older Podman versions don't cause DB
+				// corruption.
+				// Look up all dependencies and see that they
+				// still exist before appending.
+				ctrExists := ctrBkt.Bucket(id)
+				if ctrExists == nil {
+					return nil
+				}
+
 				deps = append(deps, string(id))
 				return nil
 			})
@@ -1629,6 +1596,11 @@ func (s *BoltState) VolumeInUse(volume *Volume) ([]string, error) {
 			return err
 		}
 
+		ctrBucket, err := getCtrBucket(tx)
+		if err != nil {
+			return err
+		}
+
 		volDB := volBucket.Bucket([]byte(volume.Name()))
 		if volDB == nil {
 			volume.valid = false
@@ -1642,6 +1614,13 @@ func (s *BoltState) VolumeInUse(volume *Volume) ([]string, error) {
 
 		// Iterate through and add dependencies
 		err = dependsBkt.ForEach(func(id, value []byte) error {
+			// Look up all dependencies and see that they
+			// still exist before appending.
+			ctrExists := ctrBucket.Bucket(id)
+			if ctrExists == nil {
+				return nil
+			}
+
 			depCtrs = append(depCtrs, string(id))
 
 			return nil
diff --git a/libpod/boltdb_state_internal.go b/libpod/boltdb_state_internal.go
index b6a0759b1..a6900a6d3 100644
--- a/libpod/boltdb_state_internal.go
+++ b/libpod/boltdb_state_internal.go
@@ -564,23 +564,17 @@ func (s *BoltState) addContainer(ctr *Container, pod *Pod) error {
 			}
 		}
 
-		// Add container to volume dependencies bucket if container is using a named volume
-		if ctr.runtime.config.VolumePath == "" {
-			return nil
-		}
-		for _, vol := range ctr.config.Spec.Mounts {
-			if strings.Contains(vol.Source, ctr.runtime.config.VolumePath) {
-				volName := strings.Split(vol.Source[len(ctr.runtime.config.VolumePath)+1:], "/")[0]
-				volDB := volBkt.Bucket([]byte(volName))
-				if volDB == nil {
-					return errors.Wrapf(ErrNoSuchVolume, "no volume with name %s found in database", volName)
-				}
+		// Add container to named volume dependencies buckets
+		for _, vol := range ctr.config.NamedVolumes {
+			volDB := volBkt.Bucket([]byte(vol.Name))
+			if volDB == nil {
+				return errors.Wrapf(ErrNoSuchVolume, "no volume with name %s found in database when adding container %s", vol.Name, ctr.ID())
+			}
 
-				ctrDepsBkt := volDB.Bucket(volDependenciesBkt)
-				if depExists := ctrDepsBkt.Get(ctrID); depExists == nil {
-					if err := ctrDepsBkt.Put(ctrID, ctrID); err != nil {
-						return errors.Wrapf(err, "error storing container dependencies %q for volume %s in ctrDependencies bucket in DB", ctr.ID(), volName)
-					}
+			ctrDepsBkt := volDB.Bucket(volDependenciesBkt)
+			if depExists := ctrDepsBkt.Get(ctrID); depExists == nil {
+				if err := ctrDepsBkt.Put(ctrID, ctrID); err != nil {
+					return errors.Wrapf(err, "error adding container %s to volume %s dependencies", ctr.ID(), vol.Name)
 				}
 			}
 		}
@@ -745,22 +739,19 @@ func (s *BoltState) removeContainer(ctr *Container, pod *Pod, tx *bolt.Tx) error
 		}
 	}
 
-	// Remove container from volume dependencies bucket if container is using a named volume
-	for _, vol := range ctr.config.Spec.Mounts {
-		if strings.Contains(vol.Source, ctr.runtime.config.VolumePath) {
-			volName := strings.Split(vol.Source[len(ctr.runtime.config.VolumePath)+1:], "/")[0]
-
-			volDB := volBkt.Bucket([]byte(volName))
-			if volDB == nil {
-				// Let's assume the volume was already deleted and continue to remove the container
-				continue
-			}
+	// Remove container from named volume dependencies buckets
+	for _, vol := range ctr.config.NamedVolumes {
+		volDB := volBkt.Bucket([]byte(vol.Name))
+		if volDB == nil {
+			// Let's assume the volume was already deleted and
+			// continue to remove the container
+			continue
+		}
 
-			ctrDepsBkt := volDB.Bucket(volDependenciesBkt)
-			if depExists := ctrDepsBkt.Get(ctrID); depExists != nil {
-				if err := ctrDepsBkt.Delete(ctrID); err != nil {
-					return errors.Wrapf(err, "error deleting container dependencies %q for volume %s in ctrDependencies bucket in DB", ctr.ID(), volName)
-				}
+		ctrDepsBkt := volDB.Bucket(volDependenciesBkt)
+		if depExists := ctrDepsBkt.Get(ctrID); depExists == nil {
+			if err := ctrDepsBkt.Delete(ctrID); err != nil {
+				return errors.Wrapf(err, "error deleting container %s dependency on volume %s", ctr.ID(), vol.Name)
 			}
 		}
 	}
diff --git a/libpod/container.go b/libpod/container.go
index b02e536c5..74dc5be5c 100644
--- a/libpod/container.go
+++ b/libpod/container.go
@@ -371,7 +371,7 @@ type ContainerConfig struct {
 }
 
 // ContainerNamedVolume is a named volume that will be mounted into the
-// container.
+// container. Each named volume is a libpod Volume present in the state.
 type ContainerNamedVolume struct {
 	// Name is the name of the volume to mount in.
 	// Must resolve to a valid volume present in this Podman.
@@ -502,6 +502,22 @@ func (c *Container) StaticDir() string {
 	return c.config.StaticDir
 }
 
+// NamedVolumes returns the container's named volumes.
+// The name of each is guaranteed to point to a valid libpod Volume present in
+// the state.
+func (c *Container) NamedVolumes() []*ContainerNamedVolume {
+	volumes := []*ContainerNamedVolume{}
+	for _, vol := range c.config.NamedVolumes {
+		newVol := new(ContainerNamedVolume)
+		newVol.Name = vol.Name
+		newVol.Dest = vol.Dest
+		newVol.Options = vol.Options
+		volumes = append(volumes, newVol)
+	}
+
+	return volumes
+}
+
 // Privileged returns whether the container is privileged
 func (c *Container) Privileged() bool {
 	return c.config.Privileged
diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index daa32007a..22df36c11 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -1403,22 +1403,6 @@ func getExcludedCGroups() (excludes []string) {
 	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()
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index 504d6c135..4d6bf61a3 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -195,6 +195,7 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
 	if err := c.makeBindMounts(); err != nil {
 		return nil, err
 	}
+
 	// Check if the spec file mounts contain the label Relabel flags z or Z.
 	// If they do, relabel the source directory and then remove the option.
 	for i := range g.Config.Mounts {
@@ -218,6 +219,23 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
 
 	g.SetProcessSelinuxLabel(c.ProcessLabel())
 	g.SetLinuxMountLabel(c.MountLabel())
+
+	// Add named volumes
+	for _, namedVol := range c.config.NamedVolumes {
+		volume, err := c.runtime.GetVolume(namedVol.Name)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error retrieving volume %s to add to container %s", namedVol.Name, c.ID())
+		}
+		mountPoint := volume.MountPoint()
+		volMount := spec.Mount{
+			Type:        "bind",
+			Source:      mountPoint,
+			Destination: namedVol.Dest,
+			Options:     namedVol.Options,
+		}
+		g.AddMount(volMount)
+	}
+
 	// Add bind mounts to container
 	for dstPath, srcPath := range c.state.BindMounts {
 		newMount := spec.Mount{
diff --git a/libpod/in_memory_state.go b/libpod/in_memory_state.go
index ab4fc8ba7..2669206df 100644
--- a/libpod/in_memory_state.go
+++ b/libpod/in_memory_state.go
@@ -249,11 +249,8 @@ func (s *InMemoryState) AddContainer(ctr *Container) error {
 	}
 
 	// Add container to volume dependencies
-	for _, vol := range ctr.config.Spec.Mounts {
-		if strings.Contains(vol.Source, ctr.runtime.config.VolumePath) {
-			volName := strings.Split(vol.Source[len(ctr.runtime.config.VolumePath)+1:], "/")[0]
-			s.addCtrToVolDependsMap(ctr.ID(), volName)
-		}
+	for _, vol := range ctr.config.NamedVolumes {
+		s.addCtrToVolDependsMap(ctr.ID(), vol.Name)
 	}
 
 	return nil
@@ -306,12 +303,9 @@ func (s *InMemoryState) RemoveContainer(ctr *Container) error {
 		s.removeCtrFromDependsMap(ctr.ID(), depCtr)
 	}
 
-	// Remove container from volume dependencies
-	for _, vol := range ctr.config.Spec.Mounts {
-		if strings.Contains(vol.Source, ctr.runtime.config.VolumePath) {
-			volName := strings.Split(vol.Source[len(ctr.runtime.config.VolumePath)+1:], "/")[0]
-			s.removeCtrFromVolDependsMap(ctr.ID(), volName)
-		}
+	// Remove this container from volume dependencies
+	for _, vol := range ctr.config.NamedVolumes {
+		s.removeCtrFromVolDependsMap(ctr.ID(), vol.Name)
 	}
 
 	return nil
@@ -492,22 +486,6 @@ func (s *InMemoryState) RemoveVolume(volume *Volume) error {
 	return nil
 }
 
-// RemoveVolCtrDep updates the container dependencies of the volume
-func (s *InMemoryState) RemoveVolCtrDep(volume *Volume, ctrID string) error {
-	if !volume.valid {
-		return errors.Wrapf(ErrVolumeRemoved, "volume with name %s is not valid", volume.Name())
-	}
-
-	if _, ok := s.volumes[volume.Name()]; !ok {
-		return errors.Wrapf(ErrNoSuchVolume, "volume with name %s doesn't exists in state", volume.Name())
-	}
-
-	// Remove container that is using this volume
-	s.removeCtrFromVolDependsMap(ctrID, volume.Name())
-
-	return nil
-}
-
 // VolumeInUse checks if the given volume is being used by at least one container
 func (s *InMemoryState) VolumeInUse(volume *Volume) ([]string, error) {
 	if !volume.valid {
diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go
index da2399685..ec43d5dcb 100644
--- a/libpod/runtime_ctr.go
+++ b/libpod/runtime_ctr.go
@@ -173,26 +173,29 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options ..
 		ctr.config.ConmonPidFile = filepath.Join(ctr.config.StaticDir, "conmon.pid")
 	}
 
-	// Go through the volume mounts and check for named volumes
-	// If the named volme already exists continue, otherwise create
-	// the storage for the named volume.
-	for i, vol := range ctr.config.Spec.Mounts {
-		if vol.Source[0] != '/' && isNamedVolume(vol.Source) {
-			volInfo, err := r.state.Volume(vol.Source)
-			if err != nil {
-				newVol, err := r.newVolume(ctx, WithVolumeName(vol.Source), withSetCtrSpecific(), WithVolumeUID(ctr.RootUID()), WithVolumeGID(ctr.RootGID()))
-				if err != nil {
-					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()
-		}
-	}
+	// // Go through the volume mounts and check for named volumes
+	// // If the named volme already exists continue, otherwise create
+	// // the storage for the named volume.
+	// for i, vol := range ctr.config.Spec.Mounts {
+	// 	if vol.Source[0] != '/' && isNamedVolume(vol.Source) {
+	// 		volInfo, err := r.state.Volume(vol.Source)
+	// 		if err != nil {
+	// 			newVol, err := r.newVolume(ctx, WithVolumeName(vol.Source), withSetCtrSpecific())
+	// 			if err != nil {
+	// 				return nil, errors.Wrapf(err, "error creating named volume %q", vol.Source)
+	// 			}
+	// 			ctr.config.Spec.Mounts[i].Source = newVol.MountPoint()
+				// if err := os.Chown(ctr.config.Spec.Mounts[i].Source, ctr.RootUID(), ctr.RootGID()); err != nil {
+				// 	return nil, errors.Wrapf(err, "cannot chown %q to %d:%d", ctr.config.Spec.Mounts[i].Source, ctr.RootUID(), ctr.RootGID())
+				// }
+	// 			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()
+	// 	}
+	// }
 
 	if ctr.config.LogPath == "" {
 		ctr.config.LogPath = filepath.Join(ctr.config.StaticDir, "ctr.log")
@@ -344,13 +347,6 @@ 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 != "" {
@@ -415,8 +411,12 @@ 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 !removeVolume {
+		return cleanupErr
+	}
+
+	for _, v := range c.config.NamedVolumes {
+		if volume, err := runtime.state.Volume(v.Name); err == nil {
 			if !volume.IsCtrSpecific() {
 				continue
 			}
diff --git a/libpod/runtime_volume_linux.go b/libpod/runtime_volume_linux.go
index db5c29242..39c2f808d 100644
--- a/libpod/runtime_volume_linux.go
+++ b/libpod/runtime_volume_linux.go
@@ -98,14 +98,12 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool) error
 		if !force {
 			return errors.Wrapf(ErrVolumeBeingUsed, "volume %s is being used by the following container(s): %s", v.Name(), depsStr)
 		}
-		// If using force, log the warning that the volume is being used by at least one container
-		logrus.Warnf("volume %s is being used by the following container(s): %s", v.Name(), depsStr)
-		// Remove the container dependencies so we can go ahead and delete the volume
-		for _, dep := range deps {
-			if err := r.state.RemoveVolCtrDep(v, dep); err != nil {
-				return errors.Wrapf(err, "unable to remove container dependency %q from volume %q while trying to delete volume by force", dep, v.Name())
-			}
-		}
+
+		// TODO: force-removing a volume makes *no sense*
+		// I do not believe we should be allowing this at all.
+		// For now, return angry errors.
+		// TODO: need to do something more sane in this case
+		return errors.Wrapf(ErrVolumeBeingUsed, "TODO: FIXME - still refusing to remove because force-removing an in-use volume is not good.")
 	}
 
 	// Set volume as invalid so it can no longer be used
diff --git a/libpod/state.go b/libpod/state.go
index 4296fc3cd..d0ad1a1f8 100644
--- a/libpod/state.go
+++ b/libpod/state.go
@@ -192,10 +192,6 @@ type State interface {
 	// AddVolume adds the specified volume to state. The volume's name
 	// must be unique within the list of existing volumes
 	AddVolume(volume *Volume) error
-	// RemoveVolCtrDep updates the list of container dependencies that the
-	// volume has. It either deletes the dependent container ID from
-	// the sub-bucket
-	RemoveVolCtrDep(volume *Volume, ctrID string) error
 	// RemoveVolume removes the specified volume.
 	// Only volumes that have no container dependencies can be removed
 	RemoveVolume(volume *Volume) error