Skip to content
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.

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

copy: compute blob compression on reused blobs based on source MediaType #1138

Merged
merged 2 commits into from
Feb 19, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 42 additions & 1 deletion copy/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -1069,6 +1069,26 @@ type diffIDResult struct {
// copyLayer copies a layer with srcInfo (with known Digest and Annotations and possibly known Size) in src to dest, perhaps (de/re/)compressing it,
// and returns a complete blobInfo of the copied layer, and a value for LayerDiffIDs if diffIDIsNeeded
func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, toEncrypt bool, pool *mpb.Progress) (types.BlobInfo, digest.Digest, error) {
// If the srcInfo doesn't contain compression information, try to compute it from the
// MediaType, which was either read from a manifest by way of LayerInfos() or constructed
// by LayerInfosForCopy(), if it was supplied at all. If we succeed in copying the blob,
// the BlobInfo we return will be passed to UpdatedImage() and then to UpdateLayerInfos(),
// which uses the compression information to compute the updated MediaType values.
// (Sadly UpdatedImage() is documented to not update MediaTypes from
// ManifestUpdateOptions.LayerInfos[].MediaType, so we are doing it indirectly.)
//
// This MIME type → compression mapping belongs in manifest-specific code in our manifest
// package (but we should preferably replace/change UpdatedImage instead of productizing
// this workaround).
if srcInfo.CompressionAlgorithm == nil {
switch srcInfo.MediaType {
case manifest.DockerV2Schema2LayerMediaType, imgspecv1.MediaTypeImageLayerGzip:
srcInfo.CompressionAlgorithm = &compression.Gzip
case imgspecv1.MediaTypeImageLayerZstd:
srcInfo.CompressionAlgorithm = &compression.Zstd
}
}

cachedDiffID := ic.c.blobInfoCache.UncompressedDigest(srcInfo.Digest) // May be ""
// Diffs are needed if we are encrypting an image or trying to decrypt an image
diffIDIsNeeded := ic.diffIDsAreNeeded && cachedDiffID == "" || toEncrypt || (isOciEncrypted(srcInfo.MediaType) && ic.c.ociDecryptConfig != nil)
Expand Down Expand Up @@ -1097,6 +1117,19 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
Artifact: srcInfo,
}
}

// If the reused blob has the same digest as the one we asked for, but
// the transport didn't/couldn't supply compression info, fill it in based
// on what we know from the srcInfos we were given.
// If the srcInfos came from LayerInfosForCopy(), then UpdatedImage() will
// call UpdateLayerInfos(), which uses this information to compute the
// MediaType value for the updated layer infos, and it the transport
// didn't pass the information along from its input to its output, then
// it can derive the MediaType incorrectly.
if blobInfo.Digest == srcInfo.Digest && blobInfo.CompressionAlgorithm == nil {
blobInfo.CompressionOperation = srcInfo.CompressionOperation
blobInfo.CompressionAlgorithm = srcInfo.CompressionAlgorithm
}
return blobInfo, cachedDiffID, nil
}
}
Expand Down Expand Up @@ -1353,7 +1386,15 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr
compressionOperation = types.PreserveOriginal
inputInfo = srcInfo
uploadCompressorName = srcCompressorName
uploadCompressionFormat = nil
// Remember if the original blob was compressed, and if so how, so that if
// LayerInfosForCopy() returned something that differs from what was in the
// source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(),
// it will be able to correctly derive the MediaType for the copied blob.
if isCompressed {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please document that this, as a side effect, causes a MIME type update if LayerInfosForCopy replaces a blob, because LayerInfosForCopy’s MediaType update does not cause a manifest edit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added Remember if the original blob was compressed, and if so how, so that if LayerInfosForCopy() returned something that differs from what was in the source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), it will be able to correctly derive the MediaType for the copied blob.

uploadCompressionFormat = &compressionFormat
} else {
uploadCompressionFormat = nil
}
}

// === Encrypt the stream for valid mediatypes if ociEncryptConfig provided
Expand Down