From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id E3D2D1FF2C8 for ; Wed, 17 Jul 2024 11:44:12 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AF0A438E6D; Wed, 17 Jul 2024 11:44:13 +0200 (CEST) From: Max Carrara To: pve-devel@lists.proxmox.com Date: Wed, 17 Jul 2024 11:40:34 +0200 Message-Id: <20240717094034.124857-37-m.carrara@proxmox.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240717094034.124857-1-m.carrara@proxmox.com> References: <20240717094034.124857-1-m.carrara@proxmox.com> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.030 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pve-devel] [RFC pve-storage 36/36] plugin: zfspool: refactor method `zfs_request` into helper subroutine X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 --- 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: Should this subroutine be invoked in the context of an I or +I worker, the above is disregarded and the timeout has a minimum of +B, automatically increasing it if it is below. Should no timeout +be provided in this case, the default is B instead. + +=item C<$method> + +The subcommand of the C CLI to run. This can be something like C<"get"> +(C), C<"list"> (C), etc. + +There are two exceptions, however: If C<$method> is C<"zpool_list"> or +C<"zpool_import">, C or C will respectively be called +instead. + +=item C<@params> + +Optional. Further parameters to pass along to the C or C 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