Move volume ref counting store to a package.

- Add unit tests to make sure the functionality is correct.
- Add FilterByDriver to allow filtering volumes by driver, for future
  `volume ls` filtering and whatnot.

Signed-off-by: David Calavera <david.calavera@gmail.com>

daemon/container_unix.go

@@ -27,6 +27,7 @@ import (
 	"github.com/docker/docker/runconfig"
 	"github.com/docker/docker/utils"
 	"github.com/docker/docker/volume"
+	"github.com/docker/docker/volume/store"
 	"github.com/docker/libnetwork"
 	"github.com/docker/libnetwork/netlabel"
 	"github.com/docker/libnetwork/options"
@@ -1225,7 +1226,7 @@ func (container *Container) removeMountPoints(rm bool) error {
 			// not an error, but an implementation detail.
 			// This prevents docker from logging "ERROR: Volume in use"
 			// where there is another container using the volume.
-			if err != nil && err != ErrVolumeInUse {
+			if err != nil && err != store.ErrVolumeInUse {
 				rmErrors = append(rmErrors, err.Error())
 			}
 		}

daemon/daemon.go

@@ -49,6 +49,7 @@ import (
 	"github.com/docker/docker/trust"
 	volumedrivers "github.com/docker/docker/volume/drivers"
 	"github.com/docker/docker/volume/local"
+	"github.com/docker/docker/volume/store"
 	"github.com/docker/libnetwork"
 	"github.com/opencontainers/runc/libcontainer/netlink"
 )
@@ -114,7 +115,7 @@ type Daemon struct {
 	RegistryService  *registry.Service
 	EventsService    *events.Events
 	netController    libnetwork.NetworkController
-	volumes          *volumeStore
+	volumes          *store.VolumeStore
 	root             string
 	shutdown         bool
 }
@@ -1121,11 +1122,15 @@ func (daemon *Daemon) verifyContainerSettings(hostConfig *runconfig.HostConfig,
 	return verifyPlatformContainerSettings(daemon, hostConfig, config)
 }
 
-func configureVolumes(config *Config) (*volumeStore, error) {
+func configureVolumes(config *Config) (*store.VolumeStore, error) {
 	volumesDriver, err := local.New(config.Root)
 	if err != nil {
 		return nil, err
 	}
+
 	volumedrivers.Register(volumesDriver, volumesDriver.Name())
-	return newVolumeStore(volumesDriver.List()), nil
+	s := store.New()
+	s.AddAll(volumesDriver.List())
+
+	return s, nil
 }

daemon/daemon_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/docker/docker/volume"
 	volumedrivers "github.com/docker/docker/volume/drivers"
 	"github.com/docker/docker/volume/local"
+	"github.com/docker/docker/volume/store"
 )
 
 //
@@ -505,7 +506,7 @@ func initDaemonForVolumesTest(tmp string) (*Daemon, error) {
 	daemon := &Daemon{
 		repository: tmp,
 		root:       tmp,
-		volumes:    newVolumeStore([]volume.Volume{}),
+		volumes:    store.New(),
 	}
 
 	volumesDriver, err := local.New(tmp)

daemon/delete.go

@@ -6,6 +6,7 @@ import (
 	"path"
 
 	"github.com/Sirupsen/logrus"
+	"github.com/docker/docker/volume/store"
 )
 
 // ContainerRmConfig is a holder for passing in runtime config.
@@ -152,7 +153,7 @@ func (daemon *Daemon) VolumeRm(name string) error {
 		return err
 	}
 	if err := daemon.volumes.Remove(v); err != nil {
-		if err == ErrVolumeInUse {
+		if err == store.ErrVolumeInUse {
 			return fmt.Errorf("Conflict: %v", err)
 		}
 		return fmt.Errorf("Error while removing volume %s: %v", name, err)

daemon/volumes.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"sync"
 
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/api/types"
@@ -15,17 +14,12 @@ import (
 	"github.com/docker/docker/pkg/chrootarchive"
 	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/volume"
-	"github.com/docker/docker/volume/drivers"
 )
 
 var (
 	// ErrVolumeReadonly is used to signal an error when trying to copy data into
 	// a volume mount that is not writable.
 	ErrVolumeReadonly = errors.New("mounted volume is marked read-only")
-	// ErrVolumeInUse is a typed error returned when trying to remove a volume that is currently in use by a container
-	ErrVolumeInUse = errors.New("volume is in use")
-	// ErrNoSuchVolume is a typed error returned if the requested volume doesn't exist in the volume store
-	ErrNoSuchVolume = errors.New("no such volume")
 )
 
 // mountPoint is the intersection point between a volume and a container. It
@@ -105,148 +99,6 @@ func copyExistingContents(source, destination string) error {
 	return copyOwnership(source, destination)
 }
 
-func newVolumeStore(vols []volume.Volume) *volumeStore {
-	store := &volumeStore{
-		vols: make(map[string]*volumeCounter),
-	}
-	for _, v := range vols {
-		store.vols[v.Name()] = &volumeCounter{v, 0}
-	}
-	return store
-}
-
-// volumeStore is a struct that stores the list of volumes available and keeps track of their usage counts
-type volumeStore struct {
-	vols map[string]*volumeCounter
-	mu   sync.Mutex
-}
-
-type volumeCounter struct {
-	volume.Volume
-	count int
-}
-
-func getVolumeDriver(name string) (volume.Driver, error) {
-	if name == "" {
-		name = volume.DefaultDriverName
-	}
-	return volumedrivers.Lookup(name)
-}
-
-// Create tries to find an existing volume with the given name or create a new one from the passed in driver
-func (s *volumeStore) Create(name, driverName string, opts map[string]string) (volume.Volume, error) {
-	s.mu.Lock()
-	if vc, exists := s.vols[name]; exists {
-		v := vc.Volume
-		s.mu.Unlock()
-		return v, nil
-	}
-	s.mu.Unlock()
-	logrus.Debugf("Registering new volume reference: driver %s, name %s", driverName, name)
-
-	vd, err := getVolumeDriver(driverName)
-	if err != nil {
-		return nil, err
-	}
-
-	v, err := vd.Create(name, opts)
-	if err != nil {
-		return nil, err
-	}
-
-	s.mu.Lock()
-	s.vols[v.Name()] = &volumeCounter{v, 0}
-	s.mu.Unlock()
-
-	return v, nil
-}
-
-// Get looks if a volume with the given name exists and returns it if so
-func (s *volumeStore) Get(name string) (volume.Volume, error) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	vc, exists := s.vols[name]
-	if !exists {
-		return nil, ErrNoSuchVolume
-	}
-	return vc.Volume, nil
-}
-
-// Remove removes the requested volume. A volume is not removed if the usage count is > 0
-func (s *volumeStore) Remove(v volume.Volume) error {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	name := v.Name()
-	logrus.Debugf("Removing volume reference: driver %s, name %s", v.DriverName(), name)
-	vc, exists := s.vols[name]
-	if !exists {
-		return ErrNoSuchVolume
-	}
-
-	if vc.count != 0 {
-		return ErrVolumeInUse
-	}
-
-	vd, err := getVolumeDriver(vc.DriverName())
-	if err != nil {
-		return err
-	}
-	if err := vd.Remove(vc.Volume); err != nil {
-		return err
-	}
-	delete(s.vols, name)
-	return nil
-}
-
-// Increment increments the usage count of the passed in volume by 1
-func (s *volumeStore) Increment(v volume.Volume) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	logrus.Debugf("Incrementing volume reference: driver %s, name %s", v.DriverName(), v.Name())
-
-	vc, exists := s.vols[v.Name()]
-	if !exists {
-		s.vols[v.Name()] = &volumeCounter{v, 1}
-		return
-	}
-	vc.count++
-}
-
-// Decrement decrements the usage count of the passed in volume by 1
-func (s *volumeStore) Decrement(v volume.Volume) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	logrus.Debugf("Decrementing volume reference: driver %s, name %s", v.DriverName(), v.Name())
-
-	vc, exists := s.vols[v.Name()]
-	if !exists {
-		return
-	}
-	vc.count--
-}
-
-// Count returns the usage count of the passed in volume
-func (s *volumeStore) Count(v volume.Volume) int {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	vc, exists := s.vols[v.Name()]
-	if !exists {
-		return 0
-	}
-	return vc.count
-}
-
-// List returns all the available volumes
-func (s *volumeStore) List() []volume.Volume {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	var ls []volume.Volume
-	for _, vc := range s.vols {
-		ls = append(ls, vc.Volume)
-	}
-	return ls
-}
-
 // volumeToAPIType converts a volume.Volume to the type used by the remote API
 func volumeToAPIType(v volume.Volume) *types.Volume {
 	return &types.Volume{

daemon/volumes_linux_unit_test.go

@@ -2,34 +2,7 @@
 
 package daemon
 
-import (
-	"testing"
-
-	"github.com/docker/docker/volume"
-	"github.com/docker/docker/volume/drivers"
-)
-
-type fakeDriver struct{}
-
-func (fakeDriver) Name() string                                                      { return "fake" }
-func (fakeDriver) Create(name string, opts map[string]string) (volume.Volume, error) { return nil, nil }
-func (fakeDriver) Remove(v volume.Volume) error                                      { return nil }
-
-func TestGetVolumeDriver(t *testing.T) {
-	_, err := getVolumeDriver("missing")
-	if err == nil {
-		t.Fatal("Expected error, was nil")
-	}
-
-	volumedrivers.Register(fakeDriver{}, "fake")
-	d, err := getVolumeDriver("fake")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if d.Name() != "fake" {
-		t.Fatalf("Expected fake driver, got %s\n", d.Name())
-	}
-}
+import "testing"
 
 func TestParseBindMount(t *testing.T) {
 	cases := []struct {

volume/drivers/extpoint.go

@@ -97,3 +97,12 @@ func Lookup(name string) (volume.Driver, error) {
 	drivers.extensions[name] = d
 	return d, nil
 }
+
+// GetDriver returns a volume driver by it's name.
+// If the driver is empty, it looks for the local driver.
+func GetDriver(name string) (volume.Driver, error) {
+	if name == "" {
+		name = volume.DefaultDriverName
+	}
+	return Lookup(name)
+}

volume/drivers/extpoint_test.go

@@ -0,0 +1,23 @@
+package volumedrivers
+
+import (
+	"testing"
+
+	"github.com/docker/docker/volume/testutils"
+)
+
+func TestGetDriver(t *testing.T) {
+	_, err := GetDriver("missing")
+	if err == nil {
+		t.Fatal("Expected error, was nil")
+	}
+
+	Register(volumetestutils.FakeDriver{}, "fake")
+	d, err := GetDriver("fake")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if d.Name() != "fake" {
+		t.Fatalf("Expected fake driver, got %s\n", d.Name())
+	}
+}

volume/store/store.go

@@ -0,0 +1,189 @@
+package store
+
+import (
+	"errors"
+	"sync"
+
+	"github.com/Sirupsen/logrus"
+	"github.com/docker/docker/volume"
+	"github.com/docker/docker/volume/drivers"
+)
+
+var (
+	// ErrVolumeInUse is a typed error returned when trying to remove a volume that is currently in use by a container
+	ErrVolumeInUse = errors.New("volume is in use")
+	// ErrNoSuchVolume is a typed error returned if the requested volume doesn't exist in the volume store
+	ErrNoSuchVolume = errors.New("no such volume")
+)
+
+// New initializes a VolumeStore to keep
+// reference counting of volumes in the system.
+func New() *VolumeStore {
+	return &VolumeStore{
+		vols: make(map[string]*volumeCounter),
+	}
+}
+
+// VolumeStore is a struct that stores the list of volumes available and keeps track of their usage counts
+type VolumeStore struct {
+	vols map[string]*volumeCounter
+	mu   sync.Mutex
+}
+
+// volumeCounter keeps track of references to a volume
+type volumeCounter struct {
+	volume.Volume
+	count uint
+}
+
+// AddAll adds a list of volumes to the store
+func (s *VolumeStore) AddAll(vols []volume.Volume) {
+	for _, v := range vols {
+		s.vols[v.Name()] = &volumeCounter{v, 0}
+	}
+}
+
+// Create tries to find an existing volume with the given name or create a new one from the passed in driver
+func (s *VolumeStore) Create(name, driverName string, opts map[string]string) (volume.Volume, error) {
+	s.mu.Lock()
+	if vc, exists := s.vols[name]; exists {
+		v := vc.Volume
+		s.mu.Unlock()
+		return v, nil
+	}
+	s.mu.Unlock()
+	logrus.Debugf("Registering new volume reference: driver %s, name %s", driverName, name)
+
+	vd, err := volumedrivers.GetDriver(driverName)
+	if err != nil {
+		return nil, err
+	}
+
+	v, err := vd.Create(name, opts)
+	if err != nil {
+		return nil, err
+	}
+
+	s.mu.Lock()
+	s.vols[v.Name()] = &volumeCounter{v, 0}
+	s.mu.Unlock()
+
+	return v, nil
+}
+
+// Get looks if a volume with the given name exists and returns it if so
+func (s *VolumeStore) Get(name string) (volume.Volume, error) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	vc, exists := s.vols[name]
+	if !exists {
+		return nil, ErrNoSuchVolume
+	}
+	return vc.Volume, nil
+}
+
+// Remove removes the requested volume. A volume is not removed if the usage count is > 0
+func (s *VolumeStore) Remove(v volume.Volume) error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	name := v.Name()
+	logrus.Debugf("Removing volume reference: driver %s, name %s", v.DriverName(), name)
+	vc, exists := s.vols[name]
+	if !exists {
+		return ErrNoSuchVolume
+	}
+
+	if vc.count > 0 {
+		return ErrVolumeInUse
+	}
+
+	vd, err := volumedrivers.GetDriver(vc.DriverName())
+	if err != nil {
+		return err
+	}
+	if err := vd.Remove(vc.Volume); err != nil {
+		return err
+	}
+	delete(s.vols, name)
+	return nil
+}
+
+// Increment increments the usage count of the passed in volume by 1
+func (s *VolumeStore) Increment(v volume.Volume) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	logrus.Debugf("Incrementing volume reference: driver %s, name %s", v.DriverName(), v.Name())
+
+	vc, exists := s.vols[v.Name()]
+	if !exists {
+		s.vols[v.Name()] = &volumeCounter{v, 1}
+		return
+	}
+	vc.count++
+}
+
+// Decrement decrements the usage count of the passed in volume by 1
+func (s *VolumeStore) Decrement(v volume.Volume) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	logrus.Debugf("Decrementing volume reference: driver %s, name %s", v.DriverName(), v.Name())
+
+	vc, exists := s.vols[v.Name()]
+	if !exists {
+		return
+	}
+	if vc.count == 0 {
+		return
+	}
+	vc.count--
+}
+
+// Count returns the usage count of the passed in volume
+func (s *VolumeStore) Count(v volume.Volume) uint {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	vc, exists := s.vols[v.Name()]
+	if !exists {
+		return 0
+	}
+	return vc.count
+}
+
+// List returns all the available volumes
+func (s *VolumeStore) List() []volume.Volume {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	var ls []volume.Volume
+	for _, vc := range s.vols {
+		ls = append(ls, vc.Volume)
+	}
+	return ls
+}
+
+// FilterByDriver returns the available volumes filtered by driver name
+func (s *VolumeStore) FilterByDriver(name string) []volume.Volume {
+	return s.filter(byDriver(name))
+}
+
+// filterFunc defines a function to allow filter volumes in the store
+type filterFunc func(vol volume.Volume) bool
+
+// byDriver generates a filterFunc to filter volumes by their driver name
+func byDriver(name string) filterFunc {
+	return func(vol volume.Volume) bool {
+		return vol.DriverName() == name
+	}
+}
+
+// filter returns the available volumes filtered by a filterFunc function
+func (s *VolumeStore) filter(f filterFunc) []volume.Volume {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	var ls []volume.Volume
+	for _, vc := range s.vols {
+		if f(vc.Volume) {
+			ls = append(ls, vc.Volume)
+		}
+	}
+	return ls
+}

volume/store/store_test.go

@@ -0,0 +1,152 @@
+package store
+
+import (
+	"testing"
+
+	"github.com/docker/docker/volume"
+	"github.com/docker/docker/volume/drivers"
+	vt "github.com/docker/docker/volume/testutils"
+)
+
+func TestList(t *testing.T) {
+	volumedrivers.Register(vt.FakeDriver{}, "fake")
+	s := New()
+	s.AddAll([]volume.Volume{vt.NewFakeVolume("fake1"), vt.NewFakeVolume("fake2")})
+	l := s.List()
+	if len(l) != 2 {
+		t.Fatalf("Expected 2 volumes in the store, got %v: %v", len(l), l)
+	}
+}
+
+func TestGet(t *testing.T) {
+	volumedrivers.Register(vt.FakeDriver{}, "fake")
+	s := New()
+	s.AddAll([]volume.Volume{vt.NewFakeVolume("fake1"), vt.NewFakeVolume("fake2")})
+	v, err := s.Get("fake1")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if v.Name() != "fake1" {
+		t.Fatalf("Expected fake1 volume, got %v", v)
+	}
+
+	if _, err := s.Get("fake4"); err != ErrNoSuchVolume {
+		t.Fatalf("Expected ErrNoSuchVolume error, got %v", err)
+	}
+}
+
+func TestCreate(t *testing.T) {
+	volumedrivers.Register(vt.FakeDriver{}, "fake")
+	s := New()
+	v, err := s.Create("fake1", "fake", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if v.Name() != "fake1" {
+		t.Fatalf("Expected fake1 volume, got %v", v)
+	}
+	if l := s.List(); len(l) != 1 {
+		t.Fatalf("Expected 1 volume in the store, got %v: %v", len(l), l)
+	}
+
+	if _, err := s.Create("none", "none", nil); err == nil {
+		t.Fatalf("Expected unknown driver error, got nil")
+	}
+
+	_, err = s.Create("fakeError", "fake", map[string]string{"error": "create error"})
+	if err == nil || err.Error() != "create error" {
+		t.Fatalf("Expected create error, got %v", err)
+	}
+}
+
+func TestRemove(t *testing.T) {
+	volumedrivers.Register(vt.FakeDriver{}, "fake")
+	s := New()
+	if err := s.Remove(vt.NoopVolume{}); err != ErrNoSuchVolume {
+		t.Fatalf("Expected ErrNoSuchVolume error, got %v", err)
+	}
+	v, err := s.Create("fake1", "fake", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	s.Increment(v)
+	if err := s.Remove(v); err != ErrVolumeInUse {
+		t.Fatalf("Expected ErrVolumeInUse error, got %v", err)
+	}
+	s.Decrement(v)
+	if err := s.Remove(v); err != nil {
+		t.Fatal(err)
+	}
+	if l := s.List(); len(l) != 0 {
+		t.Fatalf("Expected 0 volumes in the store, got %v, %v", len(l), l)
+	}
+}
+
+func TestIncrement(t *testing.T) {
+	s := New()
+	v := vt.NewFakeVolume("fake1")
+	s.Increment(v)
+	if l := s.List(); len(l) != 1 {
+		t.Fatalf("Expected 1 volume, got %v, %v", len(l), l)
+	}
+	if c := s.Count(v); c != 1 {
+		t.Fatalf("Expected 1 counter, got %v", c)
+	}
+
+	s.Increment(v)
+	if l := s.List(); len(l) != 1 {
+		t.Fatalf("Expected 1 volume, got %v, %v", len(l), l)
+	}
+	if c := s.Count(v); c != 2 {
+		t.Fatalf("Expected 2 counter, got %v", c)
+	}
+
+	v2 := vt.NewFakeVolume("fake2")
+	s.Increment(v2)
+	if l := s.List(); len(l) != 2 {
+		t.Fatalf("Expected 2 volume, got %v, %v", len(l), l)
+	}
+}
+
+func TestDecrement(t *testing.T) {
+	s := New()
+	v := vt.NoopVolume{}
+	s.Decrement(v)
+	if c := s.Count(v); c != 0 {
+		t.Fatalf("Expected 0 volumes, got %v", c)
+	}
+
+	s.Increment(v)
+	s.Increment(v)
+	s.Decrement(v)
+	if c := s.Count(v); c != 1 {
+		t.Fatalf("Expected 1 volume, got %v", c)
+	}
+
+	s.Decrement(v)
+	if c := s.Count(v); c != 0 {
+		t.Fatalf("Expected 0 volumes, got %v", c)
+	}
+
+	// Test counter cannot be negative.
+	s.Decrement(v)
+	if c := s.Count(v); c != 0 {
+		t.Fatalf("Expected 0 volumes, got %v", c)
+	}
+}
+
+func TestFilterByDriver(t *testing.T) {
+	s := New()
+
+	s.Increment(vt.NewFakeVolume("fake1"))
+	s.Increment(vt.NewFakeVolume("fake2"))
+	s.Increment(vt.NoopVolume{})
+
+	if l := s.FilterByDriver("fake"); len(l) != 2 {
+		t.Fatalf("Expected 2 volumes, got %v, %v", len(l), l)
+	}
+
+	if l := s.FilterByDriver("noop"); len(l) != 1 {
+		t.Fatalf("Expected 1 volume, got %v, %v", len(l), l)
+	}
+}

volume/testutils/testutils.go

@@ -0,0 +1,68 @@
+package volumetestutils
+
+import (
+	"fmt"
+
+	"github.com/docker/docker/volume"
+)
+
+// NoopVolume is a volume that doesn't perform any operation
+type NoopVolume struct{}
+
+// Name is the name of the volume
+func (NoopVolume) Name() string { return "noop" }
+
+// DriverName is the name of the driver
+func (NoopVolume) DriverName() string { return "noop" }
+
+// Path is the filesystem path to the volume
+func (NoopVolume) Path() string { return "noop" }
+
+// Mount mounts the volume in the container
+func (NoopVolume) Mount() (string, error) { return "noop", nil }
+
+// Unmount unmounts the volume from the container
+func (NoopVolume) Unmount() error { return nil }
+
+// FakeVolume is a fake volume with a random name
+type FakeVolume struct {
+	name string
+}
+
+// NewFakeVolume creates a new fake volume for testing
+func NewFakeVolume(name string) volume.Volume {
+	return FakeVolume{name: name}
+}
+
+// Name is the name of the volume
+func (f FakeVolume) Name() string { return f.name }
+
+// DriverName is the name of the driver
+func (FakeVolume) DriverName() string { return "fake" }
+
+// Path is the filesystem path to the volume
+func (FakeVolume) Path() string { return "fake" }
+
+// Mount mounts the volume in the container
+func (FakeVolume) Mount() (string, error) { return "fake", nil }
+
+// Unmount unmounts the volume from the container
+func (FakeVolume) Unmount() error { return nil }
+
+// FakeDriver is a driver that generates fake volumes
+type FakeDriver struct{}
+
+// Name is the name of the driver
+func (FakeDriver) Name() string { return "fake" }
+
+// Create initializes a fake volume.
+// It returns an error if the options include an "error" key with a message
+func (FakeDriver) Create(name string, opts map[string]string) (volume.Volume, error) {
+	if opts != nil && opts["error"] != "" {
+		return nil, fmt.Errorf(opts["error"])
+	}
+	return NewFakeVolume(name), nil
+}
+
+// Remove deletes a volume.
+func (FakeDriver) Remove(v volume.Volume) error { return nil }