docker-daemon: transparently decompress layers in GetBlob #193

cyphar · 2016-12-27T10:31:29Z

In particular, this ensures that layers are decompressed if the source
layer is compressed but the destination does not permit compression.

This was a particular issue when doing a conversion from oci: to any
transport based on the docker tarfile format (because the Docker tarfile
format references DiffIDs, not the blob digests).

Closes #192
Signed-off-by: Aleksa Sarai [email protected]

cyphar · 2016-12-27T10:32:17Z

We can't really add nice tests for this until #148 is merged, because this is only really exposed when you have a compressed source going to a docker-tarfile source.

cyphar · 2016-12-27T10:32:37Z

However, I am adding tests for this to skopeo, so we will be able to test this change.

cyphar · 2016-12-27T11:36:53Z

Eugh, this uncovered another issue. Currently inside tarfile we use inputInfo.Digest.String() even when the digest was not given... SIGH.

cyphar · 2016-12-27T12:39:52Z

Okay, so CopySuite.TestCopyCompression tests that the old semantics work -- in an attempt to use dir: to check that compression was actually done. I'm not really sure how to change this test so that it still tests the same thing it tested previously (that compression is applied).

cyphar · 2016-12-28T06:52:21Z

I've added a ::compress option to dir: transports. It's not a pretty hack, but basically this ensures that we can mirror the old test functionality (though the semantics of what's being tested is slightly different).

mtrmac · 2017-01-07T04:11:45Z

Eugh, this uncovered another issue. Currently inside tarfile we use inputInfo.Digest.String() even when the digest was not given... SIGH.

AFAICT that is just an oversight ( #203 ). Though actually improving the code to handle such a case, as you are doing, would be nicer.

mtrmac · 2017-01-07T04:23:03Z

At the highest level, ShouldCompressLayers has been intended to be as is, i.e. one-directional: either compress, or leave in the original format; not as a compress/uncompress toggle.

This is useful e.g. for the remote-repository→docker-daemon: copy, where the daemon already can transparently decompress layers, so we should neither compress nor decompress the remote input, only downloading it (and verifying digests/signatures) is perfectly fine. Also…

I've added a ::compress option to dir: transports. It's not a pretty hack, but basically this ensures that we can mirror the old test functionality (though the semantics of what's being tested is slightly different).

… yeah, basically we’ve been treating dir: as a debugging output which records the inputs given as close as possible, and it would be nice to retain this. So that would suggest that an ImageDestination might perhaps want a tri-state, compress/decompress/leave unchanged.

But, most importantly, the discussion in #192 on the top-level solution (or perhaps even explaining the problem?) needs to happen first.

mtrmac · 2017-01-07T04:24:38Z

copy/copy.go

+			if err != nil {
+				return types.BlobInfo{}, errors.Wrapf(err, "Error decompressing blob %s on-the-fly", srcInfo.Digest)
+			}
+		}


Note that we already might be decompressing the stream once, in the getOriginalLayerCopyWriter path above, and doing the same work twice is rather wasteful. OTOH integrating all of that here is close to madness.

(Eventually the imageCopier needs to grow good tests. Not today, I guess :) )

mtrmac · 2017-01-07T04:25:59Z

directory/directory_dest.go

@@ -41,7 +41,7 @@ func (d *dirImageDestination) SupportsSignatures() error {

 // ShouldCompressLayers returns true iff it is desirable to compress layer blobs written to this destination.
 func (d *dirImageDestination) ShouldCompressLayers() bool {
-	return false
+	return d.ref.compressed


Really not a fan of dir: modifying the blobs, because of the the “debugging” / “just tell me what is in the source” purposes.

mtrmac · 2017-01-07T04:28:07Z

directory/directory_transport.go

+	options []string
+
+	// Does the directory want compressed blobs?
+	compressed bool


(review remarks on dir: just so that they are not lost if we decided to go this way, not as an endorsement of the concept)

Why have both, and silently accept any invalid input? Merely the compressed bool would allow us to both parse and format the cases we care about.

mtrmac · 2017-01-07T04:29:36Z

directory/directory_transport.go

+			return nil, errors.Errorf("dir: invalid path option suffix: no options set: %s", path)
+		}
+		path = parts[0]
+	}


If we did do so much parsing for dir references, this should probably move to the mode of ParseReference doing the string parsing, and NewReference(path string, compressed bool) just creating an object (so that API callers can do it cheaply).

mtrmac · 2017-01-07T04:43:29Z

docker/daemon/daemon_dest.go

+		if inputInfo.Digest == "" {
+			// In some (hacky) cases a caller might not know the size, but will
+			// know the digest (where "know" == "have a fake digest").
+			inputInfo.Digest = digester.Digest()


Actually the whole handling of Digest values in this function, especially immediately below, seems to be broken (should be a separate PR) and only happens to work through a random coincidence:

The returned digest must match the file name within the tarball (because PutManifest uses the layer digests to compute layerPaths. But, right now, we create the file with inputInfo.Digest (using, in principle, any algorithm), but we always return a freshly computed digest.Canonical value; the two do not generally match.

Basically, if we are provided a digest, we should either use it as is, and return, or we need to completely ignore it and compute from scratch (but the rest of the code generally expects us not to do that, for signatures etc.).

As an extension, this also works only because inputInfo.Digest is computed using digest.Canonical, just like the the computation below; that is, I expect, by design, but the code does not make that obvious (and again, computing twice…)

(I think this comment still stands, if anything needs to happen in PutBlob.)

I think it's this way because of the shitty DiffID lie. Not sure if we can properly fix it...

mtrmac · 2017-01-07T04:44:54Z

docker/daemon/daemon_dest.go

+
+		if inputInfo.Digest == "" {
+			// In some (hacky) cases a caller might not know the size, but will
+			// know the digest (where "know" == "have a fake digest").


When does “having a fake digest” happen? copy.Image always clears the digest if it compresses (or, with this PR, decompresses), doesn’t it?

Because DiffIDs != Digests, this happens for us (when copying we just have the names of the layers that we're dealing with, but we pretend like we don't have the Digest even though the two should be the same).

I'm going to keep this hunk change, but we can discuss the comment later.

Primarily, is any part of the “handle empty Digest case“ commit still necessary? It seems quite unrelated to the PR topic “transparently decompress layers”.

Secondarily, I still can’t understand what “a fake digest” means in here.

Sigh. Okay I'll move this to a separate PR and wait even longer for stuff to get merged.

Thanks for splitting this. Smaller PRs have less to block them from getting merged :)

(Really the point is that the API as defined expect the digest to either be unknown or valid, and the rest of this PR should make that true again. If we need change the API for other reasons, sure, let’s talk about that, but actually explicitly talk about it instead of adding a weird corner case which is unjustified by the rest of the code in the repo.)

cyphar · 2017-02-01T10:49:24Z

@mtrmac @runcom This is ready for review now.

mtrmac

Highlights:

Does this work with uncompressed layers?
Is the PutBlob part necessary for this?

mtrmac · 2017-02-01T20:45:08Z

copy/fixtures

@@ -0,0 +1 @@
+../pkg/compression/fixtures


Using symlinks to deduplicate is fine, but could you please keep the copy/fixtures directory and use symlinks to individual files? That would make it easier to add other files to copy/fixtures in the future.

mtrmac · 2017-02-01T20:50:12Z

docker/daemon/daemon_src.go

+		// caller which is trying to verify the blob will run into problems),
+		// we need to decompress blobs. This is a bit ugly, but it's a
+		// consequence of making everything addressable by their DiffID rather
+		// than by their digest...


“digests != diffIDs” is true in general; it would still be worth explaining explicitly that we (choose to) generate a v2s2 manifest with the available DiffID values, so GetBlob needs to return a content matching those DiffID values even if the blob has been compressed.

mtrmac · 2017-02-01T20:50:43Z

docker/daemon/daemon_src.go

+		decompressFunc, reader, err := compression.DetectCompression(stream)
+		if err != nil {
+			return nil, 0, errors.Wrapf(err, "detecting compression in blob %s", info.Digest)
+		}


Won’t this fail on uncompressed streams, such as those created by docker save?

No, because "no compression" is considered a valid "compressor" if you look in pkg/compression. You only get errors if you get an EOF that was unexpected for example. However, I need to add a check that decompressorFunc != nil.

mtrmac · 2017-02-01T20:51:33Z

docker/daemon/daemon_src.go

+		// than by their digest...
+		decompressFunc, reader, err := compression.DetectCompression(stream)
+		if err != nil {
+			return nil, 0, errors.Wrapf(err, "detecting compression in blob %s", info.Digest)


Please capitalize error messages, perhaps wrong but consistent with the rest of the code.

mtrmac · 2017-02-01T20:53:10Z

docker/daemon/daemon_src.go

+		newStream := readCloseWrapper{
+			Reader: reader,
+			closeFunc: func() error {
+				return stream.Close()


Can't this be just closeFunc: stream.Close?

mtrmac · 2017-02-01T20:54:07Z

docker/daemon/daemon_src.go

+type readCloseWrapper struct {
+	io.Reader
+	closeFunc func() error
+}


There already is a tarReadCloser in this file. Modifying that to use this readCloseWrapper would be nice.

The two are wrapping different internal objects. I can't really see a nice way of combining the two.

AFAICS the *tar.Reader in tarReadCloser can just as well be an io.Reader; nothing depends on that being a *.tar.Reader.

mtrmac · 2017-02-01T20:58:15Z

docker/daemon/daemon_dest.go

+
+		if inputInfo.Digest == "" {
+			// In some (hacky) cases a caller might not know the size, but will
+			// know the digest (where "know" == "have a fake digest").


Primarily, is any part of the “handle empty Digest case“ commit still necessary? It seems quite unrelated to the PR topic “transparently decompress layers”.

Secondarily, I still can’t understand what “a fake digest” means in here.

mtrmac · 2017-02-01T20:58:31Z

docker/daemon/daemon_dest.go

+		if inputInfo.Digest == "" {
+			// In some (hacky) cases a caller might not know the size, but will
+			// know the digest (where "know" == "have a fake digest").
+			inputInfo.Digest = digester.Digest()


(I think this comment still stands, if anything needs to happen in PutBlob.)

cyphar · 2017-02-07T09:56:21Z

@mtrmac I've moved the PutBlob code to a different branch. We can handle that two-line change separately.

mtrmac

👍 Thanks!

mtrmac · 2017-02-10T17:54:31Z

👍 again for PullApprove

cyphar · 2017-02-15T12:25:04Z

Can we please merge this so that containers/skopeo#280 can finally go in. Not having tests for the OCI and Docker translation code path is not acceptable -- at SUSE we actually plan to use this and it's getting frustrating that it keeps BREAKING.

We now have tests, but this is necessary for containers/skopeo#280 to pass (in particular, it breaks issues that exist with docker-daemon which are going to be tested once docker-archive: [#148] is merged).

cyphar · 2017-02-17T10:13:18Z

/ping @runcom -- can we please merge this?

runcom · 2017-02-17T10:37:50Z

@cyphar could you rebase and I'll merge?

cyphar · 2017-02-17T10:39:31Z

@runcom Rebase'd.

Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2017-02-17T10:44:02Z

The build failure makes no sense, I didn't touch manifest/manifest.go...

$ make deps
go get -t -tags "btrfs_noversion libdm_no_deferred_remove" ./...
# github.com/containers/image/manifest
manifest/manifest.go:57: undefined: v1.MediaTypeImageManifestList

This is necessary because inside the docker-daemon (soon to be Docker archive) transport, the provided DigestInfo's digest is *not* the digest of the blob but is rather the DiffID of the layer. This results in issues when converting a docker-daemon image to an OCI image and then back again -- the compression copying code doesn't know what to do because the digest of the blob and the blob's "digest" in DigestInfo don't match. Fix it by always silently decompressing blobs that come from Docker archive transports. Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2017-02-17T10:51:33Z

Heh, it's broken because of opencontainers/image-spec@a125828. We aren't vendoring image-spec so go get fails.

That's just dumb. We should be vendoring things.

runcom · 2017-02-17T10:56:33Z

That's just dumb. We should be vendoring things.

No.
This is a library, I won't vendor thing and fall into the golang mess of having a library with vendor dirs which clashes with whoever import it. If you're talking about running the test with some vendored code, that may work.

cyphar · 2017-02-17T10:58:13Z

I completely agree that vendoring is a mess, but if you want to run go on your library to test it we need to vendor things. Is there a reason that we aren't just doing test-skopeo (which should run our unit tests --- right?).

runcom · 2017-02-17T10:59:20Z

I completely agree that vendoring is a mess, but if you want to run go on your library to test it we need to vendor things. Is there a reason that we aren't just doing test-skopeo (which should run our unit tests --- right?).

not sure test-skopeo would run that, if it does, I'm 100% ok doing that instead of go get'ting random libraries from the net.

runcom · 2017-02-17T11:00:38Z

@cyphar to unblock this, can you create a corresponding skopeo PR which vendor this code and run the tests there? After it's green over there I'll merge this. (and sorry for this, we need a better way to handle this, even if it means removing test-skopeo from here if we switch skopeo to not use the latest master every time)

cyphar · 2017-02-17T12:17:33Z

@runcom: containers/skopeo#307

cyphar · 2017-02-17T12:29:48Z

Currently test-unit-local doesn't test vendor/ tests. While this makes sense for most vendors, we should be testing our own code inside skopeo. I can make a PR to improve this situation if you like (I'll do it as part of the PR updating to this patch being merged).

runcom · 2017-02-17T12:40:30Z

Currently test-unit-local doesn't test vendor/ tests. While this makes sense for most vendors, we should be testing our own code inside skopeo. I can make a PR to improve this situation if you like (I'll do it as part of the PR updating to this patch being merged).

👍 thanks Aleksa

runcom · 2017-02-17T12:40:35Z

LGTM

cyphar mentioned this pull request Dec 27, 2016

docker: add docker-archive transport #148

Merged

This was referenced Dec 28, 2016

[donotmerge] integration: use ::compress option for compression tests containers/skopeo#279

Closed

integration: add test for OCI <-> docker-archive translation containers/skopeo#280

Closed

mtrmac reviewed Jan 7, 2017

View reviewed changes

mtrmac mentioned this pull request Jan 7, 2017

Clean up daemonImageDestination.PutBlob digest handling #205

Closed

cyphar mentioned this pull request Jan 30, 2017

copy: dest !compression not complied with if source is compressed #192

Closed

cyphar changed the title ~~copy: obey destination compression rules~~ docker-daemon: transparently decompress layers in GetBlob Feb 1, 2017

mtrmac reviewed Feb 1, 2017

View reviewed changes

mtrmac mentioned this pull request Feb 1, 2017

Don't enforce digest match in storage.ReapplyBlob #233

Merged

mtrmac approved these changes Feb 10, 2017

View reviewed changes

copy: move compression to separate pkg

469d6e8

Signed-off-by: Aleksa Sarai <[email protected]>

runcom merged commit 3f493f2 into containers:master Feb 17, 2017

cyphar deleted the copy-obey-destination-compression branch February 17, 2017 12:42

cyphar mentioned this pull request Feb 20, 2017

manifest: remove ManifestList type #238

Closed

		@@ -0,0 +1 @@
		../pkg/compression/fixtures

docker-daemon: transparently decompress layers in GetBlob #193

docker-daemon: transparently decompress layers in GetBlob #193

Conversation

cyphar commented Dec 27, 2016

cyphar commented Dec 27, 2016

cyphar commented Dec 27, 2016

cyphar commented Dec 27, 2016

cyphar commented Dec 27, 2016

cyphar commented Dec 28, 2016

mtrmac commented Jan 7, 2017

mtrmac commented Jan 7, 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

cyphar commented Feb 1, 2017 • edited Loading

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

cyphar commented Feb 7, 2017

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Feb 10, 2017 • edited by runcom Loading

cyphar commented Feb 15, 2017 • edited Loading

cyphar commented Feb 17, 2017

runcom commented Feb 17, 2017

cyphar commented Feb 17, 2017

cyphar commented Feb 17, 2017

cyphar commented Feb 17, 2017

runcom commented Feb 17, 2017 • edited Loading

cyphar commented Feb 17, 2017

runcom commented Feb 17, 2017

runcom commented Feb 17, 2017

cyphar commented Feb 17, 2017

cyphar commented Feb 17, 2017

runcom commented Feb 17, 2017 • edited Loading

runcom commented Feb 17, 2017

cyphar commented Feb 1, 2017 •

edited

Loading

mtrmac commented Feb 10, 2017 •

edited by runcom

Loading

cyphar commented Feb 15, 2017 •

edited

Loading

runcom commented Feb 17, 2017 •

edited

Loading

runcom commented Feb 17, 2017 •

edited

Loading