cache: Allow sharing compression variants among refs #2603

ktock · 2022-02-07T12:19:40Z

Currently, a compression variant of a ref is not visible from another. This leads to duplicated conversion as mentioned in #2346.
This commit allows refs sharing compression variants with each other.

When a ref converts a blob (compression blob A) to another compression (compression blob B), Blob A and B are linked together using GC label to make sure that all linked variants are released together (i.e. they aren't released even when refs lease only some of them). (Maybe in the future we can switch to the lease-based approach as discussed in #2347. But how can we manage the ref counter of the lease itself among refs in a simple way?)

When a ref exports a compression variant, it first walks all linked compression variant blobs and checks if there is a blob that can be reused without conversion. Conversion happens only when no blob can be reused.

latency of exporing layers

This runs two builds with two compression types (gzip and zstd). Both of the builds share the same lower layers.

A:

FROM ${REGISTRY}/${IMG}
RUN echo hello > /hello

B:

FROM ${REGISTRY}/${IMG}
RUN echo hi > /hi

Before the measurement, zstd-compressed B is stored to the registry.

Steps: https://github.com/ktock/buildkit/blob/5b9997d2b45ce57ed8ad07969840069e1292a388/hack/blob/measure.sh#L51-L71
- First, we build A as gzip then convert it to zstd.
  - Here, gzip and zstd variants of ${REGISTRY}/${IMG} are stored to the content store.
- Next, we pull zstd-compressed B (contains zstd-version of ${REGISTRY}/${IMG} in the lower layers) from the registry then convert it to gzip.
  - Here, gzip-version of ${REGISTRY}/${IMG} has already been cached during building A, so this build should reuse these blobs without duplicated conversions.
  - This PR-version of BuildKit successfully reuses it but master version of BuildKit doesn't.

The following shows the time to export layers during building gzip-version of B at the final step in the above.
This PR exports layers without conversion so it's faster than the master version.

measured on Github Actions ubuntu runner:
- PR: https://github.com/ktock/buildkit/actions/runs/1805493851
- master: https://github.com/ktock/buildkit/actions/runs/1805496498

OCI worker

`${IMG}`	master	PR
ubuntu:20.04	4.1s	0s
tomcat:10.0.0-jdk15-openjdk-buster	23.3s	0s

containerd worker

`${IMG}`	master	PR
ubuntu:20.04	25.5s	0s
tomcat:10.0.0-jdk15-openjdk-buster	190.2s	0.1s

master version of containerd worker's performance is too worse than OCI worker.
This is because of the lack of bufio writers for the content store.
This will be fixed by #2601.

tonistiigi · 2022-02-09T00:42:53Z

cache/refs.go

-	info.Labels[cachedVariantLabel] = desc.Digest.String()
-	if _, err := cs.Update(ctx, info, "labels."+cachedVariantLabel); err != nil {
+	vInfo = addBlobDescToInfo(desc, vInfo)
+	if _, err := cs.Update(ctx, vInfo, fieldsFromLabels(vInfo.Labels)...); err != nil {


Doesn't this create loops that are never cleaned up because they keep references to each other?

Thank you for the comment. Contents are cleaned up even if a loop of GC label is created when no lease reference them. Added a test to make sure that.

sipsma · 2022-02-09T17:14:26Z

cache/converter.go

@@ -129,6 +130,7 @@ var bufioPool = sync.Pool{
 }

 func (c *conversion) convert(ctx context.Context, cs content.Store, desc ocispecs.Descriptor) (*ocispecs.Descriptor, error) {
+	logrus.WithField("blob", desc).WithField("target", c.target).Debugf("converting blob to the target compression")


nit: I think there was an update recently to always use the bklog package instead of logrus directly

Thank you for the suggestion. Fixed to use bklog.

sipsma · 2022-02-09T18:11:32Z

cache/refs.go

+		return nil
+	}
+	if _, err := walkBlobVariantsOnly(ctx, cs, desc.Digest, func(desc ocispecs.Descriptor) bool { return f(desc) }, nil); err != nil {
+		logrus.WithError(err).Warn("failed to get variant blob")


Why not also return the error? Or if we don't want to return an error from this func, should we just drop the error return type?

Fixed to return an error.

sipsma · 2022-02-09T18:29:43Z

cache/remote.go

+		defer func() {
+			if rerr == nil {
+				rerr = p.ref.addBlob(ctx, p.desc)


Should this defer be after the check below for whether the ref is lazy or not?

Thank you for the review. Moved the defer to after the check.

sipsma · 2022-02-09T18:34:09Z

cache/blobs.go

@@ -267,10 +267,15 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool

 // setBlob associates a blob with the cache record.
 // A lease must be held for the blob when calling this function
-func (sr *immutableRef) setBlob(ctx context.Context, compressionType compression.Type, desc ocispecs.Descriptor) error {
+func (sr *immutableRef) setBlob(ctx context.Context, desc ocispecs.Descriptor) (rerr error) {


nit: while I get how it evolved this way, it's confusing that there are now separate setBlob and addBlob methods on *immutableRef. If you think there's a viable way to combine the two into one that might help clarify the code going forward. But if that's going to require too much refactoring or anything, then maybe just a comment on the methods explaining the difference would help.

Added a comment to the func and renamed it to linkBlob which might be less confusing.

sipsma · 2022-02-09T18:40:14Z

cache/refs.go

-}
-
-func (sr *immutableRef) addCompressionBlob(ctx context.Context, desc ocispecs.Descriptor, compressionType compression.Type) error {
+func (sr *immutableRef) addBlob(ctx context.Context, desc ocispecs.Descriptor) error {


Should this method also set the ref's size metadata to sizeUnknown so that future calls to sr.size() recalculate the new size from the addition of a new blob?

Thank you for the suggestion. Fixed to set size to sizeUnknown.

sipsma · 2022-02-10T17:53:37Z

cache/refs.go

 		return err
 	}
+	sr.queueSize(sizeUnknown) // let the future call to size() recalcultate the new size


Thanks for adding this. In order for this metadata update to be safe against race conditions I think it also needs to be wrapped in the same flightcontrol group and key as is used in the cacheRecord.size() method. The metadata update itself would have to be the same pattern as is used there too like:

cr.mu.Lock() cr.queueSize(usage.Size) if err := cr.commitMetadata(); err != nil { cr.mu.Unlock() return s, err } cr.mu.Unlock()

(We could also just add a shortcut cacheMetadata.setSize method that skips the queue and gets rid of the need for the lock, whatever works)

The metadata update itself would have to be the same pattern as is used there too

Thank you for the suggestion. Fixed it.

the same flightcontrol group and key as is used in the cacheRecord.size() method

Sorry, I'm not sure why do we need this. Size metadata seems already protected by cr.mu?

Sorry, I'm not sure why do we need this.

You're right, I was thinking about this wrong, the mutex is enough. Thanks for adding that.

sipsma · 2022-02-10T18:28:26Z

cache/manager_test.go

+	// Check if contents are cleaned up
+	for _, d := range gotChain {
+		_, err := co.cs.Info(ctx, d)
+		require.Error(t, err)


should probably also assert its ErrNotFound specifically

Thank you for the review. Fixed this.

Signed-off-by: Kohei Tokunaga <[email protected]>

ktock marked this pull request as draft February 7, 2022 12:19

ktock marked this pull request as ready for review February 7, 2022 12:20

ktock marked this pull request as draft February 7, 2022 12:25

ktock force-pushed the blobgc branch from b6c40b8 to 1c81bc2 Compare February 7, 2022 12:29

ktock marked this pull request as ready for review February 7, 2022 13:27

tonistiigi reviewed Feb 9, 2022

View reviewed changes

tonistiigi requested a review from sipsma February 9, 2022 00:43

ktock force-pushed the blobgc branch 2 times, most recently from c85e633 to c4a7d1a Compare February 9, 2022 14:31

sipsma reviewed Feb 9, 2022

View reviewed changes

ktock force-pushed the blobgc branch from c4a7d1a to 7b7d3e4 Compare February 10, 2022 02:12

sipsma reviewed Feb 10, 2022

View reviewed changes

cache: Allow sharing compression variants among refs

4280dfd

Signed-off-by: Kohei Tokunaga <[email protected]>

ktock force-pushed the blobgc branch from 7b7d3e4 to 4280dfd Compare February 11, 2022 07:44

sipsma approved these changes Feb 11, 2022

View reviewed changes

tonistiigi merged commit d50b4d4 into moby:master Feb 11, 2022

imeoer mentioned this pull request Feb 17, 2022

Support for exporting nydus compression type #2581

Merged

ktock mentioned this pull request Feb 24, 2022

Fix refs ignoring gc labels of contents #2674

Merged

aledbf mentioned this pull request Mar 16, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache: Allow sharing compression variants among refs #2603

cache: Allow sharing compression variants among refs #2603

ktock commented Feb 7, 2022 •

edited

Loading

tonistiigi Feb 9, 2022

ktock Feb 9, 2022

sipsma Feb 9, 2022

ktock Feb 10, 2022

sipsma Feb 9, 2022

ktock Feb 10, 2022

sipsma Feb 9, 2022

ktock Feb 10, 2022

sipsma Feb 9, 2022

ktock Feb 10, 2022

sipsma Feb 9, 2022

ktock Feb 10, 2022

sipsma Feb 10, 2022

ktock Feb 11, 2022

sipsma Feb 11, 2022

sipsma Feb 10, 2022

ktock Feb 11, 2022

cache: Allow sharing compression variants among refs #2603

cache: Allow sharing compression variants among refs #2603

Conversation

ktock commented Feb 7, 2022 • edited Loading

latency of exporing layers

OCI worker

containerd worker

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

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

ktock commented Feb 7, 2022 •

edited

Loading