NG: Need to clarify commit of writes in StorageDriver interface

[stevvooe]

Right now, the commit for chunks headed to backends like s3 and azure is unhandled. We need a solution such that multiple calls to WriteStream followed by some action correctly commit multipart uploads.

The original approach was to pass the total size to WriteStream, as a size argument. Unfortunately, this can be easily confused with the size of the chunk provided by the reader (a mistake that I have clearly made, even after getting clarification 😞). Issue #781 proposes to completely remove the confusing argument. The V2 api also defers the posting of the actual size to the API to the last chunk, making the use of the size argument as the completion sigil problematic.

There are a few options:

1. Bring back the size argument and clearly document that its the "total size" of the uploading content. If size is negative, leave the upload open so one only needs to pass it in the last chunk.
2. Refactor WriteStream(path string, offset int64, reader io.Reader) (nn int64, err error) into WritePartial(path string, offset int64, reader io.Reader) (nn int64, err error) and WriteCommit(path string, offset int64, reader io.Reader) (nn int64, err error).
3. (nuclear) Move the registry storage API up to the registry semantic level. This process is already represented in the storage package in the V2 registry, as LayerUpload. This avoids the entire notion of a VFS and standardizes on a common interface but defers a lot of behavior to the backend. It does have the benefit that the storage layout can be optimized for the particular backend.

The ongoing API changes are in the branch below, for reference:

https://github.com/stevvooe/docker-registry/tree/ng-storagedriver-updates

This branch will be PR'd sometime today, but I'd like for us to figure out this situation ASAP.

@BrianBland @AndreyKostov @ahmetalpbalkan @dmp42

[dmp42]

• @noxiouz @bacongobbler

[ahmetb]

@stevvooe WritePartial/WriteCommit will have almost the same functionality, right? (i.e. how will the filesystem implementations of these methods would look like?)

The notion of non-atomicity comes in the case of filesystem driver –drivers like inmemory and azure are already atomic. (i.e. if WriteStream returns err, nothing gets written, otherwise it means everything is written successfully.). However; I'm not sure about s3, does it have atomic (all or none) writes?

On the other hand, having size in the storagedriver (especially like the negative size stuff) is a bad idea. In the azure driver I did not use that size param at all. I'm not sure where it comes useful. It'd be better to remove that.

About the WriteCommit, are you planning to call it for the last chunk? When exactly it will get called? Can't it be without an offset or data passed to it? I'm not sure it contains the reader or offset as parameters... (e.g. Write(f,data), Write(f,data), ..., Commit(f, size) and commit just commits the file if f.size == size?

[AndreyKostov]

@ahmetalpbalkan You hit the nail on the head. The size parameter existed mostly because it was necessary for the s3 driver (and potentially other drivers in the future). The issue (as was stated) was that some drivers will require to explicitly be told when an upload is "done". Azure is not one of them, hence you did not need to make use of the size parameter (the purpose of which was to say, if you've written a total of size bytes, you're done).

That said, you are correct that it is not the cleanest way imaginable to handle the issue from an API perspective.

I suggest that we go with 2) and Refactor the api into:

// WriteStream stores the contents of the provided io.Reader at a
// location designated by the given path and returns the amount of bytes written.
// May be used to resume writing a stream by providing a nonzero offset.
// The offset must be no larger than the Size returned by Stat() for this path.
WriteStream(path string, offset int64, reader io.Reader) (nn int64, err error)

// WriteCommit signifies that the file at location designated by path is now
// complete. The registry will call this method after calling Stat(path) and seeing
// that the size returned is equal to the total size of the file it was uploading.
WriteCommit(path string) error

Under this version, for the Azure driver WriteCommit is a nop.

The only question is whether we also need to actually pass the size to WriteCommit, but this won't be
necessary if the registry can guarantee that it will only call it after calling Stat() and verifying the size.

[stevvooe]

@AndreyKostov LGTM! Let's just clarify that the return value nn is the number bytes read from reader.

[ahmetb]

@AndreyKostov sounds good. However I'm not sure how "nn" will be used. WriteStream should be writing whatever it gets until EOF from reader. I see that Go io.Writer.Write returns nn; however, in this case partial writes should be returning error.

If we require nn, it will be in favor of filesystem writer and other drivers will need to calculate the stream size to return the correct value.

[stevvooe]

@ahmetalpbalkan The return value nn is effectively implemented by io.Copy. If the code is structured to use io.Copy or io.CopyN, as opposed to io.ReadFull, its free.

[stevvooe]

@ahmetalpbalkan I realized that I only addressed half of your question: the nn return value is to indicate to the caller the number of bytes read from io.Reader. It allows the caller to evaluate the success of the write when no error is returned.

[BrianBland]

It turns out we don't need any of this for the s3 driver implementation.

To make the Stat behavior consistent with what has been written and what can be read, we can change the s3 driver to complete the multipart upload after each call to WriteStream. When calling WriteStream against an existing file, we can create a new multipart upload and use the old file as the first part via UploadPartCopy, then complete the upload (commit it). This will always leave us with "atomic" write operations.

[stevvooe]

@BrianBland This is great to hear. We'll hold off on the addition of adding a Commit/Complete/Sync method and update the s3 implementation. Will we have to fork the goamz driver to support this?

I'll close out this ticket completely where we hear back from @ahmetalpbalkan on the azure driver.

[BrianBland]

Yeah, we'll have to fork goamz to add support for this s3 api call, but
there's no reason they wouldn't accept it back into mainline, as it doesn't
break the package api at all.
On Dec 9, 2014 10:08 PM, "Stephen Day" [email protected] wrote:

@BrianBland https://github.com/BrianBland This is great to hear. We'll
hold off on the addition of adding a Commit/Complete/Sync method and
update the s3 implementation. Will we have to fork the goamz driver to
support this?

I'll close out this ticket completely where we hear back from
@ahmetalpbalkan https://github.com/ahmetalpbalkan on the azure driver.

—
Reply to this email directly or view it on GitHub
#814 (comment)
.

[ahmetb]

@stevvooe I agree with @AndreyKostov's comment about how the azure driver would look like in this case. Are you asking about anything specific?

[stevvooe]

@ahmetalpbalkan Checkout @BrianBland's comment. Basically, it's more effective to commit with each write, then use UploadPartCopy for s3, eliding the need for the Commit/Complete/Sync. If you could weigh in on the azure blob API, we can close this ticket and not implement the extra storage driver call.

[stevvooe]

As far as I can recall, we've worked out most of the issues with the driver API. If there are still problems, let's open up a new issue in docker/distribution referring back to this issue.