search

containerd / nerdctl

contaiNERD CTL - Docker-compatible CLI for containerd, with support for Compose, Rootless, eStargz, OCIcrypt, IPFS, ...
Apache License 2.0
7.63k stars 565 forks source link

Make zstd convert safe to use concurrently #3079

Closed apostasie closed 3 weeks ago

apostasie commented 3 weeks ago

Fix #3078 by removing use of intermediary uncompressed layer.

This incidentally gives us a small (?) perf boost (as we no longer read and write resources twice).

This will make convert tests able to be run concurrently.

@AkihiroSuda and team - PTAL at your convenience

apostasie commented 3 weeks ago

About the "perf boost": un-scientific measurement show 30% speed-up at minimum compression for Alpine - and ~15% at max compression.

apostasie commented 3 weeks ago

Adding cleaned-up test as well (now that we can parallelize).

apostasie commented 3 weeks ago

#3079 (comment)

Fixed.

ktock commented 3 weeks ago

Could you update labels.LabelUncompressed with the actual uncompressed digest, using Digester of go-digest? e.g.: https://github.com/containerd/containerd/blob/4a18adcfca4e37e829c0d3174bd04d4cbf222f9a/core/diff/apply/apply.go#L90

apostasie commented 3 weeks ago

Could you update labels.LabelUncompressed with the actual uncompressed digest, using Digester of go-digest? e.g.: https://github.com/containerd/containerd/blob/4a18adcfca4e37e829c0d3174bd04d4cbf222f9a/core/diff/apply/apply.go#L90

That would change the current behavior then, right?

Currently, on main we are calling on LayerConvertFunc (which does delete the label from the uncompressed resource), and then we just commit the original info labels, untouched: https://github.com/containerd/nerdctl/blob/main/pkg/imgutil/converter/zstd.go#L110

Can you clarify why we should change this behavior?

Is the current logic faulty on that?

ktock commented 3 weeks ago

Can you clarify why we should change this behavior? Is the current logic faulty on that?

The current logic isn't faulty but it's good to make sure that the content has the correct LabelUncompressed (this can be used for fast path as mentioned in https://github.com/containerd/nerdctl/pull/3079#discussion_r1634447937 ). But yes, we can do that as a following up so this patch looks good for now.

apostasie commented 3 weeks ago

Thanks a lot @ktock Appreciate the fast review on this!