[pve-devel] [RFC pve-storage 36/36] plugin: zfspool: refactor method `zfs_request` into helper subroutine
Max Carrara
m.carrara at proxmox.com
Wed Jul 17 11:40:34 CEST 2024
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 at 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
More information about the pve-devel
mailing list