[pve-devel] [PATCH storage] fix #1929: refactor requirements check for disks
Thomas Lamprecht
t.lamprecht at proxmox.com
Tue Sep 25 09:58:07 CEST 2018
On 9/25/18 9:27 AM, Dominik Csapak wrote:> 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
no, you misunderstand refactoring! It should clean up/move the code
around, ideally without changing something systematically.
This is not wrong, on the contrary, it's good value and changes nothing
so net effect is positive...
Then the actual bug-fix change (or feature addition) can happen much
cleaner and easier to review. It's small, one sees what actually
happens.
I don't see the merit of the "no touching things twice" argument? Yeah,
moving code up in one and then down in the next patch would be not quite
ideal. But refactoring and fixing are _two separate_ things and it makes
actually sense to split them and not intertwine them in one spaghetti
diff...
>
>>
>>> 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 ?
>
hmmm, yeah it's hard to come up with a good name, because, IMO, it's hard to
get what this method actually does or should do... And that may be a sign
of over abstraction. a per api call:
my $device = PVE::Diskmanage::abs_blockdev_path($param->{device}); # (renamed)
PVE::Diskmanage::check_unused($device); # (new) does what it says it does
PVE::Storage::storage_exists($name) if $param->{add_storage}; # new (as mentioned earlier)
(zfs wraps additionally with foreach).
with this you do not add unnecessarily hiding layers with strange names
one sees what happens but no real code duplication.
>>
>>
>>> + 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>
Another day another break/depends ;-)
yeah that's why we normally look to get multi-repo spanning interfaces,
discussed first, I also remember about my question about the split up
disk/blockdev api and if this is the right place - which got unanswered:
https://pve.proxmox.com/pipermail/pve-devel/2018-August/033252.html
> would something like: 'real_blockdev_path' be better ?
> since we are resolving softlinks and relative paths
maybe abs_blockdev_path as links and relative paths are also real ;-)
>
>>
>>> + 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;
>>>
More information about the pve-devel
mailing list