Re-add locks to volumes.

This will require a 'podman system renumber' after being applied
to get lock numbers for existing volumes.

Add the DB backend code for rewriting volume configs and use it
for updating lock numbers as part of 'system renumber'.

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

libpod/boltdb_state.go

@@ -870,7 +870,7 @@ func (s *BoltState) RewritePodConfig(pod *Pod, newCfg *PodConfig) error {
 
 	newCfgJSON, err := json.Marshal(newCfg)
 	if err != nil {
-		return errors.Wrapf(err, "error marshalling new configuration JSON for container %s", pod.ID())
+		return errors.Wrapf(err, "error marshalling new configuration JSON for pod %s", pod.ID())
 	}
 
 	db, err := s.getDBCon()
@@ -900,6 +900,50 @@ func (s *BoltState) RewritePodConfig(pod *Pod, newCfg *PodConfig) error {
 	return err
 }
 
+// RewriteVolumeConfig rewrites a volume's configuration.
+// WARNING: This function is DANGEROUS. Do not use without reading the full
+// comment on this function in state.go.
+func (s *BoltState) RewriteVolumeConfig(volume *Volume, newCfg *VolumeConfig) error {
+	if !s.valid {
+		return define.ErrDBClosed
+	}
+
+	if !volume.valid {
+		return define.ErrVolumeRemoved
+	}
+
+	newCfgJSON, err := json.Marshal(newCfg)
+	if err != nil {
+		return errors.Wrapf(err, "error marshalling new configuration JSON for volume %q", volume.Name())
+	}
+
+	db, err := s.getDBCon()
+	if err != nil {
+		return err
+	}
+	defer s.deferredCloseDBCon(db)
+
+	err = db.Update(func(tx *bolt.Tx) error {
+		volBkt, err := getVolBucket(tx)
+		if err != nil {
+			return err
+		}
+
+		volDB := volBkt.Bucket([]byte(volume.Name()))
+		if volDB == nil {
+			volume.valid = false
+			return errors.Wrapf(define.ErrNoSuchVolume, "no volume with name %q found in DB", volume.Name())
+		}
+
+		if err := volDB.Put(configKey, newCfgJSON); err != nil {
+			return errors.Wrapf(err, "error updating volume %q config JSON", volume.Name())
+		}
+
+		return nil
+	})
+	return err
+}
+
 // Pod retrieves a pod given its full ID
 func (s *BoltState) Pod(id string) (*Pod, error) {
 	if id == "" {

libpod/boltdb_state_internal.go

@@ -449,6 +449,13 @@ func (s *BoltState) getVolumeFromDB(name []byte, volume *Volume, volBkt *bolt.Bu
 		return errors.Wrapf(err, "error unmarshalling volume %s config from DB", string(name))
 	}
 
+	// Get the lock
+	lock, err := s.runtime.lockManager.RetrieveLock(volume.config.LockID)
+	if err != nil {
+		return errors.Wrapf(err, "error retrieving lock for volume %q", string(name))
+	}
+	volume.lock = lock
+
 	volume.runtime = s.runtime
 	volume.valid = true
 

libpod/in_memory_state.go

@@ -425,6 +425,26 @@ func (s *InMemoryState) RewritePodConfig(pod *Pod, newCfg *PodConfig) error {
 	return nil
 }
 
+// RewriteVolumeConfig rewrites a volume's configuration.
+// This function is DANGEROUS, even with in-memory state.
+// Please read the full comment in state.go before using it.
+func (s *InMemoryState) RewriteVolumeConfig(volume *Volume, newCfg *VolumeConfig) error {
+	if !volume.valid {
+		return define.ErrVolumeRemoved
+	}
+
+	// If the volume does not exist, return error
+	stateVol, ok := s.volumes[volume.Name()]
+	if !ok {
+		volume.valid = false
+		return errors.Wrapf(define.ErrNoSuchVolume, "volume with name %q not found in state", volume.Name())
+	}
+
+	stateVol.config = newCfg
+
+	return nil
+}
+
 // Volume retrieves a volume from its full name
 func (s *InMemoryState) Volume(name string) (*Volume, error) {
 	if name == "" {

libpod/runtime_ctr.go

@@ -253,10 +253,13 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (c *Contai
 
 	// Go through named volumes and add them.
 	// If they don't exist they will be created using basic options.
+	// 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
-		_, err := r.state.Volume(vol.Name)
+		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) != config2.ErrNoSuchVolume {
@@ -275,6 +278,8 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (c *Contai
 		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)
 	}
 
 	if ctr.config.LogPath == "" && ctr.config.LogDriver != JournaldLogging {
@@ -291,6 +296,14 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (c *Contai
 		ctr.config.Mounts = append(ctr.config.Mounts, ctr.config.ShmDir)
 	}
 
+	// Lock all named volumes we are adding ourself to, to ensure we can't
+	// use a volume being removed.
+	for _, namedVol := range ctrNamedVolumes {
+		toLock := namedVol
+		toLock.lock.Lock()
+		defer toLock.lock.Unlock()
+	}
+
 	// Add the container to the state
 	// TODO: May be worth looking into recovering from name/ID collisions here
 	if ctr.config.Pod != "" {

libpod/runtime_renumber.go

@@ -53,6 +53,23 @@ func (r *Runtime) renumberLocks() error {
 			return err
 		}
 	}
+	allVols, err := r.state.AllVolumes()
+	if err != nil {
+		return err
+	}
+	for _, vol := range allVols {
+		lock, err := r.lockManager.AllocateLock()
+		if err != nil {
+			return errors.Wrapf(err, "error allocating lock for volume %s", vol.Name())
+		}
+
+		vol.config.LockID = lock.ID()
+
+		// Write the new lock ID
+		if err := r.state.RewriteVolumeConfig(vol, vol.config); err != nil {
+			return err
+		}
+	}
 
 	r.newSystemEvent(events.Renumber)
 

libpod/runtime_volume.go

@@ -36,6 +36,10 @@ func (r *Runtime) RemoveVolume(ctx context.Context, v *Volume, force bool) error
 			return nil
 		}
 	}
+
+	v.lock.Lock()
+	defer v.lock.Unlock()
+
 	return r.removeVolume(ctx, v, force)
 }
 

libpod/runtime_volume_linux.go

@@ -28,7 +28,7 @@ func (r *Runtime) NewVolume(ctx context.Context, options ...VolumeCreateOption)
 }
 
 // newVolume creates a new empty volume
-func (r *Runtime) newVolume(ctx context.Context, options ...VolumeCreateOption) (*Volume, error) {
+func (r *Runtime) newVolume(ctx context.Context, options ...VolumeCreateOption) (_ *Volume, Err error) {
 	volume, err := newVolume(r)
 	if err != nil {
 		return nil, errors.Wrapf(err, "error creating volume")
@@ -68,6 +68,21 @@ func (r *Runtime) newVolume(ctx context.Context, options ...VolumeCreateOption)
 	}
 	volume.config.MountPoint = fullVolPath
 
+	lock, err := r.lockManager.AllocateLock()
+	if err != nil {
+		return nil, errors.Wrapf(err, "error allocating lock for new volume")
+	}
+	volume.lock = lock
+	volume.config.LockID = volume.lock.ID()
+
+	defer func() {
+		if Err != nil {
+			if err := volume.lock.Free(); err != nil {
+				logrus.Errorf("Error freeing volume lock after failed creation: %v", err)
+			}
+		}
+	}()
+
 	volume.valid = true
 
 	// Add the volume to state
@@ -110,6 +125,8 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool) error
 				return errors.Wrapf(err, "error removing container %s that depends on volume %s", dep, v.Name())
 			}
 
+			logrus.Debugf("Removing container %s (depends on volume %q)", ctr.ID(), v.Name())
+
 			// TODO: do we want to set force here when removing
 			// containers?
 			// I'm inclined to say no, in case someone accidentally
@@ -128,12 +145,24 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool) error
 		return errors.Wrapf(err, "error removing volume %s", v.Name())
 	}
 
-	// Delete the mountpoint path of the volume, that is delete the volume from /var/lib/containers/storage/volumes
+	var removalErr error
+
+	// Free the volume's lock
+	if err := v.lock.Free(); err != nil {
+		removalErr = errors.Wrapf(err, "error freeing lock for volume %s", v.Name())
+	}
+
+	// Delete the mountpoint path of the volume, that is delete the volume
+	// from /var/lib/containers/storage/volumes
 	if err := v.teardownStorage(); err != nil {
-		return errors.Wrapf(err, "error cleaning up volume storage for %q", v.Name())
+		if removalErr == nil {
+			removalErr = errors.Wrapf(err, "error cleaning up volume storage for %q", v.Name())
+		} else {
+			logrus.Errorf("error cleaning up volume storage for volume %q: %v", v.Name(), err)
+		}
 	}
 
 	defer v.newVolumeEvent(events.Remove)
 	logrus.Debugf("Removed volume %s", v.Name())
-	return nil
+	return removalErr
 }

libpod/state.go

@@ -115,12 +115,20 @@ type State interface {
 	// answer is this: use this only very sparingly, and only if you really
 	// know what you're doing.
 	RewriteContainerConfig(ctr *Container, newCfg *ContainerConfig) error
-	// PLEASE READ THE ABOVE DESCRIPTION BEFORE USING.
+	// PLEASE READ THE DESCRIPTION FOR RewriteContainerConfig BEFORE USING.
 	// This function is identical to RewriteContainerConfig, save for the
 	// fact that it is used with pods instead.
 	// It is subject to the same conditions as RewriteContainerConfig.
 	// Please do not use this unless you know what you're doing.
 	RewritePodConfig(pod *Pod, newCfg *PodConfig) error
+	// PLEASE READ THE DESCRIPTION FOR RewriteContainerConfig BEFORE USING.
+	// This function is identical to RewriteContainerConfig, save for the
+	// fact that it is used with volumes instead.
+	// It is subject to the same conditions as RewriteContainerConfig.
+	// The exception is that volumes do not have IDs, so only volume name
+	// cannot be altered.
+	// Please do not use this unless you know what you're doing.
+	RewriteVolumeConfig(volume *Volume, newCfg *VolumeConfig) error
 
 	// Accepts full ID of pod.
 	// If the pod given is not in the set namespace, an error will be

libpod/volume.go

@@ -2,6 +2,8 @@ package libpod
 
 import (
 	"time"
+
+	"github.com/containers/libpod/libpod/lock"
 )
 
 // Volume is the type used to create named volumes
@@ -11,20 +13,34 @@ type Volume struct {
 
 	valid   bool
 	runtime *Runtime
+	lock    lock.Locker
 }
 
 // VolumeConfig holds the volume's config information
 type VolumeConfig struct {
-	// Name of the volume
+	// Name of the volume.
 	Name string `json:"name"`
-
+	// ID of the volume's lock.
+	LockID uint32 `json:"lockID"`
+	// Labels for the volume.
 	Labels map[string]string `json:"labels"`
+	// The volume driver. Empty string or local does not activate a volume
+	// driver, all other volumes will.
 	Driver string `json:"driver"`
+	// The location the volume is mounted at.
 	MountPoint string `json:"mountPoint"`
+	// Time the volume was created.
 	CreatedTime time.Time `json:"createdAt,omitempty"`
+	// Options to pass to the volume driver. For the local driver, this is
+	// a list of mount options. For other drivers, they are passed to the
+	// volume driver handling the volume.
 	Options map[string]string `json:"options"`
+	// Whether this volume was created for a specific container and will be
+	// removed with it.
 	IsCtrSpecific bool `json:"ctrSpecific"`
+	// UID the volume will be created as.
 	UID int `json:"uid"`
+	// GID the volume will be created as.
 	GID int `json:"gid"`
 }
 

test/e2e/run_volume_test.go

@@ -154,4 +154,12 @@ var _ = Describe("Podman run with volumes", func() {
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Not(Equal(0)))
 	})
+
+	It("podman run with volume flag and multiple named volumes", func() {
+		session := podmanTest.Podman([]string{"run", "--rm", "-v", "testvol1:/testvol1", "-v", "testvol2:/testvol2", ALPINE, "grep", "/testvol", "/proc/self/mountinfo"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(ContainSubstring("/testvol1"))
+		Expect(session.OutputToString()).To(ContainSubstring("/testvol2"))
+	})
 })