runtime-v1: kill shim in exit handler#3226
Conversation
runtime/v1/linux/runtime.go
Outdated
There was a problem hiding this comment.
We shouldn't do this in loadTasks, because task is not added to the task list yet, this will always return.
That's why this is kept out side of cleanupAfterDeadShim before.
There was a problem hiding this comment.
Yes, I also aware this, and restore the code.
0a55186 to
f3a8615
Compare
runtime/v1/linux/runtime.go
Outdated
There was a problem hiding this comment.
The exitHandler will only be called when the shim process exits, there is no need to kill it again.
I do agree that the cleanupAfterDeadShim onClose function installed by loadTasks may try killing the shim. But please not that for shim v2 we can no longer do that, because shim v2 implementation may have per-pod shim process, which can't be killed when one container dies.
There was a problem hiding this comment.
I get some time to check the code, and try to explain my logic:
- in exit handler, the normal case should be shim exit and resources be cleanup, but in bad case, still some shim hangover. so I want to add one more kill to shim
- in loadTasks, I do not found the good place to do killShim, we get
shimclientthroughbundle.NewShimClient, but if not get response, we can not doKillShim
runtime/v1/linux/runtime.go
Outdated
There was a problem hiding this comment.
I also aware in loadTasks t.tasks is empty, so we do not need check task available here ? @Random-Liu
There was a problem hiding this comment.
This is called later on close, so we still need to check this.
The error handling below doesn't need to check, because the task is not in list yet.
There was a problem hiding this comment.
Yes, you are right, I miss the point
f3a8615 to
33e35df
Compare
|
Please rebase on |
|
It can be log the shim pid in bundle and kill shim in cleanDeadShim |
1. kill shim in cleanupAfterDeadShim avoid shim leak 2. refactor cleanupAfterDeadShim, get pid from bundle path instead of make pid as a parameter Signed-off-by: Ace-Tang <aceapril@126.com>
33e35df to
dfa51c9
Compare
Codecov Report
@@ Coverage Diff @@
## master #3226 +/- ##
=======================================
Coverage 44.63% 44.63%
=======================================
Files 113 113
Lines 12161 12161
=======================================
Hits 5428 5428
Misses 5898 5898
Partials 835 835
Continue to review full report at Codecov.
|
|
LGTM |
1 similar comment
|
LGTM |
The patch do two things,
refactor cleanupAfterDeadShim function, judge if task is available, if
has, do cleanup.
add kill shim in cleanupDeadShim, in cleanup ,
r.tasks.Delete(ctx, id)will be called, then the task should be removed from task list, so do more killShim function, make less leak shim.Signed-off-by: Ace-Tang aceapril@126.com