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

virtcontainers: keep qmp connection whenever possible#501

Merged
bergwolf merged 3 commits intokata-containers:masterfrom
bergwolf:qemu
Jul 24, 2018
Merged

virtcontainers: keep qmp connection whenever possible#501
bergwolf merged 3 commits intokata-containers:masterfrom
bergwolf:qemu

Conversation

@bergwolf
Copy link
Member

For each time a sandbox structure is created, we ensure s.Release()
is called. Then we can keep the qmp connection as long as Sandbox
pointer is alive.

All VC interfaces are still stateless as s.Release() is called before
each API returns.

OTOH, for VCSandbox APIs, FetchSandbox() must be paired with s.Release,
the same as before.

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 162981 KB
Proxy: 5046 KB
Shim: 8935 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007220 KB

bergwolf added 2 commits July 23, 2018 08:36
Unify qmp channel setup and teardown. This also fixes the issue that
sometimes qmp pointer is not reset after qmp is shutdown.

Signed-off-by: Peng Tao <bergwolf@gmail.com>
For each time a sandbox structure is created, we ensure s.Release()
is called. Then we can keep the qmp connection as long as Sandbox
pointer is alive.

All VC interfaces are still stateless as s.Release() is called before
each API returns.

OTOH, for VCSandbox APIs, FetchSandbox() must be paired with s.Release,
the same as before.

Fixes: kata-containers#500

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

PSS Measurement:
Qemu: 162947 KB
Proxy: 4994 KB
Shim: 8754 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007236 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 23, 2018

Build succeeded (third-party-check pipeline).

When enabled, do not release in memory sandbox resources in VC APIs,
and callers are expected to call sandbox.Release() to release the in
memory resources.

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

PSS Measurement:
Qemu: 169255 KB
Proxy: 5070 KB
Shim: 8836 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007228 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 23, 2018

Build succeeded (third-party-check pipeline).

func (q *qemu) qmpSetup() (*govmmQemu.QMP, error) {
func (q *qemu) qmpSetup() error {
if q.qmpMonitorCh.qmp != nil {
return nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought here, do we need this to be idempotent? Is there any reason we would call qmpSetup() with qmp being not nil?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qmpSetup() is now always called before any qmp talks. But qmpShutdown() is only called explicitly when qemu.disconnect() is called. So yes, there are lots of cases when qmpSetup() is called with qmp being not nil.

Copy link

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, but otherwise the factorization looks good and will help the global performance.

@bergwolf
Copy link
Member Author

travis-ci is stuck for more than an hour in #1003 created state. Let's merge w/o it since all other checks are passed.

@bergwolf bergwolf merged commit 99954d5 into kata-containers:master Jul 24, 2018
@egernst egernst mentioned this pull request Aug 22, 2018
@sboeuf sboeuf added the enhancement Improvement to an existing feature label Sep 12, 2018
@sboeuf
Copy link

sboeuf commented Sep 12, 2018

@bergwolf @egernst I would not put this one into the stable branches, but I'm not sure.
WDYT?

@bergwolf
Copy link
Member Author

@sboeuf No, I don't think it's a stable candidate.

@bergwolf bergwolf deleted the qemu branch September 13, 2018 03:27
zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019
…temd

systemd-target: Add chronyd.service to kata-containers.target
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement Improvement to an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants