CRI: Fix StopContainer timeout #44970
Conversation
|
This change is |
|
@k8s-bot unit test this |
pkg/kubelet/remote/remote_runtime.go
Outdated
| ctx, cancel := getContextWithTimeout(time.Duration(timeout) * time.Second) | ||
| if timeout == 0 { | ||
| // Do not set timeout when timeout is 0. | ||
| ctx, cancel = getContextWithCancel() |
There was a problem hiding this comment.
Shouldn't the logic is if timeout==0 for StopContainer, we set the default timeout value? The default one can be r.timeout here
ae0b912 to
5717b77
Compare
|
Since we have this regression for StopContainer, can we make sure there is no regression for other docker client calls? Especially for docker pull? |
5717b77 to
cfd0eff
Compare
@dchen1107 I went through all CRI functions in CRI client, timeout of other operations are set properly. |
|
@derekwaynecarr @sttts need to pick this |
|
/assign |
|
LGTM. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Random-Liu, feiskyer
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
@Random-Liu I think you should add a release note for this. |
|
Automatic merge from submit-queue (batch tested with PRs 44970, 43618) |
|
We need a test :( |
Automatic merge from submit-queue Automated cherry pick of #44970 Automated cherry pick of #44970: Fix StopContainer timeout. ```release-note CRI: respect container's stopping timeout. ``` cc @Random-Liu @dchen1107
|
Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
Fixes #44956.
I verified this PR with the example provided in #44956, and now pod deletion will respect grace period timeout:
@dchen1107 @yujuhong @feiskyer /cc @kubernetes/sig-node-bugs