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

chunked: define error for partial pulls not available #2118

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
19 changes: 19 additions & 0 deletions pkg/chunked/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
63 changes: 44 additions & 19 deletions pkg/chunked/storage_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,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) {
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
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]
Expand All @@ -157,29 +159,54 @@ 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"))
}

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)
if err == nil {
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this fails because the registry does not support range requests, nothing marks the failure as eligible for fallback, AFAICS.

Copy link
Member Author

Choose a reason for hiding this comment

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

wouldn't it fail earlier when we request the TOC through a range request? So we should treat it as a failure here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICS we request the TOC inside this function, not earlier. Is that incorrect?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah sorry, yes I got confused.

I've added the following snippet:

diff --git a/pkg/chunked/storage_linux.go b/pkg/chunked/storage_linux.go
index ce8f8a2ce..26e8f1aba 100644
--- a/pkg/chunked/storage_linux.go
+++ b/pkg/chunked/storage_linux.go
@@ -196,6 +196,13 @@ func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Diges
 	}
 
 	logrus.Debugf("Could not create differ for blob %q: %v", blobDigest, err)
+
+	// If the error is a bad request to the server, then signal to the caller that it can try a different method.  This can be done
+	// only when convert_images is disabled.
+	var badRequestErr ErrBadRequest
+	if errors.As(err, &badRequestErr) {
+		err = newErrFallbackToOrdinaryLayerDownload(err)
+	}
 	return nil, err
 }

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)
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)

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")
// If the error is a bad request to the server, then signal to the caller that it can try a different method. This can be done
// only when convert_images is disabled.
var badRequestErr ErrBadRequest
if errors.As(err, &badRequestErr) {
err = newErrFallbackToOrdinaryLayerDownload(err)
}
return nil, err
}

func makeConvertFromRawDiffer(store storage.Store, blobDigest digest.Digest, blobSize int64, iss ImageSourceSeekable, pullOptions map[string]string) (*chunkedDiffer, error) {
layersCache, err := getLayersCache(store)
if err != nil {
return nil, err
Expand Down Expand Up @@ -947,11 +974,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 len(chunksToRequest) == 1 {
return err
}

mtrmac marked this conversation as resolved.
Show resolved Hide resolved
// Merge more chunks to request
missingParts = mergeMissingChunks(missingParts, len(chunksToRequest)/2)
calculateChunksToRequest()
Expand Down