[pve-devel] [PATCH storage] fix #1929: refactor requirements check for disks

[Dominik Csapak]

On 9/24/18 10:28 AM, Thomas Lamprecht wrote:
> On 9/21/18 4:32 PM, Dominik Csapak wrote:
>> and check correctly that the storage exists only when the user
>> want to create a storage
> 
> strange line break here
> 
>> this is useful if you want to create the same storage on
>> multiple nodes
>>
> 
> could we please split this the next time?
> 1. Refactor out common stuff
> 2. fix check for add-storage

yeah generally yes, but i did not want to touch those things twice
only for the sake of having two commits, since i would have had to
refactor 'wrong' and then fix the bug i just moved

> 
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   PVE/API2/Disks/Directory.pm |  9 +--------
>>   PVE/API2/Disks/LVM.pm       |  3 +--
>>   PVE/API2/Disks/LVMThin.pm   |  9 +--------
>>   PVE/API2/Disks/ZFS.pm       | 11 +----------
>>   PVE/Diskmanage.pm           | 23 +++++++++++++++++++++++
>>   5 files changed, 27 insertions(+), 28 deletions(-)
>>
>> diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
>> index 9d27762..0a3afdb 100644
>> --- a/PVE/API2/Disks/Directory.pm
>> +++ b/PVE/API2/Disks/Directory.pm
>> @@ -200,14 +200,7 @@ __PACKAGE__->register_method ({
>>   	my $node = $param->{node};
>>   	my $type = $param->{filesystem} // 'ext4';
>>   
>> -	$dev = PVE::Diskmanage::verify_blockdev_path($dev);
>> -	die "device $dev is already in use\n" if PVE::Diskmanage::disk_is_used($dev);
>> -
>> -	my $cfg = PVE::Storage::config();
>> -
>> -	if (my $scfg = PVE::Storage::storage_config($cfg, $name, 1)) {
>> -	    die "storage ID '$name' already defined\n";
>> -	}
>> +	$dev = PVE::Diskmanage::assert_disk_requirements($name, $dev, $param->{add_storage});
>>   
>>   	my $worker = sub {
>>   	    my $path = "/mnt/pve/$name";
>> diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm
>> index 19fba07..9c66705 100644
>> --- a/PVE/API2/Disks/LVM.pm
>> +++ b/PVE/API2/Disks/LVM.pm
>> @@ -148,8 +148,7 @@ __PACKAGE__->register_method ({
>>   	my $dev = $param->{device};
>>   	my $node = $param->{node};
>>   
>> -	$dev = PVE::Diskmanage::verify_blockdev_path($dev);
>> -	die "device $dev is already in use\n" if PVE::Diskmanage::disk_is_used($dev);
>> +	$dev = PVE::Diskmanage::assert_disk_requirements($name, $dev, $param->{add_storage});
>>   
>>   	my $worker = sub {
>>   	    PVE::Diskmanage::locked_disk_action(sub {
>> diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm
>> index 5b72c8c..46630e3 100644
>> --- a/PVE/API2/Disks/LVMThin.pm
>> +++ b/PVE/API2/Disks/LVMThin.pm
>> @@ -102,14 +102,7 @@ __PACKAGE__->register_method ({
>>   	my $dev = $param->{device};
>>   	my $node = $param->{node};
>>   
>> -	$dev = PVE::Diskmanage::verify_blockdev_path($dev);
>> -	die "device $dev is already in use\n" if PVE::Diskmanage::disk_is_used($dev);
>> -
>> -	my $cfg = PVE::Storage::config();
>> -
>> -	if (my $scfg = PVE::Storage::storage_config($cfg, $name, 1)) {
>> -	    die "storage ID '$name' already defined\n";
>> -	}
>> +	$dev = PVE::Diskmanage::assert_disk_requirements($name, $dev, $param->{add_storage});
>>   
>>   	my $worker = sub {
>>   	    PVE::Diskmanage::locked_disk_action(sub {
>> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
>> index 3c36ef9..b7329f6 100644
>> --- a/PVE/API2/Disks/ZFS.pm
>> +++ b/PVE/API2/Disks/ZFS.pm
>> @@ -339,16 +339,7 @@ __PACKAGE__->register_method ({
>>   	my $ashift = $param->{ashift} // 12;
>>   	my $compression = $param->{compression} // 'on';
>>   
>> -	foreach my $dev (@$devs) {
>> -	    $dev = PVE::Diskmanage::verify_blockdev_path($dev);
>> -	    die "device $dev is already in use\n" if PVE::Diskmanage::disk_is_used($dev);
>> -	}
>> -
>> -	my $cfg = PVE::Storage::config();
>> -
>> -	if (my $scfg = PVE::Storage::storage_config($cfg, $name, 1)) {
>> -	    die "storage ID '$name' already defined\n";
>> -	}
>> +	$devs = PVE::Diskmanage::assert_disk_requirements($name, $devs, $param->{add_storage});
>>   
>>   	my $numdisks = scalar(@$devs);
>>   	my $mindisks = {
>> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
>> index 1938fa8..7cc0d0d 100644
>> --- a/PVE/Diskmanage.pm
>> +++ b/PVE/Diskmanage.pm
>> @@ -602,4 +602,27 @@ sub locked_disk_action {
>>       return $res;
>>   }
>>   
>> +sub assert_disk_requirements {
> 
> strange naming? asserts normally do not transform and/or return data structures,
> they check if a predicate is true as in all other case the current view of things
> are false and the code should abort (die in the case of perl).

yeah i was not completely happy with the name myself and tried different
things, any suggestions ?

> 
> 
>> +    my ($name, $device, $addstorage) = @_;
>> +
>> +    if (ref($device) eq 'ARRAY') {
>> +	foreach my $dev (@$device) {
>> +	    $dev = verify_blockdev_path($dev);
> 
> same here, albeit this method already exists, when reading one wonders what
> this may return, verify sound like it would just return a boolean (i.e., the
> language equivalent to boolean ;) )

yeah this could be renamed, but it would span at least 2 repositories

would something like: 'real_blockdev_path' be better ?
since we are resolving softlinks and relative paths

> 
>> +	    die "device $dev is already in use\n" if disk_is_used($dev);
>> +	}
>> +    } elsif (!ref($device)) {
>> +	$device = verify_blockdev_path($device);
>> +	die "device $device is already in use\n" if disk_is_used($device);
>> +    }
> 
> also not sure if it would be nicer if a single disk get checked only, and
> the caller iterates over the checks...

i did not do this because of the addstorage check, but if that is
separate this makes sense ofc :)

> 
>> +
>> +    if ($addstorage) {
>> +	my $cfg = PVE::Storage::config();
>> +	if (my $scfg = PVE::Storage::storage_config($cfg, $name, 1)) {
>> +	    die "storage ID '$name' already defined\n";
>> +	}
>> +    }
> 
> is this a disk_requirement? It'd say no. Maybe it's better to add
> a storage_exists($storeid, [optional $scfg]) to PVE::Storage?
> 
> Then you just could do a:
> 
> PVE::Storage::storage_exists($name) if $param->{add_storage};
> 
> to each API call, it's more explicit there without duplicating
> code. (naming, signature are - as always - loosely thought out suggestions only)

sounds reasonable

> 
>> +
>> +    return $device;
>> +}
>> +
>>   1;
>>
>