From cb403bcf07a4aae73a946811b3a0f26d2bd59655 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 19 Oct 2015 20:41:22 -0400 Subject: [PATCH] Refactor volume store's error usage Uses an errors API similar the `net` package. Signed-off-by: Brian Goff Upstream-commit: 43012fe8425650930a21703d9468ab0e777e053a Component: engine --- components/engine/daemon/delete.go | 4 +- components/engine/daemon/mounts.go | 4 +- components/engine/volume/store/errors.go | 58 ++++++++++++++++++++ components/engine/volume/store/store.go | 26 +++------ components/engine/volume/store/store_test.go | 18 +++--- 5 files changed, 80 insertions(+), 30 deletions(-) create mode 100644 components/engine/volume/store/errors.go diff --git a/components/engine/daemon/delete.go b/components/engine/daemon/delete.go index f62f71e972..018513cfad 100644 --- a/components/engine/daemon/delete.go +++ b/components/engine/daemon/delete.go @@ -7,7 +7,7 @@ import ( "github.com/Sirupsen/logrus" derr "github.com/docker/docker/errors" - "github.com/docker/docker/volume/store" + volumestore "github.com/docker/docker/volume/store" ) // ContainerRmConfig is a holder for passing in runtime config. @@ -153,7 +153,7 @@ func (daemon *Daemon) VolumeRm(name string) error { return err } if err := daemon.volumes.Remove(v); err != nil { - if err == store.ErrVolumeInUse { + if volumestore.IsInUse(err) { return derr.ErrorCodeRmVolumeInUse.WithArgs(err) } return derr.ErrorCodeRmVolume.WithArgs(name, err) diff --git a/components/engine/daemon/mounts.go b/components/engine/daemon/mounts.go index 9d195fa8ae..e9df648c8a 100644 --- a/components/engine/daemon/mounts.go +++ b/components/engine/daemon/mounts.go @@ -4,7 +4,7 @@ import ( "strings" derr "github.com/docker/docker/errors" - "github.com/docker/docker/volume/store" + volumestore "github.com/docker/docker/volume/store" ) func (daemon *Daemon) prepareMountPoints(container *Container) error { @@ -34,7 +34,7 @@ func (daemon *Daemon) removeMountPoints(container *Container, 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 != store.ErrVolumeInUse { + if err != nil && !volumestore.IsInUse(err) { rmErrors = append(rmErrors, err.Error()) } } diff --git a/components/engine/volume/store/errors.go b/components/engine/volume/store/errors.go new file mode 100644 index 0000000000..266faf70ed --- /dev/null +++ b/components/engine/volume/store/errors.go @@ -0,0 +1,58 @@ +package store + +import "errors" + +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") + // errInvalidName is a typed error returned when creating a volume with a name that is not valid on the platform + errInvalidName = errors.New("volume name is not valid on this platform") +) + +// OpErr is the error type returned by functions in the store package. It describes +// the operation, volume name, and error. +type OpErr struct { + // Err is the error that occurred during the operation. + Err error + // Op is the operation which caused the error, such as "create", or "list". + Op string + // Name is the name of the resource being requested for this op, typically the volume name or the driver name. + Name string +} + +// Error satifies the built-in error interface type. +func (e *OpErr) Error() string { + if e == nil { + return "" + } + s := e.Op + if e.Name != "" { + s = s + " " + e.Name + } + + s = s + ": " + e.Err.Error() + return s +} + +// IsInUse returns a boolean indicating whether the error indicates that a +// volume is in use +func IsInUse(err error) bool { + return isErr(err, errVolumeInUse) +} + +// IsNotExist returns a boolean indicating whether the error indicates that the volume does not exist +func IsNotExist(err error) bool { + return isErr(err, errNoSuchVolume) +} + +func isErr(err error, expected error) bool { + switch pe := err.(type) { + case nil: + return false + case *OpErr: + err = pe.Err + } + return err == expected +} diff --git a/components/engine/volume/store/store.go b/components/engine/volume/store/store.go index 73ae79fb1d..319cb22059 100644 --- a/components/engine/volume/store/store.go +++ b/components/engine/volume/store/store.go @@ -1,7 +1,6 @@ package store import ( - "errors" "sync" "github.com/Sirupsen/logrus" @@ -10,15 +9,6 @@ import ( "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") - // ErrInvalidName is a typed error returned when creating a volume with a name that is not valid on the platform - ErrInvalidName = errors.New("volume name is not valid on this platform") -) - // New initializes a VolumeStore to keep // reference counting of volumes in the system. func New() *VolumeStore { @@ -81,7 +71,7 @@ func (s *VolumeStore) Create(name, driverName string, opts map[string]string) (v vd, err := volumedrivers.GetDriver(driverName) if err != nil { - return nil, err + return nil, &OpErr{Err: err, Name: driverName, Op: "create"} } // Validate the name in a platform-specific manner @@ -90,12 +80,12 @@ func (s *VolumeStore) Create(name, driverName string, opts map[string]string) (v return nil, err } if !valid { - return nil, ErrInvalidName + return nil, &OpErr{Err: errInvalidName, Name: name, Op: "create"} } v, err := vd.Create(name, opts) if err != nil { - return nil, err + return nil, &OpErr{Op: "create", Name: name, Err: err} } s.set(name, &volumeCounter{v, 0}) @@ -110,7 +100,7 @@ func (s *VolumeStore) Get(name string) (volume.Volume, error) { vc, exists := s.get(name) if !exists { - return nil, ErrNoSuchVolume + return nil, &OpErr{Err: errNoSuchVolume, Name: name, Op: "get"} } return vc.Volume, nil } @@ -124,19 +114,19 @@ func (s *VolumeStore) Remove(v volume.Volume) error { logrus.Debugf("Removing volume reference: driver %s, name %s", v.DriverName(), name) vc, exists := s.get(name) if !exists { - return ErrNoSuchVolume + return &OpErr{Err: errNoSuchVolume, Name: name, Op: "remove"} } if vc.count > 0 { - return ErrVolumeInUse + return &OpErr{Err: errVolumeInUse, Name: name, Op: "remove"} } vd, err := volumedrivers.GetDriver(vc.DriverName()) if err != nil { - return err + return &OpErr{Err: err, Name: vc.DriverName(), Op: "remove"} } if err := vd.Remove(vc.Volume); err != nil { - return err + return &OpErr{Err: err, Name: name, Op: "remove"} } s.remove(name) diff --git a/components/engine/volume/store/store_test.go b/components/engine/volume/store/store_test.go index 0b38ce623c..b17bdb7bfc 100644 --- a/components/engine/volume/store/store_test.go +++ b/components/engine/volume/store/store_test.go @@ -1,6 +1,7 @@ package store import ( + "errors" "testing" "github.com/docker/docker/volume" @@ -30,8 +31,8 @@ func TestGet(t *testing.T) { t.Fatalf("Expected fake1 volume, got %v", v) } - if _, err := s.Get("fake4"); err != ErrNoSuchVolume { - t.Fatalf("Expected ErrNoSuchVolume error, got %v", err) + if _, err := s.Get("fake4"); !IsNotExist(err) { + t.Fatalf("Expected IsNotExist error, got %v", err) } } @@ -54,24 +55,25 @@ func TestCreate(t *testing.T) { } _, 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) + expected := &OpErr{Op: "create", Name: "fakeError", Err: errors.New("create error")} + if err != nil && err.Error() != expected.Error() { + t.Fatalf("Expected create fakeError: 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) + if err := s.Remove(vt.NoopVolume{}); !IsNotExist(err) { + t.Fatalf("Expected IsNotExist 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) + if err := s.Remove(v); !IsInUse(err) { + t.Fatalf("Expected IsInUse error, got %v", err) } s.Decrement(v) if err := s.Remove(v); err != nil {