[pve-devel] [PATCH v7 pve-storage 04/10] Basic storage operations.

Fabian Gr├╝nbichler f.gruenbichler at proxmox.com
Mon Aug 7 14:32:55 CEST 2017


On Tue, Jun 20, 2017 at 10:39:56PM +0200, mir at datanom.net wrote:
> From: Michael Rasmussen <mir at datanom.net>
> 
> Signed-off-by: Michael Rasmussen <mir at datanom.net>
> ---
>  PVE/Storage/FreeNASPlugin.pm | 597 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 589 insertions(+), 8 deletions(-)
> 
> diff --git a/PVE/Storage/FreeNASPlugin.pm b/PVE/Storage/FreeNASPlugin.pm
> index e930cea..3a18a31 100644
> --- a/PVE/Storage/FreeNASPlugin.pm
> +++ b/PVE/Storage/FreeNASPlugin.pm
> @@ -17,11 +17,15 @@ use base qw(PVE::Storage::Plugin);
>  
>  my $api = '/api/v1.0';
>  my $api_timeout = 20; # seconds
> +my $max_luns = 20; # maximum supported luns per target group in freenas is 25
> +                   # but reserve 5 for temporary LUNs (snapshots with RAM and
> +                   # snapshot backup)
>  my $rows_per_request = 50; # limit for get requests
>                             # be aware. Setting limit very low (default setting
>                             # in FreeNAS API is 20) can cause race conditions
>                             # on the FreeNAS host (seems like an unstable
>                             # pagination algorithm implemented in FreeNAS)
> +my $active_snaps = 4;                      

whitespace

>  my $version;
>  my $target_prefix = 'iqn.2005-10.org.freenas.ctl'; # automatically prepended
>                                                     # in FreeNAS
> @@ -245,6 +249,389 @@ my $freenas_list_zvol = sub {
>      return $list;
>  };
>  

whitespace

