Adds storage driver interface, tests, and three basic implementations

[BrianBland]

Re-opening of #630 with the new orphan next-generation branch.

@dmp42 @wking @bacongobbler @noxiouz

[bacongobbler]

I'm a little confused with a lot of the added complexity that this storage driver implementation brings vs. the current registry's storage implementation. There's a lack of documentation here so there's not a lot to go on asides from the discussions in IRC and the NG-related issues, so apologies for my negligence if I missed something:

Why do drivers need to be spawned as separate processes? Given a new release of a filesystem driver in a third party package, wouldn't it be easier to just update that specific package and reload the server config to use the newest version? I just don't feel like an IPC system is the solution for zero-downtime registry reloads, and we should be relying on a more package-based approach for drivers. As an example, in the current registry it's as simple as python setup.py install or pip install --upgrade docker_registry_driver_swift, changing around some configuration if it was necessary and SIGHUP the process. Boom, updated. If your setup was anything more complex than that (such as in a public-facing situation), you could just run the older and the newer version of the registry side-by-side and gracefully migrate traffic to the new registry via your proxy.

If we were to ignore my above comment, how would installing/starting a new third-party driver look like, both inside and outside of a running container? In the eyes of the core maintainers, how would we maintain and ensure that these drivers are up and running?

With this change, configuration is no longer in a central config.yml; You have to specify configuration for every driver when spawning the process. Doesn't this go against the discussion in #646?

[wking]

On Mon, Oct 27, 2014 at 09:29:19PM -0700, Matthew Fisher wrote:

Why do drivers need to be spawned as separate processes?

It makes them language-agnostic. Otherwise, folks would have to
translate existing drivers to Go (where there may not be existing
bindings for the backend in question).

As an example, in the current registry it's as simple as python setup.py install or pip install --upgrade docker_registry_driver_swift, changing around some configuration if
it was necessary and SIGHUP the process. Boom, updated. If your
setup was anything more complex than that (such as in a
public-facing situation), you could just run the older and the newer
version of the registry side-by-side and gracefully migrate traffic
to the new registry via your proxy.

All of that is still true with a network-linked storage process.
You're just SIGHUPing or proxy-handoff-ing the storage container
instead of the registry container. With network IPC for storage
decoupling you from the registry itself, you can just leave the
registry container running while you upgrade your storage drivers.

If we were to ignore my above comment, how would installing/starting
a new third-party driver look like, both inside and outside of a
running container?

Using something like my Fig config [1,2], it would look like:

$ sed -i 's/image: old-streaming-storage:1.2.3/image: new-streaming-storage:1.0.3/' fig.yml
$ fig up

from your host. I don't see a point to being inside a running
container, when it's easier to just spin up new instances from
scratch. And if you're binding the storage process to a persistent
network address (or somewhere behind a proxy on a persistent network
address), there are no changes to make to the registry process at all.

In the eyes of the core maintainers, how would we maintain and
ensure that these drivers are up and running?

I'm not a maintainer at all (just an interested third party), but I'd
have Docker images for the storage drivers, and I'd ensure the
containers were up and running using the same infrastructure I
currently use to make sure the registry container was up and running
(currently Fig for me, but there are many orchestration frameworks).

With this change, configuration is no longer in a central
config.yml; You have to specify configuration for every driver
when spawning the process.

I think we can have a central config. You just need good
namespacing, and the registry process can use the same config that the
storage driver processes use 3.

[bacongobbler]

Would it make more sense for List to return a slice of os.FileInfo structures instead for verbosity? Not sure if it'd be compatible with certain key/value store bindings, but it's a thought. http://golang.org/pkg/os/#FileInfo

We should also be pointing out whether List is recursive or non-recursive on child directories.

[BrianBland]

As written, the List method is recursive.

In the os.FileInfo interface, we'll likely have Name, Size, ModTime, and IsDir defined with reasonable storage drivers, but Mode is unlikely to provide any reasonable information (Sys is fine returning nil).
Should we create and use our own FileInfo interface/struct with only Name, Size, ModTime and IsDir?

[bacongobbler]

@wking all points SGTM (sounds good to me).

As I mentioned, there's a whole lot of code here but no documentation on the storage driver endpoints and with the IPC test suite. Is documentation going to be supplied separately?

[bacongobbler]

As mentioned in #630, Move will typically involve full copies followed by deletion, save for the local filesystem. I can see the benefit for the local filesystem but would it make sense to remove this to reduce boilerplate between storage drivers?

[BrianBland]

I'm not committed to the Move method being part of this interface, although for specific implementations such as the local filesystem driver, this can be a huge win for these operations. I guess the main question is whether or not the registry has any need to move objects in its file storage.

[bacongobbler]

Right. This is a big win for specific implementations and there's not a large hassle to manage a Move method in downstream drivers. I'm sure drivers would appreciate if a move operation was supported. I don't see a big reason to remove this, so I'm happy with keeping it in.

[wking]

On Tue, Oct 28, 2014 at 03:04:32PM -0700, Matthew Fisher wrote:

Right. This is a big win for specific implementations so there's not
a large hassle to manage a Move method in downstream drivers. I'm
sure drivers would appreciate if a move operation was supported. I
don't see a big reason to remove this, so I'm happy with keeping it
in.

I'm not following this. What is the big win, who is appreciating
Move, and why?

If a storage-driver author wants to implement a move endpoint, I have
no problem with that. If the docker-registry wants to use such an
endpoint (optionally?), I'd like to understand why we need it. I
don't see a reason why docker-registry would need a move endpoint, so
I'd rather leave it out of the API, and have storage-driver authors
not need to bother with it.

[dmp42]

We are going to use move in the registry, a lot. Tarsums / checksums need to be computed, and the file needs to be complete. So, the layers will be moved to their final destination once they are complete. Whether the driver implements a smart "move", or is just smart when copying, should be hidden under the specific method call.

I'm +1 on keeping move.

[wking]

On Tue, Oct 28, 2014 at 03:23:37PM -0700, Olivier Gambier wrote:

We are going to use move in the registry, a lot. Tarsums / checksums
need to be computed, and the file needs to be complete. So, the
layers will be moved to their final destination once they are
complete.

I'd rather offload that implementation to the storage driver itself.
The registry just needs to say “I'm going to stream $COUNT bytes [to
$PATH]” (with the presense of $PATH depending on whether or not the
streaming storage is content-addressable). Then the storage driver
can handle that however it likes, to avoid accidentally serving a
partially-complete file when someone asks for $PATH.

[dmp42]

Maybe we don't hear the same thing with "content-adressibility". This is about being able to retrieve a given binary payload from an id that can be computed again from that binary payload. This is what tarsum is for, and I simply fail to see a point in asking driver author to copy the same code over and over again in every implementation.

[wking]

On Tue, Oct 28, 2014 at 03:48:10PM -0700, Olivier Gambier wrote:

Maybe we don't hear the same thing with
"content-adressibility". This is about being able to retrieve a
given binary payload from an id that can be computed again from that
binary payload. This is what tarsum is for, and I simply fail to see
a point in asking driver author to copy the same code over and over
again in every implementation.

Right. To me “an id that can be computed again from that binary
payload” sounds like “we don't need move”. If you want to specify
that id for the initial write, that's fine 1.

[wking]

On Tue, Oct 28, 2014 at 04:00:36AM -0700, Matthew Fisher wrote:

As I mentioned, there's a whole lot of code here but no
documentation on the storage driver endpoints and with the IPC test
suite. Is documentation going to be supplied separately?

I don't mind if it's separate or not, but I hope we'll get
documentation on the interface, and a test harness to put a storage
driver process through its paces. These seem like generic interfaces
(the registry-specific translation is in the registry code, but there
are likely non-registry users who would like streaming,
content-addressable storage over libchan), so I've argued for
splitting them out (and splitting the specs out) into separate
repositories [1,2] with all the docs collected under docs.docker.com
2. I'm happy to start writing up my ideas for this more formally,
but I think we still have some details that need to get hashed out.
Do we need a ‘move’ command (I think not 3)? Do we want to
distinguish between streaming and atomic storage (I think we do 4)?
I think this would be easier to figure out if we had a single point of
discussion for the storage API (or APIs, with streaming and atomic
being discussed separately), but I'm fine chasing this around the
assorted docker-registry issues/PRs and pitching my ideas ;).

