[libvirt] [RFC] Add API to change qemu agent response timeout

[Michal Privoznik]

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