chunked: define error for partial pulls not available

[giuseppe]

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: #2115

the relative patch for containers/image is something like:

diff --git a/copy/single.go b/copy/single.go
index 324785a8..b7921837 100644
--- a/copy/single.go
+++ b/copy/single.go
@@ -819,8 +819,13 @@ 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.
+			if chunkedToc.IsPartialContentNotAvailableError(err) {
+				logrus.Debugf("Failed to retrieve partial blob: %v", err)
+				err = nil
+			}
+			return false, types.BlobInfo{}, err
+
 		}()
 		if err != nil {
 			return types.BlobInfo{}, "", err

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

LGTM
@nalind @Luap99 @mtrmac @mheon @vrothberg @saschagrunert PTAL

[mtrmac]

Starting from the c/image consumer, I’d prefer copy/single.go to not check for a specific c/storage error; c/image should define its own (private) error type for PutBlobPartial.

That, in turn, means that the c/storage code calling GetDiffer is going to already have a dependency on pkg/chunked; so we can put the error definition directly into pkg/chunked, and avoid the extra infrastructure to put it into chunked/internal+chunked/toc.

---

#2115 talks about adding a “require the GetDiffer code path to be used” option for fs-verity. We don’t need to do that right now, but how would that be added here?

It seems to me that we need a wrapping / unwrappable error mechanism (see underlying error in one of the comments), and then something like

func GetDiffer {
   …
   differ, err := getDifferInternal()
   if fallbackMarker, ok := errIsTheFallbackMarker(err); ok && fallbackDisabled {
      err = fallbackMarker.Unwrap() // no fallback, hard failure per configuration
   }
   …
}

with getDifferInternal directly, or some of its callees, adding the marker on relevant code paths.

[mtrmac]

#2115 (comment) talks about the case where a registry does not support ranged requests; that should be detected somewhere in makeZstdChunkedDiffer and turned into the “fall back” error. Similarly for the other differ.

[cgwalters]

Starting from the c/image consumer, I’d prefer copy/single.go to not check for a specific c/storage error; c/image should define its own (private) error type for PutBlobPartial.

One thing I don't quite understand is the circular relationship between the two projects...are we getting much out of having them be in separate git repositories even?

[mtrmac]

• There are c/image users who don’t use c/storage (registry clients, mirroring tools). In fact that’s the primary point of that concern — a fair bit of the generic c/image logic should not be hard-coding c/storage specifics.
• The relationship is not circular (apart from one callback from c/storage back, but I can remember only that one case where we want to invert control that way)
• The test mechanisms and CI costs are very different (sometimes in bad ways, e.g. how most of c/image tests live in the Skopeo repo); but anyway, paying the registry testing costs for c/storage, or the c/storage filesystem/driver test matrix costs for c/image, is not immediately attractive.

The relationship is not set in stone, but then again there’s a substantial cost to integrate, and no obvious benefit to doing so.

[giuseppe]

#2115 talks about adding a “require the GetDiffer code path to be used” option for fs-verity. We don’t need to do that right now, but how would that be added here?

The flag will probably look like fsverity=disable|auto|require. The advantage with this PR is that /pkg/chunked can finally return an error that causes the pull to fail instead of falling back to the current pull mechanism.

[mtrmac]

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

[giuseppe]

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

[mtrmac]

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

[giuseppe]

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
 }

[mtrmac]

LGTM. Thanks!

[rhatdan]

/lgtm