vc/qemu: add mutex to qmp monitor channel in qmpSetup() #2140
Conversation
|
/test thanks @YvesChan |
Codecov Report
@@ Coverage Diff @@
## master #2140 +/- ##
=========================================
Coverage ? 50.95%
=========================================
Files ? 110
Lines ? 15209
Branches ? 0
=========================================
Hits ? 7750
Misses ? 6505
Partials ? 954 |
There was a problem hiding this comment.
Thanks @YvesChan!
Is there any way we can add a test (either a unit test here, or a test in https://github.com/kata-containers/tests) to give us some confidence the race has been fixed?
As I mentioned above, now the containerd-shim-kata-v2 process will hold the qmp.socket all the time, so there's no chance to reproduce the race condition in current runtime version. But it's not very convenient for user to debug the qemu issue if they cannot access qmp.socket since it's held by shim. So it's just better to make qmpSetup() more robust. What if we change the qmp connection to ephemeral(free after used) in the future? who knows :) |
|
/test |
| @@ -921,6 +923,9 @@ func (q *qemu) togglePauseSandbox(pause bool) error { | |||
| } | |||
|
|
|||
| func (q *qemu) qmpSetup() error { | |||
| q.qmpMonitorCh.Lock() | |||
| defer q.qmpMonitorCh.Unlock() | |||
|
|
|||
There was a problem hiding this comment.
If we get parallel qmp setup, is it possible to get parallel qmp shutdown as well?
There was a problem hiding this comment.
Yes, so we have to add this to qmpShutdown too. I'll fix this later. Thanks :)
There was a problem hiding this comment.
ping. The commit message says "Solve possible race condition in qmpSetup() and qmpShutdown()" but the shutdown part seems missing?
There was a problem hiding this comment.
odd... I have already pushed that commit. Anyway, I do it again. Thanks :)
|
/test |
1 similar comment
|
/test |
Solve possible race condition in qmpSetup() and qmpShutdown() Fixes: kata-containers#2139 Signed-off-by: Yves Chan <[email protected]>
|
/test |
1 similar comment
|
/test |
|
This is now ready to land but I'll let @bergwolf click the lovely green button as an ack to the comment about shutdown. |
Solve possible race condition in qmpSetup()
Fixes: #2139
Signed-off-by: Yves Chan [email protected]