chunked: estargz support #1001

giuseppe · 2021-08-20T15:10:56Z

in addition to zstd:chunked, add support for the estargz format.

estargz is maintained at github.com/containerd/stargz-snapshotter

Images using estargz can be used on old clients and registries that have no support for the zstd compression algorithm.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

rhatdan · 2021-08-22T20:15:01Z

@ktock PTAL

ktock

Thank you for this PR. Overall LGTM and works with docker.io.

off-topic: chunked pull doesn't seems work on ghcr.io with the following error? (can be addressed in the following-up PRs)

DEBU[0004] Failed to retrieve partial blob: invalid status code returned when fetching blob 200 (OK)

pkg/chunked/compression.go

pkg/chunked/storage_linux.go

giuseppe · 2021-08-23T10:35:10Z

DEBU[0004] Failed to retrieve partial blob: invalid status code returned when fetching blob 200 (OK)

yes, I saw that failure as well.

I need to handle the 200 return code differently (it seems that ghcr.io turns the partial request into a 200 when requesting too much of the original file)

giuseppe · 2021-08-23T10:47:16Z

``

yes, I saw that failure as well.

I need to handle the 200 return code differently (it seems that ghcr.io turns the partial request into a 200 when requesting too much of the original file)

I cannot see this issue anymore, as ghcr.io returns the correct 206 status code. Could you please give it another try and see if something changed for you as well?

ktock · 2021-08-23T12:24:22Z

pkg/chunked/compressor/compressor.go

+		compressionLevel = *c.compressionLevel
+	}
+
+	blob, err := estargz.Build(sr, estargz.WithCompressionLevel(compressionLevel))


estargz changes DiffID(uncompressed digest) of the layer because it adds TOC entry. Could we propagate the converted digest to the callers?

It’s an extensive assumption of the current c/image/copy structure that compression/decompression/encryption/… doesn’t change DiffID values. Propagating this isn’t impossible but it will touch a large part of the public API, probably triggering discussions about hiding some of the API, and so on…

In particular we definitely don’t want to add c/image/types.BlobInfo.DiffID; that historical abuse of the transport API for internal needs of the copy implementation needs to stop at some point, so let’s stop now.

ktock · 2021-08-23T12:25:23Z

I cannot see this issue anymore, as ghcr.io returns the correct 206 status code. Could you please give it another try and see if something changed for you as well?

Thank you for fixing this. Chunked pull works with ghcr.io now.

giuseppe · 2021-08-23T12:45:36Z

Thank you for fixing this. Chunked pull works with ghcr.io now.

I've not done anything :) I could still see the behavior though for some layers, I am working on a fix for containers/image to handle a 200 status.

giuseppe · 2021-08-23T13:26:36Z

do you think it could be helpful to add another annotation (like zstd:chunked does) to store the ToC offset so to avoid a round trip to read it?

ktock · 2021-08-23T13:34:59Z

do you think it could be helpful to add another annotation (like zstd:chunked does) to store the ToC offset so to avoid a round trip to read it?

Yes I do :)

giuseppe · 2021-08-24T11:52:22Z

ready for review

ktock · 2021-08-24T12:39:04Z

@giuseppe

