[libvirt] [PATCH v2] Add API to change qemu agent response timeout

[Daniel P. Berrangé]

On Wed, Nov 06, 2019 at 03:13:21PM -0600, 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 is 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>
> ---
> I've addressed most of the previous review comments from Peter, Daniel, and
> Michal but I'm unsure yet whether there's concensus on this feature. Here's a
> v2 for your consideration.
> 
> I admit that the proposed API is not perfect. It would probably be more elegant
> if each agent command instead had its own 'timeout' argument so that the caller
> could specify the timeout for each invocation. But that is not the case, and
> introducing duplicate API for each agent command (with an additional timeout
> argument) would clutter the public API. In addition, we still have the issue
> Michal mentioned elsewhere in the thread where some commands like shutdown may
> or may not use the agent, so a timeout argument could be confusing.
> 
> So: is this a viable approach, or should I rethink it?
> 
> Changes in v2:
> - Make this an official public API rather than putting it in libvirt-qemu.
> - Don't use the qemuDomainObjEnterAgent()/ExitAgent() API, which expects you
>   to acquire an agent job. Instead, introduce
>   qemuDomainObjSetAgentResponseTimeout() which simply locks the agent while
>   setting the variable.
> - rename the function slightly for better descriptiveness: 
>   virDomainQemuAgentSetTimeout() -> virDomainAgentSetResponseTimeout()
> - added 'flags' argument for future-proofing.
> 
>  include/libvirt/libvirt-domain.h |  9 ++++
>  include/libvirt/libvirt-qemu.h   |  8 +--
>  src/driver-hypervisor.h          |  6 +++
>  src/libvirt-domain.c             | 45 +++++++++++++++++
>  src/libvirt_public.syms          |  1 +
>  src/qemu/qemu_agent.c            | 84 ++++++++++++++++++++------------
>  src/qemu/qemu_agent.h            |  4 ++
>  src/qemu/qemu_domain.c           | 15 ++++++
>  src/qemu/qemu_domain.h           |  5 ++
>  src/qemu/qemu_driver.c           | 22 +++++++++
>  src/remote/remote_driver.c       |  1 +
>  src/remote/remote_protocol.x     | 18 ++++++-
>  src/remote_protocol-structs      |  9 ++++
>  13 files changed, 190 insertions(+), 37 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 22277b0a84..83f6c1b835 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -4916,4 +4916,13 @@ int virDomainGetGuestInfo(virDomainPtr domain,
>                            int *nparams,
>                            unsigned int flags);
>  
> +typedef enum {
> +    VIR_DOMAIN_AGENT_COMMAND_BLOCK = -2,
> +    VIR_DOMAIN_AGENT_COMMAND_DEFAULT = -1,
> +    VIR_DOMAIN_AGENT_COMMAND_NOWAIT = 0,
> +} virDomainAgentCommandTimeoutValues;

These constants should all have the word "TIMEOUT_" in their name
eg VIR_DOMAIN_AGENT_COMMAND_TIMEOUT_NOWAIT

> +int virDomainAgentSetResponseTimeout(virDomainPtr domain,
> +                                     int timeout,
> +                                     unsigned int flags);


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