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.5] retry request on writer reset #7479

Merged
merged 2 commits into from Nov 17, 2022

Conversation

akhilerm
Copy link
Member

@akhilerm akhilerm commented Oct 5, 2022

This is not a clean cherry-pick of #6995
cherry-pick #7473 (clean / fix )

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

@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 akhilerm changed the title retry request on writer reset [release/1.5] retry request on writer reset Oct 5, 2022
@akhilerm
Copy link
Member Author

akhilerm commented Oct 5, 2022

/cc @dmcgowan @mikebrow

@akhilerm akhilerm force-pushed the cherry-pick-6995-1.5 branch 2 times, most recently from 637ff9b to fd0f406 Compare October 5, 2022 13:36
@estesp
Copy link
Member

estesp commented Oct 6, 2022

I don't know how to fix the overall status, but this PR has passed CI. I found 2 PRs with no GH Actions status from 11+ hours ago--both were in some hung/pending state with no jobs ever run. I had to manually use the actions management view to cancel both of them and then do a "re-run all jobs"; after that all jobs ran and passed, but the "CI / Build failed" entry won't go away and shows a red X for "Run manually canceled".

@akhilerm
Copy link
Member Author

akhilerm commented Oct 6, 2022

@estesp Arent the vagrant box tests part of 1.5? Also I will try force pushing to see if that will retrigger the CI.

Also, can I get an ok-to-test on this PR?

@samuelkarp
Copy link
Member

/ok-to-test

@akhilerm
Copy link
Member Author

@dmcgowan @mikebrow Can you review this PR?

@akhilerm
Copy link
Member Author

@kzys @samuelkarp Will you be able to look into this PR?

@dmcgowan dmcgowan added this to Ready For Review in Code Review Oct 27, 2022
@dmcgowan dmcgowan self-assigned this Oct 27, 2022
@dmcgowan
Copy link
Member

dmcgowan commented Nov 3, 2022

/retest

@mikebrow
Copy link
Member

mikebrow commented Nov 3, 2022

/test pull-containerd-node-e2e

2 similar comments
@akhilerm
Copy link
Member Author

akhilerm commented Nov 4, 2022

/test pull-containerd-node-e2e

@akhilerm
Copy link
Member Author

akhilerm commented Nov 5, 2022

/test pull-containerd-node-e2e

akhilerm and others added 2 commits November 7, 2022 12:00
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>
Signed-off-by: rongfu.leng <rongfu.leng@daocloud.io>
(cherry picked from commit 63a7d8a)
Signed-off-by: Akhil Mohan <makhil@vmware.com>
@kzys
Copy link
Member

kzys commented Nov 8, 2022

/retest

@akhilerm
Copy link
Member Author

/test pull-containerd-node-e2e-1-5

1 similar comment
@akhilerm
Copy link
Member Author

/test pull-containerd-node-e2e-1-5

@k8s-ci-robot
Copy link

k8s-ci-robot commented Nov 16, 2022

@akhilerm: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-containerd-node-e2e ac1fbc1 link true /test pull-containerd-node-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@akhilerm
Copy link
Member Author

/retest

@akhilerm
Copy link
Member Author

akhilerm commented Nov 16, 2022

@dmcgowan @samuelkarp Can this PR be merged now? The failure of pull-containerd-node-e2e can be ignored as it is no longer run for the 1.5 branch. The new job pull-containerd-node-e2e-1-5 will be used instead

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

@estesp estesp merged commit f16293a into containerd:release/1.5 Nov 17, 2022
Code Review automation moved this from Ready For Review to Done Nov 17, 2022
@akhilerm akhilerm deleted the cherry-pick-6995-1.5 branch November 17, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants