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

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(a)redhat.com&gt;

> @@ -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.