[pve-devel] [PATCH v6 pve-storage 6/6] Bug fixes and clean-ups in response to review from Fabian.

mir at datanom.net mir at datanom.net
Mon Jun 19 17:13:24 CEST 2017


From: Michael Rasmussen <mir at datanom.net>

Changes since v5:
* if ! to unless
* all helper methods now private
* remove unused includes
* remove unused code
* more descriptive variable names
* change api timeout to be in sync with the PVE API
* add loop over limit until empty resultset is returned
* fix API version check
* fix handling 409 code in create_target and create_target_group
* Remove unnecessary error handling
* Replace hardcoded max luns number with variable
* Fix error handling in freenas_create_lun method
* Write directly to file instead of using a shell to echo to file
* Fix error handling in deactivate_lun and get_active_luns
* Improve code readability and error handling in rescan_session
* Declaring variables where used in create_base
* Check if base already exists when creating base
* Improve error handling in create_base
* Fix clone_image error handling and remove unnecessary check for running base
* Remove unused variable in alloc_image
* More robust error handling in free_image
* Remove HTML code from volume_resize
* Fix error handling in shapshot rollback
* Improve code readability and error handling in activate_lun
* Improve code readability and error handling in deactivate_lun
* Replace sleep 1 with a combination of udevadm trigger udevadm settle
* Remove check of running VM and CT. relaying on parsed option running for
  Qemu and handle running LXC in PVE::API2::LXC now sends running status
  as part of call to volume_resize when storage id is freenas.

Changes since v4:
* regression and indentation fix

Changes since v3:
* Adds support for creating snapshot backups of LXC containers.
  This more or less makes the plugin feature complete.
  Only outstanding feature depends on a bug fix in the FreeNAS API
  which is first scheduled for next stable release:
  https://bugs.freenas.org/issues/23881 (Targeted for 11.1)

This patch series adds a storage plugin for FreeNAS using the FreeNAS API.
The plugin supports both Qemu and LXC VM. Features supported, see below:

                    VM          CT
create              YES         YES
delete              YES         YES
resize              (YES)       (YES) Note 1
snapshot
   offline          YES         YES
   live             YES         "YES" (state is not saved)
backup
   snapshot         YES         YES
   standby          YES         YES
   offline          YES         YES
clone
   full             YES         YES (Unavailable in GUI)
   linked           YES         YES (Unavailable in GUI)

Note 1: Due to a bug in the FreeNAS API live resizing has been disabled
in the plugin. See https://bugs.freenas.org/issues/24432

Since the plugin attaches disks through the local scsi subsystem via
openiscsi and therefore not uses libiscsi there should be support for
MPIO and authentication. MPIO is not tested since I don't have the
required hardware to do so. Authentication is not enabled but should be
easy to do if the requirements exists.

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