[pve-devel] [RFC pve-storage 36/36] plugin: zfspool: refactor method `zfs_request` into helper subroutine

[Max Carrara <m.carrara@proxmox.com>]

In order to separate the invocation of ZFS CLI utlis from the plugin,
the `zfs_request` method is refactored into a helper subroutine and
placed in the common ZFS module.

The signature is changed, removing the `$class` parameter. The body
remains the same, so no behaviour is actually altered.

The new helper sub is also documented and given a prototype.

The original method is changed to wrap the new helper and also emit a
deprecation warning when used.

The sites where the original method is called are updated to use the
helper subroutine instead.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common/ZFS.pm    |  84 +++++++++++++++++++++++
 src/PVE/Storage/ZFSPoolPlugin.pm | 112 ++++++++++++++++++-------------
 2 files changed, 150 insertions(+), 46 deletions(-)

diff --git a/src/PVE/Storage/Common/ZFS.pm b/src/PVE/Storage/Common/ZFS.pm
index 21b28d9..327a592 100644
--- a/src/PVE/Storage/Common/ZFS.pm
+++ b/src/PVE/Storage/Common/ZFS.pm
@@ -3,10 +3,16 @@ package PVE::Storage::Common::ZFS;
 use strict;
 use warnings;
 
+use PVE::RPCEnvironment;
+use PVE::Tools qw(
+    run_command
+);
+
 use parent qw(Exporter);
 
 our @EXPORT_OK = qw(
     zfs_parse_zvol_list
+    zfs_request
 );
 
 =pod
@@ -21,6 +27,84 @@ PVE::Storage::Common::ZFS - Shared ZFS utilities and command line wrappers
 
 =pod
 
+=head3 zfs_request
+
+    $output = zfs_request($scfg, $timeout, $method, @params)
+
+Wrapper that runs a ZFS command and returns its output.
+
+This subroutine has some special handling depending on which arguments are
+passed. See the description of the individual parameters below.
+
+=over
+
+=item C<$scfg>
+
+A section config hash. Unused and kept for backward compatibility.
+
+=item C<$timeout>
+
+Optional. The number of seconds to elapse before the wrapped command times out.
+
+If not provided, the invocation will time out after a default of C<10> seconds.
+
+If C<"zpool_import"> is passed as C<$method>, the timeout instead has a minimum
+of C<15> seconds and will automatically be increased if it is below.
+
+B<NOTE>: Should this subroutine be invoked in the context of an I<RPC> or
+I<REST> worker, the above is disregarded and the timeout has a minimum of
+B<five minutes>, automatically increasing it if it is below. Should no timeout
+be provided in this case, the default is B<one hour> instead.
+
+=item C<$method>
+
+The subcommand of the C<zfs> CLI to run. This can be something like C<"get">
+(C<zfs get>), C<"list"> (C<zfs list>), etc.
+
+There are two exceptions, however: If C<$method> is C<"zpool_list"> or
+C<"zpool_import">, C<zpool list> or C<zpool import> will respectively be called
+instead.
+
+=item C<@params>
+
+Optional. Further parameters to pass along to the C<zfs> or C<zpool> CLI.
+
+=back
+
+=cut
+
+sub zfs_request : prototype($$$;@) {
+    my ($scfg, $timeout, $method, @params) = @_;
+
+    my $cmd = [];
+
+    if ($method eq 'zpool_list') {
+	push @$cmd, 'zpool', 'list';
+    } elsif ($method eq 'zpool_import') {
+	push @$cmd, 'zpool', 'import';
+	$timeout = 15 if !$timeout || $timeout < 15;
+    } else {
+	push @$cmd, 'zfs', $method;
+    }
+    push @$cmd, @params;
+
+    my $msg = '';
+    my $output = sub { $msg .= "$_[0]\n" };
+
+    if (PVE::RPCEnvironment->is_worker()) {
+	$timeout = 60*60 if !$timeout;
+	$timeout = 60*5 if $timeout < 60*5;
+    } else {
+	$timeout = 10 if !$timeout;
+    }
+
+    run_command($cmd, errmsg => "zfs error", outfunc => $output, timeout => $timeout);
+
+    return $msg;
+}
+
+=pod
+
 =head3 zfs_parse_zvol_list
 
     $zvol_list = zfs_parse_zvol_list($text, $pool)
