[libvirt] [PATCH] qemu: capabilities: Don't partially reprope caps on process reconnect

Thanks to the complex capability caching code virQEMUCapsProbeQMP was never called when we were starting a new qemu VM. On the other hand, when we are reconnecting to the qemu process we reload the capability list from the status XML file. This means that the flag preventing the function being called was not set and thus we partially reprobed some of the capabilities. The recent addition of CPU hotplug clears the QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS if the machine does not support it. The partial re-probe on reconnect results into attempting to call the unsupported command and then killing the VM. Remove the partial reprobe and depend on the stored capabilities. If it will be necessary to reprobe the capabilities in the future, we should do a full reprobe rather than this partial one. --- src/qemu/qemu_capabilities.c | 17 ----------------- src/qemu/qemu_capabilities.h | 3 --- src/qemu/qemu_process.c | 4 ---- 3 files changed, 24 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8901e7b..37e5302 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2937,23 +2937,6 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps, return 0; } -int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps, - qemuMonitorPtr mon) -{ - VIR_DEBUG("qemuCaps=%p mon=%p", qemuCaps, mon); - - if (qemuCaps->usedQMP) - return 0; - - if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0) - return -1; - - if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0) - return -1; - - return 0; -} - static bool virQEMUCapsCPUFilterFeatures(const char *name, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 5255815..be71507 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -403,9 +403,6 @@ virQEMUCapsPtr virQEMUCapsNew(void); int virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon); -int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps, - qemuMonitorPtr mon); - void virQEMUCapsSet(virQEMUCapsPtr qemuCaps, virQEMUCapsFlags flag) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab0c2c8..90f1101 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1723,10 +1723,6 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob, if (qemuMonitorSetCapabilities(priv->mon) < 0) goto cleanup; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON) && - virQEMUCapsProbeQMP(priv->qemuCaps, priv->mon) < 0) - goto cleanup; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT) && qemuMonitorSetMigrationCapability(priv->mon, QEMU_MONITOR_MIGRATION_CAPS_EVENTS, -- 2.10.2

On Fri, Nov 25, 2016 at 17:14:48 +0100, Peter Krempa wrote:
Thanks to the complex capability caching code virQEMUCapsProbeQMP was never called when we were starting a new qemu VM. On the other hand, when we are reconnecting to the qemu process we reload the capability list from the status XML file. This means that the flag preventing the function being called was not set and thus we partially reprobed some of the capabilities.
The recent addition of CPU hotplug clears the QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS if the machine does not support it. The partial re-probe on reconnect results into attempting to call the unsupported command and then killing the VM.
Remove the partial reprobe and depend on the stored capabilities. If it will be necessary to reprobe the capabilities in the future, we should do a full reprobe rather than this partial one. --- src/qemu/qemu_capabilities.c | 17 ----------------- src/qemu/qemu_capabilities.h | 3 --- src/qemu/qemu_process.c | 4 ---- 3 files changed, 24 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8901e7b..37e5302 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2937,23 +2937,6 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps, return 0; }
-int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps, - qemuMonitorPtr mon) -{ - VIR_DEBUG("qemuCaps=%p mon=%p", qemuCaps, mon); - - if (qemuCaps->usedQMP) - return 0; - - if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0) - return -1; - - if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0) - return -1; - - return 0; -} -
Oh, that's some seriously ancient piece of code which should have been removed ages ago. ACK Jirka

Peter, besides the trivial typo in the subject line (s/reprope/reprobe) I agree that this is the correct fix for the capability problem when reconnecting to the qemu processes. Thanks. I suggest that you mention in the patch description that libvirt starting with version 2.2.0 is terminating all running qemu domains if the qemu binaries do not implement/have not enabled the qmp command 'query-hotpluggable-cpus'. Including the user observable internal error message for reference would maybe help as well. (internal error: unable to execute QEMU command 'query-hotpluggable-cpus': The feature 'query-hotpluggable-cpus' is not enabled) In addition this patch looks to me like a good candidate for the maintenance branches. Tested-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> On 11/25/2016 05:14 PM, Peter Krempa wrote:
Thanks to the complex capability caching code virQEMUCapsProbeQMP was never called when we were starting a new qemu VM. On the other hand, when we are reconnecting to the qemu process we reload the capability list from the status XML file. This means that the flag preventing the function being called was not set and thus we partially reprobed some of the capabilities.
The recent addition of CPU hotplug clears the QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS if the machine does not support it. The partial re-probe on reconnect results into attempting to call the unsupported command and then killing the VM.
Remove the partial reprobe and depend on the stored capabilities. If it will be necessary to reprobe the capabilities in the future, we should do a full reprobe rather than this partial one. --- src/qemu/qemu_capabilities.c | 17 ----------------- src/qemu/qemu_capabilities.h | 3 --- src/qemu/qemu_process.c | 4 ---- 3 files changed, 24 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8901e7b..37e5302 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2937,23 +2937,6 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps, return 0; }
-int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps, - qemuMonitorPtr mon) -{ - VIR_DEBUG("qemuCaps=%p mon=%p", qemuCaps, mon); - - if (qemuCaps->usedQMP) - return 0; - - if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0) - return -1; - - if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0) - return -1; - - return 0; -} -
static bool virQEMUCapsCPUFilterFeatures(const char *name, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 5255815..be71507 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -403,9 +403,6 @@ virQEMUCapsPtr virQEMUCapsNew(void); int virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon);
-int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps, - qemuMonitorPtr mon); - void virQEMUCapsSet(virQEMUCapsPtr qemuCaps, virQEMUCapsFlags flag) ATTRIBUTE_NONNULL(1);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab0c2c8..90f1101 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1723,10 +1723,6 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob, if (qemuMonitorSetCapabilities(priv->mon) < 0) goto cleanup;
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON) && - virQEMUCapsProbeQMP(priv->qemuCaps, priv->mon) < 0) - goto cleanup; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT) && qemuMonitorSetMigrationCapability(priv->mon, QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On Mon, Nov 28, 2016 at 10:59:35 +0100, Boris Fiuczynski wrote:
Peter, besides the trivial typo in the subject line (s/reprope/reprobe) I agree that this is the correct fix for the capability problem when reconnecting to the qemu processes. Thanks.
I suggest that you mention in the patch description that libvirt starting with version 2.2.0 is terminating all running qemu domains if the qemu binaries do not implement/have not enabled the qmp command 'query-hotpluggable-cpus'. Including the user observable internal error message for reference would maybe help as well. (internal error: unable to execute QEMU command 'query-hotpluggable-cpus': The feature 'query-hotpluggable-cpus' is not enabled)
I've already pushed it with the original message, right after Jiri's review.
In addition this patch looks to me like a good candidate for the maintenance branches.
I've backported it to v2.2-maint. There are no other maint branches after that.
Tested-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Peter
participants (3)
-
Boris Fiuczynski
-
Jiri Denemark
-
Peter Krempa