Support stargz snapshotter for dev stages by ktock · Pull Request #1402 · moby/buildkit

ktock · 2020-03-10T10:01:57Z

Related issues:

Throughout building steps, "pulling base image" is one of the time-consuming steps.

Recently, containerd community started "Stargz Snapshotter" (containerd/stargz-snapshotter) non-core subproject for speeding up image pulling. With this snapshotter, we can directly mount stargz-formatted (but still docker-compatible) image to nodes, which takes much shorter than downloading the whole of the image to nodes.

This commit solves the performance problem on pulling base images for "dev" stages using stargz snapshotter.

We introduced new RecordType "snapshot" for unexported base images. These images aren't guaranteed to have their whole contents in the content store but snapshots (possibly "remote" snapshots) are provided. Target (exported) base images don't have this type because its contents need to be fully available on the node for exportation.

We can test this patch with stargz snapshotter on this branch (Please also see this instruction).
We can see the benchmarking result with sample image on containerd/stargz-snapshotter#53 (comment).

This PR also intends to be a discussion thread for the design and implementation strategies of this feature so if there is a better one please tell us.

AkihiroSuda · 2020-03-11T01:37:33Z

Can we have integration tests?

AkihiroSuda · 2020-03-11T09:08:14Z

cmd/buildkitd/main_oci_worker.go

+		newFunc := func(root string) (ctdsnapshot.Snapshotter, error) {
+			gopts := []grpc.DialOption{
+				grpc.WithInsecure(),
+				grpc.WithBackoffMaxDelay(3 * time.Second),


WithBackoffMaxDelay is deprecated

See https://github.com/containerd/containerd/pull/4053/files#diff-1b43e06d76b779c2d0cb9f11f9bf69f1L100

Thanks for your feedback. I fixed it.

AkihiroSuda · 2020-03-11T09:09:53Z

cmd/buildkitd/main_oci_worker.go

 		},
+		cli.StringFlag{
+			Name:  "oci-worker-proxy-snapshotter-address",
+			Usage: "use proxy snapshotter at specified address",


With unix:// or without?

Thanks for your feedback. Explicitly described not to include unix:// prefix and added small validation against the specified path.

imho *-address should have protocol, *-path or *-socket doesn't need to.

ktock · 2020-03-13T12:00:12Z

I'm currently working on the benchmarking and let me share the latest benchmarking results for building sample images(benchmarking scripts are here). I'll work on integration tests soon.

legacy: unpatched buildkit(f1ecc78) + tarball base image
stargz: this PR's buildkit + stargz-formatted base image
estargz: this PR's buildkit + extended stargz-formatted base image

Please note each build's initial state; there is no content on the cache(so pulling base images occur for each build).

With OCI worker

With containerd worker

ktock · 2020-03-20T02:54:27Z

Added unit and integration testes and fixed some bugs to make them happy. Containerd hasn't released remote-snapshots-related patches yet(but already merged) so current integration testes are for oci worker. After these patches are released, we can apply tests for containerd worker with another PR. PTAL:pray:

AkihiroSuda

well done

AkihiroSuda · 2020-03-25T04:07:36Z

@tonistiigi WDYT?

tonistiigi

I don't like that this is controlled from the Dockerfile frontend. Frontends/LLB shouldn't optimize for a specific backend implementation. Not sure how it even works, if you rebuild with different targets and get previous cache. And layer being in a target stage does not mean it is exported (same as it not being it target stage doesn't mean it never will be exported). Every record should be exportable with every exporter (the timing can be different for different implementations, eg. #870 )

This is based on a very minimal review, so lmk if I missed anything.

ktock · 2020-03-26T05:14:06Z

@tonistiigi Thanks for you feedbacks and pointer! #870 seems to have same motivation with this patch in terms of defering pulling layers until when it's actually needed.

From #870,

All the ops using inputs should expect that a input could be a remote and not a cache.ImmutableRef. If they receive a remote but need to access the data directly (for example exec op) they can call a method on it that would extract it and change the type to immutableref. This method would be where the pull now actually happens...

Now containerd doesn't require layers being locally available even to exec something on them if these layers are remote snapshots. So in this case, we can hopefully defer the pulling (= changing the type from remote to immutableref) even until export occurs and we don't need to pull nor have immutablerefs for all unexported layers, without inconsistency.

This patch initially aimed to enable bulidkit to use the containerd's remote snapshots from scratch but now I think this feature can be applied more simply on #870 as an advanced optimization as mentioned above. Do you have plans to move #870 forward?

tonistiigi · 2020-03-26T05:29:22Z

Now containerd doesn't require layers being locally available even to exec something on them if these layers are remote snapshots. So in this case, we can hopefully defer the pulling (= changing the type from remote to immutableref) even until export occurs and we don't need to pull nor have immutablerefs for all unexported layers, without inconsistency.

Yes, it is fine to run a container without all the snapshots/blobs locally if the implementation supports it. But at any point the record used by that container or any other part of the build should remain exportable. This also complicates caching a lot. It is possible that the cache needs to store the registry ref where the rest of the data can be pulled and loading from a cache needs to check if the new build still has access to the remote resource.

we don't need to pull nor have immutablerefs for all unexported layers,

More likely I think the design is that immutableref can have this separate implementation. If you would make some parts of build work on something else than immutableref this would be a lot of exceptions.

Do you have plans to move #870 forward?

I probably don't have time to work on it on near future. But I can give you design direction if you are interested. @hinshun and I have also been discussing something called MergeOp that is very much related and needs a proper proposal/design. The hardest part is that I don't want to break Moby implementation with this and that model that doesn't have content blobs makes the whole thing even more complicated.

tonistiigi · 2020-08-03T05:35:08Z

@ktock #1475 is getting into a ready state. Feel free to take a look if it has everything you need and port this over to the logic where refs can request a data source if they only have partial data when they are loaded from cache.

ktock · 2020-08-03T06:46:35Z

Thanks! I'll take a look at.

ktock · 2020-08-17T15:09:07Z

I've ported this PR over the lazy ref logic (#1475). Thanks for the great work! Based on that logic, the new patch doesn't require changes on the frontend and remote snapshots seem to be able to be used with immutableRefs with keeping them safely exportable. The following is a summary of the design in the new patch.

immutableRef.Extract() and Mount() don't unlazy the ref if it's snapshot can be provided as a remote snapshot from the underlying snapshotter.
mutableRefs possibly have some lazy parents (immutableRefs) because Extract() method don't guarantee that the contents are unlazied.
cache.DescHandler has a new field SnapshotLabels which contains key-value pairs used by the underlying snapshotter for searching for remote snapshots corresponding to the blobs. By default, *containerimage.puller adds labels needed to query stargz blobs to the registry.

@tonistiigi @AkihiroSuda What do you think about the above design? Please tell me if I'm missing something.
I'll work on tests once we reach an agreement on the design.

tonistiigi · 2020-08-18T04:12:24Z

cc @sipsma

tonistiigi · 2020-08-18T04:15:28Z

@ktock panic in CI

sandbox.go:218: panic: runtime error: invalid memory address or nil pointer dereference
        sandbox.go:218: [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0xbc2820]
        sandbox.go:218: 
        sandbox.go:218: goroutine 595 [running]:
        sandbox.go:218: github.com/moby/buildkit/cache.(*immutableRef).prepareRemoteSnapshots(0xc0004e1580, 0x1351d60, 0xc0004c6d50, 0x0, 0xc0003cf320, 0x1, 0x1)
        sandbox.go:218: 	/src/cache/refs.go:420 +0x160
        sandbox.go:218: github.com/moby/buildkit/cache.(*immutableRef).Extract(0xc0004e1580, 0x1351d60, 0xc0004c6d50, 0x0, 0x0)
        sandbox.go:218: 	/src/cache/refs.go:394 +0x19c```

ktock · 2020-08-18T04:57:31Z

@tonistiigi Thanks for the comment. Fixed the panic.

AkihiroSuda · 2020-08-19T15:11:09Z

source/containerimage/pull.go

+			for _, desc := range p.manifest.Remote.Descriptors {
+				layers += fmt.Sprintf("%s,", desc.Digest.String())
+			}
+			if len(layers) >= 1 {


use strings.TrimSuffix to trim the extra ,.

Or consider using strings.Join

sipsma

LGTM, one minor nit. ~~Also, would it be possible to add an integ test for this (probably to client/client_test.go) either in this PR or in a follow-up?~~ EDIT: saw you already mentioned adding tests in a previous comment, nevermind :-)

sipsma · 2020-08-21T21:49:24Z

cache/refs.go

 	return sr.extract(ctx, sr.descHandlers)
 }

+func (sr *immutableRef) prepareRemoteSnapshots(ctx context.Context, dhs DescHandlers) (bool, error) {


nit: I think this method may as well be wrapped in a flightcontrol.Group, similar to sr.extract, just to de-dupe some work when possible.

Thanks for the feedback! Fixed to use fileghtcontrol.Group.

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

ktock · 2020-08-24T08:10:09Z

Thanks for the feedback. Added test for this PR.

sipsma · 2020-08-26T01:59:52Z

client/client_test.go

+
+	// Check if image layers are lazy
+	var layers []ocispec.Descriptor
+	for _, layer := range manifest.Layers[:len(manifest.Layers)-1] {


Why check all but the last layer? Something to do with stargz? Would be good to have a comment for.

Thanks for the comment. This is because the topmost layer (created by running a command on that stargz image) isn't lazy. I added comments about this and also added a check to make sure that the topmost layer isn't lazy.

Ohh duh, forgot that the topmost layer was created by run, makes sense, thanks

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

ktock mentioned this pull request Mar 10, 2020

Support stargz snapshotter for speeding up base image preparation #1396

Open

ktock force-pushed the remote-snapshotter branch from 8ed56b6 to 9deafbb Compare March 10, 2020 10:18

AkihiroSuda reviewed Mar 11, 2020

View reviewed changes

ktock force-pushed the remote-snapshotter branch 3 times, most recently from a58eebe to 425c1c7 Compare March 11, 2020 11:29

ktock force-pushed the remote-snapshotter branch 5 times, most recently from 2398937 to d8135be Compare March 20, 2020 02:32

AkihiroSuda approved these changes Mar 20, 2020

View reviewed changes

tonistiigi reviewed Mar 25, 2020

View reviewed changes

tonistiigi mentioned this pull request Apr 8, 2020

Add MergeOp to LLB #1431

Closed

ktock force-pushed the remote-snapshotter branch 3 times, most recently from e21ee71 to 46c4626 Compare August 17, 2020 14:55

ktock force-pushed the remote-snapshotter branch from 46c4626 to 6902261 Compare August 18, 2020 04:25

AkihiroSuda reviewed Aug 19, 2020

View reviewed changes

ktock force-pushed the remote-snapshotter branch from 6902261 to b81e1a3 Compare August 20, 2020 10:12

AkihiroSuda approved these changes Aug 20, 2020

View reviewed changes

sipsma approved these changes Aug 21, 2020

View reviewed changes

ktock force-pushed the remote-snapshotter branch from b81e1a3 to a2415fc Compare August 22, 2020 14:18

Enable to use remote snapshots for refs

c975424

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

ktock force-pushed the remote-snapshotter branch 3 times, most recently from 6388c6c to 7e25d84 Compare August 24, 2020 07:41

sipsma reviewed Aug 26, 2020

View reviewed changes

ktock force-pushed the remote-snapshotter branch from 7e25d84 to 95add53 Compare August 27, 2020 06:49

Add integration test for containerd and stargz snapshotter

7618920

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

ktock force-pushed the remote-snapshotter branch from 95add53 to 7618920 Compare August 27, 2020 06:50

sipsma merged commit a57f8a2 into moby:master Aug 31, 2020

AkihiroSuda mentioned this pull request Sep 1, 2020

Add stargz snapshotter to moby/buildkit image #1663

Closed

ktock deleted the remote-snapshotter branch September 1, 2020 05:43

ktock mentioned this pull request Dec 9, 2020

Add eStargz specification to OCI v1 (support lazy pulling) opencontainers/image-spec#815

Open

Conversation

ktock commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Mar 11, 2020

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!

ktock commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

With OCI worker

With containerd worker

Uh oh!

ktock commented Mar 20, 2020

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Mar 25, 2020

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

ktock commented Mar 26, 2020

Uh oh!

tonistiigi commented Mar 26, 2020

Uh oh!

tonistiigi commented Aug 3, 2020

Uh oh!

ktock commented Aug 3, 2020

Uh oh!

ktock commented Aug 17, 2020

Uh oh!

tonistiigi commented Aug 18, 2020

Uh oh!

tonistiigi commented Aug 18, 2020

Uh oh!

ktock commented Aug 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipsma left a comment • 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!

ktock commented Aug 24, 2020

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ktock commented Mar 10, 2020 •

edited

Loading

ktock commented Mar 13, 2020 •

edited

Loading

sipsma left a comment •

edited

Loading