[pve-devel] [PATCH pve-storage 0/5] v5 fix regression and indentation
Fabian Grünbichler
f.gruenbichler at proxmox.com
Fri Jun 16 14:21:34 CEST 2017
On Fri, Jun 16, 2017 at 03:10:27AM +0200, Michael Rasmussen wrote:
> v5 regression and indentation fix
> V4 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)
Because of the nature of this patch series and the way you sent the
patches, I'll give you some broad feedback in one go for the newly
introduced files, and not for the individual patches.
If/When you send a v6, please follow the following guide lines (to make
both reviewing and incorporating the reviews easier):
- don't change the cover letter subject when sending new versions of a
patch series, only bump the version (e.g., send your patch series as
"add FreeNAS storage plugin" "v2 add FreeNAS storage plugin" etc -
some of us rely on this to filter patch series, and every mail
deviating from this generates extra effort for manual processing)
- don't add new patches fixing stuff in older patches, but split your
changes in a logical manner instead of chronologically
the latter means, instead of doing
- big patch introducing plugin
- fix issue one in previous commit
- fix issue two in first commit
- fix issue three in second commit
- ...
- fix issue seventeen in commit fourteen which attempted to fix issue
eight in commit five which attempted to fix issue four in commit four
which attempted to fix issue three in commit two which attempted to
fix issue one in huge first commit
you should do
- add plugin file with name and options
- add FreeNAS API access code
- add OS iSCSI interaction code
- implement foo
- implement bar
- add regression tests
- ...
otherwise, I/we have to review huge changesets over and over, which is a
PITA ;) bonus points for marking which patches have changed and which
haven't, and giving a short summary of the changes between versions
broken down by individual patches.
e.g., if you get a review saying "there is issue XYZ with your
implementation of foo", you just need to update the (hopefully small)
commit which introduced this issue. the usual way to do this is to have
a branch for v1, and then when you get reviews you can check it out as
v2, and either use fixup commits[1] or an editing rebase workflow[2] to
incorporate changes. once you are happy with your v2, you can easily
diff it with v1 (both are separate branches), generate a new patch
series and re-send it as v2.
1: "git commit --fixup COMMITYOUWANTTOFIX; git rebase -i --autosquash FIRSTCOMMITOFYOURPATCHSERIES^"
2: "git rebase -i FIRSTCOMMITOFYOURPATCHSERIES^" and following the instructions ;)
but of course, any other work flow that works for you is okay on your
side, as long as the end result fits the criteria ;) note that it is
okay to add new patches in a new version of a patch series if it makes
sense (e.g., because between version x and y a new feature was added
that warrants to get its own patch/commit, or because you split a bigger
patch into two or more smaller ones).
keep in mind that the end result of the iterative code and review
process should be a patch series that can be applied, and not a change
log of how the final version of the code was iteratively improved.
>
> 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.
>
> Michael Rasmussen (5):
> First beta of FreeNAS storage plugin. Missing snapshot backup for LXC
> Proper check if VM/CT is running
> Fix missing $vmid
> Add support for creating LXC snapshot backup
> Fix regression and indentation
>
> PVE/Storage.pm | 2 +
> PVE/Storage/FreeNASPlugin.pm | 1307 ++++++++++++++++++++++++++++++++++++++++++
> PVE/Storage/Makefile | 2 +-
> PVE/Storage/Plugin.pm | 2 +-
> 4 files changed, 1311 insertions(+), 2 deletions(-)
> create mode 100644 PVE/Storage/FreeNASPlugin.pm
>
general remark: indentation is wrong in a lot of places. I know we have
a slightly strange indentation style, but please adhere to it when
introducing new code (or touching existing stuff).
see https://pve.proxmox.com/wiki/Perl_Style_Guide for details.
TL;DR: indent with 4 spaces, and convert every 8 of those into a tab
another style point is the use of "unless" instead of "if !" - we prefer
the latter (almost all the other code in pve-storage that uses "unless"
was also written by you ;))
I would recommend making some of the helper methods private, e.g.:
my $some_helper = sub {
...
};
instead of
sub some_helper {
...
}
this makes it easier to separate public interface and low level
implementation. also, putting type, options, and the rest of the
# Configuration block first helps when quickly checking a plugin's code.
I haven't tested the plugin in any form, this is just from a rough
reading of the code from top to bottom, and then once more to correct
stuff I misunderstood in the first go.
one big point that I haven't really looked at is whether you need to
guard some operations with locks. especially the calls to iscsiadm are
triggered in a lot of code paths, and I don't know whether those are
problematic when executed concurrently or not. some of the higher level
stuff is already guarded with cluster_lock_storage one layer above the
plugins, in PVE::Storage (basically everything that adds or removes a
volume or needs an invariant volume list):
- volume_is_base_and_used
- vdisk_clone
- vdisk_create_base
- vdisk_alloc
- vdisk_free
also haven't checked for how it would handle an already existing local
(open-)iscsi setup.. did you?
so what follows is the current FreeNASPlugin.pm, with all 5 of your
patches applied. comments are inline (lines not starting with '> ').
I hope you are not discouraged by the amount of comments - but it's a
lot of code in one big drop ;)
> package PVE::Storage::FreeNASPlugin;
>
> use strict;
> use warnings;
> use IO::File;
is this used anywhere?
> use POSIX;
> use PVE::Tools qw(run_command);
> use PVE::Storage::Plugin;
> use PVE::RPCEnvironment;
> use PVE::QemuServer;
> use PVE::LXC;
these two are not allow in PVE::Storage code - they would cause a
circular dependency. I'll propose an alternative solution further below.
> use LWP::UserAgent;
> use HTTP::Status;
maybe it would make sense to bundle all the places where we need an
HTTP(s) client, unify them and move the actual client setup etc to
pve-common? (this is not for you to implement or care about though, just
a reminder to pve-devel :P)
> use JSON;
> use Data::Dumper;
not used anymore
>
> use base qw(PVE::Storage::Plugin);
>
> my $api = '/api/v1.0';
> my $timeout = 60; # seconds
maybe name this more explicitly? it's only the timeout for the REST API
requests after all. also note that synchronous requests made over the
PVE API have a timeout of 30 seconds, so you probably want to lower this
anyway if you are not in a worker..
> 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
this seems kind of arbitrary? if you really need such limit, you also
need to implement pagination or something like it, otherwise you are in
trouble.
> my $version;
> my $fullversion;
AFAICT the latter is only used internally in freenas_get_version..
> my $target_prefix = 'iqn.2005-10.org.freenas.ctl';
likewise, only used internally in freenas_get_target_name. IMHO this
should be configurable though - it is on the FreeNAS side IIRC?
>
> sub freenas_request {
> my ($scfg, $request, $section, $data, $valid_code) = @_;
if data is passed in, it is always first run through encode_json on the
caller side. why not do the encoding here, so that if you need to
change/adapt something, you only have one place to change it? also might
make it possible to inline some $data hashes in calling places..
$valid_code is not used anywhere
> my $ua = LWP::UserAgent->new;
> $ua->agent("ProxmoxUA/0.1");
> $ua->ssl_opts( verify_hostname => 0 );
this should not be the default and/or it should be configurable.
> $ua->timeout($timeout);
> push @{ $ua->requests_redirectable }, 'POST';
> push @{ $ua->requests_redirectable }, 'PUT';
> push @{ $ua->requests_redirectable }, 'DELETE';
> my $req;
>
> my $url = "https://$scfg->{portal}$api/$section";
>
> if ($request eq 'GET') {
> $req = HTTP::Request->new(GET => $url);
> } elsif ($request eq 'POST') {
> $req = HTTP::Request->new(POST => $url);
> $req->content($data);
> } elsif ($request eq 'PUT') {
> $req = HTTP::Request->new(PUT => $url);
> $req->content($data);
> } elsif ($request eq 'DELETE') {
> $req = HTTP::Request->new(DELETE => $url);
> } else {
> die "$request: Unknown request";
> }
>
> $req->content_type('application/json');
> $req->authorization_basic($scfg->{username}, $scfg->{password});
> my $res = $ua->request($req);
>
> return $res->code unless $res->is_success;
>
> return $res->content;
this overloading seems potentially error prone. why not return a pair
($code, $content) if successful, and die with $code (or
HTTP::Status::status_message($code)) if not? that's what you do on every
call at the call site anyway, except for once in freenas_create_target.
furthermore, you always follow the request with decode_json unless it is
a delete operation (which probably returns an empty response?), so why
not pull that into freenas_request as well?
> }
>
> sub freenas_get_version {
> 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};
> if ($fullversion =~ /^\w+-(\d+)\.(\d*)\.(\d*)/) {
> my $minor = $2;
> my $micro = $3;
>
> if ($minor) {
> $minor = "0$minor" unless $minor > 9;
> } else {
> $minor = '00';
> }
>
> if ($micro) {
> $micro = "0$micro" unless $micro > 9;
> } else {
> $micro = '00';
> }
>
> $version = "$1$minor$micro";
> } else {
> $version = '90200';
is there a reason for this strange default? does FreeNAS 9 not return a
version?
also, wouldn't it make sense to check for a supported version here, and
bail out if not?
> }
> }
>
> sub freenas_list_zvol {
> my ($scfg) = @_;
>
> freenas_get_version($scfg) unless $version;
why - you are not doing anything with the result?
>
> 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");
can't this be limited to a certain pool?
> die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
> my $snapshots = decode_json($response);
>
> my $list = ();
> my $hide = {};
> my $vmid;
> my $parent;
> foreach my $zvol (@$zvols) {
> next unless $zvol->{name} =~ /^(base|vm)-(\d+)-disk-\d+$/;
> $vmid = $2;
> $parent = undef;
> foreach my $snap (@$snapshots) {
> next unless $snap->{name} eq "__base__$vmid";
this seems a very strange way to handle this.. is it not possible to get
the origin of a dataset/zvol using the API? creating an artificial extra
snapshot per VM for this seems very workaround-ish
if we need to keep the clone workaround, we should IMHO do something
like:
vm-XXX-disk-1
vm-XXX-disk-1 at __baseorigin__
base-XXX-disk-1 (clone of vm-XXX-disk-1 at __baseorigin__)
base-XXX-disk-1 at __base__
and then if we want to do a linked clone base-XXX-disk-1/vm-YYY-disk-1
we just need to clone vm-YYY-disk-1 based on base-XXX-disk-1 at __base__
> $parent = $snap->{filesystem} =~ /^$scfg->{pool}\/(.+)$/ ? $1 : undef;
> }
> $list->{$scfg->{pool}}->{$zvol->{name}} = {
> name => $zvol->{name},
> size => $zvol->{volsize},
> parent => $parent,
> vmid => $vmid,
> format => 'raw',
> };
> if ($zvol->{name} =~ /^base-(.*)/) {
> $hide->{"vm-$1"} = 1;
> }
shouldn't you hide those vm-XXX-foo zvols for which a base-XXX-foo clone
exists? instead of hiding the clones?
> }
>
> delete @{$list->{$scfg->{pool}}}{keys %$hide};
>
> return $list;
> }
>
> sub freenas_no_more_extents {
> my ($scfg, $target) = @_;
>
> my $response = freenas_request($scfg, 'GET', "services/iscsi/targettoextent?limit=$limit");
no way to limit by target on the server side?
> die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
> my $extents = decode_json($response);
> foreach my $extent (@$extents) {
> return 0 if $extent->{iscsi_target} == $target;
> }
>
> return 1;
> }
>
> sub freenas_get_target {
> my ($scfg, $vmid) = @_;
> my $target = undef;
>
> my $response = freenas_request($scfg, 'GET', "services/iscsi/target?limit=$limit");
again, no way to limit by target(-name) on the server side or get one
directly?
> die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
> my $targets = decode_json($response);
> foreach my $t (@$targets) {
> if ($t->{iscsi_target_name} eq "vm-$vmid") {
> $target = $t->{id};
> last;
> }
> }
>
> return $target;
> }
>
> sub freenas_create_target {
> my ($scfg, $vmid, $valid_code) = @_;
this is only called in one place, and $valid_code is always 409 there..
does this really deserve to be a parameter?
> my $data;
>
> freenas_get_version($scfg) unless $version;
>
> if ($version < 110000) {
> $data = {
> iscsi_target_alias => "vm-$vmid",
> iscsi_target_name => "vm-$vmid",
> };
> } elsif ($version < 110100) {
> $data = {
> iscsi_target_alias => "vm-$vmid",
> iscsi_target_name => "vm-$vmid",
> };
both branches do the same thing..
> } else {
> die "FreeNAS-$version: Unsupported!";
shouldn't this be in freenas_get_version?
> }
> 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);
see above, IMHO "die foo if $response != 409; return bar;"
> }
> my $target = decode_json($response);
>
> die "Creating target for 'vm-$vmid' failed" unless $target->{id};
freenas_get_target does not have this check - is there a reason?
>
> return $target->{id};
> }
>
> sub freenas_delete_target {
> my ($scfg, $target) = @_;
>
> my $response = freenas_request($scfg, 'DELETE', "services/iscsi/target/$target");
> die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
> }
>
> sub freenas_get_target_name {
> my ($scfg, $volname) = @_;
> my $name = undef;
>
> if ($volname =~ /^(vm|base)-(\d+)-/) {
> $name = "vm-$2";
> return "$target_prefix\:$name";
> }
>
> return undef;
> }
>
> sub freenas_create_target_group {
> my ($scfg, $target, $valid_code) = @_;
same as freenas_create_target - only used once, and $valid_code is set
to 409.
> my $data;
>
> freenas_get_version($scfg) unless $version;
>
> # 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;
> }
if is redundant, this is always called
>
> if ($version < 110000) {
> $data = {
> iscsi_target => $target,
> iscsi_target_authgroup => $scfg->{auth_group} ? $scfg->{auth_group} : undef,
> iscsi_target_portalgroup => $scfg->{portal_group},
> iscsi_target_initiatorgroup => $scfg->{initiator_group},
> iscsi_target_authtype => $scfg->{auth_type} ? $scfg->{auth_type} : 'None',
> iscsi_target_initialdigest => "Auto",
> };
> } elsif ($version < 110100) {
> $data = {
> iscsi_target => $target,
> iscsi_target_authgroup => $scfg->{auth_group} ? $scfg->{auth_group} : undef,
> iscsi_target_portalgroup => $scfg->{portal_group},
> iscsi_target_initiatorgroup => $scfg->{initiator_group},
> iscsi_target_authtype => $scfg->{auth_type} ? $scfg->{auth_type} : 'None',
> iscsi_target_initialdigest => "Auto",
> };
> } else {
> die "FreeNAS-$version: Unsupported!";
> }
see comments in freenas_create_target
>
> 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;
> }
condition is never true, as valid_code is always 409?
> die HTTP::Status::status_message($response);
> }
> 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 ($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}) {
is there now way to limit by any of this on the server side?
> $targetgroup = $tgroup->{id};
> last;
> }
> }
>
> return $targetgroup;
> }
>
> sub freenas_create_extent {
> my ($scfg, $zvol) = @_;
> my $data;
>
> freenas_get_version($scfg) unless $version;
>
> if ($version < 110000) {
> $data = {
> iscsi_target_extent_type => 'Disk',
> iscsi_target_extent_name => $zvol,
> iscsi_target_extent_disk => "zvol/$scfg->{pool}/$zvol",
> };
> } elsif ($version < 110100) {
> $data = {
> iscsi_target_extent_type => 'Disk',
> iscsi_target_extent_name => $zvol,
> iscsi_target_extent_disk => "zvol/$scfg->{pool}/$zvol",
> };
> } else {
> die "FreeNAS-$version: Unsupported!";
> }
same as the previous two such if / elsif / else branches.. why?
>
> 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};
no such check in freenas_get_extent, why?
>
> return $extent->{id};
> }
>
> sub freenas_delete_extent {
> my ($scfg, $extent) = @_;
>
> my $response = freenas_request($scfg, 'DELETE', "services/iscsi/extent/$extent");
> die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
> }
>
> sub freenas_get_extent {
> 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);
> foreach my $ext (@$extents) {
> if ($ext->{iscsi_target_extent_path} =~ /$scfg->{pool}\/$volname$/) {
no way to limit for this on the server side?
> $extent = $ext->{id};
> last;
> }
> }
>
> return $extent;
> }
>
> sub freenas_create_target_to_exent {
> my ($scfg, $target, $extent, $lunid) = @_;
> my $data;
>
> freenas_get_version($scfg) unless $version;
>
> if ($version < 110000) {
> $data = {
> iscsi_target => $target,
> iscsi_extent => $extent,
> iscsi_lunid => $lunid,
> };
> } elsif ($version < 110100) {
> $data = {
> iscsi_target => $target,
> iscsi_extent => $extent,
> iscsi_lunid => $lunid,
> };
> } else {
> die "FreeNAS-$version: Unsupported!";
> }
again
>
> 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};
and again, no such check in freenas_get_target_to_extent
>
> return $tg2extent->{id};
> }
>
> sub freenas_delete_target_to_exent {
> my ($scfg, $tg2exent) = @_;
>
> my $response = freenas_request($scfg, 'DELETE', "services/iscsi/targettoextent/$tg2exent");
> die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
> }
>
> sub freenas_get_target_to_exent {
> 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);
> foreach my $t2ext (@$t2extents) {
> if ($t2ext->{iscsi_target} == $target && $t2ext->{iscsi_extent} == $extent) {
server side limits?
> $t2extent = $t2ext->{id};
> last;
> }
> }
>
> return $t2extent;
> }
>
> sub freenas_find_free_diskname {
> my ($storeid, $scfg, $vmid, $format) = @_;
>
> my $name = undef;
> 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+)/){
why "base-" here if you only return vm-... down below?
> $disk_ids->{$2} = 1;
> }
> }
the whole foreach could be a map operation, since you already extracted
the vmid and saved it in freenas_list_zvol
>
> for (my $i = 1; $i < $max_luns + 1; $i++) {
> if (!$disk_ids->{$i}) {
> return "vm-$vmid-disk-$i";
> }
> }
this check for the max_luns limit does not account for volumes with
custom names (which are allowed in PVE, and also here in alloc_image).
>
> die "Maximum number of LUNs($max_luns) for this VM $vmid in storage '$storeid' is reached.";
> }
>
> sub freenas_get_lun_number {
> my ($scfg, $volname) = @_;
> my $lunid = undef;
>
> if ($volname =~ /^(vm|base)-\d+-disk-(\d+)$/) {
> $lunid = $2 - 1;
> } elsif ($volname =~ /^vm-(\d+)-state/) {
> # Find id for temporary LUN
> 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);
>
> foreach my $t2extent (@$t2extents) {
> next unless $t2extent->{iscsi_target} == $target &&
> $t2extent->{iscsi_lunid} + 1 > $max_luns &&
> $t2extent->{iscsi_lunid} < $max_luns + $active_snaps;
given that $max_luns is 20, and $active_snaps is 4, what is this
supposed to achieve? this seems to always "next", so
> 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);
> # 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;
$id is still $max_luns here, so this can never trigger. also, hard coded
value for active_snaps.
> $lunid = $id;
and this just sets $lunid to $max_luns?
> } elsif ($volname =~ /^(vm|base)-\d+-disk-\d+\@vzdump$/) {
> # Required to be able to exposed read-only LUNs for snapshot backup CT
why limit this to @vzdump? for example, your plugin could support full
cloning from snapshots as well, couldn't it?
> $lunid = $max_luns + $active_snaps;
> }
what about custom volume names? vm-VMID-foo is allowed!
>
> return $lunid;
> }
>
> sub freenas_create_lun {
> 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;
you are already in an eval, and your methods should return appropriate
error messages. so this wrapping should not be needed. if you want, you
can prepend "create_lun: " in your error handling below though.
> $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;
this seems like the wrong place to handle this..
> $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;
s/extend/extent
> };
> if ($@) {
when handling an error, you usually want to guard against masking your
actuall error if your clean up can die. so you should wrap those
_delete_ calls in an eval, and probably "warn" if they fail.
> my $err = $@;
> if ($tg2exent) {
> freenas_delete_target_to_exent($scfg, $tg2exent);
> }
> if ($extent) {
> freenas_delete_extent($scfg, $extent);
> }
> if ($target && freenas_no_more_extents($scfg, $target)) {
freenas_no_more_extents can die and mask your actual error message as
well.
> if ($tg) {
> freenas_delete_target_group($scfg, $tg);
> }
> freenas_delete_target($scfg, $target);
> }
> die $err;
here is where you could instead
die "create_lun: $err";
> }
> }
>
> sub freenas_create_zvol {
> 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);
>
> die "$volname: Failed creating volume" unless $zvol && $zvol->{name};
>
> return $zvol->{name};
> }
>
> sub freenas_delete_zvol {
> my ($scfg, $volname) = @_;
>
> my $response = freenas_request($scfg, 'DELETE', "storage/volume/$scfg->{pool}/zvols/$volname");
> die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
> }
>
> sub os_request {
isn't this more an "iscsiadm_request" though? all but one call use
iscsiadm, and the other one should not use run_command anyway..
> my ($cmd, $noerr, $timeout) = @_;
not that all the $cmd passed in which are not static need to be split to
ensure that the parameters are quoted correctly.
>
> $timeout = PVE::RPCEnvironment::is_worker() ? 60*60 : 5 if !$timeout;
> $noerr = 0 if !$noerr;
this is not needed - it's handled by run_command anyway.
>
> my $text = '';
>
> my $output = sub {
> my $line = shift;
> $text .= "$line\n";
> };
>
> my $exit_code = run_command($cmd, noerr => $noerr, errfunc => $output, outfunc => $output, timeout => $timeout);
>
> return wantarray ? ($exit_code, $text) : $exit_code;
maybe it would make sense to have a list of noerr exit codes here
instead handling this in the callers? e.g. build_lun_list or get_sid..
> }
>
> sub bail_out {
> my ($class, $storeid, $scfg, $volname, $err) = @_;
>
> $class->free_image($storeid, $scfg, $volname);
> die $err;
> }
not used anyhwere?
>
> sub disk_by_path {
> my ($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 ($scfg, $sid, $lun) = @_;
>
> my $luns = {};
> my $text = '';
> my $exit = 0;
>
> eval {
> ($exit, $text) = os_request("iscsiadm -m session -r $sid -P3", 1, 60);
['iscsiadm', '-m', 'session', '-r', $sid, '-P3']
> };
> if ($@) {
> # An exist code of 22 means no active session otherwise an error
> if ($exit != 22) {
> die "$@";
> }
> }
see comment in os_request
> if ($text =~ /.*Host Number:\s*(\d+)\s+State:\s+running(.*)/s) {
> my $host = $1;
> my $found = 0;
> for (split /^/, $2) {
> if ($_ =~ /Channel\s+(\d+)\s+Id\s+(\d+)\s+Lun:\s+(\d+)/) {
> if (defined $lun && $lun == $3) {
> $luns = {};
> $found = 1;
> }
> $luns->{$3} = "$host:".int($1).":$2:$3";
> last if $found;
> }
> }
> }
wouldn't it be more elegant to split lines first, then check the first
one and iterate over the rest?
>
> return $luns;
> }
>
> sub get_sid {
> my ($scfg, $volname) = @_;
> my $sid = -1;
> my $text = '';
> my $exit = 0;
>
> my $target = freenas_get_target_name($scfg, $volname);
>
> eval {
> ($exit, $text) = os_request("iscsiadm -m node -T $target -p $scfg->{portal} -s", 1, 60);
['foo', 'bar', ..]
> };
> if ($@) {
> # An exist code of 21 or 22 means no active session otherwise an error
> if ($exit != 21 || $exit != 22) {
> die "$@";
> }
> }
again
> if ($text =~ /.*\[sid\:\s*(\d+),\s*.*/) {
> $sid = $1;
> }
>
> return $sid;
> }
>
> sub create_session {
> my ($scfg, $volname) = @_;
> my $sid = -1;
> my $exit = undef;
>
> my $target = freenas_get_target_name($scfg, $volname);
>
> eval {
> $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);
> };
> }
> };
> 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);
> };
is this duplicated intentionally?
> } else {
> die $@;
> }
> }
> eval {
> $sid = get_sid($scfg, $volname);
> };
> die "$@" if $@;
> die "Could not create session" if $sid < 0;
>
> return $sid;
> }
>
> sub delete_session {
> my ($scfg, $sid) = @_;
>
> eval {
> os_request("iscsiadm -m session -r $sid --logout", 0, 60);
again [...]
> };
error messages are lost here?
> }
>
> sub remove_local_lun {
> my ($id) = @_;
>
> os_request("echo 1 > /sys/bus/scsi/devices/$id/delete", 0, 60);
write to that path instead of abusing a shell?
> }
>
> sub deactivate_luns {
> # $luns contains a hash of luns to keep
> my ($scfg, $volname, $luns) = @_;
>
> $luns = {} if !$luns;
> my $sid;
> my $list = {};
>
> eval {
> $sid = get_sid($scfg, $volname);
> };
> die "$@" if $@;
why wrap this in an eval if you die anyway?
>
> eval {
> $list = build_lun_list($scfg, $sid);
>
> foreach my $key (keys %$list) {
> next if exists($luns->{$key});
no need for "exists" here, is there?
> remove_local_lun($list->{$key});
shouldn't this be guarded by an eval? or do you want to die on the first
LUN that you fail to remove?
> }
> };
> die "$@" if $@;
same as above
> }
>
> sub get_active_luns {
> my ($class, $storeid, $scfg, $volname) = @_;
>
> my $sid = 0;
> my $luns = {};
>
> eval {
> $sid = get_sid($scfg, $volname);
> };
> die "$@" if $@;
eval {}; die; - again?
> if ($sid < 0) {
> # We have no active sessions so make one
> eval {
> $sid = create_session($scfg, $volname);
> };
> die "$@" if $@;
again
> # Since no session existed prior to this call deactivate all LUN's found
> deactivate_luns($scfg, $volname);
> } else {
> eval {
> $luns = build_lun_list($scfg, $sid);
> };
> die "$@" if $@;
again
> }
>
> return $luns;
> }
>
> sub rescan_session {
> 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);
[...] again
> deactivate_luns($scfg, $volname, $active_luns);
this reads really unfortunate, because at first glance you'd expect it
to deactivate all the active luns. maybe rename $active_luns to
$luns_to_keep or similar?
> delete_session($scfg, $sid) if !%$active_luns;
> };
> die "$@" if $@;
eval {}; die; - again?
> }
>
> sub freenas_get_latest_snapshot {
> my ($class, $scfg, $volname) = @_;
>
> my $vname = ($class->parse_volname($volname))[1];
inconsistent, please use (undef, $vname) instead, like in
volume_size_info below
>
> # abort rollback if snapshot is not the latest
> my $response = freenas_request($scfg, 'GET', "storage/snapshot");
why no limit here, but in freenas_list_zvol there is one? ;) also,
filtering by $volname on the server side would be great if possible.
> die HTTP::Status::status_message($response) if $response =~ /^\d+$/;
> my $snapshots = decode_json($response);
>
> my $recentsnap;
> foreach my $snapshot (@$snapshots) {
> next unless $snapshot->{filesystem} =~ /$scfg->{pool}\/$vname/ && $snapshot->{mostrecent};
> $recentsnap = $snapshot->{name};
> last;
> }
>
> 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",
> },
like previously said, it would probably be better to store this in
/etc/pve/priv and not expose it over the API.
> 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 used anywhere?
> 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 {
> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>
> 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);
>
> return $zvol->{volsize} if $zvol && $zvol->{volsize};
>
> die "Could not get zfs volume size\n";
> }
>
> sub parse_volname {
it would be great if some of the REs in the helper methods could be
replaced with calls to parse_volname. having such things encoded in one
place makes them easier to check and change.
> my ($class, $volname) = @_;
>
> if ($volname =~ m/^(((base)-(\d+)-\S+)\/)?((base|vm)-(\d+)-\S+)$/) {
> my $format = 'raw';
> my $isBase = ($6 eq 'base');
> return ('images', $5, $7, $2, $4, $isBase, $format);
> }
>
> die "unable to parse freenas volume name '$volname'\n";
> }
>
> sub status {
> my ($class, $storeid, $scfg, $cache) = @_;
> my $total = 0;
> my $free = 0;
> my $used = 0;
> 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 $children = $vol->{children};
> if (@$children) {
> $used = $children->[0]{used};
> $total = $children->[0]{avail};
this might warrant an explanatory comment - is this a workaround for
some API quirk?
> } else {
> $used = $vol->{used};
> $total = $vol->{avail};
> }
> $free = $total - $used;
> $active = 1;
> };
> warn $@ if $@;
>
> return ($total, $free, $used, $active);
> }
>
> sub list_images {
> my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
>
> $cache->{freenas} = freenas_list_zvol($scfg) if !$cache->{freenas};
> my $zfspool = $scfg->{pool};
> my $res = [];
>
> if (my $dat = $cache->{freenas}->{$zfspool}) {
>
> foreach my $image (keys %$dat) {
>
> my $info = $dat->{$image};
> my $volname = $info->{name};
> my $parent = $info->{parent};
> my $owner = $info->{vmid};
>
> if ($parent) {
> $info->{volid} = "$storeid:$parent/$volname";
> } else {
> $info->{volid} = "$storeid:$volname";
> }
>
> if ($vollist) {
> my $found = grep { $_ eq $info->{volid} } @$vollist;
> next if !$found;
> } else {
> next if defined ($vmid) && ($owner ne $vmid);
> }
I wonder if those two are really exclusive?
> push @$res, $info;
> }
> }
>
> return $res;
> }
>
> sub path {
> my ($class, $scfg, $volname, $storeid, $snapname) = @_;
>
> my ($vtype, $vname, $vmid) = $class->parse_volname($volname);
>
> $vname = "$vname\@$snapname" if $snapname;
>
> 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);
why not declare them down below?
> my $snap = '__base__';
see comment in freenas_list_zvol
>
> my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> $class->parse_volname($volname);
>
> die "create_base not possible with base image\n" if $isBase;
>
> my $newname = $name;
> $newname =~ s/^vm-/base-/;
IMHO you need to check whether that one is still free? not sure whether
you want to die in that case or select a different auto-assigned name..
>
> $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);
do those two really not need error handling if they fail?
> my $sid = get_sid($scfg, $name);
> if ($sid >= 0) {
> my $lid = build_lun_list($scfg, $sid, $lun);
> if ($lid && $lid->{$lun}) {
> remove_local_lun($lid->{$lun});
> }
> }
>
> eval {
> # FreeNAS API does not support renaming a zvol so create a snapshot
> # and make a clone of the snapshot instead
> $class->volume_snapshot($scfg, $storeid, $name, $snap);
>
> 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_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;
this seems wrong to me - if the snapshot or clone or setup of clone
fails, the error handling is recreating the old extent and tg2extent and
pretending the create_base was successful? or am I missing something
here?
> }
>
> return $newname;
> }
>
> sub clone_image {
> my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_;
>
> $snap ||= "__base__$vmid";
see comment in freenas_list_zvol about base / clone
>
> my ($vtype, $basename, $basevmid, undef, undef, $isBase, $format) =
> $class->parse_volname($volname);
>
> die "clone_image only works on base images" if !$isBase;
>
> my $run = PVE::QemuServer::check_running($basevmid);
> if (!$run) {
> $run = PVE::LXC::check_running($basevmid);
> }
this is not allowed here (see comment at the very top). but since
cloning only works with base images here, and those can not be part of a
running guest, it should not be needed?
>
> 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+$/;
>
> $name = "$basename/$name";
> # get ZFS dataset name from PVE volname
> my (undef, $clonedname) = $class->parse_volname($name);
>
> freenas_create_lun($scfg, $vmid, $clonedname);
>
> my $res = $class->deactivate_volume($storeid, $scfg, $basename) unless $run;
see above, this should be unconditional
> warn "Could not deactivate volume '$basename'" unless $res;
>
> return $name;
> }
>
> sub alloc_image {
> my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
> die "unsupported format '$fmt'" if $fmt ne 'raw';
>
> die "illegal name '$name' - sould be 'vm-$vmid-*'\n"
> if $name && $name !~ m/^vm-$vmid-/;
this is not handled correctly in all other places - you sometimes assume
(base|vm)-XXX-disk-\d+ is the only possible correct volname.
>
> my $volname = $name;
why introduce a new variable here?
>
> $volname = freenas_find_free_diskname($storeid, $scfg, $vmid, $fmt) if !$volname;
>
> # Size is in KB but Freenas wants in bytes
> $size *= 1024;
> my $zvol = freenas_create_zvol($scfg, $volname, $size);
>
> eval {
> freenas_create_lun($scfg, $vmid, $zvol) if $zvol;
freenas_create_zvol would have died if $zvol were undefined, the if is
redundant.
> };
> if ($@) {
> my $err = $@;
> eval {
> freenas_delete_zvol($scfg, $volname);
> };
> $err .= "\n$@" if $@;
> die $err;
why not:
warn "cleanup failed: $@\n" if $@;
die "$err\n";
> }
>
> return $volname;
> }
>
> sub free_image {
> my ($class, $storeid, $scfg, $volname, $isBase) = @_;
>
> 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;
why are these guarded by an eval? if they fail, there is nothing to free
and nothing to do in error-handling anyway?
>
> 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)) {
> if ($target_group) {
> freenas_delete_target_group($scfg, $target_group);
> }
> freenas_delete_target($scfg, $target);
> }
> 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);
> }
> };
> if ($@) {
> my $err = $@;
> freenas_create_lun($scfg, $vmid, $name) unless $isBase;
not sure about this - I guess the intention is to return to the original
state?
> die $err;
> }
>
> return undef;
> }
>
> sub volume_resize {
> my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
>
> my ($vtype, $name, $vmid) = $class->parse_volname($volname);
>
> my $run = PVE::QemuServer::check_running($vmid);
> if (!$run) {
> $run = PVE::LXC::check_running($vmid);
> }
not allowed here - see comment on the very top.
alternative solutions:
- expand storage API here to split $running into two parameters, modify
PVE::LXC and PVE::QemuServer to pass $running and the new parameter
accordingly, adapt other plugins to handle $running && $new_parameter
like they used to handle $running (if they did), adapt your plugin to
die if $running to forbid online resizing for now.
- special case your plugin in PVE::LXC to handle running containers
there (Qemu does pass $running correctly).
>
> 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;
no HTML in backend code please (except for those parts which are
generating the interface HTML of course ;))
>
> 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 $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);
> #### Required because of a bug in FreeNAS: https://bugs.freenas.org/issues/24432
> rescan_session($class, $storeid, $scfg, $name, $lunid);
> };
> die "$name: Resize with $size failed. ($@)\n" if $@;
isn't that wrong though - the volume was resized, it's just that the new
size is not exported yet and thus not visible?
> }
>
> return int($vol->{volsize}/1024);
> }
>
> sub volume_snapshot {
> my ($class, $scfg, $storeid, $volname, $snap) = @_;
>
> my $vname = ($class->parse_volname($volname))[1];
>
> my $data = {
> 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+$/;
> }
>
> sub volume_snapshot_delete {
> my ($class, $scfg, $storeid, $volname, $snap, $running) = @_;
>
> my (undef, $vname, $vmid) = $class->parse_volname($volname);
>
> 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);
> $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+$/;
> }
>
> 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);
these two could fail, not sure if they need special handling though?
>
> my $data = {
> force => bless( do{\(my $o = 0)}, 'JSON::XS::Boolean' ),
you mean JSON::false ?
> };
> 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);
again, these three could fail and no handling here?
> }
>
> sub volume_rollback_is_possible {
> my ($class, $scfg, $storeid, $volname, $snap) = @_;
>
> my (undef, $name) = $class->parse_volname($volname);
>
> my $recentsnap = $class->freenas_get_latest_snapshot($scfg, $name);
> if ($snap ne $recentsnap) {
> die "can't rollback, more recent snapshots exist";
> }
>
> return 1;
> }
>
> sub volume_snapshot_list {
> my ($class, $scfg, $storeid, $volname, $prefix) = @_;
> # return an empty array if dataset does not exist.
> die "Volume_snapshot_list is not implemented for FreeNAS.\n";
> }
>
> sub volume_has_feature {
> my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running) = @_;
>
> my $features = {
> snapshot => { current => 1, snap => 1},
> clone => { base => 1},
> template => { current => 1},
> copy => { base => 1, current => 1},
> };
clone and copy would in theory also be possible for snapshots, unless I
am overlooking something? you'd need to adapt the plugin in some places
though. the other ZFS plugins don't allow it either at the moment, not
sure why ;)
>
> my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> $class->parse_volname($volname);
>
> my $key = undef;
>
> if ($snapname) {
> $key = 'snap';
> } else {
> $key = $isBase ? 'base' : 'current';
> }
>
> return 1 if $features->{$feature}->{$key};
>
> return undef;
> }
>
> sub activate_storage {
> my ($class, $storeid, $scfg, $cache) = @_;
>
> return 1;
> }
>
> sub deactivate_storage {
> my ($class, $storeid, $scfg, $cache) = @_;
>
> return 1;
> }
>
> # Procedure for activating a LUN:
> #
> # if session does not exist
> # login to target
> # deactivate all luns in session
> # get list of active luns
> # get lun number to activate
> # make list of our luns (active + new lun)
> # rescan session
> # deactivate all luns except our luns
> sub activate_volume {
> my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> my $lun;
>
> my (undef, $name, $vmid) = $class->parse_volname($volname);
>
> my $active_luns = 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";
where does this hard-coded 0:0:0: come from? wouldn't it be better to
call get_active_luns again, or to let freenas_create_lun return some of
the info it has anyway?
> };
> if ($@) {
> die "$@ - unable to activate snapshot from remote FreeNAS storage";
> }
> }
>
> $lun = freenas_get_lun_number($scfg, $name);
> $active_luns->{$lun} = "0:0:0:$lun";
same as above
>
> 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);
[...] again
> sleep 1;
nope! this might be fine for debugging, but you need to use udevadm
to trigger and settle if you want to wait for device nodes to appear.
> # Remove all LUN's from session which is not currently active
> deactivate_luns($scfg, $name, $active_luns);
same as in rescan_session, I'd suggest renaming $active_luns (but at
least here there is a comment that explains it a bit).
> };
> die "$@" if $@;
why? no error handling, no need for eval {}.
>
> return 1;
> }
>
> # Procedure for deactivating a LUN:
> #
> # if session exists
> # get lun number to deactivate
> # deactivate lun
> sub deactivate_volume {
> my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>
> my $name = ($class->parse_volname($volname))[1];
again, (undef, $name)
>
> my $active_luns = get_active_luns($class, $storeid, $scfg, $name);
>
> my $lun = freenas_get_lun_number($scfg, $name);
> delete $active_luns->{$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;
same comment about $active_luns
> };
> die $@ if $@;
and again eval {}; die; does not make sense
>
> return 1;
> }
>
> 1;
More information about the pve-devel
mailing list