> +my $freenas_no_more_extents = sub {

isn't this misnamed? you are checking whether AN extent already exists
for a specific target, not whether there are "(no) more extents"?

> +    my ($scfg, $target) = @_;
> +    

whitespace

> +    my $extents = $freenas_request->($scfg, 'GET', "services/iscsi/targettoextent/");
> +    foreach my $extent (@$extents) {
> +        return 0 if $extent->{iscsi_target} == $target;
> +    }
> +    

whitespace

> +    return 1;
> +};
> +
> +my $freenas_get_target = sub {
> +    my ($scfg, $vmid) = @_;
> +    my $target = undef;

not needed, see below

> +    

whitespace

> +    my $targets = $freenas_request->($scfg, 'GET', "services/iscsi/target/");
> +    foreach my $t (@$targets) {
> +        if ($t->{iscsi_target_name} eq "vm-$vmid") {
> +            $target = $t->{id};
> +            last;

return $t->{id} if $t->{iscsi_target_name} eq "vm-$vmid";

> +        }
> +    }
> +    

whitespace

> +    return $target;

return undef (although I think this is technically not needed)

> +};
> +
> +my $freenas_create_target = sub {
> +    my ($scfg, $vmid) = @_;
> +    my ($data, $target);
> +    

whitespace

> +    $freenas_get_version->($scfg);
> +
> +    if ($version < 110000) {
> +        $data = {
> +            iscsi_target_alias => "vm-$vmid",
> +            iscsi_target_name => "vm-$vmid",
> +        };
> +    } elsif ($version < 110100) {
> +        $data = {
> +            iscsi_target_alias => "vm-$vmid",
> +            iscsi_target_name => "vm-$vmid",
> +        };
> +    } else {
> +        die "FreeNAS-$version: Unsupported!\n";
> +    }

I don't get why you have this if here, but not in get_target..

> +
> +    eval {
> +        $target = $freenas_request->(
> +            $scfg, 'POST', "services/iscsi/target/", encode_json($data));
> +    };
> +    if ($@) {
> +        if ($@ =~ /^(\d+)\s*$/) {
> +            # Fetch existing target if code is 409 (conflict)
> +            die HTTP::Status::status_message($1) unless $1 == 409;
> +            return $freenas_get_target->($scfg, $vmid);
> +        }

IMHO this is ugly:
- unless
- make freenas_request have a list of 'allowed' error status codes, and
  use the array variant here to get code and data.

> +        die "Creating target for 'vm-$vmid' failed: $@\n";
> +    }
> +    

whitespace

> +    return $target->{id};
> +};
> +
> +my $freenas_delete_target = sub {
> +    my ($scfg, $target) = @_;
> +
> +    $freenas_request->($scfg, 'DELETE', "services/iscsi/target/$target/");
> +};
> +
> +my $freenas_get_target_name = sub {

this does not make a freenas_request, so IMHO it should be named
differently. also, it seems this is included in patches #3 and #4?

> +    my ($scfg, $volname) = @_;
> +    my $name = undef;
> +    

whitespace

> +    if ($volname =~ /^(vm|base)-(\d+)-/) {
> +        $name = "vm-$2";
> +        return "$target_prefix\:$name";
> +    }
> +
> +    return undef;
> +};
> +
> +my $freenas_get_target_group = sub {
> +    my ($scfg, $target) = @_;
> +    my $targetgroup = undef;
> +    

whitespace

> +    my $targetgroups = $freenas_request->($scfg, 'GET', "services/iscsi/targetgroup/");
> +
> +    foreach my $tgroup (@$targetgroups) {
> +        if ($tgroup->{iscsi_target} == $target && 
> +            $tgroup->{iscsi_target_portalgroup} == $scfg->{portal_group} &&
> +            $tgroup->{iscsi_target_initiatorgroup} == $scfg->{initiator_group}) {
> +            $targetgroup = $tgroup->{id};
> +            last;

see above, drop the last, return from within the foreach, drop the
useless $targetgroup

> +        }
> +    }
> +    

whitespace

> +    return $targetgroup;
> +};
> +
> +my $freenas_create_target_group = sub {
> +    my ($scfg, $target) = @_;
> +    my $data;
> +    

whitespace

> +    $freenas_get_version->($scfg);
> +
> +    # Trying to create a target group which already exists will cause and internal
> +    # server error so if creating an existing target group should be allowed (return
> +    # existing target group number we must search prior to create
> +    my $tg = $freenas_get_target_group->($scfg, $target);
> +    return $tg if $tg;

why do you have this here, but not for create_target, which has the same
semantics?

also, you overload $tg with two meanings:
- get_target_group returns the ID
- POST to services/iscsi/targetgroup returns the whole JSON response

I wonder whether explicitly marking the subs and variables only
containing IDs as such would prevent such confusions?

> +    

whitespace

> +    if ($version < 110000) {
> +        $data = {
> +            iscsi_target => $target,
> +            iscsi_target_authgroup => $scfg->{auth_group} ? $scfg->{auth_group} : undef,
> +            iscsi_target_portalgroup => $scfg->{portal_group},
> +            iscsi_target_initiatorgroup => $scfg->{initiator_group},
> +            iscsi_target_authtype => $scfg->{auth_type} ? $scfg->{auth_type} : 'None',
> +            iscsi_target_initialdigest => "Auto",
> +        };
> +    } elsif ($version < 110100) {
> +        $data = {
> +            iscsi_target => $target,
> +            iscsi_target_authgroup => $scfg->{auth_group} ? $scfg->{auth_group} : undef,
> +            iscsi_target_portalgroup => $scfg->{portal_group},
> +            iscsi_target_initiatorgroup => $scfg->{initiator_group},
> +            iscsi_target_authtype => $scfg->{auth_type} ? $scfg->{auth_type} : 'None',
> +            iscsi_target_initialdigest => "Auto",
> +        };
> +    } else {
> +        die "FreeNAS-$version: Unsupported!\n";
> +    }

same comment like for the if in create_target

> +
> +    eval {
> +        $tg = $freenas_request->(
> +            $scfg, 'POST', "services/iscsi/targetgroup/", encode_json($data));
> +    };
> +    if ($@) {
> +        if ($@ =~ /^(\d+)\s*$/) {
> +            # Fetch existing target group if code is 409 (conflict)
> +            die HTTP::Status::status_message($1)."\n" unless $1 == 409;
> +            return $freenas_get_target_group->($scfg, $target);
> +        }

and again, this is really convoluted - see create_target

> +        die "Creating target group for target '$target' failed: $@\n";
> +    }
> +    

whitespace

> +    return $tg->{id};
> +};
> +
> +my $freenas_delete_target_group = sub {
> +    my ($scfg, $tg) = @_;
> +
> +    $freenas_request->($scfg, 'DELETE', "services/iscsi/targetgroup/$tg");
> +};
> +
> +my $freenas_create_extent = sub {
> +    my ($scfg, $zvol) = @_;
> +    my $data;
> +    

whitespace

> +    $freenas_get_version->($scfg);
> +    

whitespace

> +    if ($version < 110000) {
> +        $data = {
> +            iscsi_target_extent_type => 'Disk',
> +            iscsi_target_extent_name => $zvol,
> +            iscsi_target_extent_disk => "zvol/$scfg->{pool}/$zvol",
> +        };
> +    } elsif ($version < 110100) {
> +        $data = {
> +            iscsi_target_extent_type => 'Disk',
> +            iscsi_target_extent_name => $zvol,
> +            iscsi_target_extent_disk => "zvol/$scfg->{pool}/$zvol",
> +        };
> +    } else {
> +        die "FreeNAS-$version: Unsupported!\n";
> +    }

again - no such if in the associated GET sub?

> +
> +    my $extent = $freenas_request->(
> +        $scfg, 'POST', "services/iscsi/extent/", encode_json($data));
> +    

whitespace

> +    return $extent->{id};
> +};
> +
> +my $freenas_delete_extent = sub {
> +    my ($scfg, $extent) = @_;
> +
> +    $freenas_request->($scfg, 'DELETE', "services/iscsi/extent/$extent/");
> +};
> +
> +my $freenas_get_extent = sub {
> +    my ($scfg, $volname) = @_;
> +    my $extent = undef;
> +    

whitespace

> +    my $extents = $freenas_request->($scfg, 'GET', "services/iscsi/extent/");
> +    foreach my $ext (@$extents) {
> +        if ($ext->{iscsi_target_extent_path} =~ /$scfg->{pool}\/$volname$/) {
> +            $extent = $ext->{id};
> +            last;
> +        }

and the same comment again, drop the last, return here, ...

> +    }
> +    

whitespace

> +    return $extent;
> +};
> +
> +my $freenas_create_target_to_exent = sub {
> +    my ($scfg, $target, $extent, $lunid) = @_;
> +    my $data;
> +    

whitespace

> +    $freenas_get_version->($scfg);
> +    

whitespace

> +    if ($version < 110000) {
> +        $data = {
> +            iscsi_target => $target,
> +            iscsi_extent => $extent,
> +            iscsi_lunid => $lunid,
> +        };
> +    } elsif ($version < 110100) {
> +        $data = {
> +            iscsi_target => $target,
> +            iscsi_extent => $extent,
> +            iscsi_lunid => $lunid,
> +        };
> +    } else {
> +        die "FreeNAS-$version: Unsupported!\n";
> +    }

and again, no such if in the GET sub?

> +
> +    my $tg2extent = $freenas_request->(
> +        $scfg, 'POST', "services/iscsi/targettoextent/", encode_json($data));

does this not have the same semantics as create_target and
create_target_group?

> +    

whitespace

> +    return $tg2extent->{id};
> +};
> +
> +my $freenas_delete_target_to_exent = sub {
> +    my ($scfg, $tg2exent) = @_;
> +
> +    $freenas_request->($scfg, 'DELETE', "services/iscsi/targettoextent/$tg2exent/");
> +};
> +
> +my $freenas_get_target_to_exent = sub {
> +    my ($scfg, $extent, $target) = @_;
> +    my $t2extent = undef;
> +    

whitespace

> +    my $t2extents = $freenas_request->($scfg, 'GET', "services/iscsi/targettoextent/");
> +    foreach my $t2ext (@$t2extents) {
> +        if ($t2ext->{iscsi_target} == $target && $t2ext->{iscsi_extent} == $extent) {
> +            $t2extent = $t2ext->{id};
> +            last;
> +        }

again, drop the last, return here, ...

> +    }
> +    

whitespace

> +    return $t2extent;
> +};
> +
> +my $freenas_find_free_diskname = sub {
> +    my ($storeid, $scfg, $vmid, $format) = @_;
> +
> +    my $name = undef;
> +    my $volumes = $freenas_list_zvol->($scfg);
> +
> +    my $disk_ids = {};
> +    my $dat = $volumes->{$scfg->{pool}};
> +
> +    foreach my $image (keys %$dat) {
> +        my $volname = $dat->{$image}->{name};
> +        if ($volname =~ m/vm-$vmid-disk-(\d+)/){
> +            $disk_ids->{$1} = 1;
> +        }
> +    }
> +
> +    for (my $i = 1; $i < $max_luns + 1; $i++) {
> +        if (!$disk_ids->{$i}) {
> +            return "vm-$vmid-disk-$i";
> +        }
> +    }

again, here you have the assumption that only numbers from 1 to $max_lun
are valid - I would really like to decouple the LUN numbering from the
disk index.

> +        

whitespace

> +    die "Maximum number of LUNs($max_luns) for this VM $vmid in storage '$storeid' is reached.\n";
> +};
> +
> +my $freenas_get_lun_number = sub {
> +    my ($scfg, $volname) = @_;
> +    my $lunid = undef;
> +    

whitespace

> +    if ($volname =~ /^(vm|base)-\d+-disk-(\d+)$/) {
> +        $lunid = $2 - 1;

is there no other way? this seriously limits stuff that works with other
storage plugins, like custom disk names..

> +    } elsif ($volname =~ /^vm-(\d+)-state/) {
> +        # Find id for temporary LUN
> +        my $target = $freenas_get_target->($scfg, $1);
> +        my $id = $max_luns;
> +        my $t2extents = $freenas_request->($scfg, 'GET', "services/iscsi/targettoextent/");
> +
> +        foreach my $t2extent (@$t2extents) {
> +            next unless $t2extent->{iscsi_target} == $target &&
> +                        $t2extent->{iscsi_lunid} + 1 > $max_luns &&
> +                        $t2extent->{iscsi_lunid} < $max_luns + $active_snaps;
> +            my $eid = $freenas_get_extent->($scfg, $volname);
> +            if ($eid) {
> +                my $extent = $freenas_request->($scfg, 'GET', "services/iscsi/extent/$eid/");
> +                # Request to get lunid for an existing lun
> +                last if $t2extent->{iscsi_extent} eq $eid;
> +            }
> +            $id++;
> +        }
> +        die "Max snapshots ($active_snaps) is reached\n" unless ($id - $max_luns) < $active_snaps;
> +        $lunid = $id;
> +    } elsif ($volname =~ /^(vm|base)-\d+-disk-\d+\@vzdump$/) {
> +        # Required to be able to exposed read-only LUNs for snapshot backup CT
> +        $lunid = $max_luns + $active_snaps;
> +    }
> +    

whitespace

> +    return $lunid;
> +};
> +
> +my $freenas_create_lun = sub {
> +    my ($scfg, $vmid, $zvol) = @_;
> +    my ($target, $tg, $extent, $tg2exent) = (undef, undef, undef, undef);
> +    

whitespace

> +    eval {
> +        $target = $freenas_create_target->($scfg, $vmid);
> +        die "create_lun-> Could not create target for VM '$vmid'\n" unless $target;
> +        $tg = $freenas_create_target_group->($scfg, $target);
> +        die "create_lun-> Could not create target group for VM '$vmid'\n" unless $tg;
> +        $extent = $freenas_create_extent->($scfg, $zvol);
> +        die "create_lun-> Could not create extent for VM '$vmid'\n" unless $extent;
> +        my $lunid = $freenas_get_lun_number->($scfg, $zvol);
> +        die "create_lun-> $zvol: Bad name format for VM '$vmid'\n" unless defined $lunid;
> +        $tg2exent = $freenas_create_target_to_exent->($scfg, $target, $extent, $lunid);
> +        die "create_lun-> Could not create target to extent for VM '$vmid'\n" unless defined $tg2exent;
> +    };

the feedback I gave on this is also not incorporated (yet?)

> +    if ($@) {
> +        my $err = $@;
> +        my $no_more_extents = 0;
> +        if ($tg2exent) {
> +            eval {
> +                $freenas_delete_target_to_exent->($scfg, $tg2exent);
> +            };
> +            warn "Could not delete target to extent: $@\n" if $@;
> +        }
> +        if ($extent) {
> +            eval {
> +                $freenas_delete_extent->($scfg, $extent);
> +            };
> +            warn "Could not delete extent: $@\n" if $@;
> +        }
> +        eval {
> +            $no_more_extents = $freenas_no_more_extents->($scfg, $target);
> +        };
> +        warn "Could not decide whether more extents exists: $@\n" if $@;
> +        if ($target && $no_more_extents) {
> +            if ($tg) {
> +                eval {
> +                    $freenas_delete_target_group->($scfg, $tg);
> +                };
> +                warn "Could not delete target group: $@\n" if $@;
> +            }
> +            eval {
> +                $freenas_delete_target->($scfg, $target);
> +            };
> +            warn "Could not delete target: $@\n" if $@;
> +        }
> +        die "create_lun: $err\n";
> +    }
> +};
> +
> +my $freenas_create_zvol = sub {
> +    my ($scfg, $volname, $size) = @_;
> +    

whitespace

> +    my $data = {
> +        name => $volname,
> +        volsize => $size,
> +    };
> +    my $zvol = $freenas_request->(
> +        $scfg, 'POST', "storage/volume/$scfg->{pool}/zvols/", encode_json($data));
> +
> +    die "$volname: Failed creating volume\n" unless $zvol && $zvol->{name};
> +    

whitespace

> +    return $zvol->{name};
> +};
> +
> +my $freenas_delete_zvol = sub {
> +    my ($scfg, $volname) = @_;
> +
> +    $freenas_request->($scfg, 'DELETE', "storage/volume/$scfg->{pool}/zvols/$volname/");
> +};
> +
>  my $os_request = sub {
>      my ($cmd, $noerr, $timeout) = @_;
>  
> @@ -263,16 +650,49 @@ my $os_request = sub {
>      return wantarray ? ($exit_code, $text) : $exit_code;
>  };
>  
> -my $freenas_get_target_name = sub {

see way above - I guess this was a mishap when using 'git add -p'?

> +my $disk_by_path = sub {
>      my ($scfg, $volname) = @_;
> -    my $name = undef;
> -    

whitespace

> -    if ($volname =~ /^(vm|base)-(\d+)-/) {
> -        $name = "vm-$2";
> -        return "$target_prefix\:$name";
> +
> +    my $target = $freenas_get_target_name->($scfg, $volname);
> +    my $lun = $freenas_get_lun_number->($scfg, $volname);
> +    my $path = "/dev/disk/by-path/ip-$scfg->{portal}\:3260-iscsi-$target-lun-$lun";
> +
> +    return $path;
> +};
> +
> +my $build_lun_list = sub {
> +    my ($scfg, $sid, $lun) = @_;
> +
> +    my $luns = {};
> +    my $text = '';
> +    my $exit = 0;
> +
> +    eval {
> +        ($exit, $text) = $os_request->(
> +            ['iscsiadm', '-m', 'session', '-r', $sid, '-P3'], 1, 60);
> +    };
> +    if ($@) {
> +        # An exist code of 22 means no active session otherwise an error
> +        if ($exit != 22) {
> +            die "$@\n";
> +        }

again, this is not how run_command with $noerr works.

> +    }
> +    if ($text =~ /.*Host Number:\s*(\d+)\s+State:\s+running(.*)/s) {
> +        my $host = $1;
> +        my $found = 0;
> +        for (split /^/, $2) {
> +            if ($_ =~ /Channel\s+(\d+)\s+Id\s+(\d+)\s+Lun:\s+(\d+)/) {
> +                if (defined $lun && $lun == $3) {
> +                    $luns = {};
> +                    $found = 1;

no need for $found and reseting $luns - simply return here?

> +                }
> +                $luns->{$3} = "$host:".int($1).":$2:$3";
> +                last if $found;
> +            }
> +        }

why not iterate line by line and match each line? would make the code
easier to read. some comment about expected output would also be nice.

>      }
>  

whitespace

> -    return undef;
> +    return $luns;
>  };
>  

whitespace

>  my $get_sid = sub {
> @@ -360,6 +780,59 @@ my $remove_local_lun = sub {
>      }
>  };
>  

whitespace

> +my $deactivate_luns = sub {
> +    # $luns contains a hash of luns to keep
> +    my ($scfg, $volname, $luns) = @_;
> +
> +    $luns = {} unless $luns;

unless

> +    my $sid;
> +    my $list = {};
> +
> +    $sid = $get_sid->($scfg, $volname);
> +
> +    $list = $build_lun_list->($scfg, $sid);
> +
> +    foreach my $key (keys %$list) {
> +        next if exists($luns->{$key});

s/exists/defined/

> +        eval {
> +            $remove_local_lun->($list->{$key});
> +        };
> +        warn "Remove local LUN '$list->{$key}' failed: $@\n" if $@;

$remove_local_lun already warns?

> +    }
> +};
> +
> +my $get_active_luns = sub {
> +    my ($class, $storeid, $scfg, $volname) = @_;
> +
> +    my $sid = 0;

in other places, you use -1 to denote an uninitialized sid?

> +    my $luns = {};
> +
> +    $sid = $get_sid->($scfg, $volname);
> +
> +    if ($sid < 0) {
> +        # We have no active sessions so make one
> +        $sid = $create_session->($scfg, $volname);
> +        # Since no session existed prior to this call deactivate all LUN's found
> +        $deactivate_luns->($scfg, $volname);

the sub is called get_active_luns, but if there is no active session
there are no active LUNs, so why create a session and deactivate all
LUNs here? this seems like a strange side-effect that belongs somewhere
else (or should be made explicit)

> +    } else {
> +        $luns = $build_lun_list->($scfg, $sid);
> +    }
> +
> +    return $luns;
> +};
> +
> +my $rescan_session = sub {
> +    my ($class, $storeid, $scfg, $volname, $exclude_lun) = @_;
> +
> +    my $luns_to_keep = $get_active_luns->($class, $storeid, $scfg, $volname);
> +    delete $luns_to_keep->{$exclude_lun} if defined $exclude_lun;
> +    my $sid = $get_sid->($scfg, $volname);
> +    die "Missing session\n" if $sid < 0;
> +    $os_request->(['iscsiadm', '-m', 'session', '-r', $sid, '-R'], 0, 60);
> +    $deactivate_luns->($scfg, $volname, $luns_to_keep);
> +    $delete_session->($scfg, $sid) unless %$luns_to_keep;

unless

> +};
> +
>  # Storage implementation
>  
>  sub volume_size_info {
> @@ -453,6 +926,12 @@ sub path {
>  
>      my ($vtype, $vname, $vmid) = $class->parse_volname($volname);
>  

whitespace

> +    $vname = "$vname\@$snapname" if $snapname;
> +
> +    my $luns = $get_active_luns->($class, $storeid, $scfg, $vname);

not used anywhere (or is this only because of the side-effect above!?)?

> +    my $path = $disk_by_path->($scfg, $vname);
> +    

whitespace

> +    return ($path, $vmid, $vtype);
>  }
>  
>  sub create_base {
> @@ -475,6 +954,28 @@ sub alloc_image {
>      my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
>      die "unsupported format '$fmt'\n" if $fmt ne 'raw';
>  
> +    die "illegal name '$name' - sould be 'vm-$vmid-*'\n"

s/sould/should

> +    if $name && $name !~ m/^vm-$vmid-/;
> +
> +    $name = $freenas_find_free_diskname->($storeid, $scfg, $vmid, $fmt) unless $name;

unless

> +    

whitespace

> +    # Size is in KB but Freenas wants in bytes
> +    $size *= 1024;
> +    my $zvol = $freenas_create_zvol->($scfg, $name, $size);
> +
> +    eval {
> +        $freenas_create_lun->($scfg, $vmid, $zvol);
> +    };
> +    if ($@) {
> +        my $err = $@;
> +        eval {
> +            $freenas_delete_zvol->($scfg, $name);
> +        };
> +        warn "Cleanup failed: $@\n" if $@;
> +        die "$err\n";
> +    }
> +
> +    return $name;
>  }
>  
>  sub free_image {
> @@ -482,6 +983,47 @@ sub free_image {
>  
>      my ($vtype, $name, $vmid, $basename) = $class->parse_volname($volname);
>  
> +    my $target = $freenas_get_target->($scfg, $vmid);
> +    die "free_image-> missing target\n" unless $target;
> +    my $extent = $freenas_get_extent->($scfg, $name);
> +    die "free_image-> missing extent\n" unless $extent;
> +    my $tg2exent = $freenas_get_target_to_exent->($scfg, $extent, $target);
> +    die "free_image-> missing target to extent\n" unless $tg2exent;
> +    my $target_group = $freenas_get_target_group->($scfg, $target);
> +    die "free_image-> missing target group\n" unless $target_group;
> +    my $lun = $freenas_get_lun_number->($scfg, $name);
> +    die "free_image-> missing LUN\n" unless defined $lun;
> +    

whitespace

> +    eval {
> +        my $res = $class->deactivate_volume($storeid, $scfg, $volname);
> +        warn "Could not deactivate volume '$volname'\n" unless $res;
> +        $freenas_delete_target_to_exent->($scfg, $tg2exent);
> +        $freenas_delete_extent->($scfg, $extent);
> +        if ($target && $freenas_no_more_extents->($scfg, $target)) {
> +            if ($target_group) {
> +                $freenas_delete_target_group->($scfg, $target_group);
> +            }
> +            $freenas_delete_target->($scfg, $target);
> +        }
> +        $freenas_delete_zvol->($scfg, $name);
> +        $class->volume_snapshot_delete($scfg, $storeid, $basename, "__base__$vmid") if $basename;
> +        if ($isBase) {
> +            $basename = $name;
> +            $basename =~ s/^base-/vm-/;
> +            $class->volume_snapshot_delete($scfg, $storeid, $basename, '__base__')  if $basename;
> +            $freenas_delete_zvol->($scfg, $basename);
> +        }
> +    };
> +    if ($@) {
> +        my $err = $@;
> +        eval {
> +            $freenas_create_lun->($scfg, $vmid, $name) unless $isBase;
> +        };
> +        warn "Recreate LUN failed: $@\n" if $@;
> +        die "$err\n";
> +    }
> +
> +    return undef;
>  }
>  
>  sub volume_resize {
> @@ -579,6 +1121,35 @@ sub deactivate_storage {
>  # deactivate all luns except our luns
>  sub activate_volume {
>      my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> +    my $lun;
> +    

whitespace

> +    my (undef, $name, $vmid) = $class->parse_volname($volname);
> +    

whitespace

> +    my $luns_to_keep = $get_active_luns->($class, $storeid, $scfg, $name);
> +
> +    if ($snapname) {
> +        eval {
> +            $freenas_create_lun->($scfg, $vmid, "$name\@$snapname");
> +            $lun = $freenas_get_lun_number->($scfg, "$name\@$snapname");
> +            $luns_to_keep->{$lun} = "0:0:0:$lun";
> +        };
> +        if ($@) {
> +            die "$@ - unable to activate snapshot from remote FreeNAS storage\n";
> +        }
> +    }
> +    

whitespace

> +    $lun = $freenas_get_lun_number->($scfg, $name);
> +    $luns_to_keep->{$lun} = "0:0:0:$lun";
> +
> +    my $sid = $get_sid->($scfg, $name);
> +    die "activate_volume-> Missing session\n" if $sid < 0;
> +    # Add new LUN's to session
> +    $os_request->(['iscsiadm', '-m', 'session', '-r', $sid, '-R'], 0, 60);
> +    $os_request->(
> +        ['udevadm', 'trigger', '--type=devices', '--subsystem-match=scsi_disk'], 0, 60);
> +    $os_request->(['udevadm', 'settle', '-t', $api_timeout], 0, 60);
> +    # Remove all LUN's from session which is not currently active
> +    $deactivate_luns->($scfg, $name, $luns_to_keep);
>  
>      return 1;
>  }
> @@ -593,7 +1164,17 @@ sub deactivate_volume {
>  
>      my (undef, $name) = $class->parse_volname($volname);
>  
> -   return 1;
> +    my $luns_to_keep = $get_active_luns->($class, $storeid, $scfg, $name);
> +
> +    my $lun = $freenas_get_lun_number->($scfg, $name);
> +    delete $luns_to_keep->{$lun};
> +
> +    my $sid = $get_sid->($scfg, $name);
> +    die "deactivate_volume-> Missing session\n" if $sid < 0;
> +    $deactivate_luns->($scfg, $name, $luns_to_keep);
> +    $delete_session->($scfg, $sid) unless %$luns_to_keep;
> +
> +    return 1;
>  }
>  
>  1;
> -- 
> 2.11.0
> 
> 
> ----
> 
> This mail was virus scanned and spam checked before delivery.
> This mail is also DKIM signed. See header dkim-signature.
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



More information about the pve-devel mailing list