From d9d84ae2693004b51cf5e7ca9e1f47731bb373cf Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Mon, 9 Feb 2015 14:44:58 -0800 Subject: [PATCH] Integrate context with storage package This changeset integrates context with the storage package. Debug messages have been added to exported methods. Existing log messages will now include contextual details through logger fields to aid in debugging. This integration focuses on logging and may be followed up with a metric-oriented change in the future. Signed-off-by: Stephen J Day --- registry/app.go | 13 +++++-------- storage/blobstore.go | 7 ++++--- storage/layer_test.go | 10 +++++++--- storage/layerstore.go | 6 ++++++ storage/layerupload.go | 3 +++ storage/manifeststore.go | 8 ++++++++ storage/manifeststore_test.go | 7 ++++--- storage/notifications/listener_test.go | 4 +++- storage/registry.go | 9 +++++++-- storage/services.go | 3 ++- 10 files changed, 49 insertions(+), 21 deletions(-) diff --git a/registry/app.go b/registry/app.go index f40d35ef..d2f9e2d9 100644 --- a/registry/app.go +++ b/registry/app.go @@ -227,7 +227,8 @@ func (app *App) dispatcher(dispatch dispatchFunc) http.Handler { // decorate the authorized repository with an event bridge. context.Repository = notifications.Listen( - context.Repository, app.eventBridge(context, r)) + app.registry.Repository(context, getName(context)), + app.eventBridge(context, r)) handler := dispatch(context, r) ssrw := &singleStatusResponseWriter{ResponseWriter: w} @@ -276,9 +277,6 @@ func (app *App) authorized(w http.ResponseWriter, r *http.Request, context *Cont repo := getName(context) if app.accessController == nil { - // No access controller, so we simply provide access. - context.Repository = app.registry.Repository(repo) - return nil // access controller is not enabled. } @@ -357,12 +355,11 @@ func (app *App) authorized(w http.ResponseWriter, r *http.Request, context *Cont return err } + // TODO(stevvooe): This pattern needs to be cleaned up a bit. One context + // should be replaced by another, rather than replacing the context on a + // mutable object. context.Context = ctx - // At this point, the request should have access to the repository under - // the requested operation. Make is available on the context. - context.Repository = app.registry.Repository(repo) - return nil } diff --git a/storage/blobstore.go b/storage/blobstore.go index 04d247ff..ac123f44 100644 --- a/storage/blobstore.go +++ b/storage/blobstore.go @@ -3,10 +3,10 @@ package storage import ( "fmt" - "github.com/Sirupsen/logrus" - + ctxu "github.com/docker/distribution/context" "github.com/docker/distribution/digest" "github.com/docker/distribution/storagedriver" + "golang.org/x/net/context" ) // TODO(stevvooe): Currently, the blobStore implementation used by the @@ -19,6 +19,7 @@ import ( // backend links. type blobStore struct { *registry + ctx context.Context } // exists reports whether or not the path exists. If the driver returns error @@ -110,7 +111,7 @@ func (bs *blobStore) resolve(path string) (string, error) { func (bs *blobStore) put(p []byte) (digest.Digest, error) { dgst, err := digest.FromBytes(p) if err != nil { - logrus.Errorf("error digesting content: %v, %s", err, string(p)) + ctxu.GetLogger(bs.ctx).Errorf("error digesting content: %v, %s", err, string(p)) return "", err } diff --git a/storage/layer_test.go b/storage/layer_test.go index 2a551694..c7d64b79 100644 --- a/storage/layer_test.go +++ b/storage/layer_test.go @@ -13,6 +13,7 @@ import ( "github.com/docker/distribution/storagedriver" "github.com/docker/distribution/storagedriver/inmemory" "github.com/docker/distribution/testutil" + "golang.org/x/net/context" ) // TestSimpleLayerUpload covers the layer upload process, exercising common @@ -30,10 +31,11 @@ func TestSimpleLayerUpload(t *testing.T) { t.Fatalf("error allocating upload store: %v", err) } + ctx := context.Background() imageName := "foo/bar" driver := inmemory.New() registry := NewRegistryWithDriver(driver) - ls := registry.Repository(imageName).Layers() + ls := registry.Repository(ctx, imageName).Layers() h := sha256.New() rd := io.TeeReader(randomDataReader, h) @@ -133,10 +135,11 @@ func TestSimpleLayerUpload(t *testing.T) { // open, read, seek, read works. More specific edge cases should be covered in // other tests. func TestSimpleLayerRead(t *testing.T) { + ctx := context.Background() imageName := "foo/bar" driver := inmemory.New() registry := NewRegistryWithDriver(driver) - ls := registry.Repository(imageName).Layers() + ls := registry.Repository(ctx, imageName).Layers() randomLayerReader, tarSumStr, err := testutil.CreateRandomTarFile() if err != nil { @@ -237,10 +240,11 @@ func TestSimpleLayerRead(t *testing.T) { // TestLayerUploadZeroLength uploads zero-length func TestLayerUploadZeroLength(t *testing.T) { + ctx := context.Background() imageName := "foo/bar" driver := inmemory.New() registry := NewRegistryWithDriver(driver) - ls := registry.Repository(imageName).Layers() + ls := registry.Repository(ctx, imageName).Layers() upload, err := ls.Upload() if err != nil { diff --git a/storage/layerstore.go b/storage/layerstore.go index 7dd7e2ac..b6578792 100644 --- a/storage/layerstore.go +++ b/storage/layerstore.go @@ -4,6 +4,7 @@ import ( "time" "code.google.com/p/go-uuid/uuid" + ctxu "github.com/docker/distribution/context" "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" "github.com/docker/distribution/storagedriver" @@ -14,6 +15,8 @@ type layerStore struct { } func (ls *layerStore) Exists(digest digest.Digest) (bool, error) { + ctxu.GetLogger(ls.repository.ctx).Debug("(*layerStore).Exists") + // Because this implementation just follows blob links, an existence check // is pretty cheap by starting and closing a fetch. _, err := ls.Fetch(digest) @@ -31,6 +34,7 @@ func (ls *layerStore) Exists(digest digest.Digest) (bool, error) { } func (ls *layerStore) Fetch(dgst digest.Digest) (Layer, error) { + ctxu.GetLogger(ls.repository.ctx).Debug("(*layerStore).Fetch") bp, err := ls.path(dgst) if err != nil { return nil, err @@ -52,6 +56,7 @@ func (ls *layerStore) Fetch(dgst digest.Digest) (Layer, error) { // is already in progress or the layer has already been uploaded, this // will return an error. func (ls *layerStore) Upload() (LayerUpload, error) { + ctxu.GetLogger(ls.repository.ctx).Debug("(*layerStore).Upload") // NOTE(stevvooe): Consider the issues with allowing concurrent upload of // the same two layers. Should it be disallowed? For now, we allow both @@ -89,6 +94,7 @@ func (ls *layerStore) Upload() (LayerUpload, error) { // Resume continues an in progress layer upload, returning the current // state of the upload. func (ls *layerStore) Resume(uuid string) (LayerUpload, error) { + ctxu.GetLogger(ls.repository.ctx).Debug("(*layerStore).Resume") startedAtPath, err := ls.repository.registry.pm.path(uploadStartedAtPathSpec{ name: ls.repository.Name(), uuid: uuid, diff --git a/storage/layerupload.go b/storage/layerupload.go index 0c69e30d..54860913 100644 --- a/storage/layerupload.go +++ b/storage/layerupload.go @@ -7,6 +7,7 @@ import ( "time" "github.com/Sirupsen/logrus" + ctxu "github.com/docker/distribution/context" "github.com/docker/distribution/digest" "github.com/docker/distribution/storagedriver" "github.com/docker/docker/pkg/tarsum" @@ -44,6 +45,7 @@ func (luc *layerUploadController) StartedAt() time.Time { // contents of the uploaded layer. The checksum should be provided in the // format :. func (luc *layerUploadController) Finish(digest digest.Digest) (Layer, error) { + ctxu.GetLogger(luc.layerStore.repository.ctx).Debug("(*layerUploadController).Finish") canonical, err := luc.validateLayer(digest) if err != nil { return nil, err @@ -68,6 +70,7 @@ func (luc *layerUploadController) Finish(digest digest.Digest) (Layer, error) { // Cancel the layer upload process. func (luc *layerUploadController) Cancel() error { + ctxu.GetLogger(luc.layerStore.repository.ctx).Debug("(*layerUploadController).Cancel") if err := luc.removeResources(); err != nil { return err } diff --git a/storage/manifeststore.go b/storage/manifeststore.go index f84a0208..1f798dde 100644 --- a/storage/manifeststore.go +++ b/storage/manifeststore.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + ctxu "github.com/docker/distribution/context" "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" "github.com/docker/libtrust" @@ -77,14 +78,17 @@ var _ ManifestService = &manifestStore{} // } func (ms *manifestStore) Tags() ([]string, error) { + ctxu.GetLogger(ms.repository.ctx).Debug("(*manifestStore).Tags") return ms.tagStore.tags() } func (ms *manifestStore) Exists(tag string) (bool, error) { + ctxu.GetLogger(ms.repository.ctx).Debug("(*manifestStore).Exists") return ms.tagStore.exists(tag) } func (ms *manifestStore) Get(tag string) (*manifest.SignedManifest, error) { + ctxu.GetLogger(ms.repository.ctx).Debug("(*manifestStore).Get") dgst, err := ms.tagStore.resolve(tag) if err != nil { return nil, err @@ -94,6 +98,8 @@ func (ms *manifestStore) Get(tag string) (*manifest.SignedManifest, error) { } func (ms *manifestStore) Put(tag string, manifest *manifest.SignedManifest) error { + ctxu.GetLogger(ms.repository.ctx).Debug("(*manifestStore).Put") + // TODO(stevvooe): Add check here to see if the revision is already // present in the repository. If it is, we should merge the signatures, do // a shallow verify (or a full one, doesn't matter) and return an error @@ -118,6 +124,8 @@ func (ms *manifestStore) Put(tag string, manifest *manifest.SignedManifest) erro // semantics in the future, but this will maintain consistency. The underlying // blobs are left alone. func (ms *manifestStore) Delete(tag string) error { + ctxu.GetLogger(ms.repository.ctx).Debug("(*manifestStore).Delete") + revisions, err := ms.tagStore.revisions(tag) if err != nil { return err diff --git a/storage/manifeststore_test.go b/storage/manifeststore_test.go index 15bf27be..8284ce94 100644 --- a/storage/manifeststore_test.go +++ b/storage/manifeststore_test.go @@ -6,20 +6,21 @@ import ( "reflect" "testing" - "github.com/docker/distribution/testutil" - "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" "github.com/docker/distribution/storagedriver/inmemory" + "github.com/docker/distribution/testutil" "github.com/docker/libtrust" + "golang.org/x/net/context" ) func TestManifestStorage(t *testing.T) { + ctx := context.Background() name := "foo/bar" tag := "thetag" driver := inmemory.New() registry := NewRegistryWithDriver(driver) - repo := registry.Repository(name) + repo := registry.Repository(ctx, name) ms := repo.Manifests() exists, err := ms.Exists(tag) diff --git a/storage/notifications/listener_test.go b/storage/notifications/listener_test.go index 17af8b15..30d3f43f 100644 --- a/storage/notifications/listener_test.go +++ b/storage/notifications/listener_test.go @@ -11,6 +11,7 @@ import ( "github.com/docker/distribution/storagedriver/inmemory" "github.com/docker/distribution/testutil" "github.com/docker/libtrust" + "golang.org/x/net/context" ) func TestListener(t *testing.T) { @@ -18,7 +19,8 @@ func TestListener(t *testing.T) { tl := &testListener{ ops: make(map[string]int), } - repository := Listen(registry.Repository("foo/bar"), tl) + ctx := context.Background() + repository := Listen(registry.Repository(ctx, "foo/bar"), tl) // Now take the registry through a number of operations checkExerciseRepository(t, repository) diff --git a/storage/registry.go b/storage/registry.go index b1e20eec..ed865007 100644 --- a/storage/registry.go +++ b/storage/registry.go @@ -1,6 +1,9 @@ package storage -import "github.com/docker/distribution/storagedriver" +import ( + "github.com/docker/distribution/storagedriver" + "golang.org/x/net/context" +) // registry is the top-level implementation of Registry for use in the storage // package. All instances should descend from this object. @@ -32,8 +35,9 @@ func NewRegistryWithDriver(driver storagedriver.StorageDriver) Registry { // Repository returns an instance of the repository tied to the registry. // Instances should not be shared between goroutines but are cheap to // allocate. In general, they should be request scoped. -func (reg *registry) Repository(name string) Repository { +func (reg *registry) Repository(ctx context.Context, name string) Repository { return &repository{ + ctx: ctx, registry: reg, name: name, } @@ -42,6 +46,7 @@ func (reg *registry) Repository(name string) Repository { // repository provides name-scoped access to various services. type repository struct { *registry + ctx context.Context name string } diff --git a/storage/services.go b/storage/services.go index cfb8c787..7e6ac476 100644 --- a/storage/services.go +++ b/storage/services.go @@ -3,6 +3,7 @@ package storage import ( "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" + "golang.org/x/net/context" ) // TODO(stevvooe): These types need to be moved out of the storage package. @@ -12,7 +13,7 @@ type Registry interface { // Repository should return a reference to the named repository. The // registry may or may not have the repository but should always return a // reference. - Repository(name string) Repository + Repository(ctx context.Context, name string) Repository } // Repository is a named collection of manifests and layers.