Skip to content

Conversation

@staugust
Copy link

@staugust staugust commented Apr 9, 2021

Ⅰ. Describe what this PR did

Make pouch stop -t 0 ${cid} behaves as its help message describes, that is to say stop container immediately.
See https://github.com/moby/moby/blob/68bec0fcf7a5eeb59c027287d06598098edc9f2c/daemon/stop.go#L71

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

Here's command to verify it:

cid=$(pouch create -it --net=host --entrypoint=cat ubuntu:bionic)
pouch start ${cid}
time pouch stop -t 0 ${cid}

For now, pouch stop costs about 10 seconds to stop the container that doesn't response to sig-term. With this commit, pouch stops container immediately.

Ⅴ. Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@staugust staugust reopened this Apr 9, 2021
@rudyfly rudyfly requested a review from cxz66666 July 7, 2022 11:43
return nil
}

if timeout == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can have a look at ProbeContainer function, if timeout is 0, it will never return errtypes.ErrTimeout message, but you can see ctrd/containerd.go L455, which only call SIGKILL when get errtypes.ErrTimeout.
In other words, if just delete this code directly, it will never delete those containers which ignore SIGTERM singal!

@cxz66666
Copy link
Collaborator

I already add a pr which add kill command for pouch, after it merge you can simply use pouch kill [id] to stop a container immediately!
So I will close this PR

@cxz66666 cxz66666 closed this Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants