Some layered products such as oVirt have requested a way to avoid being
blocked by guest agent commands when querying a loaded vm. For example,
many guest agent commands are polled periodically to monitor changes,
and rather than blocking the calling process, they'd prefer to simply
time out when an agent query is taking too long.

This patch adds a way for the user to specify a custom agent timeout
that is applied to all agent commands.

One special case to note here is the 'guest-sync' command. 'guest-sync'
is issued internally prior to calling any other command. (For example,
when libvirt wants to call 'guest-get-fsinfo', we first call
'guest-sync' and then call 'guest-get-fsinfo').

Previously, the 'guest-sync' command used a 5-second timeout
(VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT), whereas the actual command that
followed always blocked indefinitely
(VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK). As part of this patch, if a
custom timeout is specified that is shorter than
5 seconds,  this new timeout also used for 'guest-sync'. If there is no
custom timeout or if the custom timeout is longer than 5 seconds, we
will continue to use the 5-second timeout.

See https://bugzilla.redhat.com/show_bug.cgi?id=1705426 for additional details.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 include/libvirt/libvirt-qemu.h |  2 +
 src/driver-hypervisor.h        |  5 +++
 src/libvirt-qemu.c             | 40 ++++++++++++++++++++
 src/libvirt_qemu.syms          |  4 ++
 src/qemu/qemu_agent.c          | 69 +++++++++++++++++++---------------
 src/qemu/qemu_agent.h          |  3 ++
 src/qemu/qemu_driver.c         | 24 ++++++++++++
 src/qemu_protocol-structs      |  8 ++++
 src/remote/qemu_protocol.x     | 18 ++++++++-
 src/remote/remote_driver.c     |  1 +
 10 files changed, 143 insertions(+), 31 deletions(-)

diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h
index 891617443f..8d3cc776e9 100644
--- a/include/libvirt/libvirt-qemu.h
+++ b/include/libvirt/libvirt-qemu.h
@@ -53,6 +53,8 @@ typedef enum {
 char *virDomainQemuAgentCommand(virDomainPtr domain, const char *cmd,
                                 int timeout, unsigned int flags);
 
+int virDomainQemuAgentSetTimeout(virDomainPtr domain, int timeout);
+
 /**
  * virConnectDomainQemuMonitorEventCallback:
  * @conn: the connection pointer
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index 015b2cd01c..2f17bff844 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1372,6 +1372,10 @@ typedef int
                             int *nparams,
                             unsigned int flags);
 
+typedef int
+(*virDrvDomainQemuAgentSetTimeout)(virDomainPtr domain,
+                                   int timeout);
+
 typedef struct _virHypervisorDriver virHypervisorDriver;
 typedef virHypervisorDriver *virHypervisorDriverPtr;
 
@@ -1632,4 +1636,5 @@ struct _virHypervisorDriver {
     virDrvDomainCheckpointGetParent domainCheckpointGetParent;
     virDrvDomainCheckpointDelete domainCheckpointDelete;
     virDrvDomainGetGuestInfo domainGetGuestInfo;
+    virDrvDomainQemuAgentSetTimeout domainQemuAgentSetTimeout;
 };
diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c
index 1afb5fe529..73f119cb23 100644
--- a/src/libvirt-qemu.c
+++ b/src/libvirt-qemu.c
@@ -216,6 +216,46 @@ virDomainQemuAgentCommand(virDomainPtr domain,
     return NULL;
 }
 
+/**
+ * virDomainQemuAgentSetTimeout:
+ * @domain: a domain object
+ * @timeout: timeout in seconds
+ *
+ * Set how long to wait for a response from qemu agent commands. By default,
+ * agent commands block forever waiting for a response.
+ *
+ * @timeout must be -2, -1, 0 or positive.
+ * VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK(-2): meaning to block forever waiting for
+ * a result.
+ * VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT(-1): use default timeout value.
+ * VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT(0): does not wait.
+ * positive value: wait for @timeout seconds
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+virDomainQemuAgentSetTimeout(virDomainPtr domain,
+                             int timeout)
+{
+    virConnectPtr conn;
+
+    virResetLastError();
+
+    virCheckDomainReturn(domain, -1);
+    conn = domain->conn;
+
+    if (conn->driver->domainQemuAgentSetTimeout) {
+        if (conn->driver->domainQemuAgentSetTimeout(domain, timeout) < 0)
+            goto error;
+        return 0;
+    }
+
+    virReportUnsupportedError();
+
+ error:
+    virDispatchError(conn);
+    return -1;
+}
 
 /**
  * virConnectDomainQemuMonitorEventRegister:
diff --git a/src/libvirt_qemu.syms b/src/libvirt_qemu.syms
index 3a297e3a2b..348caea72e 100644
--- a/src/libvirt_qemu.syms
+++ b/src/libvirt_qemu.syms
@@ -30,3 +30,7 @@ LIBVIRT_QEMU_1.2.3 {
         virConnectDomainQemuMonitorEventDeregister;
         virConnectDomainQemuMonitorEventRegister;
 } LIBVIRT_QEMU_0.10.0;
+LIBVIRT_QEMU_5.8.0 {
+    global:
+        virDomainQemuAgentSetTimeout;
+} LIBVIRT_QEMU_1.2.3;
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 34e1a85d64..86352aaec5 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -128,6 +128,7 @@ struct _qemuAgent {
      * but fire up an event on qemu monitor instead.
      * Take that as indication of successful completion */
     qemuAgentEvent await_event;
+    int timeout;
 };
 
 static virClassPtr qemuAgentClass;
@@ -696,6 +697,8 @@ qemuAgentOpen(virDomainObjPtr vm,
     if (!(mon = virObjectLockableNew(qemuAgentClass)))
         return NULL;
 
+    /* agent commands block by default, user can choose different behavior */
+    mon->timeout = VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK;
     mon->fd = -1;
     if (virCondInit(&mon->notify) < 0) {
         virReportSystemError(errno, "%s",
@@ -851,6 +854,11 @@ static int qemuAgentSend(qemuAgentPtr mon,
             return -1;
         if (seconds == VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT)
             seconds = QEMU_AGENT_WAIT_TIME;
+
+        /* if user specified a custom agent timeout that is lower than the
+         * default timeout, use the shorter timeout instead */
+        if ((mon->timeout > 0) && (mon->timeout < seconds))
+            seconds = mon->timeout;
         then = now + seconds * 1000ull;
     }
 
@@ -1305,8 +1313,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
     if (!cmd)
         goto cleanup;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
         goto cleanup;
 
     if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
@@ -1343,8 +1350,7 @@ int qemuAgentFSThaw(qemuAgentPtr mon)
     if (!cmd)
         return -1;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
         goto cleanup;
 
     if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
@@ -1381,8 +1387,7 @@ qemuAgentSuspend(qemuAgentPtr mon,
         return -1;
 
     mon->await_event = QEMU_AGENT_EVENT_SUSPEND;
-    ret = qemuAgentCommand(mon, cmd, &reply, false,
-                           VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
+    ret = qemuAgentCommand(mon, cmd, &reply, false, mon->timeout);
 
     virJSONValueFree(cmd);
     virJSONValueFree(reply);
@@ -1438,8 +1443,7 @@ qemuAgentFSTrim(qemuAgentPtr mon,
     if (!cmd)
         return ret;
 
-    ret = qemuAgentCommand(mon, cmd, &reply, false,
-                           VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
+    ret = qemuAgentCommand(mon, cmd, &reply, false, mon->timeout);
 
     virJSONValueFree(cmd);
     virJSONValueFree(reply);
@@ -1460,8 +1464,7 @@ qemuAgentGetVCPUs(qemuAgentPtr mon,
     if (!(cmd = qemuAgentMakeCommand("guest-get-vcpus", NULL)))
         return -1;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
         goto cleanup;
 
     if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
@@ -1576,8 +1579,7 @@ qemuAgentSetVCPUsCommand(qemuAgentPtr mon,
                                      NULL)))
         goto cleanup;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
         goto cleanup;
 
     /* All negative values are invalid. Return of 0 is bogus since we wouldn't
@@ -1732,8 +1734,7 @@ qemuAgentGetHostname(qemuAgentPtr mon,
     if (!cmd)
         return ret;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
         if (qemuAgentErrorCommandUnsupported(reply))
             ret = -2;
         goto cleanup;
@@ -1778,8 +1779,7 @@ qemuAgentGetTime(qemuAgentPtr mon,
     if (!cmd)
         return ret;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
         goto cleanup;
 
     if (virJSONValueObjectGetNumberUlong(reply, "return", &json_time) < 0) {
@@ -1844,8 +1844,7 @@ qemuAgentSetTime(qemuAgentPtr mon,
     if (!cmd)
         return ret;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
         goto cleanup;
 
     ret = 0;
@@ -2054,8 +2053,7 @@ qemuAgentGetFSInfoInternal(qemuAgentPtr mon,
     if (!cmd)
         return ret;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
         if (qemuAgentErrorCommandUnsupported(reply))
             ret = -2;
         goto cleanup;
@@ -2347,8 +2345,7 @@ qemuAgentGetInterfaces(qemuAgentPtr mon,
     if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL)))
         goto cleanup;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
         goto cleanup;
 
     if (!(ret_array = virJSONValueObjectGet(reply, "return"))) {
@@ -2529,8 +2526,7 @@ qemuAgentSetUserPassword(qemuAgentPtr mon,
                                      NULL)))
         goto cleanup;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)
         goto cleanup;
 
     ret = 0;
@@ -2561,8 +2557,7 @@ qemuAgentGetUsers(qemuAgentPtr mon,
     if (!(cmd = qemuAgentMakeCommand("guest-get-users", NULL)))
         return -1;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
         if (qemuAgentErrorCommandUnsupported(reply))
             return -2;
         return -1;
@@ -2651,8 +2646,7 @@ qemuAgentGetOSInfo(qemuAgentPtr mon,
     if (!(cmd = qemuAgentMakeCommand("guest-get-osinfo", NULL)))
         return -1;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
         if (qemuAgentErrorCommandUnsupported(reply))
             return -2;
         return -1;
@@ -2707,8 +2701,7 @@ qemuAgentGetTimezone(qemuAgentPtr mon,
     if (!(cmd = qemuAgentMakeCommand("guest-get-timezone", NULL)))
         return -1;
 
-    if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
+    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0) {
         if (qemuAgentErrorCommandUnsupported(reply))
             return -2;
         return -1;
@@ -2737,3 +2730,19 @@ qemuAgentGetTimezone(qemuAgentPtr mon,
 
     return 0;
 }
+
+int
+qemuAgentSetTimeout(qemuAgentPtr mon,
+                    int timeout)
+{
+    if (timeout < VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("guest agent timeout '%d' is "
+                         "less than the minimum '%d'"),
+                       timeout, VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN);
+        return -1;
+    }
+
+    mon->timeout = timeout;
+    return 0;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 78e648992a..4037934b91 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -140,3 +140,6 @@ int qemuAgentGetTimezone(qemuAgentPtr mon,
                          virTypedParameterPtr *params,
                          int *nparams,
                          int *maxparams);
+
+int qemuAgentSetTimeout(qemuAgentPtr mon,
+                        int timeout);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1e041a8bac..09251cc9e2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -23434,6 +23434,29 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
     return ret;
 }
 
+static int
+qemuDomainQemuAgentSetTimeout(virDomainPtr dom,
+                              int timeout)
+{
+    virDomainObjPtr vm = NULL;
+    qemuAgentPtr agent;
+    int ret = -1;
+
+    if (!(vm = qemuDomObjFromDomain(dom)))
+        goto cleanup;
+
+    if (virDomainQemuAgentSetTimeoutEnsureACL(dom->conn, vm->def) < 0)
+        goto cleanup;
+
+    agent = qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentSetTimeout(agent, timeout);
+    qemuDomainObjExitAgent(vm, agent);
+
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    return ret;
+}
+
 static virHypervisorDriver qemuHypervisorDriver = {
     .name = QEMU_DRIVER_NAME,
     .connectURIProbe = qemuConnectURIProbe,
@@ -23670,6 +23693,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
     .domainCheckpointGetParent = qemuDomainCheckpointGetParent, /* 5.6.0 */
     .domainCheckpointDelete = qemuDomainCheckpointDelete, /* 5.6.0 */
     .domainGetGuestInfo = qemuDomainGetGuestInfo, /* 5.7.0 */
+    .domainQemuAgentSetTimeout = qemuDomainQemuAgentSetTimeout, /* 5.8.0 */
 };
 
 
diff --git a/src/qemu_protocol-structs b/src/qemu_protocol-structs
index 8501543cd9..be9e739bc6 100644
--- a/src/qemu_protocol-structs
+++ b/src/qemu_protocol-structs
@@ -47,6 +47,13 @@ struct qemu_domain_monitor_event_msg {
         u_int                      micros;
         remote_string              details;
 };
+struct qemu_domain_agent_set_timeout_args {
+        remote_nonnull_domain      dom;
+        int                        timeout;
+};
+struct qemu_domain_agent_set_timeout_ret {
+        int                        result;
+};
 enum qemu_procedure {
         QEMU_PROC_DOMAIN_MONITOR_COMMAND = 1,
         QEMU_PROC_DOMAIN_ATTACH = 2,
@@ -54,4 +61,5 @@ enum qemu_procedure {
         QEMU_PROC_CONNECT_DOMAIN_MONITOR_EVENT_REGISTER = 4,
         QEMU_PROC_CONNECT_DOMAIN_MONITOR_EVENT_DEREGISTER = 5,
         QEMU_PROC_DOMAIN_MONITOR_EVENT = 6,
+        QEMU_PROC_DOMAIN_AGENT_SET_TIMEOUT = 7,
 };
diff --git a/src/remote/qemu_protocol.x b/src/remote/qemu_protocol.x
index 423e8fadaf..9be2cfa5b7 100644
--- a/src/remote/qemu_protocol.x
+++ b/src/remote/qemu_protocol.x
@@ -80,6 +80,15 @@ struct qemu_domain_monitor_event_msg {
     remote_string details;
 };
 
+struct qemu_domain_agent_set_timeout_args {
+    remote_nonnull_domain dom;
+    int timeout;
+};
+
+struct qemu_domain_agent_set_timeout_ret {
+    int result;
+};
+
 /* Define the program number, protocol version and procedure numbers here. */
 const QEMU_PROGRAM = 0x20008087;
 const QEMU_PROTOCOL_VERSION = 1;
@@ -152,5 +161,12 @@ enum qemu_procedure {
      * @generate: both
      * @acl: none
      */
-    QEMU_PROC_DOMAIN_MONITOR_EVENT = 6
+    QEMU_PROC_DOMAIN_MONITOR_EVENT = 6,
+
+    /**
+     * @generate: both
+     * @priority: low
+     * @acl: domain:write
+     */
+    QEMU_PROC_DOMAIN_AGENT_SET_TIMEOUT = 7
 };
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 8789c5da00..94688f9c2a 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8748,6 +8748,7 @@ static virHypervisorDriver hypervisor_driver = {
     .domainCheckpointGetParent = remoteDomainCheckpointGetParent, /* 5.6.0 */
     .domainCheckpointDelete = remoteDomainCheckpointDelete, /* 5.6.0 */
     .domainGetGuestInfo = remoteDomainGetGuestInfo, /* 5.7.0 */
+    .domainQemuAgentSetTimeout = remoteDomainQemuAgentSetTimeout, /* 5.8.0 */
 };
 
 static virNetworkDriver network_driver = {
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Oct 03, 2019 at 16:52:12 -0500, Jonathon Jongsma wrote:
> Some layered products such as oVirt have requested a way to avoid being
> blocked by guest agent commands when querying a loaded vm. For example,
> many guest agent commands are polled periodically to monitor changes,
> and rather than blocking the calling process, they'd prefer to simply
> time out when an agent query is taking too long.
> 
> This patch adds a way for the user to specify a custom agent timeout
> that is applied to all agent commands.
> 
> One special case to note here is the 'guest-sync' command. 'guest-sync'
> is issued internally prior to calling any other command. (For example,
> when libvirt wants to call 'guest-get-fsinfo', we first call
> 'guest-sync' and then call 'guest-get-fsinfo').
> 
> Previously, the 'guest-sync' command used a 5-second timeout
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT), whereas the actual command that
> followed always blocked indefinitely
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK). As part of this patch, if a
> custom timeout is specified that is shorter than
> 5 seconds,  this new timeout also used for 'guest-sync'. If there is no
> custom timeout or if the custom timeout is longer than 5 seconds, we
> will continue to use the 5-second timeout.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=1705426 for additional details.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  include/libvirt/libvirt-qemu.h |  2 +
>  src/driver-hypervisor.h        |  5 +++
>  src/libvirt-qemu.c             | 40 ++++++++++++++++++++
>  src/libvirt_qemu.syms          |  4 ++
>  src/qemu/qemu_agent.c          | 69 +++++++++++++++++++---------------
>  src/qemu/qemu_agent.h          |  3 ++
>  src/qemu/qemu_driver.c         | 24 ++++++++++++
>  src/qemu_protocol-structs      |  8 ++++
>  src/remote/qemu_protocol.x     | 18 ++++++++-
>  src/remote/remote_driver.c     |  1 +
>  10 files changed, 143 insertions(+), 31 deletions(-)

[...]

> @@ -2737,3 +2730,19 @@ qemuAgentGetTimezone(qemuAgentPtr mon,
>  
>      return 0;
>  }
> +
> +int
> +qemuAgentSetTimeout(qemuAgentPtr mon,
> +                    int timeout)
> +{
> +    if (timeout < VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("guest agent timeout '%d' is "
> +                         "less than the minimum '%d'"),
> +                       timeout, VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN);

This error is misleading as -1 and -2 are special values and not actual
timeout.

> +        return -1;
> +    }
> +
> +    mon->timeout = timeout;
> +    return 0;
> +}

[...]

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1e041a8bac..09251cc9e2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -23434,6 +23434,29 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
>      return ret;
>  }
>  
> +static int
> +qemuDomainQemuAgentSetTimeout(virDomainPtr dom,
> +                              int timeout)
> +{
> +    virDomainObjPtr vm = NULL;
> +    qemuAgentPtr agent;
> +    int ret = -1;
> +
> +    if (!(vm = qemuDomObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainQemuAgentSetTimeoutEnsureACL(dom->conn, vm->def) < 0)
> +        goto cleanup;
> +
> +    agent = qemuDomainObjEnterAgent(vm);

You must acquire a job on @vm if you want to call this:

/*
 * obj must be locked before calling
 *
 * To be called immediately before any QEMU agent API call.
 * Must have already called qemuDomainObjBeginAgentJob() or
 * qemuDomainObjBeginJobWithAgent() and checked that the VM is
 * still active.
 *
 * To be followed with qemuDomainObjExitAgent() once complete
 */
qemuAgentPtr
qemuDomainObjEnterAgent(virDomainObjPtr obj)


> +    ret = qemuAgentSetTimeout(agent, timeout);
> +    qemuDomainObjExitAgent(vm, agent);

Also this API is inherently racy if you have two clients setting the
timeout and it will influence calls of a different client.

IMO the only reasonable approach is to add new APIs which have a
'timeout' parameter for any agent API which requires tunable timeout to
prevent any races and unexpected behaviour.

Other possibility may be to add a qemu config file option for this but
that is not really dynamic.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/4/19 8:14 AM, Peter Krempa wrote:
> On Thu, Oct 03, 2019 at 16:52:12 -0500, Jonathon Jongsma wrote:
>> Some layered products such as oVirt have requested a way to avoid being
>> blocked by guest agent commands when querying a loaded vm. For example,
>> many guest agent commands are polled periodically to monitor changes,
>> and rather than blocking the calling process, they'd prefer to simply
>> time out when an agent query is taking too long.
>>
>> This patch adds a way for the user to specify a custom agent timeout
>> that is applied to all agent commands.
>>
>> One special case to note here is the 'guest-sync' command. 'guest-sync'
>> is issued internally prior to calling any other command. (For example,
>> when libvirt wants to call 'guest-get-fsinfo', we first call
>> 'guest-sync' and then call 'guest-get-fsinfo').
>>
>> Previously, the 'guest-sync' command used a 5-second timeout
>> (VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT), whereas the actual command that
>> followed always blocked indefinitely
>> (VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK). As part of this patch, if a
>> custom timeout is specified that is shorter than
>> 5 seconds,  this new timeout also used for 'guest-sync'. If there is no
>> custom timeout or if the custom timeout is longer than 5 seconds, we
>> will continue to use the 5-second timeout.
>>
>> See https://bugzilla.redhat.com/show_bug.cgi?id=1705426 for additional details.
>>
>> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
>> ---
>>   include/libvirt/libvirt-qemu.h |  2 +
>>   src/driver-hypervisor.h        |  5 +++
>>   src/libvirt-qemu.c             | 40 ++++++++++++++++++++
>>   src/libvirt_qemu.syms          |  4 ++
>>   src/qemu/qemu_agent.c          | 69 +++++++++++++++++++---------------
>>   src/qemu/qemu_agent.h          |  3 ++
>>   src/qemu/qemu_driver.c         | 24 ++++++++++++
>>   src/qemu_protocol-structs      |  8 ++++
>>   src/remote/qemu_protocol.x     | 18 ++++++++-
>>   src/remote/remote_driver.c     |  1 +
>>   10 files changed, 143 insertions(+), 31 deletions(-)
> 
> [...]
> 
>> @@ -2737,3 +2730,19 @@ qemuAgentGetTimezone(qemuAgentPtr mon,
>>   
>>       return 0;
>>   }
>> +
>> +int
>> +qemuAgentSetTimeout(qemuAgentPtr mon,
>> +                    int timeout)
>> +{
>> +    if (timeout < VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN) {
>> +        virReportError(VIR_ERR_INVALID_ARG,
>> +                       _("guest agent timeout '%d' is "
>> +                         "less than the minimum '%d'"),
>> +                       timeout, VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN);
> 
> This error is misleading as -1 and -2 are special values and not actual
> timeout.
> 
>> +        return -1;
>> +    }
>> +
>> +    mon->timeout = timeout;
>> +    return 0;
>> +}
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 1e041a8bac..09251cc9e2 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -23434,6 +23434,29 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
>>       return ret;
>>   }
>>   
>> +static int
>> +qemuDomainQemuAgentSetTimeout(virDomainPtr dom,
>> +                              int timeout)
>> +{
>> +    virDomainObjPtr vm = NULL;
>> +    qemuAgentPtr agent;
>> +    int ret = -1;
>> +
>> +    if (!(vm = qemuDomObjFromDomain(dom)))
>> +        goto cleanup;
>> +
>> +    if (virDomainQemuAgentSetTimeoutEnsureACL(dom->conn, vm->def) < 0)
>> +        goto cleanup;
>> +
>> +    agent = qemuDomainObjEnterAgent(vm);
> 
> You must acquire a job on @vm if you want to call this:
> 
> /*
>   * obj must be locked before calling
>   *
>   * To be called immediately before any QEMU agent API call.
>   * Must have already called qemuDomainObjBeginAgentJob() or
>   * qemuDomainObjBeginJobWithAgent() and checked that the VM is
>   * still active.
>   *
>   * To be followed with qemuDomainObjExitAgent() once complete
>   */
> qemuAgentPtr
> qemuDomainObjEnterAgent(virDomainObjPtr obj)
> 
> 
>> +    ret = qemuAgentSetTimeout(agent, timeout);
>> +    qemuDomainObjExitAgent(vm, agent);
> 
> Also this API is inherently racy if you have two clients setting the
> timeout and it will influence calls of a different client.

How is this different to one client calling virDomainCreate() whilst the 
other calls virDomainDestroy() over the same domain? I believe our APIs 
are build on assumption that management layer calls only those APIs and 
in the right sequence so that desired state of things is achieved. If 
we'd have two management apps competing over the same domain then agent 
timeout is one of the smaller problems IMO.

> 
> IMO the only reasonable approach is to add new APIs which have a
> 'timeout' parameter for any agent API which requires tunable timeout to
> prevent any races and unexpected behaviour.

Thing is, we don't tell users which APIs will talk to monitor and which 
will talk to the agent. For instance, qemuDomainShutdownFlags() prefers 
agent, but it wasn't always like that. And even if we did tell users 
that, we would need to document that @timeout applies to guest agent 
only and might not be honoured if API decides to talk to monitor.

> 
> Other possibility may be to add a qemu config file option for this but
> that is not really dynamic.

I'd rather see this patch in than yet another qemu.conf knob.

However, reading the linked bug, it looks like to fix the problem 
described there a much simpler solution is viable.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Oct 04, 2019 at 08:14:39AM +0200, Peter Krempa wrote:
> On Thu, Oct 03, 2019 at 16:52:12 -0500, Jonathon Jongsma wrote:
> > Some layered products such as oVirt have requested a way to avoid being
> > blocked by guest agent commands when querying a loaded vm. For example,
> > many guest agent commands are polled periodically to monitor changes,
> > and rather than blocking the calling process, they'd prefer to simply
> > time out when an agent query is taking too long.
> > 
> > This patch adds a way for the user to specify a custom agent timeout
> > that is applied to all agent commands.
> > 
> > One special case to note here is the 'guest-sync' command. 'guest-sync'
> > is issued internally prior to calling any other command. (For example,
> > when libvirt wants to call 'guest-get-fsinfo', we first call
> > 'guest-sync' and then call 'guest-get-fsinfo').
> > 
> > Previously, the 'guest-sync' command used a 5-second timeout
> > (VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT), whereas the actual command that
> > followed always blocked indefinitely
> > (VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK). As part of this patch, if a
> > custom timeout is specified that is shorter than
> > 5 seconds,  this new timeout also used for 'guest-sync'. If there is no
> > custom timeout or if the custom timeout is longer than 5 seconds, we
> > will continue to use the 5-second timeout.
> > 
> > See https://bugzilla.redhat.com/show_bug.cgi?id=1705426 for additional details.
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>


> > @@ -2737,3 +2730,19 @@ qemuAgentGetTimezone(qemuAgentPtr mon,
> >  
> >      return 0;
> >  }
> > +
> > +int
> > +qemuAgentSetTimeout(qemuAgentPtr mon,
> > +                    int timeout)
> > +{
> > +    if (timeout < VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN) {
> > +        virReportError(VIR_ERR_INVALID_ARG,
> > +                       _("guest agent timeout '%d' is "
> > +                         "less than the minimum '%d'"),
> > +                       timeout, VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN);
> 
> This error is misleading as -1 and -2 are special values and not actual
> timeout.
> 
> > +        return -1;
> > +    }
> > +
> > +    mon->timeout = timeout;
> > +    return 0;
> > +}
> 
> [...]
> 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 1e041a8bac..09251cc9e2 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -23434,6 +23434,29 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
> >      return ret;
> >  }
> >  
> > +static int
> > +qemuDomainQemuAgentSetTimeout(virDomainPtr dom,
> > +                              int timeout)
> > +{
> > +    virDomainObjPtr vm = NULL;
> > +    qemuAgentPtr agent;
> > +    int ret = -1;
> > +
> > +    if (!(vm = qemuDomObjFromDomain(dom)))
> > +        goto cleanup;
> > +
> > +    if (virDomainQemuAgentSetTimeoutEnsureACL(dom->conn, vm->def) < 0)
> > +        goto cleanup;
> > +
> > +    agent = qemuDomainObjEnterAgent(vm);
> 
> You must acquire a job on @vm if you want to call this:
> 
> /*
>  * obj must be locked before calling
>  *
>  * To be called immediately before any QEMU agent API call.
>  * Must have already called qemuDomainObjBeginAgentJob() or
>  * qemuDomainObjBeginJobWithAgent() and checked that the VM is
>  * still active.
>  *
>  * To be followed with qemuDomainObjExitAgent() once complete
>  */
> qemuAgentPtr
> qemuDomainObjEnterAgent(virDomainObjPtr obj)
> 
> 
> > +    ret = qemuAgentSetTimeout(agent, timeout);
> > +    qemuDomainObjExitAgent(vm, agent);
> 
> Also this API is inherently racy if you have two clients setting the
> timeout and it will influence calls of a different client.
> 
> IMO the only reasonable approach is to add new APIs which have a
> 'timeout' parameter for any agent API which requires tunable timeout to
> prevent any races and unexpected behaviour.
> 
> Other possibility may be to add a qemu config file option for this but
> that is not really dynamic.

I guess the key question is whether we actually have a compelling
use case to vary the timeout on a per-command basis.

If not, then we could do fine with a global config that is either
recorded in the domain XML, or in the global QEMU config.

The possible causes of slowness are

 - Host is overloaded so the guest is not being scheduled
 - Guest is overloaded so the agent is not being scheduled
 - The agent has crash/deadlocked
 - The agent is maliciously not responding
 - The command genuinely takes a long time to perform its action

The first 4 of those are fine with a global timeout either on guest or
in the driver.

Only the last one really pushes for having this per-public API.

Looking commands, ones I think can take a long time are

 - FS trim - this can run for many minutes if there's alot to trim
 - FS freeze - this might need to wait for apps to quiesce their I/O IIUC
 - Guest exec - it can run any command

The latter isn't used in libvirt, can be be run via the QGA passthrough
api in libvirt-qemu.so

So sadly I think we genuinely do have a need for per-commad timeouts,
for at least some of the API calls. I don't think all of them need it
though.

I though we could likely add a qemu.conf setting for the globak timeout,
but then add a timeout to individual APIs in specific cases where we
know they can genuinely take a very long time to execute.

We must also ensure we NEVER block any regular libvirt APIs when a
guest agent comamnd is running.

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Hi, it seems the discussion has stalled so let me give the oVirt
perspective here to (hopefully) revive it. The place where we need to
configure the timeout the most is for periodic queries of the guest
state. As opposed to that, setting the timeout e.g. on shutdown command
does make sense (instead of blocking), but it is not that crucial to
have a great degree of flexibility.

Several more comments below.


On Fri, Oct 04, 2019 at 10:13:11AM +0100, Daniel P. Berrangé wrote:
> On Fri, Oct 04, 2019 at 08:14:39AM +0200, Peter Krempa wrote:
> > On Thu, Oct 03, 2019 at 16:52:12 -0500, Jonathon Jongsma wrote:
> > > Some layered products such as oVirt have requested a way to avoid being
> > > blocked by guest agent commands when querying a loaded vm. For example,
> > > many guest agent commands are polled periodically to monitor changes,
> > > and rather than blocking the calling process, they'd prefer to simply
> > > time out when an agent query is taking too long.
> > > 
> > > This patch adds a way for the user to specify a custom agent timeout
> > > that is applied to all agent commands.
> > > 
> > > One special case to note here is the 'guest-sync' command. 'guest-sync'
> > > is issued internally prior to calling any other command. (For example,
> > > when libvirt wants to call 'guest-get-fsinfo', we first call
> > > 'guest-sync' and then call 'guest-get-fsinfo').
> > > 
> > > Previously, the 'guest-sync' command used a 5-second timeout
> > > (VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT), whereas the actual command that
> > > followed always blocked indefinitely
> > > (VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK). As part of this patch, if a
> > > custom timeout is specified that is shorter than
> > > 5 seconds,  this new timeout also used for 'guest-sync'. If there is no
> > > custom timeout or if the custom timeout is longer than 5 seconds, we
> > > will continue to use the 5-second timeout.
> > > 
> > > See https://bugzilla.redhat.com/show_bug.cgi?id=1705426 for additional details.
> > > 
> > > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> 
> 
> > > @@ -2737,3 +2730,19 @@ qemuAgentGetTimezone(qemuAgentPtr mon,
> > >  
> > >      return 0;
> > >  }
> > > +
> > > +int
> > > +qemuAgentSetTimeout(qemuAgentPtr mon,
> > > +                    int timeout)
> > > +{
> > > +    if (timeout < VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN) {
> > > +        virReportError(VIR_ERR_INVALID_ARG,
> > > +                       _("guest agent timeout '%d' is "
> > > +                         "less than the minimum '%d'"),
> > > +                       timeout, VIR_DOMAIN_QEMU_AGENT_COMMAND_MIN);
> > 
> > This error is misleading as -1 and -2 are special values and not actual
> > timeout.
> > 
> > > +        return -1;
> > > +    }
> > > +
> > > +    mon->timeout = timeout;
> > > +    return 0;
> > > +}
> > 
> > [...]
> > 
> > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > index 1e041a8bac..09251cc9e2 100644
> > > --- a/src/qemu/qemu_driver.c
> > > +++ b/src/qemu/qemu_driver.c
> > > @@ -23434,6 +23434,29 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
> > >      return ret;
> > >  }
> > >  
> > > +static int
> > > +qemuDomainQemuAgentSetTimeout(virDomainPtr dom,
> > > +                              int timeout)
> > > +{
> > > +    virDomainObjPtr vm = NULL;
> > > +    qemuAgentPtr agent;
> > > +    int ret = -1;
> > > +
> > > +    if (!(vm = qemuDomObjFromDomain(dom)))
> > > +        goto cleanup;
> > > +
> > > +    if (virDomainQemuAgentSetTimeoutEnsureACL(dom->conn, vm->def) < 0)
> > > +        goto cleanup;
> > > +
> > > +    agent = qemuDomainObjEnterAgent(vm);
> > 
> > You must acquire a job on @vm if you want to call this:
> > 
> > /*
> >  * obj must be locked before calling
> >  *
> >  * To be called immediately before any QEMU agent API call.
> >  * Must have already called qemuDomainObjBeginAgentJob() or
> >  * qemuDomainObjBeginJobWithAgent() and checked that the VM is
> >  * still active.
> >  *
> >  * To be followed with qemuDomainObjExitAgent() once complete
> >  */
> > qemuAgentPtr
> > qemuDomainObjEnterAgent(virDomainObjPtr obj)
> > 
> > 
> > > +    ret = qemuAgentSetTimeout(agent, timeout);
> > > +    qemuDomainObjExitAgent(vm, agent);
> > 
> > Also this API is inherently racy if you have two clients setting the
> > timeout and it will influence calls of a different client.

As pointed in other part of the thread this is not a problem, rather
it's a fact about libvirt. You assume the management app knows what it
is doing. And if there are more management apps or users meddling with
libvirt there's always the risk of stepping on each others toes.


> > 
> > IMO the only reasonable approach is to add new APIs which have a
> > 'timeout' parameter for any agent API which requires tunable timeout to
> > prevent any races and unexpected behaviour.
> > 
> > Other possibility may be to add a qemu config file option for this but
> > that is not really dynamic.
> 
> I guess the key question is whether we actually have a compelling
> use case to vary the timeout on a per-command basis.
> 
> If not, then we could do fine with a global config that is either
> recorded in the domain XML, or in the global QEMU config.
> 

What I strive for is to keep the allowed timeout to minimum. Especially
because we have to query all the machines sequentially for the lack of
threading (either in libvirt or in oVirt). One machine taking too long
to respond can delay the queries for all the other machines.

> The possible causes of slowness are
> 
>  - Host is overloaded so the guest is not being scheduled
>  - Guest is overloaded so the agent is not being scheduled

This are generally not a problem for keeping the timeout to minimum. We
can expect that the next time we issue the query all will go well. And
if guest/host is overloaded for a long term there's not much we can do
about it (waiting longer for the command to complete won't help).

>  - The agent has crash/deadlocked

This reminds me a different issue which is there's no API to find out
whether the agent is connected or not. One either has to "give it a
shot" and handle failure, or check the domain XML (which is
inefficient).


>  - The agent is maliciously not responding

This is a interesting point, but I would put it in the same group as
"guest overloaded" from our POV.

>  - The command genuinely takes a long time to perform its action

Now this is a genuine problem. Again from our POV it makes sense to keep
long-running commands separate from the group of quick commands and run
those in a separate loop with higher timeout but less often.

> 
> The first 4 of those are fine with a global timeout either on guest or
> in the driver.
> 
> Only the last one really pushes for having this per-public API.
> 
> Looking commands, ones I think can take a long time are
> 
>  - FS trim - this can run for many minutes if there's alot to trim
>  - FS freeze - this might need to wait for apps to quiesce their I/O IIUC
>  - Guest exec - it can run any command
> 

We don't really care about the timeout here. At least not at the moment.


> The latter isn't used in libvirt, can be be run via the QGA passthrough
> api in libvirt-qemu.so
> 
> So sadly I think we genuinely do have a need for per-commad timeouts,
> for at least some of the API calls. I don't think all of them need it
> though.
> 
> I though we could likely add a qemu.conf setting for the globak timeout,
> but then add a timeout to individual APIs in specific cases where we
> know they can genuinely take a very long time to execute.

I would be against any timeout that cannot be changed via API. This
would not give us the flexibility we require.

Hope this helps,

    Tomas


> 
> We must also ensure we NEVER block any regular libvirt APIs when a
> guest agent comamnd is running.
> 
> 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 :|
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Oct 03, 2019 at 04:52:12PM -0500, Jonathon Jongsma wrote:
> Some layered products such as oVirt have requested a way to avoid being
> blocked by guest agent commands when querying a loaded vm. For example,
> many guest agent commands are polled periodically to monitor changes,
> and rather than blocking the calling process, they'd prefer to simply
> time out when an agent query is taking too long.
> 
> This patch adds a way for the user to specify a custom agent timeout
> that is applied to all agent commands.
> 
> One special case to note here is the 'guest-sync' command. 'guest-sync'
> is issued internally prior to calling any other command. (For example,
> when libvirt wants to call 'guest-get-fsinfo', we first call
> 'guest-sync' and then call 'guest-get-fsinfo').
> 
> Previously, the 'guest-sync' command used a 5-second timeout
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT), whereas the actual command that
> followed always blocked indefinitely
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK). As part of this patch, if a
> custom timeout is specified that is shorter than
> 5 seconds,  this new timeout also used for 'guest-sync'. If there is no
> custom timeout or if the custom timeout is longer than 5 seconds, we
> will continue to use the 5-second timeout.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=1705426 for additional details.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  include/libvirt/libvirt-qemu.h |  2 +

For something that is fullfilling a RFE from oVirt, adding an API in
libvirt-qemu.h is not a viable solution.

We class libvirt-qemu.h API usage as unsupported, as it is a
providing a backdoor around libvirt, for dev/test scenarios
only.

For anything that is to be used in production, it has to be
fully supported in the main libvirt API or XML schema.

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/3/19 11:52 PM, Jonathon Jongsma wrote:
> Some layered products such as oVirt have requested a way to avoid being
> blocked by guest agent commands when querying a loaded vm. For example,
> many guest agent commands are polled periodically to monitor changes,
> and rather than blocking the calling process, they'd prefer to simply
> time out when an agent query is taking too long.
> 
> This patch adds a way for the user to specify a custom agent timeout
> that is applied to all agent commands.
> 
> One special case to note here is the 'guest-sync' command. 'guest-sync'
> is issued internally prior to calling any other command. (For example,
> when libvirt wants to call 'guest-get-fsinfo', we first call
> 'guest-sync' and then call 'guest-get-fsinfo').
> 
> Previously, the 'guest-sync' command used a 5-second timeout
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT), whereas the actual command that
> followed always blocked indefinitely
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK). As part of this patch, if a
> custom timeout is specified that is shorter than
> 5 seconds,  this new timeout also used for 'guest-sync'. If there is no
> custom timeout or if the custom timeout is longer than 5 seconds, we
> will continue to use the 5-second timeout.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=1705426 for additional details.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>   include/libvirt/libvirt-qemu.h |  2 +
>   src/driver-hypervisor.h        |  5 +++
>   src/libvirt-qemu.c             | 40 ++++++++++++++++++++
>   src/libvirt_qemu.syms          |  4 ++
>   src/qemu/qemu_agent.c          | 69 +++++++++++++++++++---------------
>   src/qemu/qemu_agent.h          |  3 ++
>   src/qemu/qemu_driver.c         | 24 ++++++++++++
>   src/qemu_protocol-structs      |  8 ++++
>   src/remote/qemu_protocol.x     | 18 ++++++++-
>   src/remote/remote_driver.c     |  1 +
>   10 files changed, 143 insertions(+), 31 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h
> index 891617443f..8d3cc776e9 100644
> --- a/include/libvirt/libvirt-qemu.h
> +++ b/include/libvirt/libvirt-qemu.h
> @@ -53,6 +53,8 @@ typedef enum {
>   char *virDomainQemuAgentCommand(virDomainPtr domain, const char *cmd,
>                                   int timeout, unsigned int flags);
>   
> +int virDomainQemuAgentSetTimeout(virDomainPtr domain, int timeout);
> +
>   /**
>    * virConnectDomainQemuMonitorEventCallback:
>    * @conn: the connection pointer
> diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
> index 015b2cd01c..2f17bff844 100644
> --- a/src/driver-hypervisor.h
> +++ b/src/driver-hypervisor.h
> @@ -1372,6 +1372,10 @@ typedef int
>                               int *nparams,
>                               unsigned int flags);
>   
> +typedef int
> +(*virDrvDomainQemuAgentSetTimeout)(virDomainPtr domain,
> +                                   int timeout);
> +
>   typedef struct _virHypervisorDriver virHypervisorDriver;
>   typedef virHypervisorDriver *virHypervisorDriverPtr;
>   
> @@ -1632,4 +1636,5 @@ struct _virHypervisorDriver {
>       virDrvDomainCheckpointGetParent domainCheckpointGetParent;
>       virDrvDomainCheckpointDelete domainCheckpointDelete;
>       virDrvDomainGetGuestInfo domainGetGuestInfo;
> +    virDrvDomainQemuAgentSetTimeout domainQemuAgentSetTimeout;
>   };
> diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c
> index 1afb5fe529..73f119cb23 100644
> --- a/src/libvirt-qemu.c
> +++ b/src/libvirt-qemu.c
> @@ -216,6 +216,46 @@ virDomainQemuAgentCommand(virDomainPtr domain,
>       return NULL;
>   }
>   
> +/**
> + * virDomainQemuAgentSetTimeout:
> + * @domain: a domain object
> + * @timeout: timeout in seconds
> + *
> + * Set how long to wait for a response from qemu agent commands. By default,
> + * agent commands block forever waiting for a response.
> + *
> + * @timeout must be -2, -1, 0 or positive.
> + * VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK(-2): meaning to block forever waiting for
> + * a result.
> + * VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT(-1): use default timeout value.
> + * VIR_DOMAIN_QEMU_AGENT_COMMAND_NOWAIT(0): does not wait.
> + * positive value: wait for @timeout seconds
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +virDomainQemuAgentSetTimeout(virDomainPtr domain,
> +                             int timeout)

This definitely deserves some @flags argument. Even though we don't have 
any flags to set now, it has proven to save us in the future couple of 
times. Look at "extra flags; not used yet ...." occurring in docs for 
our public APIs.

> +{
> +    virConnectPtr conn;
> +
> +    virResetLastError();
> +
> +    virCheckDomainReturn(domain, -1);
> +    conn = domain->conn;
> +
> +    if (conn->driver->domainQemuAgentSetTimeout) {
> +        if (conn->driver->domainQemuAgentSetTimeout(domain, timeout) < 0)
> +            goto error;
> +        return 0;
> +    }
> +
> +    virReportUnsupportedError();
> +
> + error:
> +    virDispatchError(conn);
> +    return -1;
> +}
>   
>   /**
>    * virConnectDomainQemuMonitorEventRegister:
> diff --git a/src/libvirt_qemu.syms b/src/libvirt_qemu.syms
> index 3a297e3a2b..348caea72e 100644
> --- a/src/libvirt_qemu.syms
> +++ b/src/libvirt_qemu.syms
> @@ -30,3 +30,7 @@ LIBVIRT_QEMU_1.2.3 {
>           virConnectDomainQemuMonitorEventDeregister;
>           virConnectDomainQemuMonitorEventRegister;
>   } LIBVIRT_QEMU_0.10.0;
> +LIBVIRT_QEMU_5.8.0 {
> +    global:
> +        virDomainQemuAgentSetTimeout;
> +} LIBVIRT_QEMU_1.2.3;
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 34e1a85d64..86352aaec5 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -128,6 +128,7 @@ struct _qemuAgent {
>        * but fire up an event on qemu monitor instead.
>        * Take that as indication of successful completion */
>       qemuAgentEvent await_event;
> +    int timeout;
>   };
>   
>   static virClassPtr qemuAgentClass;
> @@ -696,6 +697,8 @@ qemuAgentOpen(virDomainObjPtr vm,
>       if (!(mon = virObjectLockableNew(qemuAgentClass)))
>           return NULL;
>   
> +    /* agent commands block by default, user can choose different behavior */
> +    mon->timeout = VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK;
>       mon->fd = -1;
>       if (virCondInit(&mon->notify) < 0) {
>           virReportSystemError(errno, "%s",
> @@ -851,6 +854,11 @@ static int qemuAgentSend(qemuAgentPtr mon,
>               return -1;
>           if (seconds == VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT)
>               seconds = QEMU_AGENT_WAIT_TIME;
> +
> +        /* if user specified a custom agent timeout that is lower than the
> +         * default timeout, use the shorter timeout instead */
> +        if ((mon->timeout > 0) && (mon->timeout < seconds))
> +            seconds = mon->timeout;
>           then = now + seconds * 1000ull;
>       }
>   
> @@ -1305,8 +1313,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
>       if (!cmd)
>           goto cleanup;
>   
> -    if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +    if (qemuAgentCommand(mon, cmd, &reply, true, mon->timeout) < 0)


This looks redundant - you're already passing @mon, qemuAgentCommand can 
grab ->timeout from it instead of having it as an argument.

>           goto cleanup;
>   

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list