image-tools
image-tools copied to clipboard
opencontainers
image/cas: Add a generic CAS interface
And implement that interface for tarballs based on the specs image-layout.
Also add a new oci-cas command so folks can access the new read functionality from the command line.
The Context interface follows the pattern recommended here, allowing callers to cancel long running actions (e.g. push/pull over the network for engine implementations that communicate with a remote store).
This is the current first commit in #5, to see if nibbling away at that PR one commit at a time makes review any easier. I have a directory-based backend in #5 if you want to see what that looks like. In another commit in #5 I replace the image/walker.go functionality with this new API.
Great job!
IMO, we should add List API to return all digests for Engine interface, such as:
// List Return all digests
List(ctx context.Context) (digests []string, err error)
This is needed for GC in the future.
And more, I'd like to add Exist API:
// Exist Return true if digest is exist, else return false
Exist(ctx context.Context, digest string) (bool, error)
On Thu, Nov 17, 2016 at 07:49:24PM -0800, Ye Yin wrote:
IMO, we should add
ListAPI to return all digest forEngine interface, such as:List(ctx context.Context) (digest []string, err error)This is needed for GC in the future.
Listing is expensive, and I'd rather have GC be an opaque function 1. But we can certainly revisit this if/when the rest of the CAS API lands, when it will be easier to push concrete GC implementations. In the mean time, leaving list off is safer, because adding List is a backwards compatible change while removing it is a backwards incompatible change.
On Thu, Nov 17, 2016 at 08:25:25PM -0800, Ye Yin wrote:
And more, I'd like to add
ExistAPI:// Exist Return true if digest is exist, else return false Exist(ctx context.Context, digest string) (bool, error)
Is this for “I want to put this blob, whose hash I already know, but don't want to bother if that blob is already in storage”? Maybe in the context of copying between CAS engines? And you think that calling Get and immediately closing any succesfully returned reader would be too expensive (e.g. if the engine was backed by HTTP and you could do a HEAD instead of a GET)? I think there might be a solid case in there, but I'd rather wait and get the more obviously critical portions of the API landed first. Then we can circle back and look at this sort of optimization in a second pass.
Is this for “I want to put this blob, whose hash I already know, but don't want to bother if that blob is already in storage”? Maybe in the context of copying between CAS engines? And you think that calling Get and immediately closing any succesfully returned reader would be too expensive ...
Yeah, exactly:)
Listing is expensive, and I'd rather have GC be an opaque function [1]. But we can certainly revisit this if/when the rest of the CAS API lands, when it will be easier to push concrete GC implementations. In the mean time, leaving list off is safer, because adding List is a backwards compatible change while removing it is a backwards incompatible change...
IMO, List is a very basic API, GC is one case. Get all digests is very common requirement, such as statistics, just like list all references. Whether expensive is related with underlay implementation. For directory walker, maybe not a expensive thing?
On Thu, Nov 17, 2016 at 10:46:44PM -0800, Ye Yin wrote:
IMO,
Listis a very basic API,GCis one case. Get all digests is very common requirement, such as statistics, just like list all references…
Listing all references is a necessary evil ;). To collect statistics, you can use the ref-engine's List to walk refs (or subsets thereof), and then walk the Merkle DAG down from each ref to discover blobs. If you want a particular blob to show up in lists (and not be garbage collected), just add a new ref to pin it. I don't see a use case for listing blobs in the absence of refs outside of GC.
Whether expensive is related with underlay implementation. For directory walker, maybe not a expensive thing?
For a small, local, directory-backed CAS engine, blob-list performance would be acceptable. However, my goal is to write a generic API so higher-level tools can manipulate CAS without having to care about the engine implementation. If your CAS engine involves millions of blobs accessed via a distributed hash table or sharded across many hosts, walking significant chunks of that is going to be expensive. And garbage collection is likely to be more sophisticated than stop-the-world mark and sweep. Unless we have a very convincing use case for all CAS engines to provide a List API, I'd rather avoid it.
And again, the CAS API I'm proposing here covers all the use-cases we currently have inside image-tools. If we land this without List and later decide that List is necessary, a follow-up PR can add it later. It should only matter for this PR if your argument is that you need List for a use-case this repository already addresses.
@wking I'm reading it but not debug yet.
Can we use Get(ctx context.Context, desc descriptor), to instead Get(ctx context.Context, digest string). I select descriptor as parameter in image/reader.go because it bring helpful information, like:
if hdr.Name == filepath.Join("blobs", desc.algo(), desc.hash()) instead your:
targetName := fmt.Sprintf("./blobs/%s/%s", algorithm, hash)
if header.Name == targetName{
}
Some variables in Get() is likely superfluous.
On Fri, Nov 18, 2016 at 12:20:55AM -0800, xiekeyang wrote:
Can we use
Get(ctx context.Context, desc descriptor), to instead Get(ctx context.Context, digest string).
I've avoided going with the image-spec Descriptor because while size information may be useful (e.g. you may want to compare the expected size against the retrieved blob), the media type information is not useful (the CAS engine should not care or need to track the types of stored blobs). I'd really like to avoid the image-tools descriptor, because:
- It's currently private, so I can't use it from image/cas.
- Converting between image-spec and image-tools descriptor types is an unnecessary headache for users.
- The desc.algo() and desc.hash() helpers are only really interesting to CAS-engine implementers. For everyone else, the digest might as well be an opaque string.
In a later #5 commit I add blobPath 1 and refPath 2 helpers to abstract these out. If it would make you more comfortable with this commit I can move those definitions over here.
In a later #5 commit I add blobPath [1] and refPath [2] helpers to abstract these out. If it would make you more comfortable with this commit I can move those definitions over here.
Good!
@wking
Also add a new oci-cas command so folks can access the new read functionality from the command line.
I think CAS engine interface is great, but I'm not sure that command oci-cas get is useful. Do we really need this command? (I might misunderstand the new read functionality)
On Fri, Nov 18, 2016 at 03:34:16AM -0800, xiekeyang wrote:
In a later #5 commit I add blobPath [1] and refPath [2] helpers to abstract these out. If it would make you more comfortable with this commit I can move those definitions over here.
Good!
Is that “yes, squash those into this commit”? Or is it “ah, then this commit is ok because after we chew through the rest of #5 we'll be in a good place”?
Is that “yes, squash those into this commit”? Or is it “ah, then this commit is ok because after we chew through the rest of #5 we'll be in a good place”?
Sorry, I mean could you please squash those of blobPath and refPath to this commit?
On Fri, Nov 18, 2016 at 03:40:35AM -0800, xiekeyang wrote:
I think CAS engine interface is great, but I'm not sure that command
oci-cas getis useful.
As #5 continues, oci-cas fills in to cover the rest of the interface (put and delete). Again, I'm happy to shuffle things around if you'd like all of that pulled into this commit, or all of the command line stuff pushed out into a later commit.
Do we really need this command?
“Really need” is hard to put a finger on. None of our Go code shells out to oci-cas ;). But the command line wrappers are easy to write, and I like providing a non-Go API for folks who are interested in interacting with OCI-compatible CAS engines. I certainly don't think oci-cas is going to be the highest-level command line tool we offer, but I don't see a reason to not offer it. For an analogy in another plumbing-rich context, see Git's cat-file 1 and hash-object 2.
On Fri, Nov 18, 2016 at 03:57:19AM -0800, xiekeyang wrote:
Sorry, I mean could you please squash those of
blobPathandrefPathto this commit?
blobPath shifted in with 23d8fb7 → fbae6d9. The ref interface is in a follow-up commit, but I've adjusted all of #5 to take this early-*Path-helper approach if you want to check that out.
@xiekeyang I don't think this is the right model. It doesn't have a transaction interface for verifying and setting blobs. Half of the interface is useless for the application of tar files and I am not sure what problems this solves for image-tools.
For this to be useful, we'd have to refactor the entire thing. As I said when this PR was in image, please use the docker/distribution repo, and the myriad other Go CAS projects, to inform the API.
On Wed, Nov 30, 2016 at 03:21:10PM -0800, Stephen Day wrote:
It doesn't have a transaction interface for verifying and setting blobs.
What sort of interface are you looking for? Setting blobs seems like it would be covered by Engine.Put 1, and you don't need a transactional interface for a CAS engine (although you do want a transactional implementation).
Half of the interface is useless for the application of tar files…
Which half? I can split Engine into CASReader and CASWriter or some such if that would make the tar backing more appealing.
… and I am not sure what problems this solves for image-tools.
It abstracts away the details of the CAS engine so we don't need to bake it in (like we currently do, including walker-based lookup for directory-backed CAS [2,3]!).
For this to be useful, we'd have to refactor the entire thing.
I'm happy to refactor. I've looked at the docker/distribution and camlistore interfaces 4, and this is where I ended up. But if you'd like the interface methods modified, let me know what you'd like changed and why.
@PointMicro This approach has some major design problems. What are you trying to do exactly? Maybe I can point your towards something useful.
On Wed, Feb 15, 2017 at 07:42:08PM -0800, Stephen Day wrote:
This approach has some major design problems.
I'm still not clear on what these design problems are, but I'm happy to fix them if you point them out.
Now that we have go-digest as a dependency (#104, #107), I've rerolled this PR to use the Digest type for digests with fbae6d9 → a44de0d.
I'm still not clear on what these design problems are, but I'm happy to fix them if you point them out.
And I am not going to sit down and reiterate them again. I have provided several examples, there are multiple projects to draw from (umoci, containerd, docker/distribution, camlistore, etc.) that all have addressed these issues.
Furthermore, it treats tar archives as a random access storage medium and makes multiple poor assumptions.
On Thu, Feb 16, 2017 at 02:57:16PM -0800, Stephen Day wrote:
… there are multiple projects to draw from (umoci…
umoci has addressed tar access by erroring out when pointed at a tar-backed image-layout 1. It's CAS Get interface 2 is identical to mine 3.
… containerd,…
I see no image-layout support in containerd:
containerd $ git grep blobs v0.2.5 v0.2.5:vendor/src/golang.org/x/sys/unix/syscall_linux.go: // to be uninterpreted fixed-size binary blobs--but v0.2.5:vendor/src/golang.org/x/sys/unix/syscall_solaris.go: // to be uninterpreted fixed-size binary blobs -- but
Very recently (with docker/containerd#452) you gave containerd a fetcher which adds a hints argument and makes the ID more generic 4, but is otherwise identical to my interface.
… docker/distribution, …
distribution has the same interface, except that it returns ReadSeekClosers instead of my ReadClosers 5. It also supports a getter that returns a byte array 6, but that's clearly the convenient special case because it would be very expensive for large blobs.
I couldn't find any image-layout support in docker/distribution (which isn't surprising, since it's basically an alternative store format/service).
… camlistore,…
camlistore seems to dispense with the Context, and it also returns a size (in addition to the ReadCloser and error) 7.
Furthermore, it treats tar archives as a random access storage medium and makes multiple poor assumptions.
So as far as I can tell, I'm doing the same thing as everyone else, unless you're really missing camlistore's returned size or containerd's hints.
Or are you fine with my API, and just concerned with the implementation? I don't see anyone else implementing tar-backed image-layout support, but please point me at them if I've missed them.
@wking The problem here is that you are too focused on "image layout" which is a minor part of the image format.
Or are you fine with my API, and just concerned with the implementation?
No. You're API is naive, racy and memory intensive.
I don't see anyone else implementing tar-backed image-layout support, but please point me at them if I've missed them.
Exactly. This makes no sense. The only way to do this with a CAS store would be to have a transactional addition of blobs. Effectively, you have a transaction for each blob addition, then a transaction that creates the tar file.
On Thu, Feb 16, 2017 at 03:34:29PM -0800, Stephen Day wrote:
Or are you fine with my API, and just concerned with the implementation?
No. You're API is naive, racy and memory intensive.
The Get API? It's basically the same as everyone else's 1.
The only way to do this with a CAS store would be to have a transactional addition of blobs.
There is no blob addition going on in this PR. I've just pushed ccab818 → 5df5883 which drops the Put and Delete stubs in case they were what you were pushing back against.
I've just pushed 5df5883 → fe6a35b rebasing this work onto master, since @q384566678 expressed renewed interest in #5, of which this PR is the next component. I didn't make any changes to image/cas, and most of the rebase was adjusting to the oci-image-tools consolidation from #103.
And I've just pushed fe6a35b → cda19fe fixing some go fmt issues and replacing the deprecated os.SEEK_SET with io.SeekStart to fix this lint error.