It seems to create wrong diff_ids? (maybe related to #1001 (comment))

$ skopeo copy -f oci --dest-compress-format gzip:estargz  docker://ghcr.io/stargz-containers/alpine:3.10.2-org docker://ghcr.io/ktock/myalpine:3.10.2-esgz
$ skopeo inspect --raw docker://ghcr.io/ktock/myalpine:3.10.2-esgz | jq
{
  "schemaVersion": 2,
  "config": {
    "mediaType": "application/vnd.oci.image.config.v1+json",
    "digest": "sha256:1f97f0559cbddbff6c872039e93f18c1abdc279cbe82e0eb40258c28f4c30bfd",
    "size": 585
  },
  "layers": [
    {
      "mediaType": "application/vnd.oci.image.layer.v1.tar+gzip",
      "digest": "sha256:09f11a52de5e1e41b8b7f226170df3a9b32881f13e1a330a3dcc344b576911c0",
      "size": 2803990,
      "annotations": {
        "containerd.io/snapshot/stargz/toc.digest": "sha256:2a5b40d9dac53b0ba42c6995648c5a3bb9d71d376c66123facc25d5a7b31957f",
        "io.containers.estargz.uncompressed-size": "5935104"
      }
    }
  ]
}
$ skopeo inspect --config --raw docker://ghcr.io/ktock/myalpine:3.10.2-esgz | jq '.rootfs.diff_ids'
[
  "sha256:03901b4a2ea88eeaad62dbe59b072b28b6efa00491962b8741081c5df50c65e0"
]
$ crane blob ghcr.io/ktock/myalpine:3.10.2-esgz@sha256:09f11a52de5e1e41b8b7f226170df3a9b32881f13e1a330a3dcc344b576911c0 | gunzip | sha256sum
b880ce78f96e260a9e030fe94a8c9070199bb7a20d5e86c221ceeea2c999e00b  -

b880ce78f96e260a9e030fe94a8c9070199bb7a20d5e86c221ceeea2c999e00b (sha256) doesn't match to any in .rootfs.diff_ids.

giuseppe · 2021-08-24T13:01:33Z

@rhatdan @vrothberg @mtrmac any suggestions on how can we solve the issue above?

Should we drop the estargz build part for now?

mtrmac · 2021-08-24T14:33:45Z

@rhatdan @vrothberg @mtrmac any suggestions on how can we solve the issue above?

Well, build that capability? In addition to the API overhaul ~implied above, personally I’d say it’s mostly blocked on building tests for c/image/copy, the codebase is growing in size and especially special cases so much that it’s becoming untenable without tests; I’m working on them as time allows but that hasn’t been very fast recently.

Should we drop the estargz build part for now?

Splitting the feature into two is an option, sure.

giuseppe · 2021-08-25T07:29:47Z

I've dropped the compressor part until we figure out how it must be supported.

rhatdan · 2021-08-25T12:35:28Z

@ktock @mtrmac PTAL

pkg/chunked/storage_linux.go

pkg/chunked/compression.go

pkg/chunked/storage_linux.go

giuseppe · 2021-08-25T13:51:36Z

@ktock thanks for the review. Addressed the comments

pkg/chunked/compression.go

in addition to zstd:chunked, add support for the estargz format. estargz is maintained at github.com/containerd/stargz-snapshotter Images using estargz can be used on old clients and registries that have no support for the zstd compression algorithm. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ktock

Thank you! LGTM

rhatdan

LGTM

mtrmac

Just a few belated stylistic suggestions, ACK overall.

pkg/chunked/compression.go

pkg/chunked/storage_linux.go

giuseppe force-pushed the partial-stargz-support branch 5 times, most recently from 8f9b29b to e75b31b Compare August 21, 2021 20:08

ktock reviewed Aug 23, 2021

View reviewed changes

pkg/chunked/compression.go Show resolved Hide resolved

pkg/chunked/storage_linux.go Outdated Show resolved Hide resolved

giuseppe force-pushed the partial-stargz-support branch from e75b31b to 9861129 Compare August 23, 2021 10:01

giuseppe mentioned this pull request Aug 23, 2021

compression: add support for estargz containers/image#1351

Closed

giuseppe force-pushed the partial-stargz-support branch 2 times, most recently from 8daefc0 to 8ff2f26 Compare August 23, 2021 10:37

ktock reviewed Aug 23, 2021

View reviewed changes

giuseppe force-pushed the partial-stargz-support branch from 8ff2f26 to f621b5d Compare August 24, 2021 07:43

giuseppe marked this pull request as ready for review August 24, 2021 11:52

giuseppe marked this pull request as draft August 24, 2021 12:51

giuseppe force-pushed the partial-stargz-support branch from f621b5d to 1e06673 Compare August 25, 2021 07:28

giuseppe marked this pull request as ready for review August 25, 2021 07:29

ktock reviewed Aug 25, 2021

View reviewed changes

pkg/chunked/storage_linux.go Outdated Show resolved Hide resolved

pkg/chunked/compression.go Outdated Show resolved Hide resolved

pkg/chunked/storage_linux.go Outdated Show resolved Hide resolved

giuseppe force-pushed the partial-stargz-support branch from 1e06673 to ba3fe74 Compare August 25, 2021 14:03

ktock reviewed Aug 25, 2021

View reviewed changes

pkg/chunked/compression.go Outdated Show resolved Hide resolved

giuseppe force-pushed the partial-stargz-support branch from ba3fe74 to 2855d17 Compare August 25, 2021 14:16

ktock approved these changes Aug 25, 2021

View reviewed changes

rhatdan approved these changes Aug 25, 2021

View reviewed changes

rhatdan merged commit 0b8e233 into containers:main Aug 25, 2021

mtrmac reviewed Aug 25, 2021

View reviewed changes

pkg/chunked/compression.go Show resolved Hide resolved

pkg/chunked/storage_linux.go Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chunked: estargz support #1001

chunked: estargz support #1001

giuseppe commented Aug 20, 2021

rhatdan commented Aug 22, 2021

ktock left a comment

giuseppe commented Aug 23, 2021

giuseppe commented Aug 23, 2021

ktock Aug 23, 2021

mtrmac Aug 23, 2021

mtrmac Aug 23, 2021

ktock commented Aug 23, 2021

giuseppe commented Aug 23, 2021

giuseppe commented Aug 23, 2021

ktock commented Aug 23, 2021

giuseppe commented Aug 24, 2021

ktock commented Aug 24, 2021

giuseppe commented Aug 24, 2021

mtrmac commented Aug 24, 2021

giuseppe commented Aug 25, 2021

rhatdan commented Aug 25, 2021

giuseppe commented Aug 25, 2021

ktock left a comment

rhatdan left a comment

mtrmac left a comment

chunked: estargz support #1001

chunked: estargz support #1001

Conversation

giuseppe commented Aug 20, 2021

rhatdan commented Aug 22, 2021

ktock left a comment

Choose a reason for hiding this comment

giuseppe commented Aug 23, 2021

giuseppe commented Aug 23, 2021

ktock Aug 23, 2021

Choose a reason for hiding this comment

mtrmac Aug 23, 2021

Choose a reason for hiding this comment

mtrmac Aug 23, 2021

Choose a reason for hiding this comment

ktock commented Aug 23, 2021

giuseppe commented Aug 23, 2021

giuseppe commented Aug 23, 2021

ktock commented Aug 23, 2021

giuseppe commented Aug 24, 2021

ktock commented Aug 24, 2021

giuseppe commented Aug 24, 2021

mtrmac commented Aug 24, 2021

giuseppe commented Aug 25, 2021

rhatdan commented Aug 25, 2021

giuseppe commented Aug 25, 2021

ktock left a comment

Choose a reason for hiding this comment

rhatdan left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment