image: Refactor to use cas/ref engines instead of walkers #5
Conversation
|
/cc @s-urbaniak |
wking
force-pushed
the
cas-api
branch
5 times, most recently
from
9687484 to
90c4fea
Compare
|
I've pushed 9687484 → 90c4fea with:
On Fri, Sep 16, 2016 at 01:53:40AM -0700, W. Trevor King wrote:
This email may be signed or encrypted with GnuPG (http://www.gnupg.org). |
|
This patch seems to be too great to be merged sooner. Could you please split it and submit one by one commit? Now we need the method instead of walking, and go forward to some other functionality based on it. I'm working on my local side https://github.com/xiekeyang/image-tools/commit/ff0401330fdca591f67b82924c74fc84ce9d052f referring to your implementation, which is temporary solution and should wait for your patch merged. So I hope your commit can be merged. Thanks so much. |
|
On Wed, Sep 21, 2016 at 05:27:46PM -0700, xiekeyang wrote:
First commit submitted separately as #28. If/when that lands, I'll |
|
On Wed, Sep 21, 2016 at 10:54:19PM -0700, W. Trevor King wrote:
Rebased onto master with 7c9efa7 → 1926150 now that #28 has landed. |
wking
force-pushed
the
cas-api
branch
2 times, most recently
from
cb0b850 to
4dfe06f
Compare
|
I've pushed ee44a09 → 4dfe06f fixing a few more lint issues with the
|
Making the CAS/refs Get implementations more DRY. Signed-off-by: W. Trevor King <wking@tremily.us>
A fair amount of image setup involves pushing JSON objects to CAS, so provide a convenient wrapper around that. This implementation could be improved, with at least: * Consistent key sorts, etc. to increase the chances of matching an existing CAS blob. * Streaming the marshaled JSON into the engine to avoid serializing it in memory before passing it into Engine.Put. But the API is fine, and we can improve the implementation as we go. Signed-off-by: W. Trevor King <wking@tremily.us>
Generated with: $ make install.tools $ make update-deps $ git checkout HEAD -- vendor/github.com/spf13/pflag $ git checkout HEAD -- vendor/github.com/runtime-spec I rolled back the other changes because I haven't checked for compatibility issues due to the upgrades. I also rolled back the hash bumps for those packages in glide.lock. Signed-off-by: W. Trevor King <wking@tremily.us>
As called for during today's meeting [1]. The wording is from Brandon [2]. [1]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2016/opencontainers.2016-07-27-21.00.log.html#l-54 [2]: https://github.com/opencontainers/image-spec/pull/159/files/9f8a18eb1cb2a1cbccd6fe137d743dccc2142901#r72525379 Signed-off-by: W. Trevor King <wking@tremily.us>
This is a bit awkward. For writing a tar entry, we need to know both
the name and size of the file ahead of time. The implementation in
this commit accomplishes that by reading the Put content into a
buffer, hashing and sizing the buffer, and then calling
WriteTarEntryByName to create the entry. With a filesystem-backed CAS
engine, we could avoid the buffer by writing the file to a temporary
location with rolling hash and size tracking and then renaming the
temporary file to the appropriate path.
WriteTarEntryByName itself has awkward buffering to avoid dropping
anything onto disk. It reads through its current file and writes the
new tar into a buffer, and then writes that buffer back back over its
current file. There are a few issues with this:
* It's a lot more work than you need if you're just appending a new
entry to the end of the tarball. But writing the whole file into a
buffer means we don't have to worry about the trailing blocks that
mark the end of the tarball; that's all handled transparently for us
by the Go implementation. And this implementation doesn't have to
be performant (folks should not be using tarballs to back
write-heavy engines).
* It could leave you with a corrupted tarball if the caller dies
mid-overwrite. Again, I expect folks will only ever write to a
tarball when building a tarball for publishing. If the caller dies,
you can just start over. Folks looking for a more reliable
implementation should use a filesystem-backed engine.
* It could leave you with dangling bytes at the end of the tarball. I
couldn't find a Go invocation to truncate the file. Go does have an
ftruncate(2) wrapper [1], but it doesn't seem to be exposed at the
io.Reader/io.Writer/... level. So if you write a shorter file with
the same name as the original, you may end up with some dangling
bytes.
cas.Engine.Put protects against excessive writes with a Get guard;
after hashing the new data, Put trys to Get it from the tarball and
only writes a new entry if it can't find an existing entry. This also
protects the CAS engine from the dangling-bytes issue.
The 0666 file modes and 0777 directory modes rely on the caller's
umask to appropriately limit user/group/other permissions for the
tarball itself and any content extracted to the filesystem from the
tarball.
The trailing slash manipulation (stripping before comparison and
injecting before creation) is based on part of libarchive's
description of old-style archives [2]:
name
Pathname, stored as a null-terminated string. Early tar
implementations only stored regular files (including hardlinks to
those files). One common early convention used a trailing "/"
character to indicate a directory name, allowing directory
permissions and owner information to be archived and restored.
and POSIX ustar archives [3]:
name, prefix
... The standard does not require a trailing / character on
directory names, though most implementations still include this
for compatibility reasons.
[1]: https://golang.org/pkg/syscall/#Ftruncate
[2]: https://github.com/libarchive/libarchive/wiki/ManPageTar5#old-style-archive-format
[3]: https://github.com/libarchive/libarchive/wiki/ManPageTar5#posix-ustar-archives
Signed-off-by: W. Trevor King <wking@tremily.us>
This is pretty straightforward with the new WriteTarEntryByName helper. I considered pulling the ref name -> path conversion (%s -> ./refs/%s) out into a helper function to stay DRY, but the logic is simple enough that it seemed more intuitive to leave it inline. Signed-off-by: W. Trevor King <wking@tremily.us>
The NewEngine commands for the tar-backed image-layout engines (both
the CAS and refs engines) open files O_RDWR and expect image-layout
compatible content in the tarball. That makes sense, but for folks
who *don't* have such a tarball, a helper like CreateTarFile makes it
easy to explicitly create an empty one.
The 0666 file modes and 0777 directory modes rely on the caller's
umask to appropriately limit user/group/other permissions for the
tarball itself and any content extracted to the filesystem from the
tarball.
The trailing slashes are based on part of libarchive's description of
old-style archives [1]:
name
Pathname, stored as a null-terminated string. Early tar
implementations only stored regular files (including hardlinks to
those files). One common early convention used a trailing "/"
character to indicate a directory name, allowing directory
permissions and owner information to be archived and restored.
and POSIX ustar archives [2]:
name, prefix
... The standard does not require a trailing / character on
directory names, though most implementations still include this
for compatibility reasons.
Expose this new functionality on the command line as:
$ oci-image-init image-layout PATH
where 'image-layout' is a separate level in case we support
initializing additional types of repositories in the future.
[1]: https://github.com/libarchive/libarchive/wiki/ManPageTar5#old-style-archive-format
[2]: https://github.com/libarchive/libarchive/wiki/ManPageTar5#posix-ustar-archives
Signed-off-by: W. Trevor King <wking@tremily.us>
The validation/unpacking code doesn't really care what the reference and CAS implemenations are. And the new generic interfaces in image/refs and image/cas will scale better as we add new backends than the walker interface. This replaces the simpler interface from image/reader.go with something more robust. The old tar/directory distinction between image and imageLayout is gone. The new CAS/refs engines don't support directory backends yet (I plan on adding them once the engine framework lands), but the new framework will handle tar/directory/... detection inside layout.NewEngine (and possibly inside a new (cas|refs).NewEngine when we grow engine types that aren't based on image-layout). Also replace the old methods like: func (d *descriptor) validateContent(r io.Reader) error with functions like: validateContent(ctx context.Context, descriptor *specs.Descriptor, r io.Reader) error to avoid local types that duplicate the image-spec types. This saves an extra instantiation for folks who want to validate (or whatever) a specs.Descriptor they have obtained elsewhere. I'd prefer casLayout and refsLayout for the imported packages, but Stephen doesn't want camelCase for package names [1]. [1]: opencontainers/image-spec#159 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
Most of this is new boilerplate, but oci-image-tools.7.md is based on the old oci-image-tool.1.md removed by fe363aa (*: move to opencontainers/image-tools, 2016-09-15). There's a lot going on in this repo, and it's nice to have a page that outlines everything provided by the project even though we're no longer providing a single command. Signed-off-by: W. Trevor King <wking@tremily.us>
Don't worry about: $ make lint checking lint image/cas/layout/dir.go:37::warning: duplicate of image/refs/layout/dir.go:37-54 (dupl) image/refs/layout/dir.go:37::warning: duplicate of image/cas/layout/dir.go:37-54 (dupl) cmd/oci-cas/delete.go:41::warning: duplicate of cmd/oci-cas/get.go:43-62 (dupl) cmd/oci-refs/delete.go:41::warning: duplicate of cmd/oci-refs/get.go:42-61 (dupl) cmd/oci-cas/delete.go:15::warning: duplicate of cmd/oci-refs/delete.go:15-72 (dupl) cmd/oci-refs/delete.go:15::warning: duplicate of cmd/oci-cas/delete.go:15-72 (dupl) cmd/oci-cas/get.go:43::warning: duplicate of cmd/oci-refs/get.go:42-61 (dupl) cmd/oci-refs/get.go:42::warning: duplicate of cmd/oci-cas/get.go:43-62 (dupl) make: *** [lint] Error 1 The commands are all similar (open an engine, perform some method, print the result), but are short enough that extracting out helpers would be more trouble and indirection than it's worth. Oddly, dupl seems happy to print: "duplicate of oci-cas/get.go:..." and "duplicate of get.go:..." if I exclude: "duplicate of cmd/oci-cas/get.go:..." or "duplicate of .*oci-cas/get.go:..." I want to get "oci-cas" and "oci-refs" in the exclusion regular expression somewhere to avoid accidentally skipping dupl checks for other get.go and similar if they show up somewhere else in the repository, so I'm matching on the initial filename. Signed-off-by: W. Trevor King <wking@tremily.us>
These are much nicer than the tar engines (hooray atomic renames :), so switch the manifest tests tests back to using the directory-backed engines. I also switched the man-page examples over to directory-backed layouts, now that they're what oci-image-init generates by default. And I added command-line wrappers for the delete methods, now that we have a backend that implements it. I do with there was a paginated, callback-based directory lister we could use instead of ioutils.ReadDir. On the other hand, by the time directories get big enough for that to matter we may be sharding them anyway. Signed-off-by: W. Trevor King <wking@tremily.us>
|
@cyphar Sorry for the slow cycle, but it looks like that link is broken. Shall we take this design into an issue and close this PR? |
|
On Fri, Nov 18, 2016 at 12:11:25PM -0800, Stephen Day wrote:
He removed it in cyphar/umoci@8d19f36. The last version before that
I'm happy to discuss any design differences between what I have here |
|
SGTM, What is the current state of this patch? |
Wait, is there really renewed interest in this over a year since I filed it as opencontainers/image-spec#159? If so, you should probably start with #40, which has just the CAS part of this PR. And getting a green light or concrete improvement suggestions from @stevvooe would be nice, since he's been pushing back against my proposed CAS interface, most recently here. |
|
@q384566678 We worked hard on defining a useful content api in https://godoc.org/github.com/containerd/containerd/content. It handles all the cases of resumption, keeps memory usage low and handles pre and post known digest entries, as well as concurrency issues. We've also been able to use There may be a few tweaks we still need to make, but it is the culmination of work around the old docker/distribution, docker and containerd, as well as some research on distributed content addressable storage systems. I'd advocate for closing this PR. Let me know if you have questions. |
Comparing that API's type Provider interface {
Reader(ctx context.Context, dgst digest.Digest) (io.ReadCloser, error)
ReaderAt(ctx context.Context, dgst digest.Digest) (io.ReaderAt, error)
}with my type Engine interface {
Get(ctx context.Context, digest digest.Digest) (reader io.ReadCloser, err error)
}You can see tht we have exactly the same API. And it is also shared by umoci, where it is |
|
@wking @stevvooe @xiekeyang @cyphar |
|
In case folks are interested, I have more recent work on this front in the stand-alone casengine repo, and there's a good ref-engine reader in oci-discovery (although there is currently no ref-engine writer there). @xiekeyang and I would be happy to get feedback on those and, eventually if feedback is positive, transfer those to the OCI. |
Abstract out the details of the image-layout format, since the validators and unpackers shouldn't care about that sort of backend stuff. This should make it easy to build backends based on zip files, HTTP, the Docker registry format, etc.
Carried from opencontainers/image-spec#159. The outstanding issue there was what to do about tar handling. I'm fine with this PR as it stands, but I've floated a few alternatives in case the maintainers want to pick an alternative path.