[BrianBland]

As I mentioned, there's a whole lot of code here but no
documentation on the storage driver endpoints and with
the IPC test suite. Is documentation going to be supplied
separately?

Sorry the code is lacking on that front right now. Let me start off by explaining the intent behind the IPC storage driver system.

Because go is a compiled language, we can either distribute binaries or require each user to have a fully configured go development environment with the docker-registry repository and all other dependencies to allow for any modifications including the addition of third-party storage drivers. With an IPC system in place, the user only needs to include a new driver executable to add or replace a storage driver.

The actual IPC implementation is inspired by the plugin system in mitchellh/packer, and as currently written, the StorageDriverClient creates and manages a child process with a shared unix domain socket. As the driver is running in a separate process and the IPC system is written on top of libchan, the storage driver is not tied to the same language as the registry itself, but admittedly the language implementations of libchan are currently limited. The language separation is primarily a side effect of the IPC implementation, but writing storage drivers in python or other languages is not out of the question.

As for swapping out drivers without restarting your registry process, this is not in the current design, and would have to be approached carefully. Switching to a different storage system with different state is not well-defined behavior for the registry, so this would only be supported for retaining the same driver name and parameters as originally defined while launching the registry process itself. We would also need to agree on the signal or action that triggers a restart of the storage driver child process.

With this change, configuration is no longer in a central
config.yml; You have to specify configuration for every driver
when spawning the process.

Because the registry process is in charge of launching the storage driver, it can forward any driver configuration parameters to the process on startup. In each of the driver mains, the driver is able to read a json-serialized parameters map, just as defined in the configuration file or environment variables on the parent process (see #646).

[BrianBland]

I don't mind if it's separate or not, but I hope we'll get
documentation on the interface, and a test harness to put a storage
driver process through its paces.

Currently any storage driver can use the same test suites than any of the default drivers are using by calling testsuites.RegisterInProcessSuite and testsuites.RegisterIPCSuite. Both of these suites have the same tests and only differ in that one runs the storage driver locally and the other as a child process over IPC.

[wking]

On Tue, Oct 28, 2014 at 10:10:21AM -0700, Brian Bland wrote:

As for swapping out drivers without restarting your registry
process, this is not in the current design, and would have to be
approached carefully. Switching to a different storage system with
different state is not well-defined behavior for the registry, so
this would only be supported for retaining the same driver name and
parameters as originally defined while launching the registry
process itself. We would also need to agree on the signal or action
that triggers a restart of the storage driver child process.

Obviously, the registry can only serve images that are stored in its
storage backend. If you want to keep serving the same images when you
switch storage drivers, you're going to want to make sure both the old
and new drivers are pointing at the same backing data, or that the new
driver points at a suitably upgraded copy of the old data. It seems
easier (to me) to manage this if the storage drivers are running in
separate containers, distinct from the registry code (which shouldn't
care that there's a new process serving its storage requests).

Since it's harder to pass around links to unix sockets, I'd probably
start off with a networked libchan connection. Then I'd add support
for a unix socket connection if folks wanted the increased efficiency
at the cost of keeping the registry and storage-driver containers on
the same host. You'd also have to teach your orchestration layer and
likely the registry how to handle rolling over the socket connection
to a new storage driver's socket.

I don't think we need to preserve the driver name (why would the
registry care about the name of the driver it was connecting to, so
long as the libchan protocol and connection endpoint stay the same?).
I don't think we need to preserve the driver parameters either (for
the same reason).

I'd leave swapping storage containers to the orchestration layer
(e.g. Fig), so no need to teach the registry how to do that. For 100%
uptime, you would need a stable proxy between the registry and the
storage drivers to keep connections to the old driver alive until they
completed and forward new connections to the new driver. Then the
orchestration layer could reap the old driver once it's outstanding
connections closed. Hmm, maybe libchan connections never close? Can
anyone chime in on proxying and rolling over libchan connections?

[wking]

On Tue, Oct 28, 2014 at 10:27:32AM -0700, Brian Bland wrote:

I don't mind if it's separate or not, but I hope we'll get
documentation on the interface, and a test harness to put a storage
driver process through its paces.

Currently any storage driver can use the same test suites than any
of the default drivers are using by calling
testsuites.RegisterInProcessSuite and
testsuites.RegisterIPCSuite. Both of these suites have the same
tests and only differ in that one runs the storage driver locally
and the other as a child process over IPC.

If I have a running container that exposes an endpoint for the
libchan-over-HTTP storage driver API, I'd like to be able to run:

$ test-streaming-storage http://my-storage.example.com/

and have it run through the test suite without my having to dip into the
Go. Maybe that's already possible, and I'm just not a good enough Go
reader to tell?

[dmp42]

@bacongobbler @wking @BrianBland

Thanks a lot for this.

I feel a quick (short) summary can be useful here:

• drivers authors shouldn't have to bother about IPC / libchan at all in their implementation, and how their driver is used (IPC or hard-compiled) is rather up to the person running the registry
• compiling your driver into the registry and using it as-is is definitely something that should be possible and that we will use for "mainline" drivers (with the downside it would require you to build a modified registry to import your driver package)
• this IPC solution is not (IMHO) meant to solve availability or hot-switching questions (though people may use it for that if they want)
• the remote-network-driver use case might be interesting - for now we should focus on having it run with local drivers and subprocesses
• that IPC design was meant to solve the following problems specifically:
  • ability to use a custom driver against a vanilla build of the registry without having compile a custom registry
  • open the possibility to have different languages for drivers

Now, I'm happy if that design let other things to be done, and opens-up creativity, but the bottom-line is (@bacongobbler): make like simple to drivers authors and clean separation between the drivers and the registry.

[dmp42]

@BrianBland let's go for another review of this as soon as @BrianBland provides some documentation and drivers authors (@noxiouz @bacongobbler) can kick the philosophy in the tires.

[dmp42]

er -> err

[BrianBland]

And this is why we have CI...

[dmp42]

@ahmetalpbalkan registry do need a move operation at the driver level

The main reason for that is ids used to store blobs will be content-adressable - hence requiring to first store some given blob in a temporary place, then move it to the final destination.

If you don't have a move operation, then no big deal, wrap a copy+delete (as long as your copy is efficient enough).

[ahmetb]

@dmp42 that works as long as the caller (registry) does not assume that Move operation is atomic (transactional) and if the code crashes in the middle of that, you may end up with 2 copies (since Delete isn't executed yet).

[dmp42]

@ahmetalpbalkan fair enough

[stevvooe]

How big do we expect directories to get? Do we have any reasonable bound on the number of directory entries?

[BrianBland]

Technically if the prefix is at a high enough level, this could include all repositories and layers, which could be quite large.

We may want to change this to mimic the behavior that @wking suggested in #616

Count(prefix string) (int, error)
List(prefix string, size int, from int) ([]string, error)

[bacongobbler]

What would be the use case for using from and size? How do you see them being used in the registry? If we're concerned about having the prefix at a high enough level being slow, we could make it optionally non-recursive:

Count(prefix string) (int, error)
List(prefix string, recursive bool) ([]string, error)

That should make List("/", false) only display ["images/", "repositories/"] and List("/images", false) significantly less time-consuming than recursively listing every directory under /images.

[wking]

On Mon, Nov 03, 2014 at 01:22:40PM -0800, Matthew Fisher wrote:

What would be the use case for using from and size? How do you
see them being used in the registry? If we're concerned about having
the prefix at a high enough level being slow, we could make it
optionally non-recursive:

Count(prefix string) (int, error)
List(prefix string, recursive bool) ([]string, error)

That should make List("/", false) only display ["images", "repositories"] and List("/images", false) significantly less
time-consuming than recursively listing * every* directory under
/images.

And List("/repositories", false) would list all the namespaces? I'd
rather say:

List("namespace/", 0, 100)

for “give me the first hundred namespaces”. I'd avoid recursion by
using namespaced keys (intead of heirarchical keys):

namespace/alice
namespace/bob
namespace/library
…
repository/alice/busybox
repository/bob/debian
repository/library/debian
…
tag/alice/busybox/0.1
tag/bob/debian/latest
tag/library/debian/latest

so you could iterate through namespaces (with a “namespace/” prefix),
or through repositories in a namespace (with a “repositories/alice/”
prefix), or through all repositories (with a “repositories/” prefix),
…

[BrianBland]

After doing a little bit of research it looks like the from and size arguments would cause some issues with certain storage systems including S3.

The S3 list equivalent GET Bucket supports max-keys and a marker argument, which is the last key of then previous result set, or the key before the first that should be returned from the call. Implementing the interface with List(prefix string, size int, from int) with this model seems like it would require loading all results up until the from argument so that we can determine what the appropriate marker is.

I'm all for namespaced keys, but we should find a good solution for loading all namespaces unless we're fine returning a number of results equal to the number of users who own repositories in the worst case.

[stevvooe]

After thinking about this more carefully, we thought it might be better to keep this IPC call simple. Without introducing problems with missing or inconsistent directory listings, the variation in the drivers makes getting a reentrant interface right problematic. The other issue here is that whether or not these calls are reentrant, you end up allocating the same amount of memory. Thus, large directories will cause latency in registry requests no matter what.

As an alternative, we can mitigate listing sizes by controlling underlying directory layout. For now, let's keep List(prefix string) ([]sting, error) as the interface method. We should also define toList` to not return recursive results. (@dmp42 Was there a reason to support recursive directory listings?)

We can always revisit this in the future if we've made the wrong decision today.

[dmp42]

@stevvooe We don't need/want recursive directory listing (disclaimer: haven't read all comments, and assume recursive means -r)
And simpler is better :)

[wking]

On Mon, Nov 03, 2014 at 07:05:04PM -0800, Stephen Day wrote:

Without introducing problems with missing or inconsistent directory
listings, the variation in the drivers makes getting a reentrant
interface right problematic.

Using either a marker string 1 or an opaque marker type 2 seems to
support all proposed backends. With the marker stored in the
registry, the storage backend doesn't need to keep track of open list
calls, so a reentrant interface is easy. Or am I missing something?

The other issue here is that whether or not these calls are
reentrant, you end up allocating the same amount of memory. Thus,
large directories will cause latency in registry requests no matter
what.

How so? ZRANGEBYSCORE is O(log(N)+M) with N being the number of
elements in the sorted set and M the number of elements being
returned. That scales pretty well. More importantly, it lets you
start streaming results from the registry to the client before you've
finished listing the whole directory.

We can always revisit this in the future if we've made the wrong
decision today.

True, but we're already having trouble with the current layout (#614).

[stevvooe]

@wking Before I address your points, fear not, we intend to address #614, in one way or another. We just need to understand the right way to do it.

Using either a marker string [1] or an opaque marker type [2] seems to
support all proposed backends. With the marker stored in the
registry, the storage backend doesn't need to keep track of open list
calls, so a reentrant interface is easy. Or am I missing something?

Yes, the marker or index will support most backends, but the behavior may be different on updates to that directory. With a marker string, an insert before the marker after the first call will lead to a missed entry. With an index, one may have duplicate entries. The behavior between various backends under these conditions may vary greatly, requiring calling code (ie the registry) to handle that variation. By going with the simpler call, we avoid this variation and leave it up to the driver to present a "consistent" view of the directory state.

How so? ZRANGEBYSCORE is O(log(N)+M) with N being the number of
elements in the sorted set and M the number of elements being
returned. That scales pretty well. More importantly, it lets you
start streaming results from the registry to the client before you've
finished listing the whole directory.

That's not the case I'm worried about. If we want to iterate over the entries of a large directory to support an API operation, the latency of the registry request will still be proportional to the size of the directory, reentrant or not. This should be avoided, if possible.

Let's proceed with this simplified API and table this discussion, for now. We have collected some really good suggestions but we really need to look at how we can avoid this bottleneck. Once the V2 paths are laid out, we can come back and see if we really need the reentrant API call.

[wking]

On Mon, Nov 03, 2014 at 09:07:51PM -0800, Stephen Day wrote:

Using either a marker string [1] or an opaque marker type [2]
seems to support all proposed backends. With the marker stored in
the registry, the storage backend doesn't need to keep track of
open list calls, so a reentrant interface is easy. Or am I
missing something?

Yes, the marker or index will support most backends, but the
behavior may be different on updates to that directory. With a
marker string, an insert before the marker after the first call will
lead to a missed entry.

That entry wouldn't have been part of the results from an initial
list-all call anyway. I'm fine skipping over it during an iterative
list.

With an index, one may have duplicate entries.

So just have the registry remember the last entry it got (as well as
the opaque marker), and then skip anything <= that entry when parsing
the next batch of entries.

How so? ZRANGEBYSCORE is O(log(N)+M) with N being the number of
elements in the sorted set and M the number of elements being
returned. That scales pretty well. More importantly, it lets you
start streaming results from the registry to the client before
you've finished listing the whole directory.

That's not the case I'm worried about. If we want to iterate over
the entries of a large directory to support an API operation, the
latency of the registry request will still be proportional to the
size of the directory, reentrant or not.

No, if the request is “give me a list of tags and their associated
image IDs so I can pull them” (#614), you can start writing back to
the client immediately, without waiting for the tag list to complete.
That's just a:

storage → registry → client

pipe, and there's no reason to block the registry → client leg.

Let's proceed with this simplified API and table this discussion,
for now. We have collected some really good suggestions but we
really need to look at how we can avoid this bottleneck. Once the V2
paths are laid out, we can come back and see if we really need the
reentrant API call.

Well, the API is versioned, so it shouldn't be too hard to adopt this
later. Still, it doesn't seem that complicated to me, and I think
we'll need to adopt something that lets us stream results. This is
what other storage engines already support for streamed listings, so I
expect it's a reasonable choice for how to structure this ;).

[wking]

On Tue, Oct 21, 2014 at 03:06:43PM -0700, Brian Bland wrote:

A main/storagedriver/inmemory/inmemory.go (10)

Very minor, and feel free to ignore, but I like “memory” more than
“inmemory”.

Random thoughts on the docs:

• “The storage driver API is designed to model a filesystem-like
  key/value storage…”. Why mention “filesystem-like” at all? Why not
  “The storage driver API allows key/value storage…”.
• My interpretation of “prefix” was “the initial portion of the key
  string” (e.g. lists with a “namespace/”, “namespace/b”, or
  “namespace/bob” prefix should all return the “namespace/bob” key,
  and the “namespace/bobby” key). Maybe we only want to support
  prefixes that end in a slash (which would make this more
  filesystem-like)?
• How do we hook up the local test suite to exercise an external,
  IPC-based driver? Maybe it's just not possible yet?
• How do we register an external, IPC-based driver? Do we have to
  recompile the registry to do so? A way to do this via the config
  and/or environment variables would be nice.
• You don't need libchan support in another language to have a non-Go
  driver. You could write a Go shim to convert the libchan IPC to a
  protocol that your target language does support. Mapping the
  current storage driver API to HTTP should be pretty simple, since we
  don't use libchan's special features 1.

[stevvooe]

I'm not sure that this comment belongs with this commit, but it might be reasonable to use an integration test build tag to separate the unit tests from the integration tests.

[stevvooe]

TL; DR LGTM!

I think this change is looking good. I'm putting together the specification for the V2 API tonight/tomorrow morning and will have a better idea of the underlying storage layout. We may make some changes going forward with information there but let's go ahead and get this PR merged.

[dmp42]

LGTM

This has been under scrutiny for two weeks, long enough I guess.
We can certainly fix things down the road now.

Merging.