Re: [pve-devel] [PATCH storage v2 2/3] disks: die if storage name is already in use

[Aaron Lauterer <a.lauterer@proxmox.com>]

On 8/19/22 10:20, Fabian Grünbichler wrote:
> On August 18, 2022 5:22 pm, Aaron Lauterer wrote:
>>
>>>>    
>>>> diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm
>>>> index 6e4331a..a27afe2 100644
>>>> --- a/PVE/API2/Disks/LVM.pm
>>>> +++ b/PVE/API2/Disks/LVM.pm
>>>> @@ -152,6 +152,9 @@ __PACKAGE__->register_method ({
>>>>    	PVE::Diskmanage::assert_disk_unused($dev);
>>>>    	PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
>>>>    
>>>> +	die "volume group with name '${name}' already exists\n"
>>>> +	    if PVE::Storage::LVMPlugin::lvm_vgs()->{$name};
>>>
>>> probably better off inside the worker, since `vgs` might take a while
>>> (although we also use it in the index API call in this module..)
>>
>>   From a GUI perspective: putting it in a worker would result in the user to hit
>> okay and then will see the failed task right? Keeping it as is will result in an
>> error popping up when clicking ok/create and the user can edit the name instead
>> of starting all over again. Though, if `vgs` really needs a bit, that error
>> popping up could take a moment or two.
> 
> yes, putting it in the worker means no early-failure. but not putting it
> in the worker potentially means this API endpoint cannot be called on
> systems with busy LVM at all (30s timeout for API requests!). so
> early-failure checks should almost always only do "logical" things that
> are cheap (like reading a conf file and checking invariants), and
> nothing that can reasonably block for some time (like storage
> operations, starting guests, ..). I know we are not 100% consistent
> there (the worst offender is probably the storage content API endpoint
> ;)), but we should try to not introduce more problems of that fashion
> but rather work on removing the existing ones.

good point. I'll put the check inside the worker.