[pve-devel] [PATCH v6 pve-storage 1/1] A lot of bug fixes and clean-ups

mir at datanom.net mir at datanom.net
Mon Jun 19 16:44:21 CEST 2017


From: Michael Rasmussen <mir at datanom.net>

fixes

TODO

Signed-off-by: Michael Rasmussen <mir at datanom.net>
---
 PVE/Storage/FreeNASPlugin.pm | 1011 ++++++++++++++++++++++--------------------
 1 file changed, 533 insertions(+), 478 deletions(-)

diff --git a/PVE/Storage/FreeNASPlugin.pm b/PVE/Storage/FreeNASPlugin.pm
index 9fe64d6..7860eb8 100644
--- a/PVE/Storage/FreeNASPlugin.pm
+++ b/PVE/Storage/FreeNASPlugin.pm
@@ -2,7 +2,6 @@ package PVE::Storage::FreeNASPlugin;
 
 use strict;
 use warnings;
-use IO::File;
 use POSIX;
 use PVE::Tools qw(run_command);
 use PVE::Storage::Plugin;
@@ -17,31 +16,84 @@ use Data::Dumper;
 use base qw(PVE::Storage::Plugin);
 
 my $api = '/api/v1.0';
-my $timeout = 60; # seconds
+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 $active_snaps = 4;
-my $limit = 10000; # limit for get requests
+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 $version;
-my $fullversion;
-my $target_prefix = 'iqn.2005-10.org.freenas.ctl';
+my $target_prefix = 'iqn.2005-10.org.freenas.ctl'; # automatically prepended
+                                                   # in FreeNAS
 
-sub freenas_request {
-    my ($scfg, $request, $section, $data, $valid_code) = @_;
+# Configuration
+
+sub type {
+    return 'freenas';
+}
+
+sub plugindata {
+    return {
+        content => [ {images => 1, rootdir => 1}, {images => 1 , rootdir => 1} ],
+        format => [ { raw => 1 } , 'raw' ],
+    };
+}
+
+sub properties {
+    return {
+        password => {
+            description => "password",
+            type => "string",
+        },
+        portal_group => {
+            description => "Portal Group ID",
+            type => "integer",
+        },
+        initiator_group => {
+            description => "Initiator Group ID",
+            type => "integer",
+        },
+    };
+}
+
+sub options {
+    return {
+        portal => { fixed => 1 },
+        pool => { fixed => 1 },
+        portal_group => { fixed => 1 },
+        initiator_group => { fixed => 1 },
+#       blocksize => { optional => 1 }, not available in 9.2.x. Appear in 11.x
+        username => { optional => 1 },
+        password => { optional => 1 },
+#       sparse => { optional => 1 }, not available in 9.2.x. Appear in 11.x
+#                                    in 9.2.x all zvols are created sparse!
+        nodes => { optional => 1 },
+        disable => { optional => 1 },
+        content => { optional => 1 },
+    };
+}
+
+# private methods
+
+my $freenas_request = sub {
+    my ($scfg, $request, $section, $data) = @_;
     my $ua = LWP::UserAgent->new;
     $ua->agent("ProxmoxUA/0.1");
     $ua->ssl_opts( verify_hostname => 0 );
-    $ua->timeout($timeout);
+    $ua->timeout($api_timeout);
     push @{ $ua->requests_redirectable }, 'POST';
     push @{ $ua->requests_redirectable }, 'PUT';
     push @{ $ua->requests_redirectable }, 'DELETE';
-    my $req;
+    my ($req, $res, $content) = (undef, undef, undef);
 
     my $url = "https://$scfg->{portal}$api/$section";
 
     if ($request eq 'GET') {
-        $req = HTTP::Request->new(GET => $url);
+        $req = HTTP::Request->new;
     } elsif ($request eq 'POST') {
         $req = HTTP::Request->new(POST => $url);
         $req->content($data);
@@ -51,25 +103,91 @@ sub freenas_request {
     } elsif ($request eq 'DELETE') {
         $req = HTTP::Request->new(DELETE => $url);
     } else {
-        die "$request: Unknown request";
+        die "$request: Unknown request\n";
     }
 
     $req->content_type('application/json');
     $req->authorization_basic($scfg->{username}, $scfg->{password});
-    my $res = $ua->request($req);
+    
+    if ($request eq 'GET') {
+        my $offset = 0;
+        my $keep_going = 1;
+        my $tmp;
+        $req->method('GET');
+        while ($keep_going) {
+            my $rows = 0;
+            my $uri = "$url?offset=$offset&limit=$rows_per_request";
+            $req->uri($uri);
+            $res = $ua->request($req);
+            do {
+                $keep_going = 0;
+                last;
+            } unless $res->is_success || $res->content ne "";
+            eval {
+                $tmp = decode_json($res->content);
+            };
+            do {
+                # Not JSON or invalid JSON payload
+                $tmp = $res->content;
+                if (defined $content && ref($content) eq 'ARRAY') {
+                    # error
+                    push(@$content, [$tmp]);
+                } elsif (defined $content) {
+                    $content .= $res->content;
+                } else {
+                    $content = $res->content;
+                }
+                $keep_going = 0;
+                last;
+            } if $@;
+            # We got valid JSON payload
+            if (defined $content && ref($content) eq 'ARRAY') {
+                if (ref($tmp) eq 'ARRAY') {
+                    push(@$content, @$tmp);
+                } else {
+                    # error
+                    push(@$content, [$tmp]);
+                    $keep_going = 0;
+                    last;
+                }
+            } elsif (defined $content) {
+                if (ref($tmp) eq 'ARRAY') {
+                    # error
+                    $content .= "@$tmp";
+                } else {
+                    $content .= $tmp;
+                }
+                $keep_going = 0;
+                last;
+            } else {
+                $content = $tmp;
+                if (ref($tmp) ne 'ARRAY') {
+                    $keep_going = 0;
+                    last;
+                }
+            }
+            $rows = @$tmp;
+            $keep_going = 0 unless $rows >= $rows_per_request;
+            $offset += $rows;
+        } 
+    } else {
+        $res = $ua->request($req);
+        eval {
+            $content = decode_json($res->content);
+        };
+        $content = $res->content if $@;
+    }
 
-    return $res->code unless $res->is_success;
+    die $res->code."\n" unless $res->is_success;
 
-    return $res->content;
-}
+    return wantarray ? ($res->code, $content) : $content;
+};
 
-sub freenas_get_version {
+my $freenas_get_version = sub {
     my ($scfg) = @_;
     
-    my $response = freenas_request($scfg, 'GET', "system/version");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $info = decode_json($response);
-    $fullversion = $info->{fullversion};
+    my $response = $freenas_request->($scfg, 'GET', "system/version/");
+    my $fullversion = $response->{fullversion};
     if ($fullversion =~ /^\w+-(\d+)\.(\d*)\.(\d*)/) {
         my $minor = $2;
         my $micro = $3;
@@ -88,21 +206,19 @@ sub freenas_get_version {
             
         $version = "$1$minor$micro";
     } else {
-        $version = '90200';
+        die "$fullversion: Cannot parse\n";
     }
-}
 
-sub freenas_list_zvol {
+    die "$fullversion: Unsupported version\n" if $version < 90200;
+};
+
+my $freenas_list_zvol = sub {
     my ($scfg) = @_;
 
-    freenas_get_version($scfg) unless $version;
+    $freenas_get_version->($scfg);
     
-    my $response = freenas_request($scfg, 'GET', "storage/volume/$scfg->{pool}/zvols?limit=$limit");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $zvols = decode_json($response);
-    $response = freenas_request($scfg, 'GET', "storage/snapshot?limit=$limit");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $snapshots = decode_json($response);
+    my $zvols = $freenas_request->($scfg, 'GET', "storage/volume/$scfg->{pool}/zvols/");
+    my $snapshots = $freenas_request->($scfg, 'GET', "storage/snapshot/");
 
     my $list = ();
     my $hide = {};
@@ -131,28 +247,24 @@ sub freenas_list_zvol {
     delete @{$list->{$scfg->{pool}}}{keys %$hide};
     
     return $list;
-}
+};
 
-sub freenas_no_more_extents {
+my $freenas_no_more_extents = sub {
     my ($scfg, $target) = @_;
     
-    my $response = freenas_request($scfg, 'GET', "services/iscsi/targettoextent?limit=$limit");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $extents = decode_json($response);
+    my $extents = $freenas_request->($scfg, 'GET', "services/iscsi/targettoextent/");
     foreach my $extent (@$extents) {
         return 0 if $extent->{iscsi_target} == $target;
     }
     
     return 1;
-}
+};
     
-sub freenas_get_target {
+my $freenas_get_target = sub {
     my ($scfg, $vmid) = @_;
     my $target = undef;
     
-    my $response = freenas_request($scfg, 'GET', "services/iscsi/target?limit=$limit");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $targets = decode_json($response);
+    my $targets = $freenas_request->($scfg, 'GET', "services/iscsi/target/");
     foreach my $t (@$targets) {
         if ($t->{iscsi_target_name} eq "vm-$vmid") {
             $target = $t->{id};
@@ -161,13 +273,13 @@ sub freenas_get_target {
     }
     
     return $target;
-}
+};
 
-sub freenas_create_target {
-    my ($scfg, $vmid, $valid_code) = @_;
-    my $data;
+my $freenas_create_target = sub {
+    my ($scfg, $vmid) = @_;
+    my ($data, $target);
     
-    freenas_get_version($scfg) unless $version;
+    $freenas_get_version->($scfg);
 
     if ($version < 110000) {
         $data = {
@@ -180,28 +292,32 @@ sub freenas_create_target {
             iscsi_target_name => "vm-$vmid",
         };
     } else {
-        die "FreeNAS-$version: Unsupported!";
+        die "FreeNAS-$version: Unsupported!\n";
     }
-    my $response = freenas_request($scfg, 'POST', "services/iscsi/target", encode_json($data), $valid_code);
-    if ($response =~ /^\d+$/) {
-        return freenas_get_target($scfg, $vmid) if $valid_code && $response == $valid_code;
-        die HTTP::Status::status_message($response);
+
+    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);
+        }
+        die "Creating target for 'vm-$vmid' failed: $@\n";
     }
-    my $target = decode_json($response);
-    
-    die "Creating target for 'vm-$vmid' failed" unless $target->{id};
     
     return $target->{id};
-}
+};
 
-sub freenas_delete_target {
+my $freenas_delete_target = sub {
     my ($scfg, $target) = @_;
 
-    my $response = freenas_request($scfg, 'DELETE', "services/iscsi/target/$target");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-}
+    $freenas_request->($scfg, 'DELETE', "services/iscsi/target/$target/");
+};
 
-sub freenas_get_target_name {
+my $freenas_get_target_name = sub {
     my ($scfg, $volname) = @_;
     my $name = undef;
     
@@ -211,21 +327,37 @@ sub freenas_get_target_name {
     }
 
     return undef;
-}
+};
+
+my $freenas_get_target_group = sub {
+    my ($scfg, $target) = @_;
+    my $targetgroup = undef;
+    
+    my $targetgroups = $freenas_request->($scfg, 'GET', "services/iscsi/targetgroup/");
 
-sub freenas_create_target_group {
-    my ($scfg, $target, $valid_code) = @_;
+    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;
+        }
+    }
+    
+    return $targetgroup;
+};
+
+my $freenas_create_target_group = sub {
+    my ($scfg, $target) = @_;
     my $data;
     
-    freenas_get_version($scfg) unless $version;
+    $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
-    if ($valid_code && $valid_code == 409) {
-        my $tg = freenas_get_target_group($scfg, $target);
-        return $tg if $tg;
-    }
+    my $tg = $freenas_get_target_group->($scfg, $target);
+    return $tg if $tg;
     
     if ($version < 110000) {
         $data = {
@@ -246,55 +378,36 @@ sub freenas_create_target_group {
             iscsi_target_initialdigest => "Auto",
         };
     } else {
-        die "FreeNAS-$version: Unsupported!";
+        die "FreeNAS-$version: Unsupported!\n";
     }
 
-    my $response = freenas_request($scfg, 'POST', "services/iscsi/targetgroup", encode_json($data), $valid_code);
-    if ($response =~ /^\d+$/) {
-        if ($valid_code != 409) {
-            return freenas_get_target_group($scfg, $target) if $valid_code && $response == $valid_code;
+    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);
         }
-        die HTTP::Status::status_message($response);
+        die "Creating target group for target '$target' failed: $@\n";
     }
-    my $tg = decode_json($response);
-    
-    die "Creating target group for target '$target' failed" unless $tg->{id};
     
     return $tg->{id};
-}
+};
 
-sub freenas_delete_target_group {
+my $freenas_delete_target_group = sub {
     my ($scfg, $tg) = @_;
 
-    my $response = freenas_request($scfg, 'DELETE', "services/iscsi/targetgroup/$tg");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-}
-
-sub freenas_get_target_group {
-    my ($scfg, $target) = @_;
-    my $targetgroup = undef;
-    
-    my $response = freenas_request($scfg, 'GET', "services/iscsi/targetgroup?limit=$limit");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $targetgroups = decode_json($response);
-
-    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;
-        }
-    }
-    
-    return $targetgroup;
-}
+    $freenas_request->($scfg, 'DELETE', "services/iscsi/targetgroup/$tg");
+};
 
-sub freenas_create_extent {
+my $freenas_create_extent = sub {
     my ($scfg, $zvol) = @_;
     my $data;
     
-    freenas_get_version($scfg) unless $version;
+    $freenas_get_version->($scfg);
     
     if ($version < 110000) {
         $data = {
@@ -309,32 +422,26 @@ sub freenas_create_extent {
             iscsi_target_extent_disk => "zvol/$scfg->{pool}/$zvol",
         };
     } else {
-        die "FreeNAS-$version: Unsupported!";
+        die "FreeNAS-$version: Unsupported!\n";
     }
 
-    my $response = freenas_request($scfg, 'POST', "services/iscsi/extent", encode_json($data));
-    die HTTP::Status::status_message($response) if ($response =~ /^\d+$/);
-    my $extent = decode_json($response);
-    
-    die "Creating LUN for volume '$zvol' failed" unless $extent->{id};
+    my $extent = $freenas_request->(
+        $scfg, 'POST', "services/iscsi/extent/", encode_json($data));
     
     return $extent->{id};
-}
+};
 
-sub freenas_delete_extent {
+my $freenas_delete_extent = sub {
     my ($scfg, $extent) = @_;
 
-    my $response = freenas_request($scfg, 'DELETE', "services/iscsi/extent/$extent");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-}
+    $freenas_request->($scfg, 'DELETE', "services/iscsi/extent/$extent/");
+};
 
-sub freenas_get_extent {
+my $freenas_get_extent = sub {
     my ($scfg, $volname) = @_;
     my $extent = undef;
     
-    my $response = freenas_request($scfg, 'GET', "services/iscsi/extent?limit=$limit");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $extents = decode_json($response);
+    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};
@@ -343,13 +450,13 @@ sub freenas_get_extent {
     }
     
     return $extent;
-}
+};
 
-sub freenas_create_target_to_exent {
+my $freenas_create_target_to_exent = sub {
     my ($scfg, $target, $extent, $lunid) = @_;
     my $data;
     
-    freenas_get_version($scfg) unless $version;
+    $freenas_get_version->($scfg);
     
     if ($version < 110000) {
         $data = {
@@ -364,32 +471,26 @@ sub freenas_create_target_to_exent {
             iscsi_lunid => $lunid,
         };
     } else {
-        die "FreeNAS-$version: Unsupported!";
+        die "FreeNAS-$version: Unsupported!\n";
     }
 
-    my $response = freenas_request($scfg, 'POST', "services/iscsi/targettoextent", encode_json($data));
-    die HTTP::Status::status_message($response) if ($response =~ /^\d+$/);
-    my $tg2extent = decode_json($response);
-    
-    die "Creating view for LUN '$extent' failed" unless $tg2extent->{id};
+    my $tg2extent = $freenas_request->(
+        $scfg, 'POST', "services/iscsi/targettoextent/", encode_json($data));
     
     return $tg2extent->{id};
-}
+};
 
-sub freenas_delete_target_to_exent {
+my $freenas_delete_target_to_exent = sub {
     my ($scfg, $tg2exent) = @_;
 
-    my $response = freenas_request($scfg, 'DELETE', "services/iscsi/targettoextent/$tg2exent");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-}
+    $freenas_request->($scfg, 'DELETE', "services/iscsi/targettoextent/$tg2exent/");
+};
 
-sub freenas_get_target_to_exent {
+my $freenas_get_target_to_exent = sub {
     my ($scfg, $extent, $target) = @_;
     my $t2extent = undef;
     
-    my $response = freenas_request($scfg, 'GET', "services/iscsi/targettoextent?limit=$limit");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $t2extents = decode_json($response);
+    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};
@@ -398,21 +499,21 @@ sub freenas_get_target_to_exent {
     }
     
     return $t2extent;
-}
+};
 
-sub freenas_find_free_diskname {
+my $freenas_find_free_diskname = sub {
     my ($storeid, $scfg, $vmid, $format) = @_;
 
     my $name = undef;
-    my $volumes = freenas_list_zvol($scfg);
+    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|base)-$vmid-disk-(\d+)/){
-            $disk_ids->{$2} = 1;
+        if ($volname =~ m/vm-$vmid-disk-(\d+)/){
+            $disk_ids->{$1} = 1;
         }
     }
 
@@ -422,10 +523,10 @@ sub freenas_find_free_diskname {
         }
     }
         
-    die "Maximum number of LUNs($max_luns) for this VM $vmid in storage '$storeid' is reached.";
-}
+    die "Maximum number of LUNs($max_luns) for this VM $vmid in storage '$storeid' is reached.\n";
+};
 
-sub freenas_get_lun_number {
+my $freenas_get_lun_number = sub {
     my ($scfg, $volname) = @_;
     my $lunid = undef;
     
@@ -433,27 +534,23 @@ sub freenas_get_lun_number {
         $lunid = $2 - 1;
     } elsif ($volname =~ /^vm-(\d+)-state/) {
         # Find id for temporary LUN
-        my $target = freenas_get_target($scfg, $1);
+        my $target = $freenas_get_target->($scfg, $1);
         my $id = $max_luns;
-        my $response = freenas_request($scfg, 'GET', "services/iscsi/targettoextent?limit=$limit");
-        die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-        my $t2extents = decode_json($response);
+        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);
+            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);
+                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 (4) is reached" unless ($id - $max_luns) < $active_snaps;
+        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
@@ -461,70 +558,85 @@ sub freenas_get_lun_number {
     }
     
     return $lunid;
-}
+};
 
-sub freenas_create_lun {
+my $freenas_create_lun = sub {
     my ($scfg, $vmid, $zvol) = @_;
     my ($target, $tg, $extent, $tg2exent) = (undef, undef, undef, undef);
     
     eval {
-        $target = freenas_create_target($scfg, $vmid, 409);
-        die "create_lun-> Could not create target for VM '$vmid'" unless $target;
-        $tg = freenas_create_target_group($scfg, $target, 409);
-        die "create_lun-> Could not create target group for VM '$vmid'" unless $tg;
-        $extent = freenas_create_extent($scfg, $zvol);
-        die "create_lun-> Could not create extent for VM '$vmid'" unless $extent;
-        my $lunid = freenas_get_lun_number($scfg, $zvol);
-        die "create_lun-> $zvol: Bad name format for VM '$vmid'" unless defined $lunid;
-        $tg2exent = freenas_create_target_to_exent($scfg, $target, $extent, $lunid);
-        die "create_lun-> Could not create target to extend for VM '$vmid'" unless defined $tg2exent;
+        $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;
     };
     if ($@) {
         my $err = $@;
+        my $no_more_extents = 0;
         if ($tg2exent) {
-            freenas_delete_target_to_exent($scfg, $tg2exent);
+            eval {
+                $freenas_delete_target_to_exent->($scfg, $tg2exent);
+            };
+            warn "Could not delete target to extent: $@\n" if $@;
         }
         if ($extent) {
-            freenas_delete_extent($scfg, $extent);
+            eval {
+                $freenas_delete_extent->($scfg, $extent);
+            };
+            warn "Could not delete extent: $@\n" if $@;
         }
-        if ($target && freenas_no_more_extents($scfg, $target)) {
+        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) {
-                freenas_delete_target_group($scfg, $tg);
+                eval {
+                    $freenas_delete_target_group->($scfg, $tg);
+                };
+                warn "Could not delete target group: $@\n" if $@;
             }
-            freenas_delete_target($scfg, $target);
+            eval {
+                $freenas_delete_target->($scfg, $target);
+            };
+            warn "Could not delete target: $@\n" if $@;
         }
-        die $err;
+        die "create_lun: $err\n";
     }
-}
+};
 
-sub freenas_create_zvol {
+my $freenas_create_zvol = sub {
     my ($scfg, $volname, $size) = @_;
     
     my $data = {
         name => $volname,
         volsize => $size,
     };
-    my $response = freenas_request($scfg, 'POST', "storage/volume/$scfg->{pool}/zvols", encode_json($data));
-    die HTTP::Status::status_message($response) if ($response =~ /^\d+$/);
-    my $zvol = decode_json($response);
+    my $zvol = $freenas_request->(
+        $scfg, 'POST', "storage/volume/$scfg->{pool}/zvols/", encode_json($data));
 
-    die "$volname: Failed creating volume" unless $zvol && $zvol->{name};
+    die "$volname: Failed creating volume\n" unless $zvol && $zvol->{name};
     
     return $zvol->{name};
-}
+};
 
-sub freenas_delete_zvol {
+my $freenas_delete_zvol = sub {
     my ($scfg, $volname) = @_;
 
-    my $response = freenas_request($scfg, 'DELETE', "storage/volume/$scfg->{pool}/zvols/$volname");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-}
+    $freenas_request->($scfg, 'DELETE', "storage/volume/$scfg->{pool}/zvols/$volname/");
+};
 
-sub os_request {
+my $os_request = sub {
     my ($cmd, $noerr, $timeout) = @_;
 
-    $timeout = PVE::RPCEnvironment::is_worker() ? 60*60 : 5 if !$timeout;
-    $noerr = 0 if !$noerr;
+    $timeout = 5 unless $timeout;
+    $noerr = 0 unless $noerr;
 
     my $text = '';
 
@@ -536,26 +648,19 @@ sub os_request {
     my $exit_code = run_command($cmd, noerr => $noerr, errfunc => $output, outfunc => $output, timeout => $timeout);
 
     return wantarray ? ($exit_code, $text) : $exit_code;
-}
-
-sub bail_out {
-    my ($class, $storeid, $scfg, $volname, $err) = @_;
-    
-    $class->free_image($storeid, $scfg, $volname);
-    die $err;
-}
+};
 
-sub disk_by_path {
+my $disk_by_path = sub {
     my ($scfg, $volname) = @_;
 
-    my $target = freenas_get_target_name($scfg, $volname);
-    my $lun = freenas_get_lun_number($scfg, $volname);
+    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;
-}
+};
 
-sub build_lun_list {
+my $build_lun_list = sub {
     my ($scfg, $sid, $lun) = @_;
 
     my $luns = {};
@@ -563,12 +668,13 @@ sub build_lun_list {
     my $exit = 0;
 
     eval {
-        ($exit, $text) = os_request("iscsiadm -m session -r $sid -P3", 1, 60);
+        ($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 "$@";
+            die "$@\n";
         }
     }
     if ($text =~ /.*Host Number:\s*(\d+)\s+State:\s+running(.*)/s) {
@@ -587,23 +693,24 @@ sub build_lun_list {
     }
 
     return $luns;
-}
+};
 
-sub get_sid {
+my $get_sid = sub {
     my ($scfg, $volname) = @_;
     my $sid = -1;
     my $text = '';
     my $exit = 0;
     
-    my $target = freenas_get_target_name($scfg, $volname);
+    my $target = $freenas_get_target_name->($scfg, $volname);
 
     eval {
-        ($exit, $text) = os_request("iscsiadm -m node -T $target -p $scfg->{portal} -s", 1, 60);
+        ($exit, $text) = $os_request->(
+            ['iscsiadm', '-m', 'node', '-T', $target, '-p', $scfg->{portal}, '-s'], 1, 60);
     };
     if ($@) {
         # An exist code of 21 or 22 means no active session otherwise an error
         if ($exit != 21 || $exit != 22) {
-            die "$@";
+            die "$@\n";
         }
     }
     if ($text =~ /.*\[sid\:\s*(\d+),\s*.*/) {
@@ -611,133 +718,128 @@ sub get_sid {
     }
 
     return $sid;
-}
+};
 
-sub create_session {
+my $create_session = sub {
     my ($scfg, $volname) = @_;
     my $sid = -1;
     my $exit = undef;
     
-    my $target = freenas_get_target_name($scfg, $volname);
+    my $target = $freenas_get_target_name->($scfg, $volname);
 
     eval {
-        $exit = os_request("iscsiadm -m node -T $target -p $scfg->{portal} --login", 1, 60);
+        $exit = $os_request->(
+            ['iscsiadm', '-m', 'node', '-T', $target, '-p', $scfg->{portal}, '--login'], 1, 60);
         if ($exit == 21) {
             eval {
-                os_request("iscsiadm -m discovery -t sendtargets -p $scfg->{portal}", 0, 60);
-                os_request("iscsiadm -m node -T $target -p $scfg->{portal} --login", 0, 60);
+                $os_request->(
+                    ['iscsiadm', '-m', 'discovery', '-t', 'sendtargets', '-p', $scfg->{portal}], 0, 60);
+                $os_request->(
+                    ['iscsiadm', '-m', 'node', '-T', $target, '-p', $scfg->{portal}, '--login'], 0, 60);
             };
         }
     };
     if ($@) {
         if ($exit == 21) {
             eval {
-                os_request("iscsiadm -m discovery -t sendtargets -p $scfg->{portal}", 0, 60);
-                os_request("iscsiadm -m node -T $target -p $scfg->{portal} --login", 0, 60);
+                $os_request->(
+                    ['iscsiadm', '-m', 'discovery', '-t', 'sendtargets', '-p', $scfg->{portal}], 0, 60);
+                $os_request->(
+                    ['iscsiadm', '-m', 'node', '-T', $target, '-p', $scfg->{portal}, '--login'], 0, 60);
             };
         } else {
-            die $@;
+            die "$@\n";
         }
     }
     eval {
-        $sid = get_sid($scfg, $volname);
+        $sid = $get_sid->($scfg, $volname);
     };
-    die "$@" if $@;
-    die "Could not create session" if $sid < 0;
+    die "$@\n" if $@;
+    die "Could not create session\n" if $sid < 0;
     
     return $sid;
-}
+};
 
-sub delete_session {
+my $delete_session = sub {
     my ($scfg, $sid) = @_;
     
     eval {
-        os_request("iscsiadm -m session -r $sid --logout", 0, 60);
+        $os_request->(['iscsiadm', '-m', 'session', '-r', $sid, '--logout'], 0, 60);
     };
-}
+    warn "Delete session failed: $@\n" if $@;
+};
 
-sub remove_local_lun {
+my $remove_local_lun = sub {
     my ($id) = @_;
     
-    os_request("echo 1 > /sys/bus/scsi/devices/$id/delete", 0, 60);
-}
+    open (my $fh, '>', "/sys/bus/scsi/devices/$id/delete") or
+        warn "Remove local LUN failed: $!\n";
+    if ($fh) {
+        print $fh '1';
+        close $fh;
+    }
+};
 
-sub deactivate_luns {
+my $deactivate_luns = sub {
     # $luns contains a hash of luns to keep
     my ($scfg, $volname, $luns) = @_;
 
-    $luns = {} if !$luns;
+    $luns = {} unless $luns;
     my $sid;
     my $list = {};
 
-    eval {      
-        $sid = get_sid($scfg, $volname);
-    };
-    die "$@" if $@;
+    $sid = $get_sid->($scfg, $volname);
 
-    eval {
-        $list = build_lun_list($scfg, $sid);
+    $list = $build_lun_list->($scfg, $sid);
 
-        foreach my $key (keys %$list) {
-            next if exists($luns->{$key});
-            remove_local_lun($list->{$key});
-        }
-    };
-    die "$@" if $@;
-}
+    foreach my $key (keys %$list) {
+        next if exists($luns->{$key});
+        eval {
+            $remove_local_lun->($list->{$key});
+        };
+        warn "Remove local LUN '$list->{$key}' failed: $@\n" if $@;
+    }
+};
 
-sub get_active_luns {
+my $get_active_luns = sub {
     my ($class, $storeid, $scfg, $volname) = @_;
 
     my $sid = 0;
     my $luns = {};
 
-    eval {
-        $sid = get_sid($scfg, $volname);
-    };
-    die "$@" if $@;
+    $sid = $get_sid->($scfg, $volname);
+
     if ($sid < 0) {
         # We have no active sessions so make one
-        eval {
-            $sid = create_session($scfg, $volname);
-        };
-        die "$@" if $@;
+        $sid = $create_session->($scfg, $volname);
         # Since no session existed prior to this call deactivate all LUN's found
-        deactivate_luns($scfg, $volname);
+        $deactivate_luns->($scfg, $volname);
     } else {
-        eval {
-            $luns = build_lun_list($scfg, $sid);
-        };
-        die "$@" if $@;
+        $luns = $build_lun_list->($scfg, $sid);
     }
 
     return $luns;
-}
+};
 
-sub rescan_session {
+my $rescan_session = sub {
     my ($class, $storeid, $scfg, $volname, $exclude_lun) = @_;
 
-    eval {
-        my $active_luns = get_active_luns($class, $storeid, $scfg, $volname);
-        delete $active_luns->{$exclude_lun} if defined $exclude_lun;
-        my $sid = get_sid($scfg, $volname);
-        die "Missing session" if $sid < 0;
-        os_request("iscsiadm -m session -r $sid -R", 0, 60);
-        deactivate_luns($scfg, $volname, $active_luns);
-        delete_session($scfg, $sid) if !%$active_luns;
-    };
-    die "$@" if $@;
-}
+    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;
+};
 
-sub freenas_get_latest_snapshot {
+my $freenas_get_latest_snapshot = sub {
     my ($class, $scfg, $volname) = @_;
 
-    my $vname = ($class->parse_volname($volname))[1];
+    my (undef, $vname) = $class->parse_volname($volname);
 
     # abort rollback if snapshot is not the latest
-    my $response = freenas_request($scfg, 'GET', "storage/snapshot");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $snapshots = decode_json($response);
+    my $snapshots = $freenas_request->($scfg, 'GET', "storage/snapshot/");
     
     my $recentsnap;
     foreach my $snapshot (@$snapshots) {
@@ -747,55 +849,8 @@ sub freenas_get_latest_snapshot {
     }
 
     return $recentsnap;
-}
+};
     
-# Configuration
-
-sub type {
-    return 'freenas';
-}
-
-sub plugindata {
-    return {
-        content => [ {images => 1, rootdir => 1}, {images => 1 , rootdir => 1} ],
-        format => [ { raw => 1 } , 'raw' ],
-    };
-}
-
-sub properties {
-    return {
-        password => {
-            description => "password",
-            type => "string",
-        },
-        portal_group => {
-            description => "Portal Group ID",
-            type => "integer",
-        },
-        initiator_group => {
-            description => "Initiator Group ID",
-            type => "integer",
-        },
-    };
-}
-
-sub options {
-    return {
-        portal => { fixed => 1 },
-        pool => { fixed => 1 },
-        portal_group => { fixed => 1 },
-        initiator_group => { fixed => 1 },
-        blocksize => { optional => 1 },
-        username => { optional => 1 },
-        password => { optional => 1 },
-#       sparse => { optional => 1 }, not available in 9.2.x. Appear in 11.x
-#                                    in 9.2.x all zvols are created sparse!
-        nodes => { optional => 1 },
-        disable => { optional => 1 },
-        content => { optional => 1 },
-    };
-}
-
 # Storage implementation
 
 sub volume_size_info {
@@ -803,9 +858,7 @@ sub volume_size_info {
 
     my (undef, $vname) = $class->parse_volname($volname);
 
-    my $response = freenas_request($scfg, 'GET', "storage/volume/$scfg->{pool}/zvols/$vname");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $zvol = decode_json($response);
+    my $zvol = $freenas_request->($scfg, 'GET', "storage/volume/$scfg->{pool}/zvols/$vname/");
     
     return $zvol->{volsize} if $zvol && $zvol->{volsize};
     
@@ -832,9 +885,7 @@ sub status {
     my $active = 0;
     
     eval {
-        my $response = freenas_request($scfg, 'GET', "storage/volume/$scfg->{pool}");
-        die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-        my $vol = decode_json($response);
+        my $vol = $freenas_request->($scfg, 'GET', "storage/volume/$scfg->{pool}/");
         my $children = $vol->{children};
         if (@$children) {
             $used = $children->[0]{used};
@@ -854,7 +905,7 @@ sub status {
 sub list_images {
     my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
 
-    $cache->{freenas} = freenas_list_zvol($scfg) if !$cache->{freenas};
+    $cache->{freenas} = $freenas_list_zvol->($scfg) unless $cache->{freenas};
     my $zfspool = $scfg->{pool};
     my $res = [];
 
@@ -875,7 +926,7 @@ sub list_images {
             
             if ($vollist) {
                 my $found = grep { $_ eq $info->{volid} } @$vollist;
-                next if !$found;
+                next unless $found;
             } else {
                 next if defined ($vmid) && ($owner ne $vmid);
             }
@@ -893,15 +944,14 @@ sub path {
 
     $vname = "$vname\@$snapname" if $snapname;
 
-    my $luns = get_active_luns($class, $storeid, $scfg, $vname);
-    my $path = disk_by_path($scfg, $vname);
+    my $luns = $get_active_luns->($class, $storeid, $scfg, $vname);
+    my $path = $disk_by_path->($scfg, $vname);
     
     return ($path, $vmid, $vtype);
 }
 
 sub create_base {
     my ($class, $storeid, $scfg, $volname) = @_;
-    my ($lun, $target);
     my $snap = '__base__';
 
     my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
@@ -911,22 +961,35 @@ sub create_base {
 
     my $newname = $name;
     $newname =~ s/^vm-/base-/;
+    # Check for existing base
+    my $base;
+    eval {
+        $base = $freenas_get_extent->($scfg, $newname);
+    };
+    warn "Check for existing base failed: $@\n" if $@;
+    die "$newname: Base already exists\n" if $base;
+    
+    my $target = $freenas_get_target->($scfg, $vmid);
+    die "create_base-> missing target\n" unless $target;
+    my $extent = $freenas_get_extent->($scfg, $name);
+    die "create_base-> missing extent\n" unless $extent;
+    my $tg2exent = $freenas_get_target_to_exent->($scfg, $extent, $target);
+    die "create_base-> missing target to extent\n" unless $tg2exent;
+    my $lun = $freenas_get_lun_number->($scfg, $name);
+    die "create_base-> missing LUN\n" unless defined $lun;
+
+    # if disable access to base vm fails bail
+    eval {
+        $freenas_delete_target_to_exent->($scfg, $tg2exent);
+        $freenas_delete_extent->($scfg, $extent);
+    };
+    die "Could not convert '$name' to '$newname': $@\n" if $@;
 
-    $target = freenas_get_target($scfg, $vmid);
-    die "create_base-> missing target" unless $target;
-    my $extent = freenas_get_extent($scfg, $name);
-    die "create_base-> missing extent" unless $extent;
-    my $tg2exent = freenas_get_target_to_exent($scfg, $extent, $target);
-    die "create_base-> missing target to extent" unless $tg2exent;
-    $lun = freenas_get_lun_number($scfg, $name);
-    die "create_base-> missing LUN" unless defined $lun;
-    freenas_delete_target_to_exent($scfg, $tg2exent);
-    freenas_delete_extent($scfg, $extent);
-    my $sid = get_sid($scfg, $name);
+    my $sid = $get_sid->($scfg, $name);
     if ($sid >= 0) {
-        my $lid = build_lun_list($scfg, $sid, $lun);
+        my $lid = $build_lun_list->($scfg, $sid, $lun);
         if ($lid && $lid->{$lun}) {
-            remove_local_lun($lid->{$lun});
+            $remove_local_lun->($lid->{$lun});
         }
     }
 
@@ -938,16 +1001,17 @@ sub create_base {
         my $data = {
             name => "$scfg->{pool}/$newname"
         };
-        my $response = freenas_request($scfg, 'POST', "storage/snapshot/$scfg->{pool}/$name\@$snap/clone/", encode_json($data));
-        die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
+        $freenas_request->(
+            $scfg, 'POST', "storage/snapshot/$scfg->{pool}/$name\@$snap/clone/", encode_json($data));
 
-        freenas_create_lun($scfg, $vmid, $newname);
+        $freenas_create_lun->($scfg, $vmid, $newname);
     };
     if ($@) {
-        $extent = freenas_create_extent($scfg, $name);
-        die "create_base-> Could not create extent for VM '$vmid'" unless $extent;
-        $tg2exent = freenas_create_target_to_exent($scfg, $target, $extent, $lun);
-        die "create_base-> Could not create target to extend for VM '$vmid'" unless defined $tg2exent;
+        # if creating base fails restore previous state
+        $extent = $freenas_create_extent->($scfg, $name);
+        die "create_base-> Could not create extent for VM '$vmid'\n" unless $extent;
+        $tg2exent = $freenas_create_target_to_exent->($scfg, $target, $extent, $lun);
+        die "create_base-> Could not create target to extend for VM '$vmid'\n" unless defined $tg2exent;
     }
 
     return $newname;
@@ -961,63 +1025,56 @@ sub clone_image {
     my ($vtype, $basename, $basevmid, undef, undef, $isBase, $format) =
         $class->parse_volname($volname);
 
-    die "clone_image only works on base images" if !$isBase;
+    die "clone_image only works on base images\n" unless $isBase;
 
-    my $run = PVE::QemuServer::check_running($basevmid);
-    if (!$run) {
-        $run = PVE::LXC::check_running($basevmid);
-    }
-    
-    my $name = freenas_find_free_diskname($storeid, $scfg, $vmid, $format);
+    my $name = $freenas_find_free_diskname->($storeid, $scfg, $vmid, $format);
 
     $class->volume_snapshot($scfg, $storeid, $basename, $snap);
     
     my $data = {
         name => "$scfg->{pool}/$name"
     };
-    my $response = freenas_request($scfg, 'POST', "storage/snapshot/$scfg->{pool}/$basename\@$snap/clone/", encode_json($data));
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
+    $freenas_request->(
+        $scfg, 'POST', "storage/snapshot/$scfg->{pool}/$basename\@$snap/clone/", encode_json($data));
 
     $name = "$basename/$name";
     # get ZFS dataset name from PVE volname
     my (undef, $clonedname) = $class->parse_volname($name);
 
-    freenas_create_lun($scfg, $vmid, $clonedname);
+    $freenas_create_lun->($scfg, $vmid, $clonedname);
     
-    my $res = $class->deactivate_volume($storeid, $scfg, $basename) unless $run;
-    warn "Could not deactivate volume '$basename'" unless $res;
+    my $res = $class->deactivate_volume($storeid, $scfg, $basename);
+    die "Could not deactivate volume '$basename'\n" unless $res;
     
     return $name;
 }
 
 sub alloc_image {
     my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
-    die "unsupported format '$fmt'" if $fmt ne 'raw';
+    die "unsupported format '$fmt'\n" if $fmt ne 'raw';
 
     die "illegal name '$name' - sould be 'vm-$vmid-*'\n"
     if $name && $name !~ m/^vm-$vmid-/;
 
-    my $volname = $name;
-
-    $volname = freenas_find_free_diskname($storeid, $scfg, $vmid, $fmt) if !$volname;
+    $name = $freenas_find_free_diskname->($storeid, $scfg, $vmid, $fmt) unless $name;
     
     # Size is in KB but Freenas wants in bytes
     $size *= 1024;
-    my $zvol = freenas_create_zvol($scfg, $volname, $size);
+    my $zvol = $freenas_create_zvol->($scfg, $name, $size);
 
     eval {
-        freenas_create_lun($scfg, $vmid, $zvol) if $zvol;
+        $freenas_create_lun->($scfg, $vmid, $zvol);
     };
     if ($@) {
         my $err = $@;
         eval {
-            freenas_delete_zvol($scfg, $volname);
+            $freenas_delete_zvol->($scfg, $name);
         };
-        $err .= "\n$@" if $@;
-        die $err;
+        warn "Cleanup failed: $@\n" if $@;
+        die "$err\n";
     }
 
-    return $volname;
+    return $name;
 }
 
 sub free_image {
@@ -1025,41 +1082,44 @@ sub free_image {
 
     my ($vtype, $name, $vmid, $basename) = $class->parse_volname($volname);
 
-    eval {
-        my $target = freenas_get_target($scfg, $vmid);
-        die "free_image-> missing target" unless $target;
-        my $extent = freenas_get_extent($scfg, $name);
-        die "free_image-> missing extent" unless $extent;
-        my $tg2exent = freenas_get_target_to_exent($scfg, $extent, $target);
-        die "free_image-> missing target to extent" unless $tg2exent;
-        my $target_group = freenas_get_target_group($scfg, $target);
-        die "free_image-> missing target group" unless $target_group;
-        my $lun = freenas_get_lun_number($scfg, $name);
-        die "free_image-> missing LUN" unless defined $lun;
+    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;
     
+    eval {
         my $res = $class->deactivate_volume($storeid, $scfg, $volname);
-        warn "Could not deactivate volume '$volname'" unless $res;
-        freenas_delete_target_to_exent($scfg, $tg2exent);
-        freenas_delete_extent($scfg, $extent);
-        if ($target && freenas_no_more_extents($scfg, $target)) {
+        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_group->($scfg, $target_group);
             }
-            freenas_delete_target($scfg, $target);
+            $freenas_delete_target->($scfg, $target);
         }
-        freenas_delete_zvol($scfg, $name);
+        $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);
+            $freenas_delete_zvol->($scfg, $basename);
         }
     };
     if ($@) {
         my $err = $@;
-        freenas_create_lun($scfg, $vmid, $name) unless $isBase;
-        die $err;
+        eval {
+            $freenas_create_lun->($scfg, $vmid, $name) unless $isBase;
+        };
+        warn "Recreate LUN failed: $@\n" if $@;
+        die "$err\n";
     }
 
     return undef;
@@ -1070,41 +1130,35 @@ sub volume_resize {
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $run = PVE::QemuServer::check_running($vmid);
-    if (!$run) {
-        $run = PVE::LXC::check_running($vmid);
-    }
-    
-    die 'mode failure - unable to resize disk(s) on a running system due to FreeNAS bug.<br />
-         See bug report: <a href="https://bugs.freenas.org/issues/24432" target="_blank">#24432</a><br />'      if $run;
+    die "mode failure - unable to resize disk(s) on a running system due to FreeNAS bug.\n
+         See bug report: https://bugs.freenas.org/issues/24432" if $running;
     
     my $data = {
         volsize => $size,
     };
-    my $response = freenas_request($scfg, 'PUT', "storage/volume/$scfg->{pool}/zvols/$name", encode_json($data));
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    my $vol = decode_json($response);
+    my $vol = $freenas_request->(
+        $scfg, 'PUT', "storage/volume/$scfg->{pool}/zvols/$name", encode_json($data));
 
-    my $sid = get_sid($scfg, $name);
+    my $sid = $get_sid->($scfg, $name);
     if ($sid >= 0) {
         eval {
 #### Required because of a bug in FreeNAS: https://bugs.freenas.org/issues/24432
-            my $targetname = freenas_get_target_name($scfg, $name);
-            die "volume_resize-> Missing target name" unless $targetname;
-            my $target = freenas_get_target($scfg, $vmid);
-            die "volume_resize-> Missing target" unless $target;
-            my $extent = freenas_get_extent($scfg, $name);
-            die "volume_resize-> Missing extent" unless $extent;
-            my $tg2exent = freenas_get_target_to_exent($scfg, $extent, $target);
-            die "volume_resize-> Missing target to extent" unless $tg2exent;
-            my $lunid = freenas_get_lun_number($scfg, $name);
-            die "volume_resize-> Missing LUN" unless defined $lunid;
-            freenas_delete_target_to_exent($scfg, $tg2exent);
-            freenas_create_target_to_exent($scfg, $target, $extent, $lunid);
+            my $targetname = $freenas_get_target_name->($scfg, $name);
+            die "volume_resize-> Missing target name\n" unless $targetname;
+            my $target = $freenas_get_target->($scfg, $vmid);
+            die "volume_resize-> Missing target\n" unless $target;
+            my $extent = $freenas_get_extent->($scfg, $name);
+            die "volume_resize-> Missing extent\n" unless $extent;
+            my $tg2exent = $freenas_get_target_to_exent->($scfg, $extent, $target);
+            die "volume_resize-> Missing target to extent\n" unless $tg2exent;
+            my $lunid = $freenas_get_lun_number->($scfg, $name);
+            die "volume_resize-> Missing LUN\n" unless defined $lunid;
+            $freenas_delete_target_to_exent->($scfg, $tg2exent);
+            $freenas_create_target_to_exent->($scfg, $target, $extent, $lunid);
 #### Required because of a bug in FreeNAS: https://bugs.freenas.org/issues/24432
-            rescan_session($class, $storeid, $scfg, $name, $lunid);
+            $rescan_session->($class, $storeid, $scfg, $name, $lunid);
         };
-        die "$name: Resize with $size failed. ($@)\n" if $@;
+        die "$name: Resize with $size failed or could not export new size. ($@)\n" if $@;
     }
 
     return int($vol->{volsize}/1024);
@@ -1119,8 +1173,7 @@ sub volume_snapshot {
         dataset => "$scfg->{pool}/$vname",
         name => $snap,
     };
-    my $response = freenas_request($scfg, 'POST', "storage/snapshot/", encode_json($data));
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
+    $freenas_request->($scfg, 'POST', "storage/snapshot/", encode_json($data));
 }
 
 sub volume_snapshot_delete {
@@ -1130,49 +1183,55 @@ sub volume_snapshot_delete {
 
     if ($snap eq 'vzdump') {
         eval {
-            my $target = freenas_get_target($scfg, $vmid);
-            die "volume_snapshot_delete-> missing target" unless $target;
-            my $extent = freenas_get_extent($scfg, "$vname\@$snap");
-            die "volume_snapshot_delete-> missing extent" unless $extent;
-            my $tg2exent = freenas_get_target_to_exent($scfg, $extent, $target);
-            die "volume_snapshot_delete-> missing target to extent" unless $tg2exent;
-            my $lun = freenas_get_lun_number($scfg, "$vname\@$snap");
-            die "volume_snapshot_delete-> missing LUN" unless defined $lun;
-            freenas_delete_target_to_exent($scfg, $tg2exent);
-            freenas_delete_extent($scfg, $extent);
+            my $target = $freenas_get_target->($scfg, $vmid);
+            die "volume_snapshot_delete-> missing target\n" unless $target;
+            my $extent = $freenas_get_extent->($scfg, "$vname\@$snap");
+            die "volume_snapshot_delete-> missing extent\n" unless $extent;
+            my $tg2exent = $freenas_get_target_to_exent->($scfg, $extent, $target);
+            die "volume_snapshot_delete-> missing target to extent\n" unless $tg2exent;
+            my $lun = $freenas_get_lun_number->($scfg, "$vname\@$snap");
+            die "volume_snapshot_delete-> missing LUN\n" unless defined $lun;
+            $freenas_delete_target_to_exent->($scfg, $tg2exent);
+            $freenas_delete_extent->($scfg, $extent);
             $class->deactivate_volume($storeid, $scfg, "$vname\@$snap");
         };
         warn "$@ - unable to deactivate snapshot from remote FreeNAS storage" if $@;
     }
 
-    my $response = freenas_request($scfg, 'DELETE', "storage/snapshot/$scfg->{pool}/$vname\@$snap");
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
+    $freenas_request->($scfg, 'DELETE', "storage/snapshot/$scfg->{pool}/$vname\@$snap/");
 }
 
 sub volume_snapshot_rollback {
     my ($class, $scfg, $storeid, $volname, $snap) = @_;
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
-    my $target = freenas_get_target($scfg, $vmid);
-    die "volume_resize-> Missing target" unless $target;
-    my $extent = freenas_get_extent($scfg, $name);
-    die "volume_resize-> Missing extent" unless $extent;
-    my $tg2exent = freenas_get_target_to_exent($scfg, $extent, $target);
-    die "volume_resize-> Missing target to extent" unless $tg2exent;
-    my $lunid = freenas_get_lun_number($scfg, $name);
-    die "volume_resize-> Missing LUN" unless defined $lunid;
-    freenas_delete_target_to_exent($scfg, $tg2exent);
-    freenas_delete_extent($scfg, $extent);
+    my $target = $freenas_get_target->($scfg, $vmid);
+    die "volume_resize-> Missing target\n" unless $target;
+    my $extent = $freenas_get_extent->($scfg, $name);
+    die "volume_resize-> Missing extent\n" unless $extent;
+    my $tg2exent = $freenas_get_target_to_exent->($scfg, $extent, $target);
+    die "volume_resize-> Missing target to extent\n" unless $tg2exent;
+    my $lunid = $freenas_get_lun_number->($scfg, $name);
+    die "volume_resize-> Missing LUN\n" unless defined $lunid;
+
+    eval {
+        $freenas_delete_target_to_exent->($scfg, $tg2exent);
+        $freenas_delete_extent->($scfg, $extent);
+    };
+    warn "Failed to remove current extent. Trying to proceed anyway: $@\n" if $@;
     
     my $data = {
         force => bless( do{\(my $o = 0)}, 'JSON::XS::Boolean' ),
     };
-    my $response = freenas_request($scfg, 'POST', "storage/snapshot/$scfg->{pool}/$name\@$snap/rollback/", encode_json($data));
-    die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
-    
-    $extent = freenas_create_extent($scfg, $name);
-    freenas_create_target_to_exent($scfg, $target, $extent, $lunid);
-    rescan_session($class, $storeid, $scfg, $name, $lunid);
+    $freenas_request->(
+        $scfg, 'POST', "storage/snapshot/$scfg->{pool}/$name\@$snap/rollback/", encode_json($data));
+
+    eval {
+        $extent = $freenas_create_extent->($scfg, $name);
+        $freenas_create_target_to_exent->($scfg, $target, $extent, $lunid);
+        $rescan_session->($class, $storeid, $scfg, $name, $lunid);
+    };
+    die "Rollback failed: $@\n" if $@;
 }
 
 sub volume_rollback_is_possible {
@@ -1180,9 +1239,9 @@ sub volume_rollback_is_possible {
     
     my (undef, $name) = $class->parse_volname($volname);
 
-    my $recentsnap = $class->freenas_get_latest_snapshot($scfg, $name);
+    my $recentsnap = $class->freenas_get_latest_snapshot->($scfg, $name);
     if ($snap ne $recentsnap) {
-        die "can't rollback, more recent snapshots exist";
+        die "can't rollback, more recent snapshots exist\n";
     }
 
     return 1; 
@@ -1248,32 +1307,31 @@ sub activate_volume {
     
     my (undef, $name, $vmid) = $class->parse_volname($volname);
     
-    my $active_luns = get_active_luns($class, $storeid, $scfg, $name);
+    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");
-            $active_luns->{$lun} = "0:0:0:$lun";
+            $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";
+            die "$@ - unable to activate snapshot from remote FreeNAS storage\n";
         }
     }
     
-    $lun = freenas_get_lun_number($scfg, $name);
-    $active_luns->{$lun} = "0:0:0:$lun";
+    $lun = $freenas_get_lun_number->($scfg, $name);
+    $luns_to_keep->{$lun} = "0:0:0:$lun";
 
-    eval {
-        my $sid = get_sid($scfg, $name);
-        die "activate_volume-> Missing session" if $sid < 0;
-        # Add new LUN's to session
-        os_request("iscsiadm -m session -r $sid -R", 0, 60);
-        sleep 1;
-        # Remove all LUN's from session which is not currently active
-        deactivate_luns($scfg, $name, $active_luns);
-    };
-    die "$@" if $@;
+    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;
 }
@@ -1285,21 +1343,18 @@ sub activate_volume {
 #   deactivate lun
 sub deactivate_volume {
     my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
-    
+
     my $name = ($class->parse_volname($volname))[1];
 
-    my $active_luns = get_active_luns($class, $storeid, $scfg, $name);
+    my $luns_to_keep = $get_active_luns->($class, $storeid, $scfg, $name);
 
-    my $lun = freenas_get_lun_number($scfg, $name);
-    delete $active_luns->{$lun};
+    my $lun = $freenas_get_lun_number->($scfg, $name);
+    delete $luns_to_keep->{$lun};
 
-    eval {
-        my $sid = get_sid($scfg, $name);
-        die "deactivate_volume-> Missing session" if $sid < 0;
-        deactivate_luns($scfg, $name, $active_luns);
-        delete_session($scfg, $sid) if !%$active_luns;
-    };
-    die $@ if $@;
+    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;
 }
-- 
2.11.0


----

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




More information about the pve-devel mailing list