[pve-devel] [PATCH] Add genegic multipath storage plugin and module for it to manipulate LUNs on Netapp storage

[Dmitry Petuhov]

14.07.2016 17:55, Dominik Csapak wrote:
> first, i believe your commit message got merged in the title?
Yes, I'm still learning to use git :-)


> second, i believe your intention is good (support for netapp and so on), but your approach is backwards:
>
> instead of a multipath plugin with multiple backends, why
> not make a plugin for each backend itself, which maps the
> disks to multipath devices.
>
> this way the options for the plugins don't get confusing in the future
> and you can still have the code they share factored out in a module
I was inspired by ZFSPlugin and its target-specific backends. BTW, it should easy to convert them to be used by multipath plugin.

I don't think there would be much confusion: SANs mostly have pretty same set of options, sometimes little differently named, but with same meaning. Like my `array' property is used as netpp's `aggregate', and can also be used as `pool' for zfs-backed iscsi, or as `vdisk' in HP MSA. These differences can be masked for user by UI: different captions for different backends, while same set of properties in storage.cfg.


> third, sadly we do not have a netapp here, so i don't see how we could test this. (and if we accept this, we would have to support it),
> comments to this from anyone?
Maybe vendors could lease by request some hardware samples to developers of such cool hypervisor software?


>> +        $list->{$name}->{'onlnie'} = ($lun->{'state'} eq 'online')?'true':'false';
> s/onlnie/online/
D'oh! That's why UI shows storage status as offline!


>> +  if (ref $ref eq 'ARRAY'){
>> +  return 1;
>> +  }
>> +  return 0;
> nothing in this method after this gets executed in any way, so why have it? also the code is not very nice, why would
> you try to convert to an array and parse the error message?
>
> i dont think a check like this is necessary, but correct me if i'm wrong
Yes, looks so. I was just copies Alexanre's code here. I'll rewrite it.


>> +        netapp_create_volume($scfg, $vol, int($size*1.05));
> why $size*1.05 ? is there a reason for this?
> it would be good to comment such things
That's how many is being reserved if you create LUN via this SAN's GUI. Actually, should not be needed because we enable autogrow for volume just after creation.


> the code
>
> my $vol = $name;
> $vol =~ s/-/_/g;
> $vol .= '_vol';
>
> is in nearly every case statement, i think it would be better to do this
> once in an if/else statement at the beginning of the  method,
> so if the code has to be changed, it only has to be changed once
>
Looks reasonable.


>> +sub renew_media {
>> +    my ($scfg) = @_;
>> +    run_command(['/sbin/rescan-scsi-bus', '-r', '-i', '-a']);
>> +#    switch($scfg->{media}) {
>> +#    case 'fc'    { system('/bin/bash', '-c',
>> +#        'for host in `ls /sys/class/fc_host`; do '.
>> +#        'echo 1 > /sys/class/fc_host/$host/issue_lip & done'); }
>> +#    case 'scsi'  { system('/bin/bash', '-c',
>> +#        'for host in `ls /sys/class/scsi_host`; do '.
>> +#        'echo - - -  > /sys/class/scsi_host/$host/scan & done'); }
>> +#    case 'iscsi' { system('/usr/bin/iscsiadm', '-m', 'session', '--rescan'); }
>> +#    }
>> +}
>> +
> why have the commented code here? should it be only temporarily disabled?
> if yes, please add a comment, otherwise why leave it here
Just not to lose it. Maybe need to move it to some separate helper script and use instead of old and ugly `rescan-scsi-bus'.

 
>> +    #get paths to delete them later
>> +#    my @paths;
>> +#    run_command(['/sbin/multipath', '-l', "0x$wwn"], outfunc => sub {
>> +#    my ($line) = @_;
>> +#    return unless $line =~ m/\s+\S+\s+(\d+:\d+:\d+:\d+)\s+(sd[a-z]+)\s+/;
>> +#    push @paths, $2;
>> +#    });
> same as above
This part and later ones are not needed.