-
Notifications
You must be signed in to change notification settings - Fork 191
Enable force kill container with SIGKILL if timeout #559
Conversation
enable force kill after graceful time Signed-off-by: Crazykev <[email protected]>
Signed-off-by: HaoZhang <[email protected]>
Signed-off-by: HaoZhang <[email protected]>
|
||
future := utils.NewFutureSet() | ||
waitTime := time.Duration(graceful) * time.Second | ||
if graceful == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, if graceful == 0
, we should not wait, and fire force kill directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, if graceful == 0
, it is force kill directly:
...
forceKill := graceful == 0
resChan := p.sandbox.WaitProcess(true, []string{c.Id()}, -1)
err := c.terminate(forceKill)
...
This waitTime
is the time we wait for container finish event before abort this mission(or try it with SIGKILL in non-zero graceful
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, do we need wait 5 seconds and kill again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If graceful == 0
, it just wait 5s and won't kill again(return timeout error); if kill with image specified signal(graceful != 0), it will wait graceful time to kill with SIGKILL again.
Is this 5 seconds too long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If graceful == 0
, does this mean the user don't want to wait anything, just return at once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop if we use docker:
$ docker stop --help
Usage: docker stop [OPTIONS] CONTAINER [CONTAINER...]
Stop a running container.
Sending SIGTERM and then SIGKILL after a grace period
--help Print usage
-t, --time=10 Seconds to wait for stop before killing it
@laijs could you review this patch? Its size is far beyond what I imagined before, but looks good except for the |
LGTM |
ref: #549
stopContainers()
to enable force kill container after timeout, change the way it works, originalgraceful
guaranteestopContianers()
will return in graceful time, now ensure container have graceful time to stop with default signal before forcibly kill it.cMap
andeMap
, since we don't need it in current approach(Am I correct?)StopContainer
grpc api since CRI need it(/cc @feiskyer )