diff --git a/src/PVE/Storage/ZFSPoolPlugin.pm b/src/PVE/Storage/ZFSPoolPlugin.pm
index fdfedca..c666166 100644
--- a/src/PVE/Storage/ZFSPoolPlugin.pm
+++ b/src/PVE/Storage/ZFSPoolPlugin.pm
@@ -132,31 +132,13 @@ sub path {
 sub zfs_request {
     my ($class, $scfg, $timeout, $method, @params) = @_;
 
-    my $cmd = [];
-
-    if ($method eq 'zpool_list') {
-	push @$cmd, 'zpool', 'list';
-    } elsif ($method eq 'zpool_import') {
-	push @$cmd, 'zpool', 'import';
-	$timeout = 15 if !$timeout || $timeout < 15;
-    } else {
-	push @$cmd, 'zfs', $method;
-    }
-    push @$cmd, @params;
-
-    my $msg = '';
-    my $output = sub { $msg .= "$_[0]\n" };
-
-    if (PVE::RPCEnvironment->is_worker()) {
-	$timeout = 60*60 if !$timeout;
-	$timeout = 60*5 if $timeout < 60*5;
-    } else {
-	$timeout = 10 if !$timeout;
-    }
-
-    run_command($cmd, errmsg => "zfs error", outfunc => $output, timeout => $timeout);
+    warn get_deprecation_warning(
+	"PVE::Storage::Common::ZFS::zfs_request"
+    );
 
-    return $msg;
+    return PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, $timeout, $method, @params
+    );
 }
 
 sub zfs_wait_for_zvol_link {
@@ -254,8 +236,9 @@ sub list_images {
 sub zfs_get_properties {
     my ($class, $scfg, $properties, $dataset, $timeout) = @_;
 
-    my $result = $class->zfs_request($scfg, $timeout, 'get', '-o', 'value',
-				     '-Hp', $properties, $dataset);
+    my $result = PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, $timeout, 'get', '-o', 'value', '-Hp', $properties, $dataset
+    );
     my @values = split /\n/, $result;
     return wantarray ? @values : $values[0];
 }
@@ -295,7 +278,7 @@ sub zfs_create_zvol {
 
     push @$cmd, '-V', "${size}k", "$scfg->{pool}/$zvol";
 
-    $class->zfs_request($scfg, undef, @$cmd);
+    PVE::Storage::Common::ZFS::zfs_request($scfg, undef, @$cmd);
 }
 
 sub zfs_create_subvol {
@@ -307,7 +290,7 @@ sub zfs_create_subvol {
     my $cmd = ['create', '-o', 'acltype=posixacl', '-o', 'xattr=sa',
 	       '-o', "refquota=${quota}", $dataset];
 
-    $class->zfs_request($scfg, undef, @$cmd);
+    PVE::Storage::Common::ZFS::zfs_request($scfg, undef, @$cmd);
 }
 
 sub zfs_delete_zvol {
@@ -317,7 +300,9 @@ sub zfs_delete_zvol {
 
     for (my $i = 0; $i < 6; $i++) {
 
-	eval { $class->zfs_request($scfg, undef, 'destroy', '-r', "$scfg->{pool}/$zvol"); };
+	eval { PVE::Storage::Common::ZFS::zfs_request(
+		$scfg, undef, 'destroy', '-r', "$scfg->{pool}/$zvol");
+	};
 	if ($err = $@) {
 	    if ($err =~ m/^zfs error:(.*): dataset is busy.*/) {
 		sleep(1);
@@ -338,7 +323,7 @@ sub zfs_delete_zvol {
 sub zfs_list_zvol {
     my ($class, $scfg) = @_;
 
-    my $text = $class->zfs_request(
+    my $text = PVE::Storage::Common::ZFS::zfs_request(
 	$scfg,
 	10,
 	'list',
@@ -383,7 +368,7 @@ sub zfs_get_sorted_snapshot_list {
     my $vname = ($class->parse_volname($volname))[1];
     push @params, "$scfg->{pool}\/$vname";
 
-    my $text = $class->zfs_request($scfg, undef, 'list', @params);
+    my $text = PVE::Storage::Common::ZFS::zfs_request($scfg, undef, 'list', @params);
     my @snapshots = split(/\n/, $text);
 
     my $snap_names = [];
@@ -435,7 +420,9 @@ sub volume_snapshot {
 
     my $vname = ($class->parse_volname($volname))[1];
 
-    $class->zfs_request($scfg, undef, 'snapshot', "$scfg->{pool}/$vname\@$snap");
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'snapshot', "$scfg->{pool}/$vname\@$snap"
+    );
 }
 
 sub volume_snapshot_delete {
@@ -444,7 +431,9 @@ sub volume_snapshot_delete {
     my $vname = ($class->parse_volname($volname))[1];
 
     $class->deactivate_volume($storeid, $scfg, $vname, $snap, {});
-    $class->zfs_request($scfg, undef, 'destroy', "$scfg->{pool}/$vname\@$snap");
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'destroy', "$scfg->{pool}/$vname\@$snap"
+    );
 }
 
 sub volume_snapshot_rollback {
@@ -452,13 +441,19 @@ sub volume_snapshot_rollback {
 
     my (undef, $vname, undef, undef, undef, undef, $format) = $class->parse_volname($volname);
 
-    my $msg = $class->zfs_request($scfg, undef, 'rollback', "$scfg->{pool}/$vname\@$snap");
+    my $msg = PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'rollback', "$scfg->{pool}/$vname\@$snap"
+    );
 
     # we have to unmount rollbacked subvols, to invalidate wrong kernel
     # caches, they get mounted in activate volume again
     # see zfs bug #10931 https://github.com/openzfs/zfs/issues/10931
     if ($format eq 'subvol') {
-	eval { $class->zfs_request($scfg, undef, 'unmount', "$scfg->{pool}/$vname"); };
+	eval {
+	    PVE::Storage::Common::ZFS::zfs_request(
+		$scfg, undef, 'unmount', "$scfg->{pool}/$vname"
+	    );
+	};
 	if (my $err = $@) {
 	    die $err if $err !~ m/not currently mounted$/;
 	}
@@ -503,7 +498,7 @@ sub volume_snapshot_info {
     my $vname = ($class->parse_volname($volname))[1];
     push @params, "$scfg->{pool}\/$vname";
 
-    my $text = $class->zfs_request($scfg, undef, 'list', @params);
+    my $text = PVE::Storage::Common::ZFS::zfs_request($scfg, undef, 'list', @params);
     my @lines = split(/\n/, $text);
 
     my $info = {};
@@ -545,7 +540,11 @@ sub activate_storage {
 
     my $pool_imported = sub {
 	my @param = ('-o', 'name', '-H', $pool);
-	my $res = eval { $class->zfs_request($scfg, undef, 'zpool_list', @param) };
+	my $res = eval {
+	    PVE::Storage::Common::ZFS::zfs_request(
+		$scfg, undef, 'zpool_list', @param
+	    )
+	};
 	warn "$@\n" if $@;
 
 	return defined($res) && $res =~ m/$pool/;
@@ -554,13 +553,13 @@ sub activate_storage {
     if (!$pool_imported->()) {
 	# import can only be done if not yet imported!
 	my @param = ('-d', '/dev/disk/by-id/', '-o', 'cachefile=none', $pool);
-	eval { $class->zfs_request($scfg, undef, 'zpool_import', @param) };
+	eval { PVE::Storage::Common::ZFS::zfs_request($scfg, undef, 'zpool_import', @param) };
 	if (my $err = $@) {
 	    # just could've raced with another import, so recheck if it is imported
 	    die "could not activate storage '$storeid', $err\n" if !$pool_imported->();
 	}
     }
-    eval { $class->zfs_request($scfg, undef, 'mount', '-a') };
+    eval { PVE::Storage::Common::ZFS::zfs_request($scfg, undef, 'mount', '-a') };
     die "could not activate storage '$storeid', $@\n" if $@;
     return 1;
 }
@@ -582,7 +581,9 @@ sub activate_volume {
     } elsif ($format eq 'subvol') {
 	my $mounted = $class->zfs_get_properties($scfg, 'mounted', "$scfg->{pool}/$dataset");
 	if ($mounted !~ m/^yes$/) {
-	    $class->zfs_request($scfg, undef, 'mount', "$scfg->{pool}/$dataset");
+	    PVE::Storage::Common::ZFS::zfs_request(
+		$scfg, undef, 'mount', "$scfg->{pool}/$dataset"
+	    );
 	}
     }
 
@@ -607,11 +608,24 @@ sub clone_image {
     my $name = $class->find_free_diskname($storeid, $scfg, $vmid, $format);
 
     if ($format eq 'subvol') {
-	my $size = $class->zfs_request($scfg, undef, 'list', '-Hp', '-o', 'refquota', "$scfg->{pool}/$basename");
+	my $size = PVE::Storage::Common::ZFS::zfs_request(
+	    $scfg, undef, 'list', '-Hp', '-o', 'refquota', "$scfg->{pool}/$basename"
+	);
+
 	chomp($size);
-	$class->zfs_request($scfg, undef, 'clone', "$scfg->{pool}/$basename\@$snap", "$scfg->{pool}/$name", '-o', "refquota=$size");
+
+	PVE::Storage::Common::ZFS::zfs_request(
+	    $scfg,
+	    undef,
+	    'clone',
+	    "$scfg->{pool}/$basename\@$snap",
+	    "$scfg->{pool}/$name",
+	    '-o', "refquota=$size"
+	);
     } else {
-	$class->zfs_request($scfg, undef, 'clone', "$scfg->{pool}/$basename\@$snap", "$scfg->{pool}/$name");
+	PVE::Storage::Common::ZFS::zfs_request(
+	    $scfg, undef, 'clone', "$scfg->{pool}/$basename\@$snap", "$scfg->{pool}/$name"
+	);
     }
 
     return "$basename/$name";
@@ -635,7 +649,9 @@ sub create_base {
     }
     my $newvolname = $basename ? "$basename/$newname" : "$newname";
 
-    $class->zfs_request($scfg, undef, 'rename', "$scfg->{pool}/$name", "$scfg->{pool}/$newname");
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'rename', "$scfg->{pool}/$name", "$scfg->{pool}/$newname"
+    );
 
     my $running  = undef; #fixme : is create_base always offline ?
 
@@ -660,7 +676,9 @@ sub volume_resize {
 	$new_size = $new_size + $padding;
     }
 
-    $class->zfs_request($scfg, undef, 'set', "$attr=${new_size}k", "$scfg->{pool}/$vname");
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, undef, 'set', "$attr=${new_size}k", "$scfg->{pool}/$vname"
+    );
 
     return $new_size;
 }
@@ -811,7 +829,9 @@ sub rename_volume {
 				  noerr => 1, quiet => 1);
     die "target volume '${target_volname}' already exists\n" if $exists;
 
-    $class->zfs_request($scfg, 5, 'rename', ${source_zfspath}, ${target_zfspath});
+    PVE::Storage::Common::ZFS::zfs_request(
+	$scfg, 5, 'rename', ${source_zfspath}, ${target_zfspath}
+    );
 
     $base_name = $base_name ? "${base_name}/" : '';
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel