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

[release/1.6] retry request on writer reset #7461

Merged
merged 1 commit into from Oct 5, 2022

Conversation

akhilerm
Copy link
Member

@akhilerm akhilerm commented Oct 1, 2022

Cherry-pick (clean) #6995

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
(cherry picked from commit 8f4c23b)
Signed-off-by: Akhil Mohan makhil@vmware.com

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>
(cherry picked from commit 8f4c23b)
Signed-off-by: Akhil Mohan <makhil@vmware.com>
@k8s-ci-robot
Copy link

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@akhilerm
Copy link
Member Author

akhilerm commented Oct 1, 2022

/assign @estesp @dmcgowan

@akhilerm
Copy link
Member Author

akhilerm commented Oct 3, 2022

/cc @dmcgowan @estesp @mikebrow

@akhilerm
Copy link
Member Author

akhilerm commented Oct 3, 2022

/unassign @estesp @dmcgowan

@mikebrow
Copy link
Member

mikebrow commented Oct 3, 2022

/ok-to-test

@mikebrow
Copy link
Member

mikebrow commented Oct 3, 2022

did you encounter any merge issues or was it a clean cherry pick? nvm I checked :-) clean cherry.. added comment to PR text.. Cheers.

@@ -81,6 +151,12 @@ func TestCopy(t *testing.T) {
testcase.source.size,
testcase.source.digest)

// if an error is expected then further comparisons are not required
Copy link
Member Author

Choose a reason for hiding this comment

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

@mikebrow There was one merge conflict while cherry-picking here in the tests. Other than that it was clean.

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@samuelkarp samuelkarp merged commit 6496ac6 into containerd:release/1.6 Oct 5, 2022
@akhilerm akhilerm deleted the cherry-pick-6995-1.6 branch October 5, 2022 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants