From: Max Carrara <m.carrara@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [RFC pve-storage 36/36] plugin: zfspool: refactor method `zfs_request` into helper subroutine
Date: Wed, 17 Jul 2024 11:40:34 +0200 [thread overview]
Message-ID: <20240717094034.124857-37-m.carrara@proxmox.com> (raw)
In-Reply-To: <20240717094034.124857-1-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
prev parent reply other threads:[~2024-07-17 9:44 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-17 9:39 [pve-devel] [RFC pve-storage 00/36] Refactor / Cleanup of Storage Plugins Max Carrara
2024-07-17 9:39 ` [pve-devel] [RFC pve-storage 01/36] plugin: base: remove old fixme comments Max Carrara
2024-07-17 16:02 ` Thomas Lamprecht
2024-07-18 7:43 ` Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 02/36] plugin: btrfs: make plugin-specific helpers private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 03/36] plugin: cephfs: " Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 04/36] api: remove unused import of CIFS storage plugin Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 05/36] plugin: cifs: make plugin-specific helpers private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 06/36] api: remove unused import of LVM storage plugin Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 07/36] common: introduce common module Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 08/36] plugin: dir: move helper subs of directory plugin to common modules Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 09/36] plugin: lvm: move LVM helper subroutines into separate common module Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 10/36] api: replace usages of deprecated LVM helper subs with new ones Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 11/36] plugin: lvmthin: replace usages of deprecated LVM helpers " Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 12/36] plugin: lvmthin: move helper that lists thinpools to common LVM module Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 13/36] common: lvm: update code style Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 14/36] api: replace usages of deprecated LVM thin pool helper sub Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 15/36] plugin: btrfs: replace deprecated helpers from directory plugin Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 16/36] plugin: dir: factor storage methods into separate common subs Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 17/36] plugin: dir: factor path validity check into helper methods Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 18/36] plugin: btrfs: remove dependency on directory plugin Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 19/36] plugin: cifs: " Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 20/36] plugin: cephfs: " Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 21/36] plugin: nfs: " Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 22/36] plugin: btrfs: make helper methods private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 23/36] plugin: esxi: make helper subroutines private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 24/36] plugin: esxi: remove unused helper subroutine `query_vmdk_size` Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 25/36] plugin: esxi: make helper methods private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 26/36] plugin: gluster: make helper subroutines private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 27/36] plugin: iscsi-direct: make helper subroutine `iscsi_ls` private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 28/36] plugin: iscsi: factor helper functions into common module Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 29/36] plugin: iscsi: make helper subroutine `iscsi_session` private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 30/36] plugin: lvm: update definition of subroutine `check_tags` Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 31/36] plugin: lvmthin: update definition of subroutine `activate_lv` Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 32/36] plugin: nfs: make helper subroutines private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 33/36] plugin: rbd: update private sub signatures and make helpers private Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 34/36] common: zfs: introduce module for common ZFS helpers Max Carrara
2024-07-17 9:40 ` [pve-devel] [RFC pve-storage 35/36] plugin: zfspool: move helper `zfs_parse_zvol_list` to common module Max Carrara
2024-07-17 9:40 ` Max Carrara [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240717094034.124857-37-m.carrara@proxmox.com \
--to=m.carrara@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox