From 4b99d024efde2c8a561c625f9d677adcb10d0f41 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 30 Sep 2024 10:04:18 +0200 Subject: [PATCH] chunked: define error for partial pulls not available define a new error type so that the caller can determine whether it is safe to ignore the error and retrieve the resource fully. Closes: https://github.com/containers/storage/issues/2115 Signed-off-by: Giuseppe Scrivano --- pkg/chunked/storage.go | 19 ++++++++++ pkg/chunked/storage_linux.go | 68 ++++++++++++++++++++++++++---------- 2 files changed, 69 insertions(+), 18 deletions(-) diff --git a/pkg/chunked/storage.go b/pkg/chunked/storage.go index 752ee25200..cc9ee85bf0 100644 --- a/pkg/chunked/storage.go +++ b/pkg/chunked/storage.go @@ -23,3 +23,22 @@ type ErrBadRequest struct { //nolint: errname func (e ErrBadRequest) Error() string { return "bad request" } + +// ErrFallbackToOrdinaryLayerDownload is a custom error type that +// suggests to the caller that a fallback mechanism can be used +// instead of a hard failure. +type ErrFallbackToOrdinaryLayerDownload struct { + Err error +} + +func (c ErrFallbackToOrdinaryLayerDownload) Error() string { + return c.Err.Error() +} + +func (c ErrFallbackToOrdinaryLayerDownload) Unwrap() error { + return c.Err +} + +func newErrFallbackToOrdinaryLayerDownload(err error) error { + return ErrFallbackToOrdinaryLayerDownload{Err: err} +} diff --git a/pkg/chunked/storage_linux.go b/pkg/chunked/storage_linux.go index 60cada2cc5..689a163062 100644 --- a/pkg/chunked/storage_linux.go +++ b/pkg/chunked/storage_linux.go @@ -96,6 +96,10 @@ type chunkedDiffer struct { useFsVerity graphdriver.DifferFsVerity fsVerityDigests map[string]string fsVerityMutex sync.Mutex + + // canFallback is set to true if the pull operation can be retried with a different + // method. + canFallback bool } var xattrsToIgnore = map[string]interface{}{ @@ -143,11 +147,13 @@ func (c *chunkedDiffer) convertTarToZstdChunked(destDirectory string, payload *o } // GetDiffer returns a differ than can be used with ApplyDiffWithDiffer. +// If it returns an error that implements IsErrFallbackToOrdinaryLayerDownload, the caller can +// retry the operation with a different method. func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Digest, blobSize int64, annotations map[string]string, iss ImageSourceSeekable) (graphdriver.Differ, error) { pullOptions := store.PullOptions() if !parseBooleanPullOption(pullOptions, "enable_partial_images", true) { - return nil, errors.New("enable_partial_images not configured") + return nil, newErrFallbackToOrdinaryLayerDownload(errors.New("partial images are disabled")) } zstdChunkedTOCDigestString, hasZstdChunkedTOC := annotations[internal.ManifestChecksumKey] @@ -157,29 +163,50 @@ func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Diges return nil, errors.New("both zstd:chunked and eStargz TOC found") } + convertImages := parseBooleanPullOption(pullOptions, "convert_images", false) + + if !hasZstdChunkedTOC && !hasEstargzTOC && !convertImages { + return nil, newErrFallbackToOrdinaryLayerDownload(errors.New("no TOC found and convert_images is not configured")) + } + + // The fallback mechanism must be avoided when convert_images is enabled. + canUseFallback := !convertImages + + var err error + var differ graphdriver.Differ + // At this point one of hasZstdChunkedTOC, hasEstargzTOC or convertImages is true. if hasZstdChunkedTOC { - zstdChunkedTOCDigest, err := digest.Parse(zstdChunkedTOCDigestString) - if err != nil { - return nil, fmt.Errorf("parsing zstd:chunked TOC digest %q: %w", zstdChunkedTOCDigestString, err) + zstdChunkedTOCDigest, err2 := digest.Parse(zstdChunkedTOCDigestString) + if err2 != nil { + return nil, err2 } - return makeZstdChunkedDiffer(store, blobSize, zstdChunkedTOCDigest, annotations, iss, pullOptions) - } - if hasEstargzTOC { - estargzTOCDigest, err := digest.Parse(estargzTOCDigestString) - if err != nil { - return nil, fmt.Errorf("parsing estargz TOC digest %q: %w", estargzTOCDigestString, err) + differ, err = makeZstdChunkedDiffer(store, blobSize, zstdChunkedTOCDigest, annotations, iss, pullOptions, canUseFallback) + if err == nil { + logrus.Debugf("Created zstd:chunked differ for blob %q", blobDigest) + return differ, err + } + } else if hasEstargzTOC { + estargzTOCDigest, err2 := digest.Parse(estargzTOCDigestString) + if err2 != nil { + return nil, err + } + differ, err = makeEstargzChunkedDiffer(store, blobSize, estargzTOCDigest, iss, pullOptions, canUseFallback) + if err == nil { + logrus.Debugf("Created eStargz differ for blob %q", blobDigest) + return differ, err } - return makeEstargzChunkedDiffer(store, blobSize, estargzTOCDigest, iss, pullOptions) + } + // If convert_images is enabled, always attempt to convert it instead of returning an error or falling back to a different method. + if convertImages { + logrus.Debugf("Created differ to convert blob %q", blobDigest) + return makeConvertFromRawDiffer(store, blobDigest, blobSize, iss, pullOptions) } - return makeConvertFromRawDiffer(store, blobDigest, blobSize, iss, pullOptions) + logrus.Debugf("Could not create differ for blob %q: %v", blobDigest, err) + return nil, err } func makeConvertFromRawDiffer(store storage.Store, blobDigest digest.Digest, blobSize int64, iss ImageSourceSeekable, pullOptions map[string]string) (*chunkedDiffer, error) { - if !parseBooleanPullOption(pullOptions, "convert_images", false) { - return nil, errors.New("convert_images not configured") - } - layersCache, err := getLayersCache(store) if err != nil { return nil, err @@ -197,7 +224,7 @@ func makeConvertFromRawDiffer(store storage.Store, blobDigest digest.Digest, blo }, nil } -func makeZstdChunkedDiffer(store storage.Store, blobSize int64, tocDigest digest.Digest, annotations map[string]string, iss ImageSourceSeekable, pullOptions map[string]string) (*chunkedDiffer, error) { +func makeZstdChunkedDiffer(store storage.Store, blobSize int64, tocDigest digest.Digest, annotations map[string]string, iss ImageSourceSeekable, pullOptions map[string]string, canFallback bool) (*chunkedDiffer, error) { manifest, toc, tarSplit, tocOffset, err := readZstdChunkedManifest(iss, tocDigest, annotations) if err != nil { return nil, fmt.Errorf("read zstd:chunked manifest: %w", err) @@ -220,10 +247,11 @@ func makeZstdChunkedDiffer(store storage.Store, blobSize int64, tocDigest digest stream: iss, tarSplit: tarSplit, tocOffset: tocOffset, + canFallback: canFallback, }, nil } -func makeEstargzChunkedDiffer(store storage.Store, blobSize int64, tocDigest digest.Digest, iss ImageSourceSeekable, pullOptions map[string]string) (*chunkedDiffer, error) { +func makeEstargzChunkedDiffer(store storage.Store, blobSize int64, tocDigest digest.Digest, iss ImageSourceSeekable, pullOptions map[string]string, canFallback bool) (*chunkedDiffer, error) { manifest, tocOffset, err := readEstargzChunkedManifest(iss, blobSize, tocDigest) if err != nil { return nil, fmt.Errorf("read zstd:chunked manifest: %w", err) @@ -244,6 +272,7 @@ func makeEstargzChunkedDiffer(store storage.Store, blobSize int64, tocDigest dig pullOptions: pullOptions, stream: iss, tocOffset: tocOffset, + canFallback: canFallback, }, nil } @@ -949,6 +978,9 @@ func (c *chunkedDiffer) retrieveMissingFiles(stream ImageSourceSeekable, dirfd i if _, ok := err.(ErrBadRequest); ok { // If the server cannot handle at least 64 chunks in a single request, just give up. if len(chunksToRequest) < 64 { + if c.canFallback { + return newErrFallbackToOrdinaryLayerDownload(err) + } return err }