Add oci-archive transport that creates a tar archive of an image #314

umohnani8 · 2017-07-27T13:46:13Z

The oci-archive transport uses oci to create a directory of the image, which is then tarred up into a file. oci-archive will be used in kpod save and kpod load for oci images.
Signed-off-by: umohnani8 umohnani@redhat.com

umohnani8 · 2017-07-27T13:50:53Z

@rhatdan @mtrmac PTAL

rhatdan · 2017-07-27T15:40:42Z

oci/layout/oci_archive_dest.go

+	index imgspecv1.Index
+}
+
+var ociImgDest types.ImageDestination


Can this be added to the struct above?

rhatdan · 2017-07-27T15:41:59Z

oci/layout/oci_archive_dest.go

+	if err := tar(path); err != nil {
+		return err
+	}
+


You need to remove the ociImg when you are done.

rhatdan · 2017-07-27T15:43:23Z

oci/layout/oci_archive_dest.go

+	// temporary path of the tar file
+	tempPath := path + "1"
+	defer os.Rename(tempPath, path)
+	defer os.RemoveAll(path)


THis should happen in the caller, not here.

Why not leave it in tar? It tars up the directory, removes the directory and renames the tar file to what the directory was before.

It would be somewhat nice if the code allowed replacing an older tar file in the destination location atomically with a new image. It’s by no means required now, but it shouldn’t be impossible.

And even disregarding atomicity, it is at least somewhat plausible that the user has used exactly the same destination path in the past, and a file already exists there, so we can’t just create a directory in the same path, at least without removing the old contents.

Overall, sharing the path for both the file and directory, though kinda a neat trick, is just not worth the hassle AFAICT.

If I call into a function with a file, I don't expect to have the file modified or changed.

Also allows the caller to control the naming. I would rather have temporary names.

rhatdan · 2017-07-27T15:44:31Z

oci/layout/oci_archive_dest.go

+}
+
+// tar converts the direstory at path to a tar file and deletes the directory
+func tar(path string) error {


This should take an src,dest and then don't do the Rename stuff.

Yeah; newImageDestinationArchive should create a temporary directory (grep for temporaryDirectoryForBigFiles, not in the default path), create an oci/layout.ociReference pointing to that reference, then the Commit implementation should just tar it up into the desired destination path, and Close delete the temporary directory.

rhatdan · 2017-07-27T15:45:56Z

oci/layout/oci_archive_src.go

+}
+
+// unTar unpacks a tar file into a directory and deletes the tar file
+func unTar(path string) error {


Should have src,dest

mtrmac · 2017-07-27T19:07:41Z

oci/layout/oci_archive_dest.go

@@ -0,0 +1,126 @@
+package layout


Please use a separate …/oci/archive subpackage for all of this.

She will have to export the interface from OCI if she does.

Only as long as the code keeps relying on the internals of ociReference etc., which should not be necessary if the rest of the code converts to use temporary directories.

I started out by packaging this separately, but I was having a hard time initializing ociReference. Since all the functions of oci-archive call the functions in oci, I need to be able to set ociReference and use it to call the oci functions. I was using Get and Set functions but was failing to get it work.
Things make more sense now, so I am going to work on separating it.

oci/layout.NewReference should be all that is necessary AFAICT.

Then, perhaps, some utility subpackage to minimize the amount of copy&pasted parsing/formatting code shared between oci/layout and oci/archive, ValidatePolicyConfigurationScope and the like — depending on the details of how the two end up.

mtrmac · 2017-07-27T19:10:00Z

oci/layout/oci_archive_transport.go

+)
+
+func init() {
+	transports.Register(ArchiveTransport)


The types.ImageTransport and types.ImageReference types, due to their relevance for signature enforcement, should have test coverage close to 100%. Perhaps roughly base it on docker/archive/transport_test.go, or I might be able to help with that later.

… but let’s wait with the tests until the design, and in particular the reference parsing, is designed and nailed down.

mtrmac · 2017-07-27T19:10:28Z

oci/layout/oci_archive_transport.go

+// ArchiveTransport is an ImageTransport for OCI archive
+// it creates an oci-archive tar file by calling into the OCI transport
+// tarring the directory created by oci and deleting the directory
+var ArchiveTransport = ociArchiveTransport{}


With the oci/archive package, this should be named just Transport.

mtrmac · 2017-07-27T19:11:54Z

oci/layout/oci_archive_transport.go

+
+type ociArchiveTransport struct{}
+
+var ociTr = ociTransport{}


The types.ImageTransport implementations should generally be stateless singletons (containers-storage is the only exception so far). Just call oci/layout.Transport.$method directly if necessary.

mtrmac · 2017-07-27T19:13:14Z

oci/layout/oci_archive_transport.go

+type ociArchiveTransport struct{}
+
+var ociTr = ociTransport{}
+var ociRef types.ImageReference


This can't be a global variable, that would make it impossible to use multiple ociArchiveReference objects simultaneously. Looks like this should be a member of ociArchiveReference.

… but see the discussion in ImageSource/ImageDestination implementations; it rather seems that the ociReference lifetime should be tied to the lifetimes of the temporary directories, i.e. only members of ociArchiveImageDestination/ociArchiveImageSource

mtrmac · 2017-07-27T19:25:09Z

oci/layout/oci_archive_dest.go

+}
+
+// tar converts the direstory at path to a tar file and deletes the directory
+func tar(path string) error {


Yeah; newImageDestinationArchive should create a temporary directory (grep for temporaryDirectoryForBigFiles, not in the default path), create an oci/layout.ociReference pointing to that reference, then the Commit implementation should just tar it up into the desired destination path, and Close delete the temporary directory.

mtrmac · 2017-07-27T19:29:19Z

oci/layout/oci_archive_dest.go

+	// temporary path of the tar file
+	tempPath := path + "1"
+	defer os.Rename(tempPath, path)
+	defer os.RemoveAll(path)


It would be somewhat nice if the code allowed replacing an older tar file in the destination location atomically with a new image. It’s by no means required now, but it shouldn’t be impossible.

And even disregarding atomicity, it is at least somewhat plausible that the user has used exactly the same destination path in the past, and a file already exists there, so we can’t just create a directory in the same path, at least without removing the old contents.

Overall, sharing the path for both the file and directory, though kinda a neat trick, is just not worth the hassle AFAICT.

mtrmac · 2017-07-27T19:29:50Z

oci/layout/oci_archive_src.go

+	"github.com/pkg/errors"
+)
+
+var ociImgSrc types.ImageSource


