WIP: distribution interface improvements #193

stevvooe · 2015-02-20T03:28:33Z

This commit sketches out a series of proposed interface improvements for
defining interactions between distribution components. Please see doc.go for a
general overview.

Signed-off-by: Stephen J Day stephen.day@docker.com

Connects to #159
Closes #264

stevvooe · 2015-02-20T03:30:28Z

cc @dmcgowan @icecrime @jlhawn @crosbymichael @jfrazelle @dmp42

jlhawn · 2015-02-20T22:07:46Z

registry.go

-	"github.com/docker/distribution/manifest"
-	"golang.org/x/net/context"
-)
+import "golang.org/x/net/context"


I don't think the interface should rely on an external package like this. I see that it's used in the Registry interface below to get a repository, but wouldn't it make more sense for an implementation of Registry to decide if it's going to use x/net/context?

@jlhawn The idea here is that we subscribe to x/net/context or we don't. Really, each method should start with a context argument. Perhaps, we could refactor this interface to be transaction oriented to capture that, making context an implementation detail of the transaction.

@jlhawn Please see #156 for a discussion of this issue. No one responded.

icecrime · 2015-02-26T04:11:51Z

blobs.go

+	// Exists returns true if the blob exists.
+	Exists(dgst digest.Digest) (bool, error)
+
+	// Get the layer identifed by digest dgst.


If I understand correctly, the name "layer" in this comment is badly chosen (should include manifests and tags too).

yes, the interface used to be called "LayerService", so I think the comment just wasn't updated.

stevvooe · 2015-03-03T23:46:27Z

@jlhawn I've removed the repository field from descriptor, per our discussion in https://github.com/docker/distribution/pull/62/files#r25635220. Please update the manifest proposal accordingly.

stevvooe · 2015-03-06T23:43:15Z

tags.go

+}
+
+// TagService provides access to information about tagged objects.
+type TagService interface {


Let's simplify the tag service to be a mapping between tag name to a Descriptor.

dmcgowan · 2015-03-10T00:51:09Z

@stevvooe I think that is a fair way to look at it, since this is the top level interface it will likely get wrapped by the final implementations in such functionality. Let's re-address in a few weeks when we have a firmer picture of the Trust architecture, any change I would image is small or additive.

This commit sketches out a series of proposed interface improvements for defining interactions between distribution components. Please see doc.go for a general overview. Signed-off-by: Stephen J Day <stephen.day@docker.com>

Signed-off-by: Stephen J Day <stephen.day@docker.com>

stevvooe · 2015-03-25T00:30:34Z

@dmcgowan Added the Namespace object.

dmcgowan · 2015-03-25T03:43:13Z

registry.go

+// Scope defines the set of items that match a namespace.
+type Scope interface {
+	// Contains returns true if the name belongs to the namespace.
+	Contains(name string) (bool, error)


What's the case you would expect this to throw an error if its just doing a string or set comparison?

We can probably remove this. It really depends on how we define scope. If it is just a syntax-based set (ie, glob or equiv), then we don't need the error here.

What if the implementation has to ask a registry?

If the Scope object needs to make a request elsewhere, its implementation is more complex than Scope should be. This should really be a simple string function, not a membership check.

dmcgowan · 2015-03-25T03:50:14Z

+1 to Namespace change, when do you foresee this PR getting ready to merge?

stevvooe · 2015-03-25T17:23:53Z

@dmcgowan When 1.6 settles, this is going to be my primary goal. Realistically, we'll have to phase these changes in. For instance, changing Registry to Namespace could be a relatively small PR.

dmcgowan · 2015-03-31T23:56:33Z

registry.go

+	// Index returns an IndexService that provides search across the
+	// namespace, supporting content discovery and collective information
+	// about available images.
+	// Index(ctx context.Context) (IndexService, error)


We should take this into consideration now since we are defining the registry service as an implementation of Namespace. If namespace covers more than repositories (which I think we agree it does), then what does it mean for Registry to be a partial implementation? Should a registry throw an error when an Index is retrieved, or should Namespace be a composed type.

@dmcgowan Right now, the search/index proposal is too new. I think we can make this decision in the future. A compositional type with interfaces might make sense, but let's get a few in place before going too far.

ncdc · 2015-04-09T15:44:57Z

@stevvooe do you anticipate this going in for 2.0?

stevvooe · 2015-04-09T18:10:50Z

@ncdc No. This will happen after 2.0.

I am going to stage the changes in, starting with the blob store and working my way up.

stevvooe · 2015-04-27T22:50:32Z

blobs.go

+	io.ReaderAt
+	io.Closer
+
+	Reader() (ReadSeekCloser, error)


@dmcgowan @tibor What do you think about removing this method completely and just supporting io.ReaderAt on blob. It allows us to abstract blobs as a series of distributed chunks and defers the io.Reader implementation to the caller (ie NewReadSeeker(length int64, blob io.ReaderAt) (io.ReadCloser, error)).

Why not NewReadSeeker(blob io.ReaderAt) (io.ReadSeekCloser, error) then every read becomes a ReadAt and the underlying ReadSeeker will need to keep track of the read state.

I generally like the idea as long as we are not losing any optimizations from Seek + ReadAll

vbatts · 2015-05-06T15:06:34Z

/cc @vbatts

vbatts · 2015-05-06T15:10:51Z

blobs.go

+type BlobWriter interface {
+	io.WriteSeeker
+	io.WriterAt
+	io.ReaderFrom // TODO(stevvooe): This should be optional.


can't you just not put it here, and level up with a

type BlobWriterFrom interface { BlobWriter io.ReaderFrom }

// TODO(stevvooe): This should be optional.

io.ReaderFrom is probably the most important optimization from the client perspective. Without it either making a request per write is necessary or having an open request in which Write sends to the request body.

Understandable. Then let's drop the comment? :-)
On May 6, 2015 9:28 AM, "Derek McGowan" notifications@github.com wrote:

In blobs.go
#193 (comment):

io.Closer

Reader() (ReadSeekCloser, error)

// Handler returns an HTTP handler which serves the blob content, either

// by redirection or directly serving the content itself.

Handler(r *http.Request) (http.Handler, error)
+}

+// BlobWriter provides a handle for working with a blob store. Instances
+// should be obtained from BlobService.Writer and BlobService.Resume. If
+// supported by the store, a writer can be recovered with the id.
+type BlobWriter interface {

io.WriteSeeker

io.WriterAt

io.ReaderFrom // TODO(stevvooe): This should be optional.

// TODO(stevvooe): This should be optional.

io.ReaderFrom is probably the most important optimization from the client
perspective. Without it either making a request per write is necessary or
having an open request in which Write sends to the request body.

—
Reply to this email directly or view it on GitHub
https://github.com/docker/distribution/pull/193/files#r29775602.

@vbatts Agreed.

stevvooe · 2015-11-12T01:47:36Z

I believe this is being picked up by @RichardScothern.

stevvooe added In Progress enhancement labels Feb 20, 2015

stevvooe added this to the Registry/Future milestone Feb 20, 2015

stevvooe force-pushed the distribution-interfaces branch from 97d22c4 to ea39179 Compare February 20, 2015 04:34

jlhawn reviewed Feb 20, 2015
View reviewed changes

stevvooe force-pushed the distribution-interfaces branch from ea39179 to c15c7ed Compare February 24, 2015 02:39

dmcgowan mentioned this pull request Feb 24, 2015

propose signatures approach and API #162

Closed

stevvooe force-pushed the distribution-interfaces branch from c15c7ed to 7877252 Compare February 24, 2015 18:54

icecrime reviewed Feb 26, 2015
View reviewed changes

stevvooe force-pushed the distribution-interfaces branch 2 times, most recently from bcfbdfe to 1c48e76 Compare March 3, 2015 23:43

stevvooe reviewed Mar 6, 2015
View reviewed changes

stevvooe force-pushed the distribution-interfaces branch 2 times, most recently from 987b69d to a7703fd Compare March 9, 2015 21:40

stevvooe mentioned this pull request Mar 14, 2015

Add golang/x/net/context.Context to storage driver method calls #264

Closed

stevvooe force-pushed the distribution-interfaces branch 2 times, most recently from f34e3e6 to 6f6f459 Compare March 16, 2015 22:47

Distribution interface improvements

9c28431

This commit sketches out a series of proposed interface improvements for defining interactions between distribution components. Please see doc.go for a general overview. Signed-off-by: Stephen J Day <stephen.day@docker.com>

stevvooe force-pushed the distribution-interfaces branch from 6f6f459 to 9c28431 Compare March 16, 2015 23:28

manifest package now implements distribution.Manifest

b95c815

Signed-off-by: Stephen J Day <stephen.day@docker.com>

stevvooe removed this from the Registry/Future milestone Mar 18, 2015

Experiment with calling the registry a namespace

0d13052

Signed-off-by: Stephen J Day <stephen.day@docker.com>

dmcgowan reviewed Mar 25, 2015
View reviewed changes

dmcgowan reviewed Mar 31, 2015
View reviewed changes

This was referenced Apr 13, 2015

Design exported interface for registry clients #159

Closed

Distribution interface refactoring plan #354

Closed

dmcgowan mentioned this pull request Apr 27, 2015

Distribution client/engine integration #447

Closed

11 tasks

stevvooe reviewed Apr 27, 2015
View reviewed changes

amylindburg added the docker/1.7 label Apr 28, 2015

vbatts reviewed May 6, 2015
View reviewed changes

dmp42 assigned stevvooe Jul 23, 2015

stevvooe mentioned this pull request Jul 29, 2015

Concatenate image with registry if provided moby/moby#15093

Closed

stevvooe closed this Nov 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: distribution interface improvements #193

WIP: distribution interface improvements #193

stevvooe commented Feb 20, 2015

stevvooe commented Feb 20, 2015

jlhawn Feb 20, 2015

stevvooe Feb 23, 2015

stevvooe Feb 23, 2015

icecrime Feb 26, 2015

jlhawn Feb 26, 2015

stevvooe commented Mar 3, 2015

stevvooe Mar 6, 2015

dmcgowan commented Mar 10, 2015

stevvooe commented Mar 25, 2015

dmcgowan Mar 25, 2015

stevvooe Mar 31, 2015

dmcgowan Mar 31, 2015

stevvooe Mar 31, 2015

dmcgowan commented Mar 25, 2015

stevvooe commented Mar 25, 2015

dmcgowan Mar 31, 2015

stevvooe Apr 1, 2015

ncdc commented Apr 9, 2015

stevvooe commented Apr 9, 2015

stevvooe Apr 27, 2015

dmcgowan Apr 27, 2015

dmcgowan Apr 27, 2015

vbatts commented May 6, 2015

vbatts May 6, 2015

dmcgowan May 6, 2015

vbatts May 6, 2015

stevvooe May 6, 2015

stevvooe commented Nov 12, 2015

WIP: distribution interface improvements #193

WIP: distribution interface improvements #193

Conversation

stevvooe commented Feb 20, 2015

stevvooe commented Feb 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Mar 3, 2015

Choose a reason for hiding this comment

dmcgowan commented Mar 10, 2015

stevvooe commented Mar 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented Mar 25, 2015

stevvooe commented Mar 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Apr 9, 2015

stevvooe commented Apr 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbatts commented May 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Nov 12, 2015