Support `estargz` compression type by ktock · Pull Request #2246 · moby/buildkit

ktock · 2021-07-13T05:08:24Z

Currently, BuildKit supports lazy pulling of eStargz but not supports creating eStargz images. It would be great if it also supports the creation (there was also a discussion in slack recently). This commit adds support for compression=estargz to the cache as one of the compression variants.

buildctl build ... \
  --output type=image,name=docker.io/username/image,push=true,compression=estargz,oci-mediatypes=true

This commit also contains updates to docs to describe this usage.

tonistiigi · 2021-07-13T05:37:39Z

Do I get correctly that this creates 2 blobs. One from differ and then tries to convert it. This should be avoided. PTAL containerd/containerd#4263 (especially the last section about adding a workaround if there is no full support).

ktock · 2021-07-13T06:40:35Z

@tonistiigi Thank you for the comment.

Do I get correctly that this creates 2 blobs. One from differ and then tries to convert it. This should be avoided.

Yes, this creates one blob from differ and converts it. This might slower the overall time of estargz conversion but why I choose this approach is because it allows us to have this feature without having an additional (forked) implementation of diff.Comparer. Could you tell me why we should avoid this approach? --force-compression already does something similar. (I might have overlooked something)

PTAL containerd/containerd#4263 (especially the last section about adding a workaround if there is no full support).

Is there example/existing implementation of add a callback option to differ where I can pass the WriteCloser for compression. mentioned in containerd/containerd#4263 ?

tonistiigi · 2021-07-13T06:53:27Z

--force-compression already does something similar. (I might have overlooked something)

My understanding was that this only happened if the blob with a different compression already existed from the previous build/pull. In that case, the behavior is expected. If that is not the only case we should check that there isn't a performance or storage regression on the default path.

Is there example/existing implementation of add a callback option to differ where I can pass the WriteCloser for compression.

No that would need to be an update in the differ method in containerd library. When I looked at it last time containerd only supported uncompressed/gzip. This is needed for zstd as well, this is the case that I created the issue for.

ktock · 2021-07-13T07:12:25Z

@tonistiigi

My understanding was that this only happened if the blob with a different compression already existed from the previous build/pull. In that case, the behavior is expected.

Yes, the conversion by --force-compression happens only when the blob from the previous build/pull has the compression type different from the specified one.

If that is not the only case we should check that there isn't a performance or storage regression on the default path.

This patch creates 2 blobs only when the compression is estargz and doesn't affect the default path of gzip and uncompressed. Do you think that should be avoided even for estargz? If so I think we need to have a new diff.Comparer implementation that supports estargz. WDYT?

tonistiigi · 2021-07-13T16:40:56Z

Yes, the conversion by --force-compression happens only when the blob from the previous build/pull has the compression type different from the specified one.

Thanks for confirming.

Do you think that should be avoided even for estargz? If so I think we need to have a new diff. Comparer implementation that supports estargz. WDYT?

Yes, it would be preferred to avoid creating 2 blobs. It takes more storage and slows things down without the user expecting it. We have this issue anyway when we want to add more compression methods like zstd. As containerd differ only supports gzip I think the simplest way is to extend it with a callback method that implements the compression that it will call instead of the builtin gzip routine. Eventually, they probably want to extend their stream processors capability but I think that is a bigger change(maybe you disagree?). FYI @fuweid

ktock · 2021-07-14T13:22:14Z

cache/blobs.go

 				descr, err = sr.cm.Differ.Compare(ctx, lower, upper,
 					diff.WithMediaType(mediaType),
 					diff.WithReference(sr.ID()),
+					diff.WithCompression(compressor),
 				)


@tonistiigi Added this API to the fork of containerd (ktock/containerd@11b4622). Now this patch doesn't create 2 blobs. I'll work on upstreaming this API.

AkihiroSuda · 2021-07-20T05:07:58Z

go.mod

 	github.com/containerd/go-cni v1.0.2
 	github.com/containerd/go-runc v1.0.0
 	github.com/containerd/stargz-snapshotter v0.6.4
+	github.com/containerd/stargz-snapshotter/estargz v0.6.4


bump up to 0.7.0, and update Dockerfile as well

ktock · 2021-07-27T11:42:51Z

#2246 (comment) has been merged in containerd. Updated this patch not to depend on the fork.
Also bumped up stargz snapshotter to v0.7.0

tonistiigi · 2021-08-16T21:55:21Z

cache/blobs.go

 				mediaType = ocispec.MediaTypeImageLayer
 			case compression.Gzip:
 				mediaType = ocispec.MediaTypeImageLayerGzip
+				compressor = func(dest io.Writer, requiredMediaType string) (io.WriteCloser, error) {


why do we need to set custom compressor for plain gzip?

Fixed this not to set the custom compressor.

tonistiigi · 2021-08-16T21:56:41Z

cache/converter.go

 	return mt
 }
+
+func eStargzLayerConvertFunc(ctx context.Context, cs content.Store, desc ocispec.Descriptor) (*ocispec.Descriptor, error) {


could you move these stargz-only helper functions to a separate stargz file

Sure. Moved this into a new file.

tonistiigi · 2021-08-16T22:01:43Z

cache/remote.go

 				}
 				desc.Annotations[dslKey] = strings.Join(existingRepos, ",")
+				if dh, ok := sr.descHandlers[desc.Digest]; ok {
+					desc.Annotations = mergeEStargzAnnotations(dh.SnapshotLabels, desc.Annotations)


Just a note. It would be better if this annotations load/save wouldn't need startgz-only exceptions everywhere and would be more generic. But I don't have a specific proposal how to improve this.

Thank you for the note. Fixed the patch to make it more generic. Now *immutableRef.addCompressionBlob() saves estargz-related annotations as well so we don't need estargz-only expections.

tonistiigi · 2021-08-16T22:02:58Z

exporter/containerimage/export.go

 		}
 	}
+	if esgz && !i.ociTypes {
+		logrus.Warn("estargz compression should be used with oci-mediatypes for enabling layer verification")


You don't want to make this an error? Or always imply ocitypes for stargz, at least if not set?

Fixed this to always imply ocitypes with a warning log.

sipsma · 2021-08-16T22:26:06Z

Haven't taken a look in depth yet, but one note, if #2273 gets merged before this PR please update the new TestGetRemote in manager_test.go to also create this new estargz mediatype (right now it covers gzip and uncompressed) so we can make sure there aren't any regressions in parallel blob creation.

ktock · 2021-08-19T14:00:57Z

@sipsma Thank you for the note about the tests. Added estargz compression to TestGetRemote.

AkihiroSuda · 2021-08-23T05:54:49Z

docs/stargz-estargz.md

 For more details about lazy pull with stargz/eStargz images, please refer to the docs on these repositories.

-### Getting stargz/eStargz formatted images
+## Obtaining stargz/eStargz images


s/obtain/create/ perhaps

AkihiroSuda · 2021-08-23T05:58:50Z

exporter/containerimage/export.go

 		}
 	}
+	if esgz && !i.ociTypes {
+		logrus.Warn("forcefully turning on oci-mediatype mode for estargz")


probably s/forcefully/forcibly/

AkihiroSuda · 2021-08-23T16:16:25Z

Sorry needs rebase

ktock · 2021-08-24T04:34:12Z

Rebased. Also added tests to check the converted descriptor is valid and fixed estargz generation code to pass the tests.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

AkihiroSuda · 2021-08-25T09:19:36Z

3 LGTMs, merging

ktock marked this pull request as draft July 14, 2021 13:17

ktock force-pushed the esgzcompression branch from 7f822fc to 395d4c2 Compare July 14, 2021 13:18

ktock commented Jul 14, 2021

View reviewed changes

ktock force-pushed the esgzcompression branch 7 times, most recently from 4f79c89 to de8a342 Compare July 20, 2021 01:49

AkihiroSuda reviewed Jul 20, 2021

View reviewed changes

tonistiigi mentioned this pull request Jul 21, 2021

Compute diff from the upper directory of overlayfs-based snapshotter #2181

Merged

ktock force-pushed the esgzcompression branch from de8a342 to b058ada Compare July 26, 2021 02:05

ktock marked this pull request as ready for review July 26, 2021 02:30

ktock force-pushed the esgzcompression branch 3 times, most recently from 284ef97 to 152998f Compare August 3, 2021 01:25

ktock force-pushed the esgzcompression branch from 152998f to 0d7317d Compare August 10, 2021 06:40

tonistiigi requested review from AkihiroSuda, sipsma and tonistiigi August 16, 2021 21:53

tonistiigi reviewed Aug 16, 2021

View reviewed changes

sipsma mentioned this pull request Aug 16, 2021

Fix flightcontrol test and usage bugs #2273

Merged

ktock force-pushed the esgzcompression branch from 0d7317d to 63393f0 Compare August 19, 2021 13:11

tonistiigi approved these changes Aug 23, 2021

View reviewed changes

AkihiroSuda reviewed Aug 23, 2021

View reviewed changes

AkihiroSuda approved these changes Aug 23, 2021

View reviewed changes

ktock force-pushed the esgzcompression branch from 63393f0 to 6bf0470 Compare August 23, 2021 09:24

AkihiroSuda approved these changes Aug 23, 2021

View reviewed changes

AkihiroSuda added the status/needs-rebase label Aug 23, 2021

sipsma approved these changes Aug 23, 2021

View reviewed changes

ktock force-pushed the esgzcompression branch from 6bf0470 to a718253 Compare August 24, 2021 04:31

AkihiroSuda removed the status/needs-rebase label Aug 24, 2021

Add estargz compression type

f8d30d5

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>

ktock force-pushed the esgzcompression branch from a718253 to f8d30d5 Compare August 24, 2021 05:10

AkihiroSuda merged commit 94e9d15 into moby:master Aug 25, 2021

crazy-max mentioned this pull request Oct 25, 2021

vendor buildkit v0.9.1-0.20211025222436-33fb83eb7166 moby/moby#42968

Closed

aledbf mentioned this pull request Dec 15, 2021

Question: release with stargz support #2527

Closed

crazy-max added this to the v0.10.0 milestone Feb 4, 2022

aledbf mentioned this pull request Mar 16, 2022

Update buildkit to v0.10.0 gitpod-io/gitpod#8845

Merged

Conversation

ktock commented Jul 13, 2021

Uh oh!

tonistiigi commented Jul 13, 2021

Uh oh!

ktock commented Jul 13, 2021

Uh oh!

tonistiigi commented Jul 13, 2021

Uh oh!

ktock commented Jul 13, 2021

Uh oh!

tonistiigi commented Jul 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktock commented Jul 27, 2021

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!

sipsma commented Aug 16, 2021

Uh oh!

ktock commented Aug 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Aug 23, 2021

Uh oh!

ktock commented Aug 24, 2021

Uh oh!

AkihiroSuda commented Aug 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants