Merge pull request #2288 from mtrmac/refactor-partial-pulls

Fix c/storage destination with partial pulls

copy/single.go

@@ -695,11 +695,16 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
 			requiredCompression = ic.compressionFormat
 		}
 
+		var tocDigest digest.Digest
+
 		// Check if we have a chunked layer in storage that's based on that blob.  These layers are stored by their TOC digest.
-		tocDigest, err := chunkedToc.GetTOCDigest(srcInfo.Annotations)
+		d, err := chunkedToc.GetTOCDigest(srcInfo.Annotations)
 		if err != nil {
 			return types.BlobInfo{}, "", err
 		}
+		if d != nil {
+			tocDigest = *d
+		}
 
 		reused, reusedBlob, err := ic.c.dest.TryReusingBlobWithOptions(ctx, srcInfo, private.TryReusingBlobOptions{
 			Cache:                   ic.c.blobInfoCache,
@@ -718,7 +723,11 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
 		if reused {
 			logrus.Debugf("Skipping blob %s (already present):", srcInfo.Digest)
 			func() { // A scope for defer
-				bar := ic.c.createProgressBar(pool, false, types.BlobInfo{Digest: reusedBlob.Digest, Size: 0}, "blob", "skipped: already exists")
+				label := "skipped: already exists"
+				if reusedBlob.MatchedByTOCDigest {
+					label = "skipped: already exists (found by TOC)"
+				}
+				bar := ic.c.createProgressBar(pool, false, types.BlobInfo{Digest: reusedBlob.Digest, Size: 0}, "blob", label)
 				defer bar.Abort(false)
 				bar.mark100PercentComplete()
 			}()
@@ -751,7 +760,10 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
 				wrapped: ic.c.rawSource,
 				bar:     bar,
 			}
-			uploadedBlob, err := ic.c.dest.PutBlobPartial(ctx, &proxy, srcInfo, ic.c.blobInfoCache)
+			uploadedBlob, err := ic.c.dest.PutBlobPartial(ctx, &proxy, srcInfo, private.PutBlobPartialOptions{
+				Cache:      ic.c.blobInfoCache,
+				LayerIndex: layerIndex,
+			})
 			if err == nil {
 				if srcInfo.Size != -1 {
 					refill := srcInfo.Size - bar.Current()

internal/imagedestination/stubs/put_blob_partial.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/containers/image/v5/internal/blobinfocache"
 	"github.com/containers/image/v5/internal/private"
 	"github.com/containers/image/v5/types"
 )
@@ -39,7 +38,7 @@ func (stub NoPutBlobPartialInitialize) SupportsPutBlobPartial() bool {
 // It is available only if SupportsPutBlobPartial().
 // Even if SupportsPutBlobPartial() returns true, the call can fail, in which case the caller
 // should fall back to PutBlobWithOptions.
-func (stub NoPutBlobPartialInitialize) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, cache blobinfocache.BlobInfoCache2) (private.UploadedBlob, error) {
+func (stub NoPutBlobPartialInitialize) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, options private.PutBlobPartialOptions) (private.UploadedBlob, error) {
 	return private.UploadedBlob{}, fmt.Errorf("internal error: PutBlobPartial is not supported by the %q transport", stub.transportName)
 }
 

internal/private/private.go

@@ -55,7 +55,7 @@ type ImageDestinationInternalOnly interface {
 	// It is available only if SupportsPutBlobPartial().
 	// Even if SupportsPutBlobPartial() returns true, the call can fail, in which case the caller
 	// should fall back to PutBlobWithOptions.
-	PutBlobPartial(ctx context.Context, chunkAccessor BlobChunkAccessor, srcInfo types.BlobInfo, cache blobinfocache.BlobInfoCache2) (UploadedBlob, error)
+	PutBlobPartial(ctx context.Context, chunkAccessor BlobChunkAccessor, srcInfo types.BlobInfo, options PutBlobPartialOptions) (UploadedBlob, error)
 
 	// TryReusingBlobWithOptions checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination
 	// (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree).
@@ -100,6 +100,12 @@ type PutBlobOptions struct {
 	LayerIndex *int // If the blob is a layer, a zero-based index of the layer within the image; nil otherwise.
 }
 
+// PutBlobPartialOptions are used in PutBlobPartial.
+type PutBlobPartialOptions struct {
+	Cache      blobinfocache.BlobInfoCache2 // Cache to use and/or update.
+	LayerIndex int                          // A zero-based index of the layer within the image (PutBlobPartial is only called with layer-like blobs, not configs)
+}
+
 // TryReusingBlobOptions are used in TryReusingBlobWithOptions.
 type TryReusingBlobOptions struct {
 	Cache blobinfocache.BlobInfoCache2 // Cache to use and/or update.
@@ -118,7 +124,7 @@ type TryReusingBlobOptions struct {
 	PossibleManifestFormats []string               // A set of possible manifest formats; at least one should support the reused layer blob.
 	RequiredCompression     *compression.Algorithm // If set, reuse blobs with a matching algorithm as per implementations in internal/imagedestination/impl.helpers.go
 	OriginalCompression     *compression.Algorithm // May be nil to indicate “uncompressed” or “unknown”.
-	TOCDigest               *digest.Digest         // If specified, the blob can be looked up in the destination also by its TOC digest.
+	TOCDigest               digest.Digest          // If specified, the blob can be looked up in the destination also by its TOC digest.
 }
 
 // ReusedBlob is information about a blob reused in a destination.
@@ -130,6 +136,8 @@ type ReusedBlob struct {
 	// a differently-compressed blob.
 	CompressionOperation types.LayerCompression // Compress/Decompress, matching the reused blob; PreserveOriginal if N/A
 	CompressionAlgorithm *compression.Algorithm // Algorithm if compressed, nil if decompressed or N/A
+
+	MatchedByTOCDigest bool // Whether the layer was reused/matched by TOC digest. Used only for UI purposes.
 }
 
 // ImageSourceChunk is a portion of a blob.

manifest/oci.go

@@ -9,7 +9,6 @@ import (
 	compressiontypes "github.com/containers/image/v5/pkg/compression/types"
 	"github.com/containers/image/v5/types"
 	ociencspec "github.com/containers/ocicrypt/spec"
-	chunkedToc "github.com/containers/storage/pkg/chunked/toc"
 	"github.com/opencontainers/go-digest"
 	"github.com/opencontainers/image-spec/specs-go"
 	imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
@@ -260,44 +259,9 @@ func (m *OCI1) ImageID(diffIDs []digest.Digest) (string, error) {
 	if err := m.Config.Digest.Validate(); err != nil {
 		return "", err
 	}
-
-	// If there is any layer that is using partial content, we calculate the image ID
-	// in a different way since the diffID cannot be validated as for regular pulled images.
-	for _, layer := range m.Layers {
-		toc, err := chunkedToc.GetTOCDigest(layer.Annotations)
-		if err != nil {
-			return "", fmt.Errorf("error looking up annotation for layer %q: %w", layer.Digest, err)
-		}
-		if toc != nil {
-			return m.calculateImageIDForPartialImage(diffIDs)
-		}
-	}
-
 	return m.Config.Digest.Hex(), nil
 }
 
-func (m *OCI1) calculateImageIDForPartialImage(diffIDs []digest.Digest) (string, error) {
-	newID := digest.Canonical.Digester()
-	for i, layer := range m.Layers {
-		diffID := diffIDs[i]
-		_, err := newID.Hash().Write([]byte(diffID.Hex()))
-		if err != nil {
-			return "", fmt.Errorf("error writing diffID %q: %w", diffID, err)
-		}
-		toc, err := chunkedToc.GetTOCDigest(layer.Annotations)
-		if err != nil {
-			return "", fmt.Errorf("error looking up annotation for layer %q: %w", layer.Digest, err)
-		}
-		if toc != nil {
-			_, err = newID.Hash().Write([]byte(toc.Hex()))
-			if err != nil {
-				return "", fmt.Errorf("error writing TOC %q: %w", toc, err)
-			}
-		}
-	}
-	return newID.Digest().Hex(), nil
-}
-
 // CanChangeLayerCompression returns true if we can compress/decompress layers with mimeType in the current image
 // (and the code can handle that).
 // NOTE: Even if this returns true, the relevant format might not accept all compression algorithms; the set of accepted

oci/archive/oci_dest.go

@@ -6,7 +6,6 @@ import (
 	"io"
 	"os"
 
-	"github.com/containers/image/v5/internal/blobinfocache"
 	"github.com/containers/image/v5/internal/imagedestination"
 	"github.com/containers/image/v5/internal/imagedestination/impl"
 	"github.com/containers/image/v5/internal/private"
@@ -120,8 +119,8 @@ func (d *ociArchiveImageDestination) PutBlobWithOptions(ctx context.Context, str
 // It is available only if SupportsPutBlobPartial().
 // Even if SupportsPutBlobPartial() returns true, the call can fail, in which case the caller
 // should fall back to PutBlobWithOptions.
-func (d *ociArchiveImageDestination) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, cache blobinfocache.BlobInfoCache2) (private.UploadedBlob, error) {
-	return d.unpackedDest.PutBlobPartial(ctx, chunkAccessor, srcInfo, cache)
+func (d *ociArchiveImageDestination) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, options private.PutBlobPartialOptions) (private.UploadedBlob, error) {
+	return d.unpackedDest.PutBlobPartial(ctx, chunkAccessor, srcInfo, options)
 }
 
 // TryReusingBlobWithOptions checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination

openshift/openshift_dest.go

@@ -12,7 +12,6 @@ import (
 
 	"github.com/containers/image/v5/docker"
 	"github.com/containers/image/v5/docker/reference"
-	"github.com/containers/image/v5/internal/blobinfocache"
 	"github.com/containers/image/v5/internal/imagedestination"
 	"github.com/containers/image/v5/internal/imagedestination/impl"
 	"github.com/containers/image/v5/internal/imagedestination/stubs"
@@ -128,8 +127,8 @@ func (d *openshiftImageDestination) PutBlobWithOptions(ctx context.Context, stre
 // It is available only if SupportsPutBlobPartial().
 // Even if SupportsPutBlobPartial() returns true, the call can fail, in which case the caller
 // should fall back to PutBlobWithOptions.
-func (d *openshiftImageDestination) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, cache blobinfocache.BlobInfoCache2) (private.UploadedBlob, error) {
-	return d.docker.PutBlobPartial(ctx, chunkAccessor, srcInfo, cache)
+func (d *openshiftImageDestination) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, options private.PutBlobPartialOptions) (private.UploadedBlob, error) {
+	return d.docker.PutBlobPartial(ctx, chunkAccessor, srcInfo, options)
 }
 
 // TryReusingBlobWithOptions checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination

pkg/blobcache/dest.go

@@ -9,7 +9,6 @@ import (
 	"path/filepath"
 	"sync"
 
-	"github.com/containers/image/v5/internal/blobinfocache"
 	"github.com/containers/image/v5/internal/imagedestination"
 	"github.com/containers/image/v5/internal/imagedestination/impl"
 	"github.com/containers/image/v5/internal/private"
@@ -227,8 +226,8 @@ func (d *blobCacheDestination) SupportsPutBlobPartial() bool {
 // It is available only if SupportsPutBlobPartial().
 // Even if SupportsPutBlobPartial() returns true, the call can fail, in which case the caller
 // should fall back to PutBlobWithOptions.
-func (d *blobCacheDestination) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, cache blobinfocache.BlobInfoCache2) (private.UploadedBlob, error) {
-	return d.destination.PutBlobPartial(ctx, chunkAccessor, srcInfo, cache)
+func (d *blobCacheDestination) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, options private.PutBlobPartialOptions) (private.UploadedBlob, error) {
+	return d.destination.PutBlobPartial(ctx, chunkAccessor, srcInfo, options)
 }
 
 // TryReusingBlobWithOptions checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination