From e10a9aff7d63495403c757007af20c801839fdb5 Mon Sep 17 00:00:00 2001 From: Phil Estes Date: Mon, 12 Jun 2017 16:33:06 -0400 Subject: [PATCH] Use error interfaces for content/metadata These interfaces allow us to preserve both the checking of error "cause" as well as messages returned from the gRPC API so that the client gets full error reason instead of a default "metadata: not found" in the case of a missing image. Signed-off-by: Phil Estes --- content/content.go | 29 -------- content/errors.go | 120 +++++++++++++++++++++++++++++++++ content/locks.go | 5 +- content/store.go | 8 +-- content/writer.go | 2 +- metadata/containers.go | 12 ++-- metadata/errors.go | 115 ++++++++++++++++++++++++++++--- metadata/images.go | 6 +- metadata/namespaces.go | 6 +- remotes/docker/pusher.go | 4 +- remotes/docker/status.go | 2 +- services/content/helpers.go | 7 +- services/images/helpers.go | 4 +- services/namespaces/helpers.go | 4 +- 14 files changed, 254 insertions(+), 70 deletions(-) create mode 100644 content/errors.go diff --git a/content/content.go b/content/content.go index 9ef6b22e8..ca236d66c 100644 --- a/content/content.go +++ b/content/content.go @@ -7,26 +7,9 @@ import ( "time" "github.com/opencontainers/go-digest" - "github.com/pkg/errors" ) var ( - // ErrNotFound is returned when an item is not found. - // - // Use IsNotFound(err) to detect this condition. - ErrNotFound = errors.New("content: not found") - - // ErrExists is returned when something exists when it may not be expected. - // - // Use IsExists(err) to detect this condition. - ErrExists = errors.New("content: exists") - - // ErrLocked is returned when content is actively being uploaded, this - // indicates that another process is attempting to upload the same content. - // - // Use IsLocked(err) to detect this condition. - ErrLocked = errors.New("content: locked") - bufPool = sync.Pool{ New: func() interface{} { return make([]byte, 1<<20) @@ -106,15 +89,3 @@ type Store interface { Ingester Provider } - -func IsNotFound(err error) bool { - return errors.Cause(err) == ErrNotFound -} - -func IsExists(err error) bool { - return errors.Cause(err) == ErrExists -} - -func IsLocked(err error) bool { - return errors.Cause(err) == ErrLocked -} diff --git a/content/errors.go b/content/errors.go new file mode 100644 index 000000000..6c55fafca --- /dev/null +++ b/content/errors.go @@ -0,0 +1,120 @@ +package content + +type contentExistsErr struct { + desc string +} + +type contentNotFoundErr struct { + desc string +} + +type contentLockedErr struct { + desc string +} + +// ErrExists is returned when something exists when it may not be expected. +func ErrExists(msg string) error { + if msg == "" { + msg = "content: exists" + } + return contentExistsErr{ + desc: msg, + } +} + +// ErrNotFound is returned when an item is not found. +func ErrNotFound(msg string) error { + if msg == "" { + msg = "content: not found" + } + return contentNotFoundErr{ + desc: msg, + } +} + +// ErrLocked is returned when content is actively being uploaded, this +// indicates that another process is attempting to upload the same content. +func ErrLocked(msg string) error { + if msg == "" { + msg = "content: locked" + } + return contentLockedErr{ + desc: msg, + } +} + +func (c contentExistsErr) Error() string { + return c.desc +} +func (c contentNotFoundErr) Error() string { + return c.desc +} +func (c contentLockedErr) Error() string { + return c.desc +} + +func (c contentExistsErr) Exists() bool { + return true +} + +func (c contentNotFoundErr) NotFound() bool { + return true +} + +func (c contentLockedErr) Locked() bool { + return true +} + +// IsNotFound returns true if the error is due to a not found content item +func IsNotFound(err error) bool { + if err, ok := err.(interface { + NotFound() bool + }); ok { + return err.NotFound() + } + + causal, ok := err.(interface { + Cause() error + }) + if !ok { + return false + } + + return IsNotFound(causal.Cause()) +} + +// IsExists returns true if the error is due to an already existing content item +func IsExists(err error) bool { + if err, ok := err.(interface { + Exists() bool + }); ok { + return err.Exists() + } + + causal, ok := err.(interface { + Cause() error + }) + if !ok { + return false + } + + return IsExists(causal.Cause()) +} + +// IsLocked returns true if the error is due to a currently locked content item +func IsLocked(err error) bool { + if err, ok := err.(interface { + Locked() bool + }); ok { + return err.Locked() + } + + causal, ok := err.(interface { + Cause() error + }) + if !ok { + return false + } + + return IsLocked(causal.Cause()) +} diff --git a/content/locks.go b/content/locks.go index 44ed16d3d..4d3aa110a 100644 --- a/content/locks.go +++ b/content/locks.go @@ -1,9 +1,8 @@ package content import ( + "fmt" "sync" - - "github.com/pkg/errors" ) // Handles locking references @@ -20,7 +19,7 @@ func tryLock(ref string) error { defer locksMu.Unlock() if _, ok := locks[ref]; ok { - return errors.Wrapf(ErrLocked, "key %s is locked", ref) + return ErrLocked(fmt.Sprintf("key %s is locked", ref)) } locks[ref] = struct{}{} diff --git a/content/store.go b/content/store.go index fd68e3c5e..e780b3262 100644 --- a/content/store.go +++ b/content/store.go @@ -40,7 +40,7 @@ func (s *store) Info(ctx context.Context, dgst digest.Digest) (Info, error) { fi, err := os.Stat(p) if err != nil { if os.IsNotExist(err) { - err = ErrNotFound + err = ErrNotFound("") } return Info{}, err @@ -62,7 +62,7 @@ func (s *store) Reader(ctx context.Context, dgst digest.Digest) (io.ReadCloser, fp, err := os.Open(s.blobPath(dgst)) if err != nil { if os.IsNotExist(err) { - err = ErrNotFound + err = ErrNotFound("") } return nil, err } @@ -85,7 +85,7 @@ func (cs *store) Delete(ctx context.Context, dgst digest.Digest) error { return err } - return ErrNotFound + return ErrNotFound("") } return nil @@ -323,7 +323,7 @@ func (s *store) Abort(ctx context.Context, ref string) error { root := s.ingestRoot(ref) if err := os.RemoveAll(root); err != nil { if os.IsNotExist(err) { - return ErrNotFound + return ErrNotFound("") } return err diff --git a/content/writer.go b/content/writer.go index c50636960..5ac3b7e40 100644 --- a/content/writer.go +++ b/content/writer.go @@ -99,7 +99,7 @@ func (w *writer) Commit(size int64, expected digest.Digest) error { if err := os.Rename(ingest, target); err != nil { if os.IsExist(err) { // collision with the target file! - return ErrExists + return ErrExists("") } return err } diff --git a/metadata/containers.go b/metadata/containers.go index 40e5927d1..37d1242bf 100644 --- a/metadata/containers.go +++ b/metadata/containers.go @@ -28,7 +28,7 @@ func (s *containerStore) Get(ctx context.Context, id string) (containers.Contain bkt := getContainerBucket(s.tx, namespace, id) if bkt == nil { - return containers.Container{}, errors.Wrap(ErrNotFound, "bucket does not exist") + return containers.Container{}, ErrNotFound("bucket does not exist") } container := containers.Container{ID: id} @@ -85,7 +85,7 @@ func (s *containerStore) Create(ctx context.Context, container containers.Contai cbkt, err := bkt.CreateBucket([]byte(container.ID)) if err != nil { if err == bolt.ErrBucketExists { - err = errors.Wrap(ErrExists, "content for id already exists") + err = ErrExists("content for id already exists") } return containers.Container{}, err } @@ -107,12 +107,12 @@ func (s *containerStore) Update(ctx context.Context, container containers.Contai bkt := getContainersBucket(s.tx, namespace) if bkt == nil { - return containers.Container{}, errors.Wrap(ErrNotFound, "no containers") + return containers.Container{}, ErrNotFound("no containers") } cbkt := bkt.Bucket([]byte(container.ID)) if cbkt == nil { - return containers.Container{}, errors.Wrap(ErrNotFound, "no content for id") + return containers.Container{}, ErrNotFound("no content for id") } container.UpdatedAt = time.Now() @@ -131,11 +131,11 @@ func (s *containerStore) Delete(ctx context.Context, id string) error { bkt := getContainersBucket(s.tx, namespace) if bkt == nil { - return errors.Wrap(ErrNotFound, "no containers") + return ErrNotFound("no containers") } if err := bkt.DeleteBucket([]byte(id)); err == bolt.ErrBucketNotFound { - return errors.Wrap(ErrNotFound, "no content for id") + return ErrNotFound("no content for id") } return err } diff --git a/metadata/errors.go b/metadata/errors.go index 4607ec48e..5103fa660 100644 --- a/metadata/errors.go +++ b/metadata/errors.go @@ -1,22 +1,117 @@ package metadata -import "github.com/pkg/errors" +type metadataExistsErr struct { + desc string +} +type metadataNotFoundErr struct { + desc string +} +type metadataNotEmptyErr struct { + desc string +} -var ( - ErrExists = errors.New("metadata: exists") - ErrNotFound = errors.New("metadata: not found") - ErrNotEmpty = errors.New("metadata: namespace not empty") -) +// ErrExists is returned when an item already exists in metadata +func ErrExists(msg string) error { + if msg == "" { + msg = "metadata: exists" + } + return metadataExistsErr{ + desc: msg, + } +} -// IsNotFound returns true if the error is due to a missing image. +// ErrNotFound is returned when an item cannot be found in metadata +func ErrNotFound(msg string) error { + if msg == "" { + msg = "metadata: not found" + } + return metadataNotFoundErr{ + desc: msg, + } +} + +// ErrNotEmpty is returned when a metadata item can't be deleted because it is not empty +func ErrNotEmpty(msg string) error { + if msg == "" { + msg = "metadata: namespace not empty" + } + return metadataNotEmptyErr{ + desc: msg, + } +} + +func (m metadataExistsErr) Error() string { + return m.desc +} +func (m metadataNotFoundErr) Error() string { + return m.desc +} +func (m metadataNotEmptyErr) Error() string { + return m.desc +} + +func (m metadataExistsErr) Exists() bool { + return true +} + +func (m metadataNotFoundErr) NotFound() bool { + return true +} + +func (m metadataNotEmptyErr) NotEmpty() bool { + return true +} + +// IsNotFound returns true if the error is due to a missing metadata item func IsNotFound(err error) bool { - return errors.Cause(err) == ErrNotFound + if err, ok := err.(interface { + NotFound() bool + }); ok { + return err.NotFound() + } + + causal, ok := err.(interface { + Cause() error + }) + if !ok { + return false + } + + return IsNotFound(causal.Cause()) } +// IsExists returns true if the error is due to an already existing metadata item func IsExists(err error) bool { - return errors.Cause(err) == ErrExists + if err, ok := err.(interface { + Exists() bool + }); ok { + return err.Exists() + } + + causal, ok := err.(interface { + Cause() error + }) + if !ok { + return false + } + + return IsExists(causal.Cause()) } +// IsNotEmpty returns true if the error is due to delete request of a non-empty metadata item func IsNotEmpty(err error) bool { - return errors.Cause(err) == ErrNotEmpty + if err, ok := err.(interface { + NotEmpty() bool + }); ok { + return err.NotEmpty() + } + + causal, ok := err.(interface { + Cause() error + }) + if !ok { + return false + } + + return IsNotEmpty(causal.Cause()) } diff --git a/metadata/images.go b/metadata/images.go index 43c703019..788bb2f4d 100644 --- a/metadata/images.go +++ b/metadata/images.go @@ -30,12 +30,12 @@ func (s *imageStore) Get(ctx context.Context, name string) (images.Image, error) bkt := getImagesBucket(s.tx, namespace) if bkt == nil { - return images.Image{}, ErrNotFound + return images.Image{}, ErrNotFound("") } ibkt := bkt.Bucket([]byte(name)) if ibkt == nil { - return images.Image{}, ErrNotFound + return images.Image{}, ErrNotFound("") } image.Name = name @@ -124,7 +124,7 @@ func (s *imageStore) Delete(ctx context.Context, name string) error { return withImagesBucket(s.tx, namespace, func(bkt *bolt.Bucket) error { err := bkt.DeleteBucket([]byte(name)) if err == bolt.ErrBucketNotFound { - return ErrNotFound + return ErrNotFound("") } return err }) diff --git a/metadata/namespaces.go b/metadata/namespaces.go index ffe898f67..3cbbfd8be 100644 --- a/metadata/namespaces.go +++ b/metadata/namespaces.go @@ -25,7 +25,7 @@ func (s *namespaceStore) Create(ctx context.Context, namespace string, labels ma bkt, err := topbkt.CreateBucket([]byte(namespace)) if err != nil { if err == bolt.ErrBucketExists { - return ErrExists + return ErrExists("") } return err @@ -100,12 +100,12 @@ func (s *namespaceStore) Delete(ctx context.Context, namespace string) error { if empty, err := s.namespaceEmpty(ctx, namespace); err != nil { return err } else if !empty { - return ErrNotEmpty + return ErrNotEmpty("") } if err := bkt.DeleteBucket([]byte(namespace)); err != nil { if err == bolt.ErrBucketNotFound { - return ErrNotFound + return ErrNotFound("") } return err diff --git a/remotes/docker/pusher.go b/remotes/docker/pusher.go index f99701820..e2c08e3cb 100644 --- a/remotes/docker/pusher.go +++ b/remotes/docker/pusher.go @@ -31,7 +31,7 @@ func (p dockerPusher) Push(ctx context.Context, desc ocispec.Descriptor) (conten status, err := p.tracker.GetStatus(ref) if err == nil { if status.Offset == status.Total { - return nil, content.ErrExists + return nil, content.ErrExists("") } // TODO: Handle incomplete status } else if !content.IsNotFound(err) { @@ -72,7 +72,7 @@ func (p dockerPusher) Push(ctx context.Context, desc ocispec.Descriptor) (conten // TODO: Set updated time? }, }) - return nil, content.ErrExists + return nil, content.ErrExists("") } if resp.StatusCode != http.StatusNotFound { // TODO: log error diff --git a/remotes/docker/status.go b/remotes/docker/status.go index 34280f1d1..dc564b12c 100644 --- a/remotes/docker/status.go +++ b/remotes/docker/status.go @@ -34,7 +34,7 @@ func (t *memoryStatusTracker) GetStatus(ref string) (Status, error) { defer t.m.Unlock() status, ok := t.statuses[ref] if !ok { - return Status{}, content.ErrNotFound + return Status{}, content.ErrNotFound("") } return status, nil } diff --git a/services/content/helpers.go b/services/content/helpers.go index d518909c5..a0a12898e 100644 --- a/services/content/helpers.go +++ b/services/content/helpers.go @@ -10,13 +10,12 @@ import ( func rewriteGRPCError(err error) error { switch grpc.Code(errors.Cause(err)) { case codes.AlreadyExists: - return content.ErrExists + return content.ErrExists(grpc.ErrorDesc(err)) case codes.NotFound: - return content.ErrNotFound + return content.ErrNotFound(grpc.ErrorDesc(err)) case codes.Unavailable: - return content.ErrLocked + return content.ErrLocked(grpc.ErrorDesc(err)) } - return err } diff --git a/services/images/helpers.go b/services/images/helpers.go index 19ce9e18a..064f78726 100644 --- a/services/images/helpers.go +++ b/services/images/helpers.go @@ -69,9 +69,9 @@ func rewriteGRPCError(err error) error { switch grpc.Code(errors.Cause(err)) { case codes.AlreadyExists: - return metadata.ErrExists + return metadata.ErrExists(grpc.ErrorDesc(err)) case codes.NotFound: - return metadata.ErrNotFound + return metadata.ErrNotFound(grpc.ErrorDesc(err)) } return err diff --git a/services/namespaces/helpers.go b/services/namespaces/helpers.go index 8cb8f55ad..4092f2d4f 100644 --- a/services/namespaces/helpers.go +++ b/services/namespaces/helpers.go @@ -27,9 +27,9 @@ func rewriteGRPCError(err error) error { switch grpc.Code(errors.Cause(err)) { case codes.AlreadyExists: - return metadata.ErrExists + return metadata.ErrExists(grpc.ErrorDesc(err)) case codes.NotFound: - return metadata.ErrNotFound + return metadata.ErrNotFound(grpc.ErrorDesc(err)) } return err