Don't silently ignore errors determining size in TryReuseBlob #2709
Conversation
76f61da to
e473d28
Compare
2a3536a to
50a8bb2
Compare
e7a85f9 to
683e3b9
Compare
|
|
||
| size, err := getBlobSize(resp) | ||
| if err != nil { | ||
| size = -1 |
There was a problem hiding this comment.
Should we at least logrus.Debug the error?
There was a problem hiding this comment.
In both cases, GetBlob is explicitly allowed to return -1, so we wouldn’t expect users to need to diagnose why this is happening. Typically, the size is only used to sanity-check sizes of uploads, but the digest verification should serve the same purpose anyway.
The only adverse effect I could find is that writes to docker-daemon or docker-archive might need to store a file to disk if the size is unknown, and the input is uncompressed (if the input is compressed, the transports need to store it to disk in either case).
I don’t think this is really worth worrying about, and probably not worth the log noise, but I don’t feel strongly about this at all.
| blobSize := getBlobSize(res) | ||
| blobSize, err := getBlobSize(res) | ||
| if err != nil { | ||
| blobSize = -1 |
|
LGTM |
When looking for inexact matches, this will cause the matches to be skipped. When checking for an exact match, this will cause an upload failure; we don't have any other way to handle pre-existing blobs on the destination. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Returning the size required by the
TryReuseBlob/PutBlobAPI; otherwise we could put-1into manifests.For now, completely untested.