Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file

[Cole Robinson <crobinso redhat com>]

On 02/24/2015 10:23 AM, Peter Krempa wrote:
> On Tue, Feb 24, 2015 at 15:11:28 +0000, Daniel Berrange wrote:
>> On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote:
>>> On Tue, Feb 24, 2015 at 10:12:20 +0000, Daniel Berrange wrote:
>>>> The undefine operation should always be allowed to succeed
>>>> regardless of whether any NVRAM file exists. ie we should
>>>> not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM
>>>> flag. It is valid for the app to decide it wants the NVRAM
>>>> file left on disk, in the same way that disk images are left
>>>> on disk at undefine.
>>>> ---
>>>>  src/qemu/qemu_driver.c | 20 +++++++-------------
>>>>  1 file changed, 7 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index bec05d4..302bf48 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom,
>>>>  
>>>>      if (!virDomainObjIsActive(vm) &&
>>>>          vm->def->os.loader && vm->def->os.loader->nvram &&
>>>> -        virFileExists(vm->def->os.loader->nvram)) {
>>>> -        if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
>>>> -            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>>>> -                           _("cannot delete inactive domain with nvram"));
>>>> -            goto cleanup;
>>>> -        }
>>>> -
>>>> -        if (unlink(vm->def->os.loader->nvram) < 0) {
>>>> -            virReportSystemError(errno,
>>>> -                                 _("failed to remove nvram: %s"),
>>>> -                                 vm->def->os.loader->nvram);
>>>> -            goto cleanup;
>>>> -        }
>>>> +        virFileExists(vm->def->os.loader->nvram) &&
>>>> +        (flags & VIR_DOMAIN_UNDEFINE_NVRAM) &&
>>>> +        (unlink(vm->def->os.loader->nvram) < 0)) {
>>>> +        virReportSystemError(errno,
>>>> +                             _("failed to remove nvram: %s"),
>>>> +                             vm->def->os.loader->nvram);
>>>> +        goto cleanup;
>>>>      }
>>>>  
>>>>      if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
>>>
>>>
>>> We have prior art in denying to undefine a domain that has information
>>> stored in libvirt-internal locations such as the managed save image and
>>> snapshot metadata.
>>>
>>> While it makes sense to allow removing the VM without deleting the NVRAM
>>> file when the user specified an external path, we should avoid doing so
>>> if the NVRAM is in the libvirt managed path. (Same with externaly
>>> managed snapshots or save images).
>>
>> I'm not a real fan of refusal in the managed save case either. The issue
>> is that we're forcing a policy onto the application and not giving it any
>> chance to define a different policy. ie currently apps have a choice of
>> getting an error, or always deleting the nvram, not giving them any chance
>> to delete guest without deleting nvram which is a valid choice they should
>> be permitted to have. By removing this check we allow apps to make their
>> own policy decision with their more complete view of the world. I don't
>> think the quesiton of managed vs unmanaged storage should even come into
>> it either, because that presupposes the application will only ever use
>> managed storage. OpenStack does not currently use libvirt storage pools
>> at all but does wish to use NVRAM. We should not deny the option to undefine
>> the guest in this case.
> 
> In that case we probably should have negated the logic here and delete
> all the stuff by default and give the user option to leave the data
> behind.
> 
> The original motivation is apparently that we should not allow anything
> that would represent state of the deleted VM to be transferred
> accidentaly to a new VM with same name. For the save image or snapshots
> the risk of persisting any data is low as a save image would not
> function without it's disk and still be somewhat secure as it would
> contain the whole memory image including security. For the NVRAM though
> it might uncover data stored there or even make the VM unbootable.
> 
> I agree that the current state is not ideal as we basically force the
> user to specify all the necessary flags. I think we can safely avoid
> displaying the message in cases when it's not stored in the
> libvirt-internal path but for the internal path I'm not convinced that
> it would be a great idea to change the default.
> 
> We can perhaps add a flag that woult either enable all the "UNDEFINE*"
> flags or perhaps invert the logic of them so the user could leave them
> behind.
> 

FWIW a flag for UNDEFINE_REMOVE_ALL_STATE or something would definitely be
useful for virt-manager. The managed save, snapshot, and nvram undefine errors
all hit us and required patches, it would be useful to have one flag that
future proofs us.

Thanks,
Cole