This needs to be a member of ociArchiveImageSource, not a global variable.

mtrmac · 2017-07-27T19:30:06Z

oci/layout/oci_archive_dest.go

+
+type ociArchiveImageDestination struct {
+	ref   ociArchiveReference
+	index imgspecv1.Index


index seems unused.

mtrmac · 2017-07-27T19:37:12Z

oci/layout/oci_archive_src.go

+		return err
+	}
+	path := ociRef.(ociReference).resolvedDir
+	if err := tar(path); err != nil {


ociArchiveImageSource really must not modify the original file in any way, not even with the best intentions of replacing it later:

In the meantime others expecting to use the original file at that path can't do so.

The process may crash, leaving the original replaced by an unexpected directory forever.

Nothing guarantees that untar+tar will reproduce the original byte-for-byte exactly.

Similarly to the discussion about ociArchiveImageDestination, newImageSourceArchive should create a temporary directory and extract the image there.

(Alternatively, it would be also possible to implement ociArchiveImageSource which does not need to extract the tarball at all, by just seeking around the original tarball, like docker/tarfile/Source.openTarComponent and callers do. Then oci/archive and oci/layout could share the format implementation, differing only in the backend used to access file components. But that would only work with seekable inputs; extracting the tarball and calling the layout implementation directly is perfectly fine.)

mtrmac · 2017-07-27T19:42:55Z

oci/layout/oci_archive_src.go

+
+type ociArchiveImageSource struct {
+	ref        ociArchiveReference
+	descriptor imgspecv1.Descriptor


descriptor seems unused.

umohnani8 · 2017-07-28T15:35:42Z

I separated oci-archive to a sub package called archive. Got rid of the global variables and added ociImageDestination and ociImageSource to the respective Archive structs.
I create a temp directory in /var/tmp called oci-image and pass this path to ociReference - ociArchiveReference stores the path given by the user so that the tar file is stored there at the end.
This temp directory is then deleted in Close().
@rhatdan @mtrmac PTAL.

mtrmac · 2017-07-28T21:49:32Z

oci/archive/oci_dest.go

+
+// Reference returns the reference used to set up this destination.
+func (d *ociArchiveImageDestination) Reference() types.ImageReference {
+	return d.ociImgDest.Reference()


This should return d.ref. Same in the ImageSource implementation.

mtrmac · 2017-07-28T21:50:53Z

oci/archive/oci_dest.go

+func (d *ociArchiveImageDestination) Close() error {
+	_, tmpDir, _ := layout.GetOciReference(d.ref.ociRef)
+	defer os.RemoveAll(tmpDir)
+	return d.ociImgDest.Close()


I think a possible failure of os.RemoveAll should be reported to the user (though it arguably that whatever the user wanted to do did succeed); instead of defer, just call the two things in sequence and check errors. Same in the ImageSource implementation.

mtrmac · 2017-07-28T21:52:39Z

oci/archive/oci_dest.go

+// tar converts the direstory at src and saves it to dst
+func tar(src, dst string) error {
+	// input is a stream of bytes from the archive of the directory at path
+	input, err := archive.Tar(src, 0)


archive.Uncompressed would be more readable than 0.

@mtrmac could you explain this a bit more? What should it be if not 0?

The archive.Uncompressed typed constant. (The constant does the value 0, but it is typed and readable without checking the source code of the archive package)

mtrmac · 2017-07-28T21:53:58Z

oci/archive/oci_dest.go

+	}
+
+	// creates a temporary file
+	outFile, err := os.Create(dst)


dst is not a temporary path AFAICT.

mtrmac · 2017-07-28T21:56:18Z

oci/archive/oci_dest.go

+
+type ociArchiveImageDestination struct {
+	ref        ociArchiveReference
+	ociImgDest types.ImageDestination


This might be a bit more readable when named unpackedDest, unpacked, or something like that. Same in the ImageSource implementation.

mtrmac · 2017-07-28T21:58:24Z

oci/archive/oci_src.go

+}
+
+// unTar unpacks the tar file at src and saves it at dst
+func unTar(src, dst string) error {


(This, at 4 lines of code and 4 lines of function wrapping + 3 lines at the single call site, is small enough that it could be inlined into the caller; but having a separate function, for symmetry with tar, is also fine.)

mtrmac · 2017-07-28T22:04:41Z

oci/layout/oci_transport.go

+// GetOciReference returns the dir, resolvedDir, and tag of ociReference
+// so that it is accessible in oci/archive
+func GetOciReference(t types.ImageReference) (string, string, string) {
+	return t.(ociReference).dir, t.(ociReference).resolvedDir, t.(ociReference).tag


This would just panic if the caller supplied a wrong type. But overall, after the use in archive.ParseReference is removed, this won’t be needed: when the caller creates a reference for the temporary directory, it can just remember the directory path. That wastes a tiny bit of memory but the code is easier to follow and clearly can’t panic.

mtrmac · 2017-07-28T22:08:29Z

oci/archive/oci_transport.go

+	if err != nil {
+		return nil, errors.Wrapf(err, "error parsing reference %q", reference)
+	}
+	dir, resolvedDir, tag := layout.GetOciReference(ociArchRef)


This is still passing a file path to a transport which expects a directory.

Please just copy&paste most of oci/layout/oci_transport.go into this file, more or less with s/dir/file/g; s/layout/archive/g instead of bending the semantics like this. Or perhaps create a separate helper subpackage for the shared code, which is explicitly documented to use generic paths without knowing whether they are files or directories.

@mtrmac okay, would you prefer I copy the code from oci_transport.go or create a separate helper package? For the helper package, would the main changes just be the documentation and variable names?

For now I’d prefer to just copy that code; that will be sufficient to get this PR merged from my point of view.

We can live with two duplicates of that code for a while I think (@runcom ?); cleaning that up can be a separate PR.

agree with @mtrmac to keep duplicated code for now

mtrmac · 2017-07-28T22:13:47Z

oci/archive/oci_transport.go

+
+	// tempRef is the path where the directory of the oci image created by oci is stored
+	// this directory is deleted after a tar file is created from it
+	tempRef := "/var/tmp/oci-image:" + tag


Nothing guarantees the "oci-image:"+tag file name is available; use ioutil.TempDir to get a guaranteed-unique-and-available name.

OTOH, ParseReference may be used by callers who will never be calling NewImageSource/NewImageDestination (e.g. when parsing /etc/containers/policy.json), so this method should not be modifying the filesystem at all; so it also can’t create the layout reference. Both will need to happen later in newImageSource/newImageDestination.

Finally, please use the temporaryDirectoryForBigFiles constant the way other sites do. Eventually we’d like to have that directory configurable via types.SystemContext; for now, just using the same constant name makes it easy to find other cases.

mtrmac · 2017-07-28T22:14:50Z

oci/archive/oci_transport.go

+	// this directory is deleted after a tar file is created from it
+	tempRef := "/var/tmp/oci-image:" + tag
+	// ociRef stores the dir, resolvedDir, and tag of the temporary path
+	ociRef, err := layout.Transport.ParseReference(tempRef)


There is layout.NewReference with separate dir and tag arguments; it isn’t necessary to format them into a string only for layout.Transport.ParseReference to split them again.

umohnani8 · 2017-07-31T20:17:40Z

ignore this new push, still working on it.

umohnani8 · 2017-08-01T14:43:16Z

@mtrmac PTAL. I have created a function createOciRef that creates the temp directory and generates the ociReference from the directory's path. The functions that need ociReference will first check to see if it exists, if not createOciRef is called.

mtrmac

Just a quick look, didn’t go through line by line.

mtrmac · 2017-08-01T15:08:44Z

oci/archive/oci_dest.go

+// newImageDestination returns an ImageDestination for writing to an existing directory.
+func newImageDestination(ref ociArchiveReference, ctx *types.SystemContext) (types.ImageDestination, error) {
+	if ref.ociRef == nil {
+		if err := createOciRef(&ref); err != nil {


Does this work with

ref := ParseReference(…) // possibly repeated: dest := ref.NewImageDestination() … dest.Close() // deletes ref.ociRef.tempDirectory ?! // possibly repeated: src := ref.NewImageSource() // tempDirectory unavailable … src.Close() // deletes ref.ociRef.tempDirectory ?!

the layout.Reference, and tempDirectory, AFAICS needs to be a property of the …Source / …Destination, not of the …Reference, to match the temporary directory lifetimes which terminate at *.Close.

mtrmac · 2017-08-01T15:10:34Z

oci/archive/oci_transport.go

+
+// ValidatePolicyConfigurationScope checks that scope is a valid name for a signature.PolicyTransportScopes keys
+func (t ociArchiveTransport) ValidatePolicyConfigurationScope(scope string) error {
+	return ocilayout.Transport.ValidatePolicyConfigurationScope(scope)


Still mixing the semantics… either copy&paste layout.Transport.ValidatePolicyConfigurationScope, or extract into a shared helper which is explicitly agnostic of dir/path (i.e. rename existing layout.Transport.ValidatePolicyConfigurationScope to something like oci.ValidatePathPolicyConfigurationScope, and then call it from both {archive,layout}.Transport.ValidatePOlicyConfigurationScope

mtrmac · 2017-08-01T15:12:06Z

oci/archive/oci_transport.go

+			logrus.Error("error creating oci reference", err)
+		}
+	}
+	return ref.ociRef.StringWithinTransport()


This would return a path to the temporary directory, not to the user-specified archive. And like ParseReference, StringWithinTransport should not be modifying the filesystem.

Again, copy&paste or extract into an independent helper.

mtrmac · 2017-08-01T15:12:34Z

oci/archive/oci_transport.go

+			logrus.Error("error creating oci reference", err)
+		}
+	}
+	return ref.ociRef.PolicyConfigurationIdentity()


Should not return a path to the temporary dir — and should not be modifying the FS here.

mtrmac · 2017-08-01T15:12:53Z

oci/archive/oci_transport.go

+			logrus.Error("error creating oci reference", err)
+		}
+	}
+	return ref.ociRef.PolicyConfigurationNamespaces()


Should not return a path to the temporary dir — and should not be modifying the FS here.

mtrmac · 2017-08-01T15:14:47Z

oci/archive/oci_transport.go

+			logrus.Error("error creating oci reference", err)
+		}
+	}
+	return ref.ociRef.DeleteImage(ctx)


This would delete the just-created temporary directory, not the archive file.

Luckily layout.*.DeleteImage just fails; this should probably as well. Really deleting a single reference means deleting that entry in the index, and perhaps the referenced manifest and blob, it should not delete the whole archive.

umohnani8 · 2017-08-01T17:56:19Z

@mtrmac PTAL

rhatdan · 2017-08-01T19:24:42Z

oci/archive/oci_transport.go

+
+// ociArchiveReference is an ImageReference for OCI Archive paths
+type ociArchiveReference struct {
+	file         string


Do we still need both? file and resolved file?

yup, ref.file is being used in StringWithinTransport

umohnani8 · 2017-08-02T18:00:54Z

#318 needs to be merged in first

umohnani8 · 2017-08-07T14:48:40Z

@mtrmac PTAL.

mtrmac

Apart from the global variable, only minor or stylistic comments I think.

mtrmac · 2017-08-12T01:13:45Z

oci/archive/oci_src.go

+	tempDir     string
+}
+
+var tempDirRef *tempDirOciRef


This can’t be a global variable; multiple ociArchiveImageSource instances can exist at the same time. The necessary data can probably live inside the ociArchiveImage$something, or maybe just as local variables.

Okay, I got rid of the global variable and added this to the ociArchiveReference struct. However, I ran into an issue that I have a temporary fix for.

LoadManifestDescriptor needs to be able to access the image name from the index.json for the kpod load command. Since LoadManifestDescriptor is called before newImageSource for kpod load, I tried to check if tempDirRef has a value in the instance of ociArchiveReference and setting it in one of the functions. However, two instances were being created and ociArchiveReference wouldn't retain its value.

I can't seem to figure out why the instance of ociArchiveReference is not the same - could be that LoadManifestDescriptor is being called from libkpod/copy.go. I have a temporary fix in place that just creates a temp directory and deletes it in LoadManifestDescriptor.

@mtrmac I know this might not be the ideal way, but just wanted your feedback and advice on this issue.

However, two instances were being created and ociArchiveReference wouldn't retain its value.

Yeah,ociArchiveReference is defined to be a value of that struct type, i.e. every value of type ociArchiveReference is an independent copy of the value, not a reference to a shared underlying object. I.e. every method with a receiver of (ref ociArchiveReference) gets a fresh copy, and every parameter with type ociArchiveReference (notably the ref parameters to newImage{Source,Destination}) gets a copy. You are probably seeing the copies made during the newImage{Source,Destination} parameter passing.

In general, ImageReference is mostly being treated as an immutable-after-construction object, so storing state in there is a bit tricky; though at least caching information in it for speed up (without changing semantics) should be fine. The storage backend uses references that way.

OTOH attaching the lifetime of the temporary directories to ImageReference is, with the current structure, problematic, because multiple ImageSource/ImageDestination objects could be created from the same ImageReference over time (or even concurrently!), and there is no good place to delete the temporary directory: Image{Source,Destination}.Close would delete it after the first user is done, possibly breaking all other users, and there is no ImageReference.Close.

So, yeah, extracting the archive in LoadManifestDescriptor, and tearing down the extracted archive immediately in that function, and then extracting it again in NewImageSource, is the best fit with the interface structure. Arguably that’s just evidence of a badly designed interface which can’t deal with this reasonable use case… but the cost of making the LoadManifestDescriptor+NewImageSource sequence performant (forcing every user of ImageReference to treat them as stateful objects and to call ref.Close eventually, and dealing with multiple users and locking and reference counts in the ImageReference implementations) is fairly high.

Overall having LoadManifest extract the archive one more time seems like a reasonable compromise. Or, perhaps better, it could just seek inside the archive to get the manifest file, like tarfile.Source.readTarComponent does, without extracting gigabytes of data onto the disk; with an uncompressed archive this would be quite cheap. With a compressed archive we might still have to decompress gigabytes of data, but at least we wouldn’t have to write it to disk.

Perhaps we could also make a special-case exception, by documenting that a reference passed to LoadManifestDescriptor must be always, and exactly once, passed to ref.NewImageSource(), and implementing the pair of these calls as a special case, without dealing with the general case of multiple, possibly concurrent, callers to ref.NewImage{Source,Destination}. That would be quite ugly, but ugly only locally, without impacting the rest of the code, and high-performing. I guess I’d start with the extracting-twice solution (or streaming-and-extracting solution) first, and defer implementing this special case only if the performance improvement turned out to be necessary.

mtrmac · 2017-08-12T01:16:25Z

oci/archive/oci_dest.go

+
+// newImageDestination returns an ImageDestination for writing to an existing directory.
+func newImageDestination(ref ociArchiveReference, ctx *types.SystemContext) (types.ImageDestination, error) {
+	tempDirOciRef, err := createOciRef(ref.image)


(This creates an one-time named structure, unpacks it in the same function, and then throws it away. Perhaps createOciRef could just return a tuple of three values (reference, path, error) instead of bothering to define and return a structure.

OTOH structures with named members are more explicit than tuples with positional results, perhaps it is worth it. Or if deleteTempDir became a method on the structure.)

this doesn't happen anymore. Fixed

mtrmac · 2017-08-12T01:23:34Z

oci/archive/oci_dest.go

+func (d *ociArchiveImageDestination) SupportsSignatures() error {
+	err := d.unpackedDest.SupportsSignatures()
+	if err != nil {
+		if err := deleteTempDir(); err != nil {


SupportsSignatures returning an error is not a critical failure, no need to clean stuff up just because SupportsSignatures returns an error. Besides, Close will do the cleanup anyway (the caller is expected to ensure that Close is eventually called).

mtrmac · 2017-08-12T01:24:04Z

oci/archive/oci_dest.go

+func (d *ociArchiveImageDestination) PutBlob(stream io.Reader, inputInfo types.BlobInfo) (types.BlobInfo, error) {
+	valBlobInfo, err := d.unpackedDest.PutBlob(stream, inputInfo)
+	if err != nil {
+		if err := deleteTempDir(); err != nil {


Same here, the cleanup shouldn’t be necessary. And in other places in this file.

mtrmac · 2017-08-12T01:30:23Z

oci/archive/oci_dest.go

+	src := d.tempDir
+	// path to save tarred up file
+	dst := d.ref.resolvedFile
+	if err := tar(src, dst); err != nil {


(This works just fine; I can’t shake a feeling that maybe tar could be renamed to something that makes the semantics of its arguments explicit just by reading the call, and then the src/dst assignments and the comments could be unnecessary. But I can’t think of a good alternative name for tar; tarDirectoryTo feels awkward and insufficient. Perhaps someone smarter than me can find a better name; if not, this is quite readable as is.)

Renamed to tarDirectory - makes more sense I guess.

mtrmac · 2017-08-12T01:48:56Z

oci/archive/oci_transport.go

+	ociRef        types.ImageReference
+}
+
+// createOciRef creates the oci reference of the image


This should say layout / extracted or something like that; ociArchiveReference already is an “OCI” reference, that’s not quite the point.

mtrmac · 2017-08-12T01:49:39Z

oci/archive/oci_transport.go

+	// dir is a temporary directory used to generate the OCI image
+	dir, err := ioutil.TempDir("/var/tmp", "oci")
+	if err != nil {
+		return nil, errors.Errorf("error creating file %v", err)


Nit: s/file/directory/, and either there should be a colon before %v, or use errors.Wrapf

mtrmac · 2017-08-12T01:50:59Z

oci/archive/oci_transport.go

+
+// createOciRef creates the oci reference of the image
+func createOciRef(image string) (*tempDirOciRef, error) {
+	// dir is a temporary directory used to generate the OCI image


Generate… or untar and read. Perhaps it’s simpler to delete the comment than to make it exactly correct :)

mtrmac · 2017-08-12T01:51:43Z

transports/alltransports/alltransports.go

@@ -10,6 +10,7 @@ import (
 	_ "github.com/containers/image/docker"
 	_ "github.com/containers/image/docker/archive"
 	_ "github.com/containers/image/docker/daemon"
+	_ "github.com/containers/image/oci/archive"


Can this have a smoke test with the others?

working on the tests

… this is also outstanding, not blocking I guess.

I added tests for oci_transport, are there any other tests that I am missing?

transports/alltransports/alltransports_test.go:TestImageNameHandling

added the test. @mtrmac PTAL

mtrmac · 2017-08-12T01:54:22Z

oci/archive/oci_dest.go

+
+// newImageDestination returns an ImageDestination for writing to an existing directory.
+func newImageDestination(ref ociArchiveReference, ctx *types.SystemContext) (types.ImageDestination, error) {
+	tempDirOciRef, err := createOciRef(ref.image)


Throughout: For no really good reason, it seems that the Go convention is to use all lowercase for an acronym at the start of an identifier, but all uppercase elsewhere. So ociArchiveReference, but createOCIRef.

mtrmac · 2017-08-26T15:46:00Z

oci/archive/oci_transport.go

+
+// deletes the temporary directory created
+func (t *tempDirOciRef) deleteTempDir() error {
+	if t != nil {


Can t ever be nil here? The check doesn’t really hurt of course, but we don’t have every single method of every single type protecting against the receiver being nil.

Right, it can't really ever be nil when that function is called. Took the check out - fixed.

mtrmac · 2017-08-26T17:23:54Z

oci/archive/oci_dest.go

+
+// SupportsSignatures returns an error (to be displayed to the user) if the destination certainly can't store signatures
+func (d *ociArchiveImageDestination) SupportsSignatures() error {
+	return nil


This should also forward to d.unpackedDest (which does not return nil!)

@mtrmac are you saying this should return d.unpackedDest also?

return d.unpackedDest.SupportsSignatures()

nvm, I think I got what you meant. Fixed.

mtrmac · 2017-08-26T17:35:15Z

oci/archive/oci_src.go

+
+	descriptor, err := ocilayout.LoadManifestDescriptor(ociArchRef.tempDirRef.ociRefExtracted)
+	if err != nil {
+		if err := ociArchRef.tempDirRef.deleteTempDir(); err != nil {


There already is a defer ….deleteTempDir() above; and deleting the same name from a shared */tmp directory twice is not, strictly speaking, safe (the name could have been reused by an unrelated process in the meantime).

If the point is to not ignore the error, that can be done in the defer block, e.g. see how copy.Image does that—but only if it is really important to preserve the cleanup error.

In this particular case it seems that the error loading index error is more important than the error deleting temporary directory, at least the error loading index one shouldn't be thrown away; though notifying the user that the cleanup failed might also be useful, it is less important. (See also the discussion around #236 (comment) if you care.)

Since the error loading index error is more important, I didn't do what copy.Image does. Just took out the second deleteTempDir() and left it as a defer. Fixed.

mtrmac · 2017-08-26T17:41:26Z

oci/layout/oci_dest.go

@@ -177,8 +177,12 @@ func (d *ociImageDestination) PutManifest(m []byte) error {
 		return err
 	}

+	if d.ref.image == "" {
+		return errors.Errorf("cannot save image with empyt image.ref.name")


(ignoring the oci/layout parts, to be reviewed in #318 ).

mtrmac · 2017-08-26T17:58:26Z

oci/archive/oci_src.go

+	tempDir     string
+}
+
+var tempDirRef *tempDirOciRef


However, two instances were being created and ociArchiveReference wouldn't retain its value.

Yeah,ociArchiveReference is defined to be a value of that struct type, i.e. every value of type ociArchiveReference is an independent copy of the value, not a reference to a shared underlying object. I.e. every method with a receiver of (ref ociArchiveReference) gets a fresh copy, and every parameter with type ociArchiveReference (notably the ref parameters to newImage{Source,Destination}) gets a copy. You are probably seeing the copies made during the newImage{Source,Destination} parameter passing.

In general, ImageReference is mostly being treated as an immutable-after-construction object, so storing state in there is a bit tricky; though at least caching information in it for speed up (without changing semantics) should be fine. The storage backend uses references that way.

OTOH attaching the lifetime of the temporary directories to ImageReference is, with the current structure, problematic, because multiple ImageSource/ImageDestination objects could be created from the same ImageReference over time (or even concurrently!), and there is no good place to delete the temporary directory: Image{Source,Destination}.Close would delete it after the first user is done, possibly breaking all other users, and there is no ImageReference.Close.

So, yeah, extracting the archive in LoadManifestDescriptor, and tearing down the extracted archive immediately in that function, and then extracting it again in NewImageSource, is the best fit with the interface structure. Arguably that’s just evidence of a badly designed interface which can’t deal with this reasonable use case… but the cost of making the LoadManifestDescriptor+NewImageSource sequence performant (forcing every user of ImageReference to treat them as stateful objects and to call ref.Close eventually, and dealing with multiple users and locking and reference counts in the ImageReference implementations) is fairly high.

Overall having LoadManifest extract the archive one more time seems like a reasonable compromise. Or, perhaps better, it could just seek inside the archive to get the manifest file, like tarfile.Source.readTarComponent does, without extracting gigabytes of data onto the disk; with an uncompressed archive this would be quite cheap. With a compressed archive we might still have to decompress gigabytes of data, but at least we wouldn’t have to write it to disk.

Perhaps we could also make a special-case exception, by documenting that a reference passed to LoadManifestDescriptor must be always, and exactly once, passed to ref.NewImageSource(), and implementing the pair of these calls as a special case, without dealing with the general case of multiple, possibly concurrent, callers to ref.NewImage{Source,Destination}. That would be quite ugly, but ugly only locally, without impacting the rest of the code, and high-performing. I guess I’d start with the extracting-twice solution (or streaming-and-extracting solution) first, and defer implementing this special case only if the performance improvement turned out to be necessary.

mtrmac

This looks pretty good. Outstanding:

The reference naming etc. + tests, need to be reviewed in Fix oci to save the full image name in index.json #318 first.
Lifetime of the temporary directories. I agree with the simple solution of having LoadManifestDescriptor extract the archive again; in that case, the temporary directories should probably be attached to the ociArchive{Source,Destination} objects, not ociArchiveReference EDIT, which should be more or less immutable.

umohnani8 · 2017-08-30T15:25:03Z

@mtrmac are you saying that the tempDirOciRef struct should be attached to ociArchiveSource and ociArchiveDestination instead of ociArchiveReference?
If that is the case LoadManifestDescriptor takes in a types.ImageReference, which can be typecasted to ociArchivereference, so it made sense to me to attach it to ociArchiveReference.

mtrmac · 2017-08-30T17:37:41Z

@mtrmac are you saying that the tempDirOciRef struct should be attached to ociArchiveSource and ociArchiveDestination instead of ociArchiveReference?

Yes, probably—if we can live with the performance impact.

If that is the case LoadManifestDescriptor takes in a types.ImageReference, which can be typecasted to ociArchivereference, so it made sense to me to attach it to ociArchiveReference.

Yes, and extracting the archive only once would be nice; but types.ImageReference has no lifetime or sharing semantics, and the deletion of directories happens in Image{Source,Destination}.Close, so the creation should happen in the same object or something with a closely related lifetime.

Consider

ref := archive.NewReference(…)
src1 := ref.NewImageSource()  // Extracts temporary directory, sets ref.tempDirRef
src2 := ref.NewImageSource() // Does this reuse the directory from src1, or create a new one?
// Either way, src1 and src2 now both use ref.tempDirRef
… // use src1, src2
src1.Close() // This deletes ref.tempDirRef; does that break src2’s use of it?
LoadManifestDescriptor(ref) // Similar concerns as newImageSource; does the cleanup break src2 again?
src2.Close()

Supporting something like this, while sharing the extracted directory, would require ref to keep a (thread-safe?) reference count, and probably a ImageReference.Close to clean up, and be somewhat resilient against concurrent uses of ImageSource/ImageDestination on the same ref. It’s not impossible but it’s a fair amount of work.

Also, if you look at what the current code actually does, createOCIRef creates a new temporary directory on each call, and createUntarTempDir extracts the archive on each call; neither is reused across uses of ref. So, the code is already effectively giving a separate temporary directory to each Image{Source,Destination} instance; storing it in those instances directly instead of the shared ImageReference object would be both simpler and eliminate the problems with .Close() deleting a directory used by another user of the reference.

rhatdan · 2017-08-30T18:42:44Z

Lets go with the slow mode for now, and then if we find it to be a performace issue, we can revisit this in the future.

Fix oci to save the full image name "image:tag" instead of just the tag in index.json. Add function to retrieve the image name from index.json when loading or pulling image from oci directory. Signed-off-by: umohnani8 <umohnani@redhat.com>

umohnani8 · 2017-08-31T15:25:31Z

@mtrmac attached the tempDirOCIRef struct to ociArchiveImageSource and ociArchiveImageDestination. PTAL

mtrmac

LGTM, one very last nit.

(Thanks for keeping this up-to-date with #318!)

mtrmac · 2017-08-31T18:04:37Z

oci/archive/oci_transport.go

+// NewImageDestination returns a types.ImageDestination for this reference.
+// The caller must call .Close() on the returned ImageDestination.
+func (ref ociArchiveReference) NewImageDestination(ctx *types.SystemContext) (types.ImageDestination, error) {
+	return newImageDestination(ref, ctx)


Please keep the ctx parameter in the first position in newImage{Source,Destination}, to be consistent with other users.

runcom · 2017-08-31T18:21:20Z

LGTM

Signed-off-by: umohnani8 <umohnani@redhat.com>

mtrmac · 2017-08-31T18:30:49Z

👍

Thanks!

rhatdan reviewed Jul 27, 2017

View reviewed changes

mtrmac reviewed Jul 27, 2017

View reviewed changes

umohnani8 force-pushed the oci_archive branch 2 times, most recently from bf4293e to 5c221e4 Compare July 28, 2017 15:35

umohnani8 force-pushed the oci_archive branch from 5c221e4 to a91676e Compare July 28, 2017 15:42

mtrmac reviewed Jul 28, 2017

View reviewed changes

umohnani8 force-pushed the oci_archive branch from a91676e to d784a40 Compare July 31, 2017 18:58

umohnani8 force-pushed the oci_archive branch from d784a40 to c27f600 Compare August 1, 2017 14:39

mtrmac reviewed Aug 1, 2017

View reviewed changes

umohnani8 force-pushed the oci_archive branch from c27f600 to 75ffb7d Compare August 1, 2017 17:54

umohnani8 force-pushed the oci_archive branch from 75ffb7d to 8383061 Compare August 1, 2017 18:48

rhatdan reviewed Aug 1, 2017

View reviewed changes

umohnani8 force-pushed the oci_archive branch 2 times, most recently from ef4233b to df43627 Compare August 2, 2017 17:59

umohnani8 changed the title ~~Add oci-archive transport that creates a tar archive of an image~~ [WIP]Add oci-archive transport that creates a tar archive of an image Aug 2, 2017

umohnani8 force-pushed the oci_archive branch 4 times, most recently from fa8e87a to b0fc85b Compare August 7, 2017 14:48

umohnani8 force-pushed the oci_archive branch 2 times, most recently from bab93fc to ca1b94a Compare August 8, 2017 14:42

mtrmac reviewed Aug 12, 2017

View reviewed changes

umohnani8 force-pushed the oci_archive branch 2 times, most recently from d13e472 to a464523 Compare August 14, 2017 16:42

umohnani8 force-pushed the oci_archive branch from a464523 to ded861f Compare August 24, 2017 17:16

mtrmac reviewed Aug 26, 2017

View reviewed changes

umohnani8 force-pushed the oci_archive branch from ded861f to a3a0136 Compare August 28, 2017 13:19

umohnani8 force-pushed the oci_archive branch from a3a0136 to a7c676c Compare August 30, 2017 15:25

Fix oci to save the full image name

8789ec5

Fix oci to save the full image name "image:tag" instead of just the tag in index.json. Add function to retrieve the image name from index.json when loading or pulling image from oci directory. Signed-off-by: umohnani8 <umohnani@redhat.com>

umohnani8 force-pushed the oci_archive branch 2 times, most recently from 215c7d0 to 02be3b2 Compare August 31, 2017 15:23

umohnani8 changed the title ~~[WIP]Add oci-archive transport that creates a tar archive of an image~~ Add oci-archive transport that creates a tar archive of an image Aug 31, 2017

umohnani8 force-pushed the oci_archive branch from 02be3b2 to c905291 Compare August 31, 2017 17:20

mtrmac reviewed Aug 31, 2017

View reviewed changes

umohnani8 force-pushed the oci_archive branch from c905291 to ec4daa7 Compare August 31, 2017 18:11

Add oci-archive transport that creates a tar archive of an image

1bac672

Signed-off-by: umohnani8 <umohnani@redhat.com>

umohnani8 force-pushed the oci_archive branch from ec4daa7 to 1bac672 Compare August 31, 2017 18:27

mtrmac merged commit 960e725 into containers:master Aug 31, 2017

mtrmac mentioned this pull request Sep 4, 2017

Support pulling/pushing OCI manifests from a docker registry #338

Merged

Add oci-archive transport that creates a tar archive of an image #314

Add oci-archive transport that creates a tar archive of an image #314

Conversation

umohnani8 commented Jul 27, 2017

umohnani8 commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umohnani8 commented Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umohnani8 commented Jul 31, 2017

umohnani8 commented Aug 1, 2017

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umohnani8 commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umohnani8 commented Aug 2, 2017

umohnani8 commented Aug 7, 2017

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umohnani8 commented Jul 28, 2017 •

edited

Loading