shimv2: fix the error of reaping qemu process mistakenly #964
Conversation
|
/test |
containerd-shim-v2/service.go
Outdated
| for i := 0; i < elem.NumField(); i++ { | ||
| f := elem.Field(i) | ||
| switch typeOfT.Field(i).Name { | ||
| case "address": |
There was a problem hiding this comment.
This might be a bit "too clever" ;) Since those struct elements are private, they could change and should be able to change without breaking any clients. But we would be broken here if they changed if I'm understanding this correctly?
There was a problem hiding this comment.
Yes, you are right. If they change the struct's field name, here it will fail to get the "address" and "containerdBinaryPath" value. If containerd exported this type, it will be easy for us.
There was a problem hiding this comment.
It might be worth raising a PR on containerd shim to see if they would be prepared to make these fields public.
There was a problem hiding this comment.
Yes, to fix this issue gently there must be some adjustments in containerd side, by now I think we can workaround it in our side first and then we communicate with containerd community on how to fix it ultimately.
There was a problem hiding this comment.
I'd really appreciate if we could start the conversation with containerd community right away to get a hint at how they feel regarding this proposal. I don't want to merge to Kata Containers a workaround that might never be properly fixed if containerd community does not agree on these changes.
Please start the discussion with them by adding @jodh-intel and myself in copy!
There was a problem hiding this comment.
Agree. Please open an issue with containerd for this @lifupan.
|
@jodh-intel @sboeuf I had opened an issue in containerd, please see containerd/containerd#2855 |
|
thanks @lifupan |
Signed-off-by: fupan <lifupan@gmail.com>
For kata shimv2, the sub-reaper isn't needed, otherwise it will break the cmd.Run() calling in govmmQemu.LaunchQemu(). Fixes: kata-containers#939 Signed-off-by: fupan <lifupan@gmail.com>
lifupan
force-pushed
the
fixchildreaped
branch
from
a853bfa to
aa2611b
Compare
|
close this PR, since I had opened another one: #1004 |
For kata shimv2, it doesn't need to reap it's container processes
since all of them are running in VM, thus there is no need to
set it as a subreaper and also no need to take care of the SIGCHLD
singal.
So this commit will try to unset the subreaper and ignore the SIGCHLD
signal. Since ignoring the SIGCHLD signal will break the containerd
shim's events.Publisher publish method, thus here re-write a publish
function to publish the events to containerd.
Fixes: #939
Signed-off-by: fupan lifupan@gmail.com