[libvirt] [PATCH] qemu: Allow graceful domain destroy

[Michal Privoznik]

On 22.08.2011 15:30, Eric Blake wrote:
> On 08/22/2011 07:22 AM, Eric Blake wrote:
>> On 08/22/2011 04:49 AM, Michal Privoznik wrote:
>>>
>>> I don't think that's problem. High priority calls must be guaranteed to
>>> end in reasonably short time. And although we talk to monitor here, we
>>> are guaranteed to end. Therefore no need to change my previous patch.
>>
>> No, we are not guaranteed to end in a reasonably short time. Monitor
>> calls are, by nature, indefinite time commands, because they require
>> response from an external process.
>>
>> Rather, we should add virDomainShutdownFlags() (which would be low
>> priority, to match virDomainShutdown() being low priority), and make
>> this a flag an option to virDomainShutdownFlags(). That is, you get a
>> choice between shutdown via ACPI signal, or shutdown via monitor
>> command, but both choices involve interaction with the monitor, and are
>> therefore inherently liable to blocking (although shutdown via the
>> monitor is much more likely to succeed in a short amount of time if the
>> monitor is available, because it does not depend on the guest's reaction
>> to ACPI).
>>
>>>
>>> 'Accessing monitor' is a bit unfortunate formulation, because 99% calls
>>> which do access monitor can block indefinitely. And this is the real
>>> problem. Stuck API. It doesn't really matter if it is because of
>>> monitor, NFS, etc. I just wanted to provide guide for developers if they
>>> are in doubt whether to mark API as high or low priority.
>>
>> The point is that virDomainDestroy() can't make a monitor command, or it
>> will get stuck. I don't think it makes sense to give
>> virDomainDestroyFlags() a flag that it can only honor in cases where the
>> monitor is not stuck, but which gets skipped otherwise. I think that if
>> we support a flag, then the use of the monitor has to be unconditional
>> with that flag, which in turn implies that we need
>> virDomainShutdownFlags().
> 
> I wrote the above before even glancing at your patch.  But now, looking
> at the patch, it looks like you tried to take this into consideration -
> you are trying to add condition variables to guarantee that the monitor
> command will be attempted if possible, but that the command will give up
> on an unresponsive monitor and resort to the more drastic process kill,
> so that the command will always succeed even in the face of a stuck
> monitor.
> 
> Maybe your approach has merit after all.  But in that case, I think we
> need two ways to expose the monitor command:
> 
> virDomainDestroyFlags(, VIR_DOMAIN_DESTROY_FLUSH_CACHE) - best effort
> try to flush cache, but may still lose data because the destroy is
> guaranteed to happen after x seconds even if monitor could not be obtained
> 
> virDomainShutdownFlags(, VIR_DOMAIN_SHUTDOWN_FLUSH_CACHE) - guarantee
> that the flush cache happens, or that the command fails due to a timeout
> in obtaining the monitor
> 

Agree. I've sent v2 for destroy (DV suggested some corrections). And for
shutdown - I will sent follow up patch.