Bug 1759566

+++ This bug was initially created as a clone of Bug #1705426 +++

It is possible to call some QEMU-GA commands using libvirt interface (e.g. virDomainInterfaceAddresses) but all the commands block until the call to QEMU-GA finishes and it is not possible to specify a timeout. This is problematic for example in situations when the guest is under load when the call can take some time to finish. For the duration the libvirt domain is locked and any other interaction with libvirt is impossible.

The libvirt interface should be extended to allow setting a timeout to the guest agent calls.

--- Additional comment from Daniel Berrangé on 2019-05-02 09:24:38 UTC ---

(In reply to Tomáš Golembiovský from comment #0)
> It is possible to call some QEMU-GA commands using libvirt interface (e.g.
> virDomainInterfaceAddresses) but all the commands block until the call to
> QEMU-GA finishes and it is not possible to specify a timeout. This is
> problematic for example in situations when the guest is under load when the
> call can take some time to finish. For the duration the libvirt domain is
> locked and any other interaction with libvirt is impossible.

This is a bug in its own right. In most cases the only other APIs that should be blocked are those which also need to use the guest agent. The majority of domain APIs should be allowed, as talking to the guest agent doesn't change libvirt's view of the guest domain's state in most cases. 

> The libvirt interface should be extended to allow setting a timeout to the
> guest agent calls.

--- Additional comment from Jonathon Jongsma on 2019-10-03 19:45:17 UTC ---

(In reply to Daniel Berrangé from comment #1)
> (In reply to Tomáš Golembiovský from comment #0)
> > It is possible to call some QEMU-GA commands using libvirt interface (e.g.
> > virDomainInterfaceAddresses) but all the commands block until the call to
> > QEMU-GA finishes and it is not possible to specify a timeout. This is
> > problematic for example in situations when the guest is under load when the
> > call can take some time to finish. For the duration the libvirt domain is
> > locked and any other interaction with libvirt is impossible.
> 
> This is a bug in its own right. In most cases the only other APIs that
> should be blocked are those which also need to use the guest agent. The
> majority of domain APIs should be allowed, as talking to the guest agent
> doesn't change libvirt's view of the guest domain's state in most cases. 

As far as I can tell (from observation as well as testing), virDomainInterfaceAddresses() does not actually block other commands on the domain, because it only queries information from the agent (i.e. it calls qemuDomainObjBeginAgentJob() rather than qemuDomainObjBeginJobWithAgent()). For example, I can do the following: 
- use virsh from two different terminals
- instrument guest so that the agent commands don't respond immediately (e.g. set a breakpoint on qmp_guest_network_get_interfaces())
- in first terminal: call 'domifaddr --source agent $domain'
  - virsh waits indefinitely for a reply which is blocked by the breakpoint set above
- in second terminal: call 'setmem $domain 3G'
  - command completes immediately and memory is updated

If you have different results, please let me know.

On the other hand, qemuDomainGetFSInfo() *does* block other commands while it is running. For example, if you repeat the above procedure for 'domfsinfo $domain', the 'setmem' command will block indefinitely until the agent command completes. This is because 'domfsinfo' starts a job for both the agent and the domain. We do so because virDomainGetFSInfo() attempts to match the filesystems returned by the agent command to disks in the domain's xml definition. I believe this behavior is correct because it prevents the domain definition from changing while we query the filesystem information.

> > The libvirt interface should be extended to allow setting a timeout to the
> > guest agent calls.

So I guess this is the important part of this bug, but it is also a bit tricky. I'll send a proposal patch upstream for discussion.

--- Additional comment from Daniel Berrangé on 2019-10-04 09:02:50 UTC ---

(In reply to Jonathon Jongsma from comment #2)
> On the other hand, qemuDomainGetFSInfo() *does* block other commands while
> it is running. For example, if you repeat the above procedure for 'domfsinfo
> $domain', the 'setmem' command will block indefinitely until the agent
> command completes. This is because 'domfsinfo' starts a job for both the
> agent and the domain. We do so because virDomainGetFSInfo() attempts to
> match the filesystems returned by the agent command to disks in the domain's
> xml definition. I believe this behavior is correct because it prevents the
> domain definition from changing while we query the filesystem information.

I don't think that level of locking is really necessary. Matching up guest disks to host XML is only ever a best effort thing. There's no strong guarantee we'll successfully match.

Given that I think it is not necessary to acquire a job on the domain while the agent is running. We should just do the matching with whatever domain XML exists after we get the data back from the guest. If someone has changed storage in the meantime, so be it, we'll simply not match those changed entries.

It is not *ever* acceptable for a guest agent command to block execution of any other part of the libvirt mgmt API, except for other guest agent commands, as this is a denial of service on the mgmt app.

> > > The libvirt interface should be extended to allow setting a timeout to the
> > > guest agent calls.
> 
> So I guess this is the important part of this bug, but it is also a bit
> tricky. I'll send a proposal patch upstream for discussion.

--- Additional comment from Jonathon Jongsma on 2019-10-04 14:34:05 UTC ---

(In reply to Daniel Berrangé from comment #3)

> I don't think that level of locking is really necessary. Matching up guest
> disks to host XML is only ever a best effort thing. There's no strong
> guarantee we'll successfully match.

OK, I can submit a patch making that change, but...

> It is not *ever* acceptable for a guest agent command to block execution of
> any other part of the libvirt mgmt API, except for other guest agent
> commands, as this is a denial of service on the mgmt app.

If this is really a hard-and-fast rule, that basically means that everywhere that we use qemuDomainObjBeginJobWithAgent() is a bug? At the moment, we have 7 such cases:
 - qemuDomainShutdownFlags()
 - qemuDomainReboot()
 - qemuDomainSetVcpusFlags()
 - qemuDomainPMSuspendForDuration()
 - qemuDomainSetTime()
 - qemuDomainGetFSInfo()
 - qemuDomainGetGuestInfo()

--- Additional comment from Daniel Berrangé on 2019-10-04 14:43:08 UTC ---

Yes, I'd consider them all bugs, because we have to assume that the agent can be malicious / hostile and thus protect against a DoS when using the agent.