[pve-devel] [PATCH pve-storage 0/5] v5 fix regression and indentation

[datanom.net]

Hi Fabian,

Most of your input I will study this weekend but a can give a quick 
answers to a few comments which follows below.

On 2017-06-16 14:21, Fabian Grünbichler wrote:
>>         next unless $zvol->{name} =~ /^(base|vm)-(\d+)-disk-\d+$/;
>>         $vmid = $2;
>>         $parent = undef;
>>         foreach my $snap (@$snapshots) {
>>             next unless $snap->{name} eq "__base__$vmid";
> 
> this seems a very strange way to handle this.. is it not possible to 
> get
> the origin of a dataset/zvol using the API? creating an artificial 
> extra
> snapshot per VM for this seems very workaround-ish

Unfortunately not. This was the best way I could find to remedy this.

>>             $parent = $snap->{filesystem} =~ /^$scfg->{pool}\/(.+)$/ ? 
>> $1 : undef;
>>         }
>>         $list->{$scfg->{pool}}->{$zvol->{name}} = {
>>             name => $zvol->{name},
>>             size => $zvol->{volsize},
>>             parent => $parent,
>>             vmid => $vmid,
>>             format => 'raw',
>>         };
>>         if ($zvol->{name} =~ /^base-(.*)/) {
>>             $hide->{"vm-$1"} = 1;
>>         }
> 
> shouldn't you hide those vm-XXX-foo zvols for which a base-XXX-foo 
> clone
> exists? instead of hiding the clones?
I am hiding the vm-XXX-foo zvols? ( $hide->{"vm-$1"} )
and
>>     delete @{$list->{$scfg->{pool}}}{keys %$hide};
> 

> again, no way to limit by target(-name) on the server side or get one
> directly?

Unfortunately not.

> 
> both branches do the same thing..
> 
For now yes, but the coming API for 11.x is wastly expanded so this is 
just for preparation.
The same replies to your comment for the same in other methods.

>>     foreach my $tgroup (@$targetgroups) {
>>         if ($tgroup->{iscsi_target} == $target &&
>>             $tgroup->{iscsi_target_portalgroup} == 
>> $scfg->{portal_group} &&
>>             $tgroup->{iscsi_target_initiatorgroup} == 
>> $scfg->{initiator_group}) {
> 
> is there now way to limit by any of this on the server side?
> 

No, unfortunately not.

>>     foreach my $ext (@$extents) {
>>         if ($ext->{iscsi_target_extent_path} =~ 
>> /$scfg->{pool}\/$volname$/) {
> 
> no way to limit for this on the server side?
> 

Unfortunately not.

>>     foreach my $t2ext (@$t2extents) {
>>         if ($t2ext->{iscsi_target} == $target && 
>> $t2ext->{iscsi_extent} == $extent) {
> 
> server side limits?
> 

Unfortunately not.

> 
> this check for the max_luns limit does not account for volumes with
> custom names (which are allowed in PVE, and also here in alloc_image).
> 

Custom names? I though the disk naming scheme is vm-{id}-disk-{#}

>> 
>>         foreach my $t2extent (@$t2extents) {
>>             next unless $t2extent->{iscsi_target} == $target &&
>>                         $t2extent->{iscsi_lunid} + 1 > $max_luns &&
>>                         $t2extent->{iscsi_lunid} < $max_luns + 
>> $active_snaps;
> 
> given that $max_luns is 20, and $active_snaps is 4, what is this
> supposed to achieve? this seems to always "next", so
> 

No, you only get $max_luns <= $lun < $max_luns + $active_snaps

>>             my $eid = freenas_get_extent($scfg, $volname);
>>             if ($eid) {
>>                 $response = freenas_request($scfg, 'GET', 
>> "services/iscsi/extent/$eid");
>>                 die HTTP::Status::status_message($response) if 
>> $response =~ /^\d+$/;
>>                 my $extent = decode_json($response);
>>                 # Request to get lunid for an existing lun
>>                 last if $t2extent->{iscsi_extent} eq $eid;
>>             }
>>             $id++;
>>         }
>>         die "Max snapshots (4) is reached" unless ($id - $max_luns) < 
>> $active_snaps;
> 
> $id is still $max_luns here, so this can never trigger. also, hard 
> coded
> value for active_snaps.
> 

See comment above.

>>         $lunid = $id;
> 
> and this just sets $lunid to $max_luns?
> 

See comment above.

>>     } elsif ($volname =~ /^(vm|base)-\d+-disk-\d+\@vzdump$/) {
>>         # Required to be able to exposed read-only LUNs for snapshot 
>> backup CT
> 
> why limit this to @vzdump? for example,  your plugin could support full
> cloning from snapshots as well, couldn't it?
> 

This is to handle the special case for snapshot backup of LXC. These 
snapshots are always named vzdump.

>>         $lunid = $max_luns + $active_snaps;
>>     }
> 
> what about custom volume names? vm-VMID-foo is allowed!
> 

See comment befoere.

>> 
>>     # abort rollback if snapshot is not the latest
>>     my $response = freenas_request($scfg, 'GET', "storage/snapshot");
> 
> why no limit here, but in freenas_list_zvol there is one? ;) also,
> filtering by $volname on the server side would be great if possible.
> 

This is how the API is ;-( and no server side filtering what so ever.

> 
> like previously said, it would probably be better to store this in
> /etc/pve/priv and not expose it over the API.
> 
How is that suppose to happen? You would require to change the gui code 
for configuring storages too.

>>         blocksize => { optional => 1 },
> 
> not used anywhere?

Next version of the API.

>> sub parse_volname {
> 
> it would be great if some of the REs in the helper methods could be
> replaced with calls to parse_volname. having such things encoded in one
> place makes them easier to check and change.
> 

You lost be here ;-)

>>         if (@$children) {
>>             $used = $children->[0]{used};
>>             $total = $children->[0]{avail};
> 
> this might warrant an explanatory comment - is this a workaround for
> some API quirk?

I will provide inline comment in the code. Reason is FreeNAS/API.
A pool is created in a dataset under RPOOL which means:
If pool is empty (no zvols) size is read from the dataset
If pool contains children (zvols > 0) size is read as the some of 
children.

>> 
>>             if ($vollist) {
>>                 my $found = grep { $_ eq $info->{volid} } @$vollist;
>>                 next if !$found;
>>             } else {
>>                 next if defined ($vmid) && ($owner ne $vmid);
>>             }
> 
> I wonder if those two are really exclusive?
> 

Copy/pasted from the zfspool plugin

>> 
>>     my $data = {
>>         force => bless( do{\(my $o = 0)}, 'JSON::XS::Boolean' ),
> 
> you mean JSON::false ?
> 

Yes. I no strange but force => does not translate to force => false ;-)

>>             $active_luns->{$lun} = "0:0:0:$lun";
> 
> where does this hard-coded 0:0:0: come from? wouldn't it be better to
> call get_active_luns again, or to let freenas_create_lun return some of
> the info it has anyway?

Just placement holders. The important part is $active_luns->{$lun} is 
not 0|null|undef

> 
> nope! this might be fine for debugging, but you need to use udevadm
> to trigger and settle if you want to wait for device nodes to appear.

How do I interopt with udevadm from a Perl?

-- 
Hilsen/Regards
Michael Rasmussen

Get my public GnuPG keys:
michael <at> rasmussen <dot> cc
http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xD3C9A00E
mir <at> datanom <dot> net
http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xE501F51C
mir <at> miras <dot> org
http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xE3E80917
--------------------------------------------------------------

----

This mail was virus scanned and spam checked before delivery.
This mail is also DKIM signed. See header dkim-signature.