Bug 1885179: Avoid deadlock by closing pipe reader end #604
Conversation
|
@ricardomaraschini: An error was encountered adding this pull request to the external tracker bugs for bug 1885179 on the Bugzilla server at https://bugzilla.redhat.com:
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/severity-medium
|
@dmage: This pull request references Bugzilla bug 1885179, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @sallyom |
| @@ -52,7 +52,7 @@ func DigestCopy(dst io.ReaderFrom, src io.Reader) (layerDigest, blobDigest diges | |||
| defer close(ch) | |||
| gr, err := gzip.NewReader(pr) | |||
| if err != nil { | |||
| ch <- fmt.Errorf("unable to create gzip reader layer upload: %v", err) | |||
| pr.CloseWithError(fmt.Errorf("unable to create gzip reader layer upload: %v", err)) | |||
There was a problem hiding this comment.
My only concern is that gzip.NewReader may have already read all data from pr at this point (for example, when there were less than 10 bytes to read). In this case this error won't be detected as nothing will write into the writer.
There was a problem hiding this comment.
Yes, you are absolutely right. I think it makes sense to still send the error down through that error channel. I have updated this to just close the pipe reader end with an error.
If we don't close this pipe reader with an error we ended up having a
deadlock as we keep trying to read indefinitely from it. This deadlock
causes the error message to be misguiding when attempting to upload a
layer with invalid format (gzip: invalid header).
Without this patch oc image append using an invalid layer format fails
with either "timeout" (docker.io) or "protocol error" (quay.io) instead
of reporting appropriate error ("invalid gzip").
|
/retest |
|
thanks @ricardomaraschini and @dmage /lgtm |
|
and I've confirmed the desired behavior: $ oc image append --from=quay.io/sallyom/testcli:test --to=quay.io/sallyom/testcli:append new-layer.tar
Uploading ... error: unable to upload new layer (0): Patch "https://quay.io/v2/sallyom/testcli/blobs/uploads/ab100e66-f2c6-4c8a-b4dc-b326d6c563fb": unable to create gzip reader layer upload: gzip: invalid headerand from master (and after a minute of waiting...): $ oc image append --from=quay.io/sallyom/testcli:test --to=quay.io/sallyom/testcli:append new-layer.tar
Uploading ... error: unable to upload new layer (0): Patch "https://quay.io/v2/sallyom/testcli/blobs/uploads/b438f781-3070-45b2-8164-8dc14eba9c19": stream error: stream ID 15; PROTOCOL_ERROR |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ricardomaraschini, sallyom, soltysh 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@ricardomaraschini: All pull requests linked via external trackers have merged: Bugzilla bug 1885179 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
If we don't close this pipe reader with an error we ended up having a
deadlock as we keep trying to read indefinitely from it. This deadlock
causes the error message to be misguiding when attempting to upload a
layer with invalid format (gzip: invalid header).
Without this patch oc image append using an invalid layer format fails
with either "timeout" (docker.io) or "protocol error" (quay.io) instead
of reporting appropriate error ("invalid gzip").