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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On August 18, 2022 5:22 pm, Aaron Lauterer wrote:
> 
> 
> On 8/17/22 13:42, Fabian Grünbichler wrote:
>> On July 15, 2022 1:58 pm, Aaron Lauterer wrote:
> [...]
>>> -	my $worker = sub {
>>> -	    my $path = "/mnt/pve/$name";
>>> -	    my $mountunitname = PVE::Systemd::escape_unit($path, 1) . ".mount";
>>> -	    my $mountunitpath = "/etc/systemd/system/$mountunitname";
>>> +	my $mounted = PVE::Diskmanage::mounted_paths();
>>> +	die "a mountpoint for '${name}' already exists: ${path} ($1)\n" if $mounted->{$path};
>> 
>> nit: the mountpoint existing is not the issue (something already being
>> mounted there is!). also, where does the $1 come from?
> 
> good point and thanks for catching the $1
>> 
>>> +	die "a systemd mount unit already exists: ${mountunitpath}\n" if -e $mountunitpath;
>> 
>> could check if it's identical to the one we'd generate (in the spirit of
>> patch #3 ;))
> 
> I looked into it, depending on how hard we want to match the mount unit, this 
> could be a bit hard. It contains the /dev/disk/by-uuid/... path which will not 
> be the same as it changes with each FS creation (IIUC).
> 

makes sense, and no, we definitely don't want any fancy heuristics for 
"close enough to ignore it already exists" ;) so this can stay as it is.

>> 
>>>   
>>> +	my $worker = sub {
>>>   	    PVE::Diskmanage::locked_disk_action(sub {
>>>   		PVE::Diskmanage::assert_disk_unused($dev);
>>>   
>>> 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.

> 
> [...]
> 
>>>   sub preparetree {
>>> @@ -336,6 +340,7 @@ __PACKAGE__->register_method ({
>>>   	my $user = $rpcenv->get_user();
>>>   
>>>   	my $name = $param->{name};
>>> +	my $node = $param->{node};
>> 
>> nit: please also change the usage further below in this API handler if
>> you do this
> 
> what exactly do you mean? defining local variables for $param->{..} ones?

the usage of $param->{node} below in this handler should (also) switch 
to $node - we don't want two places with the same information, that can 
cause subtle breakage in the future when only one of them gets 
modified/..

> 
>> 
>>>   	my $devs = [PVE::Tools::split_list($param->{devices})];
>>>   	my $raidlevel = $param->{raidlevel};
>>>   	my $compression = $param->{compression} // 'on';
>>> @@ -346,6 +351,10 @@ __PACKAGE__->register_method ({
>>>   	}
>>>   	PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
>>>   
>>> +	my $pools = get_pool_data();
>> 
>> and also here
>> 
>>> +	die "pool '${name}' already exists on node '$node'\n"
>>> +	    if grep { $_->{name} eq $name } @{$pools};
>>> +
>>>   	my $numdisks = scalar(@$devs);
>>>   	my $mindisks = {
>>>   	    single => 1,
>>> -- 
>>> 2.30.2