[pve-devel] [PATCH V3 storage 2/4] adding linux LIO support
Stoiko Ivanov
s.ivanov at proxmox.com
Wed Jul 11 20:07:59 CEST 2018
comments inline
On Fri, 29 Jun 2018 10:00:53 +0200
Udo Rader <udo.rader at 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.rader at 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;
> +
More information about the pve-devel
mailing list