docker-archive generates docker legacy compatible images #370

nlewo · 2017-10-27T10:16:00Z

docker save generates image compatible with the legacy format, ie,
layers are tar, they have a configuration file and a repositories file
is created.

There are some external tools that still relie on this old format such
as mesos [1] and nixos [2].

[1] https://github.com/apache/mesos/blob//7ca46e24b3339ba27e88b99ea95362c956ef03c1/src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp#L168
[2] https://github.com/NixOS/nixpkgs/blob/5c6dc717a66b6555e5757f92376527da7a553fec/pkgs/build-support/docker/default.nix#L143

nlewo · 2017-10-27T10:19:35Z

This is related to this issue containers/skopeo#425.
I only tested this patch on images coming from docker-archive and docker sources.

nlewo · 2017-10-29T09:05:50Z

I'm not able to get the error message from the CI, and make locally fails :

INFO[0114] Fetching latest commits from 'origin' for 'github.com/gogo/protobuf/proto' 
ERRO[0114] `git fetch -f -t origin` failed:
fatal: 'origin' does not appear to be a git repository
fatal: Could not read from remote repository.

Any ideas?

mtrmac · 2017-10-31T17:21:37Z

The CI failure is

docker/tarfile/dest.go
Makefile:62: recipe for target 'validate' failed

Probably reformat the file using gofmt -s -w docker/tarfile/dest.go. Yeah, the Makefile rule for gofmt could be improved to be clearer.

As for the local make failure checking out a vendored subpackage, I don’t know. Removing the vendor subdirectory entirely and running make vendor works for me.

nlewo · 2017-11-03T23:26:20Z

Thanks @mtrmac
Regarding my local issue with make, I submitted PR #373. I also fixed several test errors in this patch but I still have an test error with Skopeo.
The error is related to dir and docker source/dest which should not be impacted by this patch:

Running skopeo --debug copy docker://busybox dir:/tmp/copy-1238023601
Running skopeo --tls-verify=false --debug copy dir:/tmp/copy-1238023601 docker://localhost:5555/busybox:unsigned
Running skopeo --tls-verify=false --debug copy docker://localhost:5555/busybox:unsigned dir:/tmp/copy-2608739164
Running diff -urN /tmp/copy-1238023601 /tmp/copy-2608739164
# the diff is not nil while it should be

Moreover, I cannot reproduce it when I use the docker registry.

Is there a trick with the test registry? (maybe it is using tarfile)
How could I get the debug messages of Skopeo in the test output?

nlewo · 2017-11-04T08:28:47Z

Ok, I've finally fixed tests

mtrmac

I’m sorry, afraid I didn’t have time to review this in detail yet (the core changes in docker/tarfile in particular), here are a few mostly API-focused notes for now.

mtrmac · 2017-11-06T16:40:24Z

copy/copy.go

