fix retry when writer is reset on push #6995
Conversation
|
Hi @akhilerm. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
cc: @dmcgowan updated the changes in the PR as discussed. Also added a reference to a commit that is used for testing with docker config. |
be51db7 to
62854c4
Compare
|
@dmcgowan @tonistiigi Can you take a look at this PR? |
|
/ok-to-test |
d4f1ad6 to
dd59f72
Compare
endocrimes
left a comment
There was a problem hiding this comment.
first pass makes sense - but lack of tests make it somewhat difficult to have confidence.
The main Copy control loop definitely needs a bit of tweaking for readability, but logic seems ~ok.
| tracker StatusTracker | ||
| } | ||
|
|
||
| func newPushWriter(db *dockerBase, ref string, expected digest.Digest, tracker StatusTracker, isManifest bool) *pushWriter { |
There was a problem hiding this comment.
the pushWriter is getting complex enough that it might be worth trying to write some basic tests for, partially to document intent.
There was a problem hiding this comment.
Added test cases for both push() and Copy() method
b868c2e to
50609e0
Compare
5b9f1a2 to
87ccc2f
Compare
2b86f2c to
b309c6c
Compare
when a put request is retried due to the response from registry, the body of the request should be seekable. A dynamic pipe is added to the body so that the content of the body can be read again. Currently a maximum of 5 resets are allowed, above which will fail the request. A new error ErrReset is introduced which informs that a reset has occured and request needs to be retried. also added tests for Copy() and push() to test the new functionality Signed-off-by: Akhil Mohan <makhil@vmware.com>
b309c6c to
8f4c23b
Compare
|
@mikebrow Can you review this PR once more? |
|
Should this change be cherry-picked to 1.6 / 1.5 ? |
Yes, I think so. This is a long-standing bug that has affected quite a few users so should be cherry-picked to our supported releases. |
A dynamic pipe is created in the body of the request, so that everytime a
GetBody()call is made, a new pipe gets generated to which the content can be written, and the http body can be read from the PipeReader, thus solving the body reuse issue.Fixes: #5978