Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

qemu: clear qmp state before wait for qemu process#536

Merged
jodh-intel merged 2 commits intokata-containers:masterfrom
bergwolf:qmp_clear
Aug 1, 2018
Merged

qemu: clear qmp state before wait for qemu process#536
jodh-intel merged 2 commits intokata-containers:masterfrom
bergwolf:qmp_clear

Conversation

@bergwolf
Copy link
Member

So that if there is any remaining state, we do not let it interfere
with the new one. This should fix the occasional vm factory hang.

So that if there is any remaining state, we do not let it interfere
with the new one. This should fix the occasional vm factory hang.

Fixes: kata-containers#535

Signed-off-by: Peng Tao <bergwolf@gmail.com>
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 171629 KB
Proxy: 5920 KB
Shim: 9089 KB

Memory inside container:
Total Memory: 2043480 KB
Free Memory: 2003620 KB

@codecov
Copy link

codecov bot commented Jul 31, 2018

Codecov Report

Merging #536 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
- Coverage   66.67%   66.66%   -0.01%     
==========================================
  Files          93       93              
  Lines        9580     9614      +34     
==========================================
+ Hits         6387     6409      +22     
- Misses       2506     2517      +11     
- Partials      687      688       +1

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 31, 2018

Build succeeded (third-party-check pipeline).

@jodh-intel
Copy link

jodh-intel commented Jul 31, 2018

lgtm

/cc @markdryan.

Approved with PullApprove

@markdryan
Copy link
Contributor

As long this change abides by the constraints in the API documentation it should be okay:

https://github.com/intel/govmm/blob/master/qemu/qmp.go#L591

If it doesn't you might get a panic.

@bergwolf
Copy link
Member Author

@markdryan At the point of hypervisor.waitsandbox(), I don't think anyone should connect/disconnect to the qemu process concurrently -- we wait for the qemu process to be up before actually talk to it in any means.

@markdryan
Copy link
Contributor

@bergwolf Can we be sure no one else has called Shutdown on this instance? If so it should be okay. You could also read from the disconnectedChannel which will block until the connection is truly closed. This is what all the test cases do, e.g., https://github.com/intel/govmm/blob/master/qemu/qmp_test.go#L497

@bergwolf
Copy link
Member Author

@markdryan q.qmpShutdown() checks for nil qmp handler before calling shutdown. So if shutdown is called before, we are still safe. The only case we need to exclude is concurrent connect/disconnect.

@markdryan
Copy link
Contributor

@bergwolf, I'd missed that. I thought you were calling Shutdown directly. Reading from the disconnected channel might still be a good idea though.

That is how govmm ensures us that the qmp channel has been cleaned
up entirely.

Signed-off-by: Peng Tao <bergwolf@gmail.com>
@bergwolf
Copy link
Member Author

@markdryan Yes, you are right. It still makes sense to wait on the disconnected channel. I've added a new commit to do it. PTAL.

@jodh-intel
Copy link

lgtm

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 167749 KB
Proxy: 5667 KB
Shim: 9076 KB

Memory inside container:
Total Memory: 2043480 KB
Free Memory: 2003092 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 31, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@jodh-intel
Copy link

Ignoring 0.01% drop in code cov...

@jodh-intel jodh-intel merged commit 487f9ef into kata-containers:master Aug 1, 2018
@egernst egernst mentioned this pull request Aug 22, 2018
@sboeuf sboeuf added bug Incorrect behaviour stable-candidate labels Sep 12, 2018
@bergwolf bergwolf deleted the qmp_clear branch September 13, 2018 03:27
zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019
…lpha1-branch-bump

# Kata Containers 1.7.0-alpha1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Incorrect behaviour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants