Races / crashes in shutdown of libvirtd daemon
by Daniel P. Berrangé
We got a new BZ filed about a libvirtd crash in shutdown
https://bugzilla.redhat.com/show_bug.cgi?id=1828207
We can see from the stack trace that the "interface" driver is in
the middle of servicing an RPC call for virConnectListAllInterfaces()
Meanwhile the libvirtd daemon is doing virObjectUnref(dmn) on the
virNetDaemonPtr object.
The fact that it is doing this unref, means that it must have already
call virStateCleanup(), given the code sequence:
/* Run event loop. */
virNetDaemonRun(dmn);
ret = 0;
virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_SHUTDOWN,
0, "shutdown", NULL, NULL);
cleanup:
/* Keep cleanup order in inverse order of startup */
virNetDaemonClose(dmn);
virNetlinkEventServiceStopAll();
if (driversInitialized) {
/* NB: Possible issue with timing window between driversInitialized
* setting if virNetlinkEventServerStart fails */
driversInitialized = false;
virStateCleanup();
}
virObjectUnref(adminProgram);
virObjectUnref(srvAdm);
virObjectUnref(qemuProgram);
virObjectUnref(lxcProgram);
virObjectUnref(remoteProgram);
virObjectUnref(srv);
virObjectUnref(dmn);
Unless I'm missing something non-obvious, this cleanup code path is
inherantly broken & racy. When virNetDaemonRun() returns the RPC
worker threads are all still active. They are all liable to still
be executing RPC calls, which means any of the drivers may be in
use. So calling virStateCleanup() is an inherantly dangerous
thing to do. There is the further complication that once we have
exitted the main loop we may prevent the RPC calls from ever
completing, as they may be waiting on an event to be dispatched.
I know we're had various patch proposals in the past to improve the
robustness of shutdown cleanup but I can't remember the outcome of the
reviews. Hopefully people involved in those threads can jump in here...
IMHO the key problem here is the virNetDeamonRun() method which just
looks at the "quit" flag and immediately returns if it is set.
This needs to be changed so that when it sees quit == true, it takes
the following actions
1. Call virNetDaemonClose() to drop all RPC clients and thus prevent
new RPC calls arriving
2. Flush any RPC calls which are queued but not yet assigned to a
worker thread
3. Tell worker threads to exit after finishing their current job
4. Wait for all worker threads to exit
5. Now virNetDaemonRun may return
At this point we can call virStateCleanup and the various other
things, as we know no drivers are still active in RPC calls.
Having said that, there could be background threads in the the
drivers which are doing work that uses the event loop thread.
So we probably need a virStateClose() method that we call from
virNetDaemonRun, *after* all worker threads are gone, which would
cleanup any background threads while the event loop is still
running.
The issue is that step 4 above ("Wait for all worker threads to exit")
may take too long, or indeed never complete. To deal with this, it
will need a timeout. In the remote_daemon.c cleanup code path, if
there are still worker threads present, then we need to skip all
cleanup and simply call _exit(0) to terminate the process with no
attempt at cleanup, since it would be unsafe to try anything else.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
4 years, 5 months
[libvirt PATCH v2 00/10] remote: introduce a custom netcat impl for ssh tunnelling
by Daniel P. Berrangé
We have long had a problem with use of netcat for ssh tunnelling because
there's no guarantee the UNIX socket path the client builds will match
the UNIX socket path the remote host uses. We don't even allow session
mode SSH tunnelling for this reason. We also can't easily auto-spawn
libvirtd in session mode.
With the introduction of modular daemons we also have potential for two
completely different UNIX socket paths even for system mode, and the
client can't know which to use.
The solution to all these problems is to introduce a custom netcat impl.
Instead passing the UNIX socket path, we pass the libvirt driver URI.
The custom netcat then decides which socket path to use based on the
remote build host environment.
We still have to support netcat for interoperability with legacy libvirt
versions, but we can default to the new virt-nc.
Daniel P. Berrangé (10):
rpc: merge logic for generating remote SSH shell script
remote: push logic for default netcat binary into common helper
remote: split off enums into separate source file
remote: split out function for parsing URI scheme
remote: parse the remote transport string earlier
remote: split out function for constructing socket path
remote: extract logic for determining daemon to connect to
remote: introduce virt-ssh-helper binary
rpc: switch order of args in virNetClientNewSSH
rpc: use new virt-ssh-helper binary for remote tunnelling
build-aux/syntax-check.mk | 2 +-
docs/uri.html.in | 24 +-
libvirt.spec.in | 2 +
po/POTFILES.in | 2 +
src/libvirt_remote.syms | 1 +
src/remote/Makefile.inc.am | 33 +++
src/remote/remote_driver.c | 323 ++++---------------------
src/remote/remote_sockets.c | 277 +++++++++++++++++++++
src/remote/remote_sockets.h | 70 ++++++
src/remote/remote_ssh_helper.c | 425 +++++++++++++++++++++++++++++++++
src/rpc/virnetclient.c | 166 ++++++++-----
src/rpc/virnetclient.h | 29 ++-
src/rpc/virnetsocket.c | 37 +--
src/rpc/virnetsocket.h | 4 +-
tests/virnetsockettest.c | 12 +-
15 files changed, 1035 insertions(+), 372 deletions(-)
create mode 100644 src/remote/remote_sockets.c
create mode 100644 src/remote/remote_sockets.h
create mode 100644 src/remote/remote_ssh_helper.c
--
2.26.2
4 years, 5 months
[PATCH] qemu: clear residual QMP caps processes during QEMU driver initialization
by Bihong Yu
>From c328ff62b11d58553fd2032a85fd3295e009b3d3 Mon Sep 17 00:00:00 2001
From: Bihong Yu <yubihong(a)huawei.com>
Date: Fri, 17 Jul 2020 16:55:12 +0800
Subject: [PATCH] qemu: clear residual QMP caps processes during QEMU driver
initialization
In some cases, the QMP capabilities processes possible residue. So we need to
clear the residual QMP caps processes during starting libvirt.
Signed-off-by:Bihong Yu <yubihong(a)huawei.com>
---
src/qemu/qemu_driver.c | 2 ++
src/qemu/qemu_process.c | 40 ++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_process.h | 2 ++
3 files changed, 44 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d185666..d627fd7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -889,6 +889,8 @@ qemuStateInitialize(bool privileged,
run_gid = cfg->group;
}
+ qemuProcessQMPClear(cfg->libDir);
+
qemu_driver->qemuCapsCache = virQEMUCapsCacheNew(cfg->libDir,
cfg->cacheDir,
run_uid,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 70fc24b..d545e3e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8312,6 +8312,46 @@ static qemuMonitorCallbacks callbacks = {
};
+/**
+ * qemuProcessQMPClear
+ *
+ * Try to kill residual QMP caps processes
+ */
+void
+qemuProcessQMPClear(const char *libDir)
+{
+ virErrorPtr orig_err;
+ DIR *dirp = NULL;
+ struct dirent *dp;
+
+ if (!virFileExists(libDir))
+ return;
+
+ if (virDirOpen(&dirp, libDir) < 0)
+ return;
+
+ virErrorPreserveLast(&orig_err);
+ while (virDirRead(dirp, &dp, libDir) > 0) {
+ g_autofree char *qmp_uniqDir = NULL;
+ g_autofree char *qmp_pidfile = NULL;
+
+ if (!STRPREFIX(dp->d_name, "qmp-"))
+ continue;
+
+ qmp_uniqDir = g_strdup_printf("%s/%s", libDir, dp->d_name);
+ qmp_pidfile = g_strdup_printf("%s/%s", qmp_uniqDir, "qmp.pid");
+
+ ignore_value(virPidFileForceCleanupPath(qmp_pidfile));
+
+ if (qmp_uniqDir)
+ rmdir(qmp_uniqDir);
+ }
+ virErrorRestore(&orig_err);
+
+ VIR_DIR_CLOSE(dirp);
+}
+
+
static void
qemuProcessQMPStop(qemuProcessQMPPtr proc)
{
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 15e67b9..b039e6c 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -233,6 +233,8 @@ qemuProcessQMPPtr qemuProcessQMPNew(const char *binary,
gid_t runGid,
bool forceTCG);
+void qemuProcessQMPClear(const char *libDir);
+
void qemuProcessQMPFree(qemuProcessQMPPtr proc);
int qemuProcessQMPStart(qemuProcessQMPPtr proc);
--
1.8.3.1
4 years, 6 months
[PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
by Collin Walling
When executing the hypervisor-cpu-compare/baseline commands and
the XML file contains a CPU definition using host-passthrough
and no model name, the commands will fail and return an error
message from the QMP response.
Let's fix this by checking for host-passthrough and a missing
model name when converting a CPU definition to a JSON object.
If these conditions are matched, then the JSON object will be
constructed using "host" as the model name.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1850680
Signed-off-by: Collin Walling <walling(a)linux.ibm.com>
---
src/qemu/qemu_monitor_json.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3070c1e6b3..448a3a9356 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5804,9 +5804,20 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu,
{
virJSONValuePtr model = virJSONValueNewObject();
virJSONValuePtr props = NULL;
+ const char *model_name = cpu->model;
size_t i;
- if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0)
+ if (!model_name) {
+ if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
+ model_name = "host";
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cpu parameter is missing a model name"));
+ goto error;
+ }
+ }
+
+ if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
goto error;
if (cpu->nfeatures || !migratable) {
--
2.26.2
4 years, 6 months
device compatibility interface for live migration with assigned devices
by Yan Zhao
hi folks,
we are defining a device migration compatibility interface that helps upper
layer stack like openstack/ovirt/libvirt to check if two devices are
live migration compatible.
The "devices" here could be MDEVs, physical devices, or hybrid of the two.
e.g. we could use it to check whether
- a src MDEV can migrate to a target MDEV,
- a src VF in SRIOV can migrate to a target VF in SRIOV,
- a src MDEV can migration to a target VF in SRIOV.
(e.g. SIOV/SRIOV backward compatibility case)
The upper layer stack could use this interface as the last step to check
if one device is able to migrate to another device before triggering a real
live migration procedure.
we are not sure if this interface is of value or help to you. please don't
hesitate to drop your valuable comments.
(1) interface definition
The interface is defined in below way:
__ userspace
/\ \
/ \write
/ read \
________/__________ ___\|/_____________
| migration_version | | migration_version |-->check migration
--------------------- --------------------- compatibility
device A device B
a device attribute named migration_version is defined under each device's
sysfs node. e.g. (/sys/bus/pci/devices/0000\:00\:02.0/$mdev_UUID/migration_version).
userspace tools read the migration_version as a string from the source device,
and write it to the migration_version sysfs attribute in the target device.
The userspace should treat ANY of below conditions as two devices not compatible:
- any one of the two devices does not have a migration_version attribute
- error when reading from migration_version attribute of one device
- error when writing migration_version string of one device to
migration_version attribute of the other device
The string read from migration_version attribute is defined by device vendor
driver and is completely opaque to the userspace.
for a Intel vGPU, string format can be defined like
"parent device PCI ID" + "version of gvt driver" + "mdev type" + "aggregator count".
for an NVMe VF connecting to a remote storage. it could be
"PCI ID" + "driver version" + "configured remote storage URL"
for a QAT VF, it may be
"PCI ID" + "driver version" + "supported encryption set".
(to avoid namespace confliction from each vendor, we may prefix a driver name to
each migration_version string. e.g. i915-v1-8086-591d-i915-GVTg_V5_8-1)
(2) backgrounds
The reason we hope the migration_version string is opaque to the userspace
is that it is hard to generalize standard comparing fields and comparing
methods for different devices from different vendors.
Though userspace now could still do a simple string compare to check if
two devices are compatible, and result should also be right, it's still
too limited as it excludes the possible candidate whose migration_version
string fails to be equal.
e.g. an MDEV with mdev_type_1, aggregator count 3 is probably compatible
with another MDEV with mdev_type_3, aggregator count 1, even their
migration_version strings are not equal.
(assumed mdev_type_3 is of 3 times equal resources of mdev_type_1).
besides that, driver version + configured resources are all elements demanding
to take into account.
So, we hope leaving the freedom to vendor driver and let it make the final decision
in a simple reading from source side and writing for test in the target side way.
we then think the device compatibility issues for live migration with assigned
devices can be divided into two steps:
a. management tools filter out possible migration target devices.
Tags could be created according to info from product specification.
we think openstack/ovirt may have vendor proprietary components to create
those customized tags for each product from each vendor.
e.g.
for Intel vGPU, with a vGPU(a MDEV device) in source side, the tags to
search target vGPU are like:
a tag for compatible parent PCI IDs,
a tag for a range of gvt driver versions,
a tag for a range of mdev type + aggregator count
for NVMe VF, the tags to search target VF may be like:
a tag for compatible PCI IDs,
a tag for a range of driver versions,
a tag for URL of configured remote storage.
b. with the output from step a, openstack/ovirt/libvirt could use our proposed
device migration compatibility interface to make sure the two devices are
indeed live migration compatible before launching the real live migration
process to start stream copying, src device stopping and target device
resuming.
It is supposed that this step would not bring any performance penalty as
-in kernel it's just a simple string decoding and comparing
-in openstack/ovirt, it could be done by extending current function
check_can_live_migrate_destination, along side claiming target resources.[1]
[1] https://specs.openstack.org/openstack/nova-specs/specs/stein/approved/lib...
Thanks
Yan
4 years, 7 months
[PATCH 0/5] remove NVDIMM auto-alignment for pSeries guests
by Daniel Henrique Barboza
Hi,
In [1] Andrea proposed that we should *not* auto-align down
the NVDIMM size for pSeries guests. Instead we should error
out and provide users with a suggested aligned value. This
patch implements it.
[1] https://www.redhat.com/archives/libvir-list/2020-July/msg01471.html
Daniel Henrique Barboza (5):
qemu_domain.c: make qemuDomainGetMemorySizeAlignment() public
qemu_validate.c: add pSeries NVDIMM size alignment validation
qemu_domain.c: do not auto-align ppc64 NVDIMMs
qemu_domain.c: change qemuDomainMemoryDeviceAlignSize() return type
Revert "formatdomain.html.in: mention pSeries NVDIMM 'align down'
mechanic"
docs/formatdomain.html.in | 6 +-
src/qemu/qemu_domain.c | 57 ++-----------------
src/qemu/qemu_domain.h | 7 ++-
src/qemu/qemu_hotplug.c | 6 +-
src/qemu/qemu_validate.c | 43 ++++++++++++--
...ory-hotplug-nvdimm-ppc64.ppc64-latest.args | 2 +-
.../memory-hotplug-nvdimm-ppc64.xml | 2 +-
.../memory-hotplug-nvdimm-ppc64.xml | 2 +-
8 files changed, 55 insertions(+), 70 deletions(-)
--
2.26.2
4 years, 7 months
[libvirt] [PATCH-for-4.2] hw/mips: Deprecate the r4k machine
by Philippe Mathieu-Daudé
The r4k machine was introduced in 2005 (6af0bf9c7) and its last
logical change was in 2005 (9542611a6). After we can count 164
maintenance commits (QEMU API changes) with the exception of
1 fix in 2015 (memory leak, commit 3ad9fd5a).
This machine was introduced as a proof of concept to run a MIPS
CPU. 2 years later, the Malta machine was add (commit 5856de80)
modeling a real platform.
Note also this machine has no specification except 5 lines in
the header of this file:
* emulates a simple machine with ISA-like bus.
* ISA IO space mapped to the 0x14000000 (PHYS) and
* ISA memory at the 0x10000000 (PHYS, 16Mb in size).
* All peripherial devices are attached to this "bus" with
* the standard PC ISA addresses.
It is time to deprecate this obsolete machine. Users are
recommended to use the Malta board, which hardware is well
documented.
Signed-off-by: Philippe Mathieu-Daudé <philmd(a)redhat.com>
---
qemu-deprecated.texi | 5 +++++
hw/mips/mips_r4k.c | 1 +
MAINTAINERS | 2 +-
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 4b4b7425ac..05265b43c8 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -266,6 +266,11 @@ The 'scsi-disk' device is deprecated. Users should use 'scsi-hd' or
@section System emulator machines
+@subsection mips r4k platform (since 4.2)
+
+This machine type is very old and unmaintained. Users should use the 'malta'
+machine type instead.
+
@subsection pc-0.12, pc-0.13, pc-0.14 and pc-0.15 (since 4.0)
These machine types are very old and likely can not be used for live migration
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index 70024235ae..0b79ad26cb 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -294,6 +294,7 @@ void mips_r4k_init(MachineState *machine)
static void mips_machine_init(MachineClass *mc)
{
+ mc->deprecation_reason = "use malta machine type instead";
mc->desc = "mips r4k platform";
mc->init = mips_r4k_init;
mc->block_default_type = IF_IDE;
diff --git a/MAINTAINERS b/MAINTAINERS
index 5e5e3e52d6..3b3a88e264 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -972,7 +972,7 @@ F: hw/net/mipsnet.c
R4000
M: Aurelien Jarno <aurelien(a)aurel32.net>
R: Aleksandar Rikalo <aleksandar.rikalo(a)rt-rk.com>
-S: Maintained
+S: Obsolete
F: hw/mips/mips_r4k.c
Fulong 2E
--
2.21.0
4 years, 7 months
[PATCH] client: fix memory leak in client msg
by Hao Wang
>From 3ad3fae4f2562a11bef8dcdd25b6a7e0b00d4e2c Mon Sep 17 00:00:00 2001
From: Hao Wang <wanghao232(a)huawei.com>
Date: Sat, 18 Jul 2020 15:43:30 +0800
Subject: [PATCH] client: fix memory leak in client msg
When closing client->waitDispatch in virNetClientIOEventLoopRemoveAll
or virNetClientIOEventLoopRemoveDone, VIR_FREE() is called to free
call->msg directly, resulting in leak of the memory call->msg->buffer
points to.
Use virNetMessageFree(call->msg) instead of VIR_FREE(call->msg).
Signed-off-by: Hao Wang <wanghao232(a)huawei.com>
---
src/rpc/virnetclient.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 441f1502a6..f899493b64 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1519,7 +1519,7 @@ static bool virNetClientIOEventLoopRemoveDone(virNetClientCallPtr call,
if (call->expectReply)
VIR_WARN("Got a call expecting a reply but without a waiting thread");
virCondDestroy(&call->cond);
- VIR_FREE(call->msg);
+ virNetMessageFree(call->msg);
VIR_FREE(call);
}
@@ -1546,7 +1546,7 @@ virNetClientIOEventLoopRemoveAll(virNetClientCallPtr call,
VIR_DEBUG("Removing call %p", call);
virCondDestroy(&call->cond);
- VIR_FREE(call->msg);
+ virNetMessageFree(call->msg);
VIR_FREE(call);
return true;
}
--
2.23.0
4 years, 7 months
[PATCH v2 00/13] resolve hangs/crashes on libvirtd shutdown
by Nikolay Shirokovskiy
I keep qemu VM event loop exiting synchronously but add code to avoid deadlock
that can be caused by this approach. I guess it is worth having synchronous
exiting of threads in this case to avoid crashes.
Patches that are already positively reviewed has appropriate 'Reviewed-by' lines.
Changes from v1:
- rename stateShutdown to state stateShutdownPrepare
- introduce net daemon shutdown callbacks
- make some adjustments in terms of qemu per VM's event loop thread
finishing
- factor out net server shutdown facilities into distinct patch
- increase shutdown timeout from 15s to 30s
Nikolay Shirokovskiy (13):
libvirt: add stateShutdownPrepare/stateShutdownWait to drivers
util: always initialize priority condition
util: add stop/drain functions to thread pool
rpc: don't unref service ref on socket behalf twice
rpc: add virNetDaemonSetShutdownCallbacks
rpc: add shutdown facilities to netserver
rpc: finish all threads before exiting main loop
qemu: don't shutdown event thread in monitor EOF callback
vireventthread: exit thread synchronously on finalize
qemu: avoid deadlock in qemuDomainObjStopWorker
qemu: implement driver's shutdown/shutdown wait methods
rpc: cleanup virNetDaemonClose method
util: remove unused virThreadPoolNew macro
scripts/check-drivername.py | 2 +
src/driver-state.h | 8 ++++
src/libvirt.c | 42 ++++++++++++++++
src/libvirt_internal.h | 2 +
src/libvirt_private.syms | 4 ++
src/libvirt_remote.syms | 2 +-
src/qemu/qemu_domain.c | 18 +++++--
src/qemu/qemu_driver.c | 32 +++++++++++++
src/qemu/qemu_process.c | 3 --
src/remote/remote_daemon.c | 6 +--
src/rpc/virnetdaemon.c | 109 ++++++++++++++++++++++++++++++++++++------
src/rpc/virnetdaemon.h | 8 +++-
src/rpc/virnetserver.c | 8 ++++
src/rpc/virnetserver.h | 1 +
src/rpc/virnetserverservice.c | 1 -
src/util/vireventthread.c | 1 +
src/util/virthreadpool.c | 65 +++++++++++++++++--------
src/util/virthreadpool.h | 6 +--
18 files changed, 267 insertions(+), 51 deletions(-)
--
1.8.3.1
4 years, 7 months
