[libvirt] [PATCH 0/7] Couple of PR fixes and improvements
QEMU implemented event when pr-manager connects to/disconnects from pr-helper process. Also, it implemented query command to learn the pr-manager state on libvirtd restart. Whilst working on patches for this, I've noticed couple of other bugs too. Michal Privoznik (7): qemuProcessStartPRDaemonHook: Try to set NS iff domain was started with one qemuDomainValidateStorageSource: Relax PR validation virStoragePRDefFormat: Suppress path formatting for migratable XML virstoragefile: Introduce virStorageSourceChainGetManagedPRAlias qemu: Wire up PR_MANAGER_STATUS_CHANGED event qemu_monitor: Introduce qemuMonitorJSONGetPRManagerInfo qemu: Fetch pr-helper process info on reconnect src/conf/domain_conf.c | 3 +- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 21 +++---- src/qemu/qemu_monitor.c | 40 ++++++++++++ src/qemu/qemu_monitor.h | 20 ++++++ src/qemu/qemu_monitor_json.c | 113 ++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++ src/qemu/qemu_process.c | 143 +++++++++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.c | 20 +++++- src/util/virstoragefile.h | 6 +- 10 files changed, 349 insertions(+), 22 deletions(-) -- 2.16.4
Users have possibility to disable qemu namespace feature (e.g. because they are running on *BSD which lacks Linux NS support). If that's the case we should not try to move qemu-pr-helper into the same namespace as qemu is in. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 40d35cbe6b..f200729cb1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2522,12 +2522,14 @@ qemuProcessStartPRDaemonHook(void *opaque) int *fds = NULL; int ret = -1; - if (virProcessGetNamespaces(vm->pid, &nfds, &fds) < 0) - return ret; + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { + if (virProcessGetNamespaces(vm->pid, &nfds, &fds) < 0) + return ret; - if (nfds > 0 && - virProcessSetNamespaces(nfds, fds) < 0) - goto cleanup; + if (nfds > 0 && + virProcessSetNamespaces(nfds, fds) < 0) + goto cleanup; + } ret = 0; cleanup: -- 2.16.4
On Wed, Jul 04, 2018 at 12:46:49 +0200, Michal Privoznik wrote:
Users have possibility to disable qemu namespace feature (e.g. because they are running on *BSD which lacks Linux NS support). If that's the case we should not try to move qemu-pr-helper into the same namespace as qemu is in.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
ACK
Actually, it is not always bug if path is provided. For instance, on domain migration the migration XML contains the path even for managed reservations. Accept this and teach our prepare code to not leak in this case. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a60bca29ca..706ee9be46 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4615,19 +4615,11 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src, } } - if (src->pr) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("reservations not supported with this QEMU binary")); - return -1; - } - - if (virStoragePRDefIsManaged(src->pr) && src->pr->path) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'path' attribute should not be provided for " - "managed reservations")); - return -1; - } + if (src->pr && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reservations not supported with this QEMU binary")); + return -1; } return 0; @@ -12852,7 +12844,10 @@ qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src, if (!src->pr) return 0; + VIR_FREE(src->pr->mgralias); + if (virStoragePRDefIsManaged(src->pr)) { + VIR_FREE(src->pr->path); if (!(src->pr->path = qemuDomainGetManagedPRSocketPath(priv))) return -1; if (VIR_STRDUP(src->pr->mgralias, qemuDomainGetManagedPRAlias()) < 0) -- 2.16.4
On Wed, Jul 04, 2018 at 12:46:50 +0200, Michal Privoznik wrote:
Actually, it is not always bug if path is provided. For instance, on domain migration the migration XML contains the path even for managed reservations. Accept this and teach our prepare code to not leak in this case.
Please reformulate this so that is clear that rather than rejecting the user provided path and alias (which is not mentioned above) for the managed PR reservation we will ignore the (user) provided path rather than reject it. Also isn't the alias part of the private data and thus not transferred during migration? It certainly should not be formatted for the migratable case.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a60bca29ca..706ee9be46 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4615,19 +4615,11 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src, } }
- if (src->pr) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("reservations not supported with this QEMU binary")); - return -1; - } - - if (virStoragePRDefIsManaged(src->pr) && src->pr->path) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'path' attribute should not be provided for " - "managed reservations")); - return -1; - } + if (src->pr && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reservations not supported with this QEMU binary")); + return -1; }
return 0; @@ -12852,7 +12844,10 @@ qemuDomainPrepareStorageSourcePR(virStorageSourcePtr src, if (!src->pr) return 0;
+ VIR_FREE(src->pr->mgralias); + if (virStoragePRDefIsManaged(src->pr)) { + VIR_FREE(src->pr->path); if (!(src->pr->path = qemuDomainGetManagedPRSocketPath(priv))) return -1; if (VIR_STRDUP(src->pr->mgralias, qemuDomainGetManagedPRAlias()) < 0) -- 2.16.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
If there are managed reservations for a disk source, the path to the pr-helper socket is generated automatically by libvirt when needed and points somewhere under priv->libDir. Therefore it is very unlikely that the path will work even on migration destination (the libDir is derived from domain short name and its ID remember?). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/util/virstoragefile.c | 6 ++++-- src/util/virstoragefile.h | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f4e59f6c91..70eb45f03a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23548,7 +23548,8 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, return -1; if (src->pr) - virStoragePRDefFormat(childBuf, src->pr); + virStoragePRDefFormat(childBuf, src->pr, + flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE); return 0; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 6ede542df6..58f67278da 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1982,11 +1982,13 @@ virStoragePRDefParseXML(xmlXPathContextPtr ctxt) void virStoragePRDefFormat(virBufferPtr buf, - virStoragePRDefPtr prd) + virStoragePRDefPtr prd, + bool migratable) { virBufferAsprintf(buf, "<reservations managed='%s'", virTristateBoolTypeToString(prd->managed)); - if (prd->path) { + if (prd->path && + (prd->managed == VIR_TRISTATE_BOOL_NO || !migratable)) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferAddLit(buf, "<source type='unix'"); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 592e19bd7f..991098e6c6 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -395,7 +395,8 @@ void virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef); void virStoragePRDefFree(virStoragePRDefPtr prd); virStoragePRDefPtr virStoragePRDefParseXML(xmlXPathContextPtr ctxt); void virStoragePRDefFormat(virBufferPtr buf, - virStoragePRDefPtr prd); + virStoragePRDefPtr prd, + bool migratable); bool virStoragePRDefIsEqual(virStoragePRDefPtr a, virStoragePRDefPtr b); bool virStoragePRDefIsManaged(virStoragePRDefPtr prd); -- 2.16.4
On Wed, Jul 04, 2018 at 12:46:51 +0200, Michal Privoznik wrote:
If there are managed reservations for a disk source, the path to the pr-helper socket is generated automatically by libvirt when needed and points somewhere under priv->libDir. Therefore it is very unlikely that the path will work even on migration destination (the libDir is derived from domain short name and its ID remember?).
Please change the question into a statement.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/util/virstoragefile.c | 6 ++++-- src/util/virstoragefile.h | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-)
ACK
When processing a pr-manager related event from qemu we will know pr-manager alias. So we need a helper to traverse backing chain and return any managed alias it finds so that the caller can compare these two aliases to learn if it is a managed helper or not. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 14 ++++++++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 18 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3e304907b9..98ff8a5aa6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2840,6 +2840,7 @@ virStoragePRDefIsEqual; virStoragePRDefIsManaged; virStoragePRDefParseXML; virStorageSourceBackingStoreClear; +virStorageSourceChainGetManagedPRAlias; virStorageSourceChainHasManagedPR; virStorageSourceClear; virStorageSourceCopy; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 58f67278da..3f2bca4970 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2041,6 +2041,20 @@ virStorageSourceChainHasManagedPR(virStorageSourcePtr src) } +const char * +virStorageSourceChainGetManagedPRAlias(virStorageSourcePtr src) +{ + virStorageSourcePtr n; + + for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (virStoragePRDefIsManaged(src->pr)) + return src->pr->mgralias; + } + + return NULL; +} + + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 991098e6c6..eb5b2b2a45 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -404,6 +404,9 @@ bool virStoragePRDefIsManaged(virStoragePRDefPtr prd); bool virStorageSourceChainHasManagedPR(virStorageSourcePtr src); +const char * +virStorageSourceChainGetManagedPRAlias(virStorageSourcePtr src); + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model); -- 2.16.4
On Wed, Jul 04, 2018 at 12:46:52 +0200, Michal Privoznik wrote:
When processing a pr-manager related event from qemu we will know pr-manager alias. So we need a helper to traverse backing chain and return any managed alias it finds so that the caller can compare these two aliases to learn if it is a managed helper or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 14 ++++++++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 18 insertions(+)
This function should not be necessary. See review for 5/7
This event is emitted on the monitor if one of pr-managers lost connection to its pr-helper process. What libvirt needs to do is restart the pr-helper process iff it corresponds to managed pr-manager. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 15 +++++++++++++ src/qemu/qemu_monitor.h | 11 +++++++++ src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++++ src/qemu/qemu_process.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6ed475ede0..ca95f6f94a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1669,6 +1669,21 @@ qemuMonitorEmitDumpCompleted(qemuMonitorPtr mon, } +int +qemuMonitorEmitPRManagerStatusChanged(qemuMonitorPtr mon, + const char *prManager, + bool connected) +{ + int ret = -1; + VIR_DEBUG("mon=%p, prManager='%s', connected=%d", mon, prManager, connected); + + QEMU_MONITOR_CALLBACK(mon, ret, domainPRManagerStatusChanged, + mon->vm, prManager, connected); + + return ret; +} + + int qemuMonitorSetCapabilities(qemuMonitorPtr mon) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index b3d62324b4..f1ea0bc541 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -273,6 +273,12 @@ typedef int (*qemuMonitorDomainDumpCompletedCallback)(qemuMonitorPtr mon, const char *error, void *opaque); +typedef int (*qemuMonitorDomainPRManagerStatusChangedCallback)(qemuMonitorPtr mon, + virDomainObjPtr vm, + const char *prManager, + bool connected, + void *opaque); + typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks; typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr; struct _qemuMonitorCallbacks { @@ -305,6 +311,7 @@ struct _qemuMonitorCallbacks { qemuMonitorDomainAcpiOstInfoCallback domainAcpiOstInfo; qemuMonitorDomainBlockThresholdCallback domainBlockThreshold; qemuMonitorDomainDumpCompletedCallback domainDumpCompleted; + qemuMonitorDomainPRManagerStatusChangedCallback domainPRManagerStatusChanged; }; char *qemuMonitorEscapeArg(const char *in); @@ -433,6 +440,10 @@ int qemuMonitorEmitDumpCompleted(qemuMonitorPtr mon, qemuMonitorDumpStatsPtr stats, const char *error); +int qemuMonitorEmitPRManagerStatusChanged(qemuMonitorPtr mon, + const char *prManager, + bool connected); + int qemuMonitorStartCPUs(qemuMonitorPtr mon); int qemuMonitorStopCPUs(qemuMonitorPtr mon); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3e90279b71..03c94cd88b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -91,6 +91,7 @@ static void qemuMonitorJSONHandleMigrationPass(qemuMonitorPtr mon, virJSONValueP static void qemuMonitorJSONHandleAcpiOstInfo(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleBlockThreshold(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleDumpCompleted(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandlePRManagerStatusChanged(qemuMonitorPtr mon, virJSONValuePtr data); typedef struct { const char *type; @@ -113,6 +114,7 @@ static qemuEventHandler eventHandlers[] = { { "MIGRATION_PASS", qemuMonitorJSONHandleMigrationPass, }, { "NIC_RX_FILTER_CHANGED", qemuMonitorJSONHandleNicRxFilterChanged, }, { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, + { "PR_MANAGER_STATUS_CHANGED", qemuMonitorJSONHandlePRManagerStatusChanged, }, { "RESET", qemuMonitorJSONHandleReset, }, { "RESUME", qemuMonitorJSONHandleResume, }, { "RTC_CHANGE", qemuMonitorJSONHandleRTCChange, }, @@ -1297,6 +1299,27 @@ qemuMonitorJSONHandleDumpCompleted(qemuMonitorPtr mon, } +static void qemuMonitorJSONHandlePRManagerStatusChanged(qemuMonitorPtr mon, + virJSONValuePtr data) +{ + const char *name; + bool connected; + + if (!(name = virJSONValueObjectGetString(data, "id"))) { + VIR_WARN("missing pr-manager alias in PR_MANAGER_STATUS_CHANGED event"); + return; + } + + if (virJSONValueObjectGetBoolean(data, "connected", &connected) < 0) { + VIR_WARN("missing connected state for %s " + "in PR_MANAGER_STATUS_CHANGED event", name); + return; + } + + qemuMonitorEmitPRManagerStatusChanged(mon, name, connected); +} + + int qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon, const char *cmd_str, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f200729cb1..94b7de76d7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1615,6 +1615,58 @@ qemuProcessHandleDumpCompleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } +static int +qemuProcessHandlePRManagerStatusChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + const char *prManager, + bool connected, + void *opaque ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv; + size_t i; + int ret = -1; + + virObjectLock(vm); + + VIR_DEBUG("pr-manager %s status changed for domain %p %s connected=%d", + prManager, vm, vm->def->name, connected); + + if (connected) { + /* Connect events are boring. */ + ret = 0; + goto cleanup; + } + /* Disconnect events are more interesting. */ + + for (i = 0; i < vm->def->ndisks; i++) { + const char *mgralias; + + mgralias = virStorageSourceChainGetManagedPRAlias(vm->def->disks[i]->src); + + if (STREQ_NULLABLE(prManager, mgralias)) + break; + } + + if (i == vm->def->ndisks) { + VIR_DEBUG("pr-manager %s not managed, ignoring event", + prManager); + ret = 0; + goto cleanup; + } + + priv = vm->privateData; + priv->prDaemonRunning = false; + + if (qemuProcessStartManagedPRDaemon(vm) < 0) + goto cleanup; + + ret = 0; + cleanup: + virObjectUnlock(vm); + return ret; +} + + static qemuMonitorCallbacks monitorCallbacks = { .eofNotify = qemuProcessHandleMonitorEOF, .errorNotify = qemuProcessHandleMonitorError, @@ -1643,6 +1695,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainAcpiOstInfo = qemuProcessHandleAcpiOstInfo, .domainBlockThreshold = qemuProcessHandleBlockThreshold, .domainDumpCompleted = qemuProcessHandleDumpCompleted, + .domainPRManagerStatusChanged = qemuProcessHandlePRManagerStatusChanged, }; static void -- 2.16.4
On Wed, Jul 04, 2018 at 12:46:53 +0200, Michal Privoznik wrote:
This event is emitted on the monitor if one of pr-managers lost connection to its pr-helper process. What libvirt needs to do is restart the pr-helper process iff it corresponds to managed pr-manager.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 15 +++++++++++++ src/qemu/qemu_monitor.h | 11 +++++++++ src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++++ src/qemu/qemu_process.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+)
[...]
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f200729cb1..94b7de76d7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1615,6 +1615,58 @@ qemuProcessHandleDumpCompleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, }
+static int +qemuProcessHandlePRManagerStatusChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + const char *prManager, + bool connected, + void *opaque ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv; + size_t i; + int ret = -1; + + virObjectLock(vm); + + VIR_DEBUG("pr-manager %s status changed for domain %p %s connected=%d", + prManager, vm, vm->def->name, connected); + + if (connected) { + /* Connect events are boring. */ + ret = 0; + goto cleanup; + } + /* Disconnect events are more interesting. */ + + for (i = 0; i < vm->def->ndisks; i++) { + const char *mgralias; + + mgralias = virStorageSourceChainGetManagedPRAlias(vm->def->disks[i]->src); + + if (STREQ_NULLABLE(prManager, mgralias)) + break;
I'm not a fan of this. We always know which is the managed alias and we also know if it is supposed to be running. The hotplug code already does not inspect disks to do this since there is only one instance.
+ } + + if (i == vm->def->ndisks) { + VIR_DEBUG("pr-manager %s not managed, ignoring event", + prManager); + ret = 0; + goto cleanup; + } + + priv = vm->privateData; + priv->prDaemonRunning = false; + + if (qemuProcessStartManagedPRDaemon(vm) < 0)
This has a timeout built in. Thus executing this from the event loop will make the whole libvirtd get stuck until it starts. This should not be in the event loop. Also does every disconnect equal to the daemon crashing/stopping?
+ goto cleanup; + + ret = 0;
On 07/04/2018 01:42 PM, Peter Krempa wrote:
On Wed, Jul 04, 2018 at 12:46:53 +0200, Michal Privoznik wrote:
This event is emitted on the monitor if one of pr-managers lost connection to its pr-helper process. What libvirt needs to do is restart the pr-helper process iff it corresponds to managed pr-manager.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 15 +++++++++++++ src/qemu/qemu_monitor.h | 11 +++++++++ src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++++ src/qemu/qemu_process.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+)
[...]
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f200729cb1..94b7de76d7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1615,6 +1615,58 @@ qemuProcessHandleDumpCompleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, }
+static int +qemuProcessHandlePRManagerStatusChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + const char *prManager, + bool connected, + void *opaque ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv; + size_t i; + int ret = -1; + + virObjectLock(vm); + + VIR_DEBUG("pr-manager %s status changed for domain %p %s connected=%d", + prManager, vm, vm->def->name, connected); + + if (connected) { + /* Connect events are boring. */ + ret = 0; + goto cleanup; + } + /* Disconnect events are more interesting. */ + + for (i = 0; i < vm->def->ndisks; i++) { + const char *mgralias; + + mgralias = virStorageSourceChainGetManagedPRAlias(vm->def->disks[i]->src); + + if (STREQ_NULLABLE(prManager, mgralias)) + break;
I'm not a fan of this. We always know which is the managed alias
Do we? I thought the whole point of storing it in source private data was to make it independent of qemuDomainGetManagedPRAlias(). Imagine I started a domain with pr-manager.alias = pr-helper0. Then restarted libvirtd to upgrade it. This new version has, however, pr-helper1 as default alias (because reasons). Then all of a sudden I receive this event with alias pr-helper0 (because that is how I started the domain). I can't do plain: STREQ(prManager, qemuDomainGetManagedPRAlias()) can I?
and we also know if it is supposed to be running.
The hotplug code already does not inspect disks to do this since there is only one instance.
Sure. But hotplug code does not have to care about old instances.
+ } + + if (i == vm->def->ndisks) { + VIR_DEBUG("pr-manager %s not managed, ignoring event", + prManager); + ret = 0; + goto cleanup; + } + + priv = vm->privateData; + priv->prDaemonRunning = false; + + if (qemuProcessStartManagedPRDaemon(vm) < 0)
This has a timeout built in. Thus executing this from the event loop will make the whole libvirtd get stuck until it starts. This should not be in the event loop.
Ah good point. I'll rewrite this to run it in the qemu event process thread (or what do we call it).
Also does every disconnect equal to the daemon crashing/stopping?
It should. If qemu sends us bogus events I'm not sure there's much we can do about it. Michal
On Wed, Jul 04, 2018 at 14:42:07 +0200, Michal Privoznik wrote:
On 07/04/2018 01:42 PM, Peter Krempa wrote:
On Wed, Jul 04, 2018 at 12:46:53 +0200, Michal Privoznik wrote:
This event is emitted on the monitor if one of pr-managers lost connection to its pr-helper process. What libvirt needs to do is restart the pr-helper process iff it corresponds to managed pr-manager.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 15 +++++++++++++ src/qemu/qemu_monitor.h | 11 +++++++++ src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++++ src/qemu/qemu_process.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+)
[...]
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f200729cb1..94b7de76d7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1615,6 +1615,58 @@ qemuProcessHandleDumpCompleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, }
+static int +qemuProcessHandlePRManagerStatusChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + const char *prManager, + bool connected, + void *opaque ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv; + size_t i; + int ret = -1; + + virObjectLock(vm); + + VIR_DEBUG("pr-manager %s status changed for domain %p %s connected=%d", + prManager, vm, vm->def->name, connected); + + if (connected) { + /* Connect events are boring. */ + ret = 0; + goto cleanup; + } + /* Disconnect events are more interesting. */ + + for (i = 0; i < vm->def->ndisks; i++) { + const char *mgralias; + + mgralias = virStorageSourceChainGetManagedPRAlias(vm->def->disks[i]->src); + + if (STREQ_NULLABLE(prManager, mgralias)) + break;
I'm not a fan of this. We always know which is the managed alias
Do we? I thought the whole point of storing it in source private data was to make it independent of qemuDomainGetManagedPRAlias(). Imagine I started a domain with pr-manager.alias = pr-helper0. Then restarted libvirtd to upgrade it. This new version has, however, pr-helper1 as default alias (because reasons). Then all of a sudden I receive this event with alias pr-helper0 (because that is how I started the domain). I can't do plain:
STREQ(prManager, qemuDomainGetManagedPRAlias())
can I?
Well you can. On upgrade path you will need to determine what the old default was and store it somewhere. The problem here is that you should not determine the alias of the default managed pr helper by inspecting the disks but you should rather store it somewhere else if you want to. Specifically in the same place as the PID is stored. The object is just one and it's a property of the VM not of the disk. By storing it in the disk you don't help in the case when something gets changed anyways. If you add something that adds a second managed pr-helper you will need to modify the code which is doing the alias detection anyways, so that argument does not make much sense either.
This function fetches status of all pr-managers. So far, qemu reports only a single attribute "connected" but that fits our needs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 25 ++++++++++++ src/qemu/qemu_monitor.h | 9 +++++ src/qemu/qemu_monitor_json.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++ 4 files changed, 128 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ca95f6f94a..3514e9f8a1 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4335,3 +4335,28 @@ qemuMonitorGetSEVMeasurement(qemuMonitorPtr mon) return qemuMonitorJSONGetSEVMeasurement(mon); } + + +int +qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, + virHashTablePtr *retinfo) +{ + int ret = -1; + virHashTablePtr info = NULL; + + *retinfo = NULL; + + QEMU_CHECK_MONITOR(mon); + + if (!(info = virHashCreate(10, virHashValueFree))) + goto cleanup; + + if (qemuMonitorJSONGetPRManagerInfo(mon, info) < 0) + goto cleanup; + + VIR_STEAL_PTR(*retinfo, info); + ret = 0; + cleanup: + virHashFree(info); + return ret; +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f1ea0bc541..e588d73678 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1156,4 +1156,13 @@ int qemuMonitorBlockdevDel(qemuMonitorPtr mon, char * qemuMonitorGetSEVMeasurement(qemuMonitorPtr mon); +typedef struct _qemuMonitorPRManagerInfo qemuMonitorPRManagerInfo; +typedef qemuMonitorPRManagerInfo *qemuMonitorPRManagerInfoPtr; +struct _qemuMonitorPRManagerInfo { + bool connected; +}; + +int qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon, + virHashTablePtr *retinfo); + #endif /* QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 03c94cd88b..460312a067 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8065,3 +8065,93 @@ qemuMonitorJSONGetSEVMeasurement(qemuMonitorPtr mon) virJSONValueFree(reply); return measurement; } + + +/* + * Example return data + * + * "return": [ + * { "connected": true, "id": "pr-helper0" } + * ]} + * + */ +static int +qemuMonitorJSONExtractPRManagerInfo(virJSONValuePtr reply, + virHashTablePtr info) +{ + qemuMonitorPRManagerInfoPtr entry = NULL; + virJSONValuePtr data; + int ret = -1; + size_t i; + + data = virJSONValueObjectGetArray(reply, "return"); + + for (i = 0; i < virJSONValueArraySize(data); i++) { + virJSONValuePtr prManager = virJSONValueArrayGet(data, i); + const char *alias; + + if (!prManager) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("prManager information was missing array element")); + goto cleanup; + } + + if (!(alias = virJSONValueObjectGetString(prManager, "id"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("prManager information was missing id")); + goto cleanup; + } + + if (VIR_ALLOC(entry) < 0) + goto cleanup; + + if (virJSONValueObjectGetBoolean(prManager, + "connected", + &entry->connected) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("prManager information was missing connected")); + goto cleanup; + } + + if (virHashAddEntry(info, alias, entry) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to add chardev '%s' info"), alias); + VIR_FREE(entry); + goto cleanup; + } + + entry = NULL; + } + + ret = 0; + cleanup: + VIR_FREE(entry); + return ret; +} + + +int +qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, + virHashTablePtr info) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-pr-managers", + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + goto cleanup; + + ret = qemuMonitorJSONExtractPRManagerInfo(reply, info); + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; + +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 6bc0dd3ad2..66536ceb97 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -550,4 +550,8 @@ int qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon, const char *nodename) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuMonitorJSONGetPRManagerInfo(qemuMonitorPtr mon, + virHashTablePtr info) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* QEMU_MONITOR_JSON_H */ -- 2.16.4
On Wed, Jul 04, 2018 at 12:46:54 +0200, Michal Privoznik wrote:
This function fetches status of all pr-managers. So far, qemu reports only a single attribute "connected" but that fits our needs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 25 ++++++++++++ src/qemu/qemu_monitor.h | 9 +++++ src/qemu/qemu_monitor_json.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++ 4 files changed, 128 insertions(+)
[...]
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 03c94cd88b..460312a067 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8065,3 +8065,93 @@ qemuMonitorJSONGetSEVMeasurement(qemuMonitorPtr mon) virJSONValueFree(reply); return measurement; } + + +/* + * Example return data + * + * "return": [ + * { "connected": true, "id": "pr-helper0" } + * ]} + * + */ +static int +qemuMonitorJSONExtractPRManagerInfo(virJSONValuePtr reply, + virHashTablePtr info) +{ + qemuMonitorPRManagerInfoPtr entry = NULL; + virJSONValuePtr data; + int ret = -1; + size_t i; + + data = virJSONValueObjectGetArray(reply, "return"); + + for (i = 0; i < virJSONValueArraySize(data); i++) { + virJSONValuePtr prManager = virJSONValueArrayGet(data, i); + const char *alias; + + if (!prManager) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("prManager information was missing array element"));
[1]
+ goto cleanup; + } + + if (!(alias = virJSONValueObjectGetString(prManager, "id"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("prManager information was missing id"));
[1]
+ goto cleanup; + } + + if (VIR_ALLOC(entry) < 0) + goto cleanup; + + if (virJSONValueObjectGetBoolean(prManager, + "connected", + &entry->connected) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("prManager information was missing connected"));
I don't think we need special errors for all these different [1] cases. Just extract everything and report that query-pr-managers returned malformed data.
+ goto cleanup; + } + + if (virHashAddEntry(info, alias, entry) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to add chardev '%s' info"), alias);
This is overwriting existing error.
+ VIR_FREE(entry);
This is done in the cleanup section too.
+ goto cleanup; + } + + entry = NULL; + } + + ret = 0; + cleanup: + VIR_FREE(entry); + return ret; +}
If qemu-pr-helper process died while libvirtd was not running no event is emitted. Therefore, when reconnecting to the monitor we must check the qemu-pr-helper process status and act accordingly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 94b7de76d7..ac148a39e9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2069,6 +2069,81 @@ qemuRefreshVirtioChannelState(virQEMUDriverPtr driver, return ret; } + +static int +qemuProcessRefreshPRManagerState(virDomainObjPtr vm, + virHashTablePtr info) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + qemuMonitorPRManagerInfoPtr prManagerInfo; + size_t i; + int ret = -1; + + for (i = 0; i < vm->def->ndisks; i++) { + const char *mgralias; + + mgralias = virStorageSourceChainGetManagedPRAlias(vm->def->disks[i]->src); + + if (!mgralias) + continue; + + if (!(prManagerInfo = virHashLookup(info, mgralias))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("missing info on pr-manager %s"), + mgralias); + goto cleanup; + } + + break; + } + + if (i == vm->def->ndisks) { + /* no managed pr-manager, return early. */ + ret = 0; + goto cleanup; + } + + priv->prDaemonRunning = prManagerInfo->connected; + + if (!priv->prDaemonRunning && + qemuProcessStartManagedPRDaemon(vm) < 0) + goto cleanup; + + ret = 0; + cleanup: + return ret; +} + + +static int +qemuRefreshPRManagerState(virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virHashTablePtr info = NULL; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup; + + ret = qemuMonitorGetPRManagerInfo(priv->mon, &info); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + if (ret < 0) + goto cleanup; + + ret = qemuProcessRefreshPRManagerState(vm, info); + + cleanup: + virHashFree(info); + return ret; +} + + static void qemuRefreshRTC(virQEMUDriverPtr driver, virDomainObjPtr vm) @@ -7722,6 +7797,9 @@ qemuProcessReconnect(void *opaque) if (qemuRefreshVirtioChannelState(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) goto error; + if (qemuRefreshPRManagerState(driver, obj) < 0) + goto error; + /* If querying of guest's RTC failed, report error, but do not kill the domain. */ qemuRefreshRTC(driver, obj); -- 2.16.4
On Wed, Jul 04, 2018 at 12:46:55 +0200, Michal Privoznik wrote:
If qemu-pr-helper process died while libvirtd was not running no event is emitted. Therefore, when reconnecting to the monitor we must check the qemu-pr-helper process status and act accordingly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 94b7de76d7..ac148a39e9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2069,6 +2069,81 @@ qemuRefreshVirtioChannelState(virQEMUDriverPtr driver, return ret; }
+ +static int +qemuProcessRefreshPRManagerState(virDomainObjPtr vm, + virHashTablePtr info) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + qemuMonitorPRManagerInfoPtr prManagerInfo; + size_t i; + int ret = -1; + + for (i = 0; i < vm->def->ndisks; i++) { + const char *mgralias; + + mgralias = virStorageSourceChainGetManagedPRAlias(vm->def->disks[i]->src); + + if (!mgralias) + continue; + + if (!(prManagerInfo = virHashLookup(info, mgralias))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("missing info on pr-manager %s"), + mgralias); + goto cleanup; + }
I don't think this is a compelling reason to kill the VM upon reconnect. Also as said, it should not be necessary to iterate the disks to see whether the managed PR helper is required and what the alias is supposed to be.
+ + break; + } + + if (i == vm->def->ndisks) { + /* no managed pr-manager, return early. */ + ret = 0; + goto cleanup; + } + + priv->prDaemonRunning = prManagerInfo->connected; + + if (!priv->prDaemonRunning && + qemuProcessStartManagedPRDaemon(vm) < 0) + goto cleanup; + + ret = 0; + cleanup: + return ret; +} + + +static int +qemuRefreshPRManagerState(virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virHashTablePtr info = NULL; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup;
Ummm, why don't you use qemuDomainObjEnterMonitor then?
+ + ret = qemuMonitorGetPRManagerInfo(priv->mon, &info); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + if (ret < 0) + goto cleanup; + + ret = qemuProcessRefreshPRManagerState(vm, info); + + cleanup: + virHashFree(info); + return ret; +} + + static void qemuRefreshRTC(virQEMUDriverPtr driver, virDomainObjPtr vm) @@ -7722,6 +7797,9 @@ qemuProcessReconnect(void *opaque) if (qemuRefreshVirtioChannelState(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) goto error;
+ if (qemuRefreshPRManagerState(driver, obj) < 0) + goto error; + /* If querying of guest's RTC failed, report error, but do not kill the domain. */ qemuRefreshRTC(driver, obj);
-- 2.16.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa