Skip to content
This repository has been archived by the owner on Sep 12, 2018. It is now read-only.

Adds prototype NG driver apis #630

Closed

Conversation

BrianBland
Copy link

Includes an IPC system for out-of-process drivers so users won't have to recompile the registry to bring their own drivers. Also, drivers can potentially be written in any language with a functioning libchan implementation.

Storage driver API is declared in driver/driver.go

Out-of-process drivers can be launched with driver/ipc/client.go
An example can be found at main/driverclient/driverclient.go
Also adds basic filesystem driver implementation
Also fixed up some formatting and import paths
@dmp42
Copy link
Contributor

dmp42 commented Oct 10, 2014

Relevant: #616 #626

Ping @wking @bacongobbler @noxiouz

@wking
Copy link
Contributor

wking commented Oct 11, 2014

On Fri, Oct 10, 2014 at 04:41:18PM -0700, Brian Bland wrote:

Includes an IPC system for out-of-process drivers so users won't
have to recompile the registry to bring their own drivers. Also,
drivers can potentially be written in any language with a
functioning libchan implementation.

I'm not up on Go / libchan, so I'm not sure how this is going to scale
with many connecting clients. Do you need a separate coroutine for
each client? If so, it seems like it would be more efficient to have
a single thread using select() or similar event-based processing (but
maybe Go's coroutines have absurdly small memory footprints?).

I'm also missing my independent configs for atomic and streaming
storage 1 and storage-generated checksum names to keep us honest on
the content-addressable side (2 and moby/moby#8093), but if this
is just a proof-of-concept, “look it works in Go”, then maybe it's too
early to be talking about that.

On the content-addressable side, it might be easier to have middleware
that translated between the content-addressable registry-side API and
a key/value storage side API (hashing on the fly and renaming from a
temporary filename after it finished writing) to make it easier to
write storage drivers for key/value backends.

return nil, err
}

var response map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose, receiver.Receive supports unpacking into an anonymous struct. It should help to get rid of type conversions.
It looks like:

var response struct {
    Contents []byte
    Error string
}
err = receiver.Receive(&response)
if err != nil {
    return nil, err
}

if p.Error != "" {
   err = errors.New(p.Error)
// err = fmt.Errorf("blabla error %s", p.Error)
}
return p.Contents, err

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I could create a request and response object for each method to do the most compile-time checking.

@noxiouz
Copy link
Contributor

noxiouz commented Oct 12, 2014

Do you need a separate coroutine for each client? If so, it seems like it would be more efficient to have a single thread using select() or similar event-based processing (but maybe Go's coroutines have absurdly small memory footprints?).

It's a common practice in golang. Each goroutine needs ~4KB. Of course, golang has epoll/kqueue under the hood, but we're not allowed to use it directly and have no need, as golang schedules our goroutines.

@BrianBland
Copy link
Author

On the content-addressable side, it might be easier to have middleware
that translated between the content-addressable registry-side API and
a key/value storage side API (hashing on the fly and renaming from a
temporary filename after it finished writing) to make it easier to
write storage drivers for key/value backends.

This could be especially valuable in the case of multiple registries pointing at the same storage backend for HA purposes. We can write the layer to layer/<tarsum>.tmp-<some random key> and then only move it to layer/<tarsum> if the sum matches our expectation.

Update: it looks like "move" operations in many (most?) storage systems aside from local filesystems involve full copies followed by deletion of the original file, so this might be a huge performance hit.

@BrianBland
Copy link
Author

I'm also missing my independent configs for atomic and streaming storage
... but if this is just a proof-of-concept, “look it works in Go”,
then maybe it's too early to be talking about that.

I'm not yet committed to either approach, but if we decide to make these two interfaces, the IPC client Start() method can ask the driver what interface it is implementing and then return the appropriate wrapper.

return fmt.Sprintf("images/%s/manifest.json", imageId)
}

func ImagePrivatePath(imageId string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should disappear entirely. Authorization is not up to the registry to decide, nor the driver.

@dmcgowan
Copy link
Contributor

Instead of creating a new send channel for each request, a single send channel can be created for sending all requests. A new goroutine can still be spawned for each decoded request. Since all channels in libchan are nested, the return channels are still unique even though they were sent through the same channel. This will save a round trip to establish the new channel, but that is likely not as important in the unix socket case.

"io"
)

type Driver interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Driver seems too generic. How about FileStore?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Driver is probably needlessly generic. I'm open to naming suggestions.

Also converts testing to using gocheck Asserts
Required for resumable download functionality
Also adds tests for Move and List
@dmp42 dmp42 mentioned this pull request Oct 15, 2014
var response PutContentResponse
err = receiver.Receive(&response)
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need panic or it's ok to return error

@noxiouz
Copy link
Contributor

noxiouz commented Oct 20, 2014

@BrianBland I sent your PR into next-generation branch. Should I have sent PR here instead of your fork?

@dmp42 dmp42 closed this Oct 21, 2014
@BrianBland
Copy link
Author

Thanks @noxiouz
Because the next-generation branch is a new orphan branch, I'm reconstructing this PR as a completely new commit with your changes incorporated.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants