Skip to content

cache: Allow sharing compression variants among refs #2603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tonistiigi merged 1 commit into from
Feb 11, 2022
Merged

cache: Allow sharing compression variants among refs #2603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 10 additions & 12 deletions cache/blobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool
return nil, errors.Errorf("unknown layer compression type")
}

if err := sr.setBlob(ctx, comp.Type, desc); err != nil {
if err := sr.setBlob(ctx, desc); err != nil {
return nil, err
}
return nil, nil
Expand All @@ -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) {
Copy link
Copy Markdown
Collaborator

@sipsma sipsma Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@ktock ktock Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if _, ok := leases.FromContext(ctx); !ok {
return errors.Errorf("missing lease requirement for setBlob")
}
defer func() {
if rerr == nil {
rerr = sr.linkBlob(ctx, desc)
}
}()

diffID, err := diffIDFromDescriptor(desc)
if err != nil {
Expand All @@ -280,10 +285,6 @@ func (sr *immutableRef) setBlob(ctx context.Context, compressionType compression
return err
}

if compressionType == compression.UnknownCompression {
return errors.Errorf("unhandled layer media type: %q", desc.MediaType)
}

sr.mu.Lock()
defer sr.mu.Unlock()

Expand Down Expand Up @@ -311,9 +312,6 @@ func (sr *immutableRef) setBlob(ctx context.Context, compressionType compression
return err
}

if err := sr.addCompressionBlob(ctx, desc, compressionType); err != nil {
return err
}
return nil
}

Expand Down Expand Up @@ -437,11 +435,11 @@ func ensureCompression(ctx context.Context, ref *immutableRef, comp compression.
// This ref can be used as the specified compressionType. Keep it lazy.
return nil, nil
}
return nil, ref.addCompressionBlob(ctx, desc, comp.Type)
return nil, ref.linkBlob(ctx, desc)
}

// First, lookup local content store
if _, err := ref.getCompressionBlob(ctx, comp.Type); err == nil {
if _, err := ref.getBlobWithCompression(ctx, comp.Type); err == nil {
return nil, nil // found the compression variant. no need to convert.
}

Expand All @@ -460,7 +458,7 @@ func ensureCompression(ctx context.Context, ref *immutableRef, comp compression.
}

// Start to track converted layer
if err := ref.addCompressionBlob(ctx, *newDesc, comp.Type); err != nil {
if err := ref.linkBlob(ctx, *newDesc); err != nil {
return nil, errors.Wrapf(err, "failed to add compression blob")
}
return nil, nil
Expand Down
2 changes: 2 additions & 0 deletions cache/converter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/containerd/containerd/images/converter"
"github.com/containerd/containerd/labels"
"github.com/moby/buildkit/identity"
"github.com/moby/buildkit/util/bklog"
"github.com/moby/buildkit/util/compression"
digest "github.com/opencontainers/go-digest"
ocispecs "github.com/opencontainers/image-spec/specs-go/v1"
Expand Down Expand Up @@ -129,6 +130,7 @@ var bufioPool = sync.Pool{
}

func (c *conversion) convert(ctx context.Context, cs content.Store, desc ocispecs.Descriptor) (*ocispecs.Descriptor, error) {
bklog.G(ctx).WithField("blob", desc).WithField("target", c.target).Debugf("converting blob to the target compression")
// prepare the source and destination
labelz := make(map[string]string)
ref := fmt.Sprintf("convert-from-%s-to-%s-%s", desc.Digest, c.target.Type.String(), identity.NewID())
Expand Down
Loading