+	if isCompressed && ic.dest.ShouldDecompressLayers() {
+		logrus.Debugf("Blob will be decompressed")
+		destStream, err = decompressor(destStream)
+		if err != nil {


There is already a decompression code around getOriginalLayerCopyWriter. If at all possible, we should only decompress the data once and share the output between the two consumers, if they exist at the same time.

@mtrmac getOriginalLayerCopyWriter is only used when the format of the source image is Docker SchemaV1. So, we could have two decompression if the source format image is Docker SchemaV1 and the destination is docker-archive.
But I think it's not trivial to change this behavior since getOriginalLayerCopyWriter is already a little bit tricky...
Do you know if SchemaV1 is still widely used?

schema1 is still around (and old images will keep using it), but hopefully its will diminish over time… just like using the repositories archive format :)

I really don’t like decompressing the possibly gigabytes of data twice, but you’re right that the code is tricky and I don’t quite have the extra time necessary to get this right, so I guess let’s live with the two separate decompressions.

mtrmac · 2017-11-06T16:42:10Z

docker/tarfile/dest.go

+			if err != nil {
+				return types.BlobInfo{}, err
+			}
+			inputInfo.Digest = digester.Digest()


It would probably be cheaper to either compute the digest always, or use io.TeeReader/io.MultiWriter, so that we make as few passes over the data as possible.

mtrmac · 2017-11-06T16:46:03Z

types/types.go

 type BlobInfo struct {
 	Digest      digest.Digest // "" if unknown.
 	Size        int64         // -1 if unknown
+	Config      bool          // true if the blob is a config blob


This is not very elegant... conceptually, the information is already available in MediaType, and more importantly if Config:true is only set by Image.ConfigInfo(), the caller structurally knows that the blob is a config without having to ask.

Maybe we should have separate ImageDestination.{PutLayer,PutBlob}, or at the very least least move this Config bool into a separate parameter of PutBlob which can be set by the caller directly instead of relying on ConfigInfo. (@runcom WDYT?)

…separate imageDestination.{PutLayer,PutConfig} that is.

If this is the only use case that needs the distinction between layers and the config, I think it would be less intrusive to add a bool paramater to the putBlob method.
Do you think there are other use cases that need this distinction?

mtrmac · 2017-11-06T16:46:51Z

types/types.go

 	// ShouldCompressLayers returns true iff it is desirable to compress layer blobs written to this destination.
 	ShouldCompressLayers() bool
+	// ShouldDecompressLayers returns true iff it is desirable to decompress layer blobs written to this destination.
+	ShouldDecompressLayers() bool


Please use a single enum (with cases compress/decompress/preserveoriginal) instead of two booleans, to make structurally sure ShouldCompressLayers&&ShouldDecompressLayers can’t be true at the same time.

nlewo · 2017-12-19T13:11:48Z

CI is failing because Skopeo needs to be patched. Someone could still have a look to this PR?

gilligan · 2018-01-11T20:49:32Z

I'm a nixos / docker tooling user and I would very much love to see this merged :)

gilligan · 2018-01-14T18:12:33Z

@nlewo @mtrmac what is missing to move this forward? what can be done to support this?

nlewo · 2018-01-17T08:51:36Z

This PR has never been really reviewed. I have had some comments from @mtrmac but I don't know if this "feature" could be merged.
One month ago, I've rebased it on master and this was not trivial:/ So, now I don't know what to do with this.
But of course, I would be happy to reply to reviews!

mtrmac · 2018-01-17T19:58:29Z

Sorry about that, I was unavailable for most of December and getting to this through my backlog took a while.

Here’s an in-progress review, I’m afraid I have to context switch to something else for the rest of today…

mtrmac · 2018-01-16T15:46:12Z

copy/copy.go

 func (c *copier) copyBlobFromStream(srcStream io.Reader, srcInfo types.BlobInfo,
 	getOriginalLayerCopyWriter func(decompressor compression.DecompressorFunc) io.Writer,
-	canCompress bool) (types.BlobInfo, error) {
+	canCompress bool, isConfig bool) (types.BlobInfo, error) {


The canCompress value comes from canModifyManifest in callers, and applies just as well to decompression; please rename the parameter, perhaps to canModifyBlob, and make it prevent both compression and decompression.

mtrmac · 2018-01-16T17:35:20Z

copy/copy.go


+	// === Decompress the layer if it is compressed and decompression is desired
+	// This is currently only used by docker-archive
+	if isCompressed && c.dest.CompressesLayers() == types.Decompress {


(This if /*complex*/ {} else if {}; if {} sequence feels like something that should be possible to express in a clearer way. I haven’t actually tried yet, though.)

Exactly, the sequence can be rewritten such as:

if compression is desired and it is possible compress() else if decompression is desired and it is possible decompress() else continue

mtrmac · 2018-01-16T17:41:42Z

docker/tarfile/dest.go

-	if inputInfo.Size == -1 { // Ouch, we need to stream the blob into a temporary file just to determine the size.
+	// Ouch, we need to stream the blob into a temporary file just to determine the size.
+	// When the layer is decompressed, we also have to generate the digest on uncompressed datas.
+	if inputInfo.Size == -1 {


Shouldn’t this be inputInfo.Size == -1 || inputInfo.Digest == ""?

I don't think it is possible that inputInfo.Digest == "" && inputInfo.Size != -1 is true but I add a guard in case of.

You’re right that the copy.Image code won’t have inputInfo.Digest == "" && inputInfo.Size != -1, but the documented behavior of the interface does allow it.

The added check below (Can not stream a blob with unknown digest to docker tarfile) is too late; check in this condition, and then at the location of the current a digest ill always be available.

mtrmac · 2018-01-16T17:45:01Z

docker/tarfile/dest.go

 		}
+		if inputInfo.Digest == "" {
+			inputInfo.Digest = digester.Digest()
+		}


It would be more consistent to keep the update of inputInfo.Digest and inputInfo.Size close together.

Not sure to understand wht you mean... but I've tryied.

Yes, this. Thanks.

mtrmac · 2018-01-16T17:47:32Z

docker/tarfile/dest.go

-	if err := d.sendFile(inputInfo.Digest.String(), inputInfo.Size, tee); err != nil {
-		return types.BlobInfo{}, err
+	// When the digest is generated on the uncompressed layer, we
+	// have to recheck if the layer has been already sent.


Why? inputInfo.Digest, if present, must match exactly the contents of the input stream. It should never happen that the stream is uncompressed but the digest value matches a compressed version and needs to be recomputed just to be correct. Is that broken somewhere?

Maybe the HasBlob check at the top should just be moved to this place.

Yes, you are right.

mtrmac · 2018-01-16T17:57:59Z

docker/tarfile/dest.go

+	t := make(map[string]string)
+	t[repoTag[1]] = rootLayerID
+	r := make(map[string]map[string]string)
+	r[repoTag[0]] = t


All of the above can be expressed as

repositories := map[string]map[string]string{ repoTag[0]:{repoTag[1]:rootLayerID} }

which seems much more readable, and does not even need the “This file looks like” comment.

mtrmac · 2018-01-16T17:58:31Z

docker/tarfile/dest.go

 }

+func createRepositoriesFile(d *Destination, rootLayerID string) error {
+	// We generate the repositories file


The function name already says that, no need to repeat it here. Or, if you want to, move it one line up and make it a proper Golang doc string.

mtrmac · 2018-01-16T18:00:10Z

docker/tarfile/dest.go

+func createRepositoriesFile(d *Destination, rootLayerID string) error {
+	// We generate the repositories file
+	// This file looks like '{repo : { tag : rootLayerSha }}'
+	repoTag := strings.Split(d.repoTag, ":")


NewDestination already works with the name and tag as separate strings extracted from a well-typed reference.NamedTagged; it is awkward to concatenate them to a string and then parse them out of the string again.

mtrmac · 2018-01-16T18:01:10Z

docker/tarfile/types.go

 	// legacyConfigFileName       = "json"
-	// legacyVersionFileName      = "VERSION"
+	legacyVersionFileName = "VERSION"
 	// legacyRepositoriesFileName = "repositories"


Either use the legacyRepositoriesFileName and legacyLayerFileName constants, or remove them from here if you want the code to hard-code the names directly.

I use these constants.

mtrmac · 2018-01-16T18:04:00Z

docker/tarfile/dest.go

+		// The legacy format requires a config file per layer
+		layerConfig := make(map[string]*json.RawMessage)
+		id := l.Digest.Hex()
+		idJSON := json.RawMessage("\"" + id + "\"")


This is awkward and potentially unsafe. Couldn't layerConfig be a map[string]interface{}, assigning some string and some *json.RawMesage values to individual entries of the map? Or actually a typed structure?

(It’s not yet clear to me whether all of this should share code with c/I/image/v1ConfigFromConfigJSON.)

I agree it's not clean but the objective is just to extract some parts of the image configuration file and use them to create layer configuration files, without really know what is inside these parts. Since contents of these parts don't seem to be relevant, I think it's not necessary to create a complex data structure just to copy/paste these JSON blobs.
I now use a map[string]interface{} where the extracted attributes are *json.RawMesage while attributes that are created are string (id and parent).

nlewo

Thanks for your comments @mtrmac.
Note, I didn't run your validation targets yet.

nlewo · 2018-01-18T17:01:36Z

docker/tarfile/dest.go

 		}
+		if inputInfo.Digest == "" {
+			inputInfo.Digest = digester.Digest()
+		}


Not sure to understand wht you mean... but I've tryied.

nlewo · 2018-01-18T17:27:58Z

docker/tarfile/dest.go

-	if inputInfo.Size == -1 { // Ouch, we need to stream the blob into a temporary file just to determine the size.
+	// Ouch, we need to stream the blob into a temporary file just to determine the size.
+	// When the layer is decompressed, we also have to generate the digest on uncompressed datas.
+	if inputInfo.Size == -1 {


I don't think it is possible that inputInfo.Digest == "" && inputInfo.Size != -1 is true but I add a guard in case of.

nlewo · 2018-01-18T17:38:58Z

docker/tarfile/dest.go

-	if err := d.sendFile(inputInfo.Digest.String(), inputInfo.Size, tee); err != nil {
-		return types.BlobInfo{}, err
+	// When the digest is generated on the uncompressed layer, we
+	// have to recheck if the layer has been already sent.


Yes, you are right.

nlewo · 2018-01-18T17:57:56Z

docker/tarfile/dest.go

+	if isConfig {
+		buf := new(bytes.Buffer)
+		buf.ReadFrom(stream)
+		d.config = buf


Ok, Thanks.

nlewo · 2018-01-18T18:17:18Z

docker/tarfile/dest.go

+func createRepositoriesFile(d *Destination, rootLayerID string) error {
+	// We generate the repositories file
+	// This file looks like '{repo : { tag : rootLayerSha }}'
+	repoTag := strings.Split(d.repoTag, ":")


nlewo · 2018-01-18T18:17:25Z

docker/tarfile/dest.go

+	t := make(map[string]string)
+	t[repoTag[1]] = rootLayerID
+	r := make(map[string]map[string]string)
+	r[repoTag[0]] = t


nlewo · 2018-01-18T19:48:21Z

docker/tarfile/dest.go

+	for i, l := range man.LayersDescriptors {
+		layerPaths = append(layerPaths, l.Digest.Hex()+"/layer.tar")
+		b := []byte("1.0")
+		if err := d.sendFile(filepath.Join(l.Digest.Hex(), legacyVersionFileName), int64(len(b)), bytes.NewReader(b)); err != nil {


nlewo · 2018-01-18T20:51:45Z

docker/tarfile/dest.go

+		// The legacy format requires a config file per layer
+		layerConfig := make(map[string]*json.RawMessage)
+		id := l.Digest.Hex()
+		idJSON := json.RawMessage("\"" + id + "\"")


I agree it's not clean but the objective is just to extract some parts of the image configuration file and use them to create layer configuration files, without really know what is inside these parts. Since contents of these parts don't seem to be relevant, I think it's not necessary to create a complex data structure just to copy/paste these JSON blobs.
I now use a map[string]interface{} where the extracted attributes are *json.RawMesage while attributes that are created are string (id and parent).

nlewo · 2018-01-18T20:56:39Z

docker/tarfile/dest.go

+		}
+	}
+
+	if err := createRepositoriesFile(d, man.LayersDescriptors[len(man.LayersDescriptors)-1].Digest.Hex()); err != nil {


I add a guard to only create the repostorie file is there is at least one layer.

nlewo · 2018-01-18T21:04:12Z

docker/tarfile/types.go

 	// legacyConfigFileName       = "json"
-	// legacyVersionFileName      = "VERSION"
+	legacyVersionFileName = "VERSION"
 	// legacyRepositoriesFileName = "repositories"


I use these constants.

mtrmac · 2018-01-19T22:59:44Z

copy/copy.go

+	if canModifyBlob && c.dest.CompressesLayers() == types.Compress && !isCompressed {
 		logrus.Debugf("Compressing blob on the fly")
 		pipeReader, pipeWriter := io.Pipe()
+


Nit: please drop this line, to keep the defer immediately after creating the object.

mtrmac · 2018-01-19T23:02:20Z

directory/directory_test.go

-	compress := dest.ShouldCompressLayers()
-	assert.False(t, compress)
-	info, err := dest.PutBlob(bytes.NewReader(blob), types.BlobInfo{Digest: digest.Digest("sha256:digest-test"), Size: int64(9)})
+	assert.Equal(t, dest.CompressesLayers(), types.PreserveOriginal)


The order of arguments should be assert.Equal(t, expectedValue, valueBeingTested)

mtrmac · 2018-01-19T23:04:36Z

docker/tarfile/dest.go

 	// a hostname-qualified reference.
 	// See https://github.com/containers/image/issues/72 for a more detailed
 	// analysis and explanation.
-	refString := fmt.Sprintf("%s:%s", ref.Name(), ref.Tag())


Please move the big comment above this line to PutManifest as well, along with the refString variable.

mtrmac · 2018-01-19T23:06:40Z

docker/tarfile/dest.go

-	tee := io.TeeReader(stream, digester.Hash())
-	if err := d.sendFile(inputInfo.Digest.String(), inputInfo.Size, tee); err != nil {
-		return types.BlobInfo{}, err
+	if inputInfo.Digest.String() == "" {


This should always be false at this point, at least after the previous if is updated (and the code below already depends on inputInfo.Digest being valid).

Not sure I'm fixing it... Let me know if you meant something else.

Right, at this point the code can unconditionally assume inputInfo.Digest is valid.

The start of PutBlob could, I think, be

func (d *Destination) PutBlob(stream io.Reader, inputInfo types.BlobInfo, isConfig bool) (types.BlobInfo, error) { // Ouch, we need to stream the blob into a temporary file just to determine the size or digest. if inputInfo.Size == -1 || inputInfo.Digest.String() == "" { logrus.Debugf("docker tarfile: input with unknown size or digest, streaming to disk first ...")

and transparently handle the hypothetical case of known size && unknown digest instead of failing.

mtrmac · 2018-01-19T23:08:50Z

docker/tarfile/dest.go

+		if err := d.sendFile(filepath.Join(inputInfo.Digest.Hex(), "layer.tar"), inputInfo.Size, stream); err != nil {
+			return types.BlobInfo{}, err
+		}
+		d.blobs[inputInfo.Digest] = types.BlobInfo{Digest: inputInfo.Digest, Size: inputInfo.Size}


d.blobs can be updated in the isConfig case as well. HasBlob is, strictly speaking, not restricted to layer blobs.

ah yes, exactly!

So, why not move the d.blobs[inputInfo.Digest = … line one line lower, out of the if?

mtrmac · 2018-01-19T23:20:30Z

types/types.go

-	// ShouldCompressLayers returns true iff it is desirable to compress layer blobs written to this destination.
-	ShouldCompressLayers() bool
+	// CompressesLayers is used to know what kind of compression should be applied on layers
+	CompressesLayers() LayerCompression


CompressesLayers, e.g. when compared with SupportsSignatures above it, reads as if it answers the question ”does the destination compress layers”, which is not the case. Name it perhaps something like DesiredLayerCompression?

mtrmac · 2018-01-19T23:23:18Z

copy/copy.go

 		originalLayerReader = destStream
 	}

 	// === Compress the layer if it is uncompressed and compression is desired


This comment now does not apply to the whole section, it is more of an alternative to the new two // == indented comments instead of a higher-level description to the two.

The original idea was that the // == comments delineate separate stages of the pipeline; so, maybe rewrite this one as something like // == Deal with layer compression/decompression if necessary,
and drop the two indented // == ones.

mtrmac · 2018-01-19T23:51:42Z

docker/tarfile/dest.go

+
+		// All layers expect the root one have a parent
+		if i != 0 {
+			layerConfig["parent"] = man.LayersDescriptors[i-1].Digest.Hex()


An image may in principle use exactly the same layers several times, in a pathological case even in sequence; in that case this would create multiple versions of files with the same name in a single tar archive, and a loop in the “parent” links with some of the versions.

Instead, the IDs used in here need to be distinct even if the same l.Digest.Hex() value is repeated, see ChainID in https://gist.github.com/aaronlehmann/b42a2eaf633fc949f93b or the implementation in moby/moby/image/tarexport.

I think this could be implemented but I don't have any image to test this logic. Do you know how could I produce this kind of image or where I could find one?
cc @gilligan @kuznero

I guess, take any existing image (e.g. skopeo copy docker://whatever dir:tmpdir to have an easy way to edit it), edits its manifest to repeat one item of the layers array, edit its config.json to correspondingly list the layer twice in rootfs.diff_ids, and update the manifest with the updated manifest digest.

(To be honest, this would probably be the first time anyone is actually testing this corner case, so something else might turn out to be broken.)

Thx. I think I successfully built an image such like that. Currently, when I docker-load this image (after copying it with skopeo docker-archive), Docker sees 5 layers while there are 6 layers in the rootfs array.
I will see how we could handle this kind of images.

@mtrmac I've modified an image in order to reuse a layer by using the skopeo transport dir. I've tryied to copy this image to docker-daemon and it prints:

Skipping fetch of repeat blob sha256:6fbc4a21b806838b63b774b338c6ad19d696a9e655f50b4e358cc4006c3baa79

In fact, the same behavior is observed if we use the proposed docker-archive implementation: the duplicated layer is ignored.

To be honest, I don't really know if the generated image reproduces well this use case and I'm not sure what is the expected behavior when this kind of images are loaded by Docker... It would be easier by having a real image.

Moreover, in the Docker implementation, they use the chainID to generate the path of the layer in the tar stream. This is hard for us to do this because the layer is sent to the tar stream by the PutBlob method which is designed to handle each layer independently from other ones: we can not easily use previous layer sha to generate the current layer path.

Finally, it seems this kind of image would probably never been used!
So instead of adding complexity to the code for this corner case, we could print a message that says "Repeated layer is ignored".

Skipping fetch of repeat blob sha256:…

That part is an optional optimization to PutBlob and more or less known to work; I am more worried about the correctness of the generated metadata (in this case, primarily the parent links).

(I need to test this scenario out myself, maybe I am talking nonsense; hopefully tomorrow. For now just this quick note.)

OK, tested this. TL;DR: the legacy metadata in the generated tar file crashes the Docker daemon on docker load, so this really needs to be fixed.

(To be fair, the unmodified tar file which includes the new metadata as well works fine with docker load; but generating of the legacy format is the whole port of this PR, so the legacy format should be correct.)

# Build skopeo with this PR, updating layers.go as necessary $ ./skopeo --override-os linux --policy default-policy.json copy docker://busybox dir:edits $ python -mjson.tool < edits/5b0d59026729b68570d99bc4f3f7c31a2e4f2a5736435641565d93e7c25bd2c3.tar > _ # To duplicate the `rootfs.diff_ids` array entry $ vi _ $ shasum --algorithm 256 _ | cut -d ' ' -f 1 e2f0703c8b534d097a822e895b0852ceedccb83a7a122aa25f92757b0ee1fcc3 $ ls -l _ $ mv _ edits/$(shasum --algorithm 256 _ | cut -d ' ' -f 1).tar # To update the size and digest of the config, and duplicate the layer entry $ vi edits/manifest.json # Test that the result is minimally consistent $ ./skopeo --override-os linux --policy default-policy.json copy dir:edits dir:t $ rm -rf t $ ./skopeo --override-os linux --policy default-policy.json copy dir:edits docker-archive:archive0.tar:busybox:v0 $ mkdir archive0 $ cd archive0/ $ tar xf ../archive0.tar $ ls 4febd3792a1fb2153108b4fa50161c6ee5e3d16aa483a63215f936a113a88e9a manifest.json e2f0703c8b534d097a822e895b0852ceedccb83a7a122aa25f92757b0ee1fcc3.json repositories # The layer refers to itself as a parent! $ python -mjson.tool 4febd3792a1fb2153108b4fa50161c6ee5e3d16aa483a63215f936a113a88e9a/json … "id": "4febd3792a1fb2153108b4fa50161c6ee5e3d16aa483a63215f936a113a88e9a", … "parent": "4febd3792a1fb2153108b4fa50161c6ee5e3d16aa483a63215f936a113a88e9a" … # Make sure the legacy loader is used $ rm manifest.json override r--r--r-- mitr/staff for manifest.json? y $ tar cf ../archive1.tar . $ cd .. # Verify that skopeo can’t use a legacy-only archive $ ./skopeo --policy default-policy.json copy docker-archive:archive1.tar:busybox:v0 dir:t WARN[0000] docker-archive: references are not supported for sources (ignoring) FATA[0000] Error determining manifest MIME type for docker-archive:archive1.tar:docker.io/library/busybox:v0: Error loading tar component manifest.json: file does not exist # Try using the file with docker. This hangs for a while, eventually failing with # docker load < ~mitr/archive1.tar error during connect: Post http://%2Fvar%2Frun%2Fdocker.sock/v1.26/images/load?quiet=0: EOF

and the system logs show the daemon aborting with a stack overflow due to an infinite recursion in github.com/docker/docker/image/tarexport.(*tarexporter).legacyLoadImage().

Moreover, in the Docker implementation, they use the chainID to generate the path of the layer in the tar stream. This is hard for us to do this because the layer is sent to the tar stream by the PutBlob method which is designed to handle each layer independently from other ones: we can not easily use previous layer sha to generate the current layer path.

The metadata is created not in PutBlob, but in writeLegacyLayerMetadata, at which point the full ordered set of layers is available as layerDescriptors.

mtrmac · 2018-01-19T23:56:03Z

docker/tarfile/dest.go

-	return false
+// CompressesLayers indicates if layers must be compressed, decompressed or preserved
+func (d *Destination) CompressesLayers() types.LayerCompression {
+	return types.Decompress


Please move this out of docker/tarfile.Destination into the users, docker/daemon.daemonImageDestination and docker/archive.archiveImageDestination, and return Decompress only in the docker/archive implementation; for docker/daemon, let’s keep the current PreserveOriginal behavior.

mtrmac · 2018-01-20T00:06:06Z

copy/copy.go

+	if isCompressed && ic.dest.ShouldDecompressLayers() {
+		logrus.Debugf("Blob will be decompressed")
+		destStream, err = decompressor(destStream)
+		if err != nil {


schema1 is still around (and old images will keep using it), but hopefully its will diminish over time… just like using the repositories archive format :)

I really don’t like decompressing the possibly gigabytes of data twice, but you’re right that the code is tricky and I don’t quite have the extra time necessary to get this right, so I guess let’s live with the two separate decompressions.

mtrmac

Thanks!

This is a complete review, unlike the previous in-progress/partial ones.

The one critical blocker is the layer ID computation; otherwise various smaller things. From earlier, only the handling of inputInfo.Digest() == "" is outstanding (and the double decompression of schema1 images, though that is non-blocking).

nlewo · 2018-01-25T22:19:31Z

Thanks for your review. I've addressed all of your comments, excepted the one related to the ChainID which is still in discussion.

mtrmac · 2018-01-26T10:21:42Z

docker/tarfile/dest.go

+}
+
+// WriteLegacyLayerMetadata writes legacy VERSION and configuration files for all layers
+func (d *Destination) WriteLegacyLayerMetadata(layerDescriptors []manifest.Schema2Descriptor) (layerPaths []string, err error) {


Nit: please make this method private (lowercase writeLegacy…)

mtrmac · 2018-01-26T10:32:12Z

docker/tarfile/dest.go

-	tee := io.TeeReader(stream, digester.Hash())
-	if err := d.sendFile(inputInfo.Digest.String(), inputInfo.Size, tee); err != nil {
-		return types.BlobInfo{}, err
+	if inputInfo.Digest.String() == "" {


Right, at this point the code can unconditionally assume inputInfo.Digest is valid.

The start of PutBlob could, I think, be

func (d *Destination) PutBlob(stream io.Reader, inputInfo types.BlobInfo, isConfig bool) (types.BlobInfo, error) { // Ouch, we need to stream the blob into a temporary file just to determine the size or digest. if inputInfo.Size == -1 || inputInfo.Digest.String() == "" { logrus.Debugf("docker tarfile: input with unknown size or digest, streaming to disk first ...")

and transparently handle the hypothetical case of known size && unknown digest instead of failing.

mtrmac · 2018-01-26T10:41:21Z

The FAIL - does not have a valid DCO test failures: Please sign your commits (git commit --amend -s)

nlewo · 2018-02-04T11:59:56Z

@gilligan yes, it should work with and without this patch (i'm already using skopeo to push images that have been produced with dockerTools.buildImage).

gilligan · 2018-02-04T13:23:22Z

huh? Last I tried that did not work since skopeo was complaining about a missing manifest file I believe

nlewo · 2018-02-05T08:45:34Z

@gilligan I'm pushing to a registry several images generated by dockerTools.buildImage without any troubles. Can you share your image or provide more details?
I think it's not the right place here to discuss about this. Can you open an issue or ping me on the irc nixos channel (my nick is lewo).

nlewo · 2018-02-08T14:50:05Z

@mtrmac Thanks for your how to "build crazy images" :) When I had triyed, I didn't remove the manifest.json file. This explains why it was working fine for me.

I pushed a commit that avoids the loop by skipping repeated layers. A warning is printed to inform the user.

mtrmac · 2018-02-08T16:08:12Z

I pushed a commit that avoids the loop by skipping repeated layers.

That changes the content of the image; the duplicates matter (see below for an example). Please either compute a ChainID as discussed above, of generate completely new IDs which are unique within the tarball and unrelated to the layer DiffID values.

Actually, there is a far simpler way to produce such an image: Create an empty empty file, and Dockerfile with

FROM fedora

ADD empty /
RUN rm /empty
ADD empty /

This results in the following RootFS.Layers:

                "sha256:d32459d9ce237564fb93573b85cbc707600d43fbe5e46e8eeef22cad914bb516",
                "sha256:4b5c23136932e4a87cecc06f674ff3f66ca21c8a61653b97aea095b5822ded60",
                "sha256:46c039e328386fe951548c428c030f15bf6c764aa3222fedb9d114d2b3270bb8",
                "sha256:4b5c23136932e4a87cecc06f674ff3f66ca21c8a61653b97aea095b5822ded60"

i.e. the two ADD empty / directives cause two exactly identical layers to be created — and merely removing the last one changes the image, from an image which contains /empty to one which doesn’t.

mtrmac · 2018-02-14T21:37:53Z

Would https://github.com/mtrmac/image/tree/docker-archive-chainid (testable via https://github.com/mtrmac/skopeo/tree/docker-archive-chainid ) work for you? At least docker load accepts both the new and old metadata from the generated tarball, with a

FROM fedora

ADD a /
ADD a /
ADD b /
ADD c /
ADD b /
ADD c /
ADD a /

(Note how the layers end up in the root of the tarball again; the legacy paths all become symlinks.)

nlewo · 2018-02-18T19:53:44Z

@mtrmac thanks for your patches and the skopeo branch:)
Your patches work well on my docker images.

nlewo · 2018-02-28T17:07:29Z

@mtrmac I've integrated your patches and rebased this PR on master.
Are there still some blocking points?

mtrmac · 2018-03-03T03:36:26Z

👍 Thanks!

@runcom PTAL

mtrmac · 2018-03-03T03:59:59Z

Failing make test-skopeo in this PR should be fixed through containers/skopeo#480 .

nlewo · 2018-03-13T17:59:23Z

@runcom Could you please have a look?

nlewo · 2018-03-27T07:30:51Z

@mtrmac it would be really nice to get this PR merged since I'm waiting for it to improve our Docker tooling. How could we move forward?

mtrmac · 2018-03-27T11:19:47Z

@runcom ping again, this has been waiting for you for 24 days now.

runcom · 2018-03-27T15:28:56Z

@nlewo this needs one last rebase - I'm reviewing it

docker save generates image compatible with the legacy format, ie, layers are tar, they have a configuration file and a repositories file is created. There are some external tools that still relie on this old format such as mesos [1] and nixos [2]. [1] https://github.com/apache/mesos/blob//7ca46e24b3339ba27e88b99ea95362c956ef03c1/src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp#L168 [2] https://github.com/NixOS/nixpkgs/blob/5c6dc717a66b6555e5757f92376527da7a553fec/pkgs/build-support/docker/default.nix#L143 Signed-off-by: Antoine Eiche <lewo@abesis.fr>

A new layer id is generated by using the current layer id and its parent layer id. Signed-off-by: Antoine Eiche <lewo@abesis.fr>

Duplicate IDs would otherwise result in duplicate files in the tarball, with some of the versions possibly causing an import loop. Duplicate layer IDs can happen - using layer-at-a-time editing tools - naively by accident, e.g. > FROM fedora > > ADD empty / > RUN rm /empty > ADD empty / creates two identical layers - by a malicious actor Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Instead, keep all layers in the root of the tarbale, and create subdirectories only in the legacy metadata writer. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

runcom · 2018-03-27T15:45:02Z

LGTM

nlewo · 2018-03-28T10:27:13Z

I've rebased and made some tests on my side.
Thanks @mtrmac @runcom

Signed-off-by: Antoine Eiche <lewo@abesis.fr>

nlewo force-pushed the pr/docker-archive branch 2 times, most recently from 7edbab1 to dd91c5c Compare October 27, 2017 22:04

nlewo force-pushed the pr/docker-archive branch 3 times, most recently from 1968d6e to c4c8480 Compare November 2, 2017 20:12

nlewo force-pushed the pr/docker-archive branch from c4c8480 to 5e0bcdd Compare November 4, 2017 08:06

mtrmac reviewed Nov 6, 2017

View reviewed changes

nlewo force-pushed the pr/docker-archive branch 3 times, most recently from ff42919 to bb5fb7a Compare December 19, 2017 12:44

mtrmac reviewed Jan 17, 2018

View reviewed changes

nlewo commented Jan 18, 2018

View reviewed changes

nlewo force-pushed the pr/docker-archive branch 2 times, most recently from bbff322 to 50a38b3 Compare January 18, 2018 21:24

mtrmac reviewed Jan 20, 2018

View reviewed changes

mtrmac reviewed Jan 26, 2018

View reviewed changes

nlewo force-pushed the pr/docker-archive branch from 20a6720 to 8bb449d Compare January 30, 2018 17:46

nlewo force-pushed the pr/docker-archive branch from 7106151 to e107475 Compare February 8, 2018 14:51

nlewo force-pushed the pr/docker-archive branch from ca67b84 to 0dac8ec Compare February 12, 2018 20:05

mtrmac mentioned this pull request Feb 17, 2018

--dest-compress=none does not work as expected containers/skopeo#478

Closed

nlewo force-pushed the pr/docker-archive branch from 0dac8ec to a490e69 Compare February 28, 2018 16:55

mtrmac mentioned this pull request Mar 3, 2018

DO NOT MERGE: Include legacy support in docker-archive: and update (skopeo layers) containers/skopeo#480

Closed

nlewo and others added 4 commits March 27, 2018 17:34

docker-archive: repeated layers are symlinked in the tar file

3158fe0

A new layer id is generated by using the current layer id and its parent layer id. Signed-off-by: Antoine Eiche <lewo@abesis.fr>

Don't create $DiffID subdirectories for layers with no configs

7e96dbf

Instead, keep all layers in the root of the tarbale, and create subdirectories only in the legacy metadata writer. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

nlewo force-pushed the pr/docker-archive branch from e992967 to 7e96dbf Compare March 28, 2018 10:25

runcom merged commit fbc14df into containers:master Mar 28, 2018

nlewo added a commit to nlewo/skopeo that referenced this pull request Mar 28, 2018

Vendor after merging containers/image#370

61351d4

Signed-off-by: Antoine Eiche <lewo@abesis.fr>

vrothberg mentioned this pull request Jan 2, 2019

podman push to "dir:" has wrong mediaType containers/podman#2013

Closed

mtrmac mentioned this pull request May 7, 2019

Clean up daemonImageDestination.PutBlob digest handling #205

Closed

docker-archive generates docker legacy compatible images #370

docker-archive generates docker legacy compatible images #370

Uh oh!

Conversation

nlewo commented Oct 27, 2017

Uh oh!

nlewo commented Oct 27, 2017

Uh oh!

nlewo commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Oct 31, 2017

Uh oh!

nlewo commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nlewo commented Nov 4, 2017

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlewo Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlewo commented Dec 19, 2017

Uh oh!

gilligan commented Jan 11, 2018

Uh oh!

gilligan commented Jan 14, 2018

Uh oh!

nlewo commented Jan 17, 2018

Uh oh!

mtrmac commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nlewo commented Oct 29, 2017 •

edited

Loading

nlewo commented Nov 3, 2017 •

edited

Loading

nlewo Nov 15, 2017 •

edited

Loading