comments inline On Fri, 29 Jun 2018 10:00:53 +0200 Udo Rader <udo.ra...@bestsolution.at> wrote:
> --- > PVE/Storage/LunCmd/LIO.pm | 398 > ++++++++++++++++++++++++++++++++++++++ 1 file changed, 398 > insertions(+) create mode 100644 PVE/Storage/LunCmd/LIO.pm > > diff --git a/PVE/Storage/LunCmd/LIO.pm b/PVE/Storage/LunCmd/LIO.pm > new file mode 100644 > index 0000000..2d8c2ee > --- /dev/null > +++ b/PVE/Storage/LunCmd/LIO.pm > @@ -0,0 +1,398 @@ > +package PVE::Storage::LunCmd::LIO; > + > +# lightly based on code from Iet.pm > +# > +# additional changes: > +# ----------------------------------------------------------------- > +# Copyright (c) 2018 BestSolution.at EDV Systemhaus GmbH > +# All Rights Reserved. > +# > +# This software is released under the terms of the > +# > +# "GNU Affero General Public License" > +# > +# and may only be distributed and used under the terms of the > +# mentioned license. You should have received a copy of the license > +# along with this software product, if not you can download it from > +# https://www.gnu.org/licenses/agpl-3.0.en.html > +# > +# Author: udo.ra...@bestsolution.at > +# ----------------------------------------------------------------- > + > +use strict; > +use warnings; > +use PVE::Tools qw(run_command); > +use JSON; > + > +sub get_base; > + > +# targetcli constants > +# config file location differs from distro to distro > +my @CONFIG_FILES = ( > + '/etc/rtslib-fb-target/saveconfig.json', # Debian 9.x > et al > + '/etc/target/saveconfig.json' , # > ArchLinux, CentOS +); > +my $BACKSTORE = '/backstores/block'; > + > +my $SETTINGS = undef; > + > +my @ssh_opts = ('-o', 'BatchMode=yes'); > +my @ssh_cmd = ('/usr/bin/ssh', @ssh_opts); > +my @scp_cmd = ('/usr/bin/scp', @ssh_opts); > +my $id_rsa_path = '/etc/pve/priv/zfs'; > +my $targetcli = '/usr/bin/targetcli'; > + > +my $execute_command = sub { maybe call this $execute_remote_command (would help in getting an overview 6 months from now) > + my ($scfg, $exec, $timeout, $method, @params) = @_; > + > + my $msg = ''; > + my $err = undef; > + my $target; > + my $cmd; > + my $res = (); > + > + $timeout = 10 if !$timeout; > + > + my $output = sub { > + my $line = shift; > + $msg .= "$line\n"; > + }; > + > + my $errfunc = sub { > + my $line = shift; > + $err .= "$line"; > + }; > + > + if ($exec eq 'scp') { > + $target = 'root@[' . $scfg->{portal} . ']'; > + $cmd = [@scp_cmd, '-i', > "$id_rsa_path/$scfg->{portal}_id_rsa", '--', $method, > "$target:$params[0]"]; as far as I see you don't use scp for any command (and it doesn't get called from the outside as well) - can this be dropped? > + } else { > + $target = 'root@' . $scfg->{portal}; > + $cmd = [@ssh_cmd, '-i', > "$id_rsa_path/$scfg->{portal}_id_rsa", $target, '--', $method, > @params]; > + } > + > + eval { > + run_command($cmd, outfunc => $output, errfunc => $errfunc, > timeout => $timeout); > + }; > + if ($@) { > + $res = { > + result => 0, > + msg => $err, > + } > + } else { > + $res = { > + result => 1, > + msg => $msg, > + } > + } > + > + return $res; > +}; > + > +# fetch targetcli configuration from the portal > +my $read_config = sub { > + my ($scfg, $timeout) = @_; > + > + my $msg = ''; > + my $err = undef; > + my $luncmd = 'cat'; > + my $target; > + my $retry = 1; > + > + $timeout = 10 if !$timeout; > + > + my $output = sub { > + my $line = shift; > + $msg .= "$line\n"; > + }; > + > + my $errfunc = sub { > + my $line = shift; > + $err .= "$line"; > + }; > + > + $target = 'root@' . $scfg->{portal}; > + > + foreach my $oneFile (@CONFIG_FILES) { > + my $cmd = [@ssh_cmd, '-i', > "$id_rsa_path/$scfg->{portal}_id_rsa", $target, $luncmd, $oneFile]; > + eval { > + run_command($cmd, outfunc => $output, errfunc => > $errfunc, timeout => $timeout); > + }; > + if ($@) { > + die $err if ($err !~ /No such file or directory/); > + } > + $SETTINGS->{ConfigLocation} = $oneFile; $SETTINGS->{ConfigLocation} is set here but not used - maybe a left-over? > + return $msg if $msg ne ''; > + } > + > + die "No configuration found. Install targetcli on > $scfg->{portal}" if $msg eq ''; + As I just learned today - if you end the die call with a newline, it does not output filename and linenumber, which is IMO better, if the error message is not generic, but describes the problem. > + return $msg; > +}; > + > +my $get_config = sub { > + my ($scfg) = @_; > + my @conf = undef; > + > + my $config = $read_config->($scfg, undef); > + die "Missing config file" unless $config; > + > + return $config; > +}; > + > +# fetches and parses targetcli config from the portal > +my $parser = sub { > + my ($scfg) = @_; > + my $tpg = $scfg->{comstar_tg} || die "Target (Portal) Group not > set, aborting!"; > + my $tpg_tag; > + > + if ($tpg =~ /^tpg(\d+)$/) { > + $tpg_tag = $1; > + } else { > + die "Target Portal Group has invalid value, must contain > string 'tpg' and a suffix number, eg 'tpg17'"; > + } > + > + my $base = get_base; > + > + my $config = $get_config->($scfg); > + my $jsonconfig = JSON->new->utf8->decode($config); > + > + my $haveTarget = 0; > + foreach my $oneTarget (@{$jsonconfig->{targets}}) { > + # only interested in iSCSI targets > + if ($oneTarget->{fabric} eq 'iscsi' && $oneTarget->{wwn} eq > $scfg->{target}) { > + # find correct TPG > + foreach my $oneTpg (@{$oneTarget->{tpgs}}) { > + if ($oneTpg->{tag} == $tpg_tag) { > + $SETTINGS->{target} = $oneTpg; > + $haveTarget = 1; > + last; > + } > + } > + } > + } > + > + # seriously unhappy if the target server lacks iSCSI target > configuration ... > + if (!$haveTarget) { > + die "target portal group tpg$tpg_tag not found!"; > + } > +}; > + > +# removes the given lu_name from the local list of luns > +my $free_lu_name = sub { > + my ($lu_name) = @_; > + my $new; > + > + foreach my $lun (@{$SETTINGS->{target}->{luns}}) { > + if ($lun->{storage_object} ne "$BACKSTORE/$lu_name") { > + push @$new, $lun; > + } > + } > + > + $SETTINGS->{target}->{luns} = $new; > +}; > + > +# locally registers a new lun > +my $register_lun = sub { > + my ($scfg, $idx, $volname) = @_; > + > + my $conf = { > + index => $idx, > + storage_object => "$BACKSTORE/$volname", > + is_new => 1, > + }; > + push @{$SETTINGS->{target}->{luns}}, $conf; > + > + return $conf; > +}; > + > +# extracts the ZFS volume name from a device path > +my $extract_volname = sub { > + my ($scfg, $lunpath) = @_; > + my $volname = undef; > + > + my $base = get_base; > + if ($lunpath =~ /^$base\/$scfg->{pool}\/([\w\-]+)$/) { > + $volname = $1; > + } > + > + return $volname; > +}; > + > +# retrieves the LUN index for a particular object > +my $list_view = sub { > + my ($scfg, $timeout, $method, @params) = @_; > + my $lun = undef; > + > + my $object = $params[0]; > + my $volname = $extract_volname->($scfg, $params[0]); > + > + foreach my $lun (@{$SETTINGS->{target}->{luns}}) { > + if ($lun->{storage_object} eq "$BACKSTORE/$volname") { > + return $lun->{index}; > + } > + } > + > + return $lun; > +}; > + > +# determines, if the given object exists on the portal > +my $list_lun = sub { > + my ($scfg, $timeout, $method, @params) = @_; > + my $name = undef; > + > + my $object = $params[0]; > + my $volname = $extract_volname->($scfg, $params[0]); > + > + foreach my $lun (@{$SETTINGS->{target}->{luns}}) { > + if ($lun->{storage_object} eq "$BACKSTORE/$volname") { > + return $object; > + } > + } > + > + return $name; > +}; > + > +# adds a new LUN to the target > +my $create_lun = sub { > + my ($scfg, $timeout, $method, @params) = @_; > + > + if ($list_lun->($scfg, $timeout, $method, @params)) { > + die "$params[0]: LUN exists"; > + } > + > + my $device = $params[0]; > + my $volname = $extract_volname->($scfg, $device); > + my $tpg = $scfg->{comstar_tg} || die "Target (Portal) Group not > set, aborting!"; + > + # step 1: create backstore for device > + my @cliparams = ($BACKSTORE, 'create', "name=$volname", > "dev=$device" ); > + my $res = $execute_command->($scfg, 'ssh', $timeout, $targetcli, > @cliparams); > + do { > + die $res->{msg}; > + } unless $res->{result}; hmm - any reason for the do { die } unless block? (I would have used: die $res->{msg} if !$res->{result};) > + > + # step 2: register lun with target > + # > targetcli /iscsi/iqn.2018-04.at.bestsolution.somehost:target/tpg1/luns/ > create /backstores/block/foobar > + @cliparams = ("/iscsi/$scfg->{target}/$tpg/luns/", 'create', > "$BACKSTORE/$volname" ); > + $res = $execute_command->($scfg, 'ssh', $timeout, $targetcli, > @cliparams); > + do { > + die $res->{msg}; > + } unless $res->{result}; > + > + # targetcli responds with "Created LUN 99" > + # not calculating the index ourselves, because the index at the > portal might have > + # changed without our knowledge, so relying on the number that > targetcli returns > + my $lun_idx; > + if ($res->{msg} =~ /LUN (\d+)/) { > + $lun_idx = $1; > + } else { > + die "unable to determine new LUN index: $res->{msg}"; > + } > + > + $register_lun->($scfg, $lun_idx, $volname); > + > + # step 3: unfortunately, targetcli doesn't always save changes, > no matter > + # if auto_save_on_exit is true or not. So saving to be > safe ... > + $execute_command->($scfg, 'ssh', $timeout, $targetcli, > 'saveconfig'); + > + return $res->{msg}; > +}; > + > +my $delete_lun = sub { > + my ($scfg, $timeout, $method, @params) = @_; > + my $res = {msg => undef}; > + > + my $tpg = $scfg->{comstar_tg} || die "Target (Portal) Group not > set, aborting!"; + > + my $path = $params[0]; > + my $volname = $extract_volname->($scfg, $params[0]); > + > + foreach my $lun (@{$SETTINGS->{target}->{luns}}) { > + if ($lun->{storage_object} eq "$BACKSTORE/$volname") { > + # step 1: delete the lun > + my @cliparams = ("/iscsi/$scfg->{target}/$tpg/luns/", > 'delete', "lun$lun->{index}" ); > + my $res = $execute_command->($scfg, 'ssh', $timeout, > $targetcli, @cliparams); > + do { > + die $res->{msg}; > + } unless $res->{result}; > + > + # step 2: delete the backstore > + @cliparams = ($BACKSTORE, 'delete', $volname); > + $res = $execute_command->($scfg, 'ssh', $timeout, > $targetcli, @cliparams); > + do { > + die $res->{msg}; > + } unless $res->{result}; > + > + # step 3: save to be safe ... > + $execute_command->($scfg, 'ssh', $timeout, $targetcli, > 'saveconfig'); + > + # update interal cache > + $free_lu_name->($volname); > + > + last; > + } > + } > + > + return $res->{msg}; > +}; > + > +my $import_lun = sub { > + my ($scfg, $timeout, $method, @params) = @_; > + > + return $create_lun->($scfg, $timeout, $method, @params); > +}; > + > +# needed for example when the underlying ZFS volume has been resized > +my $modify_lun = sub { > + my ($scfg, $timeout, $method, @params) = @_; > + my $msg; > + > + if ($delete_lun->($scfg, $timeout, $method, @params)) { > + $msg = $create_lun->($scfg, $timeout, $method, @params); > + } > + > + return $msg; > +}; > + > +my $add_view = sub { > + my ($scfg, $timeout, $method, @params) = @_; > + > + return ''; > +}; > + > +my $get_lun_cmd_map = sub { > + my ($method) = @_; > + > + my $cmdmap = { > + create_lu => { cmd => $create_lun }, > + delete_lu => { cmd => $delete_lun }, > + import_lu => { cmd => $import_lun }, > + modify_lu => { cmd => $modify_lun }, > + add_view => { cmd => $add_view }, > + list_view => { cmd => $list_view }, > + list_lu => { cmd => $list_lun }, > + }; The double indirection here seems odd, why not use: my $cmdmap = { create_lu => $create_lun }, delete_lu => $delete_lun }, ... here > + > + die "unknown command '$method'" unless exists $cmdmap->{$method}; > + > + return $cmdmap->{$method}; > +}; > + > +sub run_lun_command { > + my ($scfg, $timeout, $method, @params) = @_; > + > + # fetch configuration from target if we haven't yet > + $parser->($scfg) unless $SETTINGS; > + my $cmdmap = $get_lun_cmd_map->($method); > + my $msg = $cmdmap->{cmd}->($scfg, $timeout, $method, @params); ...and call it with my $cmd = $get_lun_cmd_map->($method); my $msg = $cmd->($scfg, $timeout, $method, @params); here. Or even change the $get_lun_cmd_map sub for a hash and look it up directly. > + > + return $msg; > +} > + > +sub get_base { > + return '/dev'; > +} > + > +1; > + _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel