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: attempt fallback to full retrieval only when possible #2589

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 8 additions & 3 deletions copy/single.go
Original file line number Diff line number Diff line change
Expand Up @@ -819,11 +819,16 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
logrus.Debugf("Retrieved partial blob %v", srcInfo.Digest)
return true, updatedBlobInfoFromUpload(srcInfo, uploadedBlob), nil
}
logrus.Debugf("Failed to retrieve partial blob: %v", err)
return false, types.BlobInfo{}, nil
// On a "partial content not available" error, ignore it and retrieve the whole layer.
var perr private.ErrFallbackToOrdinaryLayerDownload
if errors.As(err, &perr) {
logrus.Debugf("Failed to retrieve partial blob: %v", err)
return false, types.BlobInfo{}, nil
}
return false, types.BlobInfo{}, err
}()
if err != nil {
return types.BlobInfo{}, "", err
return types.BlobInfo{}, "", fmt.Errorf("reading blob %s: %w", srcInfo.Digest, err)
}
if reused {
return blobInfo, cachedDiffID, nil
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ require (
github.com/BurntSushi/toml v1.4.0
github.com/containers/libtrust v0.0.0-20230121012942-c1716e8a8d01
github.com/containers/ocicrypt v1.2.0
github.com/containers/storage v1.55.1-0.20240930161746-4bf3f075cf3f
github.com/containers/storage v1.55.1-0.20241002203117-0eb3a0231575
github.com/cyberphone/json-canonicalization v0.0.0-20231217050601-ba74d44ecf5f
github.com/distribution/reference v0.6.0
github.com/docker/cli v27.3.1+incompatible
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ github.com/containers/libtrust v0.0.0-20230121012942-c1716e8a8d01 h1:Qzk5C6cYgle
github.com/containers/libtrust v0.0.0-20230121012942-c1716e8a8d01/go.mod h1:9rfv8iPl1ZP7aqh9YA68wnZv2NUDbXdcdPHVz0pFbPY=
github.com/containers/ocicrypt v1.2.0 h1:X14EgRK3xNFvJEfI5O4Qn4T3E25ANudSOZz/sirVuPM=
github.com/containers/ocicrypt v1.2.0/go.mod h1:ZNviigQajtdlxIZGibvblVuIFBKIuUI2M0QM12SD31U=
github.com/containers/storage v1.55.1-0.20240930161746-4bf3f075cf3f h1:Qc/dmzukhCunvppC9sfQ/gC7PBNW5tUhiRqS+ZFp7Ck=
github.com/containers/storage v1.55.1-0.20240930161746-4bf3f075cf3f/go.mod h1:Cu9jwKPeZzPCc1qE/3PmNbL1f3ymm8ltFQVwUxdz+lM=
github.com/containers/storage v1.55.1-0.20241002203117-0eb3a0231575 h1:kTjd9n1IV+6bo+4DN6HbAIEFHpX8U1aRcvjYF/b387Q=
github.com/containers/storage v1.55.1-0.20241002203117-0eb3a0231575/go.mod h1:Cu9jwKPeZzPCc1qE/3PmNbL1f3ymm8ltFQVwUxdz+lM=
github.com/coreos/go-oidc/v3 v3.11.0 h1:Ia3MxdwpSw702YW0xgfmP1GVCMA9aEFWu12XUZ3/OtI=
github.com/coreos/go-oidc/v3 v3.11.0/go.mod h1:gE3LgjOgFoHi9a4ce4/tJczr0Ai2/BoDhf0r5lltWI0=
github.com/cyberphone/json-canonicalization v0.0.0-20231217050601-ba74d44ecf5f h1:eHnXnuK47UlSTOQexbzxAZfekVz6i+LKRdj1CU5DPaM=
Expand Down
5 changes: 3 additions & 2 deletions internal/imagedestination/stubs/put_blob_partial.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ func (stub NoPutBlobPartialInitialize) SupportsPutBlobPartial() bool {
// PutBlobPartial attempts to create a blob using the data that is already present
// at the destination. chunkAccessor is accessed in a non-sequential way to retrieve the missing chunks.
// 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.
// Even if SupportsPutBlobPartial() returns true, the call can fail.
// If the call fails with ErrFallbackToOrdinaryLayerDownload, the caller can fall back to PutBlobWithOptions.
// The fallback _must not_ be done otherwise.
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)
}
Expand Down
24 changes: 22 additions & 2 deletions internal/private/private.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,9 @@ type ImageDestinationInternalOnly interface {
// PutBlobPartial attempts to create a blob using the data that is already present
// at the destination. chunkAccessor is accessed in a non-sequential way to retrieve the missing chunks.
// 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.
// Even if SupportsPutBlobPartial() returns true, the call can fail.
// If the call fails with ErrFallbackToOrdinaryLayerDownload, the caller can fall back to PutBlobWithOptions.
// The fallback _must not_ be done otherwise.
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
Expand Down Expand Up @@ -183,3 +184,22 @@ type UnparsedImage interface {
// UntrustedSignatures is like ImageSource.GetSignaturesWithFormat, but the result is cached; it is OK to call this however often you need.
UntrustedSignatures(ctx context.Context) ([]signature.Signature, error)
}

// ErrFallbackToOrdinaryLayerDownload is a custom error type returned by PutBlobPartial.
// It suggests to the caller that a fallback mechanism can be used instead of a hard failure;
// otherwise the caller of PutBlobPartial _must not_ fall back to PutBlob.
type ErrFallbackToOrdinaryLayerDownload struct {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this error defined here when a similar error is created in c/storage? Is it okay to have two same-defined types? Which would be used if they have different fields? I'm just curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

we don't want the copy package to have a dependency on containers/storage. In this way, it is possible to build containers/image without the storage backend and avoid the vendoring cost

Copy link
Member

Choose a reason for hiding this comment

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

Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Also, it’s not just the technical dependency; c/image/copy strives to contain “generic” code without hard-coding specifics of transports or image formats.

It doesn’t always succeed — we do have some special cases, and really a lot of the private API is specific to the c/storage design and constraints.)

mtrmac marked this conversation as resolved.
Show resolved Hide resolved
err error
}

func (c ErrFallbackToOrdinaryLayerDownload) Error() string {
return c.err.Error()
}

func (c ErrFallbackToOrdinaryLayerDownload) Unwrap() error {
return c.err
}

func NewErrFallbackToOrdinaryLayerDownload(err error) error {
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
return ErrFallbackToOrdinaryLayerDownload{err: err}
}
5 changes: 3 additions & 2 deletions oci/archive/oci_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,9 @@ func (d *ociArchiveImageDestination) PutBlobWithOptions(ctx context.Context, str
// PutBlobPartial attempts to create a blob using the data that is already present
// at the destination. chunkAccessor is accessed in a non-sequential way to retrieve the missing chunks.
// 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.
// Even if SupportsPutBlobPartial() returns true, the call can fail.
// If the call fails with ErrFallbackToOrdinaryLayerDownload, the caller can fall back to PutBlobWithOptions.
// The fallback _must not_ be done otherwise.
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)
}
Expand Down
5 changes: 3 additions & 2 deletions openshift/openshift_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,9 @@ func (d *openshiftImageDestination) PutBlobWithOptions(ctx context.Context, stre
// PutBlobPartial attempts to create a blob using the data that is already present
// at the destination. chunkAccessor is accessed in a non-sequential way to retrieve the missing chunks.
// 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.
// Even if SupportsPutBlobPartial() returns true, the call can fail.
// If the call fails with ErrFallbackToOrdinaryLayerDownload, the caller can fall back to PutBlobWithOptions.
// The fallback _must not_ be done otherwise.
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)
}
Expand Down
5 changes: 3 additions & 2 deletions pkg/blobcache/dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,9 @@ func (d *blobCacheDestination) SupportsPutBlobPartial() bool {
// PutBlobPartial attempts to create a blob using the data that is already present
// at the destination. chunkAccessor is accessed in a non-sequential way to retrieve the missing chunks.
// 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.
// Even if SupportsPutBlobPartial() returns true, the call can fail.
// If the call fails with ErrFallbackToOrdinaryLayerDownload, the caller can fall back to PutBlobWithOptions.
// The fallback _must not_ be done otherwise.
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)
}
Expand Down
14 changes: 11 additions & 3 deletions storage/storage_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,15 +311,23 @@ func (f *zstdFetcher) GetBlobAt(chunks []chunked.ImageSourceChunk) (chan io.Read
// PutBlobPartial attempts to create a blob using the data that is already present
// at the destination. chunkAccessor is accessed in a non-sequential way to retrieve the missing chunks.
// 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 (s *storageImageDestination) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, options private.PutBlobPartialOptions) (private.UploadedBlob, error) {
// Even if SupportsPutBlobPartial() returns true, the call can fail.
// If the call fails with ErrFallbackToOrdinaryLayerDownload, the caller can fall back to PutBlobWithOptions.
// The fallback _must not_ be done otherwise.
func (s *storageImageDestination) PutBlobPartial(ctx context.Context, chunkAccessor private.BlobChunkAccessor, srcInfo types.BlobInfo, options private.PutBlobPartialOptions) (_ private.UploadedBlob, retErr error) {
fetcher := zstdFetcher{
chunkAccessor: chunkAccessor,
ctx: ctx,
blobInfo: srcInfo,
}

defer func() {
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
var perr chunked.ErrFallbackToOrdinaryLayerDownload
if errors.As(retErr, &perr) {
retErr = private.NewErrFallbackToOrdinaryLayerDownload(retErr)
}
}()

differ, err := chunked.GetDiffer(ctx, s.imageRef.transport.store, srcInfo.Digest, srcInfo.Size, srcInfo.Annotations, &fetcher)
if err != nil {
return private.UploadedBlob{}, err
Expand Down