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@xxxxxxxxxx> > --- > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
