[PATCH 02/11] hyperv: implement domainUndefine and domainUndefineFlags

[Daniel P. Berrangé]

On Thu, Jan 07, 2021 at 06:55:45PM -0500, Matt Coleman wrote:
> > On Nov 26, 2020, at 9:26 AM, Daniel P. Berrangé <berrange at redhat.com> wrote:
> > 
> > On Tue, Nov 24, 2020 at 02:48:31PM -0500, Matt Coleman wrote:
> >> +    /* try to shut down the VM if it's not disabled, just to be safe */
> >> +    if (computerSystem->data->EnabledState != MSVM_COMPUTERSYSTEM_ENABLEDSTATE_DISABLED &&
> >> +        hypervDomainShutdown(domain) < 0) {
> >> +        goto cleanup;
> >> +    }
> > 
> > Many, but not all, drivers in libvirt allow undefining the cofig for a running
> > VM. ie we can delete the config on disk, without affecting the running VM.
> > This results in a so called "transient" VM - basically a VM which only exists
> > as long as it is running - virDomainCreateXML creates such a beast, while
> > virDomainDefineXML+virDomainCreate  creates a "persistent" VM and starts it.
> > 
> > If hyperv doesn't allow that, then shutting down the VM is likely to be
> > surprising.
> > 
> > I'd suggest we report an error indicating undefine is not permitted for a
> > running VM in such a case.
> 
> Hyper-V does not allow a VM's definition to be deleted if it's active, 
> paused, or in state transition.
> 
> If I remove the check entirely, it displays an error including 
> Hyper-V's method name, the error code from Hyper-V, and a string that's 
> generated by hypervReturnCodeToString() from hyperv_wmi.c.
> 
> For example, `virsh undefine runningvm` would throw the error:
> 
> error: Failed to undefine domain runningvm
> error: internal error: Invocation of DestroySystem returned an error: Invalid state for this operation (32775)
> 
> That simplifies the code significantly and provides details (the method 
> name and error code) that users could look up in Microsoft's 
> documentation. However, I don't think it's very clearly phrased. 
> "Domain must not be active or in state transition" with the error code 
> set to VIR_ERR_OPERATION_INVALID produces the following error output:
> 
> error: Failed to undefine domain testvm
> error: Requested operation is not valid: Domain must not be active or in state transition
> 
> It's a difference of 18 lines (51 vs 33). The shorter version 
> eliminates the result and computerSystem variables, does not use goto, 
> and only sends a single WMI request to Hyper-V since it does not have 
> to query the VM state before attempting to undefine it.

Querying state ahead of time leaves a race condition, so either just
let hyperv fail, or catch the error when it fails and report a different
error mesage.

> Would you prefer to have simpler code or a custom error message?

I don't mind